From 34fc04d5ed41ab15f882934d341aa41ceb7c3590 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 15 Oct 2024 10:36:43 +0200 Subject: [PATCH 1/9] feat(Files\Mount): Add IShareCoOwnerMount Signed-off-by: provokateurin --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/public/Files/Mount/IShareCoOwnerMount.php | 18 ++++++++++++++++++ 3 files changed, 20 insertions(+) create mode 100644 lib/public/Files/Mount/IShareCoOwnerMount.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 352c3c11a9ed0..db5bbe14b204a 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 13b13c5ae6f19..74186720355e4 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -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', diff --git a/lib/public/Files/Mount/IShareCoOwnerMount.php b/lib/public/Files/Mount/IShareCoOwnerMount.php new file mode 100644 index 0000000000000..da05b92a4c9bd --- /dev/null +++ b/lib/public/Files/Mount/IShareCoOwnerMount.php @@ -0,0 +1,18 @@ + Date: Tue, 22 Oct 2024 13:18:17 +0200 Subject: [PATCH 2/9] fix(Share20\DefaultShareProvider): Return link shares in getSharesByPath() Signed-off-by: provokateurin --- lib/private/Share20/DefaultShareProvider.php | 3 +- .../lib/Share20/DefaultShareProviderTest.php | 85 +++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index db2734190e4df..388596092b5d6 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -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( diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index 0351504e1a7cb..394ca41c1ae59 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -7,6 +7,7 @@ namespace Test\Share20; +use OC\Files\Node\Node; use OC\Share20\DefaultShareProvider; use OC\Share20\ShareAttributes; use OCP\AppFramework\Utility\ITimeFactory; @@ -3013,4 +3014,88 @@ public function testGetAllShares(): void { $this->assertEquals('token5', $share->getToken()); $this->assertEquals('myTarget5', $share->getTarget()); } + + + public function testGetSharesByPath(): void { + $qb = $this->dbConn->getQueryBuilder(); + + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_USER), + 'uid_owner' => $qb->expr()->literal('user1'), + 'uid_initiator' => $qb->expr()->literal('user1'), + 'share_with' => $qb->expr()->literal('user2'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal(1), + ]); + $qb->execute(); + + $id1 = $qb->getLastInsertId(); + + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_GROUP), + 'uid_owner' => $qb->expr()->literal('user1'), + 'uid_initiator' => $qb->expr()->literal('user1'), + 'share_with' => $qb->expr()->literal('user2'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal(1), + ]); + $qb->execute(); + + $id2 = $qb->getLastInsertId(); + + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_LINK), + 'uid_owner' => $qb->expr()->literal('user1'), + 'uid_initiator' => $qb->expr()->literal('user1'), + 'share_with' => $qb->expr()->literal('user2'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal(1), + ]); + $qb->execute(); + + $id3 = $qb->getLastInsertId(); + + $ownerPath1 = $this->createMock(File::class); + $shareOwner1Folder = $this->createMock(Folder::class); + $shareOwner1Folder->method('getFirstNodeById')->willReturn($ownerPath1); + + $ownerPath2 = $this->createMock(File::class); + $shareOwner2Folder = $this->createMock(Folder::class); + $shareOwner2Folder->method('getFirstNodeById')->willReturn($ownerPath2); + + $ownerPath3 = $this->createMock(File::class); + $shareOwner3Folder = $this->createMock(Folder::class); + $shareOwner3Folder->method('getFirstNodeById')->willReturn($ownerPath3); + + $this->rootFolder + ->method('getUserFolder') + ->willReturnMap( + [ + ['shareOwner1', $shareOwner1Folder], + ['shareOwner2', $shareOwner2Folder], + ['shareOwner3', $shareOwner3Folder], + ] + ); + + $node = $this->createMock(Node::class); + $node + ->expects($this->once()) + ->method('getId') + ->willReturn(1); + + $shares = $this->provider->getSharesByPath($node); + $this->assertCount(3, $shares); + + $this->assertEquals($id1, $shares[0]->getId()); + $this->assertEquals(IShare::TYPE_USER, $shares[0]->getShareType()); + + $this->assertEquals($id2, $shares[1]->getId()); + $this->assertEquals(IShare::TYPE_GROUP, $shares[1]->getShareType()); + + $this->assertEquals($id3, $shares[2]->getId()); + $this->assertEquals(IShare::TYPE_LINK, $shares[2]->getShareType()); + } } From 7593ce90accb0fd600d72dd4d468e34e0b9e16d3 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 22 Oct 2024 13:19:18 +0200 Subject: [PATCH 3/9] feat(Share20\Manager): Return all shares on IShareCoOwnerMounts Signed-off-by: provokateurin --- lib/private/Share20/Manager.php | 41 +++++++++++++----- tests/lib/Share20/ManagerTest.php | 69 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 11 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 4856c051307a9..ec47169eae5b7 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -17,6 +17,7 @@ use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountManager; +use OCP\Files\Mount\IShareCoOwnerMount; use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\HintException; @@ -1215,17 +1216,26 @@ public function getSharesInFolder($userId, Folder $node, $reshares = false, $sha throw new \Exception('non-shallow getSharesInFolder is no longer supported'); } - return array_reduce($providers, function ($shares, IShareProvider $provider) use ($userId, $node, $reshares) { - $newShares = $provider->getSharesInFolder($userId, $node, $reshares); - foreach ($newShares as $fid => $data) { - if (!isset($shares[$fid])) { - $shares[$fid] = []; - } + $isCoOwner = $node->getMountPoint() instanceof IShareCoOwnerMount; - $shares[$fid] = array_merge($shares[$fid], $data); + $shares = []; + foreach ($providers as $provider) { + if ($isCoOwner) { + foreach ($node->getDirectoryListing() as $childNode) { + $data = $provider->getSharesByPath($childNode); + $fid = $childNode->getId(); + $shares[$fid] ??= []; + $shares[$fid] = array_merge($shares[$fid], $data); + } + } else { + foreach ($provider->getSharesInFolder($userId, $node, $reshares) as $fid => $data) { + $shares[$fid] ??= []; + $shares[$fid] = array_merge($shares[$fid], $data); + } } - return $shares; - }, []); + } + + return $shares; } /** @@ -1244,7 +1254,11 @@ public function getSharesBy($userId, $shareType, $path = null, $reshares = false return []; } - $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + if ($path !== null && $path->getMountPoint() instanceof IShareCoOwnerMount) { + $shares = array_filter($provider->getSharesByPath($path), static fn (IShare $share) => $share->getShareType() === $shareType); + } else { + $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + } /* * Work around so we don't return expired shares but still follow @@ -1288,7 +1302,12 @@ public function getSharesBy($userId, $shareType, $path = null, $reshares = false $offset += $added; // Fetch again $limit shares - $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + if ($path !== null && $path->getMountPoint() instanceof IShareCoOwnerMount) { + // We already fetched all shares, so end here + $shares = []; + } else { + $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + } // No more shares means we are done if (empty($shares)) { diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index a4c1dd3803d2e..918a5c616e5f0 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -23,6 +23,7 @@ use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountManager; use OCP\Files\Mount\IMountPoint; +use OCP\Files\Mount\IShareCoOwnerMount; use OCP\Files\Node; use OCP\Files\Storage\IStorage; use OCP\HintException; @@ -2895,6 +2896,31 @@ public function testGetSharesBy(): void { $this->assertSame($share, $shares[0]); } + public function testGetSharesByCoOwner(): void { + $mount = $this->createMock(IShareCoOwnerMount::class); + + $node = $this->createMock(Folder::class); + $node + ->expects($this->once()) + ->method('getMountPoint') + ->willReturn($mount); + + $share = $this->manager->newShare(); + $share->setNode($node); + $share->setShareType(IShare::TYPE_USER); + + $this->defaultProvider + ->expects($this->once()) + ->method('getSharesByPath') + ->with($this->equalTo($node)) + ->willReturn([$share]); + + $shares = $this->manager->getSharesBy('user', IShare::TYPE_USER, $node, true, 1, 1); + + $this->assertCount(1, $shares); + $this->assertSame($share, $shares[0]); + } + /** * Test to ensure we correctly remove expired link shares * @@ -4493,6 +4519,49 @@ public function testGetSharesInFolder(): void { $this->assertSame($expects, $result); } + public function testGetSharesInFolderCoOwner(): void { + $factory = new DummyFactory2($this->createMock(IServerContainer::class)); + + $manager = $this->createManager($factory); + + $factory->setProvider($this->defaultProvider); + $extraProvider = $this->createMock(IShareProvider::class); + $factory->setSecondProvider($extraProvider); + + $share1 = $this->createMock(IShare::class); + $share2 = $this->createMock(IShare::class); + + $mount = $this->createMock(IShareCoOwnerMount::class); + + $file = $this->createMock(File::class); + $file + ->method('getId') + ->willReturn(1); + + $folder = $this->createMock(Folder::class); + $folder + ->method('getMountPoint') + ->willReturn($mount); + $folder + ->method('getDirectoryListing') + ->willReturn([$file]); + + $this->defaultProvider + ->method('getSharesByPath') + ->with($file) + ->willReturn([$share1]); + + $extraProvider + ->method('getSharesByPath') + ->with($file) + ->willReturn([$share2]); + + $this->assertSame([ + 1 => [$share1, $share2], + ], $manager->getSharesInFolder('user', $folder)); + } + + public function testGetAccessList(): void { $factory = new DummyFactory2($this->createMock(IServerContainer::class)); From 90436a1bf901e459efe1e4166aa6838f88f67d87 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 15 Oct 2024 09:26:50 +0200 Subject: [PATCH 4/9] fix(files_sharing): Remove duplicate link/email edit share logic Signed-off-by: provokateurin --- .../lib/Controller/ShareAPIController.php | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 71f73f777a5d9..b88f0357dfbb3 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -1235,18 +1235,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') { From 1b9728698cb71668ea72311c0fdb6db4705832ca Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 15 Oct 2024 11:08:21 +0200 Subject: [PATCH 5/9] feat(files_sharing): Allow users with share permission to manage shares on IShareCoOwnerMount Signed-off-by: provokateurin --- .../lib/Controller/ShareAPIController.php | 15 ++- .../Controller/ShareAPIControllerTest.php | 124 ++++++++++++++++++ 2 files changed, 138 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index b88f0357dfbb3..100b6a87d30d7 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -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; @@ -1476,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; } @@ -1541,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)) { + 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! @@ -1576,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; } diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 768bebe0271a4..a0657d82bb108 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -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; @@ -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') @@ -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 @@ -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(); From 1edd2a70887d1fa9327bb80aa65a62ebc182ea91 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 4 Nov 2024 12:11:48 +0100 Subject: [PATCH 6/9] fix(Share20\Manager): Ensure node is still accessible when checking share Signed-off-by: provokateurin --- lib/private/Share20/Manager.php | 10 +++++++ tests/lib/Share20/ManagerTest.php | 45 ++++++++++++++++++++++++++++--- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index ec47169eae5b7..2b140edb8b756 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1488,6 +1488,16 @@ protected function checkShare(IShare $share): void { $this->deleteShare($share); throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); } + + try { + $share->getNode(); + // Ignore share, file is still accessible + } catch (NotFoundException) { + // Access lost + $this->deleteShare($share); + throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); + } + if ($this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'no') === 'yes') { $uids = array_unique([$share->getShareOwner(),$share->getSharedBy()]); foreach ($uids as $uid) { diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 918a5c616e5f0..33188d5a0fe3c 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -25,6 +25,7 @@ use OCP\Files\Mount\IMountPoint; use OCP\Files\Mount\IShareCoOwnerMount; use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\Files\Storage\IStorage; use OCP\HintException; use OCP\IConfig; @@ -667,6 +668,36 @@ public function testGetShareById(): void { } + public function testGetShareByIdNodeAccessible(): void { + $share = $this->createMock(IShare::class); + $share + ->expects($this->exactly(2)) + ->method('getNode') + ->willThrowException(new NotFoundException()); + + $this->defaultProvider + ->expects($this->once()) + ->method('getShareById') + ->with(42) + ->willReturn($share); + + $this->defaultProvider + ->expects($this->once()) + ->method('getChildren') + ->with($share) + ->willReturn([]); + + + $this->defaultProvider + ->expects($this->once()) + ->method('delete') + ->with($share); + + $this->expectException(ShareNotFound::class); + $this->manager->getShareById('default:42'); + } + + public function testGetExpiredShareById(): void { $this->expectException(\OCP\Share\Exceptions\ShareNotFound::class); @@ -2875,10 +2906,11 @@ public function testCreateShareOfIncomingFederatedShare(): void { } public function testGetSharesBy(): void { - $share = $this->manager->newShare(); - $node = $this->createMock(Folder::class); + $share = $this->manager->newShare(); + $share->setNode($node); + $this->defaultProvider->expects($this->once()) ->method('getSharesBy') ->with( @@ -2930,6 +2962,8 @@ public function testGetSharesByCoOwner(): void { * deleted (as they are evaluated). but share 8 should still be there. */ public function testGetSharesByExpiredLinkShares(): void { + $node = $this->createMock(File::class); + $manager = $this->createManagerMock() ->setMethods(['deleteShare']) ->getMock(); @@ -2943,6 +2977,7 @@ public function testGetSharesByExpiredLinkShares(): void { for ($i = 0; $i < 8; $i++) { $share = $this->manager->newShare(); $share->setId($i); + $share->setNode($node); $shares[] = $share; } @@ -2963,8 +2998,6 @@ public function testGetSharesByExpiredLinkShares(): void { $shares2[] = clone $shares[$i]; } - $node = $this->createMock(File::class); - /* * Simulate the getSharesBy call. */ @@ -3126,8 +3159,10 @@ public function testGetShareByTokenHideDisabledUser(): void { $date = new \DateTime(); $date->setTime(0, 0, 0); $date->add(new \DateInterval('P2D')); + $node = $this->createMock(File::class); $share = $this->manager->newShare(); $share->setExpirationDate($date); + $share->setNode($node); $share->setShareOwner('owner'); $share->setSharedBy('sharedBy'); @@ -3205,8 +3240,10 @@ public function testGetShareByTokenNotExpired(): void { $date = new \DateTime(); $date->setTime(0, 0, 0); $date->add(new \DateInterval('P2D')); + $node = $this->createMock(Folder::class); $share = $this->manager->newShare(); $share->setExpirationDate($date); + $share->setNode($node); $this->defaultProvider->expects($this->once()) ->method('getShareByToken') From c7a1c1f0c4fa8ba336b48d975f7512e7f7a53c29 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 4 Nov 2024 11:55:09 +0100 Subject: [PATCH 7/9] feat(Repair): Delete all invalid shares Signed-off-by: provokateurin --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Repair.php | 2 + lib/private/Repair/DeleteInvalidShares.php | 45 +++++++++ lib/private/Share20/Manager.php | 2 +- lib/public/Share/IManager.php | 4 +- tests/lib/Repair/DeleteInvalidSharesTest.php | 100 +++++++++++++++++++ 7 files changed, 152 insertions(+), 3 deletions(-) create mode 100644 lib/private/Repair/DeleteInvalidShares.php create mode 100644 tests/lib/Repair/DeleteInvalidSharesTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index db5bbe14b204a..22a1067e16279 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1813,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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 74186720355e4..aa4c1e44e5bae 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1854,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', diff --git a/lib/private/Repair.php b/lib/private/Repair.php index b1a824ba5e324..661bda6b0af65 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -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; @@ -213,6 +214,7 @@ public static function getExpensiveRepairSteps() { ), \OC::$server->get(ValidatePhoneNumber::class), \OC::$server->get(DeleteSchedulingObjects::class), + new DeleteInvalidShares(), ]; } diff --git a/lib/private/Repair/DeleteInvalidShares.php b/lib/private/Repair/DeleteInvalidShares.php new file mode 100644 index 0000000000000..fd9892e0597da --- /dev/null +++ b/lib/private/Repair/DeleteInvalidShares.php @@ -0,0 +1,45 @@ +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); + } +} diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 2b140edb8b756..04e7ee392a514 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1483,7 +1483,7 @@ public function getShareByToken($token) { * * @throws ShareNotFound */ - protected function checkShare(IShare $share): void { + public function checkShare(IShare $share): void { if ($share->isExpired()) { $this->deleteShare($share); throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index b07bc8f80515e..79b1148e44dac 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -513,9 +513,9 @@ public function registerShareProvider(string $shareProviderClass): void; * * Get all the shares as iterable to reduce memory overhead * Note, since this opens up database cursors the iterable should - * be fully itterated. + * be fully iterated. * - * @return iterable + * @return iterable * @since 18.0.0 */ public function getAllShares(): iterable; diff --git a/tests/lib/Repair/DeleteInvalidSharesTest.php b/tests/lib/Repair/DeleteInvalidSharesTest.php new file mode 100644 index 0000000000000..84da15a767abb --- /dev/null +++ b/tests/lib/Repair/DeleteInvalidSharesTest.php @@ -0,0 +1,100 @@ +connection = Server::get(IDBConnection::class); + $this->deleteAllShares(); + + $this->repair = new DeleteInvalidShares(); + } + + protected function tearDown(): void { + $this->deleteAllShares(); + + parent::tearDown(); + } + + protected function deleteAllShares(): void { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('share')->executeStatement(); + } + + public function testSharesInaccessibleNode(): void { + // The dummy user will not have the node as we never even create it for the test + $this->createUser('user1', 'user1'); + + $qb = $this->connection->getQueryBuilder(); + $shareValues = [ + 'share_type' => $qb->expr()->literal(IShare::TYPE_USER), + 'share_with' => $qb->expr()->literal('user2'), + 'uid_owner' => $qb->expr()->literal('user1'), + 'uid_initiator' => $qb->expr()->literal('user1'), + 'item_type' => $qb->expr()->literal('file'), + 'item_source' => $qb->expr()->literal(123), + 'item_target' => $qb->expr()->literal('/123'), + 'file_source' => $qb->expr()->literal(123), + 'file_target' => $qb->expr()->literal('/123'), + ]; + + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share') + ->values($shareValues) + ->executeStatement(); + $id = $qb->getLastInsertId(); + + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->executeQuery(); + $rows = $result->fetchAll(); + $this->assertEquals([['id' => $id]], $rows); + $result->closeCursor(); + + $outputMock = $this->getMockBuilder(IOutput::class) + ->disableOriginalConstructor() + ->getMock(); + $outputMock + ->expects($this->once()) + ->method('info') + ->with('Removed 1 invalid shares'); + + $this->repair->run($outputMock); + + $query = $this->connection->getQueryBuilder(); + $result = $query->select('id') + ->from('share') + ->executeQuery(); + $rows = $result->fetchAll(); + $this->assertEquals([], $rows); + $result->closeCursor(); + } +} From 23850f6717dc140bf1d64158a82c199effbd0d9f Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 14 Nov 2024 07:41:41 +0100 Subject: [PATCH 8/9] fix(files_sharing,files): Do not validate shares before adjusting the owner Signed-off-by: provokateurin --- .../lib/Service/OwnershipTransferService.php | 4 ++-- apps/files_sharing/lib/Updater.php | 18 ++++++++------ lib/private/Share20/Manager.php | 24 +++++++++++-------- lib/public/Share/IManager.php | 9 ++++--- 4 files changed, 33 insertions(+), 22 deletions(-) diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php index 80c61d39ef5b5..71764019abf3e 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -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; @@ -464,7 +464,7 @@ private function restoreShares( } $share->setNodeId($newNodeId); - $this->shareManager->updateShare($share); + $this->shareManager->updateShare($share, onlyValid: false); } } } catch (NotFoundException $e) { diff --git a/apps/files_sharing/lib/Updater.php b/apps/files_sharing/lib/Updater.php index 82075fb9ba5ca..226cf3e7fd489 100644 --- a/apps/files_sharing/lib/Updater.php +++ b/apps/files_sharing/lib/Updater.php @@ -50,10 +50,14 @@ 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); @@ -61,9 +65,9 @@ private static function moveShareInOrOutOfShare($path): void { $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); @@ -111,7 +115,7 @@ private static function moveShareInOrOutOfShare($path): void { $share->setShareOwner($newOwner); $share->setPermissions($newPermissions); - $shareManager->updateShare($share); + $shareManager->updateShare($share, onlyValid: false); } } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 04e7ee392a514..02916c87ae1ce 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -786,13 +786,13 @@ public function createShare(IShare $share) { * @throws \InvalidArgumentException * @throws GenericShareException */ - public function updateShare(IShare $share) { + public function updateShare(IShare $share, bool $onlyValid = true) { $expirationDateUpdated = false; $this->canShare($share); try { - $originalShare = $this->getShareById($share->getFullId()); + $originalShare = $this->getShareById($share->getFullId(), onlyValid: $onlyValid); } catch (\UnexpectedValueException $e) { throw new \InvalidArgumentException($this->l->t('Share does not have a full ID')); } @@ -1241,7 +1241,7 @@ public function getSharesInFolder($userId, Folder $node, $reshares = false, $sha /** * @inheritdoc */ - public function getSharesBy($userId, $shareType, $path = null, $reshares = false, $limit = 50, $offset = 0) { + public function getSharesBy($userId, $shareType, $path = null, $reshares = false, $limit = 50, $offset = 0, bool $onlyValid = true) { if ($path !== null && !($path instanceof \OCP\Files\File) && !($path instanceof \OCP\Files\Folder)) { @@ -1270,11 +1270,13 @@ public function getSharesBy($userId, $shareType, $path = null, $reshares = false while (true) { $added = 0; foreach ($shares as $share) { - try { - $this->checkShare($share); - } catch (ShareNotFound $e) { - // Ignore since this basically means the share is deleted - continue; + if ($onlyValid) { + try { + $this->checkShare($share); + } catch (ShareNotFound $e) { + // Ignore since this basically means the share is deleted + continue; + } } $added++; @@ -1366,7 +1368,7 @@ public function getDeletedSharedWith($userId, $shareType, $node = null, $limit = /** * @inheritdoc */ - public function getShareById($id, $recipient = null) { + public function getShareById($id, $recipient = null, bool $onlyValid = true) { if ($id === null) { throw new ShareNotFound(); } @@ -1381,7 +1383,9 @@ public function getShareById($id, $recipient = null) { $share = $provider->getShareById($id, $recipient); - $this->checkShare($share); + if ($onlyValid) { + $this->checkShare($share); + } return $share; } diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index 79b1148e44dac..f3d4c2f56dc5f 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -41,11 +41,12 @@ public function createShare(IShare $share); * The state can't be changed this way: use acceptShare * * @param IShare $share + * @param bool $onlyValid Only updates valid shares, invalid shares will be deleted automatically and are not updated * @return IShare The share object * @throws \InvalidArgumentException * @since 9.0.0 */ - public function updateShare(IShare $share); + public function updateShare(IShare $share, bool $onlyValid = true); /** * Accept a share. @@ -127,10 +128,11 @@ public function getSharesInFolder($userId, Folder $node, $reshares = false, $sha * @param bool $reshares * @param int $limit The maximum number of returned results, -1 for all results * @param int $offset + * @param bool $onlyValid Only returns valid shares, invalid shares will be deleted automatically and are not returned * @return IShare[] * @since 9.0.0 */ - public function getSharesBy($userId, $shareType, $path = null, $reshares = false, $limit = 50, $offset = 0); + public function getSharesBy($userId, $shareType, $path = null, $reshares = false, $limit = 50, $offset = 0, bool $onlyValid = true); /** * Get shares shared with $user. @@ -168,11 +170,12 @@ public function getDeletedSharedWith($userId, $shareType, $node = null, $limit = * * @param string $id * @param string|null $recipient userID of the recipient + * @param bool $onlyValid Only returns valid shares, invalid shares will be deleted automatically and are not returned * @return IShare * @throws ShareNotFound * @since 9.0.0 */ - public function getShareById($id, $recipient = null); + public function getShareById($id, $recipient = null, bool $onlyValid = true); /** * Get the share by token possible with password From cd4dc541e77b40b23f368597e9ba0b4b5e1ee64a Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 4 Nov 2024 12:29:40 +0100 Subject: [PATCH 9/9] fix(SharingEntry): Display owner name if the owner is someone else Signed-off-by: provokateurin --- apps/files_sharing/src/components/SharingEntry.vue | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apps/files_sharing/src/components/SharingEntry.vue b/apps/files_sharing/src/components/SharingEntry.vue index e97ff332422ec..13b2e51e86170 100644 --- a/apps/files_sharing/src/components/SharingEntry.vue +++ b/apps/files_sharing/src/components/SharingEntry.vue @@ -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}', { + initiator: this.share.ownerDisplayName, + }) + } return title }, tooltip() {