fix(admin-delegation): Prevent delegation to group if delegation already exist

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>

apps/settings/composer/composer/autoload_classmap.php

@@ -67,6 +67,7 @@ return array(
     'OCA\\Settings\\Sections\\Personal\\Security' => $baseDir . '/../lib/Sections/Personal/Security.php',
     'OCA\\Settings\\Sections\\Personal\\SyncClients' => $baseDir . '/../lib/Sections/Personal/SyncClients.php',
     'OCA\\Settings\\Service\\AuthorizedGroupService' => $baseDir . '/../lib/Service/AuthorizedGroupService.php',
+    'OCA\\Settings\\Service\\ConflictException' => $baseDir . '/../lib/Service/ConflictException.php',
     'OCA\\Settings\\Service\\NotFoundException' => $baseDir . '/../lib/Service/NotFoundException.php',
     'OCA\\Settings\\Service\\ServiceException' => $baseDir . '/../lib/Service/ServiceException.php',
     'OCA\\Settings\\Settings\\Admin\\ArtificialIntelligence' => $baseDir . '/../lib/Settings/Admin/ArtificialIntelligence.php',

apps/settings/composer/composer/autoload_static.php

@@ -82,6 +82,7 @@ class ComposerStaticInitSettings
         'OCA\\Settings\\Sections\\Personal\\Security' => __DIR__ . '/..' . '/../lib/Sections/Personal/Security.php',
         'OCA\\Settings\\Sections\\Personal\\SyncClients' => __DIR__ . '/..' . '/../lib/Sections/Personal/SyncClients.php',
         'OCA\\Settings\\Service\\AuthorizedGroupService' => __DIR__ . '/..' . '/../lib/Service/AuthorizedGroupService.php',
+        'OCA\\Settings\\Service\\ConflictException' => __DIR__ . '/..' . '/../lib/Service/ConflictException.php',
         'OCA\\Settings\\Service\\NotFoundException' => __DIR__ . '/..' . '/../lib/Service/NotFoundException.php',
         'OCA\\Settings\\Service\\ServiceException' => __DIR__ . '/..' . '/../lib/Service/ServiceException.php',
         'OCA\\Settings\\Settings\\Admin\\ArtificialIntelligence' => __DIR__ . '/..' . '/../lib/Settings/Admin/ArtificialIntelligence.php',

apps/settings/lib/Command/AdminDelegation/Add.php

@@ -9,6 +9,7 @@ namespace OCA\Settings\Command\AdminDelegation;
 
 use OC\Core\Command\Base;
 use OCA\Settings\Service\AuthorizedGroupService;
+use OCA\Settings\Service\ConflictException;
 use OCP\IGroupManager;
 use OCP\Settings\IDelegatedSettings;
 use OCP\Settings\IManager;
@@ -50,7 +51,12 @@ class Add extends Base {
 			return 3;
 		}
 
-		$this->authorizedGroupService->create($groupId, $settingClass);
+		try {
+			$this->authorizedGroupService->create($groupId, $settingClass);
+		} catch (ConflictException) {
+			$io->warning('Administration of ' . $settingClass . ' is already delegated to ' . $groupId . '.');
+			return 4;
+		}
 
 		$io->success('Administration of ' . $settingClass . ' delegated to ' . $groupId . '.');
 

apps/settings/lib/Service/AuthorizedGroupService.php

@@ -57,8 +57,19 @@ class AuthorizedGroupService {
 	 * @param string $class
 	 * @return AuthorizedGroup
 	 * @throws Exception
+	 * @throws ConflictException
 	 */
 	public function create(string $groupId, string $class): AuthorizedGroup {
+		// Check if the group is already assigned to this class
+		try {
+			$existing = $this->mapper->findByGroupIdAndClass($groupId, $class);
+			if ($existing) {
+				throw new ConflictException('Group is already assigned to this class');
+			}
+		} catch (DoesNotExistException $e) {
+			// This is expected when no duplicate exists, continue with creation
+		}
+
 		$authorizedGroup = new AuthorizedGroup();
 		$authorizedGroup->setGroupId($groupId);
 		$authorizedGroup->setClass($class);

apps/settings/lib/Service/ConflictException.php

@@ -0,0 +1,10 @@
+<?php
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\Settings\Service;
+
+class ConflictException extends ServiceException {
+}

apps/settings/tests/Command/AdminDelegation/AddTest.php

@@ -11,6 +11,7 @@ namespace OCA\Settings\Tests\Command\AdminDelegation;
 use OC\Settings\AuthorizedGroup;
 use OCA\Settings\Command\AdminDelegation\Add;
 use OCA\Settings\Service\AuthorizedGroupService;
+use OCA\Settings\Service\ConflictException;
 use OCA\Settings\Settings\Admin\Server;
 use OCP\IGroupManager;
 use OCP\Settings\IManager;
@@ -78,6 +79,35 @@ class AddTest extends TestCase {
 		$this->assertEquals(0, $result);
 	}
 
+	public function testExecuteHandlesDuplicateAssignment(): void {
+		$settingClass = 'OCA\\Settings\\Settings\\Admin\\Server';
+		$groupId = 'testgroup';
+
+		// Mock valid delegated settings class
+		$this->input->expects($this->exactly(2))
+			->method('getArgument')
+			->willReturnMap([
+				['settingClass', $settingClass],
+				['groupId', $groupId]
+			]);
+
+		// Mock group exists
+		$this->groupManager->expects($this->once())
+			->method('groupExists')
+			->with($groupId)
+			->willReturn(true);
+
+		// Mock ConflictException when trying to create duplicate
+		$this->authorizedGroupService->expects($this->once())
+			->method('create')
+			->with($groupId, $settingClass)
+			->willThrowException(new ConflictException('Group is already assigned to this class'));
+
+		$result = $this->command->execute($this->input, $this->output);
+
+		$this->assertEquals(4, $result, 'Duplicate assignment should return exit code 4');
+	}
+
 	public function testExecuteInvalidSettingClass(): void {
 		// Use a real class that exists but doesn't implement IDelegatedSettings
 		$settingClass = 'stdClass';

apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php

@@ -0,0 +1,157 @@
+<?php
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\Settings\Tests\Integration;
+
+use OC\Settings\AuthorizedGroup;
+use OC\Settings\AuthorizedGroupMapper;
+use OCA\Settings\Service\AuthorizedGroupService;
+use OCA\Settings\Service\ConflictException;
+use OCP\AppFramework\Db\DoesNotExistException;
+use Test\TestCase;
+
+/**
+ * Integration test for duplicate prevention in AuthorizedGroupService
+ * This test verifies the complete flow of duplicate detection and prevention
+ */
+#[\PHPUnit\Framework\Attributes\Group('DB')]
+class DuplicateAssignmentIntegrationTest extends TestCase {
+
+	private AuthorizedGroupService $service;
+	private AuthorizedGroupMapper $mapper;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		// Use real mapper for integration testing
+		$this->mapper = \OCP\Server::get(AuthorizedGroupMapper::class);
+		$this->service = new AuthorizedGroupService($this->mapper);
+	}
+
+	protected function tearDown(): void {
+		// Clean up any test data
+		try {
+			$allGroups = $this->mapper->findAll();
+			foreach ($allGroups as $group) {
+				if (str_starts_with($group->getGroupId(), 'test_')
+					|| str_starts_with($group->getClass(), 'TestClass')) {
+					$this->mapper->delete($group);
+				}
+			}
+		} catch (\Exception $e) {
+			// Ignore cleanup errors
+		}
+		parent::tearDown();
+	}
+
+	public function testDuplicateAssignmentPrevention(): void {
+		$groupId = 'test_duplicate_group';
+		$class = 'TestClass\\DuplicateTest';
+
+		// First assignment should succeed
+		$result1 = $this->service->create($groupId, $class);
+		$this->assertInstanceOf(AuthorizedGroup::class, $result1);
+		$this->assertEquals($groupId, $result1->getGroupId());
+		$this->assertEquals($class, $result1->getClass());
+		$this->assertNotNull($result1->getId());
+
+		// Second assignment of same group to same class should throw ConflictException
+		$this->expectException(ConflictException::class);
+		$this->expectExceptionMessage('Group is already assigned to this class');
+
+		$this->service->create($groupId, $class);
+	}
+
+	public function testDifferentGroupsSameClassAllowed(): void {
+		$groupId1 = 'test_group_1';
+		$groupId2 = 'test_group_2';
+		$class = 'TestClass\\MultiGroup';
+
+		// Both assignments should succeed
+		$result1 = $this->service->create($groupId1, $class);
+		$result2 = $this->service->create($groupId2, $class);
+
+		$this->assertEquals($groupId1, $result1->getGroupId());
+		$this->assertEquals($groupId2, $result2->getGroupId());
+		$this->assertEquals($class, $result1->getClass());
+		$this->assertEquals($class, $result2->getClass());
+		$this->assertNotEquals($result1->getId(), $result2->getId());
+	}
+
+	public function testSameGroupDifferentClassesAllowed(): void {
+		$groupId = 'test_multi_class_group';
+		$class1 = 'TestClass\\First';
+		$class2 = 'TestClass\\Second';
+
+		// Both assignments should succeed
+		$result1 = $this->service->create($groupId, $class1);
+		$result2 = $this->service->create($groupId, $class2);
+
+		$this->assertEquals($groupId, $result1->getGroupId());
+		$this->assertEquals($groupId, $result2->getGroupId());
+		$this->assertEquals($class1, $result1->getClass());
+		$this->assertEquals($class2, $result2->getClass());
+		$this->assertNotEquals($result1->getId(), $result2->getId());
+	}
+
+	public function testCreateAfterDelete(): void {
+		$groupId = 'test_recreate_group';
+		$class = 'TestClass\\Recreate';
+
+		// Create initial assignment
+		$result1 = $this->service->create($groupId, $class);
+		$initialId = $result1->getId();
+
+		// Delete the assignment
+		$this->service->delete($initialId);
+
+		// Verify it's deleted by trying to find it
+		$this->expectException(\OCP\AppFramework\Db\DoesNotExistException::class);
+		try {
+			$this->service->find($initialId);
+		} catch (\OCA\Settings\Service\NotFoundException $e) {
+			// Expected - now create the same assignment again, which should succeed
+			$result2 = $this->service->create($groupId, $class);
+
+			$this->assertEquals($groupId, $result2->getGroupId());
+			$this->assertEquals($class, $result2->getClass());
+			$this->assertNotEquals($initialId, $result2->getId());
+			return;
+		}
+
+		$this->fail('Expected NotFoundException when finding deleted group');
+	}
+
+	/**
+	 * Test the mapper's findByGroupIdAndClass method behavior with duplicates
+	 */
+	public function testMapperFindByGroupIdAndClassBehavior(): void {
+		$groupId = 'test_mapper_group';
+		$class = 'TestClass\\MapperTest';
+
+		// Initially should throw DoesNotExistException
+		$this->expectException(DoesNotExistException::class);
+		$this->mapper->findByGroupIdAndClass($groupId, $class);
+	}
+
+	/**
+	 * Test that mapper returns existing record after creation
+	 */
+	public function testMapperFindsExistingRecord(): void {
+		$groupId = 'test_existing_group';
+		$class = 'TestClass\\Existing';
+
+		// Create the record first
+		$created = $this->service->create($groupId, $class);
+
+		// Now mapper should find it
+		$found = $this->mapper->findByGroupIdAndClass($groupId, $class);
+
+		$this->assertEquals($created->getId(), $found->getId());
+		$this->assertEquals($groupId, $found->getGroupId());
+		$this->assertEquals($class, $found->getClass());
+	}
+}

apps/settings/tests/Service/AuthorizedGroupServiceTest.php

@@ -12,6 +12,8 @@ namespace OCA\Settings\Tests\Service;
 use OC\Settings\AuthorizedGroup;
 use OC\Settings\AuthorizedGroupMapper;
 use OCA\Settings\Service\AuthorizedGroupService;
+use OCA\Settings\Service\ConflictException;
+use OCP\AppFramework\Db\DoesNotExistException;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
@@ -26,11 +28,72 @@ class AuthorizedGroupServiceTest extends TestCase {
 		$this->service = new AuthorizedGroupService($this->mapper);
 	}
 
+	public function testCreateSuccessWhenNoDuplicateExists(): void {
+		$groupId = 'testgroup';
+		$class = 'TestClass';
+
+		// Mock that no existing assignment is found (throws DoesNotExistException)
+		$this->mapper->expects($this->once())
+			->method('findByGroupIdAndClass')
+			->with($groupId, $class)
+			->willThrowException(new DoesNotExistException('Not found'));
+
+		// Mock the successful creation
+		$expectedGroup = new AuthorizedGroup();
+		$expectedGroup->setGroupId($groupId);
+		$expectedGroup->setClass($class);
+		$expectedGroup->setId(123);
+
+		$this->mapper->expects($this->once())
+			->method('insert')
+			->willReturn($expectedGroup);
+
+		$result = $this->service->create($groupId, $class);
+
+		$this->assertInstanceOf(AuthorizedGroup::class, $result);
+		$this->assertEquals($groupId, $result->getGroupId());
+		$this->assertEquals($class, $result->getClass());
+	}
+
+	public function testCreateThrowsConflictExceptionWhenDuplicateExists(): void {
+		$groupId = 'testgroup';
+		$class = 'TestClass';
+
+		// Mock that an existing assignment is found
+		$existingGroup = new AuthorizedGroup();
+		$existingGroup->setGroupId($groupId);
+		$existingGroup->setClass($class);
+		$existingGroup->setId(42);
+
+		$this->mapper->expects($this->once())
+			->method('findByGroupIdAndClass')
+			->with($groupId, $class)
+			->willReturn($existingGroup);
+
+		// Mapper insert should never be called when duplicate exists
+		$this->mapper->expects($this->never())
+			->method('insert');
+
+		$this->expectException(ConflictException::class);
+		$this->expectExceptionMessage('Group is already assigned to this class');
+
+		$this->service->create($groupId, $class);
+	}
+
 	public function testCreateAllowsDifferentGroupsSameClass(): void {
 		$groupId1 = 'testgroup1';
 		$groupId2 = 'testgroup2';
 		$class = 'TestClass';
 
+		// Mock that no duplicate exists for group1
+		$this->mapper->expects($this->exactly(2))
+			->method('findByGroupIdAndClass')
+			->willReturnCallback(function ($groupId, $classArg) use ($groupId1, $groupId2, $class) {
+				$this->assertContains($groupId, [$groupId1, $groupId2]);
+				$this->assertEquals($class, $classArg);
+				throw new DoesNotExistException('Not found');
+			});
+
 		$expectedGroup1 = new AuthorizedGroup();
 		$expectedGroup1->setGroupId($groupId1);
 		$expectedGroup1->setClass($class);
@@ -60,6 +123,15 @@ class AuthorizedGroupServiceTest extends TestCase {
 		$class1 = 'TestClass1';
 		$class2 = 'TestClass2';
 
+		// Mock that no duplicate exists for either class
+		$this->mapper->expects($this->exactly(2))
+			->method('findByGroupIdAndClass')
+			->willReturnCallback(function ($groupIdArg, $class) use ($groupId, $class1, $class2) {
+				$this->assertEquals($groupId, $groupIdArg);
+				$this->assertContains($class, [$class1, $class2]);
+				throw new DoesNotExistException('Not found');
+			});
+
 		$expectedGroup1 = new AuthorizedGroup();
 		$expectedGroup1->setGroupId($groupId);
 		$expectedGroup1->setClass($class1);