Skip to content

Commit 51cde1a

Browse files
author
Kent Delante
authored
Merge pull request #59001 from IONOS-Productivity/tkl/dev/files_external-delegation-rebased-on-NC
feat(files_external): convert to delegated settings
2 parents c20fccc + faff52d commit 51cde1a

5 files changed

Lines changed: 208 additions & 8 deletions

File tree

‎apps/files_external/lib/Controller/AjaxController.php‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77
*/
88
namespace OCA\Files_External\Controller;
99

10+
use OC\Settings\AuthorizedGroupMapper;
1011
use OCA\Files_External\Lib\Auth\Password\GlobalAuth;
1112
use OCA\Files_External\Lib\Auth\PublicKey\RSA;
13+
use OCA\Files_External\Settings\Admin;
1214
use OCP\AppFramework\Controller;
1315
use OCP\AppFramework\Http;
16+
use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting;
1417
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
1518
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
1619
use OCP\AppFramework\Http\JSONResponse;
@@ -38,6 +41,7 @@ public function __construct(
3841
private IGroupManager $groupManager,
3942
private IUserManager $userManager,
4043
private IL10N $l10n,
44+
private AuthorizedGroupMapper $authorizedGroupMapper,
4145
) {
4246
parent::__construct($appName, $request);
4347
}
@@ -51,6 +55,7 @@ public function __construct(
5155
* @param int|null $offset The offset from which to start returning results
5256
* @return JSONResponse
5357
*/
58+
#[AuthorizedAdminSetting(settings: Admin::class)]
5459
public function getApplicableEntities(string $pattern = '', ?int $limit = null, ?int $offset = null): JSONResponse {
5560
$groups = [];
5661
foreach ($this->groupManager->search($pattern, $limit, $offset) as $group) {
@@ -112,10 +117,14 @@ public function saveGlobalCredentials($uid, $user, $password): JSONResponse {
112117
], Http::STATUS_UNAUTHORIZED);
113118
}
114119

115-
// Non-admins can only edit their own credentials
116-
// Admin can edit global credentials
120+
// Non-admins can only edit their own credentials.
121+
// Admin or delegated admin can edit global credentials (uid === '').
122+
// Cannot use #[AuthorizedAdminSetting] here because this endpoint is
123+
// #[NoAdminRequired] and must also allow users to edit their own (uid !== '')
124+
// credentials — the two paths share one method.
117125
$allowedToEdit = $uid === ''
118126
? $this->groupManager->isAdmin($currentUser->getUID())
127+
|| in_array(Admin::class, $this->authorizedGroupMapper->findAllClassesForUser($currentUser), true)
119128
: $currentUser->getUID() === $uid;
120129

121130
if ($allowedToEdit) {

‎apps/files_external/lib/Controller/GlobalStoragesController.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99

1010
use OCA\Files_External\NotFoundException;
1111
use OCA\Files_External\Service\GlobalStoragesService;
12+
use OCA\Files_External\Settings\Admin;
1213
use OCP\AppFramework\Http;
14+
use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting;
1315
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
1416
use OCP\AppFramework\Http\DataResponse;
1517
use OCP\IConfig;
@@ -60,6 +62,7 @@ public function __construct(
6062
* @param ?array $applicableGroups groups for which to mount the storage
6163
* @param ?int $priority priority
6264
*/
65+
#[AuthorizedAdminSetting(settings: Admin::class)]
6366
#[PasswordConfirmationRequired(strict: true)]
6467
public function create(
6568
string $mountPoint,
@@ -123,6 +126,7 @@ public function create(
123126
* @param ?array $applicableGroups groups for which to mount the storage
124127
* @param ?int $priority priority
125128
*/
129+
#[AuthorizedAdminSetting(settings: Admin::class)]
126130
#[PasswordConfirmationRequired(strict: true)]
127131
public function update(
128132
int $id,
@@ -173,4 +177,22 @@ public function update(
173177
Http::STATUS_OK
174178
);
175179
}
180+
181+
// PHP attributes are not inherited, so these methods override the parent
182+
// solely to attach #[AuthorizedAdminSetting] and expose them to delegated admins.
183+
#[AuthorizedAdminSetting(settings: Admin::class)]
184+
public function index() {
185+
return parent::index();
186+
}
187+
188+
#[AuthorizedAdminSetting(settings: Admin::class)]
189+
public function show(int $id, $testOnly = true) {
190+
return parent::show($id, $testOnly);
191+
}
192+
193+
#[AuthorizedAdminSetting(settings: Admin::class)]
194+
#[PasswordConfirmationRequired(strict: true)]
195+
public function destroy(int $id) {
196+
return parent::destroy($id);
197+
}
176198
}

‎apps/files_external/lib/Settings/Admin.php‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,11 @@
1313
use OCP\AppFramework\Http\TemplateResponse;
1414
use OCP\AppFramework\Services\IInitialState;
1515
use OCP\Encryption\IManager;
16+
use OCP\IL10N;
1617
use OCP\IURLGenerator;
17-
use OCP\Settings\ISettings;
18+
use OCP\Settings\IDelegatedSettings;
1819

19-
class Admin implements ISettings {
20+
class Admin implements IDelegatedSettings {
2021
use CommonSettingsTrait;
2122

2223
public function __construct(
@@ -26,6 +27,7 @@ public function __construct(
2627
private GlobalAuth $globalAuth,
2728
private IInitialState $initialState,
2829
private IURLGenerator $urlGenerator,
30+
private IL10N $l10n,
2931
) {
3032
$this->visibility = BackendService::VISIBILITY_ADMIN;
3133
}
@@ -65,4 +67,13 @@ public function getSection() {
6567
public function getPriority() {
6668
return 40;
6769
}
70+
71+
public function getName(): string {
72+
return $this->l10n->t('External storage');
73+
}
74+
75+
public function getAuthorizedAppConfig(): array {
76+
// No app config keys require delegation for external storage.
77+
return [];
78+
}
6879
}

‎apps/files_external/tests/Controller/AjaxControllerTest.php‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77
*/
88
namespace OCA\Files_External\Tests\Controller;
99

10+
use OC\Settings\AuthorizedGroupMapper;
1011
use OCA\Files_External\Controller\AjaxController;
1112
use OCA\Files_External\Lib\Auth\Password\GlobalAuth;
1213
use OCA\Files_External\Lib\Auth\PublicKey\RSA;
14+
use OCA\Files_External\Settings\Admin;
1315
use OCP\AppFramework\Http\JSONResponse;
16+
use OCP\IGroup;
1417
use OCP\IGroupManager;
1518
use OCP\IL10N;
1619
use OCP\IRequest;
@@ -28,6 +31,7 @@ class AjaxControllerTest extends TestCase {
2831
private IGroupManager&MockObject $groupManager;
2932
private IUserManager&MockObject $userManager;
3033
private IL10N&MockObject $l10n;
34+
private AuthorizedGroupMapper&MockObject $authorizedGroupMapper;
3135
private AjaxController $ajaxController;
3236

3337
protected function setUp(): void {
@@ -38,6 +42,7 @@ protected function setUp(): void {
3842
$this->groupManager = $this->createMock(IGroupManager::class);
3943
$this->userManager = $this->createMock(IUserManager::class);
4044
$this->l10n = $this->createMock(IL10N::class);
45+
$this->authorizedGroupMapper = $this->createMock(AuthorizedGroupMapper::class);
4146

4247
$this->ajaxController = new AjaxController(
4348
'files_external',
@@ -48,6 +53,7 @@ protected function setUp(): void {
4853
$this->groupManager,
4954
$this->userManager,
5055
$this->l10n,
56+
$this->authorizedGroupMapper,
5157
);
5258

5359
$this->l10n->expects($this->any())
@@ -62,6 +68,50 @@ protected function setUp(): void {
6268
parent::setUp();
6369
}
6470

71+
public function testGetApplicableEntitiesReturnsGroupsAndUsers(): void {
72+
$group = $this->createMock(IGroup::class);
73+
$group->method('getGID')->willReturn('group1');
74+
$group->method('getDisplayName')->willReturn('Group One');
75+
76+
$user = $this->createMock(IUser::class);
77+
$user->method('getUID')->willReturn('user1');
78+
$user->method('getDisplayName')->willReturn('User One');
79+
80+
$this->groupManager
81+
->expects($this->once())
82+
->method('search')
83+
->with('test', 10, 0)
84+
->willReturn([$group]);
85+
$this->userManager
86+
->expects($this->once())
87+
->method('searchDisplayName')
88+
->with('test', 10, 0)
89+
->willReturn([$user]);
90+
91+
$response = $this->ajaxController->getApplicableEntities('test', 10, 0);
92+
$this->assertSame(200, $response->getStatus());
93+
$this->assertSame(['group1' => 'Group One'], $response->getData()['groups']);
94+
$this->assertSame(['user1' => 'User One'], $response->getData()['users']);
95+
}
96+
97+
public function testGetApplicableEntitiesWithNoResults(): void {
98+
$this->groupManager
99+
->expects($this->once())
100+
->method('search')
101+
->with('', null, null)
102+
->willReturn([]);
103+
$this->userManager
104+
->expects($this->once())
105+
->method('searchDisplayName')
106+
->with('', null, null)
107+
->willReturn([]);
108+
109+
$response = $this->ajaxController->getApplicableEntities();
110+
$this->assertSame(200, $response->getStatus());
111+
$this->assertSame([], $response->getData()['groups']);
112+
$this->assertSame([], $response->getData()['users']);
113+
}
114+
65115
public function testGetSshKeys(): void {
66116
$this->rsa
67117
->expects($this->once())
@@ -153,4 +203,91 @@ public function testSaveGlobalCredentialsAsNormalUserForAnotherUser(): void {
153203
$this->assertSame($response->getStatus(), 403);
154204
$this->assertSame('Permission denied', $response->getData()['message']);
155205
}
206+
207+
public function testSaveGlobalCredentialsAsAdminForGlobal(): void {
208+
$user = $this->createMock(IUser::class);
209+
$user->method('getUID')->willReturn('MyAdminUid');
210+
$this->userSession->method('getUser')->willReturn($user);
211+
$this->groupManager
212+
->expects($this->once())
213+
->method('isAdmin')
214+
->with('MyAdminUid')
215+
->willReturn(true);
216+
$this->authorizedGroupMapper
217+
->expects($this->never())
218+
->method('findAllClassesForUser');
219+
$this->globalAuth
220+
->expects($this->once())
221+
->method('saveAuth')
222+
->with('', 'test', 'password');
223+
224+
$response = $this->ajaxController->saveGlobalCredentials('', 'test', 'password');
225+
$this->assertSame(200, $response->getStatus());
226+
}
227+
228+
public function testSaveGlobalCredentialsAsDelegatedAdminForGlobal(): void {
229+
$user = $this->createMock(IUser::class);
230+
$user->method('getUID')->willReturn('DelegatedUid');
231+
$this->userSession->method('getUser')->willReturn($user);
232+
$this->groupManager
233+
->expects($this->once())
234+
->method('isAdmin')
235+
->with('DelegatedUid')
236+
->willReturn(false);
237+
$this->authorizedGroupMapper
238+
->expects($this->once())
239+
->method('findAllClassesForUser')
240+
->with($user)
241+
->willReturn([Admin::class]);
242+
$this->globalAuth
243+
->expects($this->once())
244+
->method('saveAuth')
245+
->with('', 'test', 'password');
246+
247+
$response = $this->ajaxController->saveGlobalCredentials('', 'test', 'password');
248+
$this->assertSame(200, $response->getStatus());
249+
}
250+
251+
public function testSaveGlobalCredentialsAsDelegatedAdminForAnotherUser(): void {
252+
// Delegated admins may only set global (uid='') credentials, not impersonate other users.
253+
$user = $this->createMock(IUser::class);
254+
$user->method('getUID')->willReturn('DelegatedUid');
255+
$this->userSession->method('getUser')->willReturn($user);
256+
$this->groupManager
257+
->expects($this->never())
258+
->method('isAdmin');
259+
$this->authorizedGroupMapper
260+
->expects($this->never())
261+
->method('findAllClassesForUser');
262+
$this->globalAuth
263+
->expects($this->never())
264+
->method('saveAuth');
265+
266+
$response = $this->ajaxController->saveGlobalCredentials('OtherUserUid', 'test', 'password');
267+
$this->assertSame(403, $response->getStatus());
268+
$this->assertSame('Permission denied', $response->getData()['message']);
269+
}
270+
271+
public function testSaveGlobalCredentialsAsNormalUserForGlobal(): void {
272+
$user = $this->createMock(IUser::class);
273+
$user->method('getUID')->willReturn('NormalUid');
274+
$this->userSession->method('getUser')->willReturn($user);
275+
$this->groupManager
276+
->expects($this->once())
277+
->method('isAdmin')
278+
->with('NormalUid')
279+
->willReturn(false);
280+
$this->authorizedGroupMapper
281+
->expects($this->once())
282+
->method('findAllClassesForUser')
283+
->with($user)
284+
->willReturn([]);
285+
$this->globalAuth
286+
->expects($this->never())
287+
->method('saveAuth');
288+
289+
$response = $this->ajaxController->saveGlobalCredentials('', 'test', 'password');
290+
$this->assertSame(403, $response->getStatus());
291+
$this->assertSame('Permission denied', $response->getData()['message']);
292+
}
156293
}

‎apps/files_external/tests/Settings/AdminTest.php‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212
use OCA\Files_External\Service\BackendService;
1313
use OCA\Files_External\Service\GlobalStoragesService;
1414
use OCA\Files_External\Settings\Admin;
15-
use OCP\App\IAppManager;
1615
use OCP\AppFramework\Http\TemplateResponse;
1716
use OCP\AppFramework\Services\IInitialState;
1817
use OCP\Encryption\IManager;
18+
use OCP\IL10N;
1919
use OCP\IURLGenerator;
2020
use PHPUnit\Framework\MockObject\MockObject;
2121
use Test\TestCase;
@@ -27,7 +27,7 @@ class AdminTest extends TestCase {
2727
private GlobalAuth&MockObject $globalAuth;
2828
private IInitialState&MockObject $initialState;
2929
private IURLGenerator&MockObject $urlGenerator;
30-
private IAppManager&MockObject $appManager;
30+
private IL10N&MockObject $l10n;
3131
private Admin $admin;
3232

3333
protected function setUp(): void {
@@ -38,7 +38,10 @@ protected function setUp(): void {
3838
$this->globalAuth = $this->createMock(GlobalAuth::class);
3939
$this->initialState = $this->createMock(IInitialState::class);
4040
$this->urlGenerator = $this->createMock(IURLGenerator::class);
41-
$this->appManager = $this->createMock(IAppManager::class);
41+
$this->l10n = $this->createMock(IL10N::class);
42+
$this->l10n->method('t')->willReturnCallback(function ($text) {
43+
return $text;
44+
});
4245

4346
$this->admin = new Admin(
4447
$this->encryptionManager,
@@ -47,7 +50,7 @@ protected function setUp(): void {
4750
$this->globalAuth,
4851
$this->initialState,
4952
$this->urlGenerator,
50-
$this->appManager,
53+
$this->l10n,
5154
);
5255
}
5356

@@ -123,4 +126,22 @@ public function testGetSection(): void {
123126
public function testGetPriority(): void {
124127
$this->assertSame(40, $this->admin->getPriority());
125128
}
129+
130+
public function testGetName(): void {
131+
$this->l10n->expects($this->once())
132+
->method('t')
133+
->with('External storage')
134+
->willReturn('External storage');
135+
136+
$this->assertSame('External storage', $this->admin->getName());
137+
}
138+
139+
public function testGetAuthorizedAppConfig(): void {
140+
$this->assertSame([], $this->admin->getAuthorizedAppConfig());
141+
}
142+
143+
public function testImplementsIDelegatedSettings(): void {
144+
$this->assertInstanceOf(\OCP\Settings\IDelegatedSettings::class, $this->admin);
145+
$this->assertInstanceOf(\OCP\Settings\ISettings::class, $this->admin);
146+
}
126147
}

0 commit comments

Comments
 (0)