Skip to content

Commit f9e52ec

Browse files
Merge pull request #57013 from nextcloud/backport/56646/stable32
[stable32] fix(admin-delegation): Prevent delegation to group if delegation already
2 parents 884a5f5 + 2b270c8 commit f9e52ec

10 files changed

Lines changed: 333 additions & 11 deletions

File tree

apps/settings/composer/autoload.php

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,7 @@
1414
echo $err;
1515
}
1616
}
17-
trigger_error(
18-
$err,
19-
E_USER_ERROR
20-
);
17+
throw new RuntimeException($err);
2118
}
2219

2320
require_once __DIR__ . '/composer/autoload_real.php';

apps/settings/composer/composer/InstalledVersions.php

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,23 @@
2626
*/
2727
class InstalledVersions
2828
{
29+
/**
30+
* @var string|null if set (by reflection by Composer), this should be set to the path where this class is being copied to
31+
* @internal
32+
*/
33+
private static $selfDir = null;
34+
2935
/**
3036
* @var mixed[]|null
3137
* @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<string, array{pretty_version?: string, version?: string, reference?: string|null, type?: string, install_path?: string, aliases?: string[], dev_requirement: bool, replaced?: string[], provided?: string[]}>}|array{}|null
3238
*/
3339
private static $installed;
3440

41+
/**
42+
* @var bool
43+
*/
44+
private static $installedIsLocalDir;
45+
3546
/**
3647
* @var bool|null
3748
*/
@@ -309,6 +320,24 @@ public static function reload($data)
309320
{
310321
self::$installed = $data;
311322
self::$installedByVendor = array();
323+
324+
// when using reload, we disable the duplicate protection to ensure that self::$installed data is
325+
// always returned, but we cannot know whether it comes from the installed.php in __DIR__ or not,
326+
// so we have to assume it does not, and that may result in duplicate data being returned when listing
327+
// all installed packages for example
328+
self::$installedIsLocalDir = false;
329+
}
330+
331+
/**
332+
* @return string
333+
*/
334+
private static function getSelfDir()
335+
{
336+
if (self::$selfDir === null) {
337+
self::$selfDir = strtr(__DIR__, '\\', '/');
338+
}
339+
340+
return self::$selfDir;
312341
}
313342

314343
/**
@@ -322,19 +351,27 @@ private static function getInstalled()
322351
}
323352

324353
$installed = array();
354+
$copiedLocalDir = false;
325355

326356
if (self::$canGetVendors) {
357+
$selfDir = self::getSelfDir();
327358
foreach (ClassLoader::getRegisteredLoaders() as $vendorDir => $loader) {
359+
$vendorDir = strtr($vendorDir, '\\', '/');
328360
if (isset(self::$installedByVendor[$vendorDir])) {
329361
$installed[] = self::$installedByVendor[$vendorDir];
330362
} elseif (is_file($vendorDir.'/composer/installed.php')) {
331363
/** @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<string, array{pretty_version?: string, version?: string, reference?: string|null, type?: string, install_path?: string, aliases?: string[], dev_requirement: bool, replaced?: string[], provided?: string[]}>} $required */
332364
$required = require $vendorDir.'/composer/installed.php';
333-
$installed[] = self::$installedByVendor[$vendorDir] = $required;
334-
if (null === self::$installed && strtr($vendorDir.'/composer', '\\', '/') === strtr(__DIR__, '\\', '/')) {
335-
self::$installed = $installed[count($installed) - 1];
365+
self::$installedByVendor[$vendorDir] = $required;
366+
$installed[] = $required;
367+
if (self::$installed === null && $vendorDir.'/composer' === $selfDir) {
368+
self::$installed = $required;
369+
self::$installedIsLocalDir = true;
336370
}
337371
}
372+
if (self::$installedIsLocalDir && $vendorDir.'/composer' === $selfDir) {
373+
$copiedLocalDir = true;
374+
}
338375
}
339376
}
340377

@@ -350,7 +387,7 @@ private static function getInstalled()
350387
}
351388
}
352389

