Merge pull request #57119 from nextcloud/backport/56646/stable31

[stable31] fix(admin-delegation): Prevent delegation to group if delegation alre…

apps/settings/composer/composer/autoload_classmap.php

@@ -63,6 +63,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

@@ -78,6 +78,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

@@ -0,0 +1,148 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+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;
+use PHPUnit\Framework\MockObject\MockObject;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Test\TestCase;
+
+class AddTest extends TestCase {
+
+	private IManager&MockObject $settingManager;
+	private AuthorizedGroupService&MockObject $authorizedGroupService;
+	private IGroupManager&MockObject $groupManager;
+	private Add $command;
+	private InputInterface&MockObject $input;
+	private OutputInterface&MockObject $output;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->settingManager = $this->createMock(IManager::class);
+		$this->authorizedGroupService = $this->createMock(AuthorizedGroupService::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
+
+		$this->command = new Add(
+			$this->settingManager,
+			$this->authorizedGroupService,
+			$this->groupManager
+		);
+
+		$this->input = $this->createMock(InputInterface::class);
+		$this->output = $this->createMock(OutputInterface::class);
+	}
+
+	public function testExecuteSuccessfulDelegation(): void {
+		$settingClass = Server::class;
+		$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 successful creation
+		$authorizedGroup = new AuthorizedGroup();
+		$authorizedGroup->setGroupId($groupId);
+		$authorizedGroup->setClass($settingClass);
+
+		$this->authorizedGroupService->expects($this->once())
+			->method('create')
+			->with($groupId, $settingClass)
+			->willReturn($authorizedGroup);
+
+		$result = self::invokePrivate($this->command, 'execute', [$this->input, $this->output]);
+
+		$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 = self::invokePrivate($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';
+
+		$this->input->expects($this->once())
+			->method('getArgument')
+			->with('settingClass')
+			->willReturn($settingClass);
+
+		$result = self::invokePrivate($this->command, 'execute', [$this->input, $this->output]);
+
+		// Should return exit code 2 for invalid setting class
+		$this->assertEquals(2, $result);
+	}
+
+	public function testExecuteNonExistentGroup(): void {
+		$settingClass = Server::class;
+		$groupId = 'nonexistentgroup';
+
+		$this->input->expects($this->exactly(2))
+			->method('getArgument')
+			->willReturnMap([
+				['settingClass', $settingClass],
+				['groupId', $groupId]
+			]);
+
+		// Mock group does not exist
+		$this->groupManager->expects($this->once())
+			->method('groupExists')
+			->with($groupId)
+			->willReturn(false);
+
+		$result = self::invokePrivate($this->command, 'execute', [$this->input, $this->output]);
+
+		// Should return exit code 3 for non-existent group
+		$this->assertEquals(3, $result);
+	}
+}

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

@@ -0,0 +1,86 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Settings\Tests\Service;
+
+use OC\Settings\AuthorizedGroup;
+use OC\Settings\AuthorizedGroupMapper;
+use OCA\Settings\Service\AuthorizedGroupService;
+use PHPUnit\Framework\MockObject\MockObject;
+use Test\TestCase;
+
+class AuthorizedGroupServiceTest extends TestCase {
+
+	private AuthorizedGroupMapper&MockObject $mapper;
+	private AuthorizedGroupService $service;
+
+	protected function setUp(): void {
+		parent::setUp();
+		$this->mapper = $this->createMock(AuthorizedGroupMapper::class);
+		$this->service = new AuthorizedGroupService($this->mapper);
+	}
+
+	public function testCreateAllowsDifferentGroupsSameClass(): void {
+		$groupId1 = 'testgroup1';
+		$groupId2 = 'testgroup2';
+		$class = 'TestClass';
+
+		$expectedGroup1 = new AuthorizedGroup();
+		$expectedGroup1->setGroupId($groupId1);
+		$expectedGroup1->setClass($class);
+		$expectedGroup1->setId(123);
+
+		$expectedGroup2 = new AuthorizedGroup();
+		$expectedGroup2->setGroupId($groupId2);
+		$expectedGroup2->setClass($class);
+		$expectedGroup2->setId(124);
+
+		$this->mapper->expects($this->exactly(2))
+			->method('insert')
+			->willReturnOnConsecutiveCalls($expectedGroup1, $expectedGroup2);
+
+		// Both creations should succeed
+		$result1 = $this->service->create($groupId1, $class);
+		$this->assertEquals($groupId1, $result1->getGroupId());
+		$this->assertEquals($class, $result1->getClass());
+
+		$result2 = $this->service->create($groupId2, $class);
+		$this->assertEquals($groupId2, $result2->getGroupId());
+		$this->assertEquals($class, $result2->getClass());
+	}
+
+	public function testCreateAllowsSameGroupDifferentClasses(): void {
+		$groupId = 'testgroup';
+		$class1 = 'TestClass1';
+		$class2 = 'TestClass2';
+
+		$expectedGroup1 = new AuthorizedGroup();
+		$expectedGroup1->setGroupId($groupId);
+		$expectedGroup1->setClass($class1);
+		$expectedGroup1->setId(123);
+
+		$expectedGroup2 = new AuthorizedGroup();
+		$expectedGroup2->setGroupId($groupId);
+		$expectedGroup2->setClass($class2);
+		$expectedGroup2->setId(124);
+
+		$this->mapper->expects($this->exactly(2))
+			->method('insert')
+			->willReturnOnConsecutiveCalls($expectedGroup1, $expectedGroup2);
+
+		// Both creations 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());
+	}
+}