From fb6a8abd19609afbc9d28513dedf677d7bd7b48d Mon Sep 17 00:00:00 2001 From: nfebe Date: Thu, 16 Oct 2025 15:34:30 +0200 Subject: [PATCH] fix(sharing): Allow public share access for everyone When a logged-in user accesses a public share link in the same browser, the system was incorrectly checking if that user's groups were excluded from creating link shares. This caused share not found errors for users in excluded groups, even though public shares should be accessible to anyone with the link. The group exclusion setting (`shareapi_allow_links_exclude_groups`) is intended to restrict share creation, not share access. Public shares are meant to be anonymous and accessible regardless of the viewer identity or group membership. We now check the exclusion for the share creator and not the viewer. Signed-off-by: nfebe --- lib/private/Share20/Manager.php | 28 +++++++++- lib/public/Share/IManager.php | 4 +- tests/lib/Share20/ManagerTest.php | 91 ++++++++++++++++++++++++++++++- 3 files changed, 118 insertions(+), 5 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index cf6b668471e61..3fffbffffc23d 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1413,7 +1413,7 @@ public function getShareByToken($token) { } $share = null; try { - if ($this->shareApiAllowLinks()) { + if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') === 'yes') { $provider = $this->factory->getProviderForType(IShare::TYPE_LINK); $share = $provider->getShareByToken($token); } @@ -1496,6 +1496,17 @@ protected function checkShare(IShare $share): void { } } } + + // For link and email shares, verify the share owner can still create such shares + if ($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) { + $shareOwner = $this->userManager->get($share->getShareOwner()); + if ($shareOwner === null) { + throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); + } + if (!$this->userCanCreateLinkShares($shareOwner)) { + throw new ShareNotFound($this->l->t('The requested share does not exist anymore')); + } + } } /** @@ -1742,14 +1753,15 @@ public function shareApiEnabled() { /** * Is public link sharing enabled * + * @param ?IUser $user User to check against group exclusions, defaults to current session user * @return bool */ - public function shareApiAllowLinks() { + public function shareApiAllowLinks(?IUser $user = null) { if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') !== 'yes') { return false; } - $user = $this->userSession->getUser(); + $user = $user ?? $this->userSession->getUser(); if ($user) { $excludedGroups = json_decode($this->config->getAppValue('core', 'shareapi_allow_links_exclude_groups', '[]')); if ($excludedGroups) { @@ -1761,6 +1773,16 @@ public function shareApiAllowLinks() { return true; } + /** + * Check if a specific user can create link shares + * + * @param IUser $user The user to check + * @return bool + */ + protected function userCanCreateLinkShares(IUser $user): bool { + return $this->shareApiAllowLinks($user); + } + /** * Is password on public link requires * diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index f6bb3e1059f4c..892f4ccb8d2e4 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -298,10 +298,12 @@ public function shareApiEnabled(); /** * Is public link sharing enabled * + * @param ?IUser $user User to check against group exclusions, defaults to current session user * @return bool * @since 9.0.0 + * @since 31.0.12 Added optional $user parameter */ - public function shareApiAllowLinks(); + public function shareApiAllowLinks(?IUser $user = null); /** * Is password on public link required diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index c4688f445d72a..20cbf8fc1779c 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -3260,21 +3260,29 @@ public function testGetShareByTokenWithPublicLinksDisabled(): void { public function testGetShareByTokenPublicUploadDisabled(): void { $this->config - ->expects($this->exactly(3)) + ->expects($this->exactly(5)) ->method('getAppValue') ->willReturnMap([ ['core', 'shareapi_allow_links', 'yes', 'yes'], ['core', 'shareapi_allow_public_upload', 'yes', 'no'], ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], + ['core', 'shareapi_allow_links_exclude_groups', '[]', '[]'], ]); $share = $this->manager->newShare(); $share->setShareType(IShare::TYPE_LINK) ->setPermissions(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE); $share->setSharedWith('sharedWith'); + $share->setShareOwner('shareOwner'); $folder = $this->createMock(\OC\Files\Node\Folder::class); $share->setNode($folder); + $shareOwner = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('shareOwner') + ->willReturn($shareOwner); + $this->defaultProvider->expects($this->once()) ->method('getShareByToken') ->willReturn('validToken') @@ -3285,6 +3293,87 @@ public function testGetShareByTokenPublicUploadDisabled(): void { $this->assertSame(\OCP\Constants::PERMISSION_READ, $res->getPermissions()); } + public function testGetShareByTokenShareOwnerExcludedFromLinkShares(): void { + $this->expectException(ShareNotFound::class); + $this->expectExceptionMessage('The requested share does not exist anymore'); + + $this->config + ->expects($this->exactly(4)) + ->method('getAppValue') + ->willReturnMap([ + ['core', 'shareapi_allow_links', 'yes', 'yes'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], + ['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'], + ]); + + $this->l->expects($this->once()) + ->method('t') + ->willReturnArgument(0); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_LINK) + ->setPermissions(Constants::PERMISSION_READ); + $share->setShareOwner('shareOwner'); + $file = $this->createMock(File::class); + $share->setNode($file); + + $shareOwner = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('shareOwner') + ->willReturn($shareOwner); + + $this->groupManager->expects($this->once()) + ->method('getUserGroupIds') + ->with($shareOwner) + ->willReturn(['excludedGroup', 'otherGroup']); + + $this->defaultProvider->expects($this->once()) + ->method('getShareByToken') + ->with('token') + ->willReturn($share); + + $this->manager->getShareByToken('token'); + } + + public function testGetShareByTokenShareOwnerNotExcludedFromLinkShares(): void { + $this->config + ->expects($this->exactly(4)) + ->method('getAppValue') + ->willReturnMap([ + ['core', 'shareapi_allow_links', 'yes', 'yes'], + ['files_sharing', 'hide_disabled_user_shares', 'no', 'no'], + ['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'], + ]); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_LINK) + ->setPermissions(Constants::PERMISSION_READ); + $share->setShareOwner('shareOwner'); + $file = $this->createMock(File::class); + $share->setNode($file); + + $shareOwner = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('shareOwner') + ->willReturn($shareOwner); + + $this->groupManager->expects($this->once()) + ->method('getUserGroupIds') + ->with($shareOwner) + ->willReturn(['allowedGroup', 'otherGroup']); + + $this->defaultProvider->expects($this->once()) + ->method('getShareByToken') + ->with('token') + ->willReturn($share); + + $res = $this->manager->getShareByToken('token'); + + $this->assertSame($share, $res); + } + public function testCheckPasswordNoLinkShare(): void { $share = $this->createMock(IShare::class); $share->method('getShareType')->willReturn(IShare::TYPE_USER);