Skip to content

Commit e84fc69

Browse files
hamza221hamza221
committed
fix: keep share visible for share owner when iinitiator is disactivated
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
1 parent ab7b254 commit e84fc69

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';
@@ -3262,6 +3324,83 @@ public function testCreateShareUser(): void {
32623324
$manager->createShare($share);
32633325
}
32643326

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

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

0 commit comments

Comments
 (0)