Skip to content

fix(DeclarativeSettings): Make declarative settings delegation aware #61864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marcelklehr wants to merge 1 commit into
base: master
Choose a base branch
from
+90 −12
Open

fix(DeclarativeSettings): Make declarative settings delegation aware #61864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 50 additions & 11 deletions lib/private/Settings/DeclarativeManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use OCP\Settings\IDeclarativeManager;
use OCP\Settings\IDeclarativeSettingsForm;
use OCP\Settings\IDeclarativeSettingsFormWithHandlers;
use OCP\Settings\IManager as ISettingsManager;
use Psr\Log\LoggerInterface;

/**
Expand Down Expand Up @@ -52,6 +53,7 @@ public function __construct(
private IAppConfig $appConfig,
private LoggerInterface $logger,
private ICrypto $crypto,
private ISettingsManager $settingsManager,
) {
}

Expand Down Expand Up @@ -106,20 +108,20 @@ public function loadSchemas(): void {
*/
#[\Override]
public function getFormIDs(IUser $user, string $type, string $section): array {
$isAdmin = $this->groupManager->isAdmin($user->getUID());
/** @var array<string, list<string>> $formIds */
$formIds = [];

foreach ($this->appSchemas as $app => $schemas) {
$ids = [];
usort($schemas, [$this, 'sortSchemasByPriorityCallback']);
foreach ($schemas as $schema) {
if ($schema['section_type'] === DeclarativeSettingsTypes::SECTION_TYPE_ADMIN && !$isAdmin) {
if ($schema['section_type'] !== $type || $schema['section_id'] !== $section) {
continue;
}
if ($schema['section_type'] === $type && $schema['section_id'] === $section) {
$ids[] = $schema['id'];
if (!$this->canAccessSection($user, $schema['section_type'], $schema['section_id'])) {
continue;
}
$ids[] = $schema['id'];
}

if (!empty($ids)) {
Expand All @@ -136,7 +138,6 @@ public function getFormIDs(IUser $user, string $type, string $section): array {
*/
#[\Override]
public function getFormsWithValues(IUser $user, ?string $type, ?string $section): array {
$isAdmin = $this->groupManager->isAdmin($user->getUID());
$forms = [];

foreach ($this->appSchemas as $app => $schemas) {
Expand All @@ -147,8 +148,9 @@ public function getFormsWithValues(IUser $user, ?string $type, ?string $section)
if ($section !== null && $schema['section_id'] !== $section) {
continue;
}
// If listing all fields skip the admin fields which a non-admin user has no access to
if ($type === null && $schema['section_type'] === 'admin' && !$isAdmin) {
// Skip sections the user is not allowed to access. Admin sections require
// admin rights or an admin delegation covering the section.
if (!$this->canAccessSection($user, $schema['section_type'], $schema['section_id'])) {
continue;
}

Expand Down Expand Up @@ -218,24 +220,61 @@ private function getSectionType(string $app, string $fieldId): string {
throw new Exception('Unknown fieldId "' . $fieldId . '"');
}

/**
* @throws Exception
*/
private function getSectionId(string $app, string $fieldId): string {
if (array_key_exists($app, $this->appSchemas)) {
foreach ($this->appSchemas[$app] as $schema) {
foreach ($schema['fields'] as $field) {
if ($field['id'] == $fieldId) {
return $schema['section_id'];
}
}
}
}

throw new Exception('Unknown fieldId "' . $fieldId . '"');
}
Comment on lines +226 to +238

@come-nc come-nc Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be avoided, it’s a net loss in performance.
For getValue which is private we can add the section id to the parameters.
For setValue it may be more complicated but it’s worth trying to avoid it as well if possible. (notably if user is an admin no need to search for section id)


/**
* @psalm-param DeclarativeSettingsSectionType $sectionType
* @throws NotAdminException
*/
private function assertAuthorized(IUser $user, string $sectionType): void {
if ($sectionType === 'admin' && !$this->groupManager->isAdmin($user->getUID())) {
private function assertAuthorized(IUser $user, string $sectionType, string $sectionId): void {
if (!$this->canAccessSection($user, $sectionType, $sectionId)) {
throw new NotAdminException('Logged in user does not have permission to access these settings.');
}
}

/**
* Whether the user may access the given settings section.
*
* Personal sections are always accessible. Admin sections require admin rights
* or an admin delegation covering the section: delegation is section-based, so
* being authorized for any (non-declarative) setting of the section also grants
* access to the declarative forms of that same section.
Comment on lines +254 to +256

@come-nc come-nc Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delegation is not section-based, it’s for each settings class.

*
* @psalm-param DeclarativeSettingsSectionType $sectionType
*/
private function canAccessSection(IUser $user, string $sectionType, string $sectionId): bool {
if ($sectionType !== DeclarativeSettingsTypes::SECTION_TYPE_ADMIN) {
return true;
}
if ($this->groupManager->isAdmin($user->getUID())) {
return true;
}
return $this->settingsManager->getAllowedAdminSettings($sectionId, $user) !== [];
}

/**
* @return DeclarativeSettingsValueTypes
* @throws Exception
* @throws NotAdminException
*/
private function getValue(IUser $user, string $app, string $formId, string $fieldId): mixed {
$sectionType = $this->getSectionType($app, $fieldId);
$this->assertAuthorized($user, $sectionType);
$this->assertAuthorized($user, $sectionType, $this->getSectionId($app, $fieldId));

$storageType = $this->getStorageType($app, $fieldId);
switch ($storageType) {
Expand All @@ -260,7 +299,7 @@ private function getValue(IUser $user, string $app, string $formId, string $fiel
#[\Override]
public function setValue(IUser $user, string $app, string $formId, string $fieldId, mixed $value): void {
$sectionType = $this->getSectionType($app, $fieldId);
$this->assertAuthorized($user, $sectionType);
$this->assertAuthorized($user, $sectionType, $this->getSectionId($app, $fieldId));

$storageType = $this->getStorageType($app, $fieldId);
switch ($storageType) {
Expand Down
41 changes: 40 additions & 1 deletion tests/lib/Settings/DeclarativeManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use OCP\Settings\IDeclarativeManager;
use OCP\Settings\IDeclarativeSettingsForm;
use OCP\Settings\IDeclarativeSettingsFormWithHandlers;
use OCP\Settings\IManager as ISettingsManager;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
use Test\TestCase;
Expand Down Expand Up @@ -54,6 +55,9 @@ class DeclarativeManagerTest extends TestCase {
/** @var ICrypto|MockObject */
private $crypto;

/** @var ISettingsManager|MockObject */
private $settingsManager;

Comment on lines +58 to +60

@come-nc come-nc Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @var ISettingsManager|MockObject */
private $settingsManager;
private ISettingsManager&MockObject $settingsManager;

/** @var IUser|MockObject */
private $user;

Expand Down Expand Up @@ -265,6 +269,7 @@ protected function setUp(): void {
$this->appConfig = $this->createMock(IAppConfig::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->crypto = $this->createMock(ICrypto::class);
$this->settingsManager = $this->createMock(ISettingsManager::class);

$this->declarativeManager = new DeclarativeManager(
$this->eventDispatcher,
Expand All @@ -274,6 +279,7 @@ protected function setUp(): void {
$this->appConfig,
$this->logger,
$this->crypto,
$this->settingsManager,
);

$this->user = $this->createMock(IUser::class);
Expand Down Expand Up @@ -556,8 +562,41 @@ public function testAdminFormUserUnauthorized(): void {
$schema = self::validSchemaAllFields;
$this->declarativeManager->registerSchema($app, $schema);

// A non-admin user without delegation for the section must not receive the admin
// declarative form, but this must not throw: throwing would abort rendering of a
// whole section a user can otherwise legitimately access through admin delegation.
$this->settingsManager->method('getAllowedAdminSettings')->willReturn([]);
$forms = $this->declarativeManager->getFormsWithValues($this->user, $schema['section_type'], $schema['section_id']);
$this->assertEmpty($forms);

// Writing to an admin declarative form is still forbidden for such a user.
$this->expectException(\Exception::class);
$this->declarativeManager->getFormsWithValues($this->user, $schema['section_type'], $schema['section_id']);
$this->declarativeManager->setValue($this->user, $app, $schema['id'], 'some_real_setting', '120m');
}

public function testAdminFormDelegatedUserAuthorized(): void {
$app = 'testing';
$schema = self::validSchemaAllFields;
$this->declarativeManager->registerSchema($app, $schema);

// The user is not an admin but was granted access to the section through admin
// delegation (i.e. getAllowedAdminSettings returns settings for the section).
$this->settingsManager->method('getAllowedAdminSettings')
->with($schema['section_id'], $this->user)
->willReturn([['a delegated setting']]);
$this->config->method('getAppValue')
->willReturnCallback(fn ($app, $configkey, $default) => $default);

// The declarative form of the delegated section must be visible ...
$forms = $this->declarativeManager->getFormsWithValues($this->user, $schema['section_type'], $schema['section_id']);
$this->assertCount(1, $forms);
$this->assertSame($schema['id'], $forms[0]['id']);

// ... and writable.
$this->appConfig->expects($this->once())
->method('setValueString')
->with($app, 'some_real_setting', '120m');
$this->declarativeManager->setValue($this->user, $app, $schema['id'], 'some_real_setting', '120m');
}

/**
Expand Down
Loading