diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php index 8732881f9966c..2d8fcc2965dc4 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/apps/settings/composer/composer/autoload_classmap.php @@ -63,6 +63,7 @@ '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', diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php index c7358bc249df0..336d1d60cd1ec 100644 --- a/apps/settings/composer/composer/autoload_static.php +++ b/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', diff --git a/apps/settings/lib/Command/AdminDelegation/Add.php b/apps/settings/lib/Command/AdminDelegation/Add.php index 5cbef5c5d157b..0badfa52bca24 100644 --- a/apps/settings/lib/Command/AdminDelegation/Add.php +++ b/apps/settings/lib/Command/AdminDelegation/Add.php @@ -9,6 +9,7 @@ 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 @@ protected function execute(InputInterface $input, OutputInterface $output): int 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 . '.'); diff --git a/apps/settings/lib/Service/AuthorizedGroupService.php b/apps/settings/lib/Service/AuthorizedGroupService.php index 20966446d6187..c95b4260f796d 100644 --- a/apps/settings/lib/Service/AuthorizedGroupService.php +++ b/apps/settings/lib/Service/AuthorizedGroupService.php @@ -57,8 +57,19 @@ private function handleException(\Exception $e): void { * @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); diff --git a/apps/settings/lib/Service/ConflictException.php b/apps/settings/lib/Service/ConflictException.php new file mode 100644 index 0000000000000..eab17d7cf0b12 --- /dev/null +++ b/apps/settings/lib/Service/ConflictException.php @@ -0,0 +1,10 @@ +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 = $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 = $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 = $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 = $this->command->execute($this->input, $this->output); + + // Should return exit code 3 for non-existent group + $this->assertEquals(3, $result); + } +} diff --git a/apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php b/apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php new file mode 100644 index 0000000000000..a66f2d398c9a5 --- /dev/null +++ b/apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php @@ -0,0 +1,157 @@ +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()); + } +} diff --git a/apps/settings/tests/Service/AuthorizedGroupServiceTest.php b/apps/settings/tests/Service/AuthorizedGroupServiceTest.php new file mode 100644 index 0000000000000..7e0c912bba818 --- /dev/null +++ b/apps/settings/tests/Service/AuthorizedGroupServiceTest.php @@ -0,0 +1,86 @@ +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()); + } +}