Unify the permission checking in one place only
Signed-off-by: Joas Schilling <coding@schilljs.com>
This commit is contained in:
parent
16d1354239
commit
03005dea43
|
@ -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);
|
||||
}
|
||||
|
|
|
@ -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());
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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];
|
||||
|
|
Loading…
Reference in New Issue