Skip to content

Commit 77ae691

Browse files
hamza221hamza221
committed
fix: keep share visible for share owner when iinitiator is disactivated
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com> Assisted-by: ClaudeCode:claude-opus-4-8
1 parent 3c4a5cf commit 77ae691

2 files changed

Lines changed: 270 additions & 16 deletions

File tree

‎lib/private/Share20/Manager.php‎

Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -625,12 +625,23 @@ public function createShare(IShare $share): IShare {
625625
$share->setTarget($target);
626626
}
627627
} catch (AlreadySharedException $e) {
628+
$existingShare = $e->getExistingShare();
629+
630+
/*
631+
* Reusing a share which is hidden because one of the accounts involved
632+
* in it is disabled would leave the recipient without access, so report
633+
* it to the sharer instead. Such a share has to be deleted first.
634+
*/
635+
if ($this->isHiddenDisabledUserShare($existingShare)) {
636+
throw $e;
637+
}
638+
628639
// If a share for the same target already exists, dont create a new one,
629640
// but do trigger the hooks and notifications again
630641
$oldShare = $share;
631642

632643
// Reuse the node we already have
633-
$share = $e->getExistingShare();
644+
$share = $existingShare;
634645
$share->setNode($oldShare->getNode());
635646
}
636647

@@ -1151,7 +1162,7 @@ public function getSharesBy(string $userId, int $shareType, ?Node $path = null,
11511162
return [];
11521163
}
11531164

