Unify the permission checking in one place only

Signed-off-by: Joas Schilling <coding@schilljs.com>

diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 66e39bb0715ae876e2aba3360c95d54284e31b07..09489861e1c1b8a3d266f0096a9f59942e0b3b4e 100644 (file)
--- 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 f00b5c424bf15262a9f3dfc8cefe6a9514eb84cf..80b47d85df6c26b8499e9e13aa71b9da508c4382 100644 (file)
--- 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,13 +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());
+               } catch (OCSException $e) {
+                       $this->assertEquals('Can’t increase permissions of path/file', $e->getMessage());
                }
        }
 

diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php
index 4e80e1d0668b4693d782fc5ed54a969424aae45c..c8aafd976524f8db1a777624a763bf65634f5a9e 100644 (file)
--- a/lib/private/Share20/Manager.php
+++ b/lib/private/Share20/Manager.php
@@ -290,16 +290,9 @@ 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;
-               } else if ($share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) {
+               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);
@@ -316,6 +309,16 @@ class Manager implements IManager {
                                        $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 747ad556c05462cf30471f875be819604e85f6bf..170c58769db9e9b7d775a3c84b967a9904bab0e3 100644 (file)
--- 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,10 +594,6 @@ class ManagerTest extends \Test\TestCase {
                $limitedPermssions->method('isShareable')->willReturn(true);
                $limitedPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
                $limitedPermssions->method('getPath')->willReturn('path');
-
-               $owner = $this->createMock(IUser::class);
-               $owner->method('getUID')
-                       ->willReturn($user0);
                $limitedPermssions->method('getOwner')
                        ->willReturn($owner);
 
@@ -611,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];
@@ -627,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];