From 99142a67d87330c8da96a18cb948fb592e1f9474 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/4] 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 ed8a19d87efdd..a0db8a1167efb 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1349,7 +1349,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; @@ -1408,7 +1408,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]); } @@ -1454,7 +1454,7 @@ public function getShareById($id, $recipient = null) { $share = $provider->getShareById($id, $recipient); - $this->checkExpireDate($share); + $this->checkShare($share); return $share; } @@ -1538,7 +1538,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 @@ -1551,11 +1551,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 da391a550fe0f9f8ce21a55c812356b4ee464833 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/4] 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 | 103 +++++++++++++++++++++++------- 1 file changed, 81 insertions(+), 22 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 4e613d1cf5c88..34214916c3658 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2739,10 +2739,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); @@ -2785,10 +2787,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); @@ -2838,10 +2842,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); @@ -2891,6 +2897,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'); @@ -2928,10 +2989,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); @@ -2963,10 +3026,13 @@ public function testGetShareByTokenWithPublicLinksDisabled() { public function testGetShareByTokenPublicUploadDisabled() { $this->config - ->expects($this->at(0)) + ->expects($this->exactly(3)) ->method('getAppValue') - ->with('core', 'shareapi_allow_links', 'yes') - ->willReturn('yes'); + ->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(); $share->setShareType(IShare::TYPE_LINK) @@ -2975,13 +3041,6 @@ public function testGetShareByTokenPublicUploadDisabled() { $folder = $this->createMock(\OC\Files\Node\Folder::class); $share->setNode($folder); - $this->config - ->expects($this->at(1)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_public_upload', 'yes', 'no'], - ]); - $this->defaultProvider->expects($this->once()) ->method('getShareByToken') ->willReturn('validToken') From 92862be923f4aa3188ffee820890dc766230344b 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/4] 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 a0db8a1167efb..3a041d1ba2b6b 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1565,7 +1565,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 6e6797b593f7a3c94a2436e678fb7b1ea92ba2aa 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/4] 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 3a041d1ba2b6b..7c7277fdc5678 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; @@ -668,7 +667,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(); @@ -1565,7 +1563,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')); } }