Skip to content

Commit 6eda258

Browse files
nfebenfebe
committed
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 <fenn25.fn@gmail.com>
1 parent d2a8c00 commit 6eda258

3 files changed

Lines changed: 118 additions & 5 deletions

File tree

‎lib/private/Share20/Manager.php‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1402,7 +1402,7 @@ public function getShareByToken($token) {
14021402
}
14031403
$share = null;
14041404
try {
1405-
if ($this->shareApiAllowLinks()) {
1405+
if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') === 'yes') {
14061406
$provider = $this->factory->getProviderForType(IShare::TYPE_LINK);
14071407
$share = $provider->getShareByToken($token);
14081408
}
@@ -1485,6 +1485,17 @@ protected function checkShare(IShare $share): void {
14851485
}
14861486
}
14871487
}
1488+
1489+
// For link and email shares, verify the share owner can still create such shares
1490+
if ($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) {
1491+
$shareOwner = $this->userManager->get($share->getShareOwner());
1492+
if ($shareOwner === null) {
1493+
throw new ShareNotFound($this->l->t('The requested share does not exist anymore'));
1494+
}
1495+
if (!$this->userCanCreateLinkShares($shareOwner)) {
1496+
throw new ShareNotFound($this->l->t('The requested share does not exist anymore'));
1497+
}
1498+
}
14881499
}
14891500