1154-
if ($onlyValid && $this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'no') === 'yes') {
1165+
if ($onlyValid && $this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'yes') === 'yes') {
11551166
/*
11561167
* If shares from disabled users are hidden, check user status first to avoid useless work.
11571168
* 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,
11811192
$added++;
11821193
if ($onlyValid) {
11831194
try {
1184-
$this->checkShare($share, $added);
1195+
$this->checkShare($share, $added, $userId);
11851196
} catch (ShareNotFound $e) {
11861197
// Ignore since this basically means the share is deleted
11871198
continue;
@@ -1243,7 +1254,7 @@ public function getSharedWith(string $userId, int $shareType, ?Node $node = null
12431254
// remove all shares which are already expired
12441255
foreach ($shares as $key => $share) {
12451256
try {
1246-
$this->checkShare($share);
1257+
$this->checkShare($share, viewer: $userId);
12471258
} catch (ShareNotFound $e) {
12481259
unset($shares[$key]);
12491260
}
@@ -1282,10 +1293,10 @@ public function getSharedWithByPath(string $userId, int $shareType, string $path
12821293
$shares = new \IteratorIterator($shares);
12831294
}
12841295

1285-
return new \CallbackFilterIterator($shares, function (IShare $share) {
1296+
return new \CallbackFilterIterator($shares, function (IShare $share) use ($userId) {
12861297
// remove all shares which are already expired
12871298
try {
1288-
$this->checkShare($share);
1299+
$this->checkShare($share, viewer: $userId);
12891300
return true;
12901301
} catch (ShareNotFound $e) {
12911302
return false;
@@ -1319,7 +1330,7 @@ public function getShareById($id, $recipient = null, bool $onlyValid = true): IS
13191330
$share = $provider->getShareById($id, $recipient);
13201331

13211332
if ($onlyValid) {
1322-
$this->checkShare($share);
1333+
$this->checkShare($share, viewer: $recipient);
13231334
}
13241335

13251336
return $share;
@@ -1403,27 +1414,51 @@ public function getShareByToken(string $token): IShare {
14031414
return $share;
14041415
}
14051416

1417+
/**
1418+
* Whether a share has to be hidden because one of the accounts involved in it
1419+
* is disabled.
1420+
*
1421+
* A share initiated by an account that got disabled stays visible to the owner
1422+
* of the shared item, so that they can still manage or delete it.
1423+
*
1424+
* @param ?string $viewer Account the share is checked for, if known
1425+
*/
1426+
private function isHiddenDisabledUserShare(IShare $share, ?string $viewer = null): bool {
1427+
if ($this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'yes') !== 'yes') {
1428+
return false;
1429+
}
1430+
1431+
$uids = [$share->getShareOwner()];
1432+
if ($viewer !== $share->getShareOwner()) {
1433+
$uids[] = $share->getSharedBy();
1434+
}
1435+
1436+
foreach (array_unique($uids) as $uid) {
1437+
$user = $this->userManager->get($uid);
1438+
if ($user?->isEnabled() === false) {
1439+
return true;
1440+
}
1441+
}
1442+
1443+
return false;
1444+
}
1445+
14061446
/**
14071447
* Check expire date and disabled owner
14081448
*
14091449
* @param int &$added If given, will be decremented if the share is deleted
1450+
* @param ?string $viewer Account the share is checked for, defaults to the account of the current session
14101451
* @throws ShareNotFound
14111452
*/
1412-
private function checkShare(IShare $share, int &$added = 1): void {
1453+
private function checkShare(IShare $share, int &$added = 1, ?string $viewer = null): void {
14131454
if ($share->isExpired()) {
14141455
$this->deleteShare($share);
14151456
// Remove 1 to added, because this share was deleted
14161457
$added--;
14171458
throw new ShareNotFound($this->l->t('The requested share does not exist anymore'));
14181459
}
1419-
if ($this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'yes') === 'yes') {
1420-
$uids = array_unique([$share->getShareOwner(), $share->getSharedBy()]);
1421-
foreach ($uids as $uid) {
1422-
$user = $this->userManager->get($uid);
1423-
if ($user?->isEnabled() === false) {
1424-
throw new ShareNotFound($this->l->t('The requested share does not exist anymore'));
1425-
}
1426-
}
1460+
if ($this->isHiddenDisabledUserShare($share, $viewer ?? $this->userSession->getUser()?->getUID())) {
1461+
throw new ShareNotFound($this->l->t('The requested share does not exist anymore'));
14271462
}
14281463

14291464
// For link and email shares, verify the share owner can still create such shares

‎tests/lib/Share20/ManagerTest.php‎

Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -813,6 +813,61 @@ public function testGetShareById(): void {
813813
$this->assertEquals($share, $this->manager->getShareById('default:42'));
814814
}
815815

816+
public function testGetShareByIdOfDisabledInitiatorAsShareOwner(): void {
817+
$this->config->method('getAppValue')
818+
->willReturnMap([
819+
['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'],
820+
]);
821+
822+
$share = $this->manager->newShare();
823+
$share->setShareType(IShare::TYPE_USER)
824+
->setShareOwner('owner')
825+
->setSharedBy('initiator')
826+
->setSharedWith('recipient');
827+
828+
$this->userManager->method('get')->willReturnMap([
829+
['owner', $this->createEnabledUser(true)],
830+
['initiator', $this->createEnabledUser(false)],
831+
]);
832+
833+
$this->defaultProvider
834+
->expects($this->once())
835+
->method('getShareById')
836+
->with('42', 'owner')
837+
->willReturn($share);
838+
839+
$this->assertSame($share, $this->manager->getShareById('default:42', 'owner'));
840+
}
841+
842+
public function testGetShareByIdOfDisabledInitiatorAsOtherUser(): void {
843+
$this->expectException(ShareNotFound::class);
844+
$this->expectExceptionMessage('The requested share does not exist anymore');
845+
846+
$this->config->method('getAppValue')
847+
->willReturnMap([
848+
['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'],
849+
]);
850+
851+
$share = $this->manager->newShare();
852+
$share->setShareType(IShare::TYPE_USER)
853+
->setShareOwner('owner')
854+
->setSharedBy('initiator')
855+
->setSharedWith('recipient');
856+
857+
$this->userManager->method('get')->willReturnMap([
858+
['owner', $this->createEnabledUser(true)],
859+
['initiator', $this->createEnabledUser(false)],
860+
]);
861+
862+
$this->defaultProvider
863+
->expects($this->once())
864+
->method('getShareById')
865+
->with('42', 'recipient')
866+
->willReturn($share);
867+
868+
$this->manager->getShareById('default:42', 'recipient');
869+
}
870+
816871
public function testGetExpiredShareById(): void {
817872
$this->expectException(ShareNotFound::class);
818873

@@ -960,6 +1015,13 @@ public function createShare($id, int $type, ?Node $node, $sharedWith, $sharedBy,
9601015
return $share;
9611016
}
9621017

1018+
private function createEnabledUser(bool $enabled): IUser&MockObject {
1019+
$user = $this->createMock(IUser::class);
1020+
$user->method('isEnabled')->willReturn($enabled);
1021+
1022+
return $user;
1023+
}
1024+
9631025
public static function dataGeneralChecks(): array {
9641026
$user0 = 'user0';
9651027
$user2 = 'user1';
@@ -3261,6 +3323,83 @@ public function testCreateShareUser(): void {
32613323
$manager->createShare($share);
32623324
}
32633325

3326+
public function testCreateShareUserAlreadySharedReusesExistingShare(): void {
3327+
/** @var Manager&MockObject $manager */
3328+
$manager = $this->createManagerMock()
3329+
->onlyMethods(['generalChecks', 'userCreateChecks', 'pathCreateChecks'])
3330+
->getMock();
3331+
3332+
$shareOwner = $this->createMock(IUser::class);
3333+
$shareOwner->method('getUID')->willReturn('shareOwner');
3334+
3335+
$path = $this->createMock(File::class);
3336+
$path->method('getOwner')->willReturn($shareOwner);
3337+
$path->method('getName')->willReturn('target');
3338+
$path->method('getStorage')->willReturn($this->createMock(IStorage::class));
3339+
3340+
$share = $this->createShare(null, IShare::TYPE_USER, $path, 'sharedWith', 'sharedBy', null, Constants::PERMISSION_ALL);
3341+
$existingShare = $this->createShare('42', IShare::TYPE_USER, $path, 'sharedWith', 'shareOwner', 'shareOwner', Constants::PERMISSION_READ);
3342+
3343+
$this->config->method('getAppValue')
3344+
->willReturnMap([
3345+
['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'],
3346+
]);
3347+
$this->userManager->method('get')->willReturnMap([
3348+
['shareOwner', $this->createEnabledUser(true)],
3349+
]);
3350+
3351+
$manager->method('userCreateChecks')
3352+
->willThrowException(new AlreadySharedException('Already shared', $existingShare));
3353+
3354+
$this->defaultProvider->expects($this->never())
3355+
->method('create');
3356+
3357+
$existingShare->expects($this->once())
3358+
->method('setNode')
3359+
->with($path);
3360+
3361+
$this->assertSame($existingShare, $manager->createShare($share));
3362+
}
3363+
3364+
public function testCreateShareUserAlreadySharedByDisabledInitiator(): void {
3365+
$this->expectException(AlreadySharedException::class);
3366+
3367+
/** @var Manager&MockObject $manager */
3368+
$manager = $this->createManagerMock()
3369+
->onlyMethods(['generalChecks', 'userCreateChecks', 'pathCreateChecks'])
3370+
->getMock();
3371+
3372+
$shareOwner = $this->createMock(IUser::class);
3373+
$shareOwner->method('getUID')->willReturn('shareOwner');
3374+
3375+
$path = $this->createMock(File::class);
3376+
$path->method('getOwner')->willReturn($shareOwner);
3377+
$path->method('getName')->willReturn('target');
3378+
$path->method('getStorage')->willReturn($this->createMock(IStorage::class));
3379+
3380+
$share = $this->createShare(null, IShare::TYPE_USER, $path, 'sharedWith', 'shareOwner', null, Constants::PERMISSION_ALL);
3381+
$existingShare = $this->createShare('42', IShare::TYPE_USER, $path, 'sharedWith', 'initiator', 'shareOwner', Constants::PERMISSION_READ);
3382+
3383+
$this->config->method('getAppValue')
3384+
->willReturnMap([
3385+
['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'],
3386+
]);
3387+
$this->userManager->method('get')->willReturnMap([
3388+
['shareOwner', $this->createEnabledUser(true)],
3389+
['initiator', $this->createEnabledUser(false)],
3390+
]);
3391+
3392+
$manager->method('userCreateChecks')
3393+
->willThrowException(new AlreadySharedException('Already shared', $existingShare));
3394+
3395+
$this->defaultProvider->expects($this->never())
3396+
->method('create');
3397+
$this->dispatcher->expects($this->never())
3398+
->method('dispatchTyped');
3399+
3400+
$manager->createShare($share);
3401+
}
3402+
32643403
public function testCreateShareGroup(): void {
32653404
$manager = $this->createManagerMock()
32663405
->onlyMethods(['generalChecks', 'groupCreateChecks', 'pathCreateChecks', 'validateExpirationDateInternal'])
@@ -3685,6 +3824,86 @@ public function testGetSharesByOwnerless(): void {
36853824
$this->assertSame($share, $shares[0]);
36863825
}
36873826

3827+
public function testGetSharesByOfDisabledInitiatorAsShareOwner(): void {
3828+
$this->config->method('getAppValue')
3829+
->willReturnMap([
3830+
['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'],
3831+
]);
3832+
3833+
$share = $this->manager->newShare();
3834+
$share->setShareType(IShare::TYPE_USER)
3835+
->setShareOwner('owner')
3836+
->setSharedBy('initiator')
3837+
->setSharedWith('recipient');
3838+
3839+
$this->userManager->method('get')->willReturnMap([
3840+
['owner', $this->createEnabledUser(true)],
3841+
['initiator', $this->createEnabledUser(false)],
3842+
]);
3843+
3844+
$this->defaultProvider->expects($this->once())
3845+
->method('getSharesBy')
3846+
->willReturn([$share]);
3847+
3848+
$shares = $this->manager->getSharesBy('owner', IShare::TYPE_USER, $this->createMock(Folder::class), true, -1, 0);
3849+
3850+
$this->assertCount(1, $shares);
3851+
$this->assertSame($share, $shares[0]);
3852+
}
3853+
3854+
public function testGetSharesByOfDisabledInitiatorAsOtherUser(): void {
3855+
$this->config->method('getAppValue')
3856+
->willReturnMap([
3857+
['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'],
3858+
]);
3859+
3860+
$share = $this->manager->newShare();
3861+
$share->setShareType(IShare::TYPE_USER)
3862+
->setShareOwner('owner')
3863+
->setSharedBy('initiator')
3864+
->setSharedWith('recipient');
3865+
3866+
$this->userManager->method('get')->willReturnMap([
3867+
['owner', $this->createEnabledUser(true)],
3868+
['initiator', $this->createEnabledUser(false)],
3869+
['resharer', $this->createEnabledUser(true)],
3870+
]);
3871+
3872+
$this->defaultProvider->expects($this->once())
3873+
->method('getSharesBy')
3874+
->willReturn([$share]);
3875+
3876+
$shares = $this->manager->getSharesBy('resharer', IShare::TYPE_USER, $this->createMock(Folder::class), true, -1, 0);
3877+
3878+
$this->assertCount(0, $shares);
3879+
}
3880+
3881+
public function testGetSharedWithOfDisabledInitiator(): void {
3882+
$this->config->method('getAppValue')
3883+
->willReturnMap([
3884+
['files_sharing', 'hide_disabled_user_shares', 'yes', 'yes'],
3885+
]);
3886+
3887+
$share = $this->manager->newShare();
3888+
$share->setShareType(IShare::TYPE_USER)
3889+
->setShareOwner('owner')
3890+
->setSharedBy('initiator')
3891+
->setSharedWith('recipient');
3892+
3893+
$this->userManager->method('get')->willReturnMap([
3894+
['owner', $this->createEnabledUser(true)],
3895+
['initiator', $this->createEnabledUser(false)],
3896+
]);
3897+
3898+
$this->defaultProvider->expects($this->once())
3899+
->method('getSharedWith')
3900+
->willReturn([$share]);
3901+
3902+
$shares = $this->manager->getSharedWith('recipient', IShare::TYPE_USER);
3903+
3904+
$this->assertCount(0, $shares);
3905+
}
3906+
36883907
/**
36893908
* Test to ensure we correctly remove expired link shares
36903909
*

0 commit comments

Comments
 (0)