Prevent duplicate display names

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'));
 				}
 			});

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;
 	}
 

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.
 	 *

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'],

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");
 		}

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);
+	}
 }

templates/admin.php

@@ -32,4 +32,11 @@
 			<?php p($l->t('Only group admins are allowed to create custom groups'));?>
 		</label>
 	</p>
+	<p>
+		<input type="checkbox" name="allow_duplicate_names" id="allowDuplicateNames" class="checkbox"
+			   value="1" <?php if ($_['allowDuplicateNames']) print_unescaped('checked="checked"'); ?> />
+		<label for="allowDuplicateNames">
+			<?php p($l->t('Allow creating multiple groups with the same name'));?>
+		</label>
+	</p>
 </div>

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() {

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');
 

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]],