14901501
/**
@@ -1731,14 +1742,15 @@ public function shareApiEnabled() {
17311742
/**
17321743
* Is public link sharing enabled
17331744
*
1745+
* @param ?IUser $user User to check against group exclusions, defaults to current session user
17341746
* @return bool
17351747
*/
1736-
public function shareApiAllowLinks() {
1748+
public function shareApiAllowLinks(?IUser $user = null) {
17371749
if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') !== 'yes') {
17381750
return false;
17391751
}
17401752

1741-
$user = $this->userSession->getUser();
1753+
$user = $user ?? $this->userSession->getUser();
17421754
if ($user) {
17431755
$excludedGroups = json_decode($this->config->getAppValue('core', 'shareapi_allow_links_exclude_groups', '[]'));
17441756
if ($excludedGroups) {
@@ -1750,6 +1762,16 @@ public function shareApiAllowLinks() {
17501762
return true;
17511763
}
17521764

1765+
/**
1766+
* Check if a specific user can create link shares
1767+
*
1768+
* @param IUser $user The user to check
1769+
* @return bool
1770+
*/
1771+
protected function userCanCreateLinkShares(IUser $user): bool {
1772+
return $this->shareApiAllowLinks($user);
1773+
}
1774+
17531775
/**
17541776
* Is password on public link requires
17551777
*

‎lib/public/Share/IManager.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,10 +294,12 @@ public function shareApiEnabled();
294294
/**
295295
* Is public link sharing enabled
296296
*
297+
* @param ?IUser $user User to check against group exclusions, defaults to current session user
297298
* @return bool
298299
* @since 9.0.0
300+
* @since 33.0.0 Added optional $user parameter
299301
*/
300-
public function shareApiAllowLinks();
302+
public function shareApiAllowLinks(?IUser $user = null);
301303

302304
/**
303305
* Is password on public link required

‎tests/lib/Share20/ManagerTest.php‎

Lines changed: 90 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3227,21 +3227,29 @@ public function testGetShareByTokenWithPublicLinksDisabled() {
32273227

32283228
public function testGetShareByTokenPublicUploadDisabled() {
32293229
$this->config
3230-
->expects($this->exactly(3))
3230+
->expects($this->exactly(5))
32313231
->method('getAppValue')
32323232
->willReturnMap([
32333233
['core', 'shareapi_allow_links', 'yes', 'yes'],
32343234
['core', 'shareapi_allow_public_upload', 'yes', 'no'],
32353235
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3236+
['core', 'shareapi_allow_links_exclude_groups', '[]', '[]'],
32363237
]);
32373238

32383239
$share = $this->manager->newShare();
32393240
$share->setShareType(IShare::TYPE_LINK)
32403241
->setPermissions(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE);
32413242
$share->setSharedWith('sharedWith');
3243+
$share->setShareOwner('shareOwner');
32423244
$folder = $this->createMock(\OC\Files\Node\Folder::class);
32433245
$share->setNode($folder);
32443246

3247+
$shareOwner = $this->createMock(IUser::class);
3248+
$this->userManager->expects($this->once())
3249+
->method('get')
3250+
->with('shareOwner')
3251+
->willReturn($shareOwner);
3252+
32453253
$this->defaultProvider->expects($this->once())
32463254
->method('getShareByToken')
32473255
->willReturn('validToken')
@@ -3252,6 +3260,87 @@ public function testGetShareByTokenPublicUploadDisabled() {
32523260
$this->assertSame(\OCP\Constants::PERMISSION_READ, $res->getPermissions());
32533261
}
32543262

3263+
public function testGetShareByTokenShareOwnerExcludedFromLinkShares(): void {
3264+
$this->expectException(ShareNotFound::class);
3265+
$this->expectExceptionMessage('The requested share does not exist anymore');
3266+
3267+
$this->config
3268+
->expects($this->exactly(4))
3269+
->method('getAppValue')
3270+
->willReturnMap([
3271+
['core', 'shareapi_allow_links', 'yes', 'yes'],
3272+
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3273+
['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'],
3274+
]);
3275+
3276+
$this->l->expects($this->once())
3277+
->method('t')
3278+
->willReturnArgument(0);
3279+
3280+
$share = $this->manager->newShare();
3281+
$share->setShareType(IShare::TYPE_LINK)
3282+
->setPermissions(\OCP\Constants::PERMISSION_READ);
3283+
$share->setShareOwner('shareOwner');
3284+
$file = $this->createMock(File::class);
3285+
$share->setNode($file);
3286+
3287+
$shareOwner = $this->createMock(IUser::class);
3288+
$this->userManager->expects($this->once())
3289+
->method('get')
3290+
->with('shareOwner')
3291+
->willReturn($shareOwner);
3292+
3293+
$this->groupManager->expects($this->once())
3294+
->method('getUserGroupIds')
3295+
->with($shareOwner)
3296+
->willReturn(['excludedGroup', 'otherGroup']);
3297+
3298+
$this->defaultProvider->expects($this->once())
3299+
->method('getShareByToken')
3300+
->with('token')
3301+
->willReturn($share);
3302+
3303+
$this->manager->getShareByToken('token');
3304+
}
3305+
3306+
public function testGetShareByTokenShareOwnerNotExcludedFromLinkShares(): void {
3307+
$this->config
3308+
->expects($this->exactly(4))
3309+
->method('getAppValue')
3310+
->willReturnMap([
3311+
['core', 'shareapi_allow_links', 'yes', 'yes'],
3312+
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3313+
['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'],
3314+
]);
3315+
3316+
$share = $this->manager->newShare();
3317+
$share->setShareType(IShare::TYPE_LINK)
3318+
->setPermissions(\OCP\Constants::PERMISSION_READ);
3319+
$share->setShareOwner('shareOwner');
3320+
$file = $this->createMock(File::class);
3321+
$share->setNode($file);
3322+
3323+
$shareOwner = $this->createMock(IUser::class);
3324+
$this->userManager->expects($this->once())
3325+
->method('get')
3326+
->with('shareOwner')
3327+
->willReturn($shareOwner);
3328+
3329+
$this->groupManager->expects($this->once())
3330+
->method('getUserGroupIds')
3331+
->with($shareOwner)
3332+
->willReturn(['allowedGroup', 'otherGroup']);
3333+
3334+
$this->defaultProvider->expects($this->once())
3335+
->method('getShareByToken')
3336+
->with('token')
3337+
->willReturn($share);
3338+
3339+
$res = $this->manager->getShareByToken('token');
3340+
3341+
$this->assertSame($share, $res);
3342+
}
3343+
32553344
public function testCheckPasswordNoLinkShare() {
32563345
$share = $this->createMock(IShare::class);
32573346
$share->method('getShareType')->willReturn(IShare::TYPE_USER);

0 commit comments

Comments
 (0)