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.

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

Implement share co-owning #49073

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions apps/files/lib/Service/OwnershipTransferService.php
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ private function collectUsersShares(
foreach ($supportedShareTypes as $shareType) {
$offset = 0;
while (true) {
$sharePage = $this->shareManager->getSharesBy($sourceUid, $shareType, null, true, 50, $offset);
$sharePage = $this->shareManager->getSharesBy($sourceUid, $shareType, null, true, 50, $offset, onlyValid: false);
$progress->advance(count($sharePage));
if (empty($sharePage)) {
break;
Expand Down Expand Up @@ -464,7 +464,7 @@ private function restoreShares(
}
$share->setNodeId($newNodeId);

$this->shareManager->updateShare($share);
$this->shareManager->updateShare($share, onlyValid: false);
}
}
} catch (NotFoundException $e) {
Expand Down
27 changes: 14 additions & 13 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
use OCP\Files\Folder;
use OCP\Files\InvalidPathException;
use OCP\Files\IRootFolder;
use OCP\Files\Mount\IShareCoOwnerMount;
use OCP\Files\Node;
use OCP\Files\NotFoundException;
use OCP\IConfig;
Expand Down Expand Up @@ -1235,18 +1236,6 @@ public function updateShare(
if ($share->getShareType() === IShare::TYPE_LINK
|| $share->getShareType() === IShare::TYPE_EMAIL) {

/**
* We do not allow editing link shares that the current user
* doesn't own. This is confusing and lead to errors when
* someone else edit a password or expiration date without
* the share owner knowing about it.
* We only allow deletion
*/

if ($share->getSharedBy() !== $this->userId) {
throw new OCSForbiddenException($this->l->t('You are not allowed to edit link shares that you don\'t own'));
}

// Update hide download state
$attributes = $share->getAttributes() ?? $share->newAttributes();
if ($hideDownload === 'true') {
Expand Down Expand Up @@ -1488,7 +1477,7 @@ protected function canAccessShare(IShare $share, bool $checkGroups = true): bool
// Does the currentUser have access to the shared file?
$userFolder = $this->rootFolder->getUserFolder($this->userId);
$file = $userFolder->getFirstNodeById($share->getNodeId());
if ($file && $this->shareProviderResharingRights($this->userId, $share, $file)) {
if ($file !== null && $this->shareProviderResharingRights($this->userId, $share, $file)) {
return true;
}

Expand Down Expand Up @@ -1553,6 +1542,12 @@ protected function canEditShare(IShare $share): bool {
return true;
}

$userFolder = $this->rootFolder->getUserFolder($this->userId);
$file = $userFolder->getFirstNodeById($share->getNodeId());
if ($file !== null && $file->getMountPoint() instanceof IShareCoOwnerMount && $this->shareProviderResharingRights($this->userId, $share, $file)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe if ($file?->getMountPoint() instanceof [...]) {

return true;
}

//! we do NOT support some kind of `admin` in groups.
//! You cannot edit shares shared to a group you're
//! a member of if you're not the share owner or the file owner!
Expand Down Expand Up @@ -1588,6 +1583,12 @@ protected function canDeleteShare(IShare $share): bool {
return true;
}

$userFolder = $this->rootFolder->getUserFolder($this->userId);
$file = $userFolder->getFirstNodeById($share->getNodeId());
if ($file !== null && $file->getMountPoint() instanceof IShareCoOwnerMount && $this->shareProviderResharingRights($this->userId, $share, $file)) {
return true;
}

return false;
}

Expand Down
18 changes: 11 additions & 7 deletions apps/files_sharing/lib/Updater.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,24 @@ private static function moveShareInOrOutOfShare($path): void {

$shareManager = \OC::$server->getShareManager();

// We intentionally include invalid shares, as they have been automatically invalidated due to the node no longer
// being accessible for the user. Only in this case where we adjust the share after it was moved we want to ignore
// this to be able to still adjust it.

// FIXME: should CIRCLES be included here ??
$shares = $shareManager->getSharesBy($user->getUID(), IShare::TYPE_USER, $src, false, -1);
$shares = array_merge($shares, $shareManager->getSharesBy($user->getUID(), IShare::TYPE_GROUP, $src, false, -1));
$shares = array_merge($shares, $shareManager->getSharesBy($user->getUID(), IShare::TYPE_ROOM, $src, false, -1));
$shares = $shareManager->getSharesBy($user->getUID(), IShare::TYPE_USER, $src, false, -1, onlyValid: false);
$shares = array_merge($shares, $shareManager->getSharesBy($user->getUID(), IShare::TYPE_GROUP, $src, false, -1, onlyValid: false));
$shares = array_merge($shares, $shareManager->getSharesBy($user->getUID(), IShare::TYPE_ROOM, $src, false, -1, onlyValid: false));

if ($src instanceof Folder) {
$cacheAccess = Server::get(FileAccess::class);

$sourceStorageId = $src->getStorage()->getCache()->getNumericStorageId();
$sourceInternalPath = $src->getInternalPath();
$subShares = array_merge(
$shareManager->getSharesBy($user->getUID(), IShare::TYPE_USER),
$shareManager->getSharesBy($user->getUID(), IShare::TYPE_GROUP),
$shareManager->getSharesBy($user->getUID(), IShare::TYPE_ROOM),
$shareManager->getSharesBy($user->getUID(), IShare::TYPE_USER, onlyValid: false),
$shareManager->getSharesBy($user->getUID(), IShare::TYPE_GROUP, onlyValid: false),
$shareManager->getSharesBy($user->getUID(), IShare::TYPE_ROOM, onlyValid: false),
);
$shareSourceIds = array_map(fn (IShare $share) => $share->getNodeId(), $subShares);
$shareSources = $cacheAccess->getByFileIdsInStorage($shareSourceIds, $sourceStorageId);
Expand Down Expand Up @@ -111,7 +115,7 @@ private static function moveShareInOrOutOfShare($path): void {

$share->setShareOwner($newOwner);
$share->setPermissions($newPermissions);
$shareManager->updateShare($share);
$shareManager->updateShare($share, onlyValid: false);
}
}

Expand Down
5 changes: 5 additions & 0 deletions apps/files_sharing/src/components/SharingEntry.vue
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ export default {
} else if (this.share.type === this.SHARE_TYPES.SHARE_TYPE_GUEST) {
title += ` (${t('files_sharing', 'guest')})`
}
if (!this.isShareOwner && this.share.ownerDisplayName) {
title += t('files_sharing', ' by {initiator}', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leading / trailing spaces in translations are very likely to break

Suggested change
title += t('files_sharing', ' by {initiator}', {
title += ' ' + t('files_sharing', 'by {initiator}', {

initiator: this.share.ownerDisplayName,
})
}
return title
},
tooltip() {
Expand Down
124 changes: 124 additions & 0 deletions apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
use OCP\Files\Mount\IMountPoint;
use OCP\Files\Mount\IShareCoOwnerMount;
use OCP\Files\NotFoundException;
use OCP\Files\Storage\IStorage;
use OCP\IConfig;
Expand Down Expand Up @@ -232,10 +233,20 @@ public function testDeleteShareLocked(): void {
$this->expectExceptionMessage('Could not delete share');

$node = $this->getMockBuilder(File::class)->getMock();
$node->method('getId')->willReturn(1);

$share = $this->newShare();
$share->setNode($node);

$userFolder = $this->getMockBuilder(Folder::class)->getMock();
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);

$userFolder->method('getFirstNodeById')
->with($share->getNodeId())
->willReturn($node);

$this->shareManager
->expects($this->once())
->method('getShareById')
Expand Down Expand Up @@ -476,6 +487,62 @@ public function testDeleteSharedWithGroupIDontBelongTo(): void {
$this->ocs->deleteShare(42);
}

public function testDeleteShareCoOwner(): void {
$ocs = $this->mockFormatShare();

$mount = $this->createMock(IShareCoOwnerMount::class);

$file = $this->createMock(File::class);
$file
->expects($this->exactly(2))
->method('getPermissions')
->willReturn(Constants::PERMISSION_SHARE);
$file
->expects($this->once())
->method('getMountPoint')
->willReturn($mount);

$userFolder = $this->createMock(Folder::class);
$userFolder
->expects($this->exactly(2))
->method('getFirstNodeById')
->with(2)
->willReturn($file);

$this->rootFolder
->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);

$share = $this->createMock(IShare::class);
$share
->expects($this->once())
->method('getNode')
->willReturn($file);
$share
->expects($this->exactly(2))
->method('getNodeId')
->willReturn(2);
$share
->expects($this->exactly(2))
->method('getPermissions')
->willReturn(Constants::PERMISSION_SHARE);

$this->shareManager
->expects($this->once())
->method('getShareById')
->with('ocinternal:1', $this->currentUser)
->willReturn($share);

$this->shareManager
->expects($this->once())
->method('deleteShare')
->with($share);

$result = $ocs->deleteShare(1);
$this->assertInstanceOf(DataResponse::class, $result);
}

/*
* FIXME: Enable once we have a federated Share Provider

Expand Down Expand Up @@ -3748,6 +3815,63 @@ public function testUpdateShareCanIncreasePermissionsIfOwner(): void {
$this->assertInstanceOf(DataResponse::class, $result);
}

public function testUpdateShareCoOwner(): void {
$ocs = $this->mockFormatShare();

$mount = $this->createMock(IShareCoOwnerMount::class);

$file = $this->createMock(File::class);
$file
->expects($this->exactly(2))
->method('getPermissions')
->willReturn(Constants::PERMISSION_SHARE);
$file
->expects($this->once())
->method('getMountPoint')
->willReturn($mount);

$userFolder = $this->createMock(Folder::class);
$userFolder
->expects($this->exactly(2))
->method('getFirstNodeById')
->with(2)
->willReturn($file);

$this->rootFolder
->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);

$share = $this->createMock(IShare::class);
$share
->expects($this->once())
->method('getNode')
->willReturn($file);
$share
->expects($this->exactly(2))
->method('getNodeId')
->willReturn(2);
$share
->expects($this->exactly(2))
->method('getPermissions')
->willReturn(Constants::PERMISSION_SHARE);

$this->shareManager
->expects($this->once())
->method('getShareById')
->with('ocinternal:1', $this->currentUser)
->willReturn($share);

$this->shareManager
->expects($this->once())
->method('updateShare')
->with($share)
->willReturn($share);

$result = $ocs->updateShare(1, Constants::PERMISSION_ALL);
$this->assertInstanceOf(DataResponse::class, $result);
}

public function dataFormatShare() {
$file = $this->getMockBuilder(File::class)->getMock();
$folder = $this->getMockBuilder(Folder::class)->getMock();
Expand Down
2 changes: 2 additions & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@
'OCP\\Files\\Mount\\IMountManager' => $baseDir . '/lib/public/Files/Mount/IMountManager.php',
'OCP\\Files\\Mount\\IMountPoint' => $baseDir . '/lib/public/Files/Mount/IMountPoint.php',
'OCP\\Files\\Mount\\IMovableMount' => $baseDir . '/lib/public/Files/Mount/IMovableMount.php',
'OCP\\Files\\Mount\\IShareCoOwnerMount' => $baseDir . '/lib/public/Files/Mount/IShareCoOwnerMount.php',
'OCP\\Files\\Mount\\ISystemMountPoint' => $baseDir . '/lib/public/Files/Mount/ISystemMountPoint.php',
'OCP\\Files\\Node' => $baseDir . '/lib/public/Files/Node.php',
'OCP\\Files\\NotEnoughSpaceException' => $baseDir . '/lib/public/Files/NotEnoughSpaceException.php',
Expand Down Expand Up @@ -1812,6 +1813,7 @@
'OC\\Repair\\ClearGeneratedAvatarCache' => $baseDir . '/lib/private/Repair/ClearGeneratedAvatarCache.php',
'OC\\Repair\\ClearGeneratedAvatarCacheJob' => $baseDir . '/lib/private/Repair/ClearGeneratedAvatarCacheJob.php',
'OC\\Repair\\Collation' => $baseDir . '/lib/private/Repair/Collation.php',
'OC\\Repair\\DeleteInvalidShares' => $baseDir . '/lib/private/Repair/DeleteInvalidShares.php',
'OC\\Repair\\Events\\RepairAdvanceEvent' => $baseDir . '/lib/private/Repair/Events/RepairAdvanceEvent.php',
'OC\\Repair\\Events\\RepairErrorEvent' => $baseDir . '/lib/private/Repair/Events/RepairErrorEvent.php',
'OC\\Repair\\Events\\RepairFinishEvent' => $baseDir . '/lib/private/Repair/Events/RepairFinishEvent.php',
Expand Down
2 changes: 2 additions & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\Files\\Mount\\IMountManager' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/IMountManager.php',
'OCP\\Files\\Mount\\IMountPoint' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/IMountPoint.php',
'OCP\\Files\\Mount\\IMovableMount' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/IMovableMount.php',
'OCP\\Files\\Mount\\IShareCoOwnerMount' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/IShareCoOwnerMount.php',
'OCP\\Files\\Mount\\ISystemMountPoint' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/ISystemMountPoint.php',
'OCP\\Files\\Node' => __DIR__ . '/../../..' . '/lib/public/Files/Node.php',
'OCP\\Files\\NotEnoughSpaceException' => __DIR__ . '/../../..' . '/lib/public/Files/NotEnoughSpaceException.php',
Expand Down Expand Up @@ -1853,6 +1854,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\Repair\\ClearGeneratedAvatarCache' => __DIR__ . '/../../..' . '/lib/private/Repair/ClearGeneratedAvatarCache.php',
'OC\\Repair\\ClearGeneratedAvatarCacheJob' => __DIR__ . '/../../..' . '/lib/private/Repair/ClearGeneratedAvatarCacheJob.php',
'OC\\Repair\\Collation' => __DIR__ . '/../../..' . '/lib/private/Repair/Collation.php',
'OC\\Repair\\DeleteInvalidShares' => __DIR__ . '/../../..' . '/lib/private/Repair/DeleteInvalidShares.php',
'OC\\Repair\\Events\\RepairAdvanceEvent' => __DIR__ . '/../../..' . '/lib/private/Repair/Events/RepairAdvanceEvent.php',
'OC\\Repair\\Events\\RepairErrorEvent' => __DIR__ . '/../../..' . '/lib/private/Repair/Events/RepairErrorEvent.php',
'OC\\Repair\\Events\\RepairFinishEvent' => __DIR__ . '/../../..' . '/lib/private/Repair/Events/RepairFinishEvent.php',
Expand Down
2 changes: 2 additions & 0 deletions lib/private/Repair.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use OC\Repair\ClearFrontendCaches;
use OC\Repair\ClearGeneratedAvatarCache;
use OC\Repair\Collation;
use OC\Repair\DeleteInvalidShares;
use OC\Repair\Events\RepairAdvanceEvent;
use OC\Repair\Events\RepairErrorEvent;
use OC\Repair\Events\RepairFinishEvent;
Expand Down Expand Up @@ -213,6 +214,7 @@ public static function getExpensiveRepairSteps() {
),
\OC::$server->get(ValidatePhoneNumber::class),
\OC::$server->get(DeleteSchedulingObjects::class),
new DeleteInvalidShares(),
];
}

Expand Down
45 changes: 45 additions & 0 deletions lib/private/Repair/DeleteInvalidShares.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OC\Repair;

use OC\Share20\Manager;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;
use OCP\Server;
use OCP\Share;
use OCP\Share\Exceptions\ShareNotFound;

/**
* Deletes shares with invalid data
*/
class DeleteInvalidShares implements IRepairStep {
public function getName() {
return 'Delete invalid shares';
}

private function checkShares(IOutput $output) {
/** @var Manager $shareManager */
$shareManager = Server::get(Share\IManager::class);
$deleted = 0;
/** @var Share\IShare $share */
foreach ($shareManager->getAllShares() as $share) {
try {
$shareManager->checkShare($share);
} catch (ShareNotFound $e) {
$deleted++;
}
}

if ($deleted > 0) {
$output->info('Removed ' . $deleted . ' invalid shares');
}
}

public function run(IOutput $output) {
$this->checkShares($output);
}
}
3 changes: 2 additions & 1 deletion lib/private/Share20/DefaultShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,8 @@ public function getSharesByPath(Node $path) {
->andWhere(
$qb->expr()->orX(
$qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USER)),
$qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))
$qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP)),
$qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_LINK)),
)
)
->andWhere($qb->expr()->orX(
Expand Down
Loading
Loading