From 8f7c64253ed6f1165ede51d70c788160f3c26a91 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 14 Apr 2015 11:27:51 +0200 Subject: [PATCH 1/8] Correctly generate the feedback URL for remote share The trailing slash was added in c78e3c4a7fa1d2f474ab58551e67a50e093f6ed8 to correctly generate the encryption keys --- apps/files_sharing/lib/external/manager.php | 2 +- lib/private/share/share.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/files_sharing/lib/external/manager.php b/apps/files_sharing/lib/external/manager.php index 7b8b756dd5..88f2571c23 100644 --- a/apps/files_sharing/lib/external/manager.php +++ b/apps/files_sharing/lib/external/manager.php @@ -235,7 +235,7 @@ class Manager { */ private function sendFeedbackToRemote($remote, $token, $id, $feedback) { - $url = $remote . \OCP\Share::BASE_PATH_TO_SHARE_API . '/' . $id . '/' . $feedback . '?format=' . \OCP\Share::RESPONSE_FORMAT; + $url = rtrim($remote, '/') . \OCP\Share::BASE_PATH_TO_SHARE_API . '/' . $id . '/' . $feedback . '?format=' . \OCP\Share::RESPONSE_FORMAT; $fields = array('token' => $token); $result = $this->httpHelper->post($url, $fields); diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 617eeeb9d9..eeb50be44d 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -2426,7 +2426,7 @@ class Share extends Constants { list($user, $remote) = explode('@', $shareWith, 2); if ($user && $remote) { - $url = $remote . self::BASE_PATH_TO_SHARE_API . '?format=' . self::RESPONSE_FORMAT; + $url = rtrim($remote, '/') . self::BASE_PATH_TO_SHARE_API . '?format=' . self::RESPONSE_FORMAT; $local = \OC::$server->getURLGenerator()->getAbsoluteURL('/'); @@ -2459,7 +2459,7 @@ class Share extends Constants { * @return bool */ private static function sendRemoteUnshare($remote, $id, $token) { - $url = $remote . self::BASE_PATH_TO_SHARE_API . '/' . $id . '/unshare?format=' . self::RESPONSE_FORMAT; + $url = rtrim($remote, '/') . self::BASE_PATH_TO_SHARE_API . '/' . $id . '/unshare?format=' . self::RESPONSE_FORMAT; $fields = array('token' => $token, 'format' => 'json'); $result = self::tryHttpPost($url, $fields); $status = json_decode($result['result'], true); From 66dca76f7272876ba19f85709f24e0457c90ca9d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 27 Apr 2015 17:14:52 +0200 Subject: [PATCH 2/8] check the called URL --- .../files_sharing/tests/external/managertest.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/apps/files_sharing/tests/external/managertest.php b/apps/files_sharing/tests/external/managertest.php index 3fd1f5444b..af0f188012 100644 --- a/apps/files_sharing/tests/external/managertest.php +++ b/apps/files_sharing/tests/external/managertest.php @@ -22,16 +22,20 @@ namespace OCA\Files_Sharing\Tests\External; use OC\Files\Storage\StorageFactory; +use OCA\Files_Sharing\External\Manager; use OCA\Files_Sharing\Tests\TestCase; class ManagerTest extends TestCase { - /** @var \OCA\Files_Sharing\External\Manager **/ + /** @var Manager **/ private $manager; /** @var \OC\Files\Mount\Manager */ private $mountManager; + /** @var \PHPUnit_Framework_MockObject_MockObject */ + private $httpHelper; + private $uid; protected function setUp() { @@ -39,16 +43,22 @@ class ManagerTest extends TestCase { $this->uid = $this->getUniqueID('user'); $this->mountManager = new \OC\Files\Mount\Manager(); - $this->manager = new \OCA\Files_Sharing\External\Manager( + $this->httpHelper = $httpHelper = $this->getMockBuilder('\OC\HTTPHelper')->disableOriginalConstructor()->getMock(); + /** @var \OC\HTTPHelper $httpHelper */ + $this->manager = new Manager( \OC::$server->getDatabaseConnection(), $this->mountManager, new StorageFactory(), - $this->getMockBuilder('\OC\HTTPHelper')->disableOriginalConstructor()->getMock(), + $httpHelper, $this->uid ); } public function testAddShare() { + $this->httpHelper->expects($this->exactly(4)) + ->method('post') + ->with($this->stringStartsWith('http://localhost/ocs/v1.php/cloud/shares'), $this->anything()); + $shareData1 = [ 'remote' => 'http://localhost', 'token' => 'token1', From d146c13abd74f5919c08f5a936f0c9d35791be8f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 27 Apr 2015 22:07:35 +0200 Subject: [PATCH 3/8] Add tests for the remote sharing url --- tests/lib/share/share.php | 55 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index 124ad450e2..5d8f61fb64 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -105,6 +105,12 @@ class Test_Share extends \Test\TestCase { parent::tearDown(); } + protected function setHttpHelper($httpHelper) { + \OC::$server->registerService('HTTPHelper', function ($c) use ($httpHelper) { + return $httpHelper; + }); + } + public function testShareInvalidShareType() { $message = 'Share type foobar is not valid for test.txt'; try { @@ -1024,10 +1030,55 @@ class Test_Share extends \Test\TestCase { ); } + public function dataRemoteShareUrlCalls() { + return [ + ['admin@localhost', 'localhost'], + ['admin@https://localhost', 'localhost'], + ['admin@http://localhost', 'localhost'], + ['admin@localhost/subFolder', 'localhost/subFolder'], + ]; + } + + /** + * @dataProvider dataRemoteShareUrlCalls + * + * @param string $shareWith + * @param string $urlHost + */ + public function testRemoteShareUrlCalls($shareWith, $urlHost) { + $oldHttpHelper = \OC::$server->query('HTTPHelper'); + $httpHelperMock = $this->getMockBuilder('OC\HttpHelper') + ->disableOriginalConstructor() + ->getMock(); + $this->setHttpHelper($httpHelperMock); + + $httpHelperMock->expects($this->at(0)) + ->method('post') + ->with($this->stringStartsWith('https://' . $urlHost . '/ocs/v1.php/cloud/shares'), $this->anything()) + ->willReturn(['success' => false, 'result' => 'Exception']); + $httpHelperMock->expects($this->at(1)) + ->method('post') + ->with($this->stringStartsWith('http://' . $urlHost . '/ocs/v1.php/cloud/shares'), $this->anything()) + ->willReturn(['success' => true, 'result' => json_encode(['ocs' => ['meta' => ['statuscode' => 100]]])]); + + $httpHelperMock->expects($this->at(2)) + ->method('post') + ->with($this->stringStartsWith('https://' . $urlHost . '/ocs/v1.php/cloud/shares'), $this->anything()) + ->willReturn(['success' => false, 'result' => 'Exception']); + $httpHelperMock->expects($this->at(3)) + ->method('post') + ->with($this->stringStartsWith('http://' . $urlHost . '/ocs/v1.php/cloud/shares'), $this->anything()) + ->willReturn(['success' => true, 'result' => json_encode(['ocs' => ['meta' => ['statuscode' => 100]]])]); + + \OCP\Share::shareItem('test', 'test.txt', \OCP\Share::SHARE_TYPE_REMOTE, $shareWith, \OCP\Constants::PERMISSION_READ); + \OCP\Share::unshare('test', 'test.txt', \OCP\Share::SHARE_TYPE_REMOTE, $shareWith); + $this->setHttpHelper($oldHttpHelper); + } + /** * @dataProvider dataProviderTestGroupItems - * @param type $ungrouped - * @param type $grouped + * @param array $ungrouped + * @param array $grouped */ function testGroupItems($ungrouped, $grouped) { From 9fb7d0bca9006b1d19768fb214a0883e8158f455 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 27 Apr 2015 22:08:44 +0200 Subject: [PATCH 4/8] Correctly remove the protocol before prepeding it --- lib/private/share/share.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index eeb50be44d..d2bdca22f3 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -2461,6 +2461,7 @@ class Share extends Constants { private static function sendRemoteUnshare($remote, $id, $token) { $url = rtrim($remote, '/') . self::BASE_PATH_TO_SHARE_API . '/' . $id . '/unshare?format=' . self::RESPONSE_FORMAT; $fields = array('token' => $token, 'format' => 'json'); + $url = self::removeProtocolFromUrl($url); $result = self::tryHttpPost($url, $fields); $status = json_decode($result['result'], true); From 02c60949ddbf54d8c1791c0f7686b13cbe3b7002 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 28 Apr 2015 08:13:49 +0200 Subject: [PATCH 5/8] make scrutinizer happy --- tests/lib/share/share.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index 5d8f61fb64..23274c394c 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -106,7 +106,7 @@ class Test_Share extends \Test\TestCase { } protected function setHttpHelper($httpHelper) { - \OC::$server->registerService('HTTPHelper', function ($c) use ($httpHelper) { + \OC::$server->registerService('HTTPHelper', function () use ($httpHelper) { return $httpHelper; }); } From 2eecfcbb8040aa7ca36df6ecd3f3de6b6c328cd8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 28 Apr 2015 09:10:59 +0200 Subject: [PATCH 6/8] Fix scrutinizer complains and return type doc --- lib/private/share/share.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index d2bdca22f3..1ece47df7e 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -2395,15 +2395,17 @@ class Share extends Constants { * * @param string $url * @param array $fields post parameters - * @return bool + * @return array */ private static function tryHttpPost($url, $fields) { $protocol = 'https://'; - $success = false; + $result = [ + 'success' => false, + 'result' => '', + ]; $try = 0; - while ($success === false && $try < 2) { + while ($result['success'] === false && $try < 2) { $result = \OC::$server->getHTTPHelper()->post($protocol . $url, $fields); - $success = $result['success']; $try++; $protocol = 'http://'; } From 34181c3aefa4d64198736d36d7d7b0749ad03b8e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 28 Apr 2015 14:29:04 +0200 Subject: [PATCH 7/8] Correctly send Federate-Cloud Share Feedback against the remote share ID --- apps/files_sharing/lib/external/manager.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/files_sharing/lib/external/manager.php b/apps/files_sharing/lib/external/manager.php index 88f2571c23..0246e2d9c8 100644 --- a/apps/files_sharing/lib/external/manager.php +++ b/apps/files_sharing/lib/external/manager.php @@ -174,7 +174,7 @@ class Manager { */ private function getShare($id) { $getShare = $this->connection->prepare(' - SELECT `remote`, `share_token`, `name` + SELECT `remote`, `remote_id`, `share_token`, `name` FROM `*PREFIX*share_external` WHERE `id` = ? AND `user` = ?'); $result = $getShare->execute(array($id, $this->uid)); @@ -203,7 +203,7 @@ class Manager { `mountpoint_hash` = ? WHERE `id` = ? AND `user` = ?'); $acceptShare->execute(array(1, $mountPoint, $hash, $id, $this->uid)); - $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $id, 'accept'); + $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'accept'); } } @@ -220,7 +220,7 @@ class Manager { $removeShare = $this->connection->prepare(' DELETE FROM `*PREFIX*share_external` WHERE `id` = ? AND `user` = ?'); $removeShare->execute(array($id, $this->uid)); - $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $id, 'decline'); + $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline'); } } @@ -229,13 +229,13 @@ class Manager { * * @param string $remote * @param string $token - * @param int $id + * @param int $remoteId Share id on the remote host * @param string $feedback * @return boolean */ - private function sendFeedbackToRemote($remote, $token, $id, $feedback) { + private function sendFeedbackToRemote($remote, $token, $remoteId, $feedback) { - $url = rtrim($remote, '/') . \OCP\Share::BASE_PATH_TO_SHARE_API . '/' . $id . '/' . $feedback . '?format=' . \OCP\Share::RESPONSE_FORMAT; + $url = rtrim($remote, '/') . \OCP\Share::BASE_PATH_TO_SHARE_API . '/' . $remoteId . '/' . $feedback . '?format=' . \OCP\Share::RESPONSE_FORMAT; $fields = array('token' => $token); $result = $this->httpHelper->post($url, $fields); From b55ef51a27e73a30e0829e273cdf111e21a77403 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 28 Apr 2015 14:56:13 +0200 Subject: [PATCH 8/8] Add tests for the correct share id on the call aswell --- .../tests/external/managertest.php | 18 +++++++++++++++--- tests/lib/share/share.php | 13 ++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/apps/files_sharing/tests/external/managertest.php b/apps/files_sharing/tests/external/managertest.php index af0f188012..f7b216530d 100644 --- a/apps/files_sharing/tests/external/managertest.php +++ b/apps/files_sharing/tests/external/managertest.php @@ -55,9 +55,6 @@ class ManagerTest extends TestCase { } public function testAddShare() { - $this->httpHelper->expects($this->exactly(4)) - ->method('post') - ->with($this->stringStartsWith('http://localhost/ocs/v1.php/cloud/shares'), $this->anything()); $shareData1 = [ 'remote' => 'http://localhost', @@ -96,6 +93,10 @@ class ManagerTest extends TestCase { $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->httpHelper->expects($this->at(0)) + ->method('post') + ->with($this->stringStartsWith('http://localhost/ocs/v1.php/cloud/shares/' . $openShares[0]['remote_id']), $this->anything()); + // Accept the first share $this->manager->acceptShare($openShares[0]['id']); @@ -127,6 +128,10 @@ class ManagerTest extends TestCase { $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->httpHelper->expects($this->at(0)) + ->method('post') + ->with($this->stringStartsWith('http://localhost/ocs/v1.php/cloud/shares/' . $openShares[1]['remote_id'] . '/decline'), $this->anything()); + // Decline the third share $this->manager->declineShare($openShares[1]['id']); @@ -150,6 +155,13 @@ class ManagerTest extends TestCase { $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->httpHelper->expects($this->at(0)) + ->method('post') + ->with($this->stringStartsWith('http://localhost/ocs/v1.php/cloud/shares/' . $openShares[0]['remote_id'] . '/decline'), $this->anything()); + $this->httpHelper->expects($this->at(1)) + ->method('post') + ->with($this->stringStartsWith('http://localhost/ocs/v1.php/cloud/shares/' . $acceptedShares[0]['remote_id'] . '/decline'), $this->anything()); + $this->manager->removeUserShares($this->uid); $this->assertEmpty(\Test_Helper::invokePrivate($this->manager, 'getShares', [null]), 'Asserting all shares for the user have been deleted'); diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php index 23274c394c..3aec11cf6a 100644 --- a/tests/lib/share/share.php +++ b/tests/lib/share/share.php @@ -1061,16 +1061,19 @@ class Test_Share extends \Test\TestCase { ->with($this->stringStartsWith('http://' . $urlHost . '/ocs/v1.php/cloud/shares'), $this->anything()) ->willReturn(['success' => true, 'result' => json_encode(['ocs' => ['meta' => ['statuscode' => 100]]])]); - $httpHelperMock->expects($this->at(2)) + \OCP\Share::shareItem('test', 'test.txt', \OCP\Share::SHARE_TYPE_REMOTE, $shareWith, \OCP\Constants::PERMISSION_READ); + $shares = \OCP\Share::getItemShared('test', 'test.txt'); + $share = array_shift($shares); + + $httpHelperMock->expects($this->at(0)) ->method('post') - ->with($this->stringStartsWith('https://' . $urlHost . '/ocs/v1.php/cloud/shares'), $this->anything()) + ->with($this->stringStartsWith('https://' . $urlHost . '/ocs/v1.php/cloud/shares/' . $share['id'] . '/unshare'), $this->anything()) ->willReturn(['success' => false, 'result' => 'Exception']); - $httpHelperMock->expects($this->at(3)) + $httpHelperMock->expects($this->at(1)) ->method('post') - ->with($this->stringStartsWith('http://' . $urlHost . '/ocs/v1.php/cloud/shares'), $this->anything()) + ->with($this->stringStartsWith('http://' . $urlHost . '/ocs/v1.php/cloud/shares/' . $share['id'] . '/unshare'), $this->anything()) ->willReturn(['success' => true, 'result' => json_encode(['ocs' => ['meta' => ['statuscode' => 100]]])]); - \OCP\Share::shareItem('test', 'test.txt', \OCP\Share::SHARE_TYPE_REMOTE, $shareWith, \OCP\Constants::PERMISSION_READ); \OCP\Share::unshare('test', 'test.txt', \OCP\Share::SHARE_TYPE_REMOTE, $shareWith); $this->setHttpHelper($oldHttpHelper); }