Skip to content

Commit 8cd9c11

Browse files
printminion-coprintminion-co
committed
fix(admin-delegation): Prevent delegation to group if delegation already exist
Signed-off-by: Misha M.-Kupriyanov <[email protected]>
1 parent 9d92f20 commit 8cd9c11

File tree

8 files changed

+288
-1
lines changed

8 files changed

+288
-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('The ' . $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: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
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+
class DuplicateAssignmentIntegrationTest extends TestCase {
21+
22+
private AuthorizedGroupService $service;
23+
private AuthorizedGroupMapper $mapper;
24+
25+
protected function setUp(): void {
26+
parent::setUp();
27+
28+
// Use real mapper for integration testing
29+
$this->mapper = \OC::$server->get(AuthorizedGroupMapper::class);
30+
$this->service = new AuthorizedGroupService($this->mapper);
31+
}
32+
33+
protected function tearDown(): void {
34+
// Clean up any test data
35+
try {
36+
$allGroups = $this->mapper->findAll();
37+
foreach ($allGroups as $group) {
38+
if (str_starts_with($group->getGroupId(), 'test_') ||
39+
str_starts_with($group->getClass(), 'TestClass')) {
40+
$this->mapper->delete($group);
41+
}
42+
}
43+
} catch (\Exception $e) {
44+
// Ignore cleanup errors
45+
}
46+
parent::tearDown();
47+
}
48+
49+
public function testDuplicateAssignmentPrevention(): void {
50+
$groupId = 'test_duplicate_group';
51+
$class = 'TestClass\\DuplicateTest';
52+
53+
// First assignment should succeed
54+
$result1 = $this->service->create($groupId, $class);
55+
$this->assertInstanceOf(AuthorizedGroup::class, $result1);
56+
$this->assertEquals($groupId, $result1->getGroupId());
57+
$this->assertEquals($class, $result1->getClass());
58+
$this->assertNotNull($result1->getId());
59+
60+
// Second assignment of same group to same class should throw ConflictException
61+
$this->expectException(ConflictException::class);
62+
$this->expectExceptionMessage('Group is already assigned to this class');
63+
64+
$this->service->create($groupId, $class);
65+
}
66+
67+
public function testDifferentGroupsSameClassAllowed(): void {
68+
$groupId1 = 'test_group_1';
69+
$groupId2 = 'test_group_2';
70+
$class = 'TestClass\\MultiGroup';
71+
72+
// Both assignments should succeed
73+
$result1 = $this->service->create($groupId1, $class);
74+
$result2 = $this->service->create($groupId2, $class);
75+
76+
$this->assertEquals($groupId1, $result1->getGroupId());
77+
$this->assertEquals($groupId2, $result2->getGroupId());
78+
$this->assertEquals($class, $result1->getClass());
79+
$this->assertEquals($class, $result2->getClass());
80+
$this->assertNotEquals($result1->getId(), $result2->getId());
81+
}
82+
83+
public function testSameGroupDifferentClassesAllowed(): void {
84+
$groupId = 'test_multi_class_group';
85+
$class1 = 'TestClass\\First';
86+
$class2 = 'TestClass\\Second';
87+
88+
// Both assignments should succeed
89+
$result1 = $this->service->create($groupId, $class1);
90+
$result2 = $this->service->create($groupId, $class2);
91+
92+
$this->assertEquals($groupId, $result1->getGroupId());
93+
$this->assertEquals($groupId, $result2->getGroupId());
94+
$this->assertEquals($class1, $result1->getClass());
95+
$this->assertEquals($class2, $result2->getClass());
96+
$this->assertNotEquals($result1->getId(), $result2->getId());
97+
}
98+
99+
public function testCreateAfterDelete(): void {
100+
$groupId = 'test_recreate_group';
101+
$class = 'TestClass\\Recreate';
102+
103+
// Create initial assignment
104+
$result1 = $this->service->create($groupId, $class);
105+
$initialId = $result1->getId();
106+
107+
// Delete the assignment
108+
$this->service->delete($initialId);
109+
110+
// Verify it's deleted by trying to find it
111+
$this->expectException(\OCP\AppFramework\Db\DoesNotExistException::class);
112+
try {
113+
$this->service->find($initialId);
114+
} catch (\OCA\Settings\Service\NotFoundException $e) {
115+
// Expected - now create the same assignment again, which should succeed
116+
$result2 = $this->service->create($groupId, $class);
117+
118+
$this->assertEquals($groupId, $result2->getGroupId());
119+
$this->assertEquals($class, $result2->getClass());
120+
$this->assertNotEquals($initialId, $result2->getId());
121+
return;
122+
}
123+
124+
$this->fail('Expected NotFoundException when finding deleted group');
125+
}
126+
127+
/**
128+
* Test the mapper's findByGroupIdAndClass method behavior with duplicates
129+
*/
130+
public function testMapperFindByGroupIdAndClassBehavior(): void {
131+
$groupId = 'test_mapper_group';
132+
$class = 'TestClass\\MapperTest';
133+
134+
// Initially should throw DoesNotExistException
135+
$this->expectException(DoesNotExistException::class);
136+
$this->mapper->findByGroupIdAndClass($groupId, $class);
137+
}
138+
139+
/**
140+
* Test that mapper returns existing record after creation
141+
*/
142+
public function testMapperFindsExistingRecord(): void {
143+
$groupId = 'test_existing_group';
144+
$class = 'TestClass\\Existing';
145+
146+
// Create the record first
147+
$created = $this->service->create($groupId, $class);
148+
149+
// Now mapper should find it
150+
$found = $this->mapper->findByGroupIdAndClass($groupId, $class);
151+
152+
$this->assertEquals($created->getId(), $found->getId());
153+
$this->assertEquals($groupId, $found->getGroupId());
154+
$this->assertEquals($class, $found->getClass());
155+
}
156+
}

‎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)