Skip to content

Commit 68b9108

Browse files
nfebenfebe
authored
Merge pull request #55811 from nextcloud/fix/public-share-group-exclusion-access
fix(sharing): Allow public share access for everyone
2 parents 86c2dd4 + 6bccaf7 commit 68b9108

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 33.0.0 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
@@ -3325,21 +3325,29 @@ public function testGetShareByTokenWithPublicLinksDisabled(): void {
33253325

33263326
public function testGetShareByTokenPublicUploadDisabled(): void {
33273327
$this->config
3328-
->expects($this->exactly(3))
3328+
->expects($this->exactly(5))
33293329
->method('getAppValue')
33303330
->willReturnMap([
33313331
['core', 'shareapi_allow_links', 'yes', 'yes'],
33323332
['core', 'shareapi_allow_public_upload', 'yes', 'no'],
33333333
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3334+
['core', 'shareapi_allow_links_exclude_groups', '[]', '[]'],
33343335
]);
33353336

33363337
$share = $this->manager->newShare();
33373338
$share->setShareType(IShare::TYPE_LINK)
33383339
->setPermissions(Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE);
33393340
$share->setSharedWith('sharedWith');
3341+
$share->setShareOwner('shareOwner');
33403342
$folder = $this->createMock(\OC\Files\Node\Folder::class);
33413343
$share->setNode($folder);
33423344

3345+
$shareOwner = $this->createMock(IUser::class);
3346+
$this->userManager->expects($this->once())
3347+
->method('get')
3348+
->with('shareOwner')
3349+
->willReturn($shareOwner);
3350+
33433351
$this->defaultProvider->expects($this->once())
33443352
->method('getShareByToken')
33453353
->willReturn('validToken')
@@ -3350,6 +3358,87 @@ public function testGetShareByTokenPublicUploadDisabled(): void {
33503358
$this->assertSame(Constants::PERMISSION_READ, $res->getPermissions());
33513359
}
33523360

3361+
public function testGetShareByTokenShareOwnerExcludedFromLinkShares(): void {
3362+
$this->expectException(ShareNotFound::class);
3363+
$this->expectExceptionMessage('The requested share does not exist anymore');
3364+
3365+
$this->config
3366+
->expects($this->exactly(4))
3367+
->method('getAppValue')
3368+
->willReturnMap([
3369+
['core', 'shareapi_allow_links', 'yes', 'yes'],
3370+
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3371+
['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'],
3372+
]);
3373+
3374+
$this->l->expects($this->once())
3375+
->method('t')
3376+
->willReturnArgument(0);
3377+
3378+
$share = $this->manager->newShare();
3379+
$share->setShareType(IShare::TYPE_LINK)
3380+
->setPermissions(Constants::PERMISSION_READ);
3381+
$share->setShareOwner('shareOwner');
3382+
$file = $this->createMock(File::class);
3383+
$share->setNode($file);
3384+
3385+
$shareOwner = $this->createMock(IUser::class);
3386+
$this->userManager->expects($this->once())
3387+
->method('get')
3388+
->with('shareOwner')
3389+
->willReturn($shareOwner);
3390+
3391+
$this->groupManager->expects($this->once())
3392+
->method('getUserGroupIds')
3393+
->with($shareOwner)
3394+
->willReturn(['excludedGroup', 'otherGroup']);
3395+
3396+
$this->defaultProvider->expects($this->once())
3397+
->method('getShareByToken')
3398+
->with('token')
3399+
->willReturn($share);
3400+
3401+
$this->manager->getShareByToken('token');
3402+
}
3403+
3404+
public function testGetShareByTokenShareOwnerNotExcludedFromLinkShares(): void {
3405+
$this->config
3406+
->expects($this->exactly(4))
3407+
->method('getAppValue')
3408+
->willReturnMap([
3409+
['core', 'shareapi_allow_links', 'yes', 'yes'],
3410+
['files_sharing', 'hide_disabled_user_shares', 'no', 'no'],
3411+
['core', 'shareapi_allow_links_exclude_groups', '[]', '["excludedGroup"]'],
3412+
]);
3413+
3414+
$share = $this->manager->newShare();
3415+
$share->setShareType(IShare::TYPE_LINK)
3416+
->setPermissions(Constants::PERMISSION_READ);
3417+
$share->setShareOwner('shareOwner');
3418+
$file = $this->createMock(File::class);
3419+
$share->setNode($file);
3420+
3421+
$shareOwner = $this->createMock(IUser::class);
3422+
$this->userManager->expects($this->once())
3423+
->method('get')
3424+
->with('shareOwner')
3425+
->willReturn($shareOwner);
3426+
3427+
$this->groupManager->expects($this->once())
3428+
->method('getUserGroupIds')
3429+
->with($shareOwner)
3430+
->willReturn(['allowedGroup', 'otherGroup']);
3431+
3432+
$this->defaultProvider->expects($this->once())
3433+
->method('getShareByToken')
3434+
->with('token')
3435+
->willReturn($share);
3436+
3437+
$res = $this->manager->getShareByToken('token');
3438+
3439+
$this->assertSame($share, $res);
3440+
}
3441+
33533442
public function testCheckPasswordNoLinkShare(): void {
33543443
$share = $this->createMock(IShare::class);
33553444
$share->method('getShareType')->willReturn(IShare::TYPE_USER);

0 commit comments

Comments
 (0)