From 84192ac433ad2c0eb1108c16117ceab9c4c54f2d Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 22 Oct 2025 18:43:30 +0200 Subject: [PATCH 1/9] fix(performance): move array_merge outside of loop Signed-off-by: Anna Larch --- apps/provisioning_api/lib/Controller/UsersController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index ef88985fdafeb..c56821c9326e0 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -128,8 +128,9 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = $users = []; foreach ($subAdminOfGroups as $group) { - $users = array_merge($users, $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset)); + $users[] = $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset); } + $users = array_merge(...$users); } /** @var list $users */ From a63fd32d74ea98a76a5436320ca4e54483184cbb Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 28 Oct 2025 16:19:06 +0100 Subject: [PATCH 2/9] fixup! fix(performance): move array_merge outside of loop --- .../lib/Controller/UsersController.php | 87 +++++++++---------- 1 file changed, 40 insertions(+), 47 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index c56821c9326e0..c41b49fae88c2 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -36,6 +36,8 @@ use OCP\Files\IRootFolder; use OCP\Group\ISubAdmin; use OCP\HintException; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; @@ -60,6 +62,7 @@ class UsersController extends AUserDataOCSController { private IL10N $l10n; + private ICache $cache; public function __construct( string $appName, @@ -81,6 +84,7 @@ public function __construct( private IEventDispatcher $eventDispatcher, private IPhoneNumberUtil $phoneNumberUtil, private IAppManager $appManager, + private ICacheFactory $cacheFactory, ) { parent::__construct( $appName, @@ -96,6 +100,7 @@ public function __construct( ); $this->l10n = $l10nFactory->get($appName); + $this->cache = $this->cacheFactory->createDistributed('usercache'); } /** @@ -111,31 +116,46 @@ public function __construct( #[NoAdminRequired] public function getUsers(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { $user = $this->userSession->getUser(); - $users = []; - - // Admin? Or SubAdmin? $uid = $user->getUID(); - $subAdminManager = $this->groupManager->getSubAdmin(); + + $users = $this->cache->get($uid . '_' . $search); + if (!empty($users)) { + return new DataResponse([ + 'users' => $users + ]); + } + $isAdmin = $this->groupManager->isAdmin($uid); $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($uid); if ($isAdmin || $isDelegatedAdmin) { $users = $this->userManager->search($search, $limit, $offset); - } elseif ($subAdminManager->isSubAdmin($user)) { - $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($user); - foreach ($subAdminOfGroups as $key => $group) { - $subAdminOfGroups[$key] = $group->getGID(); - } + $users = array_keys($users); + $this->cache->set($uid . '_' . $search, $users, 10); + return new DataResponse([ + 'users' => $users + ]); + } - $users = []; - foreach ($subAdminOfGroups as $group) { - $users[] = $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset); - } - $users = array_merge(...$users); + $users = []; + $subAdminManager = $this->groupManager->getSubAdmin(); + if (!$subAdminManager->isSubAdmin($user)) { + return new DataResponse([ + 'users' => $users + ]); + } + + $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($user); + foreach ($subAdminOfGroups as $key => $group) { + $subAdminOfGroups[$key] = $group->getGID(); } - /** @var list $users */ - $users = array_keys($users); + foreach ($subAdminOfGroups as $group) { + foreach ($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset) as $uid => $user) { + $users[] = $uid; + } + } + $this->cache->set($uid . '_' . $search, $users, 10); return new DataResponse([ 'users' => $users ]); @@ -153,29 +173,7 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = */ #[NoAdminRequired] public function getUsersDetails(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { - $currentUser = $this->userSession->getUser(); - $users = []; - - // Admin? Or SubAdmin? - $uid = $currentUser->getUID(); - $subAdminManager = $this->groupManager->getSubAdmin(); - $isAdmin = $this->groupManager->isAdmin($uid); - $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($uid); - if ($isAdmin || $isDelegatedAdmin) { - $users = $this->userManager->search($search, $limit, $offset); - $users = array_keys($users); - } elseif ($subAdminManager->isSubAdmin($currentUser)) { - $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($currentUser); - foreach ($subAdminOfGroups as $key => $group) { - $subAdminOfGroups[$key] = $group->getGID(); - } - - $users = []; - foreach ($subAdminOfGroups as $group) { - $users[] = array_keys($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset)); - } - $users = array_merge(...$users); - } + $users = $this->getUsers($search, $limit, $offset)->getData()['users']; $usersDetails = []; foreach ($users as $userId) { @@ -188,14 +186,9 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of $userData = null; $this->logger->warning('Found one enabled account that is removed from its backend, but still exists in Nextcloud database', ['accountId' => $userId]); } - // Do not insert empty entry - if ($userData !== null) { - $usersDetails[$userId] = $userData; - } else { - // Logged user does not have permissions to see this user - // only showing its id - $usersDetails[$userId] = ['id' => $userId]; - } + + // $userdata === null means logged user does not have permissions to see this user + $usersDetails[$userId] = $userData ?? ['id' => $userId]; } return new DataResponse([ From 0d172165b4957652606ed9bd939429efc63df432 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 28 Oct 2025 16:43:57 +0100 Subject: [PATCH 3/9] fixup! fix(performance): move array_merge outside of loop --- .../lib/Controller/UsersController.php | 104 +++++++++--------- 1 file changed, 54 insertions(+), 50 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index c41b49fae88c2..19c9d03b9aca0 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -64,6 +64,8 @@ class UsersController extends AUserDataOCSController { private IL10N $l10n; private ICache $cache; + private const CACHE_PREFIX = 'provisioning_usercache'; + public function __construct( string $appName, IRequest $request, @@ -100,7 +102,7 @@ public function __construct( ); $this->l10n = $l10nFactory->get($appName); - $this->cache = $this->cacheFactory->createDistributed('usercache'); + $this->cache = $this->cacheFactory->createDistributed(self::CACHE_PREFIX); } /** @@ -814,6 +816,7 @@ public function editUserMultiValue( throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); $subAdminManager = $this->groupManager->getSubAdmin(); $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID()); $isAdminOrSubadmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()) @@ -914,6 +917,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); $permittedFields = []; if ($targetUser->getUID() === $currentLoggedInUser->getUID()) { if ($targetUser->canChangeDisplayName()) { @@ -1289,6 +1293,7 @@ public function deleteUser(string $userId): DataResponse { throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); // Go ahead with the delete if ($targetUser->delete()) { return new DataResponse(); @@ -1335,7 +1340,6 @@ public function enableUser(string $userId): DataResponse { */ private function setEnabled(string $userId, bool $value): DataResponse { $currentLoggedInUser = $this->userSession->getUser(); - $targetUser = $this->userManager->get($userId); if ($targetUser === null || $targetUser->getUID() === $currentLoggedInUser->getUID()) { throw new OCSException('', 101); @@ -1349,6 +1353,7 @@ private function setEnabled(string $userId, bool $value): DataResponse { throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); // enable/disable the user now $targetUser->setEnabled($value); return new DataResponse(); @@ -1381,22 +1386,21 @@ public function getUsersGroups(string $userId): DataResponse { return new DataResponse([ 'groups' => $this->groupManager->getUserGroupIds($targetUser) ]); - } else { - $subAdminManager = $this->groupManager->getSubAdmin(); + } - // Looking up someone else - if ($subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { - // Return the group that the method caller is subadmin of for the user in question - $groups = array_values(array_intersect( - array_map(static fn (IGroup $group) => $group->getGID(), $subAdminManager->getSubAdminsGroups($loggedInUser)), - $this->groupManager->getUserGroupIds($targetUser) - )); - return new DataResponse(['groups' => $groups]); - } else { - // Not permitted - throw new OCSException('', OCSController::RESPOND_NOT_FOUND); - } + $subAdminManager = $this->groupManager->getSubAdmin(); + + // Looking up someone else + if (!$subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { + throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + + // Return the group that the method caller is subadmin of for the user in question + $groups = array_values(array_intersect( + array_map(static fn (IGroup $group) => $group->getGID(), $subAdminManager->getSubAdminsGroups($loggedInUser)), + $this->groupManager->getUserGroupIds($targetUser) + )); + return new DataResponse(['groups' => $groups]); } /** @@ -1439,41 +1443,41 @@ function (Group $group) { return new DataResponse([ 'groups' => $groups, ]); - } else { - $subAdminManager = $this->groupManager->getSubAdmin(); + } - // Looking up someone else - if ($subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { - // Return the group that the method caller is subadmin of for the user in question - $gids = array_values(array_intersect( - array_map( - static fn (IGroup $group) => $group->getGID(), - $subAdminManager->getSubAdminsGroups($loggedInUser), - ), - $this->groupManager->getUserGroupIds($targetUser) - )); - $groups = array_map( - function (string $gid) { - $group = $this->groupManager->get($gid); - return [ - 'id' => $group->getGID(), - 'displayname' => $group->getDisplayName(), - 'usercount' => $group->count(), - 'disabled' => $group->countDisabled(), - 'canAdd' => $group->canAddUser(), - 'canRemove' => $group->canRemoveUser(), - ]; - }, - $gids, - ); - return new DataResponse([ - 'groups' => $groups, - ]); - } else { - // Not permitted - throw new OCSException('', OCSController::RESPOND_NOT_FOUND); - } + $subAdminManager = $this->groupManager->getSubAdmin(); + + // Looking up someone else + if (!$subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { + // Not permitted + throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + + // Return the group that the method caller is subadmin of for the user in question + $gids = array_values(array_intersect( + array_map( + static fn (IGroup $group) => $group->getGID(), + $subAdminManager->getSubAdminsGroups($loggedInUser), + ), + $this->groupManager->getUserGroupIds($targetUser) + )); + $groups = array_map( + function (string $gid) { + $group = $this->groupManager->get($gid); + return [ + 'id' => $group->getGID(), + 'displayname' => $group->getDisplayName(), + 'usercount' => $group->count(), + 'disabled' => $group->countDisabled(), + 'canAdd' => $group->canAddUser(), + 'canRemove' => $group->canRemoveUser(), + ]; + }, + $gids, + ); + return new DataResponse([ + 'groups' => $groups, + ]); } /** @@ -1575,7 +1579,7 @@ public function addToGroup(string $userId, string $groupid = ''): DataResponse { public function removeFromGroup(string $userId, string $groupid): DataResponse { $loggedInUser = $this->userSession->getUser(); - if ($groupid === null || trim($groupid) === '') { + if (trim($groupid) === '') { throw new OCSException('', 101); } From 3dde009c7b27f25f01c7ae6d148500eae2f174ec Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 25 Mar 2026 21:52:05 +0100 Subject: [PATCH 4/9] fixup! fix(performance): move array_merge outside of loop --- .../lib/AppInfo/Application.php | 6 + .../lib/Controller/AUserDataOCSController.php | 22 ++- .../lib/Controller/GroupsController.php | 3 + .../lib/Controller/UsersController.php | 29 ++-- .../lib/Listener/UserDataCacheListener.php | 46 ++++++ .../tests/Controller/GroupsControllerTest.php | 10 +- .../tests/Controller/UsersControllerTest.php | 139 ++++++++++++++++++ .../Listener/UserDataCacheListenerTest.php | 87 +++++++++++ 8 files changed, 322 insertions(+), 20 deletions(-) create mode 100644 apps/provisioning_api/lib/Listener/UserDataCacheListener.php create mode 100644 apps/provisioning_api/tests/Listener/UserDataCacheListenerTest.php diff --git a/apps/provisioning_api/lib/AppInfo/Application.php b/apps/provisioning_api/lib/AppInfo/Application.php index 9595dd4a60671..e18cb4d29ebc4 100644 --- a/apps/provisioning_api/lib/AppInfo/Application.php +++ b/apps/provisioning_api/lib/AppInfo/Application.php @@ -8,6 +8,7 @@ use OC\Group\Manager as GroupManager; use OCA\Provisioning_API\Capabilities; +use OCA\Provisioning_API\Listener\UserDataCacheListener; use OCA\Provisioning_API\Listener\UserDeletedListener; use OCA\Provisioning_API\Middleware\ProvisioningApiMiddleware; use OCA\Settings\Mailer\NewUserMailHelper; @@ -27,6 +28,8 @@ use OCP\Mail\IMailer; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; +use OCP\User\Events\PasswordUpdatedEvent; +use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserDeletedEvent; use OCP\Util; use Psr\Container\ContainerInterface; @@ -37,7 +40,10 @@ public function __construct(array $urlParams = []) { } public function register(IRegistrationContext $context): void { + $context->registerEventListener(UserChangedEvent::class, UserDataCacheListener::class); + $context->registerEventListener(UserDeletedEvent::class, UserDataCacheListener::class); $context->registerEventListener(UserDeletedEvent::class, UserDeletedListener::class); + $context->registerEventListener(PasswordUpdatedEvent::class, UserDataCacheListener::class); $context->registerService(NewUserMailHelper::class, function (ContainerInterface $c) { return new NewUserMailHelper( diff --git a/apps/provisioning_api/lib/Controller/AUserDataOCSController.php b/apps/provisioning_api/lib/Controller/AUserDataOCSController.php index 5f3f474ec7b7b..6469b537937ab 100644 --- a/apps/provisioning_api/lib/Controller/AUserDataOCSController.php +++ b/apps/provisioning_api/lib/Controller/AUserDataOCSController.php @@ -22,6 +22,8 @@ use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; use OCP\Group\ISubAdmin; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IRequest; use OCP\IUser; @@ -50,6 +52,10 @@ abstract class AUserDataOCSController extends OCSController { public const USER_FIELD_MANAGER = 'manager'; public const USER_FIELD_NOTIFICATION_EMAIL = 'notify_email'; + private const CACHE_TTL = 300; // 5 minutes + + private ICache $cache; + public function __construct( string $appName, IRequest $request, @@ -61,8 +67,10 @@ public function __construct( protected ISubAdmin $subAdminManager, protected IFactory $l10nFactory, protected IRootFolder $rootFolder, + ICacheFactory $cacheFactory, ) { parent::__construct($appName, $request); + $this->cache = $cacheFactory->createDistributed('provisioning_api'); } /** @@ -79,6 +87,16 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar $currentLoggedInUser = $this->userSession->getUser(); assert($currentLoggedInUser !== null, 'No user logged in'); + $isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()); + $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID()); + + $cacheKey = 'user_data_' . $userId . '_' . ($isAdmin || $isDelegatedAdmin ? 'admin' : 'noadmin') . ($includeScopes ? '_scoped' : ''); + /** @var array|null $cached */ + $cached = $this->cache->get($cacheKey); + if ($cached !== null) { + return $cached; + } + $data = []; // Check if the target user exists @@ -86,9 +104,6 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar if ($targetUserObject === null) { throw new OCSNotFoundException('User does not exist'); } - - $isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()); - $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID()); if ($isAdmin || $isDelegatedAdmin || $this->groupManager->getSubAdmin()->isUserAccessible($currentLoggedInUser, $targetUserObject)) { @@ -197,6 +212,7 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar 'setPassword' => $backend instanceof ISetPasswordBackend || $backend->implementsActions(Backend::SET_PASSWORD), ]; + $this->cache->set($cacheKey, $data, self::CACHE_TTL); return $data; } diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index 62f639b48e4e4..b1a2196ada4e1 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -23,6 +23,7 @@ use OCP\AppFramework\OCSController; use OCP\Files\IRootFolder; use OCP\Group\ISubAdmin; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; @@ -51,6 +52,7 @@ public function __construct( IFactory $l10nFactory, IRootFolder $rootFolder, private LoggerInterface $logger, + ICacheFactory $cacheFactory, ) { parent::__construct($appName, $request, @@ -62,6 +64,7 @@ public function __construct( $subAdminManager, $l10nFactory, $rootFolder, + $cacheFactory, ); } diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 19c9d03b9aca0..6ce97c52ad537 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -99,6 +99,7 @@ public function __construct( $subAdminManager, $l10nFactory, $rootFolder, + $cacheFactory, ); $this->l10n = $l10nFactory->get($appName); @@ -151,9 +152,9 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = $subAdminOfGroups[$key] = $group->getGID(); } - foreach ($subAdminOfGroups as $group) { - foreach ($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset) as $uid => $user) { - $users[] = $uid; + $users = []; + foreach ($subAdminOfGroups as $group) { + $users += $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset); } } @@ -234,25 +235,21 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, } elseif ($subAdminManager->isSubAdmin($currentUser)) { $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($currentUser); - $users = []; + $usersSet = []; /* We have to handle offset ourselve for correctness */ $tempLimit = ($limit === null ? null : $limit + $offset); foreach ($subAdminOfGroups as $group) { - $users = array_unique(array_merge( - $users, - array_map( - fn (IUser $user): string => $user->getUID(), - array_filter( - $group->searchUsers($search), - fn (IUser $user): bool => !$user->isEnabled() - ) - ) - )); - if (($tempLimit !== null) && (count($users) >= $tempLimit)) { + foreach (array_filter( + $group->searchUsers($search), + fn (IUser $user): bool => !$user->isEnabled() + ) as $user) { + $usersSet[$user->getUID()] = true; + } + if (($tempLimit !== null) && (count($usersSet) >= $tempLimit)) { break; } } - $users = array_slice($users, $offset, $limit); + $users = array_slice(array_keys($usersSet), $offset, $limit); } $usersDetails = []; diff --git a/apps/provisioning_api/lib/Listener/UserDataCacheListener.php b/apps/provisioning_api/lib/Listener/UserDataCacheListener.php new file mode 100644 index 0000000000000..08ec18969541b --- /dev/null +++ b/apps/provisioning_api/lib/Listener/UserDataCacheListener.php @@ -0,0 +1,46 @@ + + */ +class UserDataCacheListener implements IEventListener { + + private ICache $cache; + + public function __construct(ICacheFactory $cacheFactory) { + $this->cache = $cacheFactory->createDistributed('provisioning_api'); + } + + public function handle(Event $event): void { + if ($event instanceof UserChangedEvent) { + $uid = $event->getUser()->getUID(); + } elseif ($event instanceof UserDeletedEvent) { + $uid = $event->getUser()->getUID(); + } elseif ($event instanceof PasswordUpdatedEvent) { + $uid = $event->getUser()->getUID(); + } else { + return; + } + + // Clear all cached variants for this user (admin, noadmin, scoped, etc.) + $this->cache->clear('user_data_' . $uid); + } +} diff --git a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php index 129853d0c9bd2..848e33e0b868a 100644 --- a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php +++ b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php @@ -14,6 +14,8 @@ use OCP\AppFramework\OCS\OCSException; use OCP\Files\IRootFolder; use OCP\Group\ISubAdmin; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IGroup; use OCP\IRequest; @@ -38,6 +40,8 @@ class GroupsControllerTest extends \Test\TestCase { protected GroupsController&MockObject $api; private IRootFolder $rootFolder; + private ICache&MockObject $cache; + private ICacheFactory&MockObject $cacheFactory; protected function setUp(): void { @@ -53,6 +57,9 @@ protected function setUp(): void { $this->l10nFactory = $this->createMock(IFactory::class); $this->logger = $this->createMock(LoggerInterface::class); $this->rootFolder = $this->createMock(IRootFolder::class); + $this->cache = $this->createMock(ICache::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory->method('createDistributed')->willReturn($this->cache); $this->groupManager ->method('getSubAdmin') @@ -70,7 +77,8 @@ protected function setUp(): void { $this->subAdminManager, $this->l10nFactory, $this->rootFolder, - $this->logger + $this->logger, + $this->cacheFactory, ]) ->onlyMethods(['fillStorageInfo']) ->getMock(); diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index c135360b8f19e..461c8e50a7c03 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -26,6 +26,8 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\IRootFolder; use OCP\Group\ISubAdmin; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IGroup; use OCP\IL10N; @@ -65,6 +67,8 @@ class UsersControllerTest extends TestCase { private IRootFolder $rootFolder; private IPhoneNumberUtil $phoneNumberUtil; private IAppManager $appManager; + private ICache&MockObject $cache; + private ICacheFactory&MockObject $cacheFactory; protected function setUp(): void { parent::setUp(); @@ -87,6 +91,9 @@ protected function setUp(): void { $this->phoneNumberUtil = new PhoneNumberUtil(); $this->appManager = $this->createMock(IAppManager::class); $this->rootFolder = $this->createMock(IRootFolder::class); + $this->cache = $this->createMock(ICache::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory->method('createDistributed')->willReturn($this->cache); $l10n = $this->createMock(IL10N::class); $l10n->method('t')->willReturnCallback(fn (string $txt, array $replacement = []) => sprintf($txt, ...$replacement)); @@ -113,6 +120,7 @@ protected function setUp(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->cacheFactory, ]) ->onlyMethods(['fillStorageInfo']) ->getMock(); @@ -4383,6 +4391,137 @@ public function testGetEditableFields(bool $allowedToChangeDisplayName, bool $al $this->assertEquals($expectedResp, $this->api->getEditableFields('userId')); } + public function testGetUserDataReturnsCachedDataOnCacheHit(): void { + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser->method('getUID')->willReturn('admin'); + $this->userSession->method('getUser')->willReturn($loggedInUser); + $this->groupManager->method('isAdmin')->with('admin')->willReturn(true); + $this->groupManager->method('isDelegatedAdmin')->with('admin')->willReturn(false); + + $cachedData = ['id' => 'UID', 'displayname' => 'Test User', 'email' => 'test@example.com']; + $this->cache + ->method('get') + ->with('user_data_UID_admin') + ->willReturn($cachedData); + + // With a cache hit the database must not be consulted at all + $this->userManager->expects($this->never())->method('get'); + $this->accountManager->expects($this->never())->method('getAccount'); + + $result = $this->api->getUser('UID'); + $this->assertSame($cachedData, $result->getData()); + } + + public function testGetUserDataPopulatesCacheOnMiss(): void { + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser->method('getUID')->willReturn('admin'); + $this->userSession->method('getUser')->willReturn($loggedInUser); + $this->groupManager->method('isAdmin')->with('admin')->willReturn(true); + $this->groupManager->method('isDelegatedAdmin')->with('admin')->willReturn(false); + + // Cache miss + $this->cache->method('get')->willReturn(null); + + $targetUser = $this->createMock(IUser::class); + $targetUser->method('getUID')->willReturn('UID'); + $targetUser->method('getSystemEMailAddress')->willReturn('test@example.com'); + $targetUser->method('getPrimaryEMailAddress')->willReturn('test@example.com'); + $targetUser->method('getDisplayName')->willReturn('Test User'); + $targetUser->method('getLastLogin')->willReturn(0); + $targetUser->method('getFirstLogin')->willReturn(0); + $targetUser->method('getBackendClassName')->willReturn('Database'); + $targetUser->method('getBackend')->willReturn($this->createMock(\OCP\UserInterface::class)); + $targetUser->method('getHome')->willReturn('/home/UID'); + $targetUser->method('getManagerUids')->willReturn([]); + $targetUser->method('getQuota')->willReturn('none'); + $this->userManager->method('get')->with('UID')->willReturn($targetUser); + + $subAdminManager = $this->createMock(\OC\SubAdmin::class); + $subAdminManager->method('getSubAdminsGroups')->willReturn([]); + $this->groupManager->method('getSubAdmin')->willReturn($subAdminManager); + $this->groupManager->method('getUserGroups')->willReturn([]); + + $this->mockAccount($targetUser, [ + IAccountManager::PROPERTY_ADDRESS => ['value' => ''], + IAccountManager::PROPERTY_PHONE => ['value' => ''], + IAccountManager::PROPERTY_TWITTER => ['value' => ''], + IAccountManager::PROPERTY_BLUESKY => ['value' => ''], + IAccountManager::PROPERTY_FEDIVERSE => ['value' => ''], + IAccountManager::PROPERTY_WEBSITE => ['value' => ''], + IAccountManager::PROPERTY_ORGANISATION => ['value' => ''], + IAccountManager::PROPERTY_ROLE => ['value' => ''], + IAccountManager::PROPERTY_HEADLINE => ['value' => ''], + IAccountManager::PROPERTY_BIOGRAPHY => ['value' => ''], + IAccountManager::PROPERTY_PROFILE_ENABLED => ['value' => '0'], + IAccountManager::PROPERTY_PRONOUNS => ['value' => ''], + ]); + $emailCollection = $this->createMock(\OCP\Accounts\IAccountPropertyCollection::class); + $emailCollection->method('getProperties')->willReturn([]); + $account = $this->accountManager->getAccount($targetUser); + $this->accountManager->method('getAccount')->with($targetUser)->willReturnCallback(function () use ($targetUser, $emailCollection) { + $account = $this->createMock(\OCP\Accounts\IAccount::class); + $account->method('getPropertyCollection')->willReturn($emailCollection); + $account->method('getProperty')->willReturnCallback(function (string $name) { + $prop = $this->createMock(IAccountProperty::class); + $prop->method('getValue')->willReturn(''); + $prop->method('getScope')->willReturn(''); + return $prop; + }); + return $account; + }); + $this->config->method('getUserValue')->willReturn('true'); + $this->l10nFactory->method('getUserLanguage')->willReturn('en'); + $this->api->method('fillStorageInfo')->willReturn(['used' => 0, 'quota' => -3, 'free' => -3, 'total' => -3, 'relative' => 0]); + + // The computed data must be stored in the cache with the 5-minute TTL + $this->cache + ->expects($this->once()) + ->method('set') + ->with('user_data_UID_admin', $this->isArray(), 300); + + $this->api->getUser('UID'); + } + + public function testGetUserDataUsesAdminCacheKey(): void { + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser->method('getUID')->willReturn('admin'); + $this->userSession->method('getUser')->willReturn($loggedInUser); + $this->groupManager->method('isAdmin')->with('admin')->willReturn(true); + $this->groupManager->method('isDelegatedAdmin')->with('admin')->willReturn(false); + + $this->cache + ->expects($this->once()) + ->method('get') + ->with($this->stringContains('_admin')) + ->willReturn(['id' => 'UID']); + + $this->api->getUser('UID'); + } + + public function testGetUserDataUsesNonAdminCacheKey(): void { + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser->method('getUID')->willReturn('subadmin'); + $this->userSession->method('getUser')->willReturn($loggedInUser); + $this->groupManager->method('isAdmin')->with('subadmin')->willReturn(false); + $this->groupManager->method('isDelegatedAdmin')->with('subadmin')->willReturn(false); + + $targetUser = $this->createMock(IUser::class); + $targetUser->method('getUID')->willReturn('UID'); + $this->userManager->method('get')->with('UID')->willReturn($targetUser); + + $subAdminManager = $this->createMock(\OC\SubAdmin::class); + $subAdminManager->method('isUserAccessible')->with($loggedInUser, $targetUser)->willReturn(true); + $this->groupManager->method('getSubAdmin')->willReturn($subAdminManager); + + $this->cache + ->expects($this->once()) + ->method('get') + ->with($this->stringContains('_noadmin')) + ->willReturn(['id' => 'UID']); + + $this->api->getUser('UID'); + } + private function mockAccount($targetUser, $accountProperties) { $mockedProperties = []; diff --git a/apps/provisioning_api/tests/Listener/UserDataCacheListenerTest.php b/apps/provisioning_api/tests/Listener/UserDataCacheListenerTest.php new file mode 100644 index 0000000000000..2a870c56212f8 --- /dev/null +++ b/apps/provisioning_api/tests/Listener/UserDataCacheListenerTest.php @@ -0,0 +1,87 @@ +cache = $this->createMock(ICache::class); + $cacheFactory = $this->createMock(ICacheFactory::class); + $cacheFactory->method('createDistributed') + ->with('provisioning_api') + ->willReturn($this->cache); + + $this->listener = new UserDataCacheListener($cacheFactory); + } + + private function makeUser(string $uid): IUser&MockObject { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + return $user; + } + + public function testHandleUserChangedEventClearsCache(): void { + $user = $this->makeUser('alice'); + $event = new UserChangedEvent($user, 'displayName', 'New Name', 'Old Name'); + + $this->cache + ->expects($this->once()) + ->method('clear') + ->with('user_data_alice'); + + $this->listener->handle($event); + } + + public function testHandleUserDeletedEventClearsCache(): void { + $user = $this->makeUser('bob'); + $event = new UserDeletedEvent($user); + + $this->cache + ->expects($this->once()) + ->method('clear') + ->with('user_data_bob'); + + $this->listener->handle($event); + } + + public function testHandlePasswordUpdatedEventClearsCache(): void { + $user = $this->makeUser('carol'); + $event = new PasswordUpdatedEvent($user, 'newpassword'); + + $this->cache + ->expects($this->once()) + ->method('clear') + ->with('user_data_carol'); + + $this->listener->handle($event); + } + + public function testHandleUnrelatedEventDoesNothing(): void { + $this->cache->expects($this->never())->method('clear'); + $this->cache->expects($this->never())->method('remove'); + + $this->listener->handle(new Event()); + } +} From 06dd3ef32ee75e28236d38e2c0410fe0a29bcef9 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 25 Mar 2026 22:26:19 +0100 Subject: [PATCH 5/9] fixup! fix(performance): move array_merge outside of loop --- apps/provisioning_api/lib/Controller/UsersController.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 6ce97c52ad537..61c7e949e87b5 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -152,12 +152,12 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = $subAdminOfGroups[$key] = $group->getGID(); } - $users = []; - foreach ($subAdminOfGroups as $group) { - $users += $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset); - } + $users = []; + foreach ($subAdminOfGroups as $group) { + $users += $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset); } + $users = array_keys($users); $this->cache->set($uid . '_' . $search, $users, 10); return new DataResponse([ 'users' => $users From ed28046fa59a18d01d0947e9ecda8acec28824e5 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 25 Mar 2026 22:29:42 +0100 Subject: [PATCH 6/9] fixup! fix(performance): move array_merge outside of loop --- apps/provisioning_api/lib/Listener/UserDataCacheListener.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/provisioning_api/lib/Listener/UserDataCacheListener.php b/apps/provisioning_api/lib/Listener/UserDataCacheListener.php index 08ec18969541b..a7071eb9f26e2 100644 --- a/apps/provisioning_api/lib/Listener/UserDataCacheListener.php +++ b/apps/provisioning_api/lib/Listener/UserDataCacheListener.php @@ -10,8 +10,8 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; -use OCP\ICacheFactory; use OCP\ICache; +use OCP\ICacheFactory; use OCP\User\Events\PasswordUpdatedEvent; use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserDeletedEvent; From 08f9e63eff8bcd9c2aab8addb0e1162ca7060703 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 25 Mar 2026 22:30:41 +0100 Subject: [PATCH 7/9] fixup! fix(performance): move array_merge outside of loop --- apps/provisioning_api/composer/composer/autoload_classmap.php | 1 + apps/provisioning_api/composer/composer/autoload_static.php | 1 + 2 files changed, 2 insertions(+) diff --git a/apps/provisioning_api/composer/composer/autoload_classmap.php b/apps/provisioning_api/composer/composer/autoload_classmap.php index 7a007f4577d9e..1c1f47b75f015 100644 --- a/apps/provisioning_api/composer/composer/autoload_classmap.php +++ b/apps/provisioning_api/composer/composer/autoload_classmap.php @@ -17,6 +17,7 @@ 'OCA\\Provisioning_API\\Controller\\UsersController' => $baseDir . '/../lib/Controller/UsersController.php', 'OCA\\Provisioning_API\\Controller\\VerificationController' => $baseDir . '/../lib/Controller/VerificationController.php', 'OCA\\Provisioning_API\\FederatedShareProviderFactory' => $baseDir . '/../lib/FederatedShareProviderFactory.php', + 'OCA\\Provisioning_API\\Listener\\UserDataCacheListener' => $baseDir . '/../lib/Listener/UserDataCacheListener.php', 'OCA\\Provisioning_API\\Listener\\UserDeletedListener' => $baseDir . '/../lib/Listener/UserDeletedListener.php', 'OCA\\Provisioning_API\\Middleware\\Exceptions\\NotSubAdminException' => $baseDir . '/../lib/Middleware/Exceptions/NotSubAdminException.php', 'OCA\\Provisioning_API\\Middleware\\ProvisioningApiMiddleware' => $baseDir . '/../lib/Middleware/ProvisioningApiMiddleware.php', diff --git a/apps/provisioning_api/composer/composer/autoload_static.php b/apps/provisioning_api/composer/composer/autoload_static.php index 537203fb53d3a..bd5ce0ad63042 100644 --- a/apps/provisioning_api/composer/composer/autoload_static.php +++ b/apps/provisioning_api/composer/composer/autoload_static.php @@ -32,6 +32,7 @@ class ComposerStaticInitProvisioning_API 'OCA\\Provisioning_API\\Controller\\UsersController' => __DIR__ . '/..' . '/../lib/Controller/UsersController.php', 'OCA\\Provisioning_API\\Controller\\VerificationController' => __DIR__ . '/..' . '/../lib/Controller/VerificationController.php', 'OCA\\Provisioning_API\\FederatedShareProviderFactory' => __DIR__ . '/..' . '/../lib/FederatedShareProviderFactory.php', + 'OCA\\Provisioning_API\\Listener\\UserDataCacheListener' => __DIR__ . '/..' . '/../lib/Listener/UserDataCacheListener.php', 'OCA\\Provisioning_API\\Listener\\UserDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/UserDeletedListener.php', 'OCA\\Provisioning_API\\Middleware\\Exceptions\\NotSubAdminException' => __DIR__ . '/..' . '/../lib/Middleware/Exceptions/NotSubAdminException.php', 'OCA\\Provisioning_API\\Middleware\\ProvisioningApiMiddleware' => __DIR__ . '/..' . '/../lib/Middleware/ProvisioningApiMiddleware.php', From 928a61dec291b4ad18c086236b6557919a8f42fd Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 25 Mar 2026 22:35:26 +0100 Subject: [PATCH 8/9] fixup! fix(performance): move array_merge outside of loop --- .../lib/Controller/AUserDataOCSController.php | 2 +- build/psalm-baseline.xml | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AUserDataOCSController.php b/apps/provisioning_api/lib/Controller/AUserDataOCSController.php index 6469b537937ab..9ed25fa8ab5d6 100644 --- a/apps/provisioning_api/lib/Controller/AUserDataOCSController.php +++ b/apps/provisioning_api/lib/Controller/AUserDataOCSController.php @@ -91,7 +91,7 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID()); $cacheKey = 'user_data_' . $userId . '_' . ($isAdmin || $isDelegatedAdmin ? 'admin' : 'noadmin') . ($includeScopes ? '_scoped' : ''); - /** @var array|null $cached */ + /** @var Provisioning_APIUserDetails|null $cached */ $cached = $this->cache->get($cacheKey); if ($cached !== null) { return $cached; diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index fc0e97dcaba9e..a0d838ee37452 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2074,10 +2074,6 @@ - - - - From d1a0175eb33253997e2baffbb9d40695da9306f2 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 25 Mar 2026 22:49:38 +0100 Subject: [PATCH 9/9] fixup! fix(performance): move array_merge outside of loop --- .../lib/Controller/AUserDataOCSController.php | 12 ++++++------ .../tests/Controller/UsersControllerTest.php | 3 +++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AUserDataOCSController.php b/apps/provisioning_api/lib/Controller/AUserDataOCSController.php index 9ed25fa8ab5d6..d94eaf94ca601 100644 --- a/apps/provisioning_api/lib/Controller/AUserDataOCSController.php +++ b/apps/provisioning_api/lib/Controller/AUserDataOCSController.php @@ -87,6 +87,12 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar $currentLoggedInUser = $this->userSession->getUser(); assert($currentLoggedInUser !== null, 'No user logged in'); + // Check if the target user exists + $targetUserObject = $this->userManager->get($userId); + if ($targetUserObject === null) { + throw new OCSNotFoundException('User does not exist'); + } + $isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()); $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID()); @@ -98,12 +104,6 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar } $data = []; - - // Check if the target user exists - $targetUserObject = $this->userManager->get($userId); - if ($targetUserObject === null) { - throw new OCSNotFoundException('User does not exist'); - } if ($isAdmin || $isDelegatedAdmin || $this->groupManager->getSubAdmin()->isUserAccessible($currentLoggedInUser, $targetUserObject)) { diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 461c8e50a7c03..5c36cc94632ca 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -510,6 +510,7 @@ public function testAddUserSuccessfulWithDisplayName(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->cacheFactory, ]) ->onlyMethods(['editUser']) ->getMock(); @@ -3850,6 +3851,7 @@ public function testGetCurrentUserLoggedIn(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->cacheFactory, ]) ->onlyMethods(['getUserData']) ->getMock(); @@ -3944,6 +3946,7 @@ public function testGetUser(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->cacheFactory, ]) ->onlyMethods(['getUserData']) ->getMock();