Skip to content

ShareesAPIController should respect groups disallowed from sharing #25744

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

Closed
Tetrachloromethane250 wants to merge 7 commits into from
Closed

ShareesAPIController should respect groups disallowed from sharing #25744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ce0aa8e
Add check that user is allowed to share
Tetrachloroethene Feb 22, 2021
56cc408
Match create parameters
Tetrachloroethene Feb 22, 2021
be8bd42
Match create parameters for all instances
Tetrachloroethene Feb 22, 2021
d8396ef
Apply suggestions from code review
Tetrachloromethane250 Mar 1, 2021
e40590c
Add last change from review
Tetrachloromethane250 Mar 1, 2021
ded214e
Fix existing tests
Tetrachloroethene Mar 1, 2021
5550611
Merge branch 'master' into fix/20950/Transfer-Ownership-Ignores-Shari…
skjnldsv Feb 27, 2024
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
26 changes: 25 additions & 1 deletion apps/files_sharing/lib/Controller/ShareesAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@
use OCP\Collaboration\Collaborators\SearchResultType;
use OCP\Constants;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\Share\IManager;
use OCP\Share\IShare;
use function array_slice;
Expand Down Expand Up @@ -108,6 +110,12 @@ class ShareesAPIController extends OCSController {
/** @var ISearch */
private $collaboratorSearch;

/** @var IGroupManager */
private $groupManager;

/** @var IUserManager */
private $userManager;

/**
* @param string $UserId
* @param string $appName
Expand All @@ -124,14 +132,20 @@ public function __construct(
IConfig $config,
IURLGenerator $urlGenerator,
IManager $shareManager,
ISearch $collaboratorSearch
ISearch $collaboratorSearch,
IGroupManager $groupManager,
IUserManager $userManager

) {
parent::__construct($appName, $request);
$this->userId = $UserId;
$this->config = $config;
$this->urlGenerator = $urlGenerator;
$this->shareManager = $shareManager;
$this->collaboratorSearch = $collaboratorSearch;
$this->groupManager = $groupManager;
$this->userManager = $userManager;

}

/**
Expand All @@ -152,6 +166,16 @@ public function __construct(
*/
public function search(string $search = '', string $itemType = null, int $page = 1, int $perPage = 200, $shareType = null, bool $lookup = false): DataResponse {

// if some groups are excluded, check the user is allowed to share
if ($this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes') {

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\IConfig::getAppValue has been marked as deprecated
$excludedGroups = (array)json_decode($this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''), true);

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\IConfig::getAppValue has been marked as deprecated
$usersGroups = $this->groupManager->getUserGroupIds($this->userManager->get($this->userId));

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 1 of OCP\IGroupManager::getUserGroupIds cannot be null, possibly null value provided
if (array_intersect($usersGroups, $excludedGroups) === $usersGroups) {
return new DataResponse($this->result);
}
Comment on lines +173 to +175
Copy link
Contributor

@artonge artonge Feb 28, 2024

Choose a reason for hiding this comment

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

  1. If $excludedGroups = ['A'] and $usersGroups = ['A'], then this returns early. OK
  2. If $excludedGroups = ['A'] and $usersGroups = ['B'], then this does not return early. OK
  3. If $excludedGroups = ['A'] and $usersGroups = ['A', 'B'], then this does not return early. NOT OK ?
  4. If $excludedGroups = ['A', 'B'] and $usersGroups = ['A'], then this returns early. OK

Am I right that case #3 should return early?

This might be more correct:

Suggested change
if (array_intersect($usersGroups, $excludedGroups) === $usersGroups) {
return new DataResponse($this->result);
}
if (count(array_intersect($usersGroups, $excludedGroups) !== 0) {
return new DataResponse($this->result);
}

Copy link
Author

@Tetrachloromethane250 Tetrachloromethane250 Feb 28, 2024

Choose a reason for hiding this comment

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

It's been a while since I've looked at Nextcloud, so please excuse any terminology mixups.

If I remember correctly my theory was that a user should be blocked from viewing potential Sharees only if all the groups they are a member of are blocked from sharing. (#20950 (comment))

I was looking at it from a scenario where some imaginary Admin user is also a member of a group which is blocked from sharing: should membership of a single excludedGroup prevent a user from getting a list of sharees? At the time Share Manager took the approach that a user should be blocked from sharing only if they were only a member of excluded groups so I followed that precedent (appears to still be the decision made in https://github.com/nextcloud/server/blob/master/lib/private/Share20/ShareDisableChecker.php#L52)

Either way, that's probably a higher level design decision that I'll leave to someone else.

Copy link
Contributor

@susnux susnux Mar 15, 2024

Choose a reason for hiding this comment

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

Yes sharing is only disabled if the user is only in groups disabled for sharing.

Copy link
Contributor

@susnux susnux Mar 15, 2024

Choose a reason for hiding this comment

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

I think it can be simplified to just:

if ($this->shareManager->sharingDisabledForUser($this->userId)) {
	return new DataResponse($this->result);
}

}


// only search for string larger than a given threshold
$threshold = $this->config->getSystemValueInt('sharing.minSearchStringLength', 0);
if (strlen($search) < $threshold) {
Expand Down
36 changes: 30 additions & 6 deletions apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@
use OCP\AppFramework\OCS\OCSBadRequestException;
use OCP\Collaboration\Collaborators\ISearch;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\Share\IManager;
use OCP\Share\IShare;
use PHPUnit\Framework\MockObject\MockObject;
Expand Down Expand Up @@ -65,6 +67,12 @@ class ShareesAPIControllerTest extends TestCase {
/** @var ISearch|MockObject */
protected $collaboratorSearch;

/** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */
private $groupManager;

/** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */
private $userManager;

protected function setUp(): void {
parent::setUp();

Expand All @@ -80,14 +88,20 @@ protected function setUp(): void {

$this->collaboratorSearch = $this->createMock(ISearch::class);

$this->groupManager = $this->createMock(IGroupManager::class);

$this->userManager = $this->createMock(IUserManager::class);

$this->sharees = new ShareesAPIController(
$this->uid,
'files_sharing',
$this->request,
$configMock,
$urlGeneratorMock,
$this->shareManager,
$this->collaboratorSearch
$this->collaboratorSearch,
$this->groupManager,
$this->userManager
);
}

Expand Down Expand Up @@ -242,10 +256,12 @@ public function testSearch(array $getData, string $apiSetting, string $enumSetti

/** @var IConfig|MockObject $config */
$config = $this->createMock(IConfig::class);
$config->expects($this->exactly(1))

$config->expects($this->exactly(2))
->method('getAppValue')
->with($this->anything(), $this->anything(), $this->anything())
->willReturnMap([
['core', 'shareapi_exclude_groups', 'no', 'no'],
['files_sharing', 'lookupServerEnabled', 'yes', 'yes'],
]);

Expand All @@ -269,7 +285,9 @@ public function testSearch(array $getData, string $apiSetting, string $enumSetti
$config,
$urlGenerator,
$this->shareManager,
$this->collaboratorSearch
$this->collaboratorSearch,
$this->groupManager,
$this->userManager
])
->setMethods(['isRemoteSharingAllowed', 'shareProviderExists', 'isRemoteGroupSharingAllowed'])
->getMock();
Expand Down Expand Up @@ -345,8 +363,12 @@ public function testSearchInvalid($getData, $message) {

/** @var IConfig|MockObject $config */
$config = $this->createMock(IConfig::class);
$config->expects($this->never())
->method('getAppValue');
$config->expects($this->exactly(1))
->method('getAppValue')
->with($this->anything(), $this->anything(), $this->anything())
->willReturnMap([
['core', 'shareapi_exclude_groups', 'no', 'no'],
]);

/** @var string */
$uid = 'test123';
Expand All @@ -364,7 +386,9 @@ public function testSearchInvalid($getData, $message) {
$config,
$urlGenerator,
$this->shareManager,
$this->collaboratorSearch
$this->collaboratorSearch,
$this->groupManager,
$this->userManager
])
->setMethods(['isRemoteSharingAllowed'])
->getMock();
Expand Down