Skip to content

Commit 569b46f

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 <[email protected]>
1 parent dbe44db commit 569b46f

File tree

3 files changed

+118
-5
lines changed

3 files changed

+118
-5
lines changed

‎lib/private/Share20/Manager.php‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1413,7 +1413,7 @@ public function getShareByToken($token) {
14131413
}
14141414
$share = null;
14151415
try {
1416-
if ($this->shareApiAllowLinks()) {
1416+
if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') === 'yes') {
14171417
$provider = $this->factory->getProviderForType(IShare::TYPE_LINK);
14181418
$share = $provider->getShareByToken($token);
14191419
}
@@ -1496,6 +1496,17 @@ protected function checkShare(IShare $share): void {
14961496
}
14971497
}
14981498
}
1499+
1500+
// For link and email shares, verify the share owner can still create such shares
1501+
if ($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) {
1502+
$shareOwner = $this->userManager->get($share->getShareOwner());
1503+
if ($shareOwner === null) {
1504+
throw new ShareNotFound($this->l->t('The requested share does not exist anymore'));
1505+
}
1506+
if (!$this->userCanCreateLinkShares($shareOwner)) {
1507+
throw new ShareNotFound($this->l->t('The requested share does not exist anymore'));
1508+
}
1509+
}
14991510
}
15001511

