From b647341dbeb3ebee992d84a50c25079057aaf1a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Fri, 16 Aug 2019 15:09:15 +0200 Subject: [PATCH] Fix shares read permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user with reshare permissions on a file is now able to get any share of that file (just like the owner). Signed-off-by: John Molakvoæ (skjnldsv) --- .../lib/Controller/ShareAPIController.php | 26 ++++++-- .../Controller/ShareAPIControllerTest.php | 62 +++++++++++++++++++ .../sharing_features/sharing-v1-part2.feature | 34 +++++++++- .../sharing_features/sharing-v1-part3.feature | 4 +- 4 files changed, 117 insertions(+), 9 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 986f8cea1d88c..cde4f93a0f0d3 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -305,13 +305,13 @@ public function getShare(string $id): DataResponse { throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist')); } - if ($this->canAccessShare($share)) { - try { + try { + if ($this->canAccessShare($share)) { $share = $this->formatShare($share); return new DataResponse([$share]); - } catch (NotFoundException $e) { - //Fall trough } + } catch (NotFoundException $e) { + // Fall trough } throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist')); @@ -983,6 +983,13 @@ public function updateShare( } /** + * Does the user have read permission on the share + * + * @param \OCP\Share\IShare $share the share to check + * @param boolean $checkGroups check groups as well? + * @return boolean + * @throws NotFoundException + * * @suppress PhanUndeclaredClassMethod */ protected function canAccessShare(\OCP\Share\IShare $share, bool $checkGroups = true): bool { @@ -997,12 +1004,21 @@ protected function canAccessShare(\OCP\Share\IShare $share, bool $checkGroups = return true; } - // If the share is shared with you (or a group you are a member of) + // If the share is shared with you, you can access it! if ($share->getShareType() === Share::SHARE_TYPE_USER && $share->getSharedWith() === $this->currentUser) { return true; } + // Have reshare rights on the shared file/folder ? + // Does the currentUser have access to the shared file? + $userFolder = $this->rootFolder->getUserFolder($this->currentUser); + $files = $userFolder->getById($share->getNodeId()); + if (!empty($files) && $this->shareProviderResharingRights($this->currentUser, $share, $files[0])) { + return true; + } + + // If in the recipient group, you can see the share if ($checkGroups && $share->getShareType() === Share::SHARE_TYPE_GROUP) { $sharedWith = $this->groupManager->get($share->getSharedWith()); $user = $this->userManager->get($this->currentUser); diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 7eee526f2d14f..5a84897fe9173 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -375,6 +375,15 @@ public function testDeleteSharedWithMyGroup() { ->method('lock') ->with(\OCP\Lock\ILockingProvider::LOCK_SHARED); + $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$share->getNode()]); + $this->shareManager->expects($this->once()) ->method('deleteFromSelf') ->with($share, $this->currentUser); @@ -427,6 +436,15 @@ public function testDeleteSharedWithGroupIDontBelongTo() { ->method('lock') ->with(\OCP\Lock\ILockingProvider::LOCK_SHARED); + $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$share->getNode()]); + $this->shareManager->expects($this->never()) ->method('deleteFromSelf'); @@ -758,6 +776,11 @@ public function testGetShareInvalidNode() { ->with('ocinternal:42', 'currentUser') ->willReturn($share); + $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $this->ocs->getShare(42); } @@ -775,6 +798,27 @@ public function testCanAccessShare() { $share->method('getSharedWith')->willReturn($this->currentUser); $this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share])); + $file = $this->getMockBuilder(File::class)->getMock(); + + $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$file]); + + $file->method('getPermissions') + ->will($this->onConsecutiveCalls(\OCP\Constants::PERMISSION_SHARE, \OCP\Constants::PERMISSION_READ)); + + // getPermissions -> share + $share = $this->getMockBuilder(IShare::class)->getMock(); + $share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); + $share->method('getSharedWith')->willReturn($this->getMockBuilder(IUser::class)->getMock()); + $this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share])); + + // getPermissions -> read $share = $this->getMockBuilder(IShare::class)->getMock(); $share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); $share->method('getSharedWith')->willReturn($this->getMockBuilder(IUser::class)->getMock()); @@ -852,6 +896,15 @@ public function dataCanAccessRoomShare() { * @param bool canAccessShareByHelper */ public function testCanAccessRoomShare(bool $expected, \OCP\Share\IShare $share, bool $helperAvailable, bool $canAccessShareByHelper) { + $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$share->getNode()]); + if (!$helperAvailable) { $this->appManager->method('isEnabledForUser') ->with('spreed') @@ -1727,6 +1780,15 @@ public function testUpdateShareCantAccess() { $this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share); + $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$share->getNode()]); + $this->ocs->updateShare(42); } diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature index 5cea18b6b11cb..3316f3d94baef 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/build/integration/sharing_features/sharing-v1-part2.feature @@ -321,6 +321,36 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: Get a share with a user with resharing rights + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And file "textfile0.txt" of user "user0" is shared with user "user1" + And file "textfile0.txt" of user "user0" is shared with user "user2" + And As an "user1" + When Getting info of last share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And Share fields of last share match with + | id | A_NUMBER | + | item_type | file | + | item_source | A_NUMBER | + | share_type | 0 | + | share_with | user2 | + | file_source | A_NUMBER | + | file_target | /textfile0.txt | + | path | /textfile0 (2).txt | + | permissions | 19 | + | stime | A_NUMBER | + | storage | A_NUMBER | + | mail_send | 0 | + | uid_owner | user0 | + | storage_id | shared::/textfile0 (2).txt | + | file_parent | A_NUMBER | + | share_with_displayname | user2 | + | displayname_owner | user0 | + | mimetype | text/plain | + Scenario: Share of folder and sub-folder to same user - core#20645 Given As an "admin" And user "user0" exists @@ -427,8 +457,8 @@ Feature: sharing And file "textfile0.txt" of user "user0" is shared with user "user2" And As an "user1" When Deleting last share - Then the OCS status code should be "404" - And the HTTP status code should be "200" + Then the OCS status code should be "403" + And the HTTP status code should be "401" Scenario: Keep usergroup shares (#22143) Given As an "admin" diff --git a/build/integration/sharing_features/sharing-v1-part3.feature b/build/integration/sharing_features/sharing-v1-part3.feature index 6c34534333aa0..4a02e89a67159 100644 --- a/build/integration/sharing_features/sharing-v1-part3.feature +++ b/build/integration/sharing_features/sharing-v1-part3.feature @@ -398,8 +398,8 @@ Feature: sharing When As an "user1" And Updating last share with | permissions | 19 | - Then the OCS status code should be "404" - And the HTTP status code should be "200" + Then the OCS status code should be "403" + And the HTTP status code should be "401" Scenario: do not allow to increase link share permissions on reshare Given As an "admin"