From f6521c89602ba315395f93c398c0d2cb71d56837 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 3 Aug 2023 16:13:38 +0200 Subject: [PATCH 1/5] Set files_sharing:hide_disabled_user_shares to 'yes' to hide shares from disabled users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Share20/Manager.php | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 68babd0ec26de..4dff96924ca45 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1350,7 +1350,7 @@ public function getSharesBy($userId, $shareType, $path = null, $reshares = false $added = 0; foreach ($shares as $share) { try { - $this->checkExpireDate($share); + $this->checkShare($share); } catch (ShareNotFound $e) { //Ignore since this basically means the share is deleted continue; @@ -1409,7 +1409,7 @@ public function getSharedWith($userId, $shareType, $node = null, $limit = 50, $o // remove all shares which are already expired foreach ($shares as $key => $share) { try { - $this->checkExpireDate($share); + $this->checkShare($share); } catch (ShareNotFound $e) { unset($shares[$key]); } @@ -1455,7 +1455,7 @@ public function getShareById($id, $recipient = null) { $share = $provider->getShareById($id, $recipient); - $this->checkExpireDate($share); + $this->checkShare($share); return $share; } @@ -1539,7 +1539,7 @@ public function getShareByToken($token) { throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); } - $this->checkExpireDate($share); + $this->checkShare($share); /* * Reduce the permissions for link or email shares if public upload is not enabled @@ -1552,11 +1552,25 @@ public function getShareByToken($token) { return $share; } - protected function checkExpireDate($share) { + /** + * Check expire date and disabled owner + * + * @throws ShareNotFound + */ + protected function checkShare(IShare $share): void { if ($share->isExpired()) { $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) { + $user = $this->userManager->get($uid); + if (($user !== null) && !$user->isEnabled()) { + throw new ShareNotFound($this->l->t('The requested share comes from a disabled user')); + } + } + } } /** From 4f7efe655990e69fd79e5bcabc401b3dfa0df035 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 7 Aug 2023 16:07:20 +0200 Subject: [PATCH 2/5] Fix tests, add test for the new feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- tests/lib/Share20/ManagerTest.php | 87 ++++++++++++++++++++++++++----- 1 file changed, 75 insertions(+), 12 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index a7bb6cbcefc05..8880d2dcfd677 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2715,10 +2715,12 @@ public function testGetSharesByExpiredLinkShares() { public function testGetShareByToken() { $this->config - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getAppValue') - ->with('core', 'shareapi_allow_links', 'yes') - ->willReturn('yes'); + ->willReturnMap([ + ['core', 'shareapi_allow_links', 'yes', 'yes'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], + ]); $factory = $this->createMock(IProviderFactory::class); @@ -2761,10 +2763,12 @@ public function testGetShareByToken() { public function testGetShareByTokenRoom() { $this->config - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getAppValue') - ->with('core', 'shareapi_allow_links', 'yes') - ->willReturn('no'); + ->willReturnMap([ + ['core', 'shareapi_allow_links', 'yes', 'no'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], + ]); $factory = $this->createMock(IProviderFactory::class); @@ -2814,10 +2818,12 @@ public function testGetShareByTokenRoom() { public function testGetShareByTokenWithException() { $this->config - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getAppValue') - ->with('core', 'shareapi_allow_links', 'yes') - ->willReturn('yes'); + ->willReturnMap([ + ['core', 'shareapi_allow_links', 'yes', 'yes'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], + ]); $factory = $this->createMock(IProviderFactory::class); @@ -2865,6 +2871,61 @@ public function testGetShareByTokenWithException() { } + public function testGetShareByTokenHideDisabledUser() { + $this->expectException(\OCP\Share\Exceptions\ShareNotFound::class); + $this->expectExceptionMessage('The requested share comes from a disabled user'); + + $this->config + ->expects($this->exactly(2)) + ->method('getAppValue') + ->willReturnMap([ + ['core', 'shareapi_allow_links', 'yes', 'yes'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'yes'], + ]); + + $this->l->expects($this->once()) + ->method('t') + ->willReturnArgument(0); + + $manager = $this->createManagerMock() + ->setMethods(['deleteShare']) + ->getMock(); + + $date = new \DateTime(); + $date->setTime(0, 0, 0); + $date->add(new \DateInterval('P2D')); + $share = $this->manager->newShare(); + $share->setExpirationDate($date); + $share->setShareOwner('owner'); + $share->setSharedBy('sharedBy'); + + $sharedBy = $this->createMock(IUser::class); + $owner = $this->createMock(IUser::class); + + $this->userManager->method('get')->willReturnMap([ + ['sharedBy', $sharedBy], + ['owner', $owner], + ]); + + $owner->expects($this->once()) + ->method('isEnabled') + ->willReturn(true); + $sharedBy->expects($this->once()) + ->method('isEnabled') + ->willReturn(false); + + $this->defaultProvider->expects($this->once()) + ->method('getShareByToken') + ->with('expiredToken') + ->willReturn($share); + + $manager->expects($this->never()) + ->method('deleteShare'); + + $manager->getShareByToken('expiredToken'); + } + + public function testGetShareByTokenExpired() { $this->expectException(\OCP\Share\Exceptions\ShareNotFound::class); $this->expectExceptionMessage('The requested share does not exist anymore'); @@ -2902,10 +2963,12 @@ public function testGetShareByTokenExpired() { public function testGetShareByTokenNotExpired() { $this->config - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getAppValue') - ->with('core', 'shareapi_allow_links', 'yes') - ->willReturn('yes'); + ->willReturnMap([ + ['core', 'shareapi_allow_links', 'yes', 'yes'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], + ]); $date = new \DateTime(); $date->setTime(0, 0, 0); From 0a43593607382a2de4ba87dcc369b2caa783e182 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= <91878298+come-nc@users.noreply.github.com> Date: Thu, 10 Aug 2023 17:05:19 +0200 Subject: [PATCH 3/5] Use nullsafe call syntax instead of additionnal check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Benjamin Gaussorgues Signed-off-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com> --- lib/private/Share20/Manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 4dff96924ca45..f0e86d4180873 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1566,7 +1566,7 @@ protected function checkShare(IShare $share): void { $uids = array_unique([$share->getShareOwner(),$share->getSharedBy()]); foreach ($uids as $uid) { $user = $this->userManager->get($uid); - if (($user !== null) && !$user->isEnabled()) { + if ($user?->isEnabled() === false) { throw new ShareNotFound($this->l->t('The requested share comes from a disabled user')); } } From 2155281b8124104917cec79e86bf50dde8c170a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 7 Sep 2023 17:13:37 +0200 Subject: [PATCH 4/5] Remove modern PHP syntax, and apply codesniffer fixes in Share20/Manager.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Share20/Manager.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index f0e86d4180873..97f8fc44b3d86 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -82,7 +82,6 @@ * This class is the communication hub for all sharing related operations. */ class Manager implements IManager { - /** @var IProviderFactory */ private $factory; private LoggerInterface $logger; @@ -669,7 +668,6 @@ protected function linkCreateChecks(IShare $share) { * @param IShare $share */ protected function setLinkParent(IShare $share) { - // No sense in checking if the method is not there. if (method_exists($share, 'setParent')) { $storage = $share->getNode()->getStorage(); @@ -1566,7 +1564,7 @@ protected function checkShare(IShare $share): void { $uids = array_unique([$share->getShareOwner(),$share->getSharedBy()]); foreach ($uids as $uid) { $user = $this->userManager->get($uid); - if ($user?->isEnabled() === false) { + if (($user !== null) && ($user->isEnabled() === false)) { throw new ShareNotFound($this->l->t('The requested share comes from a disabled user')); } } From 80bc080965f1574bb8efd51ed61f28f954cd01dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 22 Feb 2024 16:11:46 +0100 Subject: [PATCH 5/5] fix(tests): Adapt ManagerTest to new getAppValue call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- tests/lib/Share20/ManagerTest.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 8880d2dcfd677..893d785794861 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -552,7 +552,7 @@ public function testVerifyPasswordHook() { /** @var ValidatePasswordPolicyEvent $event */ $this->assertSame('password', $event->getPassword()); } - ); + ); $result = self::invokePrivate($this->manager, 'verifyPassword', ['password']); $this->assertNull($result); @@ -575,7 +575,7 @@ public function testVerifyPasswordHookFails() { $this->assertSame('password', $event->getPassword()); throw new HintException('message', 'password not accepted'); } - ); + ); self::invokePrivate($this->manager, 'verifyPassword', ['password']); } @@ -3000,11 +3000,12 @@ public function testGetShareByTokenWithPublicLinksDisabled() { public function testGetShareByTokenPublicUploadDisabled() { $this->config - ->expects($this->exactly(2)) + ->expects($this->exactly(3)) ->method('getAppValue') ->willReturnMap([ ['core', 'shareapi_allow_links', 'yes', 'yes'], ['core', 'shareapi_allow_public_upload', 'yes', 'no'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], ]); $share = $this->manager->newShare();