15011512
/**
@@ -1742,14 +1753,15 @@ public function shareApiEnabled() {
17421753
/**
17431754
* Is public link sharing enabled
17441755
*
1756+
* @param ?IUser $user User to check against group exclusions, defaults to current session user
17451757
* @return bool
17461758
*/
1747-
public function shareApiAllowLinks() {
1759+
public function shareApiAllowLinks(?IUser $user = null) {
17481760
if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') !== 'yes') {
17491761
return false;
17501762
}
17511763

1752-
$user = $this->userSession->getUser();
1764+
$user = $user ?? $this->userSession->getUser();
17531765
if ($user) {
17541766
$excludedGroups = json_decode($this->config->getAppValue('core', 'shareapi_allow_links_exclude_groups', '[]'));
17551767
if ($excludedGroups) {
@@ -1761,6 +1773,16 @@ public function shareApiAllowLinks() {
17611773
return true;
17621774
}
17631775

1776+
/**
1777+
* Check if a specific user can create link shares
1778+
*
1779+
* @param IUser $user The user to check
1780+
* @return bool
1781+
*/
1782+
protected function userCanCreateLinkShares(IUser $user): bool {
1783+
return $this->shareApiAllowLinks($user);
1784+
}
1785+
17641786
/**
17651787
* Is password on public link requires
17661788
*

‎lib/public/Share/IManager.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -298,10 +298,12 @@ public function shareApiEnabled();
298298
/**
299299
* Is public link sharing enabled
300300
*
301+
* @param ?IUser $user User to check against group exclusions, defaults to current session user
301302
* @return bool
302303
* @since 9.0.0
304+
* @since 31.0.12 Added optional $user parameter
303305
*/
304-
public function shareApiAllowLinks();
306+
public function shareApiAllowLinks(?IUser $user = null);
305307

306308
/**
307309
* 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
@@ -3260,21 +3260,29 @@ public function testGetShareByTokenWithPublicLinksDisabled(): void {
32603260

32613261
public function testGetShareByTokenPublicUploadDisabled(): void {
32623262
$this->config
3263-
->expects($this->exactly(3))
3263+
->expects($this->exactly(5))
32643264
->method('getAppValue')
32653265
->willReturnMap([
32663266
['core', 'shareapi_allow_links', 'yes', 'yes'],
32673267
['core', 'shareapi_allow_public_upload', 'yes', 'no'],
32683268
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3269+
['core', 'shareapi_allow_links_exclude_groups', '[]', '[]'],
32693270
]);
32703271

32713272
$share = $this->manager->newShare();
32723273
$share->setShareType(IShare::TYPE_LINK)
32733274
->setPermissions(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE);
32743275
$share->setSharedWith('sharedWith');
3276+
$share->setShareOwner('shareOwner');
32753277
$folder = $this->createMock(\OC\Files\Node\Folder::class);
32763278
$share->setNode($folder);
32773279

3280+
$shareOwner = $this->createMock(IUser::class);
3281+
$this->userManager->expects($this->once())
3282+
->method('get')
3283+
->with('shareOwner')
3284+
->willReturn($shareOwner);
3285+
32783286
$this->defaultProvider->expects($this->once())
32793287
->method('getShareByToken')
32803288
->willReturn('validToken')
@@ -3285,6 +3293,87 @@ public function testGetShareByTokenPublicUploadDisabled(): void {
32853293
$this->assertSame(\OCP\Constants::PERMISSION_READ, $res->getPermissions());
32863294
}
32873295

3296+
public function testGetShareByTokenShareOwnerExcludedFromLinkShares(): void {
3297+
$this->expectException(ShareNotFound::class);
3298+
$this->expectExceptionMessage('The requested share does not exist anymore');
3299+
3300+
$this->config
3301+
->expects($this->exactly(4))
3302+
->method('getAppValue')
3303+
->willReturnMap([
3304+
['core', 'shareapi_allow_links', 'yes', 'yes'],
3305+
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3306+
['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'],
3307+
]);
3308+
3309+
$this->l->expects($this->once())
3310+
->method('t')
3311+
->willReturnArgument(0);
3312+
3313+
$share = $this->manager->newShare();
3314+
$share->setShareType(IShare::TYPE_LINK)
3315+
->setPermissions(Constants::PERMISSION_READ);
3316+
$share->setShareOwner('shareOwner');
3317+
$file = $this->createMock(File::class);
3318+
$share->setNode($file);
3319+
3320+
$shareOwner = $this->createMock(IUser::class);
3321+
$this->userManager->expects($this->once())
3322+
->method('get')
3323+
->with('shareOwner')
3324+
->willReturn($shareOwner);
3325+
3326+
$this->groupManager->expects($this->once())
3327+
->method('getUserGroupIds')
3328+
->with($shareOwner)
3329+
->willReturn(['excludedGroup', 'otherGroup']);
3330+
3331+
$this->defaultProvider->expects($this->once())
3332+
->method('getShareByToken')
3333+
->with('token')
3334+
->willReturn($share);
3335+
3336+
$this->manager->getShareByToken('token');
3337+
}
3338+
3339+
public function testGetShareByTokenShareOwnerNotExcludedFromLinkShares(): void {
3340+
$this->config
3341+
->expects($this->exactly(4))
3342+
->method('getAppValue')
3343+
->willReturnMap([
3344+
['core', 'shareapi_allow_links', 'yes', 'yes'],
3345+
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3346+
['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'],
3347+
]);
3348+
3349+
$share = $this->manager->newShare();
3350+
$share->setShareType(IShare::TYPE_LINK)
3351+
->setPermissions(Constants::PERMISSION_READ);
3352+
$share->setShareOwner('shareOwner');
3353+
$file = $this->createMock(File::class);
3354+
$share->setNode($file);
3355+
3356+
$shareOwner = $this->createMock(IUser::class);
3357+
$this->userManager->expects($this->once())
3358+
->method('get')
3359+
->with('shareOwner')
3360+
->willReturn($shareOwner);
3361+
3362+
$this->groupManager->expects($this->once())
3363+
->method('getUserGroupIds')
3364+
->with($shareOwner)
3365+
->willReturn(['allowedGroup', 'otherGroup']);
3366+
3367+
$this->defaultProvider->expects($this->once())
3368+
->method('getShareByToken')
3369+
->with('token')
3370+
->willReturn($share);
3371+
3372+
$res = $this->manager->getShareByToken('token');
3373+
3374+
$this->assertSame($share, $res);
3375+
}
3376+
32883377
public function testCheckPasswordNoLinkShare(): void {
32893378
$share = $this->createMock(IShare::class);
32903379
$share->method('getShareType')->willReturn(IShare::TYPE_USER);

0 commit comments

Comments
 (0)