From eec440cffd4264212b2034bb58f51cd35d28dba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 21 Jun 2022 10:29:55 +0200 Subject: [PATCH 1/6] feat: add guest invitation --- css/app.css | 14 +- js/GroupModel.js | 3 +- js/MemberModel.js | 3 +- js/MembersInputView.js | 3 +- js/MembersView.js | 11 +- js/templates/membersInputItem.handlebars | 6 +- lib/Controller/PageController.php | 25 ++- lib/Dav/MembershipNode.php | 8 +- lib/Service/GuestIntegrationHelper.php | 148 ++++++++++++++++++ lib/Service/MembershipHelper.php | 24 ++- tests/js/MembersInputViewSpec.js | 4 +- .../Dav/GroupMembershipCollectionTest.php | 125 +++++++-------- tests/unit/Dav/GroupsCollectionTest.php | 102 ++++++------ tests/unit/Dav/MembershipNodeTest.php | 83 +++++----- tests/unit/Dav/UsersCollectionTest.php | 76 ++++----- tests/unit/PageControllerTest.php | 116 +++++++------- tests/unit/Service/MembershipHelperTest.php | 69 ++++---- 17 files changed, 498 insertions(+), 322 deletions(-) create mode 100644 lib/Service/GuestIntegrationHelper.php diff --git a/css/app.css b/css/app.css index d3a34515..000a0864 100644 --- a/css/app.css +++ b/css/app.css @@ -87,10 +87,22 @@ white-space: nowrap; text-overflow: ellipsis; overflow: hidden; - line-height: 32px; vertical-align: middle; } +.customgroups-autocomplete-item .autocomplete-item-displayname { + margin-right: 5px; +} + +.customgroups-autocomplete-item .autocomplete-item-typeInfo { + font-size: smaller; + font-style: italic; +} + +.customgroups-autocomplete-item .avatardiv { + flex-shrink: 0; +} + .custom-group-buttons { margin-top: 5px; } diff --git a/js/GroupModel.js b/js/GroupModel.js index 699f3454..44843b1f 100644 --- a/js/GroupModel.js +++ b/js/GroupModel.js @@ -23,7 +23,8 @@ davProperties: { 'displayName': NS + 'display-name', - 'role': NS + 'role' + 'role': NS + 'role', + 'userTypeInfo': NS + 'user-type-info' }, initialize: function() { diff --git a/js/MemberModel.js b/js/MemberModel.js index 55c5788b..7d44fd93 100644 --- a/js/MemberModel.js +++ b/js/MemberModel.js @@ -31,7 +31,8 @@ davProperties: { 'role': NS + 'role', - 'userDisplayName': NS + 'user-display-name' + 'userDisplayName': NS + 'user-display-name', + 'userTypeInfo': NS + 'user-type-info' } }); diff --git a/js/MembersInputView.js b/js/MembersInputView.js index 3caf9dc0..3694400b 100644 --- a/js/MembersInputView.js +++ b/js/MembersInputView.js @@ -136,7 +136,8 @@ autocompleteRenderItem: function($ul, item) { var $item = $(this.itemTemplate({ displayName: item.displayName, - userId: item.userId + userId: item.userId, + typeInfo: item.type === 'guest' ? t('customgroups', 'Guest') : t('customgroups', 'User') })); /* jshint camelcase:false */ diff --git a/js/MembersView.js b/js/MembersView.js index 3cfe4a6b..7f86c140 100644 --- a/js/MembersView.js +++ b/js/MembersView.js @@ -335,6 +335,13 @@ }, _formatMember: function(member) { + var roleDisplayName = t('customgroups', 'Member'); + if (member.get('role') === OCA.CustomGroups.ROLE_ADMIN) { + roleDisplayName = t('customgroups', 'Group owner'); + } else if (member.get('userTypeInfo') === 'guest') { + roleDisplayName = t('customgroups', 'Member (Guest)'); + } + return { id: member.id, displayName: member.get('userDisplayName'), @@ -344,9 +351,7 @@ t('customgroups', 'Change role to "group owner"'), deleteLabel: t('customgroups', 'Remove member'), canAdmin: OC.isUserAdmin() || this.model.get('role') === OCA.CustomGroups.ROLE_ADMIN, - roleDisplayName: (member.get('role') === OCA.CustomGroups.ROLE_ADMIN) ? - t('customgroups', 'Group owner') : - t('customgroups', 'Member') + roleDisplayName: roleDisplayName }; }, diff --git a/js/templates/membersInputItem.handlebars b/js/templates/membersInputItem.handlebars index 02f972a0..6b7235ef 100644 --- a/js/templates/membersInputItem.handlebars +++ b/js/templates/membersInputItem.handlebars @@ -2,7 +2,11 @@
-
{{displayName}}
+
+ {{displayName}} +
+ {{typeInfo}} +
diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php index 36dbfeb7..0f6fcebf 100644 --- a/lib/Controller/PageController.php +++ b/lib/Controller/PageController.php @@ -21,8 +21,11 @@ namespace OCA\CustomGroups\Controller; +use OCA\CustomGroups\Service\GuestIntegrationHelper; +use OCA\Guests\Controller\UsersController; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\TemplateResponse; +use OCP\Constants; use OCP\IUser; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; @@ -59,6 +62,10 @@ class PageController extends Controller { * @var IUserManager */ private $userManager; + /** + * @var GuestIntegrationHelper + */ + private $guestIntegrationHelper; public function __construct( $appName, @@ -67,7 +74,8 @@ public function __construct( IUserSession $userSession, IUserManager $userManager, IGroupManager $groupManager, - CustomGroupsDatabaseHandler $handler + CustomGroupsDatabaseHandler $handler, + GuestIntegrationHelper $guestIntegrationHelper ) { parent::__construct($appName, $request); $this->handler = $handler; @@ -75,6 +83,7 @@ public function __construct( $this->userSession = $userSession; $this->userManager = $userManager; $this->groupManager = $groupManager; + $this->guestIntegrationHelper = $guestIntegrationHelper; } /** @@ -280,10 +289,22 @@ public function searchUsers($group, $pattern, $limit = 200) { $results = \array_map(function (IUser $entry) { return [ 'userId' => $entry->getUID(), - 'displayName' => $entry->getDisplayName() + 'displayName' => $entry->getDisplayName(), + 'type' => 'user' ]; }, $results); + # guest integration + if (!$results && $this->guestIntegrationHelper->canBeGuest($pattern)) { + $results = [ + [ + 'userId' => $pattern, + 'displayName' => "Add $pattern", + 'type' => 'guest' + ] + ]; + } + return new DataResponse(['results' => $results], Http::STATUS_OK); } } diff --git a/lib/Dav/MembershipNode.php b/lib/Dav/MembershipNode.php index c6428fd4..b4c77a3a 100644 --- a/lib/Dav/MembershipNode.php +++ b/lib/Dav/MembershipNode.php @@ -38,6 +38,7 @@ class MembershipNode implements \Sabre\DAV\INode, \Sabre\DAV\IProperties { public const PROPERTY_ROLE = '{http://owncloud.org/ns}role'; public const PROPERTY_USER_ID = '{http://owncloud.org/ns}user-id'; public const PROPERTY_USER_DISPLAY_NAME = '{http://owncloud.org/ns}user-display-name'; + public const PROPERTY_USER_TYPE_INFO = '{http://owncloud.org/ns}user-type-info'; /** * Custom groups handler @@ -131,11 +132,11 @@ public function delete() { )) { // possibly the membership was deleted concurrently throw new PreconditionFailed("Could not remove member \"$userId\" from group \"$groupId\""); - }; + } if ($currentUserId !== $userId) { // only notify when the removal was done by another user - $this->helper->notifyUserRemoved($userId, $this->groupInfo, $this->memberInfo); + $this->helper->notifyUserRemoved($userId, $this->groupInfo); /** * This event is deprecated. The keys of the event array are not using camel case. */ @@ -227,6 +228,9 @@ public function getProperties($properties) { if ($properties === null || \in_array(self::PROPERTY_USER_ID, $properties, true)) { $result[self::PROPERTY_USER_ID] = $this->memberInfo['user_id']; } + if ($properties === null || \in_array(self::PROPERTY_USER_TYPE_INFO, $properties, true)) { + $result[self::PROPERTY_USER_TYPE_INFO] = $this->helper->isGuest($this->getUserId()) ? 'guest' : 'user'; + } if ($properties === null || \in_array(self::PROPERTY_USER_DISPLAY_NAME, $properties, true)) { // FIXME: extremely inefficient as it will query the display name // for each user individually diff --git a/lib/Service/GuestIntegrationHelper.php b/lib/Service/GuestIntegrationHelper.php new file mode 100644 index 00000000..eb2cec94 --- /dev/null +++ b/lib/Service/GuestIntegrationHelper.php @@ -0,0 +1,148 @@ + + * + * @copyright Copyright (c) 2022, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OCA\CustomGroups\Service; + +use OCA\Guests\Controller\UsersController; +use OCA\Guests\Mail; +use OCP\App\IAppManager; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\QueryException; +use OCP\IUserSession; +use OCP\IUserManager; +use OCP\IUser; +use OCP\Mail\IMailer; +use OCP\IConfig; + +class GuestIntegrationHelper { + + /** + * @var IAppManager + */ + private $appManager; + /** + * @var IMailer + */ + private $mailer; + /** + * @var IUserManager + */ + private $userManager; + /** + * @var IConfig + */ + private $config; + /** + * @var IUserSession + */ + private $userSession; + + public function __construct( + IAppManager $appManager, + IMailer $mailer, + IUserManager $userManager, + IUserSession $userSession, + IConfig $config + ) { + $this->appManager = $appManager; + $this->mailer = $mailer; + $this->userManager = $userManager; + $this->config = $config; + $this->userSession = $userSession; + } + + public function canBeGuest(string $email): bool { + if (!$this->appManager->isEnabledForUser('guests')) { + return false; + } + if (!$this->mailer->validateMailAddress($email)) { + return false; + } + # test if the correct guest app version is used + $mail = $this->getGuestMail(); + if ($mail && !method_exists($mail, 'sendGuestPlainInviteMail')) { + # TODO: log error + return false; + } + # test if domain is black listed + $controller = $this->getGuestUsersController(); + if ($controller && method_exists($controller, 'isDomainBlocked')) { + return !$controller->isDomainBlocked($email); + } + return true; + } + + public function createGuest(string $userId): ?IUser { + if (!$this->appManager->isEnabledForUser('guests')) { + return null; + } + $controller = $this->getGuestUsersController(); + $mail = $this->getGuestMail(); + if ($controller && $mail) { + $resp = $controller->create($userId, ''); + if ($resp->getStatus() === 201) { + $user = $this->userManager->get($userId); + if (!$user) { + return null; + } + $registerToken = $this->config->getUserValue( + $userId, + 'guests', + 'registerToken', + null + ); + + try { + if ($registerToken) { + $uid = $this->userSession->getUser()->getUID(); + + // send invitation + $mail->sendGuestPlainInviteMail( + $user->getUID(), + $uid, + $registerToken + ); + } + + return $user; + } catch (DoesNotExistException|\Exception $ex) { + } + } + } + return null; + } + + private function getGuestUsersController(): ?UsersController { + try { + return \OC::$server->query(UsersController::class); + } catch (QueryException $e) { + } + return null; + } + + private function getGuestMail(): ?Mail { + try { + return \OC::$server->query(Mail::class); + } catch (QueryException $e) { + } + return null; + } +} diff --git a/lib/Service/MembershipHelper.php b/lib/Service/MembershipHelper.php index 62e1578c..c261cce7 100644 --- a/lib/Service/MembershipHelper.php +++ b/lib/Service/MembershipHelper.php @@ -98,6 +98,10 @@ class MembershipHelper { * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface */ private $dispatcher; + /** + * @var GuestIntegrationHelper + */ + private $guestIntegrationHelper; /** * Membership helper @@ -117,7 +121,8 @@ public function __construct( IGroupManager $groupManager, IManager $notificationManager, IURLGenerator $urlGenerator, - IConfig $config + IConfig $config, + GuestIntegrationHelper $guestIntegrationHelper ) { $this->groupsHandler = $groupsHandler; $this->userSession = $userSession; @@ -128,6 +133,7 @@ public function __construct( $this->config = $config; $this->dispatcher = \OC::$server->getEventDispatcher(); + $this->guestIntegrationHelper = $guestIntegrationHelper; } /** @@ -146,7 +152,12 @@ public function getUserId() { * @return IUser|null user object or null if user does not exist */ public function getUser($userId) { - return $this->userManager->get($userId); + $user = $this->userManager->get($userId); + if (!$user && \OC::$server->getAppManager()->isEnabledForUser('guests')) { + return $this->guestIntegrationHelper->createGuest($userId); + } + + return $user; } /** @@ -367,4 +378,13 @@ public function isGroupDisplayNameAvailable($displayName) { return empty($groups); } + + public function isGuest(string $userId): bool { + return (bool) $this->config->getUserValue( + $userId, + 'owncloud', + 'isGuest', + false + ); + } } diff --git a/tests/js/MembersInputViewSpec.js b/tests/js/MembersInputViewSpec.js index a0d341f6..aff54295 100644 --- a/tests/js/MembersInputViewSpec.js +++ b/tests/js/MembersInputViewSpec.js @@ -96,8 +96,8 @@ describe('MembersInputView test', function() { expect($li.is('li')).toEqual(true); expect(avatarStub.calledOnce).toEqual(true); - expect($ul.find('.autocomplete-item-text').text()).toEqual('User One'); - expect($li.find('.autocomplete-item-text').text()).toEqual('User One'); + expect($ul.find('.autocomplete-item-text').text()).toContain('User One'); + expect($li.find('.autocomplete-item-text').text()).toContain('User One'); }); it('renders tooltip error if no results', function() { diff --git a/tests/unit/Dav/GroupMembershipCollectionTest.php b/tests/unit/Dav/GroupMembershipCollectionTest.php index 272800e7..37bc5949 100644 --- a/tests/unit/Dav/GroupMembershipCollectionTest.php +++ b/tests/unit/Dav/GroupMembershipCollectionTest.php @@ -22,6 +22,7 @@ use OCA\CustomGroups\Dav\GroupMembershipCollection; use OCA\CustomGroups\CustomGroupsDatabaseHandler; +use OCA\CustomGroups\Service\GuestIntegrationHelper; use OCP\IUserManager; use OCP\IUserSession; use OCP\IUser; @@ -61,21 +62,11 @@ class GroupMembershipCollectionTest extends \Test\TestCase { */ private $helper; - /** - * @var IUserManager - */ - private $userManager; - /** * @var IGroupManager */ private $groupManager; - /** - * @var IUserSession - */ - private $userSession; - /** * @var IConfig */ @@ -94,36 +85,38 @@ class GroupMembershipCollectionTest extends \Test\TestCase { public function setUp(): void { parent::setUp(); $this->handler = $this->createMock(CustomGroupsDatabaseHandler::class); - $this->userManager = $this->createMock(IUserManager::class); + $userManager = $this->createMock(IUserManager::class); $this->groupManager = $this->createMock(IGroupManager::class); - $this->userSession = $this->createMock(IUserSession::class); + $userSession = $this->createMock(IUserSession::class); // currently logged in user $this->currentUser = $this->createMock(IUser::class); $this->currentUser->method('getUID')->willReturn(self::CURRENT_USER); - $this->userSession->method('getUser')->willReturn($this->currentUser); + $userSession->method('getUser')->willReturn($this->currentUser); $this->nodeUser = $this->createMock(IUser::class); $this->nodeUser->method('getUID')->willReturn(self::NODE_USER); - $this->userManager->method('get')->will( - $this->returnValueMap([ + $userManager->method('get')->willReturnMap( + [ [self::NODE_USER, false, $this->nodeUser], [\strtoupper(self::NODE_USER), false, $this->nodeUser], [self::CURRENT_USER, false, $this->currentUser], - ]) + ] ); $this->config = $this->createMock(IConfig::class); + $this->guestIntegrationHelper = $this->createMock(GuestIntegrationHelper::class); $this->helper = $this->getMockBuilder(MembershipHelper::class) ->setMethods(['notifyUser', 'isGroupDisplayNameAvailable']) ->setConstructorArgs([ $this->handler, - $this->userSession, - $this->userManager, + $userSession, + $userManager, $this->groupManager, $this->createMock(IManager::class), $this->createMock(IURLGenerator::class), - $this->config + $this->config, + $this->guestIntegrationHelper ]) ->getMock(); @@ -140,26 +133,26 @@ public function setUp(): void { * * @param array $memberInfo user member info */ - private function setCurrentUserMemberInfo($memberInfo) { + private function setCurrentUserMemberInfo($memberInfo): void { $this->handler->expects($this->any()) ->method('getGroupMemberInfo') ->with(1, self::CURRENT_USER) ->willReturn($memberInfo); } - private function setCurrentUserSuperAdmin($isSuperAdmin) { + private function setCurrentUserSuperAdmin($isSuperAdmin): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->with(self::CURRENT_USER) ->willReturn($isSuperAdmin); } - public function testBase() { + public function testBase(): void { $this->assertEquals('group1', $this->node->getName()); $this->assertNull($this->node->getLastModified()); } - public function testDeleteAsAdmin() { + public function testDeleteAsAdmin(): void { $this->setCurrentUserMemberInfo(['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]); $this->config->method('getSystemValue') ->willReturn(false); @@ -184,7 +177,7 @@ public function testDeleteAsAdmin() { $this->node->delete(); $this->assertSame('\OCA\CustomGroups::deleteGroup', $called[0]); - $this->assertTrue($called[1] instanceof GenericEvent); + $this->assertInstanceOf(GenericEvent::class, $called[1]); $this->assertArrayHasKey('groupName', $called[1]); $this->assertEquals('Group One', $called[1]->getArgument('groupName')); $this->assertArrayHasKey('groupId', $called[1]); @@ -193,7 +186,7 @@ public function testDeleteAsAdmin() { /** */ - public function testDeleteAsNonAdmin() { + public function testDeleteAsNonAdmin(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_MEMBER]); @@ -212,7 +205,7 @@ public function testDeleteAsNonAdmin() { /** */ - public function testDeleteAsNonMember() { + public function testDeleteAsNonMember(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(null); @@ -227,14 +220,14 @@ public function testDeleteAsNonMember() { $this->node->delete(); } - public function testGetProperties() { + public function testGetProperties(): void { $props = $this->node->getProperties(null); $this->assertEquals('Group One', $props[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); $props = $this->node->getProperties([GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); $this->assertEquals('Group One', $props[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); } - public function adminSetFlagProvider() { + public function adminSetFlagProvider(): array { return [ // admin can change display name [false, Roles::BACKEND_ROLE_ADMIN, 200, true], @@ -250,7 +243,7 @@ public function adminSetFlagProvider() { /** * @dataProvider adminSetFlagProvider */ - public function testSetProperties($isSuperAdmin, $currentUserRole, $statusCode, $called) { + public function testSetProperties($isSuperAdmin, $currentUserRole, $statusCode, $called): void { $this->setCurrentUserSuperAdmin($isSuperAdmin); $this->config->method('getSystemValue') ->willReturn(true); @@ -263,7 +256,7 @@ public function testSetProperties($isSuperAdmin, $currentUserRole, $statusCode, $this->setCurrentUserMemberInfo(null); } - $this->helper->expects($this->any()) + $this->helper ->method('isGroupDisplayNameAvailable') ->willReturn(true); @@ -290,7 +283,7 @@ public function testSetProperties($isSuperAdmin, $currentUserRole, $statusCode, $result = $propPatch->getResult(); if (isset($calledEvent)) { $this->assertSame('\OCA\CustomGroups::updateGroupName', $calledEvent[0]); - $this->assertTrue($calledEvent[1] instanceof GenericEvent); + $this->assertInstanceOf(GenericEvent::class, $calledEvent[1]); $this->assertArrayHasKey('oldGroupName', $calledEvent[1]); $this->assertEquals('Group One', $calledEvent[1]->getArgument('oldGroupName')); $this->assertArrayHasKey('newGroupName', $calledEvent[1]); @@ -302,7 +295,7 @@ public function testSetProperties($isSuperAdmin, $currentUserRole, $statusCode, $this->assertEquals($statusCode, $result[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); } - public function testSetDisplayNameNoDuplicates() { + public function testSetDisplayNameNoDuplicates(): void { $this->setCurrentUserSuperAdmin(true); $this->config->method('getSystemValue') ->willReturn(false); @@ -325,7 +318,7 @@ public function testSetDisplayNameNoDuplicates() { $this->assertEquals(409, $result[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); } - public function rolesProvider() { + public function rolesProvider(): array { return [ [false, ['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]], [false, ['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_MEMBER]], @@ -333,7 +326,7 @@ public function rolesProvider() { ]; } - public function adminProvider() { + public function adminProvider(): array { return [ [false, ['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]], [true, null], @@ -343,7 +336,7 @@ public function adminProvider() { /** * @dataProvider adminProvider */ - public function testAddMemberAsAdmin($isSuperAdmin, $currentMemberInfo) { + public function testAddMemberAsAdmin($isSuperAdmin, $currentMemberInfo): void { $this->setCurrentUserMemberInfo($currentMemberInfo); $this->setCurrentUserSuperAdmin($isSuperAdmin); @@ -373,7 +366,7 @@ public function testAddMemberAsAdmin($isSuperAdmin, $currentMemberInfo) { $this->node->createFile(self::NODE_USER); $this->assertSame('\OCA\CustomGroups::addUserToGroup', $called[0]); - $this->assertTrue($called[1] instanceof GenericEvent); + $this->assertInstanceOf(GenericEvent::class, $called[1]); $this->assertArrayHasKey('groupName', $called[1]); $this->assertEquals('Group One', $called[1]->getArgument('groupName')); $this->assertArrayHasKey('user', $called[1]); @@ -385,7 +378,7 @@ public function testAddMemberAsAdmin($isSuperAdmin, $currentMemberInfo) { /** * @dataProvider adminProvider */ - public function testAddMemberAsAdminFails($isSuperAdmin, $currentMemberInfo) { + public function testAddMemberAsAdminFails($isSuperAdmin, $currentMemberInfo): void { $this->expectException(\Sabre\DAV\Exception\PreconditionFailed::class); $this->setCurrentUserMemberInfo($currentMemberInfo); @@ -407,7 +400,7 @@ public function testAddMemberAsAdminFails($isSuperAdmin, $currentMemberInfo) { /** * @dataProvider adminProvider */ - public function testAddNonExistingMemberAsAdmin($isSuperAdmin, $currentMemberInfo) { + public function testAddNonExistingMemberAsAdmin($isSuperAdmin, $currentMemberInfo): void { $this->expectException(\Sabre\DAV\Exception\PreconditionFailed::class); $this->setCurrentUserMemberInfo($currentMemberInfo); @@ -427,7 +420,7 @@ public function testAddNonExistingMemberAsAdmin($isSuperAdmin, $currentMemberInf /** * @dataProvider adminProvider */ - public function testAddNonExistingMemberMismatchCaseAsAdmin($isSuperAdmin, $currentMemberInfo) { + public function testAddNonExistingMemberMismatchCaseAsAdmin($isSuperAdmin, $currentMemberInfo): void { $this->expectException(\Sabre\DAV\Exception\PreconditionFailed::class); $this->setCurrentUserMemberInfo($currentMemberInfo); @@ -446,7 +439,7 @@ public function testAddNonExistingMemberMismatchCaseAsAdmin($isSuperAdmin, $curr /** */ - public function testAddMemberAsNonAdmin() { + public function testAddMemberAsNonAdmin(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_MEMBER]); @@ -464,7 +457,7 @@ public function testAddMemberAsNonAdmin() { /** */ - public function testAddMemberAsNonMember() { + public function testAddMemberAsNonMember(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(null); @@ -482,7 +475,7 @@ public function testAddMemberAsNonMember() { /** */ - public function testAddMemberWithShareToMemberRestrictionAndNoCommonGroup() { + public function testAddMemberWithShareToMemberRestrictionAndNoCommonGroup(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]); @@ -496,10 +489,10 @@ public function testAddMemberWithShareToMemberRestrictionAndNoCommonGroup() { ->willReturn('yes'); $this->groupManager->method('getUserGroupIds') - ->will($this->returnValueMap([ + ->willReturnMap([ [$this->currentUser, null, ['group1', 'group2']], [$this->nodeUser, null, ['group3', 'group4']], - ])); + ]); $this->handler->expects($this->never()) ->method('addToGroup'); @@ -507,7 +500,7 @@ public function testAddMemberWithShareToMemberRestrictionAndNoCommonGroup() { $this->node->createFile(self::NODE_USER); } - public function testAddMemberWithShareToMemberRestrictionAndCommonGroup() { + public function testAddMemberWithShareToMemberRestrictionAndCommonGroup(): void { $this->setCurrentUserMemberInfo(['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]); $this->config->method('getSystemValue') ->willReturn(false); @@ -519,10 +512,10 @@ public function testAddMemberWithShareToMemberRestrictionAndCommonGroup() { ->willReturn('yes'); $this->groupManager->method('getUserGroupIds') - ->will($this->returnValueMap([ + ->willReturnMap([ [$this->currentUser, null, ['group1', 'group2']], [$this->nodeUser, null, ['group1', 'group4']], - ])); + ]); $this->handler->expects($this->once()) ->method('addToGroup') @@ -532,14 +525,14 @@ public function testAddMemberWithShareToMemberRestrictionAndCommonGroup() { $this->node->createFile(self::NODE_USER); } - public function testIsMember() { + public function testIsMember(): void { $this->setCurrentUserMemberInfo(['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_MEMBER]); $this->handler->expects($this->any()) ->method('inGroup') - ->will($this->returnValueMap([ + ->willReturnMap([ [self::NODE_USER, 1, true], ['user3', 1, false], - ])); + ]); $this->assertTrue($this->node->childExists(self::NODE_USER)); $this->assertFalse($this->node->childExists('user3')); @@ -547,7 +540,7 @@ public function testIsMember() { /** */ - public function testIsMemberAsNonMember() { + public function testIsMemberAsNonMember(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(null); @@ -559,16 +552,16 @@ public function testIsMemberAsNonMember() { $this->node->childExists(self::NODE_USER); } - public function testIsMemberAsNonMemberButSuperAdmin() { + public function testIsMemberAsNonMemberButSuperAdmin(): void { $this->setCurrentUserSuperAdmin(true); $this->setCurrentUserMemberInfo(null); $this->handler->expects($this->any()) ->method('inGroup') - ->will($this->returnValueMap([ + ->willReturnMap([ [self::NODE_USER, 1, true], ['user3', 1, false], - ])); + ]); $this->config->method('getSystemValue') ->willReturn(false); @@ -582,7 +575,7 @@ public function testIsMemberAsNonMemberButSuperAdmin() { /** * @dataProvider rolesProvider */ - public function testGetMember($isSuperAdmin, $currentMemberInfo) { + public function testGetMember($isSuperAdmin, $currentMemberInfo): void { $this->setCurrentUserSuperAdmin($isSuperAdmin); $membershipsMap = [ @@ -597,9 +590,9 @@ public function testGetMember($isSuperAdmin, $currentMemberInfo) { $this->groupManager->method('isAdmin') ->willReturn(true); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMemberInfo') - ->will($this->returnValueMap($membershipsMap)); + ->willReturnMap($membershipsMap); $memberInfo = $this->node->getChild(self::NODE_USER); @@ -609,7 +602,7 @@ public function testGetMember($isSuperAdmin, $currentMemberInfo) { /** */ - public function testGetMemberAsNonMember() { + public function testGetMemberAsNonMember(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(null); @@ -624,7 +617,7 @@ public function testGetMemberAsNonMember() { /** * @dataProvider rolesProvider */ - public function testGetMembers($isSuperAdmin, $currentMemberInfo) { + public function testGetMembers($isSuperAdmin, $currentMemberInfo): void { $this->setCurrentUserMemberInfo($currentMemberInfo); $this->setCurrentUserSuperAdmin($isSuperAdmin); @@ -633,7 +626,7 @@ public function testGetMembers($isSuperAdmin, $currentMemberInfo) { $this->groupManager->method('isAdmin') ->willReturn($isSuperAdmin); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, null) ->willReturn([ @@ -653,7 +646,7 @@ public function testGetMembers($isSuperAdmin, $currentMemberInfo) { /** * @dataProvider rolesProvider */ - public function testSearchMembers($isSuperAdmin, $currentMemberInfo) { + public function testSearchMembers($isSuperAdmin, $currentMemberInfo): void { $search = new Search('us', 16, 256); $this->config->method('getSystemValue') @@ -683,7 +676,7 @@ public function testSearchMembers($isSuperAdmin, $currentMemberInfo) { /** */ - public function testGetMembersAsNonMember() { + public function testGetMembersAsNonMember(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(null); @@ -697,7 +690,7 @@ public function testGetMembersAsNonMember() { /** */ - public function testSetName() { + public function testSetName(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->node->setName('x'); @@ -705,13 +698,13 @@ public function testSetName() { /** */ - public function testCreateDirectory() { + public function testCreateDirectory(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->node->createDirectory('somedir'); } - public function providesUpdateDisplayNameValidateException() { + public function providesUpdateDisplayNameValidateException(): array { return [ [''], [null], @@ -727,7 +720,7 @@ public function providesUpdateDisplayNameValidateException() { * @dataProvider providesUpdateDisplayNameValidateException * @param string $groupName */ - public function testUpdateDisplayNameValidateException($groupName) { + public function testUpdateDisplayNameValidateException($groupName): void { $this->expectException(\OCA\CustomGroups\Exception\ValidationException::class); $this->node->updateDisplayName($groupName); diff --git a/tests/unit/Dav/GroupsCollectionTest.php b/tests/unit/Dav/GroupsCollectionTest.php index 7e943a82..379412d2 100644 --- a/tests/unit/Dav/GroupsCollectionTest.php +++ b/tests/unit/Dav/GroupsCollectionTest.php @@ -23,6 +23,7 @@ use OCA\CustomGroups\Dav\GroupsCollection; use OCA\CustomGroups\CustomGroupsDatabaseHandler; use OCA\CustomGroups\Exception\ValidationException; +use OCA\CustomGroups\Service\GuestIntegrationHelper; use OCP\IUserManager; use OCP\IUserSession; use OCP\IUser; @@ -33,7 +34,6 @@ use OCP\IURLGenerator; use OCP\Notification\IManager; use OCP\IConfig; -use Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher; use Symfony\Component\EventDispatcher\GenericEvent; use Sabre\DAV\MkCol; @@ -58,16 +58,6 @@ class GroupsCollectionTest extends \Test\TestCase { */ private $helper; - /** - * @var IUserManager - */ - private $userManager; - - /** - * @var IGroupManager - */ - private $groupManager; - /** * @var IUserSession */ @@ -82,26 +72,28 @@ public function setUp(): void { parent::setUp(); $this->handler = $this->createMock(CustomGroupsDatabaseHandler::class); $this->handler->expects($this->never())->method('getGroup'); - $this->userManager = $this->createMock(IUserManager::class); - $this->groupManager = $this->createMock(IGroupManager::class); + $userManager = $this->createMock(IUserManager::class); + $groupManager = $this->createMock(IGroupManager::class); $this->userSession = $this->createMock(IUserSession::class); $this->config = $this->createMock(IConfig::class); $this->config->method('getAppValue') ->with() - ->will($this->returnValueMap([ + ->willReturnMap([ ['customgroups', 'allow_duplicate_names', 'false', false], ['customgroups', 'only_subadmin_can_create', 'false', false], - ])); + ]); + $this->guestIntegrationHelper = $this->createMock(GuestIntegrationHelper::class); $this->helper = new MembershipHelper( $this->handler, $this->userSession, - $this->userManager, - $this->groupManager, + $userManager, + $groupManager, $this->createMock(IManager::class), $this->createMock(IURLGenerator::class), - $this->config + $this->config, + $this->guestIntegrationHelper ); $this->collection = new GroupsCollection( $this->createMock(IGroupManager::class), @@ -111,19 +103,19 @@ public function setUp(): void { ); } - public function testBase() { + public function testBase(): void { $this->assertEquals('groups', $this->collection->getName()); $this->assertNull($this->collection->getLastModified()); } - public function testListGroups() { + public function testListGroups(): void { $this->handler->expects($this->once()) ->method('getGroups') ->with(null) - ->will($this->returnValue([ + ->willReturn([ ['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One'], ['group_id' => 2, 'uri' => 'group2', 'display_name' => 'Group Two'], - ])); + ]); $nodes = $this->collection->getChildren(); $this->assertCount(2, $nodes); @@ -134,15 +126,15 @@ public function testListGroups() { $this->assertEquals('group2', $nodes[1]->getName()); } - public function testListGroupsSearchPattern() { + public function testListGroupsSearchPattern(): void { $search = new Search('gr', 16, 256); $this->handler->expects($this->once()) ->method('getGroups') ->with($search) - ->will($this->returnValue([ + ->willReturn([ ['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One'], ['group_id' => 2, 'uri' => 'group2', 'display_name' => 'Group Two'], - ])); + ]); $nodes = $this->collection->search($search); $this->assertCount(2, $nodes); @@ -153,7 +145,7 @@ public function testListGroupsSearchPattern() { $this->assertEquals('group2', $nodes[1]->getName()); } - public function testListGroupsForUser() { + public function testListGroupsForUser(): void { $collection = new GroupsCollection( $this->createMock(IGroupManager::class), $this->handler, @@ -167,10 +159,10 @@ public function testListGroupsForUser() { $this->handler->expects($this->once()) ->method('getUserMemberships') ->with('user1', null) - ->will($this->returnValue([ + ->willReturn([ ['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One'], ['group_id' => 2, 'uri' => 'group2', 'display_name' => 'Group Two'], - ])); + ]); $nodes = $collection->getChildren(); $this->assertCount(2, $nodes); @@ -181,7 +173,7 @@ public function testListGroupsForUser() { $this->assertEquals('group2', $nodes[1]->getName()); } - public function testListGroupsForUserSearchPattern() { + public function testListGroupsForUserSearchPattern(): void { $search = new Search('gr', 16, 256); $collection = new GroupsCollection( @@ -195,10 +187,10 @@ public function testListGroupsForUserSearchPattern() { $this->handler->expects($this->once()) ->method('getUserMemberships') ->with('user1', $search) - ->will($this->returnValue([ + ->willReturn([ ['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One'], ['group_id' => 2, 'uri' => 'group2', 'display_name' => 'Group Two'], - ])); + ]); $nodes = $collection->search($search); $this->assertCount(2, $nodes); @@ -209,7 +201,7 @@ public function testListGroupsForUserSearchPattern() { $this->assertEquals('group2', $nodes[1]->getName()); } - public function testCreateGroup() { + public function testCreateGroup(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user1'); $this->userSession->method('getUser')->willReturn($user); @@ -217,11 +209,11 @@ public function testCreateGroup() { $this->handler->expects($this->once()) ->method('getGroupsByDisplayName') ->with('Group One') - ->will($this->returnValue([])); + ->willReturn([]); $this->handler->expects($this->once()) ->method('createGroup') ->with('group1', 'Group One') - ->will($this->returnValue(1)); + ->willReturn(1); $this->handler->expects($this->once()) ->method('addToGroup') ->with('user1', 1, true); @@ -238,7 +230,7 @@ public function testCreateGroup() { $this->collection->createExtendedCollection('group1', $mkCol); $this->assertSame('\OCA\CustomGroups::addGroupAndUser', $called[0]); - $this->assertTrue($called[1] instanceof GenericEvent); + $this->assertInstanceOf(GenericEvent::class, $called[1]); $this->assertArrayHasKey('groupName', $called[1]); $this->assertArrayHasKey('user', $called[1]); $this->assertArrayHasKey('groupId', $called[1]); @@ -251,7 +243,7 @@ public function testCreateGroup() { /** */ - public function testCreateGroupNoDuplicates() { + public function testCreateGroupNoDuplicates(): void { $this->expectException(\Sabre\DAV\Exception\Conflict::class); $user = $this->createMock(IUser::class); @@ -261,7 +253,7 @@ public function testCreateGroupNoDuplicates() { $this->handler->expects($this->once()) ->method('getGroupsByDisplayName') ->with('Group One') - ->will($this->returnValue([['duplicate']])); + ->willReturn([['duplicate']]); $called = []; \OC::$server->getEventDispatcher()->addListener('addGroupAndUser', function ($event) use (&$called) { @@ -275,7 +267,7 @@ public function testCreateGroupNoDuplicates() { $this->collection->createExtendedCollection('group1', $mkCol); } - public function providesTestCreateException() { + public function providesTestCreateException(): array { return [ ['', 'empty'], [null, 'empty'], @@ -290,7 +282,7 @@ public function providesTestCreateException() { /** * @dataProvider providesTestCreateException */ - public function testCreateGroupExceptions($groupName, $displayName) { + public function testCreateGroupExceptions($groupName, $displayName): void { $this->expectException(\OCA\CustomGroups\Exception\ValidationException::class); $mkCol = new MkCol([], [ @@ -304,7 +296,7 @@ public function testCreateGroupExceptions($groupName, $displayName) { * @dataProvider providesTestCreateException * @throws ValidationException */ - public function testCreateGroupExceptionsStatusCode($groupName, $displayName) { + public function testCreateGroupExceptionsStatusCode($groupName, $displayName): void { $mkCol = new MkCol([], [ GroupMembershipCollection::PROPERTY_DISPLAY_NAME => $displayName ]); @@ -318,7 +310,7 @@ public function testCreateGroupExceptionsStatusCode($groupName, $displayName) { /** */ - public function testCreateGroupNoPermission() { + public function testCreateGroupNoPermission(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $helper = $this->createMock(MembershipHelper::class); @@ -343,13 +335,13 @@ public function testCreateGroupNoPermission() { /** */ - public function testCreateGroupAlreadyExists() { + public function testCreateGroupAlreadyExists(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->handler->expects($this->once()) ->method('createGroup') ->with('group1', 'group1') - ->will($this->returnValue(null)); + ->willReturn(null); $this->handler->expects($this->never()) ->method('addToGroup') ->with('user1', 1, true); @@ -357,11 +349,11 @@ public function testCreateGroupAlreadyExists() { $this->collection->createDirectory('group1'); } - public function testGetGroup() { + public function testGetGroup(): void { $this->handler->expects($this->any()) ->method('getGroupByUri') ->with('group1') - ->will($this->returnValue(['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One'])); + ->willReturn(['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One']); $groupNode = $this->collection->getChild('group1'); $this->assertInstanceOf(GroupMembershipCollection::class, $groupNode); @@ -370,24 +362,24 @@ public function testGetGroup() { /** */ - public function testGetGroupNonExisting() { + public function testGetGroupNonExisting(): void { $this->expectException(\Sabre\DAV\Exception\NotFound::class); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupByUri') ->with('groupx') - ->will($this->returnValue(null)); + ->willReturn(null); $this->collection->getChild('groupx'); } - public function testGroupExists() { - $this->handler->expects($this->any()) + public function testGroupExists(): void { + $this->handler ->method('getGroupByUri') - ->will($this->returnValueMap([ + ->willReturnMap([ ['group1', ['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One']], ['group2', null], - ])); + ]); $this->assertTrue($this->collection->childExists('group1')); $this->assertFalse($this->collection->childExists('group2')); @@ -395,7 +387,7 @@ public function testGroupExists() { /** */ - public function testSetName() { + public function testSetName(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->setName('x'); @@ -403,7 +395,7 @@ public function testSetName() { /** */ - public function testDelete() { + public function testDelete(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->delete(); @@ -411,7 +403,7 @@ public function testDelete() { /** */ - public function testCreateFile() { + public function testCreateFile(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->createFile('somefile.txt'); diff --git a/tests/unit/Dav/MembershipNodeTest.php b/tests/unit/Dav/MembershipNodeTest.php index 122fd200..8d39ff6c 100644 --- a/tests/unit/Dav/MembershipNodeTest.php +++ b/tests/unit/Dav/MembershipNodeTest.php @@ -22,6 +22,7 @@ use OCA\CustomGroups\Dav\MembershipNode; use OCA\CustomGroups\CustomGroupsDatabaseHandler; +use OCA\CustomGroups\Service\GuestIntegrationHelper; use OCP\IUserManager; use OCP\IUserSession; use OCP\IUser; @@ -69,18 +70,13 @@ class MembershipNodeTest extends \Test\TestCase { */ private $groupManager; - /** - * @var IUserSession - */ - private $userSession; - /** @var IConfig */ private $config; public function setUp(): void { parent::setUp(); $this->handler = $this->createMock(CustomGroupsDatabaseHandler::class); - $this->userSession = $this->createMock(IUserSession::class); + $userSession = $this->createMock(IUserSession::class); $this->userManager = $this->createMock(IUserManager::class); $this->groupManager = $this->createMock(IGroupManager::class); $this->config = $this->createMock(IConfig::class); @@ -88,20 +84,22 @@ public function setUp(): void { // currently logged in user $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn(self::CURRENT_USER); - $this->userSession->expects($this->any()) + $userSession ->method('getUser') ->willReturn($user); + $this->guestIntegrationHelper = $this->createMock(GuestIntegrationHelper::class); $this->helper = $this->getMockBuilder(MembershipHelper::class) ->setMethods(['notifyUserRoleChange', 'notifyUserRemoved', 'isTheOnlyAdmin']) ->setConstructorArgs([ $this->handler, - $this->userSession, + $userSession, $this->userManager, $this->groupManager, $this->createMock(IManager::class), $this->createMock(IURLGenerator::class), - $this->config + $this->config, + $this->guestIntegrationHelper ]) ->getMock(); @@ -119,19 +117,19 @@ public function setUp(): void { * * @param array $memberInfo user member info */ - private function setCurrentUserMemberInfo($memberInfo) { - $this->handler->expects($this->any()) + private function setCurrentUserMemberInfo($memberInfo): void { + $this->handler ->method('getGroupMemberInfo') ->with(1, self::CURRENT_USER) ->willReturn($memberInfo); } - public function testBase() { + public function testBase(): void { $this->assertEquals(self::NODE_USER, $this->node->getName()); $this->assertNull($this->node->getLastModified()); } - public function testNodeName() { + public function testNodeName(): void { $node = new MembershipNode( ['group_id' => 1, 'uri' => 'group1', 'user_id' => self::NODE_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN], 'group1', @@ -142,7 +140,7 @@ public function testNodeName() { $this->assertEquals('group1', $node->getName()); } - public function testDeleteAsAdmin() { + public function testDeleteAsAdmin(): void { $memberInfo = ['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]; $this->config->method('getSystemValue') @@ -160,8 +158,7 @@ public function testDeleteAsAdmin() { ->method('notifyUserRemoved') ->with( self::NODE_USER, - ['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One'], - ['group_id' => 1, 'user_id' => self::NODE_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN] + ['group_id' => 1, 'uri' => 'group1', 'display_name' => 'Group One'] ); $this->helper->expects($this->once()) @@ -171,7 +168,7 @@ public function testDeleteAsAdmin() { $searchAdmins = new Search(); $searchAdmins->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmins) ->willReturn([$memberInfo]); @@ -190,7 +187,7 @@ public function testDeleteAsAdmin() { $this->node->delete(); $this->assertSame('\OCA\CustomGroups::removeUserFromGroup', $called[0]); - $this->assertTrue($called[1] instanceof GenericEvent); + $this->assertInstanceOf(GenericEvent::class, $called[1]); $this->assertArrayHasKey('user_displayName', $called[1]); $this->assertArrayHasKey('group_displayName', $called[1]); $this->assertEquals('customGroups.removeUserFromGroup', $newCalled[0]); @@ -204,7 +201,7 @@ public function testDeleteAsAdmin() { /** */ - public function testDeleteAsAdminFailed() { + public function testDeleteAsAdminFailed(): void { $this->expectException(\Sabre\DAV\Exception\PreconditionFailed::class); $memberInfo = ['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]; @@ -221,7 +218,7 @@ public function testDeleteAsAdminFailed() { $searchAdmins = new Search(); $searchAdmins->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmins) ->willReturn([$memberInfo]); @@ -231,7 +228,7 @@ public function testDeleteAsAdminFailed() { /** */ - public function testDeleteAsNonAdmin() { + public function testDeleteAsNonAdmin(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->setCurrentUserMemberInfo(['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_MEMBER]); @@ -252,11 +249,12 @@ public function testDeleteAsNonAdmin() { * @param int $role admin perms for the NODE_USER * @return MembershipNode new node */ - private function makeSelfNode($role) { + private function makeSelfNode($role): MembershipNode { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn(self::NODE_USER); $userSession = $this->createMock(IUserSession::class); $userSession->method('getUser')->willReturn($user); + $guestIntegrationHelper = $this->createMock(GuestIntegrationHelper::class); $helper = new MembershipHelper( $this->handler, @@ -265,7 +263,8 @@ private function makeSelfNode($role) { $this->groupManager, $this->createMock(IManager::class), $this->createMock(IURLGenerator::class), - $this->config + $this->config, + $guestIntegrationHelper ); $memberInfo = ['group_id' => 1, 'user_id' => self::NODE_USER, 'role' => $role]; @@ -276,14 +275,14 @@ private function makeSelfNode($role) { $this->handler, $helper ); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMemberInfo') ->with(1, self::NODE_USER) ->willReturn($memberInfo); return $node; } - public function testDeleteSelfAsNonAdmin() { + public function testDeleteSelfAsNonAdmin(): void { $node = $this->makeSelfNode(CustomGroupsDatabaseHandler::ROLE_MEMBER); $this->config->method('getSystemValue') @@ -337,7 +336,7 @@ public function testDeleteSelfAsNonAdmin() { /** */ - public function testDeleteAsNonMember() { + public function testDeleteAsNonMember(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->config->method('getSystemValue') @@ -354,7 +353,7 @@ public function testDeleteAsNonMember() { /** * Super admin can delete any member */ - public function testDeleteAsSuperAdmin() { + public function testDeleteAsSuperAdmin(): void { $this->config->method('getSystemValue') ->willReturn(false); $this->groupManager->method('isAdmin') @@ -371,7 +370,7 @@ public function testDeleteAsSuperAdmin() { $searchAdmins = new Search(); $searchAdmins->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmins) ->willReturn([['user_id' => 'adminuser']]); @@ -381,7 +380,7 @@ public function testDeleteAsSuperAdmin() { /** */ - public function testDeleteSelfAsLastAdmin() { + public function testDeleteSelfAsLastAdmin(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->config->method('getSystemValue') @@ -393,7 +392,7 @@ public function testDeleteSelfAsLastAdmin() { $searchAdmin = new Search(); $searchAdmin->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmin) ->willReturn([ @@ -408,7 +407,7 @@ public function testDeleteSelfAsLastAdmin() { /** */ - public function testDeleteLastAdminAsSuperAdmin() { + public function testDeleteLastAdminAsSuperAdmin(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->config->method('getSystemValue') @@ -424,7 +423,7 @@ public function testDeleteLastAdminAsSuperAdmin() { $searchAdmin = new Search(); $searchAdmin->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmin) ->willReturn([ @@ -437,7 +436,7 @@ public function testDeleteLastAdminAsSuperAdmin() { $node->delete(); } - public function propsProvider() { + public function propsProvider(): array { return [ [ MembershipNode::PROPERTY_ROLE, @@ -459,7 +458,7 @@ public function propsProvider() { /** * @dataProvider propsProvider */ - public function testGetProperties($propName, $propValue, $roleValue = 0) { + public function testGetProperties($propName, $propValue, $roleValue = 0): void { $node = new MembershipNode( ['group_id' => 1, 'user_id' => self::NODE_USER, 'role' => $roleValue, 'uri' => 'group1'], self::NODE_USER, @@ -474,7 +473,7 @@ public function testGetProperties($propName, $propValue, $roleValue = 0) { $this->assertSame($propValue, $props[$propName]); } - public function adminSetFlagProvider() { + public function adminSetFlagProvider(): array { return [ // admin can change flag for others [false, CustomGroupsDatabaseHandler::ROLE_ADMIN, Roles::DAV_ROLE_ADMIN, 200, true], @@ -496,7 +495,7 @@ public function adminSetFlagProvider() { /** * @dataProvider adminSetFlagProvider */ - public function testSetProperties($isSuperAdmin, $currentUserRole, $roleToSet, $statusCode, $called) { + public function testSetProperties($isSuperAdmin, $currentUserRole, $roleToSet, $statusCode, $called): void { $this->config->method('getSystemValue') ->willReturn(false); $this->groupManager->method('isAdmin') @@ -533,7 +532,7 @@ public function testSetProperties($isSuperAdmin, $currentUserRole, $roleToSet, $ $searchAdmin = new Search(); $searchAdmin->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmin) ->willReturn([ @@ -552,7 +551,7 @@ public function testSetProperties($isSuperAdmin, $currentUserRole, $roleToSet, $ /** * Cannot remove admin perms from last admin */ - public function testUnsetSelfAdminWhenLastAdmin() { + public function testUnsetSelfAdminWhenLastAdmin(): void { $this->config->method('getSystemValue') ->willReturn(false); $this->groupManager->method('isAdmin') @@ -564,7 +563,7 @@ public function testUnsetSelfAdminWhenLastAdmin() { $searchAdmin = new Search(); $searchAdmin->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmin) ->willReturn([ @@ -586,7 +585,7 @@ public function testUnsetSelfAdminWhenLastAdmin() { /** * Cannot remove admin perms from last admin */ - public function testUnsetdminWhenLastAdminAsSuperAdmin() { + public function testUnsetdminWhenLastAdminAsSuperAdmin(): void { $this->config->method('getSystemValue') ->willReturn(true); $this->groupManager->method('isAdmin') @@ -599,7 +598,7 @@ public function testUnsetdminWhenLastAdminAsSuperAdmin() { $searchAdmin = new Search(); $searchAdmin->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMembers') ->with(1, $searchAdmin) ->willReturn([ @@ -617,7 +616,7 @@ public function testUnsetdminWhenLastAdminAsSuperAdmin() { /** */ - public function testSetName() { + public function testSetName(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->node->setName('x'); diff --git a/tests/unit/Dav/UsersCollectionTest.php b/tests/unit/Dav/UsersCollectionTest.php index b5421495..16b797ba 100644 --- a/tests/unit/Dav/UsersCollectionTest.php +++ b/tests/unit/Dav/UsersCollectionTest.php @@ -22,6 +22,7 @@ use OCA\CustomGroups\Dav\UsersCollection; use OCA\CustomGroups\CustomGroupsDatabaseHandler; +use OCA\CustomGroups\Service\GuestIntegrationHelper; use OCP\IUserManager; use OCP\IUserSession; use OCP\IUser; @@ -40,79 +41,58 @@ class UsersCollectionTest extends \Test\TestCase { public const USER = 'user1'; - /** - * @var CustomGroupsDatabaseHandler - */ - private $handler; - /** * @var UsersCollection */ private $collection; - /** - * @var MembershipHelper - */ - private $helper; - - /** - * @var IUserManager - */ - private $userManager; - /** * @var IGroupManager */ private $groupManager; - /** - * @var IUserSession - */ - private $userSession; - - /** @var IConfig */ - private $config; - public function setUp(): void { parent::setUp(); - $this->handler = $this->createMock(CustomGroupsDatabaseHandler::class); - $this->handler->expects($this->never())->method('getGroup'); - $this->userSession = $this->createMock(IUserSession::class); - $this->userManager = $this->createMock(IUserManager::class); + $handler = $this->createMock(CustomGroupsDatabaseHandler::class); + $handler->expects($this->never())->method('getGroup'); + $userSession = $this->createMock(IUserSession::class); + $userManager = $this->createMock(IUserManager::class); $this->groupManager = $this->createMock(IGroupManager::class); $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn(self::USER); - $this->userSession->method('getUser')->willReturn($user); + $userSession->method('getUser')->willReturn($user); - $this->config = $this->createMock(IConfig::class); + $config = $this->createMock(IConfig::class); + $this->guestIntegrationHelper = $this->createMock(GuestIntegrationHelper::class); - $this->helper = new MembershipHelper( - $this->handler, - $this->userSession, - $this->userManager, + $helper = new MembershipHelper( + $handler, + $userSession, + $userManager, $this->groupManager, $this->createMock(IManager::class), $this->createMock(IURLGenerator::class), - $this->config + $config, + $this->guestIntegrationHelper ); $this->collection = new UsersCollection( $this->createMock(IGroupManager::class), - $this->handler, - $this->helper, - $this->config + $handler, + $helper, + $config ); } - public function testBase() { + public function testBase(): void { $this->assertEquals('users', $this->collection->getName()); $this->assertNull($this->collection->getLastModified()); } /** */ - public function testListUsers() { + public function testListUsers(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->getChildren(); @@ -120,13 +100,13 @@ public function testListUsers() { /** */ - public function testCreateUser() { + public function testCreateUser(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->createDirectory('user1'); } - public function testGetCurrentUser() { + public function testGetCurrentUser(): void { $membershipCollection = $this->collection->getChild(self::USER); $this->assertInstanceOf(GroupsCollection::class, $membershipCollection); $this->assertEquals(self::USER, $membershipCollection->getName()); @@ -134,30 +114,30 @@ public function testGetCurrentUser() { /** */ - public function testGetAnotherUser() { + public function testGetAnotherUser(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); $this->collection->getChild('another'); } - public function testGetAnotherUserAsAdmin() { + public function testGetAnotherUserAsAdmin(): void { $this->groupManager->method('isAdmin')->with(self::USER)->willReturn(true); $membershipCollection = $this->collection->getChild('another'); $this->assertInstanceOf(GroupsCollection::class, $membershipCollection); $this->assertEquals('another', $membershipCollection->getName()); } - public function testUserExistsCurrent() { + public function testUserExistsCurrent(): void { $this->assertTrue($this->collection->childExists(self::USER)); } - public function testUserExistsAnother() { + public function testUserExistsAnother(): void { $this->assertFalse($this->collection->childExists('another')); } /** */ - public function testSetName() { + public function testSetName(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->setName('x'); @@ -165,7 +145,7 @@ public function testSetName() { /** */ - public function testDelete() { + public function testDelete(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->delete(); @@ -173,7 +153,7 @@ public function testDelete() { /** */ - public function testCreateFile() { + public function testCreateFile(): void { $this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class); $this->collection->createFile('somefile.txt'); diff --git a/tests/unit/PageControllerTest.php b/tests/unit/PageControllerTest.php index 70e757ae..1dfafe21 100644 --- a/tests/unit/PageControllerTest.php +++ b/tests/unit/PageControllerTest.php @@ -21,7 +21,7 @@ namespace OCA\CustomGroups\Tests\unit; use OCA\CustomGroups\CustomGroupsDatabaseHandler; -use OCA\CustomGroups\Service\MembershipHelper; +use OCA\CustomGroups\Service\GuestIntegrationHelper; use OCA\CustomGroups\Controller\PageController; use OCP\IRequest; use OCP\IUser; @@ -75,6 +75,7 @@ public function setUp(): void { $this->userSession = $this->createMock(IUserSession::class); $this->userManager = $this->createMock(IUserManager::class); $this->groupManager = $this->createMock(IGroupManager::class); + $this->guestIntegrationHelper = $this->createMock(GuestIntegrationHelper::class); $this->pageController = new PageController( 'customgroups', @@ -83,7 +84,8 @@ public function setUp(): void { $this->userSession, $this->userManager, $this->groupManager, - $this->handler + $this->handler, + $this->guestIntegrationHelper ); } @@ -108,17 +110,17 @@ private function makeUser($uid, $displayName, $email = '', $searchTerms = null) return $user; } - public function testSearchAllUsers() { + public function testSearchAllUsers(): void { $user1 = $this->makeUser('user1', 'User One'); $user2 = $this->makeUser('user2', 'User Two'); $user3 = $this->makeUser('user3', 'User Three'); $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'no'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -141,12 +143,12 @@ public function testSearchAllUsers() { $this->assertTrue(isset($data['results'])); $this->assertEquals([ // user3 excluded because already a member - ['userId' => 'user1', 'displayName' => 'User One'], - ['userId' => 'user2', 'displayName' => 'User Two'], + ['userId' => 'user1', 'displayName' => 'User One', 'type' => 'user'], + ['userId' => 'user2', 'displayName' => 'User Two', 'type' => 'user'], ], $data['results']); } - public function testSearchAllUsersLimit() { + public function testSearchAllUsersLimit(): void { $allUsers = []; for ($i = 1; $i < 25; $i++) { @@ -154,11 +156,11 @@ public function testSearchAllUsersLimit() { } $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'no'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -197,7 +199,7 @@ public function testSearchAllUsersLimit() { // user3 was already member and is excluded continue; } - $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User ' . $i]; + $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User ' . $i, 'type' => 'user']; } $this->assertEquals( @@ -206,13 +208,13 @@ public function testSearchAllUsersLimit() { ); } - public function exactSearchDataProvider() { + public function exactSearchDataProvider(): array { $singleResult = [ - ['userId' => 'user1', 'displayName' => 'User One'], + ['userId' => 'user1', 'displayName' => 'User One', 'type' => 'user'], ]; $doubleResult = [ - ['userId' => 'user1', 'displayName' => 'User One'], - ['userId' => 'userone', 'displayName' => 'User One'], + ['userId' => 'user1', 'displayName' => 'User One', 'type' => 'user'], + ['userId' => 'userone', 'displayName' => 'User One', 'type' => 'user'], ]; return [ // partial fails @@ -230,7 +232,7 @@ public function exactSearchDataProvider() { /** * @dataProvider exactSearchDataProvider */ - public function testSearchAllUsersExact($searchPattern, $expectedResults) { + public function testSearchAllUsersExact($searchPattern, $expectedResults): void { $user1 = $this->makeUser('user1', 'User One', 'test@example.com', ['meh', 'moo']); $user2 = $this->makeUser('user2', 'User Two'); $userone = $this->makeUser('userone', 'User One'); // sometimes people have same display names @@ -238,11 +240,11 @@ public function testSearchAllUsersExact($searchPattern, $expectedResults) { $membertwo = $this->makeUser('membertwo', 'Member Two'); // same name but already member $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'no'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -266,7 +268,7 @@ public function testSearchAllUsersExact($searchPattern, $expectedResults) { $this->assertEquals($expectedResults, $data['results']); } - public function testSearchAllUsersExactLimit() { + public function testSearchAllUsersExactLimit(): void { $allUsers = []; // imagine a world where lots of people have the same names for ($i = 1; $i < 25; $i++) { @@ -274,11 +276,11 @@ public function testSearchAllUsersExactLimit() { } $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'no'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -315,7 +317,7 @@ public function testSearchAllUsersExactLimit() { // user3 was already member and is excluded continue; } - $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User One']; + $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User One', 'type' => 'user']; } $this->assertEquals( @@ -324,17 +326,17 @@ public function testSearchAllUsersExactLimit() { ); } - public function testSearchUsersInGroup() { + public function testSearchUsersInGroup(): void { $user1 = $this->makeUser('user1', 'User One'); $user2 = $this->makeUser('user2', 'User Two'); $user3 = $this->makeUser('user3', 'User Three'); $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'yes'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $currentUser = $this->makeUser('currentuser', 'Current User'); $this->userSession->method('getUser')->willReturn($currentUser); @@ -344,12 +346,12 @@ public function testSearchUsersInGroup() { ->with($currentUser) ->willReturn(['group1', 'group2']); - $this->groupManager->expects($this->any()) + $this->groupManager ->method('findUsersInGroup') - ->will($this->returnValueMap([ + ->willReturnMap([ ['group1', 'us', 150, 0, ['user1' => $user1, 'user2' => $user2]], ['group2', 'us', 150, 0, ['user2' => $user2, 'user3' => $user3]], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -367,12 +369,12 @@ public function testSearchUsersInGroup() { $this->assertTrue(isset($data['results'])); $this->assertEquals([ - ['userId' => 'user1', 'displayName' => 'User One'], - ['userId' => 'user2', 'displayName' => 'User Two'], + ['userId' => 'user1', 'displayName' => 'User One', 'type' => 'user'], + ['userId' => 'user2', 'displayName' => 'User Two', 'type' => 'user'], ], $data['results']); } - public function groupDataProvider() { + public function groupDataProvider(): array { $allUsers = []; for ($i = 1; $i < 100; $i++) { @@ -416,13 +418,13 @@ public function groupDataProvider() { /** * @dataProvider groupDataProvider */ - public function testSearchUsersInGroupLimit($allUsers, $groupData) { + public function testSearchUsersInGroupLimit($allUsers, $groupData): void { $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'yes'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $currentUser = $this->makeUser('currentuser', 'Current User'); $this->userSession->method('getUser')->willReturn($currentUser); @@ -432,9 +434,9 @@ public function testSearchUsersInGroupLimit($allUsers, $groupData) { ->with($currentUser) ->willReturn(['group1', 'group2', 'group3']); - $this->groupManager->expects($this->any()) + $this->groupManager ->method('findUsersInGroup') - ->will($this->returnValueMap($groupData)); + ->willReturnMap($groupData); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -459,7 +461,7 @@ public function testSearchUsersInGroupLimit($allUsers, $groupData) { // user3 was already member and is excluded continue; } - $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User ' . $i]; + $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User ' . $i, 'type' => 'user']; } $this->assertEquals( @@ -471,18 +473,18 @@ public function testSearchUsersInGroupLimit($allUsers, $groupData) { /** * @dataProvider exactSearchDataProvider */ - public function testSearchUsersInGroupExact($searchPattern, $expectedResults) { + public function testSearchUsersInGroupExact($searchPattern, $expectedResults): void { $user1 = $this->makeUser('user1', 'User One', 'test@example.com', ['meh', 'moo']); $user2 = $this->makeUser('user2', 'User Two'); $user3 = $this->makeUser('user3', 'User Three'); $userone = $this->makeUser('userone', 'User One'); // sometimes people have same display names $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'yes'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $currentUser = $this->makeUser('currentuser', 'Current User'); $this->userSession->method('getUser')->willReturn($currentUser); @@ -492,12 +494,12 @@ public function testSearchUsersInGroupExact($searchPattern, $expectedResults) { ->with($currentUser) ->willReturn(['group1', 'group2']); - $this->groupManager->expects($this->any()) + $this->groupManager ->method('findUsersInGroup') - ->will($this->returnValueMap([ + ->willReturnMap([ ['group1', \strtolower($searchPattern), 150, 0, ['user1' => $user1, 'user2' => $user2]], ['group2', \strtolower($searchPattern), 150, 0, ['user2' => $user2, 'user3' => $user3, 'userone' => $userone]], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -519,17 +521,17 @@ public function testSearchUsersInGroupExact($searchPattern, $expectedResults) { $this->assertEquals($expectedResults, $data['results']); } - public function testSearchUsersShareWithGroupOnlyAndEnumGroupMembers() { + public function testSearchUsersShareWithGroupOnlyAndEnumGroupMembers(): void { $user1 = $this->makeUser('user1', 'User One'); $user2 = $this->makeUser('user2', 'User Two'); $user3 = $this->makeUser('user3', 'User Three'); $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'no'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'yes'], - ])); + ]); $currentUser = $this->makeUser('currentuser', 'Current User'); $this->userSession->method('getUser')->willReturn($currentUser); @@ -539,11 +541,11 @@ public function testSearchUsersShareWithGroupOnlyAndEnumGroupMembers() { ->with($currentUser) ->willReturn(['group1']); - $this->groupManager->expects($this->any()) + $this->groupManager ->method('findUsersInGroup') - ->will($this->returnValueMap([ + ->willReturnMap([ ['group1', \strtolower('user3'), 150, 0, []], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -562,11 +564,11 @@ public function testSearchUsersShareWithGroupOnlyAndEnumGroupMembers() { $response = $this->pageController->searchUsers('group1', 'user3', 150); $data = $response->getData(); $this->assertTrue(isset($data['results'])); - $expectedResults = [['userId' => 'user3', 'displayName' => 'User Three']]; + $expectedResults = [['userId' => 'user3', 'displayName' => 'User Three', 'type' => 'user']]; $this->assertEquals($expectedResults, $data['results']); } - public function testSearchUsersInGroupExactLimit() { + public function testSearchUsersInGroupExactLimit(): void { $allUsers = []; for ($i = 1; $i < 25; $i++) { @@ -578,11 +580,11 @@ public function testSearchUsersInGroupExactLimit() { } $this->config->method('getAppValue') - ->will($this->returnValueMap([ + ->willReturnMap([ ['core', 'shareapi_only_share_with_group_members', 'no', 'yes'], ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'], ['core', 'shareapi_share_dialog_user_enumeration_group_members', 'no', 'no'], - ])); + ]); $currentUser = $this->makeUser('currentuser', 'Current User'); $this->userSession->method('getUser')->willReturn($currentUser); @@ -594,14 +596,14 @@ public function testSearchUsersInGroupExactLimit() { $allUsersChunk = \array_chunk($allUsers, 20); - $this->groupManager->expects($this->any()) + $this->groupManager ->method('findUsersInGroup') - ->will($this->returnValueMap([ + ->willReturnMap([ ['group1', 'user one', 20, 0, $allUsersChunk[0]], ['group1', 'user one', 20, 20, $allUsersChunk[1]], ['group1', 'user one', 20, 40, $allUsersChunk[2]], ['group2', 'user one', 20, 0, ['user' => $allUsers[10]]], - ])); + ]); $this->handler->expects($this->once()) ->method('getGroupByUri') @@ -626,7 +628,7 @@ public function testSearchUsersInGroupExactLimit() { // user3 was already member and is excluded continue; } - $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User One']; + $expectedResults[] = ['userId' => 'user' . $i, 'displayName' => 'User One', 'type' => 'user']; } $this->assertEquals( diff --git a/tests/unit/Service/MembershipHelperTest.php b/tests/unit/Service/MembershipHelperTest.php index 917c7c49..4dd756e3 100644 --- a/tests/unit/Service/MembershipHelperTest.php +++ b/tests/unit/Service/MembershipHelperTest.php @@ -20,6 +20,7 @@ */ namespace OCA\CustomGroups\Tests\unit\Service; +use OCA\CustomGroups\Service\GuestIntegrationHelper; use OCP\IUserSession; use OCP\IUserManager; use OCP\IGroupManager; @@ -62,21 +63,11 @@ class MembershipHelperTest extends \Test\TestCase { */ private $groupManager; - /** - * @var IUserSession - */ - private $userSession; - /** * @var IManager */ private $notificationManager; - /** - * @var IURLGenerator - */ - private $urlGenerator; - /** * @var IUser */ @@ -90,39 +81,41 @@ class MembershipHelperTest extends \Test\TestCase { public function setUp(): void { parent::setUp(); $this->handler = $this->createMock(CustomGroupsDatabaseHandler::class); - $this->userSession = $this->createMock(IUserSession::class); + $userSession = $this->createMock(IUserSession::class); $this->userManager = $this->createMock(IUserManager::class); // swap for 10.1, method getSubAdmin is not on the interface in 10.0 $this->groupManager = $this->createMock(\OC\Group\Manager::class); //$this->groupManager = $this->createMock(IGroupManager::class); $this->notificationManager = $this->createMock(IManager::class); - $this->urlGenerator = $this->createMock(IURLGenerator::class); + $urlGenerator = $this->createMock(IURLGenerator::class); $this->config = $this->createMock(IConfig::class); // currently logged in user $this->user = $this->createMock(IUser::class); $this->user->method('getUID')->willReturn(self::CURRENT_USER); $this->user->method('getDisplayName')->willReturn('User One'); - $this->userSession->expects($this->any()) + $userSession ->method('getUser') ->willReturn($this->user); + $this->guestIntegrationHelper = $this->createMock(GuestIntegrationHelper::class); $this->helper = new MembershipHelper( $this->handler, - $this->userSession, + $userSession, $this->userManager, $this->groupManager, $this->notificationManager, - $this->urlGenerator, - $this->config + $urlGenerator, + $this->config, + $this->guestIntegrationHelper ); } - public function testGetUserId() { + public function testGetUserId(): void { $this->assertEquals(self::CURRENT_USER, $this->helper->getUserId()); } - public function testGetUser() { + public function testGetUser(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('anotheruser'); @@ -134,7 +127,7 @@ public function testGetUser() { $this->assertEquals($user, $this->helper->getUser('anotheruser')); } - public function isUserAdminDataProvider() { + public function isUserAdminDataProvider(): array { return [ // regular member [ @@ -172,7 +165,7 @@ public function isUserAdminDataProvider() { /** * @dataProvider isUserAdminDataProvider */ - public function testIsUserAdmin($isSuperAdmin, $memberInfo, $expectedResult) { + public function testIsUserAdmin($isSuperAdmin, $memberInfo, $expectedResult): void { $this->config->method('getSystemValue') ->with('customgroups.disallow-admin-access-all', false) ->willReturn(false); @@ -181,7 +174,7 @@ public function testIsUserAdmin($isSuperAdmin, $memberInfo, $expectedResult) { ->with(self::CURRENT_USER) ->willReturn($isSuperAdmin); - $this->handler->expects($this->any()) + $this->handler ->method('getGroupMemberInfo') ->with('group1', self::CURRENT_USER) ->willReturn($memberInfo); @@ -189,7 +182,7 @@ public function testIsUserAdmin($isSuperAdmin, $memberInfo, $expectedResult) { $this->assertEquals($expectedResult, $this->helper->isUserAdmin('group1')); } - public function testDenyAdminAccess() { + public function testDenyAdminAccess(): void { $this->config->method('getSystemValue') ->with('customgroups.disallow-admin-access-all', false) ->willReturn(true); @@ -202,7 +195,7 @@ public function testDenyAdminAccess() { $this->assertFalse($this->helper->isUserAdmin('group1')); } - public function isUserMemberDataProvider() { + public function isUserMemberDataProvider(): array { return [ // regular member [ @@ -225,7 +218,7 @@ public function isUserMemberDataProvider() { /** * @dataProvider isUserMemberDataProvider */ - public function testIsUserMember($memberInfo, $expectedResult) { + public function testIsUserMember($memberInfo, $expectedResult): void { $this->handler->expects($this->once()) ->method('getGroupMemberInfo') ->with('group1', self::CURRENT_USER) @@ -234,7 +227,7 @@ public function testIsUserMember($memberInfo, $expectedResult) { $this->assertEquals($expectedResult, $this->helper->isUserMember('group1')); } - public function isTheOnlyAdminDataProvider() { + public function isTheOnlyAdminDataProvider(): array { return [ // user is not the last admin [ @@ -264,7 +257,7 @@ public function isTheOnlyAdminDataProvider() { /** * @dataProvider isTheOnlyAdminDataProvider */ - public function testIsTheOnlyAdmin($memberInfo, $expectedResult) { + public function testIsTheOnlyAdmin($memberInfo, $expectedResult): void { $searchAdmins = new Search(); $searchAdmins->setRoleFilter(CustomGroupsDatabaseHandler::ROLE_ADMIN); @@ -308,7 +301,7 @@ private function createExpectedNotification($messageId, $messageParams) { return $notification; } - public function testNotifyUser() { + public function testNotifyUser(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn(self::CURRENT_USER); $user->method('getDisplayName')->willReturn('User One'); @@ -334,7 +327,7 @@ public function testNotifyUser() { ); } - public function testNotifyUserRemoved() { + public function testNotifyUserRemoved(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn(self::CURRENT_USER); $user->method('getDisplayName')->willReturn('User One'); @@ -360,7 +353,7 @@ public function testNotifyUserRemoved() { ); } - public function testNotifyUserRoleChange() { + public function testNotifyUserRoleChange(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn(self::CURRENT_USER); $user->method('getDisplayName')->willReturn('User One'); @@ -392,7 +385,7 @@ public function testNotifyUserRoleChange() { ); $this->assertSame('\OCA\CustomGroups::changeRoleInGroup', $called[0]); - $this->assertTrue($called[1] instanceof GenericEvent); + $this->assertInstanceOf(GenericEvent::class, $called[1]); $this->assertArrayHasKey('user', $called[1]); $this->assertEquals('anotheruser', $called[1]->getArgument('user')); $this->assertArrayHasKey('groupName', $called[1]); @@ -403,7 +396,7 @@ public function testNotifyUserRoleChange() { $this->assertEquals('Member', $called[1]->getArgument('roleDisaplayName')); } - public function canCreateRolesProvider() { + public function canCreateRolesProvider(): array { return [ ['ocadmin', false, true], ['subadmin', false, true], @@ -417,13 +410,13 @@ public function canCreateRolesProvider() { /** * @dataProvider canCreateRolesProvider */ - public function testCanCreateGroups($role, $restrictToSubAdmins, $expectedResult) { + public function testCanCreateGroups($role, $restrictToSubAdmins, $expectedResult): void { $this->config->expects($this->once()) ->method('getAppValue') ->with('customgroups', 'only_subadmin_can_create', 'false') ->willReturn($restrictToSubAdmins ? 'true' : 'false'); - $this->groupManager->expects($this->any()) + $this->groupManager ->method('isAdmin') ->with(self::CURRENT_USER) ->willReturn($role === 'ocadmin'); @@ -431,19 +424,19 @@ public function testCanCreateGroups($role, $restrictToSubAdmins, $expectedResult // TODO: swap for 10.1 as 10.0 doesn't provide this interface //$subadminManager = $this->createMock(ISubAdminManager::class); $subadminManager = $this->createMock(\OC\SubAdmin::class); - $subadminManager->expects($this->any()) + $subadminManager ->method('isSubAdmin') ->with($this->user) ->willReturn($role === 'subadmin'); - $this->groupManager->expects($this->any()) + $this->groupManager ->method('getSubAdmin') ->willReturn($subadminManager); $this->assertEquals($expectedResult, $this->helper->canCreateGroups()); } - public function testIsGroupDisplayNameAvailableWhenDuplicatesAreAllowed() { + public function testIsGroupDisplayNameAvailableWhenDuplicatesAreAllowed(): void { $this->config->expects($this->once()) ->method('getAppValue') ->with('customgroups', 'allow_duplicate_names', 'false') @@ -455,7 +448,7 @@ public function testIsGroupDisplayNameAvailableWhenDuplicatesAreAllowed() { $this->assertTrue($this->helper->isGroupDisplayNameAvailable('test')); } - public function testIsGroupDisplayNameAvailableNoDuplicateExists() { + public function testIsGroupDisplayNameAvailableNoDuplicateExists(): void { $this->config->expects($this->once()) ->method('getAppValue') ->with('customgroups', 'allow_duplicate_names', 'false') @@ -469,7 +462,7 @@ public function testIsGroupDisplayNameAvailableNoDuplicateExists() { $this->assertTrue($this->helper->isGroupDisplayNameAvailable('test')); } - public function testIsGroupDisplayNameAvailableDuplicateExists() { + public function testIsGroupDisplayNameAvailableDuplicateExists(): void { $this->config->expects($this->once()) ->method('getAppValue') ->with('customgroups', 'allow_duplicate_names', 'false') From 5ccfbf5f5149877d2a17bac879c086f49ca44434 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 27 Jun 2022 22:37:23 +0200 Subject: [PATCH 2/6] fix: only add guests where the email domain holds at least one dot --- lib/Service/GuestIntegrationHelper.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/Service/GuestIntegrationHelper.php b/lib/Service/GuestIntegrationHelper.php index eb2cec94..25522b42 100644 --- a/lib/Service/GuestIntegrationHelper.php +++ b/lib/Service/GuestIntegrationHelper.php @@ -76,6 +76,13 @@ public function canBeGuest(string $email): bool { if (!$this->mailer->validateMailAddress($email)) { return false; } + + # in addition, make sure the domain holds at least one . so it matches the guests app logic + [$localPart, $domain] = explode('@', $email, 2); + if (\strpos($domain, '.') === false) { + return false; + } + # test if the correct guest app version is used $mail = $this->getGuestMail(); if ($mail && !method_exists($mail, 'sendGuestPlainInviteMail')) { From 937c7555bd5221cee7d7f596dfe8e9129c284196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 27 Jun 2022 22:55:31 +0200 Subject: [PATCH 3/6] fix: guest member entry after adding --- js/MembersInputView.js | 3 ++- js/MembersView.js | 9 ++++++++- lib/Controller/PageController.php | 2 +- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/js/MembersInputView.js b/js/MembersInputView.js index 3694400b..c8a2772d 100644 --- a/js/MembersInputView.js +++ b/js/MembersInputView.js @@ -152,7 +152,8 @@ e.preventDefault(); this.trigger('select', { userId: s.item.userId, - displayName: s.item.displayName + displayName: s.item.displayName, + type: s.item.type }); $(e.target).val(s.item.userId).blur(); } diff --git a/js/MembersView.js b/js/MembersView.js index 7f86c140..b1b1e476 100644 --- a/js/MembersView.js +++ b/js/MembersView.js @@ -202,9 +202,16 @@ var $loading = this.$('.member-input-view .loading'); $loading.removeClass('hidden'); + var displayName = data.displayName || userId + var role = null + if (data.type === 'guest') { + displayName = userId + role = 'guest' + } this.collection.create({ id: userId, - userDisplayName: data.displayName || userId + userDisplayName: displayName, + userTypeInfo: role }, { wait: true, success: function() { diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php index 0f6fcebf..8b8ac596 100644 --- a/lib/Controller/PageController.php +++ b/lib/Controller/PageController.php @@ -299,7 +299,7 @@ public function searchUsers($group, $pattern, $limit = 200) { $results = [ [ 'userId' => $pattern, - 'displayName' => "Add $pattern", + 'displayName' => $pattern, 'type' => 'guest' ] ]; From 9d6907668d17d03589a3efd7e3a2785489bce02b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 12 Jul 2022 10:05:25 +0200 Subject: [PATCH 4/6] fix: js tests --- tests/js/MembersInputViewSpec.js | 3 ++- tests/js/MembersViewSpec.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/js/MembersInputViewSpec.js b/tests/js/MembersInputViewSpec.js index aff54295..5e942a7f 100644 --- a/tests/js/MembersInputViewSpec.js +++ b/tests/js/MembersInputViewSpec.js @@ -133,7 +133,8 @@ describe('MembersInputView test', function() { expect(handler.calledOnce).toEqual(true); expect(handler.getCall(0).args[0]).toEqual({ userId: 'user2', - displayName: 'User Two' + displayName: 'User Two', + type: undefined }); }); diff --git a/tests/js/MembersViewSpec.js b/tests/js/MembersViewSpec.js index 354f3d00..01a3e4fd 100644 --- a/tests/js/MembersViewSpec.js +++ b/tests/js/MembersViewSpec.js @@ -198,7 +198,8 @@ describe('MembersView test', function() { expect(collection.create.calledOnce).toEqual(true); expect(collection.create.getCall(0).args[0]).toEqual({ id: 'newuser', - userDisplayName: 'new user display name' + userDisplayName: 'new user display name', + userTypeInfo: null }); collection.create.yieldTo('success'); From f4e8ae5f916e6eb466822fc9d3830f34d1dc3bf7 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Mon, 18 Jul 2022 10:32:49 +0200 Subject: [PATCH 5/6] Implement batch action for inviting users to groups --- js/MembersInputView.js | 171 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 16 deletions(-) diff --git a/js/MembersInputView.js b/js/MembersInputView.js index c8a2772d..39ae87fc 100644 --- a/js/MembersInputView.js +++ b/js/MembersInputView.js @@ -25,6 +25,9 @@ className: 'member-input-view', + /** @type {string} **/ + batchActionSeparator: ',', + template: function(data) { return OCA.CustomGroups.Templates.membersInput(data); }, @@ -77,16 +80,43 @@ autocompleteHandler: function (search, response) { var self = this; var $loading = this.$el.find('.loading'); + var trimmedSearch = search.term.trim(); this.$field.tooltip('hide'); $loading.removeClass('hidden'); $loading.addClass('inlineblock'); + + if (trimmedSearch.indexOf(this.batchActionSeparator) !== -1) { + return this._getUsersForBatchAction(trimmedSearch).then(function (res) { + $loading.addClass('hidden'); + $loading.removeClass('inlineblock'); + if (res.found.length) { + var labelArray = []; + for (var i = 0; i < res.found.length; i++) { + labelArray.push(res.found[i].displayName); + } + return response({ + osc: { + batch: res.found, + failedBatch: res.notFound, + displayName: labelArray.join(', '), + userId: labelArray.join(', '), + typeInfo: t('core', 'Add multiple users and guests') + } + }); + } + + self._displayError(t('core', 'No users found')); + return response(); + }) + } + $.ajax({ url: OC.generateUrl('/apps/customgroups/members'), contentType: 'application/json', dataType: 'json', data: { group: this.groupUri, - pattern: search.term.trim(), + pattern: trimmedSearch, limit: 200 } }).done(function (result, type, xhr) { @@ -114,15 +144,7 @@ response(entries); } else { var title = t('core', 'No users found for {search}', {search: self.$field.val()}); - self.$field.addClass('error') - .attr('data-original-title', title) - .tooltip('hide') - .tooltip({ - placement: 'bottom', - trigger: 'manual' - }) - .tooltip('fixTitle') - .tooltip('show'); + self._displayError(title); response(); } } @@ -134,10 +156,17 @@ }, autocompleteRenderItem: function($ul, item) { + var typeInfo; + if (item.typeInfo) { + typeInfo = item.typeInfo; + } else { + typeInfo = item.type === 'guest' ? t('customgroups', 'Guest') : t('customgroups', 'User'); + } + var $item = $(this.itemTemplate({ displayName: item.displayName, userId: item.userId, - typeInfo: item.type === 'guest' ? t('customgroups', 'Guest') : t('customgroups', 'User') + typeInfo: typeInfo })); /* jshint camelcase:false */ @@ -150,12 +179,122 @@ _onSelect: function(e, s) { e.preventDefault(); - this.trigger('select', { - userId: s.item.userId, - displayName: s.item.displayName, - type: s.item.type - }); + var members = s.item.batch || [s.item]; + + if (s.item.failedBatch && s.item.failedBatch.length) { + var failedUsersStr = s.item.failedBatch.join(', '); + OC.Notification.show( + t('core', 'Could not add the following users: {users}', {users: failedUsersStr}), + {type: 'error'} + ); + } + + for (var i = 0; i < members.length; i++) { + var member = members[i]; + this.trigger('select', { + userId: member.userId, + displayName: member.displayName, + type: member.type + }); + $(e.target).val(member.userId).blur(); + } $(e.target).val(s.item.userId).blur(); + }, + + /** + * Displays an error, e.g. when the autocomplete doesn't have results. + * + * @param {string} title - title of the error + * @private + */ + _displayError: function(title) { + this.$field.addClass('error') + .attr('data-original-title', title) + .tooltip('hide') + .tooltip({ + placement: 'bottom', + trigger: 'manual' + }) + .tooltip('fixTitle') + .tooltip('show'); + }, + + /** + * Returns a promise which includes all fetched users for batch actions once resolved. + * + * @param {string} search - trimmed search term + * @returns {Promise} + * @private + */ + _getUsersForBatchAction: function(search) { + var foundUsers = []; + var notFound = []; + var promises = []; + var users = Array.from(new Set(search.split(this.batchActionSeparator))); + + for (var i = 0; i < users.length; i++) { + if (!users[i]) { + continue; + } + var user = users[i].trim(); + promises.push( + $.ajax({ + url: OC.generateUrl('/apps/customgroups/members'), + contentType: 'application/json', + dataType: 'json', + context: { user: user }, + data: { + group: this.groupUri, + pattern: user, + limit: 200 + }, + }).done(function (result) { + for (var j = 0; j < result.results.length; j++) { + var userToAdd = result.results[j] + var addUser = true; + + // only add users that match exact with the search term + if (this.user.toLowerCase() !== userToAdd.userId.toLowerCase() + && this.user.toLowerCase() !== userToAdd.displayName.toLowerCase()) { + continue; + } + + // only add new users + for (var k = 0; k < foundUsers.length; k++) { + if (foundUsers[k].userId.toLowerCase() === userToAdd.userId.toLowerCase()) { + addUser = false; + break; + } + } + + if (addUser) { + foundUsers.push(userToAdd); + } + } + }) + ) + } + + return Promise.all(promises).then(function() { + for (var i = 0; i < users.length; i++) { + if (!users[i]) continue; + var user = users[i].trim(); + var userAdded = false; + + for (var j = 0; j < foundUsers.length; j++) { + var search = user.toLowerCase(); + if (search === foundUsers[j].userId.toLowerCase() + || search === foundUsers[j].displayName.toLowerCase()) { + userAdded = true; + break; + } + } + if (!userAdded) { + notFound.push(user); + } + } + return {found: foundUsers, notFound: notFound}; + }); } }); From ac2f32041297afe7d92d0ef161243cb06f59ad6c Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Mon, 18 Jul 2022 10:33:21 +0200 Subject: [PATCH 6/6] Add missing semicolon --- js/MembersInputView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/MembersInputView.js b/js/MembersInputView.js index 39ae87fc..277b825e 100644 --- a/js/MembersInputView.js +++ b/js/MembersInputView.js @@ -250,7 +250,7 @@ }, }).done(function (result) { for (var j = 0; j < result.results.length; j++) { - var userToAdd = result.results[j] + var userToAdd = result.results[j]; var addUser = true; // only add users that match exact with the search term