Skip to content

Commit

Permalink
Make dav respect disallowing sharing with groups
Browse files Browse the repository at this point in the history 
Closes #25390

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
  • Loading branch information
@tcitworld
tcitworld committed Feb 15, 2021
1 parent f46013c commit 6f43211
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 98 deletions.
15 changes: 10 additions & 5 deletions apps/dav/lib/DAV/GroupPrincipalBackend.php
View file
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
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
52 changes: 28 additions & 24 deletions apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
View file
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
Loading

0 comments on commit 6f43211

Please sign in to comment.