Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable29] fix: consider all paths the user has for a share source when considering max permissions #45293

Merged
merged 2 commits into from
May 13, 2024
Merged

[stable29] fix: consider all paths the user has for a share source when considering max permissions #45293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
762437d
fix: cleanup logic for getting the max reshare permissions
icewind1991 May 2, 2024
ff47b05
test: adjust test to new permissions check logic
icewind1991 May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 8 additions & 49 deletions lib/private/Share20/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
}

Expand Down
23 changes: 17 additions & 6 deletions tests/lib/Share20/ManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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],
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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);
Expand Down
Loading