use https by default if no protocol is given. Only use unsecure connection if it...

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

diff --git a/apps/federatedfilesharing/lib/AddressHandler.php b/apps/federatedfilesharing/lib/AddressHandler.php
index 3c178b813e4cb5dfec195f510421ed65099206ae..5fc41c2c804f81015a6e71eefc76156e94017f49 100644 (file)
--- 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 074991c43f8542c9e63ce616a751d0d7dcf86994..8110b4915da507d3409fa00fcadaa77088607fb7 100644 (file)
--- 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 c2e69fb2bd70861187b2e72b907b7a34e647b6ac..f62f3b62e036d1c26db9ab16b1ea470c3327419a 100644 (file)
--- 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 dbcb1ef4e87d3139e57120326c9978cb72ba040b..a5f5c6bc07885eaf024bbe90008e245eba98fe9e 100644 (file)
--- 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')