diff --git a/js/GroupsView.js b/js/GroupsView.js index 9979bec9..1bd31c0f 100644 --- a/js/GroupsView.js +++ b/js/GroupsView.js @@ -42,7 +42,7 @@ } this.collection.on('request', this._onRequest, this); - this.collection.on('sync destroy', this._onEndRequest, this); + this.collection.on('sync destroy error', this._onEndRequest, this); this.collection.on('add', this._onAddModel, this); this.collection.on('change', this._onChangeModel, this); this.collection.on('remove', this._onRemoveGroup, this); @@ -107,9 +107,17 @@ model.save({ displayName: newName }, { + wait: true, error: function(model, response) { - $displayName.text(oldName); - OC.Notification.showTemporary(t('customgroups', 'Could not rename group')); + $displayName.find('.group-display-name').text(oldName); + + // status 422 in case of validation error + if (response.status === 422) { + OC.Notification.showTemporary(t('customgroups', 'A group with this name already exists')); + return; + } else { + OC.Notification.showTemporary(t('customgroups', 'Could not rename group')); + } } }); } @@ -241,6 +249,8 @@ }, error: function(model, response) { // stop at 100 to avoid running for too long... + + // status 405 in case uri/collection already exists if (response.status === 405 && (_.isUndefined(index) || index < 100)) { if (_.isUndefined(index)) { // attempt again with index @@ -249,6 +259,11 @@ self._createGroup(groupName, index + 1); return; } + // status 409 if display name already exists + if (response.status === 409) { + OC.Notification.showTemporary(t('customgroups', 'A group with this name already exists')); + return; + } OC.Notification.showTemporary(t('customgroups', 'Could not create group')); } }); diff --git a/lib/AdminPanel.php b/lib/AdminPanel.php index 6535aa62..b07efabb 100644 --- a/lib/AdminPanel.php +++ b/lib/AdminPanel.php @@ -40,6 +40,8 @@ public function getPanel() { $tmpl = new Template('customgroups', 'admin'); $restrictToSubadmins = $this->config->getAppValue('customgroups', 'only_subadmin_can_create', 'false') === 'true'; $tmpl->assign('onlySubAdminCanCreate', $restrictToSubadmins); + $allowDuplicateNames = $this->config->getAppValue('customgroups', 'allow_duplicate_names', 'false') === 'true'; + $tmpl->assign('allowDuplicateNames', $allowDuplicateNames); return $tmpl; } diff --git a/lib/CustomGroupsDatabaseHandler.php b/lib/CustomGroupsDatabaseHandler.php index 1f7ce09b..f2901241 100644 --- a/lib/CustomGroupsDatabaseHandler.php +++ b/lib/CustomGroupsDatabaseHandler.php @@ -188,15 +188,15 @@ public function getGroupByUri($uri) { * Returns the info for a given group. * * @param string $field field to filter by - * @param string $numericGroupId numeric group id + * @param string $fieldValue field value * @return array|null group info or null if not found * @throws \Doctrine\DBAL\Exception\DriverException in case of database exception */ - private function getGroupBy($field, $numericGroupId) { + private function getGroupBy($field, $fieldValue) { $qb = $this->dbConn->getQueryBuilder(); $cursor = $qb->select(['group_id', 'uri', 'display_name']) ->from('custom_group') - ->where($qb->expr()->eq($field, $qb->createNamedParameter($numericGroupId))) + ->where($qb->expr()->eq($field, $qb->createNamedParameter($fieldValue))) ->execute(); $result = $cursor->fetch(); $cursor->closeCursor(); @@ -208,6 +208,26 @@ private function getGroupBy($field, $numericGroupId) { return $result; } + /** + * Get groups by display name in a case-insensitive manner. + * + * @param string $displayName numeric group id + * @return array[] array of group infos + * @throws \Doctrine\DBAL\Exception\DriverException in case of database exception + */ + public function getGroupsByDisplayName($displayName) { + $qb = $this->dbConn->getQueryBuilder(); + $cursor = $qb->select(['group_id', 'uri', 'display_name']) + ->from('custom_group') + ->where($qb->expr()->eq($qb->createFunction('LOWER(`display_name`)'), $qb->createNamedParameter(strtolower($displayName)))) + ->execute(); + + $results = $cursor->fetchAll(); + $cursor->closeCursor(); + + return $results; + } + /** * Returns the info for all groups. * diff --git a/lib/Dav/GroupMembershipCollection.php b/lib/Dav/GroupMembershipCollection.php index 94d9d456..73dc4d3e 100644 --- a/lib/Dav/GroupMembershipCollection.php +++ b/lib/Dav/GroupMembershipCollection.php @@ -274,6 +274,10 @@ public function updateDisplayName($displayName) { return 403; } + if ($this->groupInfo['display_name'] !== $displayName && !$this->helper->isGroupDisplayNameAvailable($displayName)) { + return 409; + } + $result = $this->groupsHandler->updateGroup( $this->groupInfo['group_id'], $this->groupInfo['uri'], diff --git a/lib/Dav/GroupsCollection.php b/lib/Dav/GroupsCollection.php index 2b52f0c2..4db49d3c 100644 --- a/lib/Dav/GroupsCollection.php +++ b/lib/Dav/GroupsCollection.php @@ -21,7 +21,8 @@ namespace OCA\CustomGroups\Dav; -use Sabre\DAV\ICollection; +use Sabre\DAV\IExtendedCollection; +use Sabre\DAV\MkCol; use Sabre\DAV\Exception\NotFound; use Sabre\DAV\Exception\MethodNotAllowed; use Sabre\DAV\Exception\Forbidden; @@ -30,11 +31,12 @@ use OCA\CustomGroups\Search; use OCA\CustomGroups\Service\MembershipHelper; use Symfony\Component\EventDispatcher\GenericEvent; +use Sabre\DAV\Exception\Conflict; /** * Collection of custom groups */ -class GroupsCollection implements ICollection { +class GroupsCollection implements IExtendedCollection { /** * Custom groups handler @@ -91,17 +93,49 @@ public function createFile($name, $data = null) { throw new MethodNotAllowed('Cannot create regular nodes'); } + public function createDirectory($name) { + if (!$this->helper->canCreateGroups()) { + throw new Forbidden('No permission to create groups'); + } + + $this->createGroup($name, $name); + } + /** * Creates a new custom group * * @param string $name group URI * @throws MethodNotAllowed if the group already exists */ - public function createDirectory($name) { + public function createExtendedCollection($name, Mkcol $mkCol) { if (!$this->helper->canCreateGroups()) { throw new Forbidden('No permission to create groups'); } - $groupId = $this->groupsHandler->createGroup($name, $name); + + $displayName = $name; + + // can't use handle() here as it's called too late + $mutations = $mkCol->getMutations(); + if (isset($mutations[GroupMembershipCollection::PROPERTY_DISPLAY_NAME])) { + $displayName = $mutations[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]; + $mkCol->setResultCode(GroupMembershipCollection::PROPERTY_DISPLAY_NAME, 202); // accepted + } + + $this->createGroup($name, $displayName); + } + + /** + * Creates a group node with the given name and display name + * + * @param string $name group uri + * @param string $displayName group display name + */ + private function createGroup($name, $displayName) { + if (!$this->helper->isGroupDisplayNameAvailable($displayName)) { + throw new Conflict("Group with display name \"$displayName\" already exists"); + } + + $groupId = $this->groupsHandler->createGroup($name, $displayName); if (is_null($groupId)) { throw new MethodNotAllowed("Group with uri \"$name\" already exists"); } diff --git a/lib/Service/MembershipHelper.php b/lib/Service/MembershipHelper.php index 5de38f44..dac19847 100644 --- a/lib/Service/MembershipHelper.php +++ b/lib/Service/MembershipHelper.php @@ -356,4 +356,21 @@ public function canCreateGroups() { || $this->groupManager->getSubAdmin()->isSubAdmin($this->userSession->getUser()) ); } + + /** + * Checks whether the given display name exists, if applicable from the configuration + * + * @param string $displayName group display name + * @return bool true if a duplicate group was found and the operation is not allowed, false if + * no duplicate group was found or duplicates are allowed + */ + public function isGroupDisplayNameAvailable($displayName) { + if ($this->config->getAppValue('customgroups', 'allow_duplicate_names', 'false') === 'true') { + return true; + } + + $groups = $this->groupsHandler->getGroupsByDisplayName($displayName); + + return empty($groups); + } } diff --git a/templates/admin.php b/templates/admin.php index 2fd8dad3..39bc0594 100644 --- a/templates/admin.php +++ b/templates/admin.php @@ -32,4 +32,11 @@ t('Only group admins are allowed to create custom groups'));?>

+

+ /> + +

diff --git a/tests/js/GroupsViewSpec.js b/tests/js/GroupsViewSpec.js index 0c9b7498..67bee6e0 100644 --- a/tests/js/GroupsViewSpec.js +++ b/tests/js/GroupsViewSpec.js @@ -279,7 +279,7 @@ describe('GroupsView test', function() { }); }); - it('shows notification in case of error', function() { + it('shows notification in case of duplicate group error', function() { var notificationStub = sinon.stub(OC.Notification, 'showTemporary'); view.$('[name=groupName]').val('newgroup'); view.$('[name=customGroupsCreationForm]').submit(); @@ -291,9 +291,10 @@ describe('GroupsView test', function() { role: OCA.CustomGroups.ROLE_ADMIN }); - collection.create.yieldTo('error', collection, {status: 404} ); + collection.create.yieldTo('error', collection, {status: 409} ); expect(notificationStub.calledOnce).toEqual(true); + expect(notificationStub.calledWith('A group with this name already exists')).toEqual(true); notificationStub.restore(); }); @@ -350,6 +351,21 @@ describe('GroupsView test', function() { expect(model.save.notCalled).toEqual(true); expect($groupEl.find('input').length).toEqual(0); }); + it('notifies in case of duplicate group error', function() { + var notificationStub = sinon.stub(OC.Notification, 'showTemporary'); + $groupEl.find('.action-rename-group').click(); + $groupEl.find('input').val('Group Renamed'); + $groupEl.find('form').submit(); + expect(model.save.calledOnce).toEqual(true); + model.save.yieldTo('error', model, {status: 422}); + + expect($groupEl.find('input').length).toEqual(0); + + expect(notificationStub.calledOnce).toEqual(true); + expect(notificationStub.calledWith('A group with this name already exists')).toEqual(true); + + notificationStub.restore(); + }); }); describe('delete group', function() { beforeEach(function() { diff --git a/tests/unit/CustomGroupsDatabaseHandlerTest.php b/tests/unit/CustomGroupsDatabaseHandlerTest.php index 580e7873..e9b79e37 100644 --- a/tests/unit/CustomGroupsDatabaseHandlerTest.php +++ b/tests/unit/CustomGroupsDatabaseHandlerTest.php @@ -182,6 +182,25 @@ public function testGetGroups() { $this->assertEquals($group1Id, $results[2]['group_id']); } + public function testGetGroupsByDisplayName() { + $group1Id = $this->handler->createGroup('my_group_1', 'One'); + $group2Id = $this->handler->createGroup('my_group_2', 'Two'); + $group3Id = $this->handler->createGroup('my_group_3', 'One'); + + $results = $this->handler->getGroupsByDisplayName('one'); + + $this->assertCount(2, $results); + + $this->assertEquals('my_group_1', $results[0]['uri']); + $this->assertEquals('One', $results[0]['display_name']); + $this->assertEquals($group1Id, $results[0]['group_id']); + + $this->assertEquals('my_group_3', $results[1]['uri']); + $this->assertEquals('One', $results[1]['display_name']); + $this->assertEquals($group3Id, $results[1]['group_id']); + + } + public function testAddToGroup() { $groupId = $this->handler->createGroup('my_group', 'My Group'); diff --git a/tests/unit/Dav/GroupMembershipCollectionTest.php b/tests/unit/Dav/GroupMembershipCollectionTest.php index fa727e93..9b11bcde 100644 --- a/tests/unit/Dav/GroupMembershipCollectionTest.php +++ b/tests/unit/Dav/GroupMembershipCollectionTest.php @@ -34,6 +34,7 @@ use OCP\Notification\IManager; use OCP\IConfig; use Symfony\Component\EventDispatcher\GenericEvent; +use OCA\CustomGroups\Dav\Roles; /** * Class GroupMembershipCollectionTest @@ -96,7 +97,7 @@ public function setUp() { ])); $this->helper = $this->getMockBuilder(MembershipHelper::class) - ->setMethods(['notifyUser']) + ->setMethods(['notifyUser', 'isGroupDisplayNameAvailable']) ->setConstructorArgs([ $this->handler, $this->userSession, @@ -193,13 +194,13 @@ public function testGetProperties() { public function adminSetFlagProvider() { return [ // admin can change display name - [false, true, 200, true], + [false, Roles::BACKEND_ROLE_ADMIN, 200, true], // non-admin cannot change anything - [false, false, 403, false], + [false, Roles::BACKEND_ROLE_MEMBER, 403, false], // non-member cannot change anything [false, null, 403, false], // super-admin non-member can change anything - [false, true, 200, true], + [false, Roles::BACKEND_ROLE_ADMIN, 200, true], ]; } @@ -215,6 +216,10 @@ public function testSetProperties($isSuperAdmin, $currentUserRole, $statusCode, $this->setCurrentUserMemberInfo(null); } + $this->helper->expects($this->any()) + ->method('isGroupDisplayNameAvailable') + ->willReturn(true); + if ($called) { $this->handler->expects($this->at(1)) ->method('updateGroup') @@ -234,6 +239,25 @@ public function testSetProperties($isSuperAdmin, $currentUserRole, $statusCode, $this->assertEquals($statusCode, $result[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); } + public function testSetDisplayNameNoDuplicates() { + $this->setCurrentUserSuperAdmin(true); + + $this->helper->expects($this->once()) + ->method('isGroupDisplayNameAvailable') + ->willReturn(false); + + $this->handler->expects($this->never()) + ->method('updateGroup'); + + $propPatch = new PropPatch([GroupMembershipCollection::PROPERTY_DISPLAY_NAME => 'Group Renamed']); + $this->node->propPatch($propPatch); + + $propPatch->commit(); + $this->assertEmpty($propPatch->getRemainingMutations()); + $result = $propPatch->getResult(); + $this->assertEquals(409, $result[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); + } + public function rolesProvider() { return [ [false, ['group_id' => 1, 'user_id' => self::CURRENT_USER, 'role' => CustomGroupsDatabaseHandler::ROLE_ADMIN]], diff --git a/tests/unit/Dav/GroupsCollectionTest.php b/tests/unit/Dav/GroupsCollectionTest.php index 4531ae15..62cbf7e7 100644 --- a/tests/unit/Dav/GroupsCollectionTest.php +++ b/tests/unit/Dav/GroupsCollectionTest.php @@ -34,6 +34,7 @@ use OCP\IConfig; use Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher; use Symfony\Component\EventDispatcher\GenericEvent; +use Sabre\DAV\MkCol; /** * Class GroupsCollectionTest @@ -71,6 +72,11 @@ class GroupsCollectionTest extends \Test\TestCase { */ private $userSession; + /** + * @var IConfig + */ + private $config; + public function setUp() { parent::setUp(); $this->handler = $this->createMock(CustomGroupsDatabaseHandler::class); @@ -79,6 +85,14 @@ public function setUp() { $this->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([ + ['customgroups', 'allow_duplicate_names', 'false', false], + ['customgroups', 'only_subadmin_can_create', 'false', false], + ])); + $this->helper = new MembershipHelper( $this->handler, $this->userSession, @@ -86,7 +100,7 @@ public function setUp() { $this->groupManager, $this->createMock(IManager::class), $this->createMock(IURLGenerator::class), - $this->createMock(IConfig::class) + $this->config ); $this->collection = new GroupsCollection($this->handler, $this->helper); } @@ -181,10 +195,14 @@ public function testCreateGroup() { $this->userSession->method('getUser')->willReturn($user); $this->handler->expects($this->at(0)) + ->method('getGroupsByDisplayName') + ->with('Group One') + ->will($this->returnValue([])); + $this->handler->expects($this->at(1)) ->method('createGroup') - ->with('group1', 'group1') + ->with('group1', 'Group One') ->will($this->returnValue(1)); - $this->handler->expects($this->at(1)) + $this->handler->expects($this->at(2)) ->method('addToGroup') ->with('user1', 1, true); @@ -194,12 +212,42 @@ public function testCreateGroup() { array_push($called, $event); }); - $this->collection->createDirectory('group1'); + $mkCol = new MkCol([], [ + GroupMembershipCollection::PROPERTY_DISPLAY_NAME => 'Group One' + ]); + $this->collection->createExtendedCollection('group1', $mkCol); $this->assertSame('addGroupAndUser', $called[0]); $this->assertTrue($called[1] instanceof GenericEvent); $this->assertArrayHasKey('groupName', $called[1]); $this->assertArrayHasKey('user', $called[1]); + + $this->assertEquals(202, $mkCol->getResult()[GroupMembershipCollection::PROPERTY_DISPLAY_NAME]); + } + + /** + * @expectedException \Sabre\DAV\Exception\Conflict + */ + public function testCreateGroupNoDuplicates() { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user1'); + $this->userSession->method('getUser')->willReturn($user); + + $this->handler->expects($this->once()) + ->method('getGroupsByDisplayName') + ->with('Group One') + ->will($this->returnValue([['duplicate']])); + + $called = array(); + \OC::$server->getEventDispatcher()->addListener('addGroupAndUser', function ($event) use (&$called) { + $called[] = 'addGroupAndUser'; + array_push($called, $event); + }); + + $mkCol = new MkCol([], [ + GroupMembershipCollection::PROPERTY_DISPLAY_NAME => 'Group One' + ]); + $this->collection->createExtendedCollection('group1', $mkCol); } /** diff --git a/tests/unit/Service/MembershipHelperTest.php b/tests/unit/Service/MembershipHelperTest.php index 5864b713..f00bd09a 100644 --- a/tests/unit/Service/MembershipHelperTest.php +++ b/tests/unit/Service/MembershipHelperTest.php @@ -547,4 +547,44 @@ public function testCanCreateGroups($role, $restrictToSubAdmins, $expectedResult $this->assertEquals($expectedResult, $this->helper->canCreateGroups()); } + + public function testIsGroupDisplayNameAvailableWhenDuplicatesAreAllowed() { + $this->config->expects($this->once()) + ->method('getAppValue') + ->with('customgroups', 'allow_duplicate_names', 'false') + ->willReturn('true'); + + $this->handler->expects($this->never()) + ->method('getGroupsByDisplayName'); + + $this->assertTrue($this->helper->isGroupDisplayNameAvailable('test')); + } + + public function testIsGroupDisplayNameAvailableNoDuplicateExists() { + $this->config->expects($this->once()) + ->method('getAppValue') + ->with('customgroups', 'allow_duplicate_names', 'false') + ->willReturn('false'); + + $this->handler->expects($this->once()) + ->method('getGroupsByDisplayName') + ->with('test') + ->willReturn([]); + + $this->assertTrue($this->helper->isGroupDisplayNameAvailable('test')); + } + + public function testIsGroupDisplayNameAvailableDuplicateExists() { + $this->config->expects($this->once()) + ->method('getAppValue') + ->with('customgroups', 'allow_duplicate_names', 'false') + ->willReturn('false'); + + $this->handler->expects($this->once()) + ->method('getGroupsByDisplayName') + ->with('test') + ->willReturn([['duplicate']]); + + $this->assertFalse($this->helper->isGroupDisplayNameAvailable('test')); + } }