From 76b1dee499771c1b5327b4b7792e98fd41049096 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 2 Nov 2016 10:44:55 +0100 Subject: [PATCH] use https by default if no protocol is given. Only use unsecure connection if it is explicitely given Signed-off-by: Bjoern Schiessle --- .../lib/AddressHandler.php | 16 ++++++ .../lib/Notifications.php | 57 +++++++++---------- .../tests/AddressHandlerTest.php | 20 +++++++ .../tests/NotificationsTest.php | 9 +-- 4 files changed, 65 insertions(+), 37 deletions(-) diff --git a/apps/federatedfilesharing/lib/AddressHandler.php b/apps/federatedfilesharing/lib/AddressHandler.php index 3c178b813e..5fc41c2c80 100644 --- a/apps/federatedfilesharing/lib/AddressHandler.php +++ b/apps/federatedfilesharing/lib/AddressHandler.php @@ -160,6 +160,22 @@ class AddressHandler { 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 index 074991c43f..8110b4915d 100644 --- a/apps/federatedfilesharing/lib/Notifications.php +++ b/apps/federatedfilesharing/lib/Notifications.php @@ -84,7 +84,6 @@ class Notifications { list($user, $remote) = $this->addressHandler->splitUserRemote($shareWith); if ($user && $remote) { - $url = $remote; $local = $this->addressHandler->generateRemoteURL(); $fields = array( @@ -99,8 +98,7 @@ class Notifications { '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 @@ class Notifications { '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 @@ class Notifications { /** * forward accept reShare to remote server - * + * * @param string $remote * @param int $remoteId * @param string $token @@ -231,8 +228,7 @@ class Notifications { $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 @@ class Notifications { } /** - * 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 @@ class Notifications { */ 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 index c2e69fb2bd..f62f3b62e0 100644 --- a/apps/federatedfilesharing/tests/AddressHandlerTest.php +++ b/apps/federatedfilesharing/tests/AddressHandlerTest.php @@ -177,6 +177,26 @@ class AddressHandlerTest extends \Test\TestCase { ]; } + /** + * @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 index dbcb1ef4e8..a5f5c6bc07 100644 --- a/apps/federatedfilesharing/tests/NotificationsTest.php +++ b/apps/federatedfilesharing/tests/NotificationsTest.php @@ -58,7 +58,7 @@ class NotificationsTest extends \Test\TestCase { /** * get instance of Notifications class - * + * * @param array $mockedMethods methods which should be mocked * @return Notifications | \PHPUnit_Framework_MockObject_MockObject */ @@ -81,7 +81,7 @@ class NotificationsTest extends \Test\TestCase { ] )->setMethods($mockedMethods)->getMock(); } - + return $instance; } @@ -94,7 +94,7 @@ class NotificationsTest extends \Test\TestCase { * @param bool $expected */ public function testSendUpdateToRemote($try, $httpRequestResult, $expected) { - $remote = 'remote'; + $remote = 'http://remote'; $id = 42; $timestamp = 63576; $token = 'token'; @@ -106,9 +106,6 @@ class NotificationsTest extends \Test\TestCase { ->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')