Skip to content

Dav respect disallow sharing with groups #25658

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

Merged
ChristophWurst merged 2 commits into from
Jun 1, 2021
Merged

Dav respect disallow sharing with groups #25658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4b5645
Also make other shares listen to the "Allow sharing with groups"
nickvergessen Jan 29, 2021
1c0d975
Make dav respect disallowing sharing with groups
tcitworld Feb 15, 2021
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
15 changes: 10 additions & 5 deletions apps/dav/lib/DAV/GroupPrincipalBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,8 @@ public function searchPrincipals($prefixPath, array $searchProperties, $test = '
if ($prefixPath !== self::PRINCIPAL_PREFIX) {
return [];
}
// If sharing is disabled, return the empty array
$shareAPIEnabled = $this->shareManager->shareApiEnabled();
if (!$shareAPIEnabled) {
// If sharing or group sharing is disabled, return the empty array
if (!$this->groupSharingEnabled()) {
return [];
}

Expand Down Expand Up @@ -273,8 +272,7 @@ public function searchPrincipals($prefixPath, array $searchProperties, $test = '
*/
public function findByUri($uri, $principalPrefix) {
// If sharing is disabled, return the empty array
$shareAPIEnabled = $this->shareManager->shareApiEnabled();
if (!$shareAPIEnabled) {
if (!$this->groupSharingEnabled()) {
return null;
}

Expand Down Expand Up @@ -340,4 +338,11 @@ protected function userToPrincipal($user) {

return $principal;
}

/**
* @return bool
*/
private function groupSharingEnabled(): bool {
return $this->shareManager->shareApiEnabled() && $this->shareManager->allowGroupSharing();
}
}
73 changes: 49 additions & 24 deletions apps/dav/tests/unit/DAV/GroupPrincipalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,20 @@
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share\IManager;
use PHPUnit\Framework\MockObject\MockObject;
use Sabre\DAV\PropPatch;

class GroupPrincipalTest extends \Test\TestCase {
/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
/** @var IConfig|MockObject */
private $config;

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

/** @var IUserSession | \PHPUnit\Framework\MockObject\MockObject */
/** @var IUserSession | MockObject */
private $userSession;

/** @var IManager | \PHPUnit\Framework\MockObject\MockObject */
/** @var IManager | MockObject */
private $shareManager;

/** @var GroupPrincipalBackend */
Expand Down Expand Up @@ -224,13 +225,22 @@ public function testSearchPrincipalsWithWrongPrefixPath() {

/**
* @dataProvider searchPrincipalsDataProvider
* @param bool $sharingEnabled
* @param bool $groupSharingEnabled
* @param bool $groupsOnly
* @param string $test
* @param array $result
*/
public function testSearchPrincipals($sharingEnabled, $groupsOnly, $test, $result) {
public function testSearchPrincipals(bool $sharingEnabled, bool $groupSharingEnabled, bool $groupsOnly, string $test, array $result): void {
$this->shareManager->expects($this->once())
->method('shareAPIEnabled')
->willReturn($sharingEnabled);

if ($sharingEnabled) {
$this->shareManager->expects($sharingEnabled ? $this->once() : $this->never())
->method('allowGroupSharing')
->willReturn($groupSharingEnabled);

if ($sharingEnabled && $groupSharingEnabled) {
$this->shareManager->expects($this->once())
->method('shareWithGroupMembersOnly')
->willReturn($groupsOnly);
Expand Down Expand Up @@ -264,7 +274,7 @@ public function testSearchPrincipals($sharingEnabled, $groupsOnly, $test, $resul
$group5 = $this->createMock(IGroup::class);
$group5->method('getGID')->willReturn('group5');

if ($sharingEnabled) {
if ($sharingEnabled && $groupSharingEnabled) {
$this->groupManager->expects($this->once())
->method('search')
->with('Foo')
Expand All @@ -280,24 +290,35 @@ public function testSearchPrincipals($sharingEnabled, $groupsOnly, $test, $resul

public function searchPrincipalsDataProvider() {
return [
[true, false, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
[true, false, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
[true, true, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
[true, true, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
[false, false, 'allof', []],
[false, false, 'anyof', []],
[true, true, false, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
[true, true, false, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
[true, true, true, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
[true, true, true, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
[true, false, false, 'allof', []],
[false, true, false, 'anyof', []],
[false, false, false, 'allof', []],
[false, false, false, 'anyof', []],
];
}

/**
* @dataProvider findByUriDataProvider
* @param bool $sharingEnabled
* @param bool $groupSharingEnabled
* @param bool $groupsOnly
* @param string $findUri
* @param string|null $result
*/
public function testFindByUri($sharingEnabled, $groupsOnly, $findUri, $result) {
public function testFindByUri(bool $sharingEnabled, bool $groupSharingEnabled, bool $groupsOnly, string $findUri, ?string $result): void {
$this->shareManager->expects($this->once())
->method('shareAPIEnabled')
->willReturn($sharingEnabled);

if ($sharingEnabled) {
$this->shareManager->expects($sharingEnabled ? $this->once() : $this->never())
->method('allowGroupSharing')
->willReturn($groupSharingEnabled);

if ($sharingEnabled && $groupSharingEnabled) {
$this->shareManager->expects($this->once())
->method('shareWithGroupMembersOnly')
->willReturn($groupsOnly);
Expand Down Expand Up @@ -325,19 +346,23 @@ public function testFindByUri($sharingEnabled, $groupsOnly, $findUri, $result) {

public function findByUriDataProvider() {
return [
[false, false, 'principal:principals/groups/group1', null],
[false, false, 'principal:principals/groups/group3', null],
[false, true, 'principal:principals/groups/group1', null],
[false, true, 'principal:principals/groups/group3', null],
[true, true, 'principal:principals/groups/group1', 'principals/groups/group1'],
[true, true, 'principal:principals/groups/group3', null],
[true, false, 'principal:principals/groups/group1', 'principals/groups/group1'],
[true, false, 'principal:principals/groups/group3', 'principals/groups/group3'],
[false, false, false, 'principal:principals/groups/group1', null],
[false, false, false, 'principal:principals/groups/group3', null],
[false, true, false, 'principal:principals/groups/group1', null],
[false, true, false, 'principal:principals/groups/group3', null],
[false, false, true, 'principal:principals/groups/group1', null],
[false, false, true, 'principal:principals/groups/group3', null],
[true, false, true, 'principal:principals/groups/group1', null],
[true, false, true, 'principal:principals/groups/group3', null],
[true, true, true, 'principal:principals/groups/group1', 'principals/groups/group1'],
[true, true, true, 'principal:principals/groups/group3', null],
[true, true, false, 'principal:principals/groups/group1', 'principals/groups/group1'],
[true, true, false, 'principal:principals/groups/group3', 'principals/groups/group3'],
];
}

/**
* @return Group|\PHPUnit\Framework\MockObject\MockObject
* @return Group|MockObject
*/
private function mockGroup($gid) {
$fooGroup = $this->createMock(Group::class);
Expand Down
4 changes: 3 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 @@ -192,7 +192,9 @@ public function search(string $search = '', string $itemType = null, int $page =
$shareTypes[] = IShare::TYPE_DECK;
}
} else {
$shareTypes[] = IShare::TYPE_GROUP;
if ($this->shareManager->allowGroupSharing()) {
$shareTypes[] = IShare::TYPE_GROUP;
}
$shareTypes[] = IShare::TYPE_EMAIL;
}

Expand Down
52 changes: 28 additions & 24 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 @@ -41,6 +41,7 @@
use OCP\IURLGenerator;
use OCP\Share\IShare;
use OCP\Share\IManager;
use PHPUnit\Framework\MockObject\MockObject;

/**
* Class ShareesTest
Expand All @@ -56,13 +57,13 @@ class ShareesAPIControllerTest extends TestCase {
/** @var string */
protected $uid;

/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */
/** @var IRequest|MockObject */
protected $request;

/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
/** @var IManager|MockObject */
protected $shareManager;

/** @var ISearch|\PHPUnit\Framework\MockObject\MockObject */
/** @var ISearch|MockObject */
protected $collaboratorSearch;

protected function setUp(): void {
Expand All @@ -72,10 +73,10 @@ protected function setUp(): void {
$this->request = $this->createMock(IRequest::class);
$this->shareManager = $this->createMock(IManager::class);

/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject $configMock */
/** @var IConfig|MockObject $configMock */
$configMock = $this->createMock(IConfig::class);

/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGeneratorMock */
/** @var IURLGenerator|MockObject $urlGeneratorMock */
$urlGeneratorMock = $this->createMock(IURLGenerator::class);

$this->collaboratorSearch = $this->createMock(ISearch::class);
Expand All @@ -91,7 +92,7 @@ protected function setUp(): void {
);
}

public function dataSearch() {
public function dataSearch(): array {
$noRemote = [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_EMAIL];
$allTypes = [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_REMOTE, IShare::TYPE_REMOTE_GROUP, IShare::TYPE_EMAIL];

Expand Down Expand Up @@ -123,6 +124,9 @@ public function dataSearch() {
[[
'itemType' => 'call',
], '', 'yes', true, true, true, $noRemote, false, true, true],
[[
'itemType' => 'call',
], '', 'yes', true, true, true, [0, 4], false, true, false],
[[
'itemType' => 'folder',
], '', 'yes', true, true, true, $allTypes, false, true, true],
Expand Down Expand Up @@ -230,14 +234,14 @@ public function dataSearch() {
* @param bool $allowGroupSharing
* @throws OCSBadRequestException
*/
public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEnabled, $isRemoteGroupSharingEnabled, $emailSharingEnabled, $shareTypes, $shareWithGroupOnly, $shareeEnumeration, $allowGroupSharing) {
$search = isset($getData['search']) ? $getData['search'] : '';
$itemType = isset($getData['itemType']) ? $getData['itemType'] : 'irrelevant';
$page = isset($getData['page']) ? $getData['page'] : 1;
$perPage = isset($getData['perPage']) ? $getData['perPage'] : 200;
$shareType = isset($getData['shareType']) ? $getData['shareType'] : null;

/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject $config */
public function testSearch(array $getData, string $apiSetting, string $enumSetting, bool $remoteSharingEnabled, bool $isRemoteGroupSharingEnabled, bool $emailSharingEnabled, array $shareTypes, bool $shareWithGroupOnly, bool $shareeEnumeration, bool $allowGroupSharing) {
$search = $getData['search'] ?? '';
$itemType = $getData['itemType'] ?? 'irrelevant';
$page = $getData['page'] ?? 1;
$perPage = $getData['perPage'] ?? 200;
$shareType = $getData['shareType'] ?? null;

/** @var IConfig|MockObject $config */
$config = $this->createMock(IConfig::class);
$config->expects($this->exactly(1))
->method('getAppValue')
Expand All @@ -246,19 +250,19 @@ public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEn
['files_sharing', 'lookupServerEnabled', 'yes', 'yes'],
]);

$this->shareManager->expects($itemType === 'file' || $itemType === 'folder' ? $this->once() : $this->never())
$this->shareManager->expects($this->once())
->method('allowGroupSharing')
->willReturn($allowGroupSharing);

/** @var string */
$uid = 'test123';
/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject $request */
/** @var IRequest|MockObject $request */
$request = $this->createMock(IRequest::class);
/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGenerator */
/** @var IURLGenerator|MockObject $urlGenerator */
$urlGenerator = $this->createMock(IURLGenerator::class);

/** @var \PHPUnit\Framework\MockObject\MockObject|\OCA\Files_Sharing\Controller\ShareesAPIController $sharees */
$sharees = $this->getMockBuilder('\OCA\Files_Sharing\Controller\ShareesAPIController')
/** @var MockObject|ShareesAPIController $sharees */
$sharees = $this->getMockBuilder(ShareesAPIController::class)
->setConstructorArgs([
$uid,
'files_sharing',
Expand Down Expand Up @@ -304,7 +308,7 @@ public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEn
$this->assertInstanceOf(Http\DataResponse::class, $sharees->search($search, $itemType, $page, $perPage, $shareType));
}

public function dataSearchInvalid() {
public function dataSearchInvalid(): array {
return [
// Test invalid pagination
[[
Expand Down Expand Up @@ -340,19 +344,19 @@ public function testSearchInvalid($getData, $message) {
$page = isset($getData['page']) ? $getData['page'] : 1;
$perPage = isset($getData['perPage']) ? $getData['perPage'] : 200;

/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject $config */
/** @var IConfig|MockObject $config */
$config = $this->createMock(IConfig::class);
$config->expects($this->never())
->method('getAppValue');

/** @var string */
$uid = 'test123';
/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject $request */
/** @var IRequest|MockObject $request */
$request = $this->createMock(IRequest::class);
/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGenerator */
/** @var IURLGenerator|MockObject $urlGenerator */
$urlGenerator = $this->createMock(IURLGenerator::class);

/** @var \PHPUnit\Framework\MockObject\MockObject|\OCA\Files_Sharing\Controller\ShareesAPIController $sharees */
/** @var MockObject|ShareesAPIController $sharees */
$sharees = $this->getMockBuilder('\OCA\Files_Sharing\Controller\ShareesAPIController')
->setConstructorArgs([
$uid,
Expand Down
11 changes: 11 additions & 0 deletions lib/private/Collaboration/Collaborators/GroupPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,14 @@
use OCP\Share\IShare;

class GroupPlugin implements ISearchPlugin {
/** @var bool */
protected $shareeEnumeration;
/** @var bool */
protected $shareWithGroupOnly;
/** @var bool */
protected $shareeEnumerationInGroupOnly;
/** @var bool */
protected $groupSharingDisabled;

/** @var IGroupManager */
private $groupManager;
Expand All @@ -56,9 +62,14 @@ public function __construct(IConfig $config, IGroupManager $groupManager, IUserS
$this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
$this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes';
$this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$this->groupSharingDisabled = $this->config->getAppValue('core', 'shareapi_allow_group_sharing', 'yes') === 'no';
}

public function search($search, $limit, $offset, ISearchResult $searchResult) {
if ($this->groupSharingDisabled) {
return false;
}

$hasMoreResults = false;
$result = ['wide' => [], 'exact' => []];

Expand Down
Loading