Merge pull request #22728 from owncloud/external-share-testremote

use ocs discover endpoint to test remote

4 files changed, 65 insertions, 28 deletions

diff --git a/apps/files_sharing/lib/controllers/externalsharescontroller.php b/apps/files_sharing/lib/controllers/externalsharescontroller.php
index 7ac2ef781a8..be65a490393 100644
--- a/apps/files_sharing/lib/controllers/externalsharescontroller.php
+++ b/apps/files_sharing/lib/controllers/externalsharescontroller.php
@@ -96,9 +96,10 @@ class ExternalSharesController extends Controller {
 	 * Test whether the specified remote is accessible
 	 *
 	 * @param string $remote
+	 * @param bool $checkVersion
 	 * @return bool
 	 */
-	protected function testUrl($remote) {
+	protected function testUrl($remote, $checkVersion = false) {
 		try {
 			$client = $this->clientService->newClient();
 			$response = json_decode($client->get(
@@ -109,7 +110,11 @@ class ExternalSharesController extends Controller {
 				]
 			)->getBody());
 
-			return !empty($response->version) && version_compare($response->version, '7.0.0', '>=');
+			if ($checkVersion) {
+				return !empty($response->version) && version_compare($response->version, '7.0.0', '>=');
+			} else {
+				return is_object($response);
+			}
 		} catch (\Exception $e) {
 			return false;
 		}
@@ -124,9 +129,17 @@ class ExternalSharesController extends Controller {
 	 * @return DataResponse
 	 */
 	public function testRemote($remote) {
-		if ($this->testUrl('https://' . $remote . '/status.php')) {
+		if (
+			$this->testUrl('https://' . $remote . '/ocs-provider/') ||
+			$this->testUrl('https://' . $remote . '/ocs-provider/index.php') ||
+			$this->testUrl('https://' . $remote . '/status.php', true)
+		) {
 			return new DataResponse('https');
-		} elseif ($this->testUrl('http://' . $remote . '/status.php')) {
+		} elseif (
+			$this->testUrl('http://' . $remote . '/ocs-provider/') ||
+			$this->testUrl('http://' . $remote . '/ocs-provider/index.php') ||
+			$this->testUrl('http://' . $remote . '/status.php', true)
+		) {
 			return new DataResponse('http');
 		} else {
 			return new DataResponse(false);
diff --git a/apps/files_sharing/lib/external/scanner.php b/apps/files_sharing/lib/external/scanner.php
index 3335fc4707c..bfb9e817f09 100644
--- a/apps/files_sharing/lib/external/scanner.php
+++ b/apps/files_sharing/lib/external/scanner.php
@@ -90,6 +90,7 @@ class Scanner extends \OC\Files\Cache\Scanner {
 		}
 		if ($data['status'] === 'success') {
 			$this->addResult($data['data'], '');
+		} elseif ($data['status'] === 'unsupported') {
 		} else {
 			throw new \Exception(
 				'Error while scanning remote share: "' .
diff --git a/apps/files_sharing/lib/external/storage.php b/apps/files_sharing/lib/external/storage.php
index ba7fba654a9..ed391f331ad 100644
--- a/apps/files_sharing/lib/external/storage.php
+++ b/apps/files_sharing/lib/external/storage.php
@@ -27,6 +27,7 @@ namespace OCA\Files_Sharing\External;
 
 use OC\Files\Storage\DAV;
 use OC\ForbiddenException;
+use OCA\FederatedFileSharing\DiscoveryManager;
 use OCA\Files_Sharing\ISharedStorage;
 use OCP\Files\NotFoundException;
 use OCP\Files\StorageInvalidException;
@@ -136,6 +137,9 @@ class Storage extends DAV implements ISharedStorage {
 		if (!$storage) {
 			$storage = $this;
 		}
+		if(!$this->remoteIsOwnCloud()) {
+			return parent::getScanner($path, $storage);
+		}
 		if (!isset($this->scanner)) {
 			$this->scanner = new Scanner($storage);
 		}
@@ -218,20 +222,39 @@ class Storage extends DAV implements ISharedStorage {
 	}
 
 	/**
-	 * check if the configured remote is a valid ownCloud instance
+	 * check if the configured remote is a valid federated share provider
 	 *
 	 * @return bool
 	 */
 	protected function testRemote() {
 		try {
-			$result = file_get_contents($this->remote . '/status.php');
-			$data = json_decode($result);
-			return is_object($data) and !empty($data->version);
+			return $this->testRemoteUrl($this->remote . '/ocs-provider/index.php')
+				|| $this->testRemoteUrl($this->remote . '/ocs-provider/')
+				|| $this->testRemoteUrl($this->remote . '/status.php');
 		} catch (\Exception $e) {
 			return false;
 		}
 	}
 
+	private function testRemoteUrl($url) {
+		$result = file_get_contents($url);
+		$data = json_decode($result);
+		return (is_object($data) and !empty($data->version));
+	}
+
+	/**
+	 * Whether the remote is an ownCloud, used since some sharing features are not
+	 * standardized. Let's use this to detect whether to use it.
+	 *
+	 * @return bool
+	 */
+	private function remoteIsOwnCloud() {
+		if(defined('PHPUNIT_RUN') || !$this->testRemoteUrl($this->getRemote() . '/status.php')) {
+			return false;
+		}
+		return true;
+	}
+
 	/**
 	 * @return mixed
 	 * @throws ForbiddenException
@@ -242,6 +265,12 @@ class Storage extends DAV implements ISharedStorage {
 		$remote = $this->getRemote();
 		$token = $this->getToken();
 		$password = $this->getPassword();
+
+		// If remote is not an ownCloud do not try to get any share info
+		if(!$this->remoteIsOwnCloud()) {
+			return ['status' => 'unsupported'];
+		}
+
 		$url = rtrim($remote, '/') . '/index.php/apps/files_sharing/shareinfo?t=' . $token;
 
 		// TODO: DI
diff --git a/apps/files_sharing/tests/controller/externalsharecontroller.php b/apps/files_sharing/tests/controller/externalsharecontroller.php
index ab5f1c153f3..bd20bffb36c 100644
--- a/apps/files_sharing/tests/controller/externalsharecontroller.php
+++ b/apps/files_sharing/tests/controller/externalsharecontroller.php
@@ -93,23 +93,17 @@ class ExternalShareControllerTest extends \Test\TestCase {
 			->disableOriginalConstructor()->getMock();
 		$response = $this->getMockBuilder('\\OCP\\Http\\Client\\IResponse')
 			->disableOriginalConstructor()->getMock();
-		$client
-			->expects($this->once())
-			->method('get')
-			->with(
-				'https://owncloud.org/status.php',
-				[
-					'timeout' => 3,
-					'connect_timeout' => 3,
-				]
-			)->will($this->returnValue($response));
 		$response
-			->expects($this->once())
+			->expects($this->exactly(2))
 			->method('getBody')
-			->will($this->returnValue('{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'));
+			->will($this->onConsecutiveCalls('Certainly not a JSON string', '{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'));
+		$client
+			->expects($this->any())
+			->method('get')
+			->will($this->returnValue($response));
 
 		$this->clientService
-			->expects($this->once())
+			->expects($this->exactly(2))
 			->method('newClient')
 			->will($this->returnValue($client));
 
@@ -123,13 +117,13 @@ class ExternalShareControllerTest extends \Test\TestCase {
 			->disableOriginalConstructor()->getMock();
 		$client
 			->method('get')
-			->will($this->onConsecutiveCalls($response, $response));
+			->will($this->returnValue($response));
 		$response
-			->expects($this->exactly(2))
+			->expects($this->exactly(5))
 			->method('getBody')
-			->will($this->onConsecutiveCalls('Certainly not a JSON string', '{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'));
+			->will($this->onConsecutiveCalls('Certainly not a JSON string', 'Certainly not a JSON string', 'Certainly not a JSON string', 'Certainly not a JSON string', '{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'));
 		$this->clientService
-			->expects($this->exactly(2))
+			->expects($this->exactly(5))
 			->method('newClient')
 			->will($this->returnValue($client));
 
@@ -143,13 +137,13 @@ class ExternalShareControllerTest extends \Test\TestCase {
 			->disableOriginalConstructor()->getMock();
 		$client
 			->method('get')
-			->will($this->onConsecutiveCalls($response, $response));
+			->will($this->returnValue($response));
 		$response
-			->expects($this->exactly(2))
+			->expects($this->exactly(6))
 			->method('getBody')
 			->will($this->returnValue('Certainly not a JSON string'));
 		$this->clientService
-			->expects($this->exactly(2))
+			->expects($this->exactly(6))
 			->method('newClient')
 			->will($this->returnValue($client));