Merge pull request #16186 from nextcloud/bugfix/noid/also-check-permissions-when-creating-a-share

Better check reshare permissions part2

apps/files_sharing/lib/Controller/ShareAPIController.php

@@ -971,41 +971,13 @@ class ShareAPIController extends OCSController {
 				}
 				$share->setExpirationDate($expireDate);
 			}
-
 		}
 
-		if ($permissions !== null && $share->getShareOwner() !== $this->currentUser) {
-
-			// Get the root mount point for the user and check the share permissions there
-			$userFolder = $this->rootFolder->getUserFolder($this->currentUser);
-			$userNodes = $userFolder->getById($share->getNodeId());
-			$userNode = array_shift($userNodes);
-
-			$userMountPointId = $userNode->getMountPoint()->getStorageRootId();
-			$userMountPoints = $userFolder->getById($userMountPointId);
-			$userMountPoint = array_shift($userMountPoints);
-
-			/* Check if this is an incoming share */
-			$incomingShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $userMountPoint, -1, 0);
-			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0));
-			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $userMountPoint, -1, 0));
-
-			/** @var \OCP\Share\IShare[] $incomingShares */
-			if (!empty($incomingShares)) {
-				$maxPermissions = 0;
-				foreach ($incomingShares as $incomingShare) {
-					$maxPermissions |= $incomingShare->getPermissions();
-				}
-
-				if ($share->getPermissions() & ~$maxPermissions) {
-					throw new OCSNotFoundException($this->l->t('Cannot increase permissions'));
-				}
-			}
-		}
-
-
 		try {
 			$share = $this->shareManager->updateShare($share);
+		} catch (GenericShareException $e) {
+			$code = $e->getCode() === 0 ? 403 : $e->getCode();
+			throw new OCSException($e->getHint(), $code);
 		} catch (\Exception $e) {
 			throw new OCSBadRequestException($e->getMessage(), $e);
 		}

apps/files_sharing/tests/Controller/ShareAPIControllerTest.php

@@ -28,6 +28,7 @@ namespace OCA\Files_Sharing\Tests\Controller;
 
 use OCP\App\IAppManager;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\AppFramework\OCS\OCSException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\Files\File;
 use OCP\Files\Folder;
@@ -45,6 +46,7 @@ use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\Files\IRootFolder;
 use OCP\Lock\LockedException;
+use OCP\Share\Exceptions\GenericShareException;
 use OCP\Share\IManager;
 use OCP\Share;
 use Test\TestCase;
