diff --git a/lib/private/Settings/DeclarativeManager.php b/lib/private/Settings/DeclarativeManager.php index a3d7e62b15abb..6f91846422698 100644 --- a/lib/private/Settings/DeclarativeManager.php +++ b/lib/private/Settings/DeclarativeManager.php @@ -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; /** @@ -52,6 +53,7 @@ public function __construct( private IAppConfig $appConfig, private LoggerInterface $logger, private ICrypto $crypto, + private ISettingsManager $settingsManager, ) { } @@ -106,7 +108,6 @@ public function loadSchemas(): void { */ #[\Override] public function getFormIDs(IUser $user, string $type, string $section): array { - $isAdmin = $this->groupManager->isAdmin($user->getUID()); /** @var array> $formIds */ $formIds = []; @@ -114,12 +115,13 @@ public function getFormIDs(IUser $user, string $type, string $section): array { $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)) { @@ -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) { @@ -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; } @@ -218,16 +220,53 @@ 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 . '"'); + } + /** * @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. + * + * @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 @@ -235,7 +274,7 @@ private function assertAuthorized(IUser $user, string $sectionType): void { */ 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) { @@ -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) { diff --git a/tests/lib/Settings/DeclarativeManagerTest.php b/tests/lib/Settings/DeclarativeManagerTest.php index fc9fbe2379a60..366bb30aadbc7 100644 --- a/tests/lib/Settings/DeclarativeManagerTest.php +++ b/tests/lib/Settings/DeclarativeManagerTest.php @@ -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; @@ -54,6 +55,9 @@ class DeclarativeManagerTest extends TestCase { /** @var ICrypto|MockObject */ private $crypto; + /** @var ISettingsManager|MockObject */ + private $settingsManager; + /** @var IUser|MockObject */ private $user; @@ -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, @@ -274,6 +279,7 @@ protected function setUp(): void { $this->appConfig, $this->logger, $this->crypto, + $this->settingsManager, ); $this->user = $this->createMock(IUser::class); @@ -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'); } /**