diff --git a/apps/settings/composer/autoload.php b/apps/settings/composer/autoload.php index b5f54a914b715..60e8c97e4d5dd 100644 --- a/apps/settings/composer/autoload.php +++ b/apps/settings/composer/autoload.php @@ -14,10 +14,7 @@ echo $err; } } - trigger_error( - $err, - E_USER_ERROR - ); + throw new RuntimeException($err); } require_once __DIR__ . '/composer/autoload_real.php'; diff --git a/apps/settings/composer/composer/InstalledVersions.php b/apps/settings/composer/composer/InstalledVersions.php index 51e734a774b3e..2052022fd8e1e 100644 --- a/apps/settings/composer/composer/InstalledVersions.php +++ b/apps/settings/composer/composer/InstalledVersions.php @@ -26,12 +26,23 @@ */ class InstalledVersions { + /** + * @var string|null if set (by reflection by Composer), this should be set to the path where this class is being copied to + * @internal + */ + private static $selfDir = null; + /** * @var mixed[]|null * @psalm-var array{root: array{name: string, pretty_version: string, version: string, reference: string|null, type: string, install_path: string, aliases: string[], dev: bool}, versions: array}|array{}|null */ private static $installed; + /** + * @var bool + */ + private static $installedIsLocalDir; + /** * @var bool|null */ @@ -309,6 +320,24 @@ public static function reload($data) { self::$installed = $data; self::$installedByVendor = array(); + + // when using reload, we disable the duplicate protection to ensure that self::$installed data is + // always returned, but we cannot know whether it comes from the installed.php in __DIR__ or not, + // so we have to assume it does not, and that may result in duplicate data being returned when listing + // all installed packages for example + self::$installedIsLocalDir = false; + } + + /** + * @return string + */ + private static function getSelfDir() + { + if (self::$selfDir === null) { + self::$selfDir = strtr(__DIR__, '\\', '/'); + } + + return self::$selfDir; } /** @@ -322,19 +351,27 @@ private static function getInstalled() } $installed = array(); + $copiedLocalDir = false; if (self::$canGetVendors) { + $selfDir = self::getSelfDir(); foreach (ClassLoader::getRegisteredLoaders() as $vendorDir => $loader) { + $vendorDir = strtr($vendorDir, '\\', '/'); if (isset(self::$installedByVendor[$vendorDir])) { $installed[] = self::$installedByVendor[$vendorDir]; } elseif (is_file($vendorDir.'/composer/installed.php')) { /** @var array{root: array{name: string, pretty_version: string, version: string, reference: string|null, type: string, install_path: string, aliases: string[], dev: bool}, versions: array} $required */ $required = require $vendorDir.'/composer/installed.php'; - $installed[] = self::$installedByVendor[$vendorDir] = $required; - if (null === self::$installed && strtr($vendorDir.'/composer', '\\', '/') === strtr(__DIR__, '\\', '/')) { - self::$installed = $installed[count($installed) - 1]; + self::$installedByVendor[$vendorDir] = $required; + $installed[] = $required; + if (self::$installed === null && $vendorDir.'/composer' === $selfDir) { + self::$installed = $required; + self::$installedIsLocalDir = true; } } + if (self::$installedIsLocalDir && $vendorDir.'/composer' === $selfDir) { + $copiedLocalDir = true; + } } } @@ -350,7 +387,7 @@ private static function getInstalled() } } - if (self::$installed !== array()) { + if (self::$installed !== array() && !$copiedLocalDir) { $installed[] = self::$installed; } diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php index 0005f6a96c35c..ba9b5354e78dc 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/apps/settings/composer/composer/autoload_classmap.php @@ -66,6 +66,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 66682ec3b2e25..0e13fd42f216d 100644 --- a/apps/settings/composer/composer/autoload_static.php +++ b/apps/settings/composer/composer/autoload_static.php @@ -7,14 +7,14 @@ class ComposerStaticInitSettings { public static $prefixLengthsPsr4 = array ( - 'O' => + 'O' => array ( 'OCA\\Settings\\' => 13, ), ); public static $prefixDirsPsr4 = array ( - 'OCA\\Settings\\' => + 'OCA\\Settings\\' => array ( 0 => __DIR__ . '/..' . '/../lib', ), @@ -81,6 +81,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 f68427a1cabd2..6064176724d44 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 @@ public 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 15aca94198a1e..3189c504de24b 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 @@ +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'; 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 index 7e0c912bba818..f8a3c5c0e47ee 100644 --- a/apps/settings/tests/Service/AuthorizedGroupServiceTest.php +++ b/apps/settings/tests/Service/AuthorizedGroupServiceTest.php @@ -12,6 +12,8 @@ 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 @@ protected function setUp(): void { $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 @@ public function testCreateAllowsSameGroupDifferentClasses(): void { $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);