diff --git a/apps/dav/lib/DAV/GroupPrincipalBackend.php b/apps/dav/lib/DAV/GroupPrincipalBackend.php index ae66d85f4c..1b96f2ee8b 100644 --- a/apps/dav/lib/DAV/GroupPrincipalBackend.php +++ b/apps/dav/lib/DAV/GroupPrincipalBackend.php @@ -196,9 +196,8 @@ class GroupPrincipalBackend implements BackendInterface { 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 []; } @@ -273,8 +272,7 @@ class GroupPrincipalBackend implements BackendInterface { */ public function findByUri($uri, $principalPrefix) { // If sharing is disabled, return the empty array - $shareAPIEnabled = $this->shareManager->shareApiEnabled(); - if (!$shareAPIEnabled) { + if (!$this->groupSharingEnabled()) { return null; } @@ -340,4 +338,11 @@ class GroupPrincipalBackend implements BackendInterface { return $principal; } + + /** + * @return bool + */ + private function groupSharingEnabled(): bool { + return $this->shareManager->shareApiEnabled() && $this->shareManager->allowGroupSharing(); + } } diff --git a/apps/dav/tests/unit/DAV/GroupPrincipalTest.php b/apps/dav/tests/unit/DAV/GroupPrincipalTest.php index ba079fb9ca..7e80930bc7 100644 --- a/apps/dav/tests/unit/DAV/GroupPrincipalTest.php +++ b/apps/dav/tests/unit/DAV/GroupPrincipalTest.php @@ -38,19 +38,20 @@ use OCP\IGroupManager; 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 */ @@ -224,13 +225,22 @@ class GroupPrincipalTest extends \Test\TestCase { /** * @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); @@ -264,7 +274,7 @@ class GroupPrincipalTest extends \Test\TestCase { $group5 = $this->createMock(IGroup::class); $group5->method('getGID')->willReturn('group5'); - if ($sharingEnabled) { + if ($sharingEnabled && $groupSharingEnabled) { $this->groupManager->expects($this->once()) ->method('search') ->with('Foo') @@ -280,24 +290,35 @@ class GroupPrincipalTest extends \Test\TestCase { 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); @@ -325,19 +346,23 @@ class GroupPrincipalTest extends \Test\TestCase { 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); diff --git a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php index 8526772606..f057bc1b89 100644 --- a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php @@ -41,6 +41,7 @@ use OCP\IRequest; use OCP\IURLGenerator; use OCP\Share\IShare; use OCP\Share\IManager; +use PHPUnit\Framework\MockObject\MockObject; /** * Class ShareesTest @@ -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 { @@ -72,10 +73,10 @@ class ShareesAPIControllerTest extends TestCase { $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); @@ -91,7 +92,7 @@ class ShareesAPIControllerTest extends TestCase { ); } - 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]; @@ -123,6 +124,9 @@ class ShareesAPIControllerTest extends TestCase { [[ '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], @@ -230,14 +234,14 @@ class ShareesAPIControllerTest extends TestCase { * @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; + 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|\PHPUnit\Framework\MockObject\MockObject $config */ + /** @var IConfig|MockObject $config */ $config = $this->createMock(IConfig::class); $config->expects($this->exactly(1)) ->method('getAppValue') @@ -246,19 +250,19 @@ class ShareesAPIControllerTest extends TestCase { ['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', @@ -304,7 +308,7 @@ class ShareesAPIControllerTest extends TestCase { $this->assertInstanceOf(Http\DataResponse::class, $sharees->search($search, $itemType, $page, $perPage, $shareType)); } - public function dataSearchInvalid() { + public function dataSearchInvalid(): array { return [ // Test invalid pagination [[ @@ -340,19 +344,19 @@ class ShareesAPIControllerTest extends TestCase { $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, diff --git a/tests/lib/Collaboration/Collaborators/GroupPluginTest.php b/tests/lib/Collaboration/Collaborators/GroupPluginTest.php index 2f7cd1061e..1cbaa17844 100644 --- a/tests/lib/Collaboration/Collaborators/GroupPluginTest.php +++ b/tests/lib/Collaboration/Collaborators/GroupPluginTest.php @@ -125,13 +125,15 @@ class GroupPluginTest extends TestCase { return $group; } - public function dataGetGroups() { + public function dataGetGroups(): array { return [ - ['test', false, true, [], [], [], [], true, false], - ['test', false, false, [], [], [], [], true, false], + ['test', false, true, false, [], [], [], [], true, false], + ['test', false, false, false, [], [], [], [], true, false], + // group sharing disabled + ['test', false, true, true, [], [], [], [], false, false], // group without display name [ - 'test', false, true, + 'test', false, true, false, [$this->getGroupMock('test1')], [], [], @@ -141,7 +143,7 @@ class GroupPluginTest extends TestCase { ], // group with display name, search by id [ - 'test', false, true, + 'test', false, true, false, [$this->getGroupMock('test1', 'Test One')], [], [], @@ -151,7 +153,7 @@ class GroupPluginTest extends TestCase { ], // group with display name, search by display name [ - 'one', false, true, + 'one', false, true, false, [$this->getGroupMock('test1', 'Test One')], [], [], @@ -161,7 +163,7 @@ class GroupPluginTest extends TestCase { ], // group with display name, search by display name, exact expected [ - 'Test One', false, true, + 'Test One', false, true, false, [$this->getGroupMock('test1', 'Test One')], [], [['label' => 'Test One', 'value' => ['shareType' => IShare::TYPE_GROUP, 'shareWith' => 'test1']]], @@ -170,7 +172,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', false, false, + 'test', false, false, false, [$this->getGroupMock('test1')], [], [], @@ -179,7 +181,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', false, true, + 'test', false, true, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -191,7 +193,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', false, false, + 'test', false, false, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -203,7 +205,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', false, true, + 'test', false, true, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -218,7 +220,7 @@ class GroupPluginTest extends TestCase { null, ], [ - 'test', false, false, + 'test', false, false, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -230,7 +232,7 @@ class GroupPluginTest extends TestCase { null, ], [ - 'test', false, true, + 'test', false, true, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -247,7 +249,7 @@ class GroupPluginTest extends TestCase { $this->getGroupMock('test'), ], [ - 'test', false, false, + 'test', false, false, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -260,10 +262,10 @@ class GroupPluginTest extends TestCase { true, $this->getGroupMock('test'), ], - ['test', true, true, [], [], [], [], true, false], - ['test', true, false, [], [], [], [], true, false], + ['test', true, true, false, [], [], [], [], true, false], + ['test', true, false, false, [], [], [], [], true, false], [ - 'test', true, true, + 'test', true, true, false, [ $this->getGroupMock('test1'), $this->getGroupMock('test2'), @@ -275,7 +277,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, false, + 'test', true, false, false, [ $this->getGroupMock('test1'), $this->getGroupMock('test2'), @@ -287,7 +289,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, true, + 'test', true, true, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -299,7 +301,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, false, + 'test', true, false, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -311,7 +313,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, true, + 'test', true, true, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -323,7 +325,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, false, + 'test', true, false, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -335,7 +337,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, true, + 'test', true, true, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -347,7 +349,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, false, + 'test', true, false, false, [ $this->getGroupMock('test'), $this->getGroupMock('test1'), @@ -359,7 +361,7 @@ class GroupPluginTest extends TestCase { false, ], [ - 'test', true, true, + 'test', true, true, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -374,7 +376,7 @@ class GroupPluginTest extends TestCase { null, ], [ - 'test', true, false, + 'test', true, false, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -386,7 +388,7 @@ class GroupPluginTest extends TestCase { null, ], [ - 'test', true, true, + 'test', true, true, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -403,7 +405,7 @@ class GroupPluginTest extends TestCase { $this->getGroupMock('test'), ], [ - 'test', true, false, + 'test', true, false, false, [ $this->getGroupMock('test0'), $this->getGroupMock('test1'), @@ -417,7 +419,7 @@ class GroupPluginTest extends TestCase { $this->getGroupMock('test'), ], [ - 'test', false, false, + 'test', false, false, false, [ $this->getGroupMock('test', null, true), $this->getGroupMock('test1'), @@ -437,6 +439,7 @@ class GroupPluginTest extends TestCase { * @param string $searchTerm * @param bool $shareWithGroupOnly * @param bool $shareeEnumeration + * @param bool $groupSharingDisabled * @param array $groupResponse * @param array $userGroupsResponse * @param array $exactExpected @@ -445,35 +448,45 @@ class GroupPluginTest extends TestCase { * @param bool|IGroup $singleGroup */ public function testSearch( - $searchTerm, - $shareWithGroupOnly, - $shareeEnumeration, + string $searchTerm, + bool $shareWithGroupOnly, + bool $shareeEnumeration, + bool $groupSharingDisabled, array $groupResponse, array $userGroupsResponse, array $exactExpected, array $expected, - $reachedEnd, + bool $reachedEnd, $singleGroup ) { $this->config->expects($this->any()) ->method('getAppValue') ->willReturnCallback( - function ($appName, $key, $default) use ($shareWithGroupOnly, $shareeEnumeration) { - if ($appName === 'core' && $key === 'shareapi_only_share_with_group_members') { - return $shareWithGroupOnly ? 'yes' : 'no'; - } elseif ($appName === 'core' && $key === 'shareapi_allow_share_dialog_user_enumeration') { - return $shareeEnumeration ? 'yes' : 'no'; + function ($appName, $key, $default) use ($shareWithGroupOnly, $shareeEnumeration, $groupSharingDisabled) { + if ($appName !== 'core') { + return $default; + } + switch ($key) { + case 'shareapi_only_share_with_group_members': + return $shareWithGroupOnly ? 'yes' : 'no'; + case 'shareapi_allow_share_dialog_user_enumeration': + return $shareeEnumeration ? 'yes' : 'no'; + case 'shareapi_allow_group_sharing': + return $groupSharingDisabled ? 'no' : 'yes'; + default: + return $default; } - return $default; } ); $this->instantiatePlugin(); - $this->groupManager->expects($this->once()) - ->method('search') - ->with($searchTerm, $this->limit, $this->offset) - ->willReturn($groupResponse); + if (!$groupSharingDisabled) { + $this->groupManager->expects($this->once()) + ->method('search') + ->with($searchTerm, $this->limit, $this->offset) + ->willReturn($groupResponse); + } if ($singleGroup !== false) { $this->groupManager->expects($this->once()) @@ -497,8 +510,10 @@ class GroupPluginTest extends TestCase { $moreResults = $this->plugin->search($searchTerm, $this->limit, $this->offset, $this->searchResult); $result = $this->searchResult->asArray(); - $this->assertEquals($exactExpected, $result['exact']['groups']); - $this->assertEquals($expected, $result['groups']); + if (!$groupSharingDisabled) { + $this->assertEquals($exactExpected, $result['exact']['groups']); + $this->assertEquals($expected, $result['groups']); + } $this->assertSame($reachedEnd, $moreResults); } }