@@ -2418,157 +2420,16 @@ class ShareAPIControllerTest extends TestCase {
 		$mountPoint->method('getStorageRootId')
 			->willReturn(42);
 
-		$this->shareManager->expects($this->never())->method('updateShare');
-
-		try {
-			$ocs->updateShare(42, 31);
-			$this->fail();
-		} catch (OCSNotFoundException $e) {
-			$this->assertEquals('Cannot increase permissions', $e->getMessage());
-		}
-	}
-
-	public function testUpdateShareCannotIncreasePermissionsLinkShare() {
-		$ocs = $this->mockFormatShare();
-
-		$folder = $this->createMock(Folder::class);
-		$folder->method('getId')
-			->willReturn(42);
-
-		$share = \OC::$server->getShareManager()->newShare();
-		$share
-			->setId(42)
-			->setSharedBy($this->currentUser)
-			->setShareOwner('anotheruser')
-			->setShareType(\OCP\Share::SHARE_TYPE_LINK)
-			->setPermissions(\OCP\Constants::PERMISSION_READ)
-			->setNode($folder);
-
-		// note: updateShare will modify the received instance but getSharedWith will reread from the database,
-		// so their values will be different
-		$incomingShare = \OC::$server->getShareManager()->newShare();
-		$incomingShare
-			->setId(42)
-			->setSharedBy($this->currentUser)
-			->setShareOwner('anotheruser')
-			->setShareType(\OCP\Share::SHARE_TYPE_USER)
-			->setSharedWith('currentUser')
-			->setPermissions(\OCP\Constants::PERMISSION_READ)
-			->setNode($folder);
-
-		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
-
-		$this->shareManager->expects($this->any())
-			->method('getSharedWith')
-			->will($this->returnValueMap([
-				['currentUser', \OCP\Share::SHARE_TYPE_USER, $share->getNode(), -1, 0, [$incomingShare]],
-				['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, []],
-				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []]
-			]));
-
-		$userFolder = $this->createMock(Folder::class);
-		$this->rootFolder->method('getUserFolder')
-			->with($this->currentUser)
-			->willReturn($userFolder);
-
-		$userFolder->method('getById')
-			->with(42)
-			->willReturn([$folder]);
-
-		$mountPoint = $this->createMock(IMountPoint::class);
-		$folder->method('getMountPoint')
-			->willReturn($mountPoint);
-		$mountPoint->method('getStorageRootId')
-			->willReturn(42);
-
-		$this->shareManager->expects($this->never())->method('updateShare');
-		$this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true);
-
-		try {
-			$ocs->updateShare(42, null, null, null, 'true');
-			$this->fail();
-		} catch (OCSNotFoundException $e) {
-			$this->assertEquals('Cannot increase permissions', $e->getMessage());
-		}
-	}
-
-	public function testUpdateShareCannotIncreasePermissionsRoomShare() {
-		$ocs = $this->mockFormatShare();
-
-		$folder = $this->createMock(Folder::class);
-		$folder->method('getId')
-			->willReturn(42);
-
-		$share = \OC::$server->getShareManager()->newShare();
-		$share
-			->setId(42)
-			->setSharedBy($this->currentUser)
-			->setShareOwner('anotheruser')
-			->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
-			->setSharedWith('group1')
-			->setPermissions(\OCP\Constants::PERMISSION_READ)
-			->setNode($folder);
-
-		// note: updateShare will modify the received instance but getSharedWith will reread from the database,
-		// so their values will be different
-		$incomingShare = \OC::$server->getShareManager()->newShare();
-		$incomingShare
-			->setId(42)
-			->setSharedBy($this->currentUser)
-			->setShareOwner('anotheruser')
-			->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
-			->setSharedWith('group1')
-			->setPermissions(\OCP\Constants::PERMISSION_READ)
-			->setNode($folder);
-
-		$this->request
-			->method('getParam')
-			->will($this->returnValueMap([
-				['permissions', null, '31'],
-			]));
-
-		$this->shareManager
-			->method('getShareById')
-			->will($this->returnCallback(
-				function ($id) use ($share) {
-					if ($id !== 'ocRoomShare:42') {
-						throw new \OCP\Share\Exceptions\ShareNotFound();
-					}
-
-					return $share;
-				}
-			));
-
-		$userFolder = $this->createMock(Folder::class);
-		$this->rootFolder->method('getUserFolder')
-			->with($this->currentUser)
-			->willReturn($userFolder);
-
-		$userFolder->method('getById')
-			->with(42)
-			->willReturn([$folder]);
-
-		$mountPoint = $this->createMock(IMountPoint::class);
-		$folder->method('getMountPoint')
-			->willReturn($mountPoint);
-		$mountPoint->method('getStorageRootId')
-			->willReturn(42);
-
-		$this->shareManager->expects($this->any())
-			->method('getSharedWith')
-			->will($this->returnValueMap([
-				['currentUser', \OCP\Share::SHARE_TYPE_USER, $share->getNode(), -1, 0, []],
-				['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, []],
-				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, [$incomingShare]]
-			]));
-
-		$this->shareManager->expects($this->never())->method('updateShare');
+		$this->shareManager->expects($this->once())
+			->method('updateShare')
+			->with($share)
+			->willThrowException(new GenericShareException('Can’t increase permissions of path/file', 'Can’t increase permissions of path/file', 404));
 
 		try {
 			$ocs->updateShare(42, 31);
 			$this->fail();
-		} catch (OCSNotFoundException $e) {
-			$this->assertEquals('Cannot increase permissions', $e->getMessage());
+		} catch (OCSException $e) {
+			$this->assertEquals('Can’t increase permissions of path/file', $e->getMessage());
 		}
 	}
 

build/integration/features/sharing-v1-part2.feature

@@ -251,6 +251,66 @@ Feature: sharing
     Then the OCS status code should be "404"
     And the HTTP status code should be "200"
 
+  Scenario: User is not allowed to reshare file with additional delete permissions
+  As an "admin"
+    Given user "user0" exists
+    And user "user1" exists
+    And user "user2" exists
+    And As an "user0"
+    And creating a share with
+      | path | /PARENT |
+      | shareType | 0 |
+      | shareWith | user1 |
+      | permissions | 16 |
+    And As an "user1"
+    When creating a share with
+      | path | /PARENT (2) |
+      | shareType | 0 |
+      | shareWith | user2 |
+      | permissions | 25 |
+    Then the OCS status code should be "404"
+    And the HTTP status code should be "200"
+
+  Scenario: User is not allowed to reshare file with additional delete permissions for files
+  As an "admin"
+    Given user "user0" exists
+    And user "user1" exists
+    And user "user2" exists
+    And As an "user0"
+    And creating a share with
+      | path | /textfile0.txt |
+      | shareType | 0 |
+      | shareWith | user1 |
+      | permissions | 16 |
+    And As an "user1"
+    When creating a share with
+      | path | /textfile0 (2).txt |
+      | shareType | 0 |
+      | shareWith | user2 |
+      | permissions | 25 |
+    Then the OCS status code should be "100"
+    And the HTTP status code should be "200"
+    When Getting info of last share
+    Then 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 (2).txt |
+      | path | /textfile0 (2).txt |
+      | permissions | 17 |
+      | stime | A_NUMBER |
+      | storage | A_NUMBER |
+      | mail_send | 0 |
+      | uid_owner | user1 |
+      | storage_id | shared::/textfile0 (2).txt |
+      | file_parent | A_NUMBER |
+      | share_with_displayname | user2 |
+      | displayname_owner | user1 |
+      | mimetype          | text/plain |
+
   Scenario: Get a share with a user which didn't received the share
     Given user "user0" exists
     And user "user1" exists

lib/private/Share20/Manager.php

@@ -269,11 +269,13 @@ class Manager implements IManager {
 
 		// And you can't share your rootfolder
 		if ($this->userManager->userExists($share->getSharedBy())) {
-			$sharedPath = $this->rootFolder->getUserFolder($share->getSharedBy())->getPath();
+			$userFolder = $this->rootFolder->getUserFolder($share->getSharedBy());
+			$userFolderPath = $userFolder->getPath();
 		} else {
-			$sharedPath = $this->rootFolder->getUserFolder($share->getShareOwner())->getPath();
+			$userFolder = $this->rootFolder->getUserFolder($share->getShareOwner());
+			$userFolderPath = $userFolder->getPath();
 		}
-		if ($sharedPath === $share->getNode()->getPath()) {
+		if ($userFolderPath === $share->getNode()->getPath()) {
 			throw new \InvalidArgumentException('You can’t share your root folder');
 		}
 
@@ -288,15 +290,35 @@ class Manager implements IManager {
 			throw new \InvalidArgumentException('A share requires permissions');
 		}
 
-		/*
-		 * Quick fix for #23536
-		 * Non moveable mount points do not have update and delete permissions
-		 * while we 'most likely' do have that on the storage.
-		 */
-		$permissions = $share->getNode()->getPermissions();
 		$mount = $share->getNode()->getMountPoint();
-		if (!($mount instanceof MoveableMount)) {
-			$permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE;
+		if ($share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) {
+			// When it's a reshare use the parent share permissions as maximum
+			$userMountPointId = $mount->getStorageRootId();
+			$userMountPoints = $userFolder->getById($userMountPointId);
+			$userMountPoint = array_shift($userMountPoints);
+
+			/* Check if this is an incoming share */
+			$incomingShares = $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_USER, $userMountPoint, -1, 0);
+			$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0));
+			$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_ROOM, $userMountPoint, -1, 0));
+
+			/** @var \OCP\Share\IShare[] $incomingShares */
+			if (!empty($incomingShares)) {
+				$permissions = 0;
+				foreach ($incomingShares as $incomingShare) {
+					$permissions |= $incomingShare->getPermissions();
+				}
+			}
+		} else {
+			/*
+			 * Quick fix for #23536
+			 * Non moveable mount points do not have update and delete permissions
+			 * while we 'most likely' do have that on the storage.
+			 */
+			$permissions = $share->getNode()->getPermissions();
+			if (!($mount instanceof MoveableMount)) {
+				$permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE;
+			}
 		}
 
 		// Check that we do not share with more permissions than we have

tests/lib/Share20/ManagerTest.php

@@ -545,6 +545,9 @@ class ManagerTest extends \Test\TestCase {
 		$user0 = 'user0';
 		$user2 = 'user1';
 		$group0 = 'group0';
+		$owner = $this->createMock(IUser::class);
+		$owner->method('getUID')
+			->willReturn($user0);
 
 		$file = $this->createMock(File::class);
 		$node = $this->createMock(Node::class);
@@ -579,6 +582,8 @@ class ManagerTest extends \Test\TestCase {
 		$nonShareAble = $this->createMock(Folder::class);
 		$nonShareAble->method('isShareable')->willReturn(false);
 		$nonShareAble->method('getPath')->willReturn('path');
+		$nonShareAble->method('getOwner')
+			->willReturn($owner);
 
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER,  $nonShareAble, $user2, $user0, $user0, 31, null, null), 'You are not allowed to share path', true];
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $nonShareAble, $group0, $user0, $user0, 31, null, null), 'You are not allowed to share path', true];
@@ -588,6 +593,8 @@ class ManagerTest extends \Test\TestCase {
 		$limitedPermssions->method('isShareable')->willReturn(true);
 		$limitedPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
 		$limitedPermssions->method('getPath')->willReturn('path');
+		$limitedPermssions->method('getOwner')
+			->willReturn($owner);
 
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER,  $limitedPermssions, $user2, $user0, $user0, null, null, null), 'A share requires permissions', true];
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, null, null, null), 'A share requires permissions', true];
@@ -604,6 +611,8 @@ class ManagerTest extends \Test\TestCase {
 		$nonMoveableMountPermssions->method('isShareable')->willReturn(true);
 		$nonMoveableMountPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
 		$nonMoveableMountPermssions->method('getPath')->willReturn('path');
+		$nonMoveableMountPermssions->method('getOwner')
+			->willReturn($owner);
 
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER,  $nonMoveableMountPermssions, $user2, $user0, $user0, 11, null, null), 'Can’t increase permissions of path', false];
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $nonMoveableMountPermssions, $group0, $user0, $user0, 11, null, null), 'Can’t increase permissions of path', false];
@@ -620,6 +629,8 @@ class ManagerTest extends \Test\TestCase {
 		$allPermssions = $this->createMock(Folder::class);
 		$allPermssions->method('isShareable')->willReturn(true);
 		$allPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL);
+		$allPermssions->method('getOwner')
+			->willReturn($owner);
 
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER,  $allPermssions, $user2, $user0, $user0, 30, null, null), 'Shares need at least read permissions', true];
 		$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 2, null, null), 'Shares need at least read permissions', true];