From 8f271e968c4aed71c4dcd7a04ae462c9a7521904 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 13 Nov 2026 19:54:56 +0100 Subject: [PATCH 1/2] feat: ask the resource type for the access of an actor Resource types now return the ways an actor has access to their resources. A check without access entries asks the resource type instead of allowing all permissions of the type, so callers no longer have to intersect the result with the access they grant. An empty list of entries means no access. Assisted-by: ClaudeCode:claude-fable-5-2 Signed-off-by: Ferdinand Thiessen --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Resource/Permission/AccessManager.php | 93 ++++++++++++++ .../Resource/Permission/PermissionManager.php | 68 ++++++---- lib/unstable/Resource/IResourceType.php | 20 +++ .../Permission/IPermissionManager.php | 22 ++-- .../Permission/IRestrictionProvider.php | 2 +- .../Resource/Permission/ResourceAccess.php | 5 +- .../Permission/PermissionManagerTest.php | 121 +++++++++++++++++- 9 files changed, 286 insertions(+), 47 deletions(-) create mode 100644 lib/private/Resource/Permission/AccessManager.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index d327b1ecd1748..983480171c938 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -2317,6 +2317,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\\AccessManager' => $baseDir . '/lib/private/Resource/Permission/AccessManager.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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 358709760df50..6cdb8e19f1d92 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -2358,6 +2358,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\\AccessManager' => __DIR__ . '/../../..' . '/lib/private/Resource/Permission/AccessManager.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', diff --git a/lib/private/Resource/Permission/AccessManager.php b/lib/private/Resource/Permission/AccessManager.php new file mode 100644 index 0000000000000..83a050f9b6c12 --- /dev/null +++ b/lib/private/Resource/Permission/AccessManager.php @@ -0,0 +1,93 @@ +> access entries by actor key and resource key */ + private CappedMemoryCache $cache; + + public function __construct( + private readonly ContainerInterface $container, + private readonly LoggerInterface $logger, + ) { + $this->cache = new CappedMemoryCache(2048); + } + + /** + * Get the access entries of the actor for every resource, the resources can be of different types. + * + * @param list $resources + * @return array> keyed by {@see ResourceReference::key}, + * an empty list if the actor has no access and null if the resource type failed + * @throws InvalidArgumentException if a resource type is not registered + */ + public function resolve(Actor $actor, array $resources): array { + // Resolved lazily: the permission manager depends on this class. + $permissionManager = $this->container->get(IPermissionManager::class); + $actorKey = $actor->key(); + + $results = []; + /** @var array> $misses */ + $misses = []; + foreach ($resources as $resource) { + if ($permissionManager->getResourceType($resource->type) === null) { + throw new InvalidArgumentException('The resource type ' . $resource->type . ' is not registered'); + } + + $resourceKey = $resource->key(); + if (array_key_exists($resourceKey, $results)) { + continue; + } + + $cached = $this->cache->get($actorKey . "\0" . $resourceKey); + $results[$resourceKey] = $cached; + if ($cached === null) { + $misses[$resource->type][] = $resource; + } + } + + foreach ($misses as $type => $typeResources) { + $resourceType = $permissionManager->getResourceType($type); + try { + $access = $resourceType->getAccess($actor, $typeResources); + } catch (Throwable $e) { + // Not cached: the resource type may succeed on the next call. + $this->logger->error('The resource type {type} failed to get the access, no access is assumed', ['type' => $type, 'exception' => $e]); + continue; + } + + foreach ($typeResources as $resource) { + $resourceKey = $resource->key(); + $entries = array_values($access[$resourceKey] ?? []); + $this->cache->set($actorKey . "\0" . $resourceKey, $entries); + $results[$resourceKey] = $entries; + } + } + + return $results; + } + + public function clearCache(): void { + $this->cache->clear(); + } +} diff --git a/lib/private/Resource/Permission/PermissionManager.php b/lib/private/Resource/Permission/PermissionManager.php index 2471b0a16b000..504512089abbb 100644 --- a/lib/private/Resource/Permission/PermissionManager.php +++ b/lib/private/Resource/Permission/PermissionManager.php @@ -49,6 +49,7 @@ public function __construct( private readonly Coordinator $coordinator, private readonly ContainerInterface $container, private readonly LoggerInterface $logger, + private readonly AccessManager $accessManager, ) { $this->cache = new CappedMemoryCache(2048); } @@ -140,27 +141,46 @@ public function getResourceType(string $id): ?IResourceType { } #[\Override] - public function check(Actor $actor, ResourceReference $resource, array $entries = []): EffectivePermissions { + public function check(Actor $actor, ResourceReference $resource, ?array $entries = null): EffectivePermissions { return $this->checkAll($actor, [new ResourceAccess($resource, $entries)])[$resource->key()]; } #[\Override] public function checkAll(Actor $actor, array $accesses): array { $actorKey = $actor->key(); + $nothing = new EffectivePermissions([]); + /** @var array $results in the order of the accesses */ $results = []; - /** @var array $misses */ - $misses = []; - $cacheKeys = []; + $unresolved = []; 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])) { + if (isset($results[$resourceKey])) { throw new InvalidArgumentException('The resource ' . $resourceKey . ' is passed more than once'); } - $cacheKeys[$resourceKey] = $actorKey . "\0" . $resourceKey . "\0" . $this->hashEntries($access->entries); + $results[$resourceKey] = $nothing; + if ($access->entries === null) { + $unresolved[] = $access->resource; + } + } + $resolved = $unresolved === [] ? [] : $this->accessManager->resolve($actor, $unresolved); + + /** @var array $misses */ + $misses = []; + $cacheKeys = []; + foreach ($accesses as $access) { + $resourceKey = $access->resource->key(); + // No entries are left if the resource type failed, this is not cached as it may succeed on the next check. + $entries = $access->entries ?? $resolved[$resourceKey] ?? []; + if ($entries === []) { + continue; + } + + $access = new ResourceAccess($access->resource, $entries); + $cacheKeys[$resourceKey] = $actorKey . "\0" . $resourceKey . "\0" . $this->hashEntries($entries); $cached = $this->cache->get($cacheKeys[$resourceKey]); if ($cached !== null) { $results[$resourceKey] = $cached; @@ -178,7 +198,6 @@ public function checkAll(Actor $actor, array $accesses): array { 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; } @@ -194,13 +213,14 @@ public function checkAll(Actor $actor, array $accesses): array { #[\Override] public function clearCache(): void { $this->cache->clear(); + $this->accessManager->clearCache(); } /** * Get the restricted permission IDs by resource key and recipient, * the empty recipient is the resource as a whole. * - * @param list $accesses + * @param list $accesses with the access entries set * @return ?array>> null if a provider failed */ private function collectRestrictions(Actor $actor, array $accesses): ?array { @@ -246,22 +266,18 @@ private function getApplicablePermissionIds(string $resourceType): array { * @param array> $restrictions restricted permission IDs by recipient */ private function evaluate(ResourceAccess $access, array $applicable, array $restrictions): EffectivePermissions { - 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)] ?? [], - ); + $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[''] ?? []))); @@ -344,13 +360,9 @@ private function recipientKey(string $kind, string $id): string { } /** - * @param list $entries + * @param non-empty-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/unstable/Resource/IResourceType.php b/lib/unstable/Resource/IResourceType.php index 614cbb3d2fab4..933bcdf687028 100644 --- a/lib/unstable/Resource/IResourceType.php +++ b/lib/unstable/Resource/IResourceType.php @@ -9,6 +9,8 @@ namespace NCU\Resource; +use NCU\Resource\Permission\AccessEntry; +use OCP\Actor\Actor; use OCP\AppFramework\Attribute\Implementable; /** @@ -43,4 +45,22 @@ public function getDisplayName(): string; * @experimental 36.0.0 */ public function getPermissionIds(): array; + + /** + * Get the ways the actor has access to the resources, e.g. as the owner or through shares. + * + * Every way is returned as stored without resolving memberships or roles, + * e.g. a share with a team is returned as recipient kind `team` and the ID of the team. + * The owner is returned as an entry with all permissions of the resource. + * If this method throws, the actor is treated as having no access to any of the resources. + * + * Results are remembered, call {@see \NCU\Resource\Permission\IPermissionManager::clearCache} + * whenever the access to a resource changes. + * + * @param list $resources Resources of this type + * @return array> keyed by {@see ResourceReference::key}, + * resources the actor has no access to are left out + * @experimental 36.0.0 + */ + public function getAccess(Actor $actor, array $resources): array; } diff --git a/lib/unstable/Resource/Permission/IPermissionManager.php b/lib/unstable/Resource/Permission/IPermissionManager.php index 3c7e295c54e36..1fc971a00c001 100644 --- a/lib/unstable/Resource/Permission/IPermissionManager.php +++ b/lib/unstable/Resource/Permission/IPermissionManager.php @@ -18,7 +18,8 @@ * Gives access to the registered permissions and resource types * and checks which permissions are restricted by other apps. * - * The check does not decide which access exists, this is up to the app owning the resource. + * Which access exists is up to the app owning the resource, it either passes the access + * or is asked for it through {@see IResourceType::getAccess}. * That app also has to enforce the result. * * @experimental 36.0.0 @@ -50,16 +51,16 @@ 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. - * If a restriction provider fails or cannot be loaded, no permission is allowed. + * The result is what is left of the permissions granted by the access entries. + * If no entries are passed, the resource type is asked for the access of the actor. + * With an empty list of entries the actor has no access and no permission is allowed. + * If a restriction provider or the resource type fails, no permission is allowed. * - * @param list $entries + * @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; + public function check(Actor $actor, ResourceReference $resource, ?array $entries = null): EffectivePermissions; /** * Same as {@see self::check} for multiple resources at once, the resources can be of different types. @@ -72,10 +73,11 @@ public function check(Actor $actor, ResourceReference $resource, array $entries public function checkAll(Actor $actor, array $accesses): array; /** - * Forget the results of previous checks. + * Forget the results of previous checks and the access returned by the resource types. * - * 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. + * Both are remembered while the process runs. An {@see IRestrictionProvider} has to call this + * whenever its restrictions change and the app owning a resource whenever the access to it changes, + * so that later checks in the same process see the change. * * @experimental 36.0.0 */ diff --git a/lib/unstable/Resource/Permission/IRestrictionProvider.php b/lib/unstable/Resource/Permission/IRestrictionProvider.php index 79bf1f5dce1f5..e978dce4cd7f1 100644 --- a/lib/unstable/Resource/Permission/IRestrictionProvider.php +++ b/lib/unstable/Resource/Permission/IRestrictionProvider.php @@ -30,7 +30,7 @@ interface IRestrictionProvider { * * Results are remembered, call {@see IPermissionManager::clearCache} whenever the restrictions change. * - * @param list $accesses + * @param list $accesses The access entries are always set * @return iterable * @experimental 36.0.0 */ diff --git a/lib/unstable/Resource/Permission/ResourceAccess.php b/lib/unstable/Resource/Permission/ResourceAccess.php index be9ff5153b640..41e0939c2cac5 100644 --- a/lib/unstable/Resource/Permission/ResourceAccess.php +++ b/lib/unstable/Resource/Permission/ResourceAccess.php @@ -21,12 +21,13 @@ #[Consumable(since: '36.0.0')] final readonly class ResourceAccess { /** - * @param list $entries Without entries restrictions apply to the resource as a whole + * @param ?list $entries Null to ask the resource type for the access of the actor, + * an empty list means the actor has no access * @experimental 36.0.0 */ public function __construct( public ResourceReference $resource, - public array $entries = [], + public ?array $entries = null, ) { } } diff --git a/tests/lib/Resource/Permission/PermissionManagerTest.php b/tests/lib/Resource/Permission/PermissionManagerTest.php index 7ba6eee2f9a12..72a649cc7b79b 100644 --- a/tests/lib/Resource/Permission/PermissionManagerTest.php +++ b/tests/lib/Resource/Permission/PermissionManagerTest.php @@ -21,6 +21,7 @@ use NCU\Resource\ResourceReference; use OC\AppFramework\Bootstrap\Coordinator; use OC\AppFramework\Bootstrap\RegistrationContext; +use OC\Resource\Permission\AccessManager; use OC\Resource\Permission\PermissionManager; use OCP\Actor\Actor; use OCP\Actor\Principal; @@ -60,7 +61,8 @@ protected function setUp(): void { }; }); - $this->manager = new PermissionManager($coordinator, $container, $this->logger); + $this->manager = new PermissionManager($coordinator, $container, $this->logger, new AccessManager($container, $this->logger)); + $this->instances[IPermissionManager::class] = $this->manager; $this->alice = Actor::of(Principal::user('alice')); } @@ -100,15 +102,33 @@ private function registerCorePermissions(): void { /** * @param list $permissionIds + * @param ?\Closure(Actor, list): array> $access defaults to all permissions for every user + * @return object{calls: list>}&IResourceType */ - private function registerResourceType(string $appId, string $id, array $permissionIds): void { - $this->instances[$id] = new class($id, $permissionIds) implements IResourceType { + private function registerResourceType(string $appId, string $id, array $permissionIds, ?\Closure $access = null): IResourceType { + $access ??= static function (Actor $actor, array $resources) use ($permissionIds): array { + $entries = []; + foreach ($resources as $resource) { + $entries[$resource->key()] = [new AccessEntry(AccessEntry::RECIPIENT_USER, $actor->principal->id, $permissionIds)]; + } + return $entries; + }; + $resourceType = new class($id, $permissionIds, $access) implements IResourceType { + /** @var list> */ + public array $calls = []; + public function __construct( private readonly string $id, private readonly array $permissionIds, + private readonly \Closure $access, ) { } + public function getAccess(Actor $actor, array $resources): array { + $this->calls[] = $resources; + return ($this->access)($actor, $resources); + } + public function getId(): string { return $this->id; } @@ -121,12 +141,16 @@ public function getPermissionIds(): array { return $this->permissionIds; } }; + $this->instances[$id] = $resourceType; $this->context->registerResourceType($appId, $id); + + return $resourceType; } /** * 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). + * Every user has access to all tables and rows with all permissions. */ private function registerTables(): void { $this->registerCorePermissions(); @@ -174,7 +198,7 @@ public function getRestrictions(Actor $actor, array $accesses): iterable { /** * @param list $expected */ - private function assertAllowed(array $expected, ResourceReference $resource, array $entries = []): void { + private function assertAllowed(array $expected, ResourceReference $resource, ?array $entries = null): void { $allowed = $this->manager->check($this->alice, $resource, $entries)->getAllowedPermissionIds(); sort($allowed); sort($expected); @@ -376,7 +400,7 @@ public function testRecipientRestrictionOnlyAppliesToEntriesOfTheRecipient(): vo $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 + // The access returned by the resource type is a direct one $this->assertAllowed([CorePermission::Read->value, CorePermission::Update->value, CorePermission::Share->value, self::EXPORT], $resource); } @@ -444,6 +468,10 @@ public function getDisplayName(): string { public function getPermissionIds(): array { return [CorePermission::Read->value, 'forms:results']; } + + public function getAccess(Actor $actor, array $resources): array { + return [$resources[0]->key() => [new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', $this->getPermissionIds())]]; + } }; $this->instances['forms-permission'] = new class extends APermission { public function getId(): string { @@ -514,7 +542,9 @@ public function testResultsAreCachedPerActorResourceAndEntries(): void { // 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->assertEquals([new ResourceAccess($row2, [ + new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value, CorePermission::Update->value, CorePermission::Delete->value]), + ])], $provider->calls[1]); $this->manager->check($this->alice, $row1, $entries); $this->manager->check($this->alice, $row1, $entries); @@ -529,11 +559,90 @@ public function testResultsAreCachedPerActorResourceAndEntries(): void { $this->assertCount(6, $provider->calls); } + public function testResourceTypeIsAskedOncePerTypeForAccessesWithoutEntries(): void { + $this->registerCorePermissions(); + $share = static fn (string $key): array => [$key => [new AccessEntry(AccessEntry::RECIPIENT_GROUP, 'staff', [CorePermission::Read->value])]]; + $tables = $this->registerResourceType('tables', self::TABLE, [CorePermission::Read->value, CorePermission::Update->value], static fn (): array => $share('tables:table/1')); + $rows = $this->registerResourceType('tables', self::ROW, [CorePermission::Read->value], static fn (): array => $share('tables:row/1')); + + $table1 = new ResourceReference(self::TABLE, '1'); + $table2 = new ResourceReference(self::TABLE, '2'); + $table3 = new ResourceReference(self::TABLE, '3'); + $row1 = new ResourceReference(self::ROW, '1'); + $results = $this->manager->checkAll($this->alice, [ + new ResourceAccess($table1), + new ResourceAccess($row1), + new ResourceAccess($table2), + new ResourceAccess($table3, [new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Update->value])]), + ]); + + $this->assertSame(['tables:table/1', 'tables:row/1', 'tables:table/2', 'tables:table/3'], array_keys($results)); + $this->assertSame([CorePermission::Read->value], $results['tables:table/1']->getAllowedPermissionIds()); + $this->assertSame([CorePermission::Read->value], $results['tables:row/1']->getAllowedPermissionIds()); + // Left out by the resource type + $this->assertSame([], $results['tables:table/2']->getAllowedPermissionIds()); + $this->assertSame([CorePermission::Update->value], $results['tables:table/3']->getAllowedPermissionIds()); + $this->assertEquals([[$table1, $table2]], $tables->calls); + $this->assertEquals([[$row1]], $rows->calls); + } + + public function testWithoutAccessNothingIsAllowedAndProvidersAreNotAsked(): void { + $this->registerTables(); + $provider = $this->registerRestrictionProvider(static fn (): array => []); + + $this->assertAllowed([], new ResourceReference(self::TABLE, '1'), []); + $this->assertSame([], $provider->calls); + } + + public function testAccessOfTheResourceTypeIsCached(): void { + $this->registerCorePermissions(); + $rows = $this->registerResourceType('tables', self::ROW, [CorePermission::Read->value]); + $provider = $this->registerRestrictionProvider(static fn (): array => []); + $row = new ResourceReference(self::ROW, '1'); + + $this->manager->check($this->alice, $row); + // The result is shared with a check that passes the same entries + $this->manager->check($this->alice, $row, [new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value])]); + $this->manager->check($this->alice, $row); + $this->assertCount(1, $rows->calls); + $this->assertCount(1, $provider->calls); + + $this->manager->clearCache(); + $this->manager->check($this->alice, $row); + $this->assertCount(2, $rows->calls); + } + + public function testFailingResourceTypeAllowsNothingAndIsNotCached(): void { + $this->registerCorePermissions(); + $fail = true; + $this->registerResourceType('tables', self::ROW, [CorePermission::Read->value], static function (Actor $actor, array $resources) use (&$fail): array { + if ($fail) { + throw new \RuntimeException('Share storage not available'); + } + return ['tables:row/1' => [new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value])]]; + }); + $this->registerResourceType('tables', self::TABLE, [CorePermission::Read->value]); + $this->logger->expects($this->once())->method('error'); + + $row = new ResourceReference(self::ROW, '1'); + $table = new ResourceReference(self::TABLE, '1'); + $results = $this->manager->checkAll($this->alice, [new ResourceAccess($row), new ResourceAccess(new ResourceReference(self::ROW, '2')), new ResourceAccess($table)]); + $this->assertSame([], $results[$row->key()]->getAllowedPermissionIds()); + $this->assertSame([CorePermission::Read->value], $results[$table->key()]->getAllowedPermissionIds()); + + $fail = false; + $this->assertAllowed([CorePermission::Read->value], $row); + } + 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]); + $resourceType->method('getAccess')->willReturn([ + 'tables:row/1' => [new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value])], + 'tables:row/2' => [new AccessEntry(AccessEntry::RECIPIENT_USER, 'alice', [CorePermission::Read->value])], + ]); $this->instances[self::ROW] = $resourceType; $this->context->registerResourceType('tables', self::ROW); From 7db1b0bffedaa297ac94e64544246e5a8823a7a4 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sat, 14 Nov 2026 09:38:43 +0100 Subject: [PATCH 2/2] feat: make the access of an actor to resources available to apps `NCU\Resource\Permission\IAccessManager` returns the access an actor was given to resources before any restriction, so apps do not have to ask the resource types themselves. Assisted-by: ClaudeCode:claude-fable-5-2 Signed-off-by: Ferdinand Thiessen --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Resource/Permission/AccessManager.php | 8 +++- lib/private/Server.php | 1 + .../Resource/Permission/IAccessManager.php | 38 +++++++++++++++++++ .../Permission/PermissionManagerTest.php | 37 +++++++++++++++++- 6 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 lib/unstable/Resource/Permission/IAccessManager.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 983480171c938..b3e8e07652c47 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -23,6 +23,7 @@ '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\\IAccessManager' => $baseDir . '/lib/unstable/Resource/Permission/IAccessManager.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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 6cdb8e19f1d92..113654baeef47 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -64,6 +64,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 '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\\IAccessManager' => __DIR__ . '/../../..' . '/lib/unstable/Resource/Permission/IAccessManager.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', diff --git a/lib/private/Resource/Permission/AccessManager.php b/lib/private/Resource/Permission/AccessManager.php index 83a050f9b6c12..00e94b44f594a 100644 --- a/lib/private/Resource/Permission/AccessManager.php +++ b/lib/private/Resource/Permission/AccessManager.php @@ -11,6 +11,7 @@ use InvalidArgumentException; use NCU\Resource\Permission\AccessEntry; +use NCU\Resource\Permission\IAccessManager; use NCU\Resource\Permission\IPermissionManager; use NCU\Resource\ResourceReference; use OCP\Actor\Actor; @@ -22,7 +23,7 @@ /** * Asks the resource types which access an actor has to resources. */ -final class AccessManager { +final class AccessManager implements IAccessManager { /** @var CappedMemoryCache> access entries by actor key and resource key */ private CappedMemoryCache $cache; @@ -33,6 +34,11 @@ public function __construct( $this->cache = new CappedMemoryCache(2048); } + #[\Override] + public function getAccess(Actor $actor, array $resources): array { + return array_filter($this->resolve($actor, $resources)); + } + /** * Get the access entries of the actor for every resource, the resources can be of different types. * diff --git a/lib/private/Server.php b/lib/private/Server.php index 584347208e58e..8676dfae26bd7 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1118,6 +1118,7 @@ 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->registerAlias(\NCU\Resource\Permission\IAccessManager::class, \OC\Resource\Permission\AccessManager::class); $this->registerService(IGlobalScaleService::class, static function (ContainerInterface $c): IGlobalScaleService { /** @var Coordinator $coordinator */ diff --git a/lib/unstable/Resource/Permission/IAccessManager.php b/lib/unstable/Resource/Permission/IAccessManager.php new file mode 100644 index 0000000000000..5c2aba83d64b7 --- /dev/null +++ b/lib/unstable/Resource/Permission/IAccessManager.php @@ -0,0 +1,38 @@ + $resources + * @return array> keyed by {@see ResourceReference::key}, + * resources the actor has no access to are left out + * @throws \InvalidArgumentException if a resource type is not registered + * @experimental 36.0.0 + */ + public function getAccess(Actor $actor, array $resources): array; +} diff --git a/tests/lib/Resource/Permission/PermissionManagerTest.php b/tests/lib/Resource/Permission/PermissionManagerTest.php index 72a649cc7b79b..25817249f7a7e 100644 --- a/tests/lib/Resource/Permission/PermissionManagerTest.php +++ b/tests/lib/Resource/Permission/PermissionManagerTest.php @@ -13,6 +13,7 @@ use NCU\Resource\Permission\AccessEntry; use NCU\Resource\Permission\APermission; use NCU\Resource\Permission\CorePermission; +use NCU\Resource\Permission\IAccessManager; use NCU\Resource\Permission\IPermission; use NCU\Resource\Permission\IPermissionManager; use NCU\Resource\Permission\IRestrictionProvider; @@ -41,6 +42,7 @@ class PermissionManagerTest extends TestCase { private RegistrationContext $context; private LoggerInterface&MockObject $logger; private PermissionManager $manager; + private AccessManager $accessManager; private Actor $alice; /** @var array */ @@ -61,7 +63,8 @@ protected function setUp(): void { }; }); - $this->manager = new PermissionManager($coordinator, $container, $this->logger, new AccessManager($container, $this->logger)); + $this->accessManager = new AccessManager($container, $this->logger); + $this->manager = new PermissionManager($coordinator, $container, $this->logger, $this->accessManager); $this->instances[IPermissionManager::class] = $this->manager; $this->alice = Actor::of(Principal::user('alice')); } @@ -634,6 +637,38 @@ public function testFailingResourceTypeAllowsNothingAndIsNotCached(): void { $this->assertAllowed([CorePermission::Read->value], $row); } + public function testGetAccess(): void { + $this->registerCorePermissions(); + $entry = new AccessEntry(AccessEntry::RECIPIENT_TEAM, 'sales', [CorePermission::Read->value]); + $fail = true; + $this->registerResourceType('tables', self::TABLE, [CorePermission::Read->value], static fn (): array => ['tables:table/1' => [$entry], 'tables:table/3' => []]); + $this->registerResourceType('tables', self::ROW, [CorePermission::Read->value], static function () use (&$fail, $entry): array { + if ($fail) { + throw new \RuntimeException('Share storage not available'); + } + return ['tables:row/1' => [$entry]]; + }); + $this->logger->expects($this->once())->method('error'); + + $resources = [ + new ResourceReference(self::TABLE, '1'), + new ResourceReference(self::TABLE, '2'), + new ResourceReference(self::TABLE, '3'), + new ResourceReference(self::ROW, '1'), + ]; + $this->assertSame(['tables:table/1' => [$entry]], $this->accessManager->getAccess($this->alice, $resources)); + + $fail = false; + $this->assertSame(['tables:table/1' => [$entry], 'tables:row/1' => [$entry]], $this->accessManager->getAccess($this->alice, $resources)); + } + + public function testGetAccessForUnregisteredResourceType(): void { + $this->assertInstanceOf(AccessManager::class, Server::get(IAccessManager::class)); + + $this->expectException(\InvalidArgumentException::class); + $this->accessManager->getAccess($this->alice, [new ResourceReference('forms:form', '1')]); + } + public function testApplicablePermissionsAreReadOncePerResourceType(): void { $this->registerCorePermissions(); $resourceType = $this->createMock(IResourceType::class);