From 3f0b8525ceda3da9d9255f968150c82f1030a612 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 12 Nov 2026 23:04:59 +0100 Subject: [PATCH 1/4] feat: add API to restrict permissions on resources of other apps Apps register their resource types and permissions, other apps register restriction providers. The app owning a resource asks the permission manager which permissions are left for an actor and enforces the result. Permissions are boolean and every app permission is a kind of one core permission, so restricting a core permission also restricts permissions the restricting app does not know. Access entries allow to restrict only the access gained through one recipient, e.g. a team. Assisted-by: ClaudeCode:claude-fable-5-2 Signed-off-by: Ferdinand Thiessen --- core/AppInfo/Application.php | 13 + core/Resource/Permission/CreatePermission.php | 42 ++ core/Resource/Permission/DeletePermission.php | 42 ++ .../Permission/DownloadPermission.php | 42 ++ core/Resource/Permission/ReadPermission.php | 42 ++ core/Resource/Permission/SharePermission.php | 42 ++ core/Resource/Permission/UpdatePermission.php | 42 ++ lib/composer/composer/autoload_classmap.php | 18 + lib/composer/composer/autoload_static.php | 18 + .../Bootstrap/RegistrationContext.php | 69 +++ .../Resource/Permission/PermissionManager.php | 338 +++++++++++++ lib/private/Server.php | 2 + .../Bootstrap/IRegistrationContext.php | 30 ++ lib/unstable/Resource/IResourceType.php | 46 ++ .../Resource/Permission/APermission.php | 34 ++ .../Resource/Permission/AccessEntry.php | 51 ++ .../Resource/Permission/CorePermission.php | 53 ++ .../Permission/EffectivePermissions.php | 45 ++ .../Resource/Permission/IPermission.php | 59 +++ .../Permission/IPermissionManager.php | 72 +++ .../Permission/IRestrictionProvider.php | 36 ++ .../Resource/Permission/ResourceAccess.php | 32 ++ .../Resource/Permission/Restriction.php | 60 +++ lib/unstable/Resource/ResourceReference.php | 47 ++ .../Permission/PermissionManagerTest.php | 470 ++++++++++++++++++ 25 files changed, 1745 insertions(+) create mode 100644 core/Resource/Permission/CreatePermission.php create mode 100644 core/Resource/Permission/DeletePermission.php create mode 100644 core/Resource/Permission/DownloadPermission.php create mode 100644 core/Resource/Permission/ReadPermission.php create mode 100644 core/Resource/Permission/SharePermission.php create mode 100644 core/Resource/Permission/UpdatePermission.php create mode 100644 lib/private/Resource/Permission/PermissionManager.php create mode 100644 lib/unstable/Resource/IResourceType.php create mode 100644 lib/unstable/Resource/Permission/APermission.php create mode 100644 lib/unstable/Resource/Permission/AccessEntry.php create mode 100644 lib/unstable/Resource/Permission/CorePermission.php create mode 100644 lib/unstable/Resource/Permission/EffectivePermissions.php create mode 100644 lib/unstable/Resource/Permission/IPermission.php create mode 100644 lib/unstable/Resource/Permission/IPermissionManager.php create mode 100644 lib/unstable/Resource/Permission/IRestrictionProvider.php create mode 100644 lib/unstable/Resource/Permission/ResourceAccess.php create mode 100644 lib/unstable/Resource/Permission/Restriction.php create mode 100644 lib/unstable/Resource/ResourceReference.php create mode 100644 tests/lib/Resource/Permission/PermissionManagerTest.php diff --git a/core/AppInfo/Application.php b/core/AppInfo/Application.php index 32ee1041c24bb..ca354f25bcac8 100644 --- a/core/AppInfo/Application.php +++ b/core/AppInfo/Application.php @@ -28,6 +28,12 @@ use OC\Core\Listener\PasswordUpdatedListener; use OC\Core\Listener\RestrictInteractionListener; use OC\Core\Notification\CoreNotifier; +use OC\Core\Resource\Permission\CreatePermission; +use OC\Core\Resource\Permission\DeletePermission; +use OC\Core\Resource\Permission\DownloadPermission; +use OC\Core\Resource\Permission\ReadPermission; +use OC\Core\Resource\Permission\SharePermission; +use OC\Core\Resource\Permission\UpdatePermission; use OC\Core\Sharing\Permission\EditSharePermissionPreset; use OC\Core\Sharing\Permission\ReshareSharePermissionType; use OC\Core\Sharing\Permission\ViewSharePermissionPreset; @@ -89,6 +95,13 @@ public function register(IRegistrationContext $context): void { return Util::getDefaultEmailAddress('lostpassword-noreply'); }); + $context->registerResourcePermission(ReadPermission::class); + $context->registerResourcePermission(DownloadPermission::class); + $context->registerResourcePermission(UpdatePermission::class); + $context->registerResourcePermission(CreatePermission::class); + $context->registerResourcePermission(DeletePermission::class); + $context->registerResourcePermission(SharePermission::class); + // register notifier $context->registerNotifierService(CoreNotifier::class); $context->registerNotifierService(AuthenticationNotifier::class); diff --git a/core/Resource/Permission/CreatePermission.php b/core/Resource/Permission/CreatePermission.php new file mode 100644 index 0000000000000..755789b79cf9b --- /dev/null +++ b/core/Resource/Permission/CreatePermission.php @@ -0,0 +1,42 @@ +value; + } + + #[\Override] + public function getDisplayName(): string { + return $this->l10nFactory->get(Application::APP_ID)->t('Create'); + } + + #[\Override] + public function getHint(): ?string { + return null; + } + + #[\Override] + public function getKindOf(): ?CorePermission { + return null; + } +} diff --git a/core/Resource/Permission/DeletePermission.php b/core/Resource/Permission/DeletePermission.php new file mode 100644 index 0000000000000..7066f28a28787 --- /dev/null +++ b/core/Resource/Permission/DeletePermission.php @@ -0,0 +1,42 @@ +value; + } + + #[\Override] + public function getDisplayName(): string { + return $this->l10nFactory->get(Application::APP_ID)->t('Delete'); + } + + #[\Override] + public function getHint(): ?string { + return null; + } + + #[\Override] + public function getKindOf(): ?CorePermission { + return null; + } +} diff --git a/core/Resource/Permission/DownloadPermission.php b/core/Resource/Permission/DownloadPermission.php new file mode 100644 index 0000000000000..d30f0eea4fd2a --- /dev/null +++ b/core/Resource/Permission/DownloadPermission.php @@ -0,0 +1,42 @@ +l10nFactory->get(Application::APP_ID)->t('Download'); + } + + #[\Override] + public function getHint(): ?string { + return null; + } + + #[\Override] + public function getKindOf(): ?CorePermission { + return CorePermission::Read; + } +} diff --git a/core/Resource/Permission/ReadPermission.php b/core/Resource/Permission/ReadPermission.php new file mode 100644 index 0000000000000..a4fcb3c804fa2 --- /dev/null +++ b/core/Resource/Permission/ReadPermission.php @@ -0,0 +1,42 @@ +value; + } + + #[\Override] + public function getDisplayName(): string { + return $this->l10nFactory->get(Application::APP_ID)->t('View'); + } + + #[\Override] + public function getHint(): ?string { + return null; + } + + #[\Override] + public function getKindOf(): ?CorePermission { + return null; + } +} diff --git a/core/Resource/Permission/SharePermission.php b/core/Resource/Permission/SharePermission.php new file mode 100644 index 0000000000000..5f7e0d2e5453b --- /dev/null +++ b/core/Resource/Permission/SharePermission.php @@ -0,0 +1,42 @@ +value; + } + + #[\Override] + public function getDisplayName(): string { + return $this->l10nFactory->get(Application::APP_ID)->t('Share'); + } + + #[\Override] + public function getHint(): ?string { + return null; + } + + #[\Override] + public function getKindOf(): ?CorePermission { + return null; + } +} diff --git a/core/Resource/Permission/UpdatePermission.php b/core/Resource/Permission/UpdatePermission.php new file mode 100644 index 0000000000000..e2229475daea3 --- /dev/null +++ b/core/Resource/Permission/UpdatePermission.php @@ -0,0 +1,42 @@ +value; + } + + #[\Override] + public function getDisplayName(): string { + return $this->l10nFactory->get(Application::APP_ID)->t('Edit'); + } + + #[\Override] + public function getHint(): ?string { + return null; + } + + #[\Override] + public function getKindOf(): ?CorePermission { + return null; + } +} diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 2d74f891b01fa..65be1554048ea 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -18,6 +18,17 @@ 'NCU\\Config\\Lexicon\\Preset' => $baseDir . '/lib/unstable/Config/Lexicon/Preset.php', 'NCU\\Config\\ValueType' => $baseDir . '/lib/unstable/Config/ValueType.php', 'NCU\\Federation\\ISignedCloudFederationProvider' => $baseDir . '/lib/unstable/Federation/ISignedCloudFederationProvider.php', + 'NCU\\Resource\\IResourceType' => $baseDir . '/lib/unstable/Resource/IResourceType.php', + 'NCU\\Resource\\Permission\\APermission' => $baseDir . '/lib/unstable/Resource/Permission/APermission.php', + 'NCU\\Resource\\Permission\\AccessEntry' => $baseDir . '/lib/unstable/Resource/Permission/AccessEntry.php', + 'NCU\\Resource\\Permission\\CorePermission' => $baseDir . '/lib/unstable/Resource/Permission/CorePermission.php', + 'NCU\\Resource\\Permission\\EffectivePermissions' => $baseDir . '/lib/unstable/Resource/Permission/EffectivePermissions.php', + 'NCU\\Resource\\Permission\\IPermission' => $baseDir . '/lib/unstable/Resource/Permission/IPermission.php', + 'NCU\\Resource\\Permission\\IPermissionManager' => $baseDir . '/lib/unstable/Resource/Permission/IPermissionManager.php', + 'NCU\\Resource\\Permission\\IRestrictionProvider' => $baseDir . '/lib/unstable/Resource/Permission/IRestrictionProvider.php', + 'NCU\\Resource\\Permission\\ResourceAccess' => $baseDir . '/lib/unstable/Resource/Permission/ResourceAccess.php', + 'NCU\\Resource\\Permission\\Restriction' => $baseDir . '/lib/unstable/Resource/Permission/Restriction.php', + 'NCU\\Resource\\ResourceReference' => $baseDir . '/lib/unstable/Resource/ResourceReference.php', 'NCU\\Search\\AccountScopedSearchResult' => $baseDir . '/lib/unstable/Search/AccountScopedSearchResult.php', 'NCU\\Search\\Exceptions\\AccountUnavailableException' => $baseDir . '/lib/unstable/Search/Exceptions/AccountUnavailableException.php', 'NCU\\Search\\Exceptions\\SearchTruncatedException' => $baseDir . '/lib/unstable/Search/Exceptions/SearchTruncatedException.php', @@ -1772,6 +1783,12 @@ 'OC\\Core\\Migrations\\Version35000Date20260527162338' => $baseDir . '/core/Migrations/Version35000Date20260527162338.php', 'OC\\Core\\Migrations\\Version36000Date20261028100000' => $baseDir . '/core/Migrations/Version36000Date20261028100000.php', 'OC\\Core\\Notification\\CoreNotifier' => $baseDir . '/core/Notification/CoreNotifier.php', + 'OC\\Core\\Resource\\Permission\\CreatePermission' => $baseDir . '/core/Resource/Permission/CreatePermission.php', + 'OC\\Core\\Resource\\Permission\\DeletePermission' => $baseDir . '/core/Resource/Permission/DeletePermission.php', + 'OC\\Core\\Resource\\Permission\\DownloadPermission' => $baseDir . '/core/Resource/Permission/DownloadPermission.php', + 'OC\\Core\\Resource\\Permission\\ReadPermission' => $baseDir . '/core/Resource/Permission/ReadPermission.php', + 'OC\\Core\\Resource\\Permission\\SharePermission' => $baseDir . '/core/Resource/Permission/SharePermission.php', + 'OC\\Core\\Resource\\Permission\\UpdatePermission' => $baseDir . '/core/Resource/Permission/UpdatePermission.php', 'OC\\Core\\ResponseDefinitions' => $baseDir . '/core/ResponseDefinitions.php', 'OC\\Core\\Service\\CronService' => $baseDir . '/core/Service/CronService.php', 'OC\\Core\\Service\\LoginFlowV2Service' => $baseDir . '/core/Service/LoginFlowV2Service.php', @@ -2299,6 +2316,7 @@ 'OC\\Repair\\RepairLogoDimension' => $baseDir . '/lib/private/Repair/RepairLogoDimension.php', 'OC\\Repair\\RepairMimeTypes' => $baseDir . '/lib/private/Repair/RepairMimeTypes.php', 'OC\\Repair\\RepairSanitizeSystemTags' => $baseDir . '/lib/private/Repair/RepairSanitizeSystemTags.php', + 'OC\\Resource\\Permission\\PermissionManager' => $baseDir . '/lib/private/Resource/Permission/PermissionManager.php', 'OC\\RichObjectStrings\\RichTextFormatter' => $baseDir . '/lib/private/RichObjectStrings/RichTextFormatter.php', 'OC\\RichObjectStrings\\Validator' => $baseDir . '/lib/private/RichObjectStrings/Validator.php', 'OC\\Route\\CachingRouter' => $baseDir . '/lib/private/Route/CachingRouter.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 092b81d4a2314..46d0acf6ab136 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -59,6 +59,17 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'NCU\\Config\\Lexicon\\Preset' => __DIR__ . '/../../..' . '/lib/unstable/Config/Lexicon/Preset.php', 'NCU\\Config\\ValueType' => __DIR__ . '/../../..' . '/lib/unstable/Config/ValueType.php', 'NCU\\Federation\\ISignedCloudFederationProvider' => __DIR__ . '/../../..' . '/lib/unstable/Federation/ISignedCloudFederationProvider.php', + 'NCU\\Resource\\IResourceType' => __DIR__ . '/../../..' . '/lib/unstable/Resource/IResourceType.php', + 'NCU\\Resource\\Permission\\APermission' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/APermission.php', + 'NCU\\Resource\\Permission\\AccessEntry' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/AccessEntry.php', + 'NCU\\Resource\\Permission\\CorePermission' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/CorePermission.php', + 'NCU\\Resource\\Permission\\EffectivePermissions' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/EffectivePermissions.php', + 'NCU\\Resource\\Permission\\IPermission' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/IPermission.php', + 'NCU\\Resource\\Permission\\IPermissionManager' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/IPermissionManager.php', + 'NCU\\Resource\\Permission\\IRestrictionProvider' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/IRestrictionProvider.php', + 'NCU\\Resource\\Permission\\ResourceAccess' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/ResourceAccess.php', + 'NCU\\Resource\\Permission\\Restriction' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/Restriction.php', + 'NCU\\Resource\\ResourceReference' => __DIR__ . '/../../..' . '/lib/unstable/Resource/ResourceReference.php', 'NCU\\Search\\AccountScopedSearchResult' => __DIR__ . '/../../..' . '/lib/unstable/Search/AccountScopedSearchResult.php', 'NCU\\Search\\Exceptions\\AccountUnavailableException' => __DIR__ . '/../../..' . '/lib/unstable/Search/Exceptions/AccountUnavailableException.php', 'NCU\\Search\\Exceptions\\SearchTruncatedException' => __DIR__ . '/../../..' . '/lib/unstable/Search/Exceptions/SearchTruncatedException.php', @@ -1813,6 +1824,12 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Migrations\\Version35000Date20260527162338' => __DIR__ . '/../../..' . '/core/Migrations/Version35000Date20260527162338.php', 'OC\\Core\\Migrations\\Version36000Date20261028100000' => __DIR__ . '/../../..' . '/core/Migrations/Version36000Date20261028100000.php', 'OC\\Core\\Notification\\CoreNotifier' => __DIR__ . '/../../..' . '/core/Notification/CoreNotifier.php', + 'OC\\Core\\Resource\\Permission\\CreatePermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/CreatePermission.php', + 'OC\\Core\\Resource\\Permission\\DeletePermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/DeletePermission.php', + 'OC\\Core\\Resource\\Permission\\DownloadPermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/DownloadPermission.php', + 'OC\\Core\\Resource\\Permission\\ReadPermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/ReadPermission.php', + 'OC\\Core\\Resource\\Permission\\SharePermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/SharePermission.php', + 'OC\\Core\\Resource\\Permission\\UpdatePermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/UpdatePermission.php', 'OC\\Core\\ResponseDefinitions' => __DIR__ . '/../../..' . '/core/ResponseDefinitions.php', 'OC\\Core\\Service\\CronService' => __DIR__ . '/../../..' . '/core/Service/CronService.php', 'OC\\Core\\Service\\LoginFlowV2Service' => __DIR__ . '/../../..' . '/core/Service/LoginFlowV2Service.php', @@ -2340,6 +2357,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Repair\\RepairLogoDimension' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairLogoDimension.php', 'OC\\Repair\\RepairMimeTypes' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairMimeTypes.php', 'OC\\Repair\\RepairSanitizeSystemTags' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairSanitizeSystemTags.php', + 'OC\\Resource\\Permission\\PermissionManager' => __DIR__ . '/../../..' . '/lib/private/Resource/Permission/PermissionManager.php', 'OC\\RichObjectStrings\\RichTextFormatter' => __DIR__ . '/../../..' . '/lib/private/RichObjectStrings/RichTextFormatter.php', 'OC\\RichObjectStrings\\Validator' => __DIR__ . '/../../..' . '/lib/private/RichObjectStrings/Validator.php', 'OC\\Route\\CachingRouter' => __DIR__ . '/../../..' . '/lib/private/Route/CachingRouter.php', diff --git a/lib/private/AppFramework/Bootstrap/RegistrationContext.php b/lib/private/AppFramework/Bootstrap/RegistrationContext.php index f86890441243b..ab8f9c608e752 100644 --- a/lib/private/AppFramework/Bootstrap/RegistrationContext.php +++ b/lib/private/AppFramework/Bootstrap/RegistrationContext.php @@ -10,6 +10,9 @@ namespace OC\AppFramework\Bootstrap; use Closure; +use NCU\Resource\IResourceType; +use NCU\Resource\Permission\IPermission; +use NCU\Resource\Permission\IRestrictionProvider; use NCU\Search\IAccountScopedSearchProvider; use OC\AppFramework\DependencyInjection\DIContainer; use OC\Support\CrashReport\Registry; @@ -178,6 +181,15 @@ class RegistrationContext { /** @var class-string|null */ private ?string $globalScaleService = null; + /** @var list> */ + private array $resourceTypes = []; + + /** @var list> */ + private array $resourcePermissions = []; + + /** @var list> */ + private array $resourceRestrictionProviders = []; + public function __construct( private LoggerInterface $logger, ) { @@ -525,6 +537,21 @@ public function registerGlobalScaleService(string $globalScaleServiceClass): voi $globalScaleServiceClass ); } + + #[\Override] + public function registerResourceType(string $class): void { + $this->context->registerResourceType($this->appId, $class); + } + + #[\Override] + public function registerResourcePermission(string $class): void { + $this->context->registerResourcePermission($this->appId, $class); + } + + #[\Override] + public function registerResourceRestrictionProvider(string $class): void { + $this->context->registerResourceRestrictionProvider($this->appId, $class); + } }; } @@ -761,6 +788,27 @@ public function registerGlobalScaleService(string $class): void { $this->globalScaleService = $class; } + /** + * @param class-string $class + */ + public function registerResourceType(string $appId, string $class): void { + $this->resourceTypes[] = new ServiceRegistration($appId, $class); + } + + /** + * @param class-string $class + */ + public function registerResourcePermission(string $appId, string $class): void { + $this->resourcePermissions[] = new ServiceRegistration($appId, $class); + } + + /** + * @param class-string $class + */ + public function registerResourceRestrictionProvider(string $appId, string $class): void { + $this->resourceRestrictionProviders[] = new ServiceRegistration($appId, $class); + } + /** * @param App[] $apps */ @@ -1174,4 +1222,25 @@ public function getConfigLexicon(string $appId): ?ILexicon { public function getGlobalScaleService(): ?string { return $this->globalScaleService; } + + /** + * @return list> + */ + public function getResourceTypes(): array { + return $this->resourceTypes; + } + + /** + * @return list> + */ + public function getResourcePermissions(): array { + return $this->resourcePermissions; + } + + /** + * @return list> + */ + public function getResourceRestrictionProviders(): array { + return $this->resourceRestrictionProviders; + } } diff --git a/lib/private/Resource/Permission/PermissionManager.php b/lib/private/Resource/Permission/PermissionManager.php new file mode 100644 index 0000000000000..1194639e26d4b --- /dev/null +++ b/lib/private/Resource/Permission/PermissionManager.php @@ -0,0 +1,338 @@ + */ + private ?array $permissions = null; + + /** @var array> IDs of the permissions that are a kind of the core permission used as key */ + private array $kinds = []; + + /** @var ?array */ + private ?array $resourceTypes = null; + + /** @var ?list */ + private ?array $restrictionProviders = null; + + /** @var array>> results by actor key, resource key and hash of the access entries */ + private array $cache = []; + + public function __construct( + private readonly Coordinator $coordinator, + private readonly ContainerInterface $container, + private readonly LoggerInterface $logger, + ) { + } + + #[\Override] + public function getPermissions(): array { + if ($this->permissions !== null) { + return $this->permissions; + } + + $context = $this->coordinator->getRegistrationContext(); + if ($context === null) { + // Not cached: nothing is registered yet this early in boot. + return []; + } + + $permissions = []; + $kinds = []; + foreach ($context->getResourcePermissions() as $registration) { + $permission = $this->instantiate($registration, IPermission::class); + if ($permission === null) { + continue; + } + + $id = $permission->getId(); + if (!$this->isValidId($id, $registration->getAppId()) || isset($permissions[$id])) { + $this->logger->error('The permission {id} of the app {app} has an invalid or already registered ID', ['id' => $id, 'app' => $registration->getAppId()]); + continue; + } + + $kindOf = $permission->getKindOf(); + if (($kindOf === null) !== (CorePermission::tryFrom($id) !== null)) { + $this->logger->error('The permission {id} has to be a kind of a core permission, and a core permission cannot be a kind of another one', ['id' => $id]); + continue; + } + + $permissions[$id] = $permission; + if ($kindOf !== null) { + $kinds[$kindOf->value][] = $id; + } + } + + $this->permissions = $permissions; + $this->kinds = $kinds; + + return $permissions; + } + + #[\Override] + public function getPermission(string $id): ?IPermission { + return $this->getPermissions()[$id] ?? null; + } + + #[\Override] + public function getResourceTypes(): array { + if ($this->resourceTypes !== null) { + return $this->resourceTypes; + } + + $context = $this->coordinator->getRegistrationContext(); + if ($context === null) { + return []; + } + + $resourceTypes = []; + foreach ($context->getResourceTypes() as $registration) { + $resourceType = $this->instantiate($registration, IResourceType::class); + if ($resourceType === null) { + continue; + } + + $id = $resourceType->getId(); + if (!$this->isValidId($id, $registration->getAppId()) || isset($resourceTypes[$id])) { + $this->logger->error('The resource type {id} of the app {app} has an invalid or already registered ID', ['id' => $id, 'app' => $registration->getAppId()]); + continue; + } + + $resourceTypes[$id] = $resourceType; + } + + $this->resourceTypes = $resourceTypes; + + return $resourceTypes; + } + + #[\Override] + public function getResourceType(string $id): ?IResourceType { + return $this->getResourceTypes()[$id] ?? null; + } + + #[\Override] + public function check(Actor $actor, ResourceReference $resource, array $entries = []): EffectivePermissions { + return $this->checkAll($actor, [new ResourceAccess($resource, $entries)])[$resource->key()]; + } + + #[\Override] + public function checkAll(Actor $actor, array $accesses): array { + $actorKey = $actor->key(); + $results = []; + /** @var array $misses */ + $misses = []; + $hashes = []; + foreach ($accesses as $access) { + $resourceKey = $access->resource->key(); + if ($this->getResourceType($access->resource->type) === null) { + throw new InvalidArgumentException('The resource type ' . $access->resource->type . ' is not registered'); + } + if (isset($results[$resourceKey]) || isset($misses[$resourceKey])) { + throw new InvalidArgumentException('The resource ' . $resourceKey . ' is passed more than once'); + } + + $hashes[$resourceKey] = $this->hashEntries($access->entries); + $cached = $this->cache[$actorKey][$resourceKey][$hashes[$resourceKey]] ?? null; + if ($cached !== null) { + $results[$resourceKey] = $cached; + } else { + $misses[$resourceKey] = $access; + } + } + + if ($misses === []) { + return $results; + } + + $restrictions = $this->collectRestrictions($actor, array_values($misses)); + foreach ($misses as $resourceKey => $access) { + if ($restrictions === null) { + // Not cached: the provider that failed may succeed on the next check. + $results[$resourceKey] = new EffectivePermissions([]); + continue; + } + + $result = $this->evaluate($access, $restrictions[$resourceKey] ?? []); + $this->cache[$actorKey][$resourceKey][$hashes[$resourceKey]] = $result; + $results[$resourceKey] = $result; + } + + return $results; + } + + /** + * Get the restricted permission IDs by resource key and recipient, + * the empty recipient is the resource as a whole. + * + * @param list $accesses + * @return ?array>> null if a provider failed + */ + private function collectRestrictions(Actor $actor, array $accesses): ?array { + $restrictions = []; + foreach ($this->getRestrictionProviders() as $provider) { + try { + foreach ($provider->getRestrictions($actor, $accesses) as $restriction) { + $recipient = $restriction->recipientKind !== null && $restriction->recipientId !== null + ? $this->recipientKey($restriction->recipientKind, $restriction->recipientId) + : ''; + foreach ($this->expand($restriction->permissionIds) as $permissionId) { + $restrictions[$restriction->resource->key()][$recipient][$permissionId] = true; + } + } + } catch (Throwable $e) { + $this->logger->error('The restriction provider {class} failed, all permissions are restricted', ['class' => $provider::class, 'exception' => $e]); + return null; + } + } + + return $restrictions; + } + + /** + * @param array> $restrictions restricted permission IDs by recipient + */ + private function evaluate(ResourceAccess $access, array $restrictions): EffectivePermissions { + $permissions = $this->getPermissions(); + $applicable = []; + foreach ($this->getResourceTypes()[$access->resource->type]->getPermissionIds() as $permissionId) { + if (isset($permissions[$permissionId])) { + $applicable[$permissionId] = true; + } + } + + if ($access->entries === []) { + $allowed = $applicable; + } else { + $allowed = []; + foreach ($access->entries as $entry) { + $granted = array_fill_keys($entry->permissionIds, true); + $notApplicable = array_diff_key($granted, $applicable); + if ($notApplicable !== []) { + $this->logger->warning('Ignoring permissions that do not apply to the resource type {type}: {permissions}', ['type' => $access->resource->type, 'permissions' => implode(', ', array_keys($notApplicable))]); + } + + $allowed += array_diff_key( + array_intersect_key($granted, $applicable), + $restrictions[$this->recipientKey($entry->recipientKind, $entry->recipientId)] ?? [], + ); + } + } + + return new EffectivePermissions(array_keys(array_diff_key($allowed, $restrictions[''] ?? []))); + } + + /** + * Add the permissions that are a kind of one of the permissions. + * + * @param list $permissionIds + * @return list + */ + private function expand(array $permissionIds): array { + $this->getPermissions(); + $expanded = $permissionIds; + foreach ($permissionIds as $permissionId) { + array_push($expanded, ...($this->kinds[$permissionId] ?? [])); + } + + return $expanded; + } + + /** + * @return list + */ + private function getRestrictionProviders(): array { + if ($this->restrictionProviders !== null) { + return $this->restrictionProviders; + } + + $context = $this->coordinator->getRegistrationContext(); + if ($context === null) { + return []; + } + + $providers = []; + foreach ($context->getResourceRestrictionProviders() as $registration) { + $provider = $this->instantiate($registration, IRestrictionProvider::class); + if ($provider !== null) { + $providers[] = $provider; + } + } + + $this->restrictionProviders = $providers; + + return $providers; + } + + /** + * @template T of object + * @param ServiceRegistration $registration + * @param class-string $interface + * @return ?T + */ + private function instantiate(ServiceRegistration $registration, string $interface): ?object { + try { + $instance = $this->container->get($registration->getService()); + } catch (Throwable $e) { + $this->logger->error('Could not load {class} of the app {app}', ['class' => $registration->getService(), 'app' => $registration->getAppId(), 'exception' => $e]); + return null; + } + + if (!$instance instanceof $interface) { + $this->logger->error('{class} of the app {app} does not implement {interface}', ['class' => $registration->getService(), 'app' => $registration->getAppId(), 'interface' => $interface]); + return null; + } + + return $instance; + } + + private function isValidId(string $id, string $appId): bool { + $prefix = $appId . ':'; + return str_starts_with($id, $prefix) + && preg_match(self::NAME_PATTERN, substr($id, strlen($prefix))) === 1; + } + + private function recipientKey(string $kind, string $id): string { + return $kind . '/' . $id; + } + + /** + * @param list $entries + */ + private function hashEntries(array $entries): string { + if ($entries === []) { + return ''; + } + + return hash('xxh128', json_encode( + array_map(static fn (AccessEntry $entry): array => [$entry->recipientKind, $entry->recipientId, $entry->permissionIds], $entries), + JSON_THROW_ON_ERROR, + )); + } +} diff --git a/lib/private/Server.php b/lib/private/Server.php index 5c81a2188805c..584347208e58e 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1117,6 +1117,8 @@ public function runInitialRegistrations(): void { $this->registerAlias(\NCU\Search\IAccountScopedSearchProviderRegistry::class, AccountScopedSearchProviderRegistry::class); + $this->registerAlias(\NCU\Resource\Permission\IPermissionManager::class, \OC\Resource\Permission\PermissionManager::class); + $this->registerService(IGlobalScaleService::class, static function (ContainerInterface $c): IGlobalScaleService { /** @var Coordinator $coordinator */ $coordinator = $c->get(Coordinator::class); diff --git a/lib/public/AppFramework/Bootstrap/IRegistrationContext.php b/lib/public/AppFramework/Bootstrap/IRegistrationContext.php index 598bbb5fdae73..f9ac0741cc3b6 100644 --- a/lib/public/AppFramework/Bootstrap/IRegistrationContext.php +++ b/lib/public/AppFramework/Bootstrap/IRegistrationContext.php @@ -486,4 +486,34 @@ public function registerConfigLexicon(string $configLexiconClass): void; * @since 34.0.3 */ public function registerGlobalScaleService(string $globalScaleServiceClass): void; + + /** + * Register a type of resource the app owns + * + * `\NCU\Resource\IResourceType` is currently experimental and may still change. + * + * @psalm-param class-string<\NCU\Resource\IResourceType> $class + * @since 36.0.0 + */ + public function registerResourceType(string $class): void; + + /** + * Register a permission that can be allowed or restricted on resources + * + * `\NCU\Resource\Permission\IPermission` is currently experimental and may still change. + * + * @psalm-param class-string<\NCU\Resource\Permission\IPermission> $class + * @since 36.0.0 + */ + public function registerResourcePermission(string $class): void; + + /** + * Register a provider that restricts permissions on resources of other apps + * + * `\NCU\Resource\Permission\IRestrictionProvider` is currently experimental and may still change. + * + * @psalm-param class-string<\NCU\Resource\Permission\IRestrictionProvider> $class + * @since 36.0.0 + */ + public function registerResourceRestrictionProvider(string $class): void; } diff --git a/lib/unstable/Resource/IResourceType.php b/lib/unstable/Resource/IResourceType.php new file mode 100644 index 0000000000000..614cbb3d2fab4 --- /dev/null +++ b/lib/unstable/Resource/IResourceType.php @@ -0,0 +1,46 @@ + + * @experimental 36.0.0 + */ + public function getPermissionIds(): array; +} diff --git a/lib/unstable/Resource/Permission/APermission.php b/lib/unstable/Resource/Permission/APermission.php new file mode 100644 index 0000000000000..446b093a1e05a --- /dev/null +++ b/lib/unstable/Resource/Permission/APermission.php @@ -0,0 +1,34 @@ + $permissionIds IDs of the permissions granted to the recipient + * @experimental 36.0.0 + */ + public function __construct( + public string $recipientKind, + public string $recipientId, + public array $permissionIds, + ) { + } +} diff --git a/lib/unstable/Resource/Permission/CorePermission.php b/lib/unstable/Resource/Permission/CorePermission.php new file mode 100644 index 0000000000000..76660bee65615 --- /dev/null +++ b/lib/unstable/Resource/Permission/CorePermission.php @@ -0,0 +1,53 @@ + $allowedPermissionIds + * @experimental 36.0.0 + */ + public function __construct( + private array $allowedPermissionIds, + ) { + } + + /** + * @experimental 36.0.0 + */ + public function isAllowed(string $permissionId): bool { + return in_array($permissionId, $this->allowedPermissionIds, true); + } + + /** + * @return list + * @experimental 36.0.0 + */ + public function getAllowedPermissionIds(): array { + return $this->allowedPermissionIds; + } +} diff --git a/lib/unstable/Resource/Permission/IPermission.php b/lib/unstable/Resource/Permission/IPermission.php new file mode 100644 index 0000000000000..db93d0aca68ff --- /dev/null +++ b/lib/unstable/Resource/Permission/IPermission.php @@ -0,0 +1,59 @@ + keyed by ID + * @experimental 36.0.0 + */ + public function getPermissions(): array; + + /** + * @experimental 36.0.0 + */ + public function getPermission(string $id): ?IPermission; + + /** + * @return array keyed by ID + * @experimental 36.0.0 + */ + public function getResourceTypes(): array; + + /** + * @experimental 36.0.0 + */ + public function getResourceType(string $id): ?IResourceType; + + /** + * Get the permissions of the actor on the resource that are not restricted. + * + * With access entries the result is what is left of the permissions granted by them. + * Without access entries the result is what is left of all permissions that apply to + * the resource type, the caller has to intersect it with the access it grants itself. + * + * @param list $entries + * @throws \InvalidArgumentException if the resource type is not registered + * @experimental 36.0.0 + */ + public function check(Actor $actor, ResourceReference $resource, array $entries = []): EffectivePermissions; + + /** + * Same as {@see self::check} for multiple resources at once, the resources can be of different types. + * + * @param list $accesses + * @return array keyed by {@see ResourceReference::key} + * @throws \InvalidArgumentException if a resource type is not registered or a resource is passed more than once + * @experimental 36.0.0 + */ + public function checkAll(Actor $actor, array $accesses): array; +} diff --git a/lib/unstable/Resource/Permission/IRestrictionProvider.php b/lib/unstable/Resource/Permission/IRestrictionProvider.php new file mode 100644 index 0000000000000..db52e1da159ce --- /dev/null +++ b/lib/unstable/Resource/Permission/IRestrictionProvider.php @@ -0,0 +1,36 @@ + $accesses + * @return iterable + * @experimental 36.0.0 + */ + public function getRestrictions(Actor $actor, array $accesses): iterable; +} diff --git a/lib/unstable/Resource/Permission/ResourceAccess.php b/lib/unstable/Resource/Permission/ResourceAccess.php new file mode 100644 index 0000000000000..be9ff5153b640 --- /dev/null +++ b/lib/unstable/Resource/Permission/ResourceAccess.php @@ -0,0 +1,32 @@ + $entries Without entries restrictions apply to the resource as a whole + * @experimental 36.0.0 + */ + public function __construct( + public ResourceReference $resource, + public array $entries = [], + ) { + } +} diff --git a/lib/unstable/Resource/Permission/Restriction.php b/lib/unstable/Resource/Permission/Restriction.php new file mode 100644 index 0000000000000..d4f3ae00e9afb --- /dev/null +++ b/lib/unstable/Resource/Permission/Restriction.php @@ -0,0 +1,60 @@ + $permissionIds + * @param ?non-empty-string $recipientKind + * @param ?non-empty-string $recipientId + */ + private function __construct( + public ResourceReference $resource, + public array $permissionIds, + public ?string $recipientKind, + public ?string $recipientId, + ) { + } + + /** + * Restrict the permissions on the resource, however the actor has access to it. + * + * @param list $permissionIds + * @experimental 36.0.0 + */ + public static function forResource(ResourceReference $resource, array $permissionIds): self { + return new self($resource, $permissionIds, null, null); + } + + /** + * Restrict the permissions only for the access the actor has through the recipient, + * see {@see AccessEntry}. + * + * @param non-empty-string $recipientKind + * @param non-empty-string $recipientId + * @param list $permissionIds + * @experimental 36.0.0 + */ + public static function forRecipient(ResourceReference $resource, string $recipientKind, string $recipientId, array $permissionIds): self { + return new self($resource, $permissionIds, $recipientKind, $recipientId); + } +} diff --git a/lib/unstable/Resource/ResourceReference.php b/lib/unstable/Resource/ResourceReference.php new file mode 100644 index 0000000000000..35ec3406e81ad --- /dev/null +++ b/lib/unstable/Resource/ResourceReference.php @@ -0,0 +1,47 @@ +type . '/' . $this->id; + } +} diff --git a/tests/lib/Resource/Permission/PermissionManagerTest.php b/tests/lib/Resource/Permission/PermissionManagerTest.php new file mode 100644 index 0000000000000..ac6a5847dc17c --- /dev/null +++ b/tests/lib/Resource/Permission/PermissionManagerTest.php @@ -0,0 +1,470 @@ + */ + private array $instances = []; + + protected function setUp(): void { + parent::setUp(); + + $this->logger = $this->createMock(LoggerInterface::class); + $this->context = new RegistrationContext($this->createMock(LoggerInterface::class)); + + $coordinator = $this->createMock(Coordinator::class); + $coordinator->method('getRegistrationContext')->willReturn($this->context); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willReturnCallback(function (string $class): object { + return $this->instances[$class] ?? throw new class('Not found: ' . $class) extends \Exception implements NotFoundExceptionInterface { + }; + }); + + $this->manager = new PermissionManager($coordinator, $container, $this->logger); + $this->alice = Actor::of(Principal::user('alice')); + } + + private function registerPermission(string $appId, string $id, ?CorePermission $kindOf): void { + $this->instances[$id] = new class($id, $kindOf) implements IPermission { + public function __construct( + private readonly string $id, + private readonly ?CorePermission $kindOf, + ) { + } + + public function getId(): string { + return $this->id; + } + + public function getDisplayName(): string { + return 'Permission ' . $this->id; + } + + public function getHint(): ?string { + return null; + } + + public function getKindOf(): ?CorePermission { + return $this->kindOf; + } + }; + $this->context->registerResourcePermission($appId, $id); + } + + private function registerCorePermissions(): void { + foreach (CorePermission::cases() as $permission) { + $this->registerPermission('core', $permission->value, null); + } + $this->registerPermission('core', CorePermission::DOWNLOAD, CorePermission::Read); + } + + /** + * @param list $permissionIds + */ + private function registerResourceType(string $appId, string $id, array $permissionIds): void { + $this->instances[$id] = new class($id, $permissionIds) implements IResourceType { + public function __construct( + private readonly string $id, + private readonly array $permissionIds, + ) { + } + + public function getId(): string { + return $this->id; + } + + public function getDisplayName(): string { + return 'Resource type ' . $this->id; + } + + public function getPermissionIds(): array { + return $this->permissionIds; + } + }; + $this->context->registerResourceType($appId, $id); + } + + /** + * Core permissions, the permission tables:export as a kind of share, and the resource types + * tables:table (read, download, update, share, export) and tables:row (read, update, delete). + */ + private function registerTables(): void { + $this->registerCorePermissions(); + $this->registerPermission('tables', self::EXPORT, CorePermission::Share); + $this->registerResourceType('tables', self::TABLE, [ + CorePermission::Read->value, + CorePermission::DOWNLOAD, + CorePermission::Update->value, + CorePermission::Share->value, + self::EXPORT, + ]); + $this->registerResourceType('tables', self::ROW, [ + CorePermission::Read->value, + CorePermission::Update->value, + CorePermission::Delete->value, + ]); + } + + /** + * @param \Closure(Actor, list): iterable $restrictions + * @return object{calls: list>}&IRestrictionProvider + */ + private function registerRestrictionProvider(\Closure $restrictions): IRestrictionProvider { + $provider = new class($restrictions) implements IRestrictionProvider { + /** @var list> */ + public array $calls = []; + + public function __construct( + private readonly \Closure $restrictions, + ) { + } + + public function getRestrictions(Actor $actor, array $accesses): iterable { + $this->calls[] = $accesses; + return ($this->restrictions)($actor, $accesses); + } + }; + $class = 'provider' . count($this->instances); + $this->instances[$class] = $provider; + $this->context->registerResourceRestrictionProvider('governance', $class); + + return $provider; + } + + /** + * @param list $expected + */ + private function assertAllowed(array $expected, ResourceReference $resource, array $entries = []): void { + $allowed = $this->manager->check($this->alice, $resource, $entries)->getAllowedPermissionIds(); + sort($allowed); + sort($expected); + $this->assertSame($expected, $allowed); + } + + public function testCorePermissionsAreRegistered(): void { + $manager = Server::get(IPermissionManager::class); + + foreach (CorePermission::cases() as $permission) { + $this->assertNull($manager->getPermission($permission->value)?->getKindOf(), $permission->value); + $this->assertNotNull($manager->getPermission($permission->value), $permission->value); + } + $this->assertSame(CorePermission::Read, $manager->getPermission(CorePermission::DOWNLOAD)?->getKindOf()); + } + + public function testGetPermissions(): void { + $this->registerTables(); + + $this->assertSame( + [...array_column(CorePermission::cases(), 'value'), CorePermission::DOWNLOAD, self::EXPORT], + array_keys($this->manager->getPermissions()), + ); + $this->assertSame(CorePermission::Share, $this->manager->getPermission(self::EXPORT)?->getKindOf()); + $this->assertNull($this->manager->getPermission('tables:unknown')); + } + + public function testAppPermissionBaseClassRequiresKind(): void { + $permission = new class extends APermission { + public function getId(): string { + return 'tables:export'; + } + + public function getDisplayName(): string { + return 'Export'; + } + + public function getKindOf(): CorePermission { + return CorePermission::Share; + } + }; + + $this->assertNull($permission->getHint()); + $this->assertSame(CorePermission::Share, $permission->getKindOf()); + } + + public static function dataInvalidPermissions(): array { + return [ + 'foreign app prefix' => ['tables', 'forms:results', CorePermission::Read], + 'core prefix from an app' => ['tables', 'core:print', CorePermission::Read], + 'missing name' => ['tables', 'tables:', CorePermission::Read], + 'invalid name' => ['tables', 'tables:Export All', CorePermission::Read], + 'app permission without kind' => ['tables', 'tables:export', null], + 'core permission with kind' => ['core', 'core:update', CorePermission::Read], + 'unknown core permission without kind' => ['core', 'core:print', null], + ]; + } + + #[DataProvider('dataInvalidPermissions')] + public function testInvalidPermissionIsSkipped(string $appId, string $id, ?CorePermission $kindOf): void { + $this->logger->expects($this->once())->method('error'); + $this->registerPermission($appId, $id, $kindOf); + + $this->assertSame([], $this->manager->getPermissions()); + } + + public function testDuplicatePermissionIsSkipped(): void { + $this->registerPermission('tables', self::EXPORT, CorePermission::Share); + $first = $this->instances[self::EXPORT]; + $this->instances['duplicate'] = clone $first; + $this->context->registerResourcePermission('tables', 'duplicate'); + $this->logger->expects($this->once())->method('error'); + + $this->assertSame([self::EXPORT => $first], $this->manager->getPermissions()); + } + + public function testBrokenRegistrationsAreSkipped(): void { + $this->context->registerResourcePermission('tables', 'missing'); + $this->instances['wrong'] = new \stdClass(); + $this->context->registerResourcePermission('tables', 'wrong'); + $this->context->registerResourceType('tables', 'missing'); + $this->context->registerResourceRestrictionProvider('governance', 'wrong'); + $this->registerTables(); + $this->logger->expects($this->exactly(4))->method('error'); + + $this->assertCount(7, $this->manager->getPermissions()); + $this->assertCount(2, $this->manager->getResourceTypes()); + $this->assertAllowed([CorePermission::Read->value, CorePermission::Update->value, CorePermission::Delete->value], new ResourceReference(self::ROW, '1')); + } + + public function testGetResourceTypes(): void { + $this->registerTables(); + $this->registerResourceType('tables', 'forms:form', []); + $this->registerResourceType('tables', self::TABLE, []); + $this->logger->expects($this->exactly(2))->method('error'); + + $this->assertSame([self::TABLE, self::ROW], array_keys($this->manager->getResourceTypes())); + $this->assertSame('Resource type ' . self::ROW, $this->manager->getResourceType(self::ROW)?->getDisplayName()); + $this->assertNull($this->manager->getResourceType('forms:form')); + } + + public function testCheckUnregisteredResourceType(): void { + $this->registerTables(); + + $this->expectException(\InvalidArgumentException::class); + $this->manager->check($this->alice, new ResourceReference('forms:form', '1')); + } + + public function testCheckSameResourceTwice(): void { + $this->registerTables(); + + $this->expectException(\InvalidArgumentException::class); + $this->manager->checkAll($this->alice, [ + new ResourceAccess(new ResourceReference(self::TABLE, '1')), + new ResourceAccess(new ResourceReference(self::TABLE, '1', Principal::user('bob'))), + ]); + } + + public function testWithoutRestrictionsAllApplicablePermissionsAreAllowed(): void { + $this->registerTables(); + $this->registerResourceType('tables', 'tables:view', [CorePermission::Read->value, 'forms:results']); + + $this->assertAllowed( + [CorePermission::Read->value, CorePermission::DOWNLOAD, CorePermission::Update->value, CorePermission::Share->value, self::EXPORT], + new ResourceReference(self::TABLE, '1'), + ); + $this->assertAllowed([CorePermission::Read->value], new ResourceReference('tables:view', '1')); + } + + public function testEntriesAreMerged(): void { + $this->registerTables(); + $this->logger->expects($this->once())->method('warning'); + + $this->assertAllowed( + [CorePermission::Read->value, CorePermission::Update->value, CorePermission::Share->value], + new ResourceReference(self::TABLE, '1'), + [ + new AccessEntry(AccessEntry::RECIPIENT_GROUP, 'staff', [CorePermission::Read->value, CorePermission::Share->value]), + // Delete does not apply to tables + new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value, CorePermission::Update->value, CorePermission::Delete->value]), + ], + ); + } + + public function testResourceRestriction(): void { + $this->registerTables(); + $this->registerRestrictionProvider(static fn (Actor $actor, array $accesses): array => [ + Restriction::forResource($accesses[0]->resource, [CorePermission::Update->value]), + ]); + + $resource = new ResourceReference(self::TABLE, '1'); + $this->assertAllowed([CorePermission::Read->value, CorePermission::DOWNLOAD, CorePermission::Share->value, self::EXPORT], $resource); + $this->assertAllowed([CorePermission::Read->value], $resource, [ + new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value, CorePermission::Update->value]), + ]); + } + + public function testRestrictingCorePermissionRestrictsItsKinds(): void { + $this->registerTables(); + $this->registerRestrictionProvider(static fn (Actor $actor, array $accesses): array => [ + Restriction::forResource($accesses[0]->resource, [CorePermission::Read->value, CorePermission::Share->value, 'forms:unknown']), + ]); + + // Read, download as a kind of read, share and export as a kind of share are restricted + $this->assertAllowed([CorePermission::Update->value], new ResourceReference(self::TABLE, '1')); + } + + public function testRestrictingKindDoesNotRestrictCorePermission(): void { + $this->registerTables(); + $this->registerRestrictionProvider(static fn (Actor $actor, array $accesses): array => [ + Restriction::forResource($accesses[0]->resource, [CorePermission::DOWNLOAD]), + ]); + + $this->assertAllowed( + [CorePermission::Read->value, CorePermission::Update->value, CorePermission::Share->value, self::EXPORT], + new ResourceReference(self::TABLE, '1'), + ); + } + + /** + * Alice is a visitor of the team sales, which only allows to read. + * The table is shared with the team and with her directly, the direct share is not restricted. + */ + public function testRecipientRestrictionOnlyAppliesToEntriesOfTheRecipient(): void { + $this->registerTables(); + $onlyRead = array_values(array_filter( + array_column(CorePermission::cases(), 'value'), + static fn (string $id): bool => $id !== CorePermission::Read->value, + )); + $this->registerRestrictionProvider(static fn (Actor $actor, array $accesses): array => [ + Restriction::forRecipient($accesses[0]->resource, AccessEntry::RECIPIENT_TEAM, 'sales', $onlyRead), + ]); + $this->registerRestrictionProvider(static fn (Actor $actor, array $accesses): array => [ + Restriction::forResource($accesses[0]->resource, [CorePermission::DOWNLOAD]), + ]); + + $resource = new ResourceReference(self::TABLE, '1', Principal::user('bob')); + $team = new AccessEntry(AccessEntry::RECIPIENT_TEAM, 'sales', [CorePermission::Read->value, CorePermission::DOWNLOAD, CorePermission::Update->value, self::EXPORT]); + $direct = new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value, CorePermission::Update->value]); + + $this->assertAllowed([CorePermission::Read->value], $resource, [$team]); + $this->assertAllowed([CorePermission::Read->value, CorePermission::Update->value], $resource, [$team, $direct]); + // Without entries only restrictions of the resource as a whole apply + $this->assertAllowed([CorePermission::Read->value, CorePermission::Update->value, CorePermission::Share->value, self::EXPORT], $resource); + } + + public function testRestrictionsOfOtherResourcesAreIgnored(): void { + $this->registerTables(); + $this->registerRestrictionProvider(static fn (): array => [ + Restriction::forResource(new ResourceReference(self::ROW, '2'), [CorePermission::Read->value]), + ]); + + $this->assertAllowed( + [CorePermission::Read->value, CorePermission::Update->value, CorePermission::Delete->value], + new ResourceReference(self::ROW, '1'), + ); + } + + public function testFailingProviderRestrictsEverythingAndIsNotCached(): void { + $this->registerTables(); + $fail = true; + $this->registerRestrictionProvider(static function () use (&$fail): array { + if ($fail) { + throw new \RuntimeException('Label storage not available'); + } + return []; + }); + $this->logger->expects($this->once())->method('error'); + + $table = new ResourceReference(self::TABLE, '1'); + $row = new ResourceReference(self::ROW, '1'); + $results = $this->manager->checkAll($this->alice, [new ResourceAccess($table), new ResourceAccess($row)]); + $this->assertSame([], $results[$table->key()]->getAllowedPermissionIds()); + $this->assertSame([], $results[$row->key()]->getAllowedPermissionIds()); + $this->assertFalse($results[$row->key()]->isAllowed(CorePermission::Read->value)); + + $fail = false; + $this->assertAllowed([CorePermission::Read->value, CorePermission::Update->value, CorePermission::Delete->value], $row); + } + + public function testCheckAll(): void { + $this->registerTables(); + $this->registerRestrictionProvider(static function (Actor $actor, array $accesses): \Generator { + foreach ($accesses as $access) { + if ($access->resource->type === self::ROW && $access->resource->id !== '1') { + yield Restriction::forResource($access->resource, [CorePermission::Delete->value]); + } + } + }); + + $accesses = [ + new ResourceAccess(new ResourceReference(self::TABLE, '1')), + new ResourceAccess(new ResourceReference(self::ROW, '1')), + new ResourceAccess(new ResourceReference(self::ROW, '2'), [ + new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value, CorePermission::Delete->value]), + ]), + ]; + $results = $this->manager->checkAll($this->alice, $accesses); + + $this->assertSame(['tables:table/1', 'tables:row/1', 'tables:row/2'], array_keys($results)); + $this->assertTrue($results['tables:table/1']->isAllowed(self::EXPORT)); + $this->assertTrue($results['tables:row/1']->isAllowed(CorePermission::Delete->value)); + $this->assertSame([CorePermission::Read->value], $results['tables:row/2']->getAllowedPermissionIds()); + foreach ($accesses as $access) { + $this->assertEquals($results[$access->resource->key()], $this->manager->check($this->alice, $access->resource, $access->entries)); + } + } + + public function testResultsAreCachedPerActorResourceAndEntries(): void { + $this->registerTables(); + $provider = $this->registerRestrictionProvider(static fn (): array => []); + $row1 = new ResourceReference(self::ROW, '1'); + $row2 = new ResourceReference(self::ROW, '2'); + $entries = [new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value])]; + + $this->manager->check($this->alice, $row1); + $this->manager->check($this->alice, $row1); + $this->assertCount(1, $provider->calls); + + // Only the resource that was not checked yet is passed to the provider + $this->manager->checkAll($this->alice, [new ResourceAccess($row1), new ResourceAccess($row2)]); + $this->assertCount(2, $provider->calls); + $this->assertEquals([new ResourceAccess($row2)], $provider->calls[1]); + + $this->manager->check($this->alice, $row1, $entries); + $this->manager->check($this->alice, $row1, $entries); + $this->assertCount(3, $provider->calls); + + $this->manager->check(Actor::of(Principal::user('bob')), $row1); + $this->manager->check(Actor::of(Principal::bot('summarizer', 'assistant'))->onBehalfOf(Principal::user('alice')), $row1); + $this->assertCount(5, $provider->calls); + } +} From 20fb77be86d53b2385d3ed186f0c18f36bf4fb75 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 12 Nov 2026 23:40:17 +0100 Subject: [PATCH 2/4] fix: harden the resource permission check Results are kept in a capped cache that restriction providers clear when their restrictions change, so long-running processes neither keep stale results nor grow without limit. A restriction provider that cannot be loaded now restricts everything, like a provider that throws. The permissions that apply to a resource type are read once per check instead of once per resource. Assisted-by: ClaudeCode:claude-fable-5-2 Signed-off-by: Ferdinand Thiessen --- .../Resource/Permission/PermissionManager.php | 63 ++++++++++----- .../Permission/IPermissionManager.php | 11 +++ .../Permission/IRestrictionProvider.php | 2 + .../Permission/PermissionManagerTest.php | 81 ++++++++++++++++++- 4 files changed, 134 insertions(+), 23 deletions(-) diff --git a/lib/private/Resource/Permission/PermissionManager.php b/lib/private/Resource/Permission/PermissionManager.php index 1194639e26d4b..2471b0a16b000 100644 --- a/lib/private/Resource/Permission/PermissionManager.php +++ b/lib/private/Resource/Permission/PermissionManager.php @@ -22,6 +22,7 @@ use OC\AppFramework\Bootstrap\Coordinator; use OC\AppFramework\Bootstrap\ServiceRegistration; use OCP\Actor\Actor; +use OCP\Cache\CappedMemoryCache; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; use Throwable; @@ -41,14 +42,15 @@ final class PermissionManager implements IPermissionManager { /** @var ?list */ private ?array $restrictionProviders = null; - /** @var array>> results by actor key, resource key and hash of the access entries */ - private array $cache = []; + /** @var CappedMemoryCache results by actor key, resource key and hash of the access entries */ + private CappedMemoryCache $cache; public function __construct( private readonly Coordinator $coordinator, private readonly ContainerInterface $container, private readonly LoggerInterface $logger, ) { + $this->cache = new CappedMemoryCache(2048); } #[\Override] @@ -148,7 +150,7 @@ public function checkAll(Actor $actor, array $accesses): array { $results = []; /** @var array $misses */ $misses = []; - $hashes = []; + $cacheKeys = []; foreach ($accesses as $access) { $resourceKey = $access->resource->key(); if ($this->getResourceType($access->resource->type) === null) { @@ -158,8 +160,8 @@ public function checkAll(Actor $actor, array $accesses): array { throw new InvalidArgumentException('The resource ' . $resourceKey . ' is passed more than once'); } - $hashes[$resourceKey] = $this->hashEntries($access->entries); - $cached = $this->cache[$actorKey][$resourceKey][$hashes[$resourceKey]] ?? null; + $cacheKeys[$resourceKey] = $actorKey . "\0" . $resourceKey . "\0" . $this->hashEntries($access->entries); + $cached = $this->cache->get($cacheKeys[$resourceKey]); if ($cached !== null) { $results[$resourceKey] = $cached; } else { @@ -172,6 +174,7 @@ public function checkAll(Actor $actor, array $accesses): array { } $restrictions = $this->collectRestrictions($actor, array_values($misses)); + $applicable = []; foreach ($misses as $resourceKey => $access) { if ($restrictions === null) { // Not cached: the provider that failed may succeed on the next check. @@ -179,14 +182,20 @@ public function checkAll(Actor $actor, array $accesses): array { continue; } - $result = $this->evaluate($access, $restrictions[$resourceKey] ?? []); - $this->cache[$actorKey][$resourceKey][$hashes[$resourceKey]] = $result; + $applicable[$access->resource->type] ??= $this->getApplicablePermissionIds($access->resource->type); + $result = $this->evaluate($access, $applicable[$access->resource->type], $restrictions[$resourceKey] ?? []); + $this->cache->set($cacheKeys[$resourceKey], $result); $results[$resourceKey] = $result; } return $results; } + #[\Override] + public function clearCache(): void { + $this->cache->clear(); + } + /** * Get the restricted permission IDs by resource key and recipient, * the empty recipient is the resource as a whole. @@ -195,8 +204,13 @@ public function checkAll(Actor $actor, array $accesses): array { * @return ?array>> null if a provider failed */ private function collectRestrictions(Actor $actor, array $accesses): ?array { + $providers = $this->getRestrictionProviders(); + if ($providers === null) { + return null; + } + $restrictions = []; - foreach ($this->getRestrictionProviders() as $provider) { + foreach ($providers as $provider) { try { foreach ($provider->getRestrictions($actor, $accesses) as $restriction) { $recipient = $restriction->recipientKind !== null && $restriction->recipientId !== null @@ -216,17 +230,22 @@ private function collectRestrictions(Actor $actor, array $accesses): ?array { } /** - * @param array> $restrictions restricted permission IDs by recipient + * Get the registered permissions that apply to the resource type. + * + * @return array */ - private function evaluate(ResourceAccess $access, array $restrictions): EffectivePermissions { - $permissions = $this->getPermissions(); - $applicable = []; - foreach ($this->getResourceTypes()[$access->resource->type]->getPermissionIds() as $permissionId) { - if (isset($permissions[$permissionId])) { - $applicable[$permissionId] = true; - } - } + private function getApplicablePermissionIds(string $resourceType): array { + return array_fill_keys( + array_intersect($this->getResourceTypes()[$resourceType]->getPermissionIds(), array_keys($this->getPermissions())), + true, + ); + } + /** + * @param array $applicable permission IDs that apply to the type of the resource + * @param array> $restrictions restricted permission IDs by recipient + */ + private function evaluate(ResourceAccess $access, array $applicable, array $restrictions): EffectivePermissions { if ($access->entries === []) { $allowed = $applicable; } else { @@ -265,9 +284,9 @@ private function expand(array $permissionIds): array { } /** - * @return list + * @return ?list null if a provider could not be loaded */ - private function getRestrictionProviders(): array { + private function getRestrictionProviders(): ?array { if ($this->restrictionProviders !== null) { return $this->restrictionProviders; } @@ -280,9 +299,11 @@ private function getRestrictionProviders(): array { $providers = []; foreach ($context->getResourceRestrictionProviders() as $registration) { $provider = $this->instantiate($registration, IRestrictionProvider::class); - if ($provider !== null) { - $providers[] = $provider; + if ($provider === null) { + // Not cached: the provider may be loadable on the next check. + return null; } + $providers[] = $provider; } $this->restrictionProviders = $providers; diff --git a/lib/unstable/Resource/Permission/IPermissionManager.php b/lib/unstable/Resource/Permission/IPermissionManager.php index 61853f2635320..3c7e295c54e36 100644 --- a/lib/unstable/Resource/Permission/IPermissionManager.php +++ b/lib/unstable/Resource/Permission/IPermissionManager.php @@ -53,6 +53,7 @@ public function getResourceType(string $id): ?IResourceType; * With access entries the result is what is left of the permissions granted by them. * Without access entries the result is what is left of all permissions that apply to * the resource type, the caller has to intersect it with the access it grants itself. + * If a restriction provider fails or cannot be loaded, no permission is allowed. * * @param list $entries * @throws \InvalidArgumentException if the resource type is not registered @@ -69,4 +70,14 @@ public function check(Actor $actor, ResourceReference $resource, array $entries * @experimental 36.0.0 */ public function checkAll(Actor $actor, array $accesses): array; + + /** + * Forget the results of previous checks. + * + * Results are remembered while the process runs. An {@see IRestrictionProvider} has to call this + * whenever its restrictions change, so that later checks in the same process see the change. + * + * @experimental 36.0.0 + */ + public function clearCache(): void; } diff --git a/lib/unstable/Resource/Permission/IRestrictionProvider.php b/lib/unstable/Resource/Permission/IRestrictionProvider.php index db52e1da159ce..79bf1f5dce1f5 100644 --- a/lib/unstable/Resource/Permission/IRestrictionProvider.php +++ b/lib/unstable/Resource/Permission/IRestrictionProvider.php @@ -28,6 +28,8 @@ interface IRestrictionProvider { * The resources can be of any type, nothing has to be returned for unknown ones. * If this method throws, every permission on all the resources is restricted. * + * Results are remembered, call {@see IPermissionManager::clearCache} whenever the restrictions change. + * * @param list $accesses * @return iterable * @experimental 36.0.0 diff --git a/tests/lib/Resource/Permission/PermissionManagerTest.php b/tests/lib/Resource/Permission/PermissionManagerTest.php index ac6a5847dc17c..7ba6eee2f9a12 100644 --- a/tests/lib/Resource/Permission/PermissionManagerTest.php +++ b/tests/lib/Resource/Permission/PermissionManagerTest.php @@ -256,9 +256,8 @@ public function testBrokenRegistrationsAreSkipped(): void { $this->instances['wrong'] = new \stdClass(); $this->context->registerResourcePermission('tables', 'wrong'); $this->context->registerResourceType('tables', 'missing'); - $this->context->registerResourceRestrictionProvider('governance', 'wrong'); $this->registerTables(); - $this->logger->expects($this->exactly(4))->method('error'); + $this->logger->expects($this->exactly(3))->method('error'); $this->assertCount(7, $this->manager->getPermissions()); $this->assertCount(2, $this->manager->getResourceTypes()); @@ -415,6 +414,64 @@ public function testFailingProviderRestrictsEverythingAndIsNotCached(): void { $this->assertAllowed([CorePermission::Read->value, CorePermission::Update->value, CorePermission::Delete->value], $row); } + public function testProviderThatCannotBeLoadedRestrictsEverything(): void { + $this->registerTables(); + $this->context->registerResourceRestrictionProvider('governance', 'label-provider'); + $this->logger->expects($this->once())->method('error'); + + $row = new ResourceReference(self::ROW, '1'); + $this->assertAllowed([], $row); + + $this->instances['label-provider'] = new class implements IRestrictionProvider { + public function getRestrictions(Actor $actor, array $accesses): iterable { + return [Restriction::forResource($accesses[0]->resource, [CorePermission::Delete->value])]; + } + }; + $this->assertAllowed([CorePermission::Read->value, CorePermission::Update->value], $row); + } + + public function testRegistrationThroughAppContext(): void { + $this->registerTables(); + $this->instances['forms-type'] = new class implements IResourceType { + public function getId(): string { + return 'forms:form'; + } + + public function getDisplayName(): string { + return 'Form'; + } + + public function getPermissionIds(): array { + return [CorePermission::Read->value, 'forms:results']; + } + }; + $this->instances['forms-permission'] = new class extends APermission { + public function getId(): string { + return 'forms:results'; + } + + public function getDisplayName(): string { + return 'See results'; + } + + public function getKindOf(): CorePermission { + return CorePermission::Read; + } + }; + $this->instances['forms-provider'] = new class implements IRestrictionProvider { + public function getRestrictions(Actor $actor, array $accesses): iterable { + return [Restriction::forResource($accesses[0]->resource, ['forms:results'])]; + } + }; + + $context = $this->context->for('forms'); + $context->registerResourceType('forms-type'); + $context->registerResourcePermission('forms-permission'); + $context->registerResourceRestrictionProvider('forms-provider'); + + $this->assertAllowed([CorePermission::Read->value], new ResourceReference('forms:form', '1')); + } + public function testCheckAll(): void { $this->registerTables(); $this->registerRestrictionProvider(static function (Actor $actor, array $accesses): \Generator { @@ -466,5 +523,25 @@ public function testResultsAreCachedPerActorResourceAndEntries(): void { $this->manager->check(Actor::of(Principal::user('bob')), $row1); $this->manager->check(Actor::of(Principal::bot('summarizer', 'assistant'))->onBehalfOf(Principal::user('alice')), $row1); $this->assertCount(5, $provider->calls); + + $this->manager->clearCache(); + $this->manager->check($this->alice, $row1); + $this->assertCount(6, $provider->calls); + } + + public function testApplicablePermissionsAreReadOncePerResourceType(): void { + $this->registerCorePermissions(); + $resourceType = $this->createMock(IResourceType::class); + $resourceType->method('getId')->willReturn(self::ROW); + $resourceType->expects($this->once())->method('getPermissionIds')->willReturn([CorePermission::Read->value]); + $this->instances[self::ROW] = $resourceType; + $this->context->registerResourceType('tables', self::ROW); + + $results = $this->manager->checkAll($this->alice, [ + new ResourceAccess(new ResourceReference(self::ROW, '1')), + new ResourceAccess(new ResourceReference(self::ROW, '2')), + ]); + + $this->assertSame([CorePermission::Read->value], $results['tables:row/2']->getAllowedPermissionIds()); } } From 851f593881c8664d4fb206c49df20d9562053ce6 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 12 Nov 2026 23:47:44 +0100 Subject: [PATCH 3/4] refactor: share the common parts of the core resource permissions Assisted-by: ClaudeCode:claude-fable-5-2 Signed-off-by: Ferdinand Thiessen --- core/Resource/Permission/ACorePermission.php | 37 +++++++++++++++++++ core/Resource/Permission/CreatePermission.php | 22 +---------- core/Resource/Permission/DeletePermission.php | 22 +---------- .../Permission/DownloadPermission.php | 17 +-------- core/Resource/Permission/ReadPermission.php | 22 +---------- core/Resource/Permission/SharePermission.php | 22 +---------- core/Resource/Permission/UpdatePermission.php | 22 +---------- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 9 files changed, 51 insertions(+), 115 deletions(-) create mode 100644 core/Resource/Permission/ACorePermission.php diff --git a/core/Resource/Permission/ACorePermission.php b/core/Resource/Permission/ACorePermission.php new file mode 100644 index 0000000000000..5450d34d92b4b --- /dev/null +++ b/core/Resource/Permission/ACorePermission.php @@ -0,0 +1,37 @@ +l10nFactory->get(Application::APP_ID); + } + + #[\Override] + public function getHint(): ?string { + return null; + } + + #[\Override] + public function getKindOf(): ?CorePermission { + return null; + } +} diff --git a/core/Resource/Permission/CreatePermission.php b/core/Resource/Permission/CreatePermission.php index 755789b79cf9b..58ad08c863aa9 100644 --- a/core/Resource/Permission/CreatePermission.php +++ b/core/Resource/Permission/CreatePermission.php @@ -10,16 +10,8 @@ namespace OC\Core\Resource\Permission; use NCU\Resource\Permission\CorePermission; -use NCU\Resource\Permission\IPermission; -use OC\Core\AppInfo\Application; -use OCP\L10N\IFactory; - -final class CreatePermission implements IPermission { - public function __construct( - private readonly IFactory $l10nFactory, - ) { - } +final class CreatePermission extends ACorePermission { #[\Override] public function getId(): string { return CorePermission::Create->value; @@ -27,16 +19,6 @@ public function getId(): string { #[\Override] public function getDisplayName(): string { - return $this->l10nFactory->get(Application::APP_ID)->t('Create'); - } - - #[\Override] - public function getHint(): ?string { - return null; - } - - #[\Override] - public function getKindOf(): ?CorePermission { - return null; + return $this->getL10N()->t('Create'); } } diff --git a/core/Resource/Permission/DeletePermission.php b/core/Resource/Permission/DeletePermission.php index 7066f28a28787..badcec5d9d3b7 100644 --- a/core/Resource/Permission/DeletePermission.php +++ b/core/Resource/Permission/DeletePermission.php @@ -10,16 +10,8 @@ namespace OC\Core\Resource\Permission; use NCU\Resource\Permission\CorePermission; -use NCU\Resource\Permission\IPermission; -use OC\Core\AppInfo\Application; -use OCP\L10N\IFactory; - -final class DeletePermission implements IPermission { - public function __construct( - private readonly IFactory $l10nFactory, - ) { - } +final class DeletePermission extends ACorePermission { #[\Override] public function getId(): string { return CorePermission::Delete->value; @@ -27,16 +19,6 @@ public function getId(): string { #[\Override] public function getDisplayName(): string { - return $this->l10nFactory->get(Application::APP_ID)->t('Delete'); - } - - #[\Override] - public function getHint(): ?string { - return null; - } - - #[\Override] - public function getKindOf(): ?CorePermission { - return null; + return $this->getL10N()->t('Delete'); } } diff --git a/core/Resource/Permission/DownloadPermission.php b/core/Resource/Permission/DownloadPermission.php index d30f0eea4fd2a..f4f72fd372860 100644 --- a/core/Resource/Permission/DownloadPermission.php +++ b/core/Resource/Permission/DownloadPermission.php @@ -10,16 +10,8 @@ namespace OC\Core\Resource\Permission; use NCU\Resource\Permission\CorePermission; -use NCU\Resource\Permission\IPermission; -use OC\Core\AppInfo\Application; -use OCP\L10N\IFactory; - -final class DownloadPermission implements IPermission { - public function __construct( - private readonly IFactory $l10nFactory, - ) { - } +final class DownloadPermission extends ACorePermission { #[\Override] public function getId(): string { return CorePermission::DOWNLOAD; @@ -27,12 +19,7 @@ public function getId(): string { #[\Override] public function getDisplayName(): string { - return $this->l10nFactory->get(Application::APP_ID)->t('Download'); - } - - #[\Override] - public function getHint(): ?string { - return null; + return $this->getL10N()->t('Download'); } #[\Override] diff --git a/core/Resource/Permission/ReadPermission.php b/core/Resource/Permission/ReadPermission.php index a4fcb3c804fa2..e9a62ac5b3ba6 100644 --- a/core/Resource/Permission/ReadPermission.php +++ b/core/Resource/Permission/ReadPermission.php @@ -10,16 +10,8 @@ namespace OC\Core\Resource\Permission; use NCU\Resource\Permission\CorePermission; -use NCU\Resource\Permission\IPermission; -use OC\Core\AppInfo\Application; -use OCP\L10N\IFactory; - -final class ReadPermission implements IPermission { - public function __construct( - private readonly IFactory $l10nFactory, - ) { - } +final class ReadPermission extends ACorePermission { #[\Override] public function getId(): string { return CorePermission::Read->value; @@ -27,16 +19,6 @@ public function getId(): string { #[\Override] public function getDisplayName(): string { - return $this->l10nFactory->get(Application::APP_ID)->t('View'); - } - - #[\Override] - public function getHint(): ?string { - return null; - } - - #[\Override] - public function getKindOf(): ?CorePermission { - return null; + return $this->getL10N()->t('View'); } } diff --git a/core/Resource/Permission/SharePermission.php b/core/Resource/Permission/SharePermission.php index 5f7e0d2e5453b..c36e7e624c479 100644 --- a/core/Resource/Permission/SharePermission.php +++ b/core/Resource/Permission/SharePermission.php @@ -10,16 +10,8 @@ namespace OC\Core\Resource\Permission; use NCU\Resource\Permission\CorePermission; -use NCU\Resource\Permission\IPermission; -use OC\Core\AppInfo\Application; -use OCP\L10N\IFactory; - -final class SharePermission implements IPermission { - public function __construct( - private readonly IFactory $l10nFactory, - ) { - } +final class SharePermission extends ACorePermission { #[\Override] public function getId(): string { return CorePermission::Share->value; @@ -27,16 +19,6 @@ public function getId(): string { #[\Override] public function getDisplayName(): string { - return $this->l10nFactory->get(Application::APP_ID)->t('Share'); - } - - #[\Override] - public function getHint(): ?string { - return null; - } - - #[\Override] - public function getKindOf(): ?CorePermission { - return null; + return $this->getL10N()->t('Share'); } } diff --git a/core/Resource/Permission/UpdatePermission.php b/core/Resource/Permission/UpdatePermission.php index e2229475daea3..07c1921402dab 100644 --- a/core/Resource/Permission/UpdatePermission.php +++ b/core/Resource/Permission/UpdatePermission.php @@ -10,16 +10,8 @@ namespace OC\Core\Resource\Permission; use NCU\Resource\Permission\CorePermission; -use NCU\Resource\Permission\IPermission; -use OC\Core\AppInfo\Application; -use OCP\L10N\IFactory; - -final class UpdatePermission implements IPermission { - public function __construct( - private readonly IFactory $l10nFactory, - ) { - } +final class UpdatePermission extends ACorePermission { #[\Override] public function getId(): string { return CorePermission::Update->value; @@ -27,16 +19,6 @@ public function getId(): string { #[\Override] public function getDisplayName(): string { - return $this->l10nFactory->get(Application::APP_ID)->t('Edit'); - } - - #[\Override] - public function getHint(): ?string { - return null; - } - - #[\Override] - public function getKindOf(): ?CorePermission { - return null; + return $this->getL10N()->t('Edit'); } } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 65be1554048ea..d327b1ecd1748 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1783,6 +1783,7 @@ 'OC\\Core\\Migrations\\Version35000Date20260527162338' => $baseDir . '/core/Migrations/Version35000Date20260527162338.php', 'OC\\Core\\Migrations\\Version36000Date20261028100000' => $baseDir . '/core/Migrations/Version36000Date20261028100000.php', 'OC\\Core\\Notification\\CoreNotifier' => $baseDir . '/core/Notification/CoreNotifier.php', + 'OC\\Core\\Resource\\Permission\\ACorePermission' => $baseDir . '/core/Resource/Permission/ACorePermission.php', 'OC\\Core\\Resource\\Permission\\CreatePermission' => $baseDir . '/core/Resource/Permission/CreatePermission.php', 'OC\\Core\\Resource\\Permission\\DeletePermission' => $baseDir . '/core/Resource/Permission/DeletePermission.php', 'OC\\Core\\Resource\\Permission\\DownloadPermission' => $baseDir . '/core/Resource/Permission/DownloadPermission.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 46d0acf6ab136..358709760df50 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1824,6 +1824,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Migrations\\Version35000Date20260527162338' => __DIR__ . '/../../..' . '/core/Migrations/Version35000Date20260527162338.php', 'OC\\Core\\Migrations\\Version36000Date20261028100000' => __DIR__ . '/../../..' . '/core/Migrations/Version36000Date20261028100000.php', 'OC\\Core\\Notification\\CoreNotifier' => __DIR__ . '/../../..' . '/core/Notification/CoreNotifier.php', + 'OC\\Core\\Resource\\Permission\\ACorePermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/ACorePermission.php', 'OC\\Core\\Resource\\Permission\\CreatePermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/CreatePermission.php', 'OC\\Core\\Resource\\Permission\\DeletePermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/DeletePermission.php', 'OC\\Core\\Resource\\Permission\\DownloadPermission' => __DIR__ . '/../../..' . '/core/Resource/Permission/DownloadPermission.php', From 015850c7bf698e96397b573356d76389f3bf134a Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 13 Nov 2026 18:29:01 +0100 Subject: [PATCH 4/4] refactor: allow checking core permissions on effective permissions directly `EffectivePermissions::isAllowed()` now also accepts a `CorePermission` case, so callers no longer need `->value` for the core permissions, and the allowed IDs are kept as a map for constant time lookups. Also document why `CorePermission::DOWNLOAD` is a constant and not a case. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen --- .../Resource/Permission/CorePermission.php | 1 + .../Permission/EffectivePermissions.php | 16 +++++--- .../Permission/EffectivePermissionsTest.php | 40 +++++++++++++++++++ 3 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 tests/lib/Resource/Permission/EffectivePermissionsTest.php diff --git a/lib/unstable/Resource/Permission/CorePermission.php b/lib/unstable/Resource/Permission/CorePermission.php index 76660bee65615..2de4ced1c3f05 100644 --- a/lib/unstable/Resource/Permission/CorePermission.php +++ b/lib/unstable/Resource/Permission/CorePermission.php @@ -13,6 +13,7 @@ /** * The permissions every other permission is a kind of. + * The cases are these root permissions; `DOWNLOAD` is a core permission as well but a kind of `Read`, so it is not a case. * * They are independent of each other, e.g. a file drop allows to create without allowing to read. * The app owning a resource applies its own dependencies when enforcing them. diff --git a/lib/unstable/Resource/Permission/EffectivePermissions.php b/lib/unstable/Resource/Permission/EffectivePermissions.php index 11158e68aab7d..1dd8dd6ed2727 100644 --- a/lib/unstable/Resource/Permission/EffectivePermissions.php +++ b/lib/unstable/Resource/Permission/EffectivePermissions.php @@ -19,20 +19,24 @@ */ #[Consumable(since: '36.0.0')] final readonly class EffectivePermissions { + /** @var array */ + private array $allowed; + /** * @param list $allowedPermissionIds * @experimental 36.0.0 */ - public function __construct( - private array $allowedPermissionIds, - ) { + public function __construct(array $allowedPermissionIds) { + $this->allowed = array_fill_keys($allowedPermissionIds, true); } /** + * @param string|CorePermission $permission ID of the permission or the core permission * @experimental 36.0.0 */ - public function isAllowed(string $permissionId): bool { - return in_array($permissionId, $this->allowedPermissionIds, true); + public function isAllowed(string|CorePermission $permission): bool { + $id = $permission instanceof CorePermission ? $permission->value : $permission; + return isset($this->allowed[$id]); } /** @@ -40,6 +44,6 @@ public function isAllowed(string $permissionId): bool { * @experimental 36.0.0 */ public function getAllowedPermissionIds(): array { - return $this->allowedPermissionIds; + return array_keys($this->allowed); } } diff --git a/tests/lib/Resource/Permission/EffectivePermissionsTest.php b/tests/lib/Resource/Permission/EffectivePermissionsTest.php new file mode 100644 index 0000000000000..3ae4a65b16d7f --- /dev/null +++ b/tests/lib/Resource/Permission/EffectivePermissionsTest.php @@ -0,0 +1,40 @@ +assertTrue($permissions->isAllowed(CorePermission::DOWNLOAD)); + $this->assertTrue($permissions->isAllowed('tables:export')); + $this->assertFalse($permissions->isAllowed('tables:import')); + } + + public function testIsAllowedByCorePermission(): void { + $permissions = new EffectivePermissions([CorePermission::Read->value]); + + $this->assertTrue($permissions->isAllowed(CorePermission::Read)); + $this->assertTrue($permissions->isAllowed(CorePermission::Read->value)); + $this->assertFalse($permissions->isAllowed(CorePermission::Update)); + } + + public function testGetAllowedPermissionIds(): void { + $this->assertSame([], (new EffectivePermissions([]))->getAllowedPermissionIds()); + $this->assertSame( + [CorePermission::Update->value, CorePermission::Read->value, 'tables:export'], + (new EffectivePermissions([CorePermission::Update->value, CorePermission::Read->value, 'tables:export']))->getAllowedPermissionIds(), + ); + } +}