From e84fc69b03830f0a40f342fe4b443263abe3aa7e Mon Sep 17 00:00:00 2001 From: Hamza Date: Fri, 21 Aug 2026 13:21:40 +0200 Subject: [PATCH] fix: keep share visible for share owner when iinitiator is disactivated Signed-off-by: Hamza --- lib/private/Share20/Manager.php | 67 ++++++--- tests/lib/Share20/ManagerTest.php | 219 ++++++++++++++++++++++++++++++ 2 files changed, 270 insertions(+), 16 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 703087a53a405..907834f3c693d 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -625,12 +625,23 @@ public function createShare(IShare $share): IShare { $share->setTarget($target); } } catch (AlreadySharedException $e) { + $existingShare = $e->getExistingShare(); + + /* + * Reusing a share which is hidden because one of the accounts involved + * in it is disabled would leave the recipient without access, so report + * it to the sharer instead. Such a share has to be deleted first. + */ + if ($this->isHiddenDisabledUserShare($existingShare)) { + throw $e; + } + // If a share for the same target already exists, dont create a new one, // but do trigger the hooks and notifications again $oldShare = $share; // Reuse the node we already have - $share = $e->getExistingShare(); + $share = $existingShare; $share->setNode($oldShare->getNode()); } @@ -1151,7 +1162,7 @@ public function getSharesBy(string $userId, int $shareType, ?Node $path = null, return []; } - if ($onlyValid && $this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'no') === 'yes') { + if ($onlyValid && $this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'yes') === 'yes') { /* * If shares from disabled users are hidden, check user status first to avoid useless work. * Otherwise all shares would’ve been filtered out by checkShare anyway. @@ -1181,7 +1192,7 @@ public function getSharesBy(string $userId, int $shareType, ?Node $path = null, $added++; if ($onlyValid) { try { - $this->checkShare($share, $added); + $this->checkShare($share, $added, $userId); } catch (ShareNotFound $e) { // Ignore since this basically means the share is deleted continue; @@ -1243,7 +1254,7 @@ public function getSharedWith(string $userId, int $shareType, ?Node $node = null // remove all shares which are already expired foreach ($shares as $key => $share) { try { - $this->checkShare($share); + $this->checkShare($share, viewer: $userId); } catch (ShareNotFound $e) { unset($shares[$key]); } @@ -1282,10 +1293,10 @@ public function getSharedWithByPath(string $userId, int $shareType, string $path $shares = new \IteratorIterator($shares); } - return new \CallbackFilterIterator($shares, function (IShare $share) { + return new \CallbackFilterIterator($shares, function (IShare $share) use ($userId) { // remove all shares which are already expired try { - $this->checkShare($share); + $this->checkShare($share, viewer: $userId); return true; } catch (ShareNotFound $e) { return false; @@ -1319,7 +1330,7 @@ public function getShareById($id, $recipient = null, bool $onlyValid = true): IS $share = $provider->getShareById($id, $recipient); if ($onlyValid) { - $this->checkShare($share); + $this->checkShare($share, viewer: $recipient); } return $share; @@ -1403,27 +1414,51 @@ public function getShareByToken(string $token): IShare { return $share; } + /** + * Whether a share has to be hidden because one of the accounts involved in it + * is disabled. + * + * A share initiated by an account that got disabled stays visible to the owner + * of the shared item, so that they can still manage or delete it. + * + * @param ?string $viewer Account the share is checked for, if known + */ + private function isHiddenDisabledUserShare(IShare $share, ?string $viewer = null): bool { + if ($this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'yes') !== 'yes') { + return false; + } + + $uids = [$share->getShareOwner()]; + if ($viewer !== $share->getShareOwner()) { + $uids[] = $share->getSharedBy(); + } + + foreach (array_unique($uids) as $uid) { + $user = $this->userManager->get($uid); + if ($user?->isEnabled() === false) { + return true; + } + } + + return false; + } + /** * Check expire date and disabled owner * * @param int &$added If given, will be decremented if the share is deleted + * @param ?string $viewer Account the share is checked for, defaults to the account of the current session * @throws ShareNotFound */ - private function checkShare(IShare $share, int &$added = 1): void { + private function checkShare(IShare $share, int &$added = 1, ?string $viewer = null): void { if ($share->isExpired()) { $this->deleteShare($share); // Remove 1 to added, because this share was deleted $added--; throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); } - if ($this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'yes') === 'yes') { - $uids = array_unique([$share->getShareOwner(), $share->getSharedBy()]); - foreach ($uids as $uid) { - $user = $this->userManager->get($uid); - if ($user?->isEnabled() === false) { - throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); - } - } + if ($this->isHiddenDisabledUserShare($share, $viewer ?? $this->userSession->getUser()?->getUID())) { + throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); } // For link and email shares, verify the share owner can still create such shares diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 68bfffe94220c..d9678404b69f4 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -813,6 +813,61 @@ public function testGetShareById(): void { $this->assertEquals($share, $this->manager->getShareById('default:42')); } + public function testGetShareByIdOfDisabledInitiatorAsShareOwner(): void { + $this->config->method('getAppValue') + ->willReturnMap([ + ['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'], + ]); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_USER) + ->setShareOwner('owner') + ->setSharedBy('initiator') + ->setSharedWith('recipient'); + + $this->userManager->method('get')->willReturnMap([ + ['owner', $this->createEnabledUser(true)], + ['initiator', $this->createEnabledUser(false)], + ]); + + $this->defaultProvider + ->expects($this->once()) + ->method('getShareById') + ->with('42', 'owner') + ->willReturn($share); + + $this->assertSame($share, $this->manager->getShareById('default:42', 'owner')); + } + + public function testGetShareByIdOfDisabledInitiatorAsOtherUser(): void { + $this->expectException(ShareNotFound::class); + $this->expectExceptionMessage('The requested share does not exist anymore'); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'], + ]); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_USER) + ->setShareOwner('owner') + ->setSharedBy('initiator') + ->setSharedWith('recipient'); + + $this->userManager->method('get')->willReturnMap([ + ['owner', $this->createEnabledUser(true)], + ['initiator', $this->createEnabledUser(false)], + ]); + + $this->defaultProvider + ->expects($this->once()) + ->method('getShareById') + ->with('42', 'recipient') + ->willReturn($share); + + $this->manager->getShareById('default:42', 'recipient'); + } + public function testGetExpiredShareById(): void { $this->expectException(ShareNotFound::class); @@ -960,6 +1015,13 @@ public function createShare($id, int $type, ?Node $node, $sharedWith, $sharedBy, return $share; } + private function createEnabledUser(bool $enabled): IUser&MockObject { + $user = $this->createMock(IUser::class); + $user->method('isEnabled')->willReturn($enabled); + + return $user; + } + public static function dataGeneralChecks(): array { $user0 = 'user0'; $user2 = 'user1'; @@ -3262,6 +3324,83 @@ public function testCreateShareUser(): void { $manager->createShare($share); } + public function testCreateShareUserAlreadySharedReusesExistingShare(): void { + /** @var Manager&MockObject $manager */ + $manager = $this->createManagerMock() + ->onlyMethods(['generalChecks', 'userCreateChecks', 'pathCreateChecks']) + ->getMock(); + + $shareOwner = $this->createMock(IUser::class); + $shareOwner->method('getUID')->willReturn('shareOwner'); + + $path = $this->createMock(File::class); + $path->method('getOwner')->willReturn($shareOwner); + $path->method('getName')->willReturn('target'); + $path->method('getStorage')->willReturn($this->createMock(IStorage::class)); + + $share = $this->createShare(null, IShare::TYPE_USER, $path, 'sharedWith', 'sharedBy', null, Constants::PERMISSION_ALL); + $existingShare = $this->createShare('42', IShare::TYPE_USER, $path, 'sharedWith', 'shareOwner', 'shareOwner', Constants::PERMISSION_READ); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'], + ]); + $this->userManager->method('get')->willReturnMap([ + ['shareOwner', $this->createEnabledUser(true)], + ]); + + $manager->method('userCreateChecks') + ->willThrowException(new AlreadySharedException('Already shared', $existingShare)); + + $this->defaultProvider->expects($this->never()) + ->method('create'); + + $existingShare->expects($this->once()) + ->method('setNode') + ->with($path); + + $this->assertSame($existingShare, $manager->createShare($share)); + } + + public function testCreateShareUserAlreadySharedByDisabledInitiator(): void { + $this->expectException(AlreadySharedException::class); + + /** @var Manager&MockObject $manager */ + $manager = $this->createManagerMock() + ->onlyMethods(['generalChecks', 'userCreateChecks', 'pathCreateChecks']) + ->getMock(); + + $shareOwner = $this->createMock(IUser::class); + $shareOwner->method('getUID')->willReturn('shareOwner'); + + $path = $this->createMock(File::class); + $path->method('getOwner')->willReturn($shareOwner); + $path->method('getName')->willReturn('target'); + $path->method('getStorage')->willReturn($this->createMock(IStorage::class)); + + $share = $this->createShare(null, IShare::TYPE_USER, $path, 'sharedWith', 'shareOwner', null, Constants::PERMISSION_ALL); + $existingShare = $this->createShare('42', IShare::TYPE_USER, $path, 'sharedWith', 'initiator', 'shareOwner', Constants::PERMISSION_READ); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'], + ]); + $this->userManager->method('get')->willReturnMap([ + ['shareOwner', $this->createEnabledUser(true)], + ['initiator', $this->createEnabledUser(false)], + ]); + + $manager->method('userCreateChecks') + ->willThrowException(new AlreadySharedException('Already shared', $existingShare)); + + $this->defaultProvider->expects($this->never()) + ->method('create'); + $this->dispatcher->expects($this->never()) + ->method('dispatchTyped'); + + $manager->createShare($share); + } + public function testCreateShareGroup(): void { $manager = $this->createManagerMock() ->onlyMethods(['generalChecks', 'groupCreateChecks', 'pathCreateChecks', 'validateExpirationDateInternal']) @@ -3686,6 +3825,86 @@ public function testGetSharesByOwnerless(): void { $this->assertSame($share, $shares[0]); } + public function testGetSharesByOfDisabledInitiatorAsShareOwner(): void { + $this->config->method('getAppValue') + ->willReturnMap([ + ['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'], + ]); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_USER) + ->setShareOwner('owner') + ->setSharedBy('initiator') + ->setSharedWith('recipient'); + + $this->userManager->method('get')->willReturnMap([ + ['owner', $this->createEnabledUser(true)], + ['initiator', $this->createEnabledUser(false)], + ]); + + $this->defaultProvider->expects($this->once()) + ->method('getSharesBy') + ->willReturn([$share]); + + $shares = $this->manager->getSharesBy('owner', IShare::TYPE_USER, $this->createMock(Folder::class), true, -1, 0); + + $this->assertCount(1, $shares); + $this->assertSame($share, $shares[0]); + } + + public function testGetSharesByOfDisabledInitiatorAsOtherUser(): void { + $this->config->method('getAppValue') + ->willReturnMap([ + ['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'], + ]); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_USER) + ->setShareOwner('owner') + ->setSharedBy('initiator') + ->setSharedWith('recipient'); + + $this->userManager->method('get')->willReturnMap([ + ['owner', $this->createEnabledUser(true)], + ['initiator', $this->createEnabledUser(false)], + ['resharer', $this->createEnabledUser(true)], + ]); + + $this->defaultProvider->expects($this->once()) + ->method('getSharesBy') + ->willReturn([$share]); + + $shares = $this->manager->getSharesBy('resharer', IShare::TYPE_USER, $this->createMock(Folder::class), true, -1, 0); + + $this->assertCount(0, $shares); + } + + public function testGetSharedWithOfDisabledInitiator(): void { + $this->config->method('getAppValue') + ->willReturnMap([ + ['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'], + ]); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_USER) + ->setShareOwner('owner') + ->setSharedBy('initiator') + ->setSharedWith('recipient'); + + $this->userManager->method('get')->willReturnMap([ + ['owner', $this->createEnabledUser(true)], + ['initiator', $this->createEnabledUser(false)], + ]); + + $this->defaultProvider->expects($this->once()) + ->method('getSharedWith') + ->willReturn([$share]); + + $shares = $this->manager->getSharedWith('recipient', IShare::TYPE_USER); + + $this->assertCount(0, $shares); + } + /** * Test to ensure we correctly remove expired link shares *