Merge pull request #1975 from nextcloud/federated-sharing-always-http…

…s-by-default use https by default if no protocol is given.
nextcloud · Nov 2, 2016 · acf01b7 · acf01b7
2 parents 7da3ba3 + 76b1dee
commit acf01b7
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 38 deletions.
diff --git a/apps/federatedfilesharing/lib/AddressHandler.php b/apps/federatedfilesharing/lib/AddressHandler.php
@@ -160,6 +160,22 @@ public function removeProtocolFromUrl($url) {
 		return $url;
 	}
 
+	/**
+	 * check if the url contain the protocol (http or https)
+	 *
+	 * @param string $url
+	 * @return bool
+	 */
+	public function urlContainProtocol($url) {
+		if (strpos($url, 'https://') === 0 ||
+			strpos($url, 'http://') === 0) {
+
+			return true;
+		}
+
+		return false;
+	}
+
 	/**
 	 * Strips away a potential file names and trailing slashes:
 	 * - http://localhost

diff --git a/apps/federatedfilesharing/lib/Notifications.php b/apps/federatedfilesharing/lib/Notifications.php
@@ -84,7 +84,6 @@ public function sendRemoteShare($token, $shareWith, $name, $remote_id, $owner, $
 		list($user, $remote) = $this->addressHandler->splitUserRemote($shareWith);
 
 		if ($user && $remote) {
-			$url = $remote;
 			$local = $this->addressHandler->generateRemoteURL();
 
 			$fields = array(
@@ -99,8 +98,7 @@ public function sendRemoteShare($token, $shareWith, $name, $remote_id, $owner, $
 				'remote' => $local,
 			);
 
-			$url = $this->addressHandler->removeProtocolFromUrl($url);
-			$result = $this->tryHttpPostToShareEndpoint($url, '', $fields);
+			$result = $this->tryHttpPostToShareEndpoint($remote, '', $fields);
 			$status = json_decode($result['result'], true);
 
 			if ($result['success'] && ($status['ocs']['meta']['statuscode'] === 100 || $status['ocs']['meta']['statuscode'] === 200)) {
@@ -135,8 +133,7 @@ public function requestReShare($token, $id, $shareId, $remote, $shareWith, $perm
 			'remoteId' => $shareId
 		);
 
-		$url = $this->addressHandler->removeProtocolFromUrl($remote);
-		$result = $this->tryHttpPostToShareEndpoint(rtrim($url, '/'), '/' . $id . '/reshare', $fields);
+		$result = $this->tryHttpPostToShareEndpoint(rtrim($remote, '/'), '/' . $id . '/reshare', $fields);
 		$status = json_decode($result['result'], true);
 
 		$httpRequestSuccessful = $result['success'];
@@ -193,7 +190,7 @@ public function sendPermissionChange($remote, $remoteId, $token, $permissions) {
 
 	/**
 	 * forward accept reShare to remote server
-	 * 
+	 *
 	 * @param string $remote
 	 * @param int $remoteId
 	 * @param string $token
@@ -231,8 +228,7 @@ public function sendUpdateToRemote($remote, $remoteId, $token, $action, $data =
 			$fields[$key] = $value;
 		}
 
-		$url = $this->addressHandler->removeProtocolFromUrl($remote);
-		$result = $this->tryHttpPostToShareEndpoint(rtrim($url, '/'), '/' . $remoteId . '/' . $action, $fields);
+		$result = $this->tryHttpPostToShareEndpoint(rtrim($remote, '/'), '/' . $remoteId . '/' . $action, $fields);
 		$status = json_decode($result['result'], true);
 
 		if ($result['success'] &&
@@ -270,7 +266,8 @@ protected function getTimestamp() {
 	}
 
 	/**
-	 * try http post first with https and then with http as a fallback
+	 * try http post with the given protocol, if no protocol is given we pick
+	 * the secure one (https)
 	 *
 	 * @param string $remoteDomain
 	 * @param string $urlSuffix
@@ -280,33 +277,31 @@ protected function getTimestamp() {
 	 */
 	protected function tryHttpPostToShareEndpoint($remoteDomain, $urlSuffix, array $fields) {
 		$client = $this->httpClientService->newClient();
-		$protocol = 'https://';
+
+		if ($this->addressHandler->urlContainProtocol($remoteDomain) === false) {
+			$remoteDomain = 'https://' . $remoteDomain;
+		}
+
 		$result = [
 			'success' => false,
 			'result' => '',
 		];
-		$try = 0;
-
-		while ($result['success'] === false && $try < 2) {
-			$endpoint = $this->discoveryManager->getShareEndpoint($protocol . $remoteDomain);
-			try {
-				$response = $client->post($protocol . $remoteDomain . $endpoint . $urlSuffix . '?format=' . self::RESPONSE_FORMAT, [
-					'body' => $fields,
-					'timeout' => 10,
-					'connect_timeout' => 10,
-				]);
-				$result['result'] = $response->getBody();
-				$result['success'] = true;
-				break;
-			} catch (\Exception $e) {
-				// if flat re-sharing is not supported by the remote server
-				// we re-throw the exception and fall back to the old behaviour.
-				// (flat re-shares has been introduced in Nextcloud 9.1)
-				if ($e->getCode() === Http::STATUS_INTERNAL_SERVER_ERROR) {
-					throw $e;
-				}
-				$try++;
-				$protocol = 'http://';
+
+		$endpoint = $this->discoveryManager->getShareEndpoint($remoteDomain);
+		try {
+			$response = $client->post($remoteDomain . $endpoint . $urlSuffix . '?format=' . self::RESPONSE_FORMAT, [
+				'body' => $fields,
+				'timeout' => 10,
+				'connect_timeout' => 10,
+			]);
+			$result['result'] = $response->getBody();
+			$result['success'] = true;
+		} catch (\Exception $e) {
+			// if flat re-sharing is not supported by the remote server
+			// we re-throw the exception and fall back to the old behaviour.
+			// (flat re-shares has been introduced in Nextcloud 9.1)
+			if ($e->getCode() === Http::STATUS_INTERNAL_SERVER_ERROR) {
+				throw $e;
 			}
 		}
 

diff --git a/apps/federatedfilesharing/tests/AddressHandlerTest.php b/apps/federatedfilesharing/tests/AddressHandlerTest.php
@@ -177,6 +177,26 @@ public function dataTestRemoveProtocolFromUrl() {
 		];
 	}
 
+	/**
+	 * @dataProvider dataTestUrlContainProtocol
+	 *
+	 * @param string $url
+	 * @param bool $expectedResult
+	 */
+	public function testUrlContainProtocol($url, $expectedResult) {
+		$result = $this->addressHandler->urlContainProtocol($url);
+		$this->assertSame($expectedResult, $result);
+	}
+
+	public function dataTestUrlContainProtocol() {
+		return [
+			['http://nextcloud.com', true],
+			['https://nextcloud.com', true],
+			['nextcloud.com', false],
+			['httpserver.com', false],
+		];
+	}
+
 	/**
 	 * @dataProvider dataTestFixRemoteUrl
 	 *

diff --git a/apps/federatedfilesharing/tests/NotificationsTest.php b/apps/federatedfilesharing/tests/NotificationsTest.php
@@ -58,7 +58,7 @@ public function setUp() {
 
 	/**
 	 * get instance of Notifications class
-	 * 
+	 *
 	 * @param array $mockedMethods methods which should be mocked
 	 * @return Notifications | \PHPUnit_Framework_MockObject_MockObject
 	 */
@@ -81,7 +81,7 @@ private function getInstance(array $mockedMethods = []) {
 					]
 				)->setMethods($mockedMethods)->getMock();
 		}
-		
+
 		return $instance;
 	}
 
@@ -94,7 +94,7 @@ private function getInstance(array $mockedMethods = []) {
 	 * @param bool $expected
 	 */
 	public function testSendUpdateToRemote($try, $httpRequestResult, $expected) {
-		$remote = 'remote';
+		$remote = 'http://remote';
 		$id = 42;
 		$timestamp = 63576;
 		$token = 'token';
@@ -106,9 +106,6 @@ public function testSendUpdateToRemote($try, $httpRequestResult, $expected) {
 			->with($remote, '/'.$id.'/unshare', ['token' => $token, 'data1Key' => 'data1Value'])
 			->willReturn($httpRequestResult);
 
-		$this->addressHandler->expects($this->once())->method('removeProtocolFromUrl')
-			->with($remote)->willReturn($remote);
-
 		// only add background job on first try
 		if ($try === 0 && $expected === false) {
 			$this->jobList->expects($this->once())->method('add')