Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hide shares by disabled users #39699

Merged
merged 3 commits into from
Aug 28, 2023
Merged

Hide shares by disabled users #39699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c9aa9a
Set files_sharing:hide_disabled_user_shares to 'yes' to hide shares f…
come-nc Aug 3, 2023
fd0de0a
Fix tests, add test for the new feature
come-nc Aug 7, 2023
24ad2e2
Use nullsafe call syntax instead of additionnal check
come-nc Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions lib/private/Share20/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,7 +1343,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;
Expand Down Expand Up @@ -1402,7 +1402,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]);
}
Expand Down Expand Up @@ -1448,7 +1448,7 @@ public function getShareById($id, $recipient = null) {

$share = $provider->getShareById($id, $recipient);

$this->checkExpireDate($share);
$this->checkShare($share);

return $share;
}
Expand Down Expand Up @@ -1532,7 +1532,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
Expand All @@ -1545,11 +1545,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?->isEnabled() === false) {
throw new ShareNotFound($this->l->t('The requested share comes from a disabled user'));
come-nc marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

/**
Expand Down
90 changes: 77 additions & 13 deletions tests/lib/Share20/ManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2729,10 +2729,12 @@ public function testGetSharesByExpiredLinkShares() {

public function testGetShareByToken() {
$this->config
->expects($this->once())
->expects($this->exactly(2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer just removing these counts altogether as they don't actually contribute to the test.

->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);

Expand Down Expand Up @@ -2774,10 +2776,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);

Expand Down Expand Up @@ -2826,10 +2830,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);

Expand Down Expand Up @@ -2876,6 +2882,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');
Expand Down Expand Up @@ -2913,10 +2974,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);
Expand Down Expand Up @@ -2948,11 +3011,12 @@ public function testGetShareByTokenWithPublicLinksDisabled() {

public function testGetShareByTokenPublicUploadDisabled() {
$this->config
->expects($this->exactly(2))
->expects($this->exactly(3))
->method('getAppValue')
->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();
Expand Down
Loading