Fix shares read permissions

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) <skjnldsv@protonmail.com>
nextcloud · Oct 4, 2019 · ff895ab · ff895ab
1 parent b1069b2
commit ff895ab
Show file tree

Hide file tree

Showing 6 changed files with 122 additions and 14 deletions.
diff --git 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
@@ -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/apps/oauth2/js/oauth2.js b/apps/oauth2/js/oauth2.js
diff --git a/apps/oauth2/js/oauth2.js.map b/apps/oauth2/js/oauth2.js.map
diff --git 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
@@ -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"