353-
if (self::$installed !== array()) {
390+
if (self::$installed !== array() && !$copiedLocalDir) {
354391
$installed[] = self::$installed;
355392
}
356393

apps/settings/composer/composer/autoload_classmap.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
'OCA\\Settings\\Sections\\Personal\\Security' => $baseDir . '/../lib/Sections/Personal/Security.php',
6767
'OCA\\Settings\\Sections\\Personal\\SyncClients' => $baseDir . '/../lib/Sections/Personal/SyncClients.php',
6868
'OCA\\Settings\\Service\\AuthorizedGroupService' => $baseDir . '/../lib/Service/AuthorizedGroupService.php',
69+
'OCA\\Settings\\Service\\ConflictException' => $baseDir . '/../lib/Service/ConflictException.php',
6970
'OCA\\Settings\\Service\\NotFoundException' => $baseDir . '/../lib/Service/NotFoundException.php',
7071
'OCA\\Settings\\Service\\ServiceException' => $baseDir . '/../lib/Service/ServiceException.php',
7172
'OCA\\Settings\\Settings\\Admin\\ArtificialIntelligence' => $baseDir . '/../lib/Settings/Admin/ArtificialIntelligence.php',

apps/settings/composer/composer/autoload_static.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,14 @@
77
class ComposerStaticInitSettings
88
{
99
public static $prefixLengthsPsr4 = array (
10-
'O' =>
10+
'O' =>
1111
array (
1212
'OCA\\Settings\\' => 13,
1313
),
1414
);
1515

1616
public static $prefixDirsPsr4 = array (
17-
'OCA\\Settings\\' =>
17+
'OCA\\Settings\\' =>
1818
array (
1919
0 => __DIR__ . '/..' . '/../lib',
2020
),
@@ -81,6 +81,7 @@ class ComposerStaticInitSettings
8181
'OCA\\Settings\\Sections\\Personal\\Security' => __DIR__ . '/..' . '/../lib/Sections/Personal/Security.php',
8282
'OCA\\Settings\\Sections\\Personal\\SyncClients' => __DIR__ . '/..' . '/../lib/Sections/Personal/SyncClients.php',
8383
'OCA\\Settings\\Service\\AuthorizedGroupService' => __DIR__ . '/..' . '/../lib/Service/AuthorizedGroupService.php',
84+
'OCA\\Settings\\Service\\ConflictException' => __DIR__ . '/..' . '/../lib/Service/ConflictException.php',
8485
'OCA\\Settings\\Service\\NotFoundException' => __DIR__ . '/..' . '/../lib/Service/NotFoundException.php',
8586
'OCA\\Settings\\Service\\ServiceException' => __DIR__ . '/..' . '/../lib/Service/ServiceException.php',
8687
'OCA\\Settings\\Settings\\Admin\\ArtificialIntelligence' => __DIR__ . '/..' . '/../lib/Settings/Admin/ArtificialIntelligence.php',

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
use OC\Core\Command\Base;
1111
use OCA\Settings\Service\AuthorizedGroupService;
12+
use OCA\Settings\Service\ConflictException;
1213
use OCP\IGroupManager;
1314
use OCP\Settings\IDelegatedSettings;
1415
use OCP\Settings\IManager;
@@ -50,7 +51,12 @@ public function execute(InputInterface $input, OutputInterface $output): int {
5051
return 3;
5152
}
5253

53-
$this->authorizedGroupService->create($groupId, $settingClass);
54+
try {
55+
$this->authorizedGroupService->create($groupId, $settingClass);
56+
} catch (ConflictException) {
57+
$io->warning('Administration of ' . $settingClass . ' is already delegated to ' . $groupId . '.');
58+
return 4;
59+
}
5460

5561
$io->success('Administration of ' . $settingClass . ' delegated to ' . $groupId . '.');
5662

apps/settings/lib/Service/AuthorizedGroupService.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,19 @@ private function handleException(\Exception $e): void {
5757
* @param string $class
5858
* @return AuthorizedGroup
5959
* @throws Exception
60+
* @throws ConflictException
6061
*/
6162
public function create(string $groupId, string $class): AuthorizedGroup {
63+
// Check if the group is already assigned to this class
64+
try {
65+
$existing = $this->mapper->findByGroupIdAndClass($groupId, $class);
66+
if ($existing) {
67+
throw new ConflictException('Group is already assigned to this class');
68+
}
69+
} catch (DoesNotExistException $e) {
70+
// This is expected when no duplicate exists, continue with creation
71+
}
72+
6273
$authorizedGroup = new AuthorizedGroup();
6374
$authorizedGroup->setGroupId($groupId);
6475
$authorizedGroup->setClass($class);
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
namespace OCA\Settings\Service;
8+
9+
class ConflictException extends ServiceException {
10+
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use OC\Settings\AuthorizedGroup;
1212
use OCA\Settings\Command\AdminDelegation\Add;
1313
use OCA\Settings\Service\AuthorizedGroupService;
14+
use OCA\Settings\Service\ConflictException;
1415
use OCP\IGroupManager;
1516
use OCP\Settings\IManager;
1617
use PHPUnit\Framework\MockObject\MockObject;
@@ -77,6 +78,35 @@ public function testExecuteSuccessfulDelegation(): void {
7778
$this->assertEquals(0, $result);
7879
}
7980

81+
public function testExecuteHandlesDuplicateAssignment(): void {
82+
$settingClass = 'OCA\\Settings\\Settings\\Admin\\Server';
83+
$groupId = 'testgroup';
84+
85+
// Mock valid delegated settings class
86+
$this->input->expects($this->exactly(2))
87+
->method('getArgument')
88+
->willReturnMap([
89+
['settingClass', $settingClass],
90+
['groupId', $groupId]
91+
]);
92+
93+
// Mock group exists
94+
$this->groupManager->expects($this->once())
95+
->method('groupExists')
96+
->with($groupId)
97+
->willReturn(true);
98+
99+
// Mock ConflictException when trying to create duplicate
100+
$this->authorizedGroupService->expects($this->once())
101+
->method('create')
102+
->with($groupId, $settingClass)
103+
->willThrowException(new ConflictException('Group is already assigned to this class'));
104+
105+
$result = $this->command->execute($this->input, $this->output);
106+
107+
$this->assertEquals(4, $result, 'Duplicate assignment should return exit code 4');
108+
}
109+
80110
public function testExecuteInvalidSettingClass(): void {
81111
// Use a real class that exists but doesn't implement IDelegatedSettings
82112
$settingClass = 'stdClass';
Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
namespace OCA\Settings\Tests\Integration;
8+
9+
use OC\Settings\AuthorizedGroup;
10+
use OC\Settings\AuthorizedGroupMapper;
11+
use OCA\Settings\Service\AuthorizedGroupService;
12+
use OCA\Settings\Service\ConflictException;
13+
use OCP\AppFramework\Db\DoesNotExistException;
14+
use Test\TestCase;
15+
16+
/**
17+
* Integration test for duplicate prevention in AuthorizedGroupService
18+
* This test verifies the complete flow of duplicate detection and prevention
19+
*/
20+
#[\PHPUnit\Framework\Attributes\Group('DB')]
21+
class DuplicateAssignmentIntegrationTest extends TestCase {
22+
23+
private AuthorizedGroupService $service;
24+
private AuthorizedGroupMapper $mapper;
25+
26+
protected function setUp(): void {
27+
parent::setUp();
28+
29+
// Use real mapper for integration testing
30+
$this->mapper = \OCP\Server::get(AuthorizedGroupMapper::class);
31+
$this->service = new AuthorizedGroupService($this->mapper);
32+
}
33+
34+
protected function tearDown(): void {
35+
// Clean up any test data
36+
try {
37+
$allGroups = $this->mapper->findAll();
38+
foreach ($allGroups as $group) {
39+
if (str_starts_with($group->getGroupId(), 'test_')
40+
|| str_starts_with($group->getClass(), 'TestClass')) {
41+
$this->mapper->delete($group);
42+
}
43+
}
44+
} catch (\Exception $e) {
45+
// Ignore cleanup errors
46+
}
47+
parent::tearDown();
48+
}
49+
50+
public function testDuplicateAssignmentPrevention(): void {
51+
$groupId = 'test_duplicate_group';
52+
$class = 'TestClass\\DuplicateTest';
53+
54+
// First assignment should succeed
55+
$result1 = $this->service->create($groupId, $class);
56+
$this->assertInstanceOf(AuthorizedGroup::class, $result1);
57+
$this->assertEquals($groupId, $result1->getGroupId());
58+
$this->assertEquals($class, $result1->getClass());
59+
$this->assertNotNull($result1->getId());
60+
61+
// Second assignment of same group to same class should throw ConflictException
62+
$this->expectException(ConflictException::class);
63+
$this->expectExceptionMessage('Group is already assigned to this class');
64+
65+
$this->service->create($groupId, $class);
66+
}
67+
68+
public function testDifferentGroupsSameClassAllowed(): void {
69+
$groupId1 = 'test_group_1';
70+
$groupId2 = 'test_group_2';
71+
$class = 'TestClass\\MultiGroup';
72+
73+
// Both assignments should succeed
74+
$result1 = $this->service->create($groupId1, $class);
75+
$result2 = $this->service->create($groupId2, $class);
76+
77+
$this->assertEquals($groupId1, $result1->getGroupId());
78+
$this->assertEquals($groupId2, $result2->getGroupId());
79+
$this->assertEquals($class, $result1->getClass());
80+
$this->assertEquals($class, $result2->getClass());
81+
$this->assertNotEquals($result1->getId(), $result2->getId());
82+
}
83+
84+
public function testSameGroupDifferentClassesAllowed(): void {
85+
$groupId = 'test_multi_class_group';
86+
$class1 = 'TestClass\\First';
87+
$class2 = 'TestClass\\Second';
88+
89+
// Both assignments should succeed
90+
$result1 = $this->service->create($groupId, $class1);
91+
$result2 = $this->service->create($groupId, $class2);
92+
93+
$this->assertEquals($groupId, $result1->getGroupId());
94+
$this->assertEquals($groupId, $result2->getGroupId());
95+
$this->assertEquals($class1, $result1->getClass());
96+
$this->assertEquals($class2, $result2->getClass());
97+
$this->assertNotEquals($result1->getId(), $result2->getId());
98+
}
99+
100+
public function testCreateAfterDelete(): void {
101+
$groupId = 'test_recreate_group';
102+
$class = 'TestClass\\Recreate';
103+
104+
// Create initial assignment
105+
$result1 = $this->service->create($groupId, $class);
106+
$initialId = $result1->getId();
107+
108+
// Delete the assignment
109+
$this->service->delete($initialId);
110+
111+
// Verify it's deleted by trying to find it
112+
$this->expectException(\OCP\AppFramework\Db\DoesNotExistException::class);
113+
try {
114+
$this->service->find($initialId);
115+
} catch (\OCA\Settings\Service\NotFoundException $e) {
116+
// Expected - now create the same assignment again, which should succeed
117+
$result2 = $this->service->create($groupId, $class);
118+
119+
$this->assertEquals($groupId, $result2->getGroupId());
120+
$this->assertEquals($class, $result2->getClass());
121+
$this->assertNotEquals($initialId, $result2->getId());
122+
return;
123+
}
124+
125+
$this->fail('Expected NotFoundException when finding deleted group');
126+
}
127+
128+
/**
129+
* Test the mapper's findByGroupIdAndClass method behavior with duplicates
130+
*/
131+
public function testMapperFindByGroupIdAndClassBehavior(): void {
132+
$groupId = 'test_mapper_group';
133+
$class = 'TestClass\\MapperTest';
134+
135+
// Initially should throw DoesNotExistException
136+
$this->expectException(DoesNotExistException::class);
137+
$this->mapper->findByGroupIdAndClass($groupId, $class);
138+
}
139+
140+
/**
141+
* Test that mapper returns existing record after creation
142+
*/
143+
public function testMapperFindsExistingRecord(): void {
144+
$groupId = 'test_existing_group';
145+
$class = 'TestClass\\Existing';
146+
147+
// Create the record first
148+
$created = $this->service->create($groupId, $class);
149+
150+
// Now mapper should find it
151+
$found = $this->mapper->findByGroupIdAndClass($groupId, $class);
152+
153+
$this->assertEquals($created->getId(), $found->getId());
154+
$this->assertEquals($groupId, $found->getGroupId());
155+
$this->assertEquals($class, $found->getClass());
156+
}
157+
}

0 commit comments

Comments
 (0)