From 22630a5ce6dd1582d5470b7f3a1d20ac2f9f12a8 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Tue, 31 Mar 2026 09:15:39 +0200 Subject: [PATCH] perf: cache authorized-group delegation lookups Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- .../lib/Controller/ShareAPIController.php | 56 +++++++- .../Controller/AuthorizedGroupController.php | 35 +---- .../lib/Service/AuthorizedGroupService.php | 83 ++++++++++- .../Controller/DelegationControllerTest.php | 23 +-- .../Service/AuthorizedGroupServiceTest.php | 129 +++++++++++++++++ .../Version34000Date20260331000000.php | 131 ++++++++++++++++++ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Security/SecurityMiddleware.php | 29 +++- .../Settings/AuthorizedGroupMapper.php | 97 ++++++++++++- lib/private/Share20/DefaultShareProvider.php | 35 ++++- .../Mock/SecurityMiddlewareController.php | 6 + .../Security/SecurityMiddlewareTest.php | 71 ++++++++-- 13 files changed, 618 insertions(+), 79 deletions(-) create mode 100644 core/Migrations/Version34000Date20260331000000.php diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 0f1ad3bd5d557..3f6c684605a48 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -80,6 +80,22 @@ class ShareAPIController extends OCSController { private ?Node $lockedNode = null; private array $trustedServerCache = []; + /** + * Per-request user object cache to avoid repeated backend lookups when + * formatting multiple shares for the same owner, sharer, or recipient. + * Keyed by UID. Stores null to avoid re-querying a UID that doesn't exist. + * + * @var array + */ + private array $userObjectCache = []; + + /** + * Per-request group object cache for the same reason as $userObjectCache. + * + * @var array + */ + private array $groupObjectCache = []; + /** * Share20OCS constructor. */ @@ -110,19 +126,47 @@ public function __construct( parent::__construct($appName, $request); } + /** + * Fetches a user object, using the request-scoped cache to avoid repeated + * backend calls for the same UID within a single API response. + * Uses array_key_exists() so that UIDs resolving to null are cached and + * not re-queried (e.g. shares from deleted users). + * + * @since 34.0.0 + * @internal + */ + private function getCachedUser(string $uid): ?\OCP\IUser { + if (!array_key_exists($uid, $this->userObjectCache)) { + $this->userObjectCache[$uid] = $this->userManager->get($uid); + } + return $this->userObjectCache[$uid]; + } + + /** + * Fetches a group object, using the request-scoped cache to avoid repeated + * backend calls for the same GID within a single API response. + * + * @since 34.0.0 + * @internal + */ + private function getCachedGroup(string $gid): ?\OCP\IGroup { + if (!array_key_exists($gid, $this->groupObjectCache)) { + $this->groupObjectCache[$gid] = $this->groupManager->get($gid); + } + return $this->groupObjectCache[$gid]; + } + /** * Convert an IShare to an array for OCS output * - * @param IShare $share - * @param Node|null $recipientNode * @return Files_SharingShare * @throws NotFoundException In case the node can't be resolved. * * @suppress PhanUndeclaredClassMethod */ protected function formatShare(IShare $share, ?Node $recipientNode = null): array { - $sharedBy = $this->userManager->get($share->getSharedBy()); - $shareOwner = $this->userManager->get($share->getShareOwner()); + $sharedBy = $this->getCachedUser($share->getSharedBy()); + $shareOwner = $this->getCachedUser($share->getShareOwner()); $isOwnShare = false; if ($shareOwner !== null) { @@ -240,7 +284,7 @@ protected function formatShare(IShare $share, ?Node $recipientNode = null): arra } if ($share->getShareType() === IShare::TYPE_USER) { - $sharedWith = $this->userManager->get($share->getSharedWith()); + $sharedWith = $this->getCachedUser($share->getSharedWith()); $result['share_with'] = $share->getSharedWith(); $result['share_with_displayname'] = $sharedWith !== null ? $sharedWith->getDisplayName() : $share->getSharedWith(); $result['share_with_displayname_unique'] = $sharedWith !== null ? ( @@ -260,7 +304,7 @@ protected function formatShare(IShare $share, ?Node $recipientNode = null): arra ]; } } elseif ($share->getShareType() === IShare::TYPE_GROUP) { - $group = $this->groupManager->get($share->getSharedWith()); + $group = $this->getCachedGroup($share->getSharedWith()); $result['share_with'] = $share->getSharedWith(); $result['share_with_displayname'] = $group !== null ? $group->getDisplayName() : $share->getSharedWith(); } elseif ($share->getShareType() === IShare::TYPE_LINK) { diff --git a/apps/settings/lib/Controller/AuthorizedGroupController.php b/apps/settings/lib/Controller/AuthorizedGroupController.php index 9ff0f2cb2158a..000d33e8bfe90 100644 --- a/apps/settings/lib/Controller/AuthorizedGroupController.php +++ b/apps/settings/lib/Controller/AuthorizedGroupController.php @@ -6,7 +6,6 @@ */ namespace OCA\Settings\Controller; -use OC\Settings\AuthorizedGroup; use OCA\Settings\Service\AuthorizedGroupService; use OCA\Settings\Service\NotFoundException; use OCP\AppFramework\Controller; @@ -26,37 +25,13 @@ public function __construct( /** * @throws NotFoundException * @throws Exception + * @throws \Throwable */ public function saveSettings(array $newGroups, string $class): DataResponse { - $currentGroups = $this->authorizedGroupService->findExistingGroupsForClass($class); - - foreach ($currentGroups as $group) { - /** @var AuthorizedGroup $group */ - $removed = true; - foreach ($newGroups as $groupData) { - if ($groupData['gid'] === $group->getGroupId()) { - $removed = false; - break; - } - } - if ($removed) { - $this->authorizedGroupService->delete($group->getId()); - } - } - - foreach ($newGroups as $groupData) { - $added = true; - foreach ($currentGroups as $group) { - /** @var AuthorizedGroup $group */ - if ($groupData['gid'] === $group->getGroupId()) { - $added = false; - break; - } - } - if ($added) { - $this->authorizedGroupService->create($groupData['gid'], $class); - } - } + // Delegate the diff-and-apply logic to the service so that the cache + // is flushed exactly once after all mutations, regardless of how many + // groups were added or removed. + $this->authorizedGroupService->saveSettings($newGroups, $class); return new DataResponse(['valid' => true]); } diff --git a/apps/settings/lib/Service/AuthorizedGroupService.php b/apps/settings/lib/Service/AuthorizedGroupService.php index 223bf8ee19e62..fffa9c9a4d556 100644 --- a/apps/settings/lib/Service/AuthorizedGroupService.php +++ b/apps/settings/lib/Service/AuthorizedGroupService.php @@ -55,7 +55,74 @@ private function handleException(Throwable $e): void { } /** - * Create a new AuthorizedGroup + * Applies a bulk delegation update for a setting class in a single + * transaction-like sequence, then invalidates the cache exactly once. + * + * @param list $newGroups Desired final state + * @throws Exception + * @throws Throwable + */ + public function saveSettings(array $newGroups, string $class): void { + $currentGroups = $this->findExistingGroupsForClass($class); + + foreach ($currentGroups as $group) { + $removed = true; + foreach ($newGroups as $groupData) { + if ($groupData['gid'] === $group->getGroupId()) { + $removed = false; + break; + } + } + + if ($removed) { + try { + // $group is already a hydrated AuthorizedGroup entity + // returned by findExistingGroupsForClass() + $this->mapper->delete($group); + } catch (\Exception $exception) { + $this->handleException($exception); + } + } + } + + // We attempt the insert unconditionally and treat a unique- + // constraint violation as idempotent — cheaper than a read-before-write + // and race-safe under concurrent saveSettings() calls. + foreach ($newGroups as $groupData) { + $added = true; + foreach ($currentGroups as $group) { + if ($groupData['gid'] === $group->getGroupId()) { + $added = false; + break; + } + } + + if ($added) { + try { + $newGroup = new AuthorizedGroup(); + $newGroup->setGroupId($groupData['gid']); + $newGroup->setClass($class); + $this->mapper->insert($newGroup); + } catch (Exception $e) { + // The DB unique constraint prevented the duplicate + // so treat as idempotent success. + if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $e; + } + } + } + } + + $this->mapper->clearCache(); + } + + /** + * Create a new AuthorizedGroup and invalidate the distributed cache. + * + * Adding a delegation may grant access to every current member of the + * affected group. A full cache clear is used rather than per-user + * invalidation to avoid an extra group-membership backend call at write + * time. * * @throws Exception * @throws ConflictException @@ -73,7 +140,14 @@ public function create(string $groupId, string $class): AuthorizedGroup { $authorizedGroup = new AuthorizedGroup(); $authorizedGroup->setGroupId($groupId); $authorizedGroup->setClass($class); - return $this->mapper->insert($authorizedGroup); + + $result = $this->mapper->insert($authorizedGroup); + + // Invalidate after successful insert so the next request re-evaluates + // all users' authorized classes. + $this->mapper->clearCache(); + + return $result; } /** @@ -84,6 +158,8 @@ public function delete(int $id): void { try { $authorizedGroup = $this->mapper->find($id); $this->mapper->delete($authorizedGroup); + // Revoking a delegation must take effect immediately. + $this->mapper->clearCache(); } catch (\Exception $exception) { $this->handleException($exception); } @@ -107,6 +183,9 @@ public function findExistingGroupsForClass(string $class): array { public function removeAuthorizationAssociatedTo(IGroup $group): void { try { $this->mapper->removeGroup($group->getGID()); + // Group deletion removes all delegations for that GID + // all affected users need re-evaluation on their next request + $this->mapper->clearCache(); } catch (\Exception $exception) { $this->handleException($exception); } diff --git a/apps/settings/tests/Controller/DelegationControllerTest.php b/apps/settings/tests/Controller/DelegationControllerTest.php index c4cbe67466b37..8d838599e242c 100644 --- a/apps/settings/tests/Controller/DelegationControllerTest.php +++ b/apps/settings/tests/Controller/DelegationControllerTest.php @@ -6,7 +6,6 @@ */ namespace OCA\Settings\Tests\Controller\Admin; -use OC\Settings\AuthorizedGroup; use OCA\Settings\Controller\AuthorizedGroupController; use OCA\Settings\Service\AuthorizedGroupService; use OCP\IRequest; @@ -28,26 +27,14 @@ protected function setUp(): void { } public function testSaveSettings(): void { - $setting = 'MySecretSetting'; - $oldGroups = []; - $oldGroups[] = AuthorizedGroup::fromParams(['groupId' => 'hello', 'class' => $setting]); - $goodbye = AuthorizedGroup::fromParams(['groupId' => 'goodbye', 'class' => $setting, 'id' => 42]); - $oldGroups[] = $goodbye; - $this->service->expects($this->once()) - ->method('findExistingGroupsForClass') - ->with('MySecretSetting') - ->willReturn($oldGroups); - - $this->service->expects($this->once()) - ->method('delete') - ->with(42); + $newGroups = [['gid' => 'hello'], ['gid' => 'world']]; + // The controller delegates the entire diff-and-apply to the service. $this->service->expects($this->once()) - ->method('create') - ->with('world', 'MySecretSetting') - ->willReturn(AuthorizedGroup::fromParams(['groupId' => 'world', 'class' => $setting])); + ->method('saveSettings') + ->with($newGroups, 'MySecretSetting'); - $result = $this->controller->saveSettings([['gid' => 'hello'], ['gid' => 'world']], 'MySecretSetting'); + $result = $this->controller->saveSettings($newGroups, 'MySecretSetting'); $this->assertEquals(['valid' => true], $result->getData()); } diff --git a/apps/settings/tests/Service/AuthorizedGroupServiceTest.php b/apps/settings/tests/Service/AuthorizedGroupServiceTest.php index 9d8ca16f94eb7..51efdcc4a711a 100644 --- a/apps/settings/tests/Service/AuthorizedGroupServiceTest.php +++ b/apps/settings/tests/Service/AuthorizedGroupServiceTest.php @@ -155,4 +155,133 @@ public function testCreateAllowsSameGroupDifferentClasses(): void { $this->assertEquals($class1, $result1->getClass()); $this->assertEquals($class2, $result2->getClass()); } + + public function testCreateClearsCacheAfterSuccessfulInsertion(): void { + $this->mapper->method('findByGroupIdAndClass') + ->willThrowException(new DoesNotExistException('not found')); + + $inserted = new AuthorizedGroup(); + $inserted->setGroupId('admins'); + $inserted->setClass('OCA\Settings\Admin\Security'); + $inserted->setId(1); + $this->mapper->method('insert')->willReturn($inserted); + + // Cache must be cleared after a new delegation is persisted so that + // every member of the group sees updated access on their next request. + $this->mapper->expects($this->once())->method('clearCache'); + + $this->service->create('admins', 'OCA\Settings\Admin\Security'); + } + + public function testCreateDoesNotClearCacheWhenConflictExceptionIsThrown(): void { + $existing = new AuthorizedGroup(); + $existing->setGroupId('admins'); + $existing->setClass('OCA\Settings\Admin\Security'); + $this->mapper->method('findByGroupIdAndClass')->willReturn($existing); + + // No DB write occurs, so the cache must not be invalidated. + $this->mapper->expects($this->never())->method('clearCache'); + + $this->expectException(ConflictException::class); + $this->service->create('admins', 'OCA\Settings\Admin\Security'); + } + + public function testDeleteClearsCacheAfterSuccessfulDeletion(): void { + $group = new AuthorizedGroup(); + $group->setId(1); + $this->mapper->method('find')->with(1)->willReturn($group); + + // Revoking a delegation must take effect immediately + $this->mapper->expects($this->once())->method('clearCache'); + + $this->service->delete(1); + } + + public function testRemoveAuthorizationAssociatedToClearsCacheOnGroupDeletion(): void { + $iGroup = $this->createMock(\OCP\IGroup::class); + $iGroup->method('getGID')->willReturn('devs'); + + // All delegations for the GID are removed + $this->mapper->expects($this->once())->method('clearCache'); + + $this->service->removeAuthorizationAssociatedTo($iGroup); + } + + public function testSaveSettingsFlushesExactlyOnce(): void { + // Current state: group 'admins' is authorized. + $existing = new AuthorizedGroup(); + $existing->setId(1); + $existing->setGroupId('admins'); + $existing->setClass('OCA\Settings\Admin\Security'); + + $this->mapper->method('findExistingGroupsForClass') + ->willReturn([$existing]); + + // New state: 'admins' removed, 'editors' added. + // findByGroupIdAndClass must throw so the insert proceeds. + $this->mapper->method('findByGroupIdAndClass') + ->willThrowException(new DoesNotExistException('not found')); + + // Key assertion: clearCache() called exactly once for the two mutations. + $this->mapper->expects($this->once())->method('clearCache'); + $this->mapper->expects($this->once())->method('delete')->with($existing); + $this->mapper->expects($this->once())->method('insert'); + + $this->service->saveSettings([['gid' => 'editors']], 'OCA\Settings\Admin\Security'); + } + + public function testSaveSettingsDoesNotCallMapperFindForDeletion(): void { + // Verifies that deletion uses the already-hydrated entity from + // findExistingGroupsForClass() without an extra mapper->find() call. + $existing = new AuthorizedGroup(); + $existing->setId(42); + $existing->setGroupId('devs'); + $existing->setClass('OCA\Settings\Admin\Users'); + + $this->mapper->method('findExistingGroupsForClass')->willReturn([$existing]); + $this->mapper->method('findByGroupIdAndClass') + ->willThrowException(new DoesNotExistException('not found')); + + // mapper->find() must NEVER be called + $this->mapper->expects($this->never())->method('find'); + $this->mapper->expects($this->once())->method('delete')->with($existing); + $this->mapper->expects($this->once())->method('clearCache'); + + // Remove 'devs', add nothing. + $this->service->saveSettings([], 'OCA\Settings\Admin\Users'); + } + + public function testSaveSettingsHandlesConcurrentInsertViaUniqueConstraint(): void { + // Simulates a concurrent insert: another process inserted the row between + // our snapshot read and our insert attempt. The DB unique constraint fires + // and we must treat it as idempotent (no exception propagated). + $this->mapper->method('findExistingGroupsForClass')->willReturn([]); + + // OCP\DB\Exception base class has getReason() + // overrides it. Use a mock so getReason() returns the expected reason code. + $uniqueViolation = $this->createMock(\OCP\DB\Exception::class); + $uniqueViolation->method('getReason') + ->willReturn(\OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); + $this->mapper->method('insert')->willThrowException($uniqueViolation); + + // clearCache() must still be called + $this->mapper->expects($this->once())->method('clearCache'); + + // Must not throw, unique constraint violation is handled as idempotent. + $this->service->saveSettings([['gid' => 'ops']], 'OCA\Settings\Admin\Security'); + } + + public function testSaveSettingsPropagatesNonUniqueDbExceptions(): void { + $this->mapper->method('findExistingGroupsForClass')->willReturn([]); + + // Use a mock so getReason() returns a non-unique reason code, causing + // saveSettings() to re-throw the exception. + $otherDbError = $this->createMock(\OCP\DB\Exception::class); + $otherDbError->method('getReason') + ->willReturn(\OCP\DB\Exception::REASON_CONNECTION_LOST); + $this->mapper->method('insert')->willThrowException($otherDbError); + + $this->expectException(\OCP\DB\Exception::class); + $this->service->saveSettings([['gid' => 'editors']], 'OCA\Settings\Admin\Security'); + } } diff --git a/core/Migrations/Version34000Date20260331000000.php b/core/Migrations/Version34000Date20260331000000.php new file mode 100644 index 0000000000000..24b3df45b0141 --- /dev/null +++ b/core/Migrations/Version34000Date20260331000000.php @@ -0,0 +1,131 @@ +connection->getQueryBuilder(); + + // Fetch all rows ordered so that within each (group_id, class) pair the + // lowest id comes first — the first occurrence is the one we keep. + $result = $qb->select('id', 'group_id', 'class') + ->from('authorized_groups') + ->orderBy('group_id') + ->addOrderBy('class') + ->addOrderBy('id') + ->executeQuery(); + + /** @var array $seen */ + $seen = []; + /** @var list $idsToDelete */ + $idsToDelete = []; + + while ($row = $result->fetch()) { + // Use NUL byte as separator — group_id and class are both max 200 chars + // of arbitrary text, so we need a separator that cannot appear in either. + $key = $row['group_id'] . "\0" . $row['class']; + if (isset($seen[$key])) { + $idsToDelete[] = (int)$row['id']; + } else { + $seen[$key] = true; + } + } + $result->closeCursor(); + + if ($idsToDelete === []) { + return; + } + + $output->info(sprintf( + 'authorized_groups: removing %d duplicate row(s) before adding unique index.', + count($idsToDelete) + )); + + // Delete in chunks of 1000 to stay within query-parameter limits across + // all supported database engines. + foreach (array_chunk($idsToDelete, 1000) as $chunk) { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('authorized_groups') + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))) + ->executeStatement(); + } + } + + /** + * Add the UNIQUE index on (group_id, class) and drop the superseded + * plain index on group_id. + */ + #[Override] + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + if (!$schema->hasTable('authorized_groups')) { + return null; + } + + $table = $schema->getTable('authorized_groups'); + + // Idempotency guard — safe to run twice (e.g. after a failed upgrade retry). + if ($table->hasIndex('admindel_group_class_uniq')) { + return null; + } + + // The original index on group_id alone is superseded by the new unique + // index on (group_id, class): any lookup by group_id will use the + // leftmost column of the composite index. + if ($table->hasIndex('admindel_groupid_idx')) { + $table->dropIndex('admindel_groupid_idx'); + } + + $table->addUniqueIndex(['group_id', 'class'], 'admindel_group_class_uniq'); + + return $schema; + } +} diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 35992e16837d2..ea173f55f4981 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1597,6 +1597,7 @@ 'OC\\Core\\Migrations\\Version33000Date20251209123503' => $baseDir . '/core/Migrations/Version33000Date20251209123503.php', 'OC\\Core\\Migrations\\Version33000Date20260126120000' => $baseDir . '/core/Migrations/Version33000Date20260126120000.php', 'OC\\Core\\Migrations\\Version34000Date20260318095645' => $baseDir . '/core/Migrations/Version34000Date20260318095645.php', + 'OC\\Core\\Migrations\\Version34000Date20260331000000' => $baseDir . '/core/Migrations/Version34000Date20260331000000.php', 'OC\\Core\\Notification\\CoreNotifier' => $baseDir . '/core/Notification/CoreNotifier.php', 'OC\\Core\\ResponseDefinitions' => $baseDir . '/core/ResponseDefinitions.php', 'OC\\Core\\Service\\CronService' => $baseDir . '/core/Service/CronService.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 79c4de8f32767..7e555db71073d 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1638,6 +1638,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Migrations\\Version33000Date20251209123503' => __DIR__ . '/../../..' . '/core/Migrations/Version33000Date20251209123503.php', 'OC\\Core\\Migrations\\Version33000Date20260126120000' => __DIR__ . '/../../..' . '/core/Migrations/Version33000Date20260126120000.php', 'OC\\Core\\Migrations\\Version34000Date20260318095645' => __DIR__ . '/../../..' . '/core/Migrations/Version34000Date20260318095645.php', + 'OC\\Core\\Migrations\\Version34000Date20260331000000' => __DIR__ . '/../../..' . '/core/Migrations/Version34000Date20260331000000.php', 'OC\\Core\\Notification\\CoreNotifier' => __DIR__ . '/../../..' . '/core/Notification/CoreNotifier.php', 'OC\\Core\\ResponseDefinitions' => __DIR__ . '/../../..' . '/core/ResponseDefinitions.php', 'OC\\Core\\Service\\CronService' => __DIR__ . '/../../..' . '/core/Service/CronService.php', diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 2a7b058fd51cf..85776b39057ba 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -60,6 +60,14 @@ class SecurityMiddleware extends Middleware { private ?bool $isAdminUser = null; private ?bool $isSubAdmin = null; + /** + * Lazily-resolved list of authorized admin-setting classes for the current + * user. Null means not yet resolved; an empty array means resolved but empty. + * + * @var list|null + */ + private ?array $authorizedClassesCache = null; + public function __construct( private readonly IRequest $request, private readonly MiddlewareUtils $middlewareUtils, @@ -94,6 +102,25 @@ private function isSubAdmin(): bool { return $this->isSubAdmin; } + /** + * Returns the setting classes the current user is delegated access to, + * memoizing the result for the lifetime of this middleware instance + * (i.e., the current request). + * + * @return list + * @since 34.0.0 + * @internal + */ + private function getAuthorizedClasses(): array { + if ($this->authorizedClassesCache === null) { + $user = $this->userSession->getUser(); + $this->authorizedClassesCache = $user !== null + ? $this->groupAuthorizationMapper->findAllClassesForUser($user) + : []; + } + return $this->authorizedClassesCache; + } + /** * This runs all the security checks before a method call. The * security checks are determined by inspecting the controller method @@ -146,7 +173,7 @@ public function beforeController($controller, $methodName) { if (!$authorized) { $settingClasses = $this->middlewareUtils->getAuthorizedAdminSettingClasses($reflectionMethod); - $authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser()); + $authorizedClasses = $this->getAuthorizedClasses(); foreach ($settingClasses as $settingClass) { $authorized = in_array($settingClass, $authorizedClasses, true); diff --git a/lib/private/Settings/AuthorizedGroupMapper.php b/lib/private/Settings/AuthorizedGroupMapper.php index 800dc5b80770c..ea520472a0f8b 100644 --- a/lib/private/Settings/AuthorizedGroupMapper.php +++ b/lib/private/Settings/AuthorizedGroupMapper.php @@ -13,6 +13,8 @@ use OCP\AppFramework\Db\QBMapper; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IDBConnection; use OCP\IGroup; use OCP\IGroupManager; @@ -23,35 +25,118 @@ * @template-extends QBMapper */ class AuthorizedGroupMapper extends QBMapper { - public function __construct(IDBConnection $db) { + + private const CACHE_PREFIX = 'nc_authorized_group_'; + + private const CACHE_TTL_DISTRIBUTED = 300; + + private const CACHE_TTL_LOCAL = 60; + + private readonly ICache $distributedCache; + + private readonly ICache $localCache; + + /** + * @param IDBConnection $db The database connection + * @param ICacheFactory $cacheFactory Factory to create the cache instances + */ + public function __construct( + IDBConnection $db, + private readonly ICacheFactory $cacheFactory, + ) { parent::__construct($db, 'authorized_groups', AuthorizedGroup::class); + // Distributed so multiple nodes share the same cached delegation map. + $this->distributedCache = $this->cacheFactory->createDistributed(self::CACHE_PREFIX); + // Per-process shield: absorbs burst traffic on a single node without + // hitting the distributed cache (and network) on every request. + $this->localCache = $this->cacheFactory->createLocal(self::CACHE_PREFIX); } /** - * @throws Exception + * Returns all setting class names that the given user is authorized to + * access via group delegation. + * + * Uses a two-tier cache strategy: + * 1. Per-process local cache (TTL: {@see self::CACHE_TTL_LOCAL} s) — shields + * the distributed cache from burst traffic within a single node. + * 2. Distributed cache (TTL: {@see self::CACHE_TTL_DISTRIBUTED} s) — shared + * across all cluster nodes; populated on cold path, backfilled to local + * tier on hit to short-circuit subsequent intra-process calls. + * + * @return list Fully-qualified class names of authorized settings + * @throws Exception When the database query fails */ public function findAllClassesForUser(IUser $user): array { - $qb = $this->db->getQueryBuilder(); + $uid = $user->getUID(); + $cacheKey = 'user_' . $uid; + + /** @var list|null $local */ + $local = $this->localCache->get($cacheKey); + if ($local !== null) { + return $local; + } + + /** @var list|null $distributed */ + $distributed = $this->distributedCache->get($cacheKey); + if ($distributed !== null) { + // Backfill local tier to short-circuit future intra-process calls. + $this->localCache->set($cacheKey, $distributed, self::CACHE_TTL_LOCAL); + return $distributed; + } $groupManager = Server::get(IGroupManager::class); $groups = $groupManager->getUserGroups($user); if (count($groups) === 0) { + // Cache empty result to avoid repeated backend calls for users + // who belong to no groups. + $this->localCache->set($cacheKey, [], self::CACHE_TTL_LOCAL); + $this->distributedCache->set($cacheKey, [], self::CACHE_TTL_DISTRIBUTED); return []; } + $qb = $this->db->getQueryBuilder(); + /** @var list $rows */ $rows = $qb->select('class') ->from($this->getTableName(), 'auth') - ->where($qb->expr()->in('group_id', array_map(static fn (IGroup $group) => $qb->createNamedParameter($group->getGID()), $groups), IQueryBuilder::PARAM_STR)) + ->where($qb->expr()->in( + 'group_id', + array_map( + static fn (IGroup $group) => $qb->createNamedParameter($group->getGID()), + $groups + ), + IQueryBuilder::PARAM_STR + )) ->executeQuery() ->fetchFirstColumn(); + $this->localCache->set($cacheKey, $rows, self::CACHE_TTL_LOCAL); + $this->distributedCache->set($cacheKey, $rows, self::CACHE_TTL_DISTRIBUTED); + return $rows; } /** - * @throws DoesNotExistException - * @throws MultipleObjectsReturnedException + * Clears the cached authorized-classes entry for a specific user. + * + * Must be called whenever a group delegation is added or removed so that + * the next request re-evaluates the user's authorizations. + */ + public function clearUserCache(string $userId): void { + $key = 'user_' . $userId; + $this->localCache->remove($key); + $this->distributedCache->remove($key); + } + + /** + * Clears all cached authorized-classes entries across both cache tiers. + */ + public function clearCache(): void { + $this->localCache->clear(); + $this->distributedCache->clear(); + } + + /** * @throws Exception */ public function find(int $id): AuthorizedGroup { diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 4c824b2aa55d2..efac46eca560a 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -1351,10 +1351,20 @@ public function userDeletedFromGroup($uid, $gid) { $userGroups = $this->groupManager->getUserGroupIds($user); $userGroups = array_diff($userGroups, $this->shareManager->shareWithGroupMembersOnlyExcludeGroupsList()); + // Method-local cache: avoids repeated backend lookups for the same + // UID across both loops. Uses array_key_exists() (not isset()) so + // that UIDs resolving to null are cached and not re-queried. + /** @var array $userCache */ + $userCache = []; + // Delete user shares received by the user from users in the group. $userReceivedShares = $this->shareManager->getSharedWith($uid, IShare::TYPE_USER, null, -1); foreach ($userReceivedShares as $share) { - $owner = $this->userManager->get($share->getSharedBy()); + $ownerUid = $share->getSharedBy(); + if (!array_key_exists($ownerUid, $userCache)) { + $userCache[$ownerUid] = $this->userManager->get($ownerUid); + } + $owner = $userCache[$ownerUid]; if ($owner === null) { continue; } @@ -1369,7 +1379,11 @@ public function userDeletedFromGroup($uid, $gid) { // Delete user shares from the user to users in the group. $userEmittedShares = $this->shareManager->getSharesBy($uid, IShare::TYPE_USER, null, true, -1); foreach ($userEmittedShares as $share) { - $recipient = $this->userManager->get($share->getSharedWith()); + $recipientUid = $share->getSharedWith(); + if (!array_key_exists($recipientUid, $userCache)) { + $userCache[$recipientUid] = $this->userManager->get($recipientUid); + } + $recipient = $userCache[$recipientUid]; if ($recipient === null) { continue; } @@ -1423,6 +1437,13 @@ public function getAccessList($nodes, $currentAccess) { $users = []; $link = false; + // Deduplicate group->getUsers() calls within this invocation. + // A group can appear multiple times when multiple nodes in $nodes are + // covered by the same group share (e.g. nested folder tree). + // getUsers() on LDAP / large DB backends is expensive; cache per GID. + /** @var array> $groupUsersCache */ + $groupUsersCache = []; + while ($row = $cursor->fetch()) { $type = (int)$row['share_type']; if ($type === IShare::TYPE_USER) { @@ -1431,14 +1452,14 @@ public function getAccessList($nodes, $currentAccess) { $users[$uid][$row['id']] = $row; } elseif ($type === IShare::TYPE_GROUP) { $gid = $row['share_with']; - $group = $this->groupManager->get($gid); - if ($group === null) { - continue; + if (!isset($groupUsersCache[$gid])) { + $group = $this->groupManager->get($gid); + // Null means the group was deleted after the share was created. + $groupUsersCache[$gid] = $group?->getUsers() ?? []; } - $userList = $group->getUsers(); - foreach ($userList as $user) { + foreach ($groupUsersCache[$gid] as $user) { $uid = $user->getUID(); $users[$uid] = $users[$uid] ?? []; $users[$uid][$row['id']] = $row; diff --git a/tests/lib/AppFramework/Middleware/Security/Mock/SecurityMiddlewareController.php b/tests/lib/AppFramework/Middleware/Security/Mock/SecurityMiddlewareController.php index c8f9878b0c107..e4177a4b70cb8 100644 --- a/tests/lib/AppFramework/Middleware/Security/Mock/SecurityMiddlewareController.php +++ b/tests/lib/AppFramework/Middleware/Security/Mock/SecurityMiddlewareController.php @@ -10,6 +10,7 @@ namespace Test\AppFramework\Middleware\Security\Mock; use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; use OCP\AppFramework\Http\Attribute\ExAppRequired; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; @@ -168,4 +169,9 @@ public function testAnnotationExAppRequired() { #[ExAppRequired] public function testAttributeExAppRequired() { } + + #[AuthorizedAdminSetting(settings: 'OCA\Settings\Admin\Security')] + #[NoCSRFRequired] + public function testAttributeAuthorizedAdminSetting(): void { + } } diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index fbb76573324c9..cb8cf078289ad 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -410,12 +410,6 @@ public static function dataCsrfOcsController(): array { ]; } - /** - * @param string $controllerClass - * @param bool $hasOcsApiHeader - * @param bool $hasBearerAuth - * @param bool $exception - */ #[\PHPUnit\Framework\Attributes\DataProvider('dataCsrfOcsController')] public function testCsrfOcsController(string $controllerClass, bool $hasOcsApiHeader, bool $hasBearerAuth, bool $exception): void { $this->request @@ -621,9 +615,6 @@ public static function exceptionProvider(): array { ]; } - /** - * @param SecurityException $exception - */ #[\PHPUnit\Framework\Attributes\DataProvider('exceptionProvider')] public function testAfterExceptionReturnsTemplateResponse(SecurityException $exception): void { $this->request = new Request( @@ -689,4 +680,66 @@ public function testExAppRequiredError(string $method): void { $this->expectException(ExAppRequiredException::class); $middleware->beforeController($this->controller, $method); } + + public function testFindAllClassesForUserIsCalledOnceWhenAuthorizedAdminSettingIsEvaluated(): void { + $middleware = $this->getMiddleware( + isLoggedIn: true, + isAdminUser: false, + isSubAdmin: false, + ); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('delegated_user'); + $this->userSession->method('getUser')->willReturn($user); + + // Key assertion: even if beforeController() is called twice on the + // same instance, findAllClassesForUser() must fire only once. + $this->authorizedGroupMapper + ->expects($this->once()) + ->method('findAllClassesForUser') + ->with($user) + ->willReturn(['OCA\Settings\Admin\Security']); + + $this->reader->reflect($this->controller, 'testAttributeAuthorizedAdminSetting'); + $middleware->beforeController($this->controller, 'testAttributeAuthorizedAdminSetting'); + // Second call on the same instance + $middleware->beforeController($this->controller, 'testAttributeAuthorizedAdminSetting'); + } + + public function testFindAllClassesForUserIsNotCalledWhenUserIsNull(): void { + $middleware = $this->getMiddleware( + isLoggedIn: false, + isAdminUser: false, + isSubAdmin: false, + ); + + $this->authorizedGroupMapper + ->expects($this->never()) + ->method('findAllClassesForUser'); + + $this->expectException(NotLoggedInException::class); + $this->reader->reflect($this->controller, 'testAttributeAuthorizedAdminSetting'); + $middleware->beforeController($this->controller, 'testAttributeAuthorizedAdminSetting'); + } + + /** + * An admin user takes the isAdminUser() early-return path and must never + * reach findAllClassesForUser(). + */ + public function testFindAllClassesForUserIsNotCalledWhenUserIsAlreadyAdmin(): void { + $middleware = $this->getMiddleware( + isLoggedIn: true, + isAdminUser: true, + isSubAdmin: false, + ); + + $this->authorizedGroupMapper + ->expects($this->never()) + ->method('findAllClassesForUser'); + + // Should not throw as admin is authorized unconditionally. + $this->reader->reflect($this->controller, 'testAttributeAuthorizedAdminSetting'); + $middleware->beforeController($this->controller, 'testAttributeAuthorizedAdminSetting'); + $this->addToAssertionCount(1); + } }