diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 66e39bb071..09489861e1 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/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); } diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index f00b5c424b..dcf264aae6 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/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'); + $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()); - } - } - - 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'); - - 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()); } } diff --git a/build/integration/features/sharing-v1-part2.feature b/build/integration/features/sharing-v1-part2.feature index f6532ea564..9fbb4cda94 100644 --- a/build/integration/features/sharing-v1-part2.feature +++ b/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 diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 18fefc021f..c8aafd9765 100644 --- a/lib/private/Share20/Manager.php +++ b/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 diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index ddbfe85722..170c58769d 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -546,6 +546,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); @@ -580,6 +583,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]; @@ -589,6 +594,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]; @@ -605,6 +612,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]; @@ -621,6 +630,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];