Skip to content

Commit 3efb1d8

Browse files
artongeartonge
authored
Merge pull request #56646 from nextcloud/fix/no_double_admin_delegations
fix(admin-delegation): Prevent delegation to group if delegation already
2 parents 52e3762 + 770ad62 commit 3efb1d8

File tree

8 files changed

+289
-1
lines changed

8 files changed

+289
-1
lines changed

‎apps/settings/composer/composer/autoload_classmap.php‎

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

‎apps/settings/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ class ComposerStaticInitSettings
8282
'OCA\\Settings\\Sections\\Personal\\Security' => __DIR__ . '/..' . '/../lib/Sections/Personal/Security.php',
8383
'OCA\\Settings\\Sections\\Personal\\SyncClients' => __DIR__ . '/..' . '/../lib/Sections/Personal/SyncClients.php',
8484
'OCA\\Settings\\Service\\AuthorizedGroupService' => __DIR__ . '/..' . '/../lib/Service/AuthorizedGroupService.php',
85+
'OCA\\Settings\\Service\\ConflictException' => __DIR__ . '/..' . '/../lib/Service/ConflictException.php',
8586
'OCA\\Settings\\Service\\NotFoundException' => __DIR__ . '/..' . '/../lib/Service/NotFoundException.php',
8687
'OCA\\Settings\\Service\\ServiceException' => __DIR__ . '/..' . '/../lib/Service/ServiceException.php',
8788
'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);

‎apps/settings/lib/Service/ConflictException.php‎

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 OCA\Settings\Settings\Admin\Server;
1516
use OCP\IGroupManager;
1617
use OCP\Settings\IManager;
@@ -78,6 +79,35 @@ public function testExecuteSuccessfulDelegation(): void {
7879
$this->assertEquals(0, $result);
7980
}
8081

82+
public function testExecuteHandlesDuplicateAssignment(): void {
83+
$settingClass = 'OCA\\Settings\\Settings\\Admin\\Server';
84+
$groupId = 'testgroup';
85+
86+
// Mock valid delegated settings class
87+
$this->input->expects($this->exactly(2))
88+
->method('getArgument')
89+
->willReturnMap([
90+
['settingClass', $settingClass],
91+
['groupId', $groupId]
92+
]);
93+
94+
// Mock group exists
95+
$this->groupManager->expects($this->once())
96+
->method('groupExists')
97+
->with($groupId)
98+
->willReturn(true);
99+
100+
// Mock ConflictException when trying to create duplicate
101+
$this->authorizedGroupService->expects($this->once())
102+
->method('create')
103+
->with($groupId, $settingClass)
104+
->willThrowException(new ConflictException('Group is already assigned to this class'));
105+
106+
$result = $this->command->execute($this->input, $this->output);
107+
108+
$this->assertEquals(4, $result, 'Duplicate assignment should return exit code 4');
109+
}
110+
81111
public function testExecuteInvalidSettingClass(): void {
82112
// Use a real class that exists but doesn't implement IDelegatedSettings
83113
$settingClass = 'stdClass';

‎apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php‎

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+
}

‎apps/settings/tests/Service/AuthorizedGroupServiceTest.php‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
use OC\Settings\AuthorizedGroup;
1313
use OC\Settings\AuthorizedGroupMapper;
1414
use OCA\Settings\Service\AuthorizedGroupService;
15+
use OCA\Settings\Service\ConflictException;
16+
use OCP\AppFramework\Db\DoesNotExistException;
1517
use PHPUnit\Framework\MockObject\MockObject;
1618
use Test\TestCase;
1719

@@ -26,11 +28,72 @@ protected function setUp(): void {
2628
$this->service = new AuthorizedGroupService($this->mapper);
2729
}
2830

31+
public function testCreateSuccessWhenNoDuplicateExists(): void {
32+
$groupId = 'testgroup';
33+
$class = 'TestClass';
34+
35+
// Mock that no existing assignment is found (throws DoesNotExistException)
36+
$this->mapper->expects($this->once())
37+
->method('findByGroupIdAndClass')
38+
->with($groupId, $class)
39+
->willThrowException(new DoesNotExistException('Not found'));
40+
41+
// Mock the successful creation
42+
$expectedGroup = new AuthorizedGroup();
43+
$expectedGroup->setGroupId($groupId);
44+
$expectedGroup->setClass($class);
45+
$expectedGroup->setId(123);
46+
47+
$this->mapper->expects($this->once())
48+
->method('insert')
49+
->willReturn($expectedGroup);
50+
51+
$result = $this->service->create($groupId, $class);
52+
53+
$this->assertInstanceOf(AuthorizedGroup::class, $result);
54+
$this->assertEquals($groupId, $result->getGroupId());
55+
$this->assertEquals($class, $result->getClass());
56+
}
57+
58+
public function testCreateThrowsConflictExceptionWhenDuplicateExists(): void {
59+
$groupId = 'testgroup';
60+
$class = 'TestClass';
61+
62+
// Mock that an existing assignment is found
63+
$existingGroup = new AuthorizedGroup();
64+
$existingGroup->setGroupId($groupId);
65+
$existingGroup->setClass($class);
66+
$existingGroup->setId(42);
67+
68+
$this->mapper->expects($this->once())
69+
->method('findByGroupIdAndClass')
70+
->with($groupId, $class)
71+
->willReturn($existingGroup);
72+
73+
// Mapper insert should never be called when duplicate exists
74+
$this->mapper->expects($this->never())
75+
->method('insert');
76+
77+
$this->expectException(ConflictException::class);
78+
$this->expectExceptionMessage('Group is already assigned to this class');
79+
80+
$this->service->create($groupId, $class);
81+
}
82+
2983
public function testCreateAllowsDifferentGroupsSameClass(): void {
3084
$groupId1 = 'testgroup1';
3185
$groupId2 = 'testgroup2';
3286
$class = 'TestClass';
3387

88+
// Mock that no duplicate exists for group1
89+
$this->mapper->expects($this->exactly(2))
90+
->method('findByGroupIdAndClass')
91+
->willReturnCallback(function ($groupId, $classArg) use ($groupId1, $groupId2, $class) {
92+
$this->assertContains($groupId, [$groupId1, $groupId2]);
93+
$this->assertEquals($class, $classArg);
94+
throw new DoesNotExistException('Not found');
95+
});
96+
3497
$expectedGroup1 = new AuthorizedGroup();
3598
$expectedGroup1->setGroupId($groupId1);
3699
$expectedGroup1->setClass($class);
@@ -60,6 +123,15 @@ public function testCreateAllowsSameGroupDifferentClasses(): void {
60123
$class1 = 'TestClass1';
61124
$class2 = 'TestClass2';
62125

126+
// Mock that no duplicate exists for either class
127+
$this->mapper->expects($this->exactly(2))
128+
->method('findByGroupIdAndClass')
129+
->willReturnCallback(function ($groupIdArg, $class) use ($groupId, $class1, $class2) {
130+
$this->assertEquals($groupId, $groupIdArg);
131+
$this->assertContains($class, [$class1, $class2]);
132+
throw new DoesNotExistException('Not found');
133+
});
134+
63135
$expectedGroup1 = new AuthorizedGroup();
64136
$expectedGroup1->setGroupId($groupId);
65137
$expectedGroup1->setClass($class1);

0 commit comments

Comments
 (0)