Merge pull request #20035 from nextcloud/bugfix/group-encode

Make sure group management works with all types of group names

apps/provisioning_api/lib/Controller/GroupsController.php

@@ -151,6 +151,8 @@ class GroupsController extends AUserData {
 	 * @throws OCSException
 	 */
 	public function getGroupUsers(string $groupId): DataResponse {
+		$groupId = urldecode($groupId);
+
 		$user = $this->userSession->getUser();
 		$isSubadminOfGroup = false;
 
@@ -190,6 +192,7 @@ class GroupsController extends AUserData {
 	 * @throws OCSException
 	 */
 	public function getGroupUsersDetails(string $groupId, string $search = '', int $limit = null, int $offset = 0): DataResponse {
+		$groupId = urldecode($groupId);
 		$currentUser = $this->userSession->getUser();
 
 		// Check the group exists
@@ -262,6 +265,8 @@ class GroupsController extends AUserData {
 	 * @throws OCSException
 	 */
 	public function updateGroup(string $groupId, string $key, string $value): DataResponse {
+		$groupId = urldecode($groupId);
+
 		if ($key === 'displayname') {
 			$group = $this->groupManager->get($groupId);
 			if ($group->setDisplayName($value)) {
@@ -282,6 +287,8 @@ class GroupsController extends AUserData {
 	 * @throws OCSException
 	 */
 	public function deleteGroup(string $groupId): DataResponse {
+		$groupId = urldecode($groupId);
+
 		// Check it exists
 		if (!$this->groupManager->groupExists($groupId)) {
 			throw new OCSException('', 101);

apps/provisioning_api/tests/Controller/GroupsControllerTest.php

@@ -75,13 +75,13 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->userSession = $this->createMock(IUserSession::class);
 		$this->accountManager = $this->createMock(AccountManager::class);
 		$this->logger = $this->createMock(ILogger::class);
-		
+
 		$this->subAdminManager = $this->createMock(SubAdmin::class);
 
 		$this->groupManager
 				->method('getSubAdmin')
 				->willReturn($this->subAdminManager);
-		
+
 		$this->api = $this->getMockBuilder(GroupsController::class)
 			->setConstructorArgs([
 				'provisioning_api',
@@ -256,7 +256,6 @@ class GroupsControllerTest extends \Test\TestCase {
 				'disabled' => 11,
 				'canAdd' => true,
 				'canRemove' => true
-				
 			]
 		]], $result->getData());
 	}
@@ -285,7 +284,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->assertEquals(['users' => ['user1', 'user2']], $result->getData());
 	}
 
-	
+
 	public function testGetGroupAsIrrelevantSubadmin() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionCode(403);
@@ -330,7 +329,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->assertEquals(['users' => ['user1', 'user2']], $result->getData());
 	}
 
-	
+
 	public function testGetGroupNonExisting() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionMessage('The requested group could not be found');
@@ -341,7 +340,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->api->getGroup($this->getUniqueID());
 	}
 
-	
+
 	public function testGetSubAdminsOfGroupsNotExists() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionMessage('Group does not exist');
@@ -388,7 +387,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->assertEquals([], $result->getData());
 	}
 
-	
+
 	public function testAddGroupEmptyGroup() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionMessage('Invalid group name');
@@ -397,7 +396,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->api->addGroup('');
 	}
 
-	
+
 	public function testAddGroupExistingGroup() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionCode(102);
@@ -438,7 +437,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->api->addGroup('Iñtërnâtiônàlizætiøn');
 	}
 
-	
+
 	public function testDeleteGroupNonExisting() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionCode(101);
@@ -446,7 +445,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->api->deleteGroup('NonExistingGroup');
 	}
 
-	
+
 	public function testDeleteAdminGroup() {
 		$this->expectException(\OCP\AppFramework\OCS\OCSException::class);
 		$this->expectExceptionCode(102);
@@ -478,6 +477,25 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->api->deleteGroup('ExistingGroup');
 	}
 
+	public function testDeleteGroupEncoding() {
+		$this->groupManager
+			->method('groupExists')
+			->with('ExistingGroup A/B')
+			->willReturn('true');
+
+		$group = $this->createGroup('ExistingGroup');
+		$this->groupManager
+			->method('get')
+			->with('ExistingGroup A/B')
+			->willReturn($group);
+		$group
+			->expects($this->once())
+			->method('delete')
+			->willReturn(true);
+
+		$this->api->deleteGroup(urlencode('ExistingGroup A/B'));
+	}
+
 	public function testGetGroupUsersDetails() {
 		$gid = 'ncg1';
 
@@ -523,4 +541,50 @@ class GroupsControllerTest extends \Test\TestCase {
 
 		$this->api->getGroupUsersDetails($gid);
 	}
+
+	public function testGetGroupUsersDetailsEncoded() {
+		$gid = 'Department A/B C/D';
+
+		$this->asAdmin();
+		$this->useAccountManager();
+
+		$users = [
+			'ncu1' => $this->createUser('ncu1'), # regular
+			'ncu2' => $this->createUser('ncu2'), # the zombie
+		];
+		$users['ncu2']->expects($this->atLeastOnce())
+			->method('getHome')
+			->willThrowException(new NoUserException());
+
+		$this->userManager->expects($this->any())
+			->method('get')
+			->willReturnCallback(function(string $uid) use ($users) {
+				return isset($users[$uid]) ? $users[$uid] : null;
+			});
+
+		$group = $this->createGroup($gid);
+		$group->expects($this->once())
+			->method('searchUsers')
+			->with('', null, 0)
+			->willReturn(array_values($users));
+
+		$this->groupManager
+			->method('get')
+			->with($gid)
+			->willReturn($group);
+		$this->groupManager->expects($this->any())
+			->method('getUserGroups')
+			->willReturn([$group]);
+
+		/** @var \PHPUnit_Framework_MockObject_MockObject */
+		$this->subAdminManager->expects($this->any())
+			->method('isSubAdminOfGroup')
+			->willReturn(false);
+		$this->subAdminManager->expects($this->any())
+			->method('getSubAdminsGroups')
+			->willReturn([]);
+
+
+		$this->api->getGroupUsersDetails(urlencode($gid));
+	}
 }

apps/settings/src/components/UserList.vue

@@ -312,6 +312,9 @@ export default {
 		settings() {
 			return this.$store.getters.getServerData
 		},
+		selectedGroupDecoded() {
+			return decodeURIComponent(this.selectedGroup)
+		},
 		filteredUsers() {
 			if (this.selectedGroup === 'disabled') {
 				return this.users.filter(user => user.enabled === false)

apps/settings/src/store/users.js

@@ -205,7 +205,7 @@ const actions = {
 		search = typeof search === 'string' ? search : ''
 		group = typeof group === 'string' ? group : ''
 		if (group !== '') {
-			return api.get(OC.linkToOCS(`cloud/groups/${encodeURIComponent(group)}/users/details?offset=${offset}&limit=${limit}&search=${search}`, 2))
+			return api.get(OC.linkToOCS(`cloud/groups/${encodeURIComponent(encodeURIComponent(group))}/users/details?offset=${offset}&limit=${limit}&search=${search}`, 2))
 				.then((response) => {
 					if (Object.keys(response.data.ocs.data.users).length > 0) {
 						context.commit('appendUsers', response.data.ocs.data.users)
@@ -275,7 +275,7 @@ const actions = {
 	 * @returns {Promise}
 	 */
 	getUsersFromGroup(context, { groupid, offset, limit }) {
-		return api.get(OC.linkToOCS(`cloud/users/${encodeURIComponent(groupid)}/details?offset=${offset}&limit=${limit}`, 2))
+		return api.get(OC.linkToOCS(`cloud/users/${encodeURIComponent(encodeURIComponent(groupid))}/details?offset=${offset}&limit=${limit}`, 2))
 			.then((response) => context.commit('getUsersFromList', response.data.ocs.data.users))
 			.catch((error) => context.commit('API_FAILURE', error))
 	},
@@ -320,7 +320,7 @@ const actions = {
 	 */
 	removeGroup(context, gid) {
 		return api.requireAdmin().then((response) => {
-			return api.delete(OC.linkToOCS(`cloud/groups/${encodeURIComponent(gid)}`, 2))
+			return api.delete(OC.linkToOCS(`cloud/groups/${encodeURIComponent(encodeURIComponent(gid))}`, 2))
 				.then((response) => context.commit('removeGroup', gid))
 				.catch((error) => { throw error })
 		}).catch((error) => context.commit('API_FAILURE', { gid, error }))

apps/settings/src/views/Users.vue

@@ -79,7 +79,7 @@
 					:key="group.id"
 					:exact="true"
 					:title="group.title"
-					:to="{ name: 'group', params: { selectedGroup: group.id } }">
+					:to="{ name: 'group', params: { selectedGroup: encodeURIComponent(group.id) } }">
 					<AppNavigationCounter v-if="group.count" slot="counter">
 						{{ group.count }}
 					</AppNavigationCounter>
@@ -149,7 +149,7 @@
 			<UserList #content
 				:users="users"
 				:show-config="showConfig"
-				:selected-group="selectedGroup"
+				:selected-group="selectedGroupDecoded"
 				:external-actions="externalActions" />
 		</AppContent>
 	</Content>
@@ -215,6 +215,9 @@ export default {
 		}
 	},
 	computed: {
+		selectedGroupDecoded() {
+			return this.selectedGroup ? decodeURIComponent(this.selectedGroup) : null
+		},
 		users() {
 			return this.$store.getters.getUsers
 		},
@@ -452,7 +455,7 @@ export default {
 				this.$router.push({
 					name: 'group',
 					params: {
-						selectedGroup: gid.trim(),
+						selectedGroup: encodeURIComponent(gid.trim()),
 					},
 				})
 			} catch {