Merge pull request #44791 from nextcloud/reshare-permission-logic

fix: consider all paths the user has for a share source when considering max permissions

lib/private/Share20/Manager.php

@@ -45,7 +45,6 @@
 use OC\KnownUser\KnownUserService;
 use OC\Share20\Exception\ProviderException;
 use OCA\Files_Sharing\AppInfo\Application;
-use OCA\Files_Sharing\ISharedStorage;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\File;
 use OCP\Files\Folder;
@@ -294,55 +293,15 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false) {
 			throw new \InvalidArgumentException('A share requires permissions');
 		}
 
-		$isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage');
 		$permissions = 0;
-
-		$isReshare = $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy();
-		if (!$isReshare && $isUpdate) {
-			// in case of update on owner-less filesystem, we use share owner to improve reshare detection
-			$isReshare = $share->getShareOwner() !== $share->getSharedBy();
-		}
-
-		if (!$isFederatedShare && $isReshare) {
-			$userMounts = array_filter($userFolder->getById($share->getNode()->getId()), function ($mount) {
-				// We need to filter since there might be other mountpoints that contain the file
-				// e.g. if the user has access to the same external storage that the file is originating from
-				return $mount->getStorage()->instanceOfStorage(ISharedStorage::class);
-			});
-			$userMount = array_shift($userMounts);
-			if ($userMount === null) {
-				throw new GenericShareException('Could not get proper share mount for ' . $share->getNode()->getId() . '. Failing since else the next calls are called with null');
-			}
-			$mount = $userMount->getMountPoint();
-			// When it's a reshare use the parent share permissions as maximum
-			$userMountPointId = $mount->getStorageRootId();
-			$userMountPoint = $userFolder->getFirstNodeById($userMountPointId);
-
-			if ($userMountPoint === null) {
-				throw new GenericShareException('Could not get proper user mount for ' . $userMountPointId . '. Failing since else the next calls are called with null');
-			}
-
-			/* Check if this is an incoming share */
-			$incomingShares = $this->getSharedWith($share->getSharedBy(), IShare::TYPE_USER, $userMountPoint, -1, 0);
-			$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), IShare::TYPE_GROUP, $userMountPoint, -1, 0));
-			$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), IShare::TYPE_CIRCLE, $userMountPoint, -1, 0));
-			$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), IShare::TYPE_ROOM, $userMountPoint, -1, 0));
-
-			/** @var IShare[] $incomingShares */
-			if (!empty($incomingShares)) {
-				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 (!($share->getNode()->getMountPoint() instanceof MoveableMount)) {
-				$permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE;
+		$nodesForUser = $userFolder->getById($share->getNodeId());
+		foreach ($nodesForUser as $node) {
+			if ($node->getInternalPath() === '' && !$node->getMountPoint() instanceof MoveableMount) {
+				// for the root of non-movable mount, the permissions we see if limited by the mount itself,
+				// so we instead use the "raw" permissions from the storage
+				$permissions |= $node->getStorage()->getPermissions('');
+			} else {
+				$permissions |= $node->getPermissions();
 			}
 		}
 

tests/lib/Share20/ManagerTest.php

@@ -615,15 +615,18 @@ public function testVerifyPasswordHookFails() {
 		self::invokePrivate($this->manager, 'verifyPassword', ['password']);
 	}
 
-	public function createShare($id, $type, $path, $sharedWith, $sharedBy, $shareOwner,
+	public function createShare($id, $type, $node, $sharedWith, $sharedBy, $shareOwner,
 		$permissions, $expireDate = null, $password = null, $attributes = null) {
 		$share = $this->createMock(IShare::class);
 
 		$share->method('getShareType')->willReturn($type);
 		$share->method('getSharedWith')->willReturn($sharedWith);
 		$share->method('getSharedBy')->willReturn($sharedBy);
 		$share->method('getShareOwner')->willReturn($shareOwner);
-		$share->method('getNode')->willReturn($path);
+		$share->method('getNode')->willReturn($node);
+		if ($node && $node->getId()) {
+			$share->method('getNodeId')->willReturn($node->getId());
+		}
 		$share->method('getPermissions')->willReturn($permissions);
 		$share->method('getAttributes')->willReturn($attributes);
 		$share->method('getExpirationDate')->willReturn($expireDate);
@@ -648,8 +651,10 @@ public function dataGeneralChecks() {
 			->willReturn(false);
 		$file->method('getStorage')
 			->willReturn($storage);
+		$file->method('getId')->willReturn(108);
 		$node->method('getStorage')
 			->willReturn($storage);
+		$node->method('getId')->willReturn(108);
 
 		$data = [
 			[$this->createShare(null, IShare::TYPE_USER, $file, null, $user0, $user0, 31, null, null), 'SharedWith is not a valid user', true],
@@ -679,6 +684,7 @@ public function dataGeneralChecks() {
 		];
 
 		$nonShareAble = $this->createMock(Folder::class);
+		$nonShareAble->method('getId')->willReturn(108);
 		$nonShareAble->method('isShareable')->willReturn(false);
 		$nonShareAble->method('getPath')->willReturn('path');
 		$nonShareAble->method('getName')->willReturn('name');
@@ -714,16 +720,22 @@ public function dataGeneralChecks() {
 		$data[] = [$this->createShare(null, IShare::TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, 17, null, null), 'Cannot increase permissions of path', true];
 		$data[] = [$this->createShare(null, IShare::TYPE_LINK, $limitedPermssions, null, $user0, $user0, 3, null, null), 'Cannot increase permissions of path', true];
 
+		$nonMovableStorage = $this->createMock(Storage\IStorage::class);
+		$nonMovableStorage->method('instanceOfStorage')
+			->with('\OCA\Files_Sharing\External\Storage')
+			->willReturn(false);
+		$nonMovableStorage->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL);
 		$nonMoveableMountPermssions = $this->createMock(Folder::class);
 		$nonMoveableMountPermssions->method('isShareable')->willReturn(true);
 		$nonMoveableMountPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
 		$nonMoveableMountPermssions->method('getId')->willReturn(108);
 		$nonMoveableMountPermssions->method('getPath')->willReturn('path');
 		$nonMoveableMountPermssions->method('getName')->willReturn('name');
+		$nonMoveableMountPermssions->method('getInternalPath')->willReturn('');
 		$nonMoveableMountPermssions->method('getOwner')
 			->willReturn($owner);
 		$nonMoveableMountPermssions->method('getStorage')
-			->willReturn($storage);
+			->willReturn($nonMovableStorage);
 
 		$data[] = [$this->createShare(null, IShare::TYPE_USER, $nonMoveableMountPermssions, $user2, $user0, $user0, 11, null, null), 'Cannot increase permissions of path', false];
 		$data[] = [$this->createShare(null, IShare::TYPE_GROUP, $nonMoveableMountPermssions, $group0, $user0, $user0, 11, null, null), 'Cannot increase permissions of path', false];
@@ -797,10 +809,9 @@ public function testGeneralChecks($share, $exceptionMessage, $exception) {
 			->method('getId')
 			->willReturn(42);
 		// Id 108 is used in the data to refer to the node of the share.
-		$userFolder->expects($this->any())
-			->method('getFirstNodeById')
+		$userFolder->method('getById')
 			->with(108)
-			->willReturn($share->getNode());
+			->willReturn([$share->getNode()]);
 		$userFolder->expects($this->any())
 			->method('getRelativePath')
 			->willReturnArgument(0);