From 1a4516305c83fc81e4ed36717eddd4b700ebd827 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Mon, 14 Sep 2026 10:02:12 +0200 Subject: [PATCH] perf(core): allow versioned avatar URLs to be cached for 30 days Signed-off-by: Peter Ringelmann --- core/AppInfo/Application.php | 4 ++ core/Controller/AvatarController.php | 37 ++++++++--- core/Listener/AvatarVersionListener.php | 49 +++++++++++++++ core/openapi-full.json | 18 ++++++ core/openapi.json | 18 ++++++ lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + lib/private/Avatar/AvatarManager.php | 47 ++++++++++---- lib/private/Avatar/AvatarVersion.php | 33 ++++++++++ lib/private/Avatar/PlaceholderAvatar.php | 3 + lib/private/Avatar/UserAvatar.php | 4 +- lib/private/Server.php | 3 +- openapi.json | 18 ++++++ .../Core/Controller/AvatarControllerTest.php | 25 +++++++- .../Listener/AvatarVersionListenerTest.php | 62 +++++++++++++++++++ tests/lib/Avatar/AvatarManagerTest.php | 45 ++++++++++++-- tests/lib/Avatar/PlaceholderAvatarTest.php | 53 ++++++++++++++++ tests/lib/Avatar/UserAvatarTest.php | 13 ++-- 18 files changed, 402 insertions(+), 34 deletions(-) create mode 100644 core/Listener/AvatarVersionListener.php create mode 100644 lib/private/Avatar/AvatarVersion.php create mode 100644 tests/Core/Listener/AvatarVersionListenerTest.php create mode 100644 tests/lib/Avatar/PlaceholderAvatarTest.php diff --git a/core/AppInfo/Application.php b/core/AppInfo/Application.php index 42a3a09f2625a..928a80d072dd0 100644 --- a/core/AppInfo/Application.php +++ b/core/AppInfo/Application.php @@ -21,6 +21,7 @@ use OC\Authentication\Notifications\Notifier as AuthenticationNotifier; use OC\Core\Listener\AddMissingIndicesListener; use OC\Core\Listener\AddMissingPrimaryKeyListener; +use OC\Core\Listener\AvatarVersionListener; use OC\Core\Listener\BeforeTemplateRenderedListener; use OC\Core\Listener\LoadAdditionalEntriesListener; use OC\Core\Listener\PasswordUpdatedListener; @@ -42,6 +43,7 @@ use OC\DirectEditing\Listeners\UserDisabledTokenCleanupListener as UserDisabledDirectEditingTokenCleanupListener; use OC\OCM\OCMDiscoveryHandler; use OC\TagManager; +use OCP\Accounts\UserUpdatedEvent; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; @@ -104,6 +106,8 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(UserDeletedEvent::class, UserDeletedFilesCleanupListener::class); $context->registerEventListener(UserDeletedEvent::class, UserDeletedWebAuthnCleanupListener::class); $context->registerEventListener(PasswordUpdatedEvent::class, PasswordUpdatedListener::class); + $context->registerEventListener(UserUpdatedEvent::class, AvatarVersionListener::class); + $context->registerEventListener(UserChangedEvent::class, AvatarVersionListener::class); // Tags $context->registerEventListener(UserDeletedEvent::class, TagManager::class); diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index c232dd140fc37..c074bf8f34996 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -9,6 +9,7 @@ namespace OC\Core\Controller; use OC\AppFramework\Utility\TimeFactory; +use OC\Avatar\AvatarManager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; @@ -23,7 +24,6 @@ use OCP\Files\File; use OCP\Files\IRootFolder; use OCP\Files\NotPermittedException; -use OCP\IAvatarManager; use OCP\IL10N; use OCP\Image; use OCP\IRequest; @@ -36,10 +36,19 @@ * @package OC\Core\Controller */ class AvatarController extends Controller { + private const CACHE_DEFAULT = 60 * 60 * 24; + + /** + * Long enough to span the gap between infrequent large calls, which is where + * the cost of refetching everyone's avatar lands. Not longer, because a + * cached avatar outlives the account it belongs to. + */ + private const CACHE_VERSIONED = 60 * 60 * 24 * 30; + public function __construct( string $appName, IRequest $request, - protected IAvatarManager $avatarManager, + protected AvatarManager $avatarManager, protected IL10N $l10n, protected IUserManager $userManager, protected IRootFolder $rootFolder, @@ -57,6 +66,7 @@ public function __construct( * @param string $userId ID of the user * @param 64|512 $size Size of the avatar * @param bool $guestFallback Fallback to guest avatar if not found + * @param string $v Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar * @return FileDisplayResponse|JSONResponse, array{}>|Response * * 200: Avatar returned @@ -68,7 +78,7 @@ public function __construct( #[FrontpageRoute(verb: 'GET', url: '/avatar/{userId}/{size}/dark')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] #[NoSameSiteCookieRequired] - public function getAvatarDark(string $userId, int $size, bool $guestFallback = false) { + public function getAvatarDark(string $userId, int $size, bool $guestFallback = false, string $v = '') { if ($size <= 64) { if ($size !== 64) { $this->logger->debug('Avatar requested in deprecated size ' . $size); @@ -96,8 +106,7 @@ public function getAvatarDark(string $userId, int $size, bool $guestFallback = f return new JSONResponse([], Http::STATUS_NOT_FOUND); } - // Cache for 1 day - $response->cacheFor(60 * 60 * 24, false, true); + $response->cacheFor($this->cacheSecondsFor($userId, $v), false, true); return $response; } @@ -107,6 +116,7 @@ public function getAvatarDark(string $userId, int $size, bool $guestFallback = f * @param string $userId ID of the user * @param 64|512 $size Size of the avatar * @param bool $guestFallback Fallback to guest avatar if not found + * @param string $v Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar * @return FileDisplayResponse|JSONResponse, array{}>|Response * * 200: Avatar returned @@ -118,7 +128,7 @@ public function getAvatarDark(string $userId, int $size, bool $guestFallback = f #[FrontpageRoute(verb: 'GET', url: '/avatar/{userId}/{size}')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] #[NoSameSiteCookieRequired] - public function getAvatar(string $userId, int $size, bool $guestFallback = false) { + public function getAvatar(string $userId, int $size, bool $guestFallback = false, string $v = '') { if ($size <= 64) { if ($size !== 64) { $this->logger->debug('Avatar requested in deprecated size ' . $size); @@ -146,11 +156,22 @@ public function getAvatar(string $userId, int $size, bool $guestFallback = false return new JSONResponse([], Http::STATUS_NOT_FOUND); } - // Cache for 1 day - $response->cacheFor(60 * 60 * 24, false, true); + $response->cacheFor($this->cacheSecondsFor($userId, $v), false, true); return $response; } + /** + * Decided here rather than by the caller: attaching a version to an avatar + * whose visibility depends on the viewer must not buy a month of caching. + */ + private function cacheSecondsFor(string $userId, string $version): int { + if ($version !== '' && $this->avatarManager->canCacheAvatarLongTerm($userId)) { + return self::CACHE_VERSIONED; + } + + return self::CACHE_DEFAULT; + } + #[NoAdminRequired] #[FrontpageRoute(verb: 'POST', url: '/avatar/')] public function postAvatar(?string $path = null): JSONResponse { diff --git a/core/Listener/AvatarVersionListener.php b/core/Listener/AvatarVersionListener.php new file mode 100644 index 0000000000000..5c9405f72b426 --- /dev/null +++ b/core/Listener/AvatarVersionListener.php @@ -0,0 +1,49 @@ + + */ +class AvatarVersionListener implements IEventListener { + public function __construct( + private AvatarVersion $avatarVersion, + ) { + } + + #[\Override] + public function handle(Event $event): void { + if ($event instanceof UserUpdatedEvent) { + $this->avatarVersion->bump($event->getUser()->getUID()); + return; + } + + if ($event instanceof UserChangedEvent && $event->getFeature() === 'enabled') { + $this->avatarVersion->bump($event->getUser()->getUID()); + } + } +} diff --git a/core/openapi-full.json b/core/openapi-full.json index 9324c67bc87e9..2362d1eb3ca20 100644 --- a/core/openapi-full.json +++ b/core/openapi-full.json @@ -9328,6 +9328,15 @@ "type": "boolean", "default": false } + }, + { + "name": "v", + "in": "query", + "description": "Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { @@ -9431,6 +9440,15 @@ "type": "boolean", "default": false } + }, + { + "name": "v", + "in": "query", + "description": "Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { diff --git a/core/openapi.json b/core/openapi.json index d6d2915708009..5d9ec8b95abfe 100644 --- a/core/openapi.json +++ b/core/openapi.json @@ -9328,6 +9328,15 @@ "type": "boolean", "default": false } + }, + { + "name": "v", + "in": "query", + "description": "Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { @@ -9431,6 +9440,15 @@ "type": "boolean", "default": false } + }, + { + "name": "v", + "in": "query", + "description": "Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 26372e14d14ae..5841141fbdc92 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1349,6 +1349,7 @@ 'OC\\Authentication\\WebAuthn\\Manager' => $baseDir . '/lib/private/Authentication/WebAuthn/Manager.php', 'OC\\Avatar\\Avatar' => $baseDir . '/lib/private/Avatar/Avatar.php', 'OC\\Avatar\\AvatarManager' => $baseDir . '/lib/private/Avatar/AvatarManager.php', + 'OC\\Avatar\\AvatarVersion' => $baseDir . '/lib/private/Avatar/AvatarVersion.php', 'OC\\Avatar\\GuestAvatar' => $baseDir . '/lib/private/Avatar/GuestAvatar.php', 'OC\\Avatar\\PlaceholderAvatar' => $baseDir . '/lib/private/Avatar/PlaceholderAvatar.php', 'OC\\Avatar\\RemoteAvatar' => $baseDir . '/lib/private/Avatar/RemoteAvatar.php', @@ -1634,6 +1635,7 @@ 'OC\\Core\\Exception\\ResetPasswordException' => $baseDir . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\AddMissingIndicesListener' => $baseDir . '/core/Listener/AddMissingIndicesListener.php', 'OC\\Core\\Listener\\AddMissingPrimaryKeyListener' => $baseDir . '/core/Listener/AddMissingPrimaryKeyListener.php', + 'OC\\Core\\Listener\\AvatarVersionListener' => $baseDir . '/core/Listener/AvatarVersionListener.php', 'OC\\Core\\Listener\\BeforeMessageLoggedEventListener' => $baseDir . '/core/Listener/BeforeMessageLoggedEventListener.php', 'OC\\Core\\Listener\\BeforeTemplateRenderedListener' => $baseDir . '/core/Listener/BeforeTemplateRenderedListener.php', 'OC\\Core\\Listener\\FeedBackHandler' => $baseDir . '/core/Listener/FeedBackHandler.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index f796225eb4d7e..e3ab46975387a 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1390,6 +1390,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Authentication\\WebAuthn\\Manager' => __DIR__ . '/../../..' . '/lib/private/Authentication/WebAuthn/Manager.php', 'OC\\Avatar\\Avatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/Avatar.php', 'OC\\Avatar\\AvatarManager' => __DIR__ . '/../../..' . '/lib/private/Avatar/AvatarManager.php', + 'OC\\Avatar\\AvatarVersion' => __DIR__ . '/../../..' . '/lib/private/Avatar/AvatarVersion.php', 'OC\\Avatar\\GuestAvatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/GuestAvatar.php', 'OC\\Avatar\\PlaceholderAvatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/PlaceholderAvatar.php', 'OC\\Avatar\\RemoteAvatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/RemoteAvatar.php', @@ -1675,6 +1676,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Exception\\ResetPasswordException' => __DIR__ . '/../../..' . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\AddMissingIndicesListener' => __DIR__ . '/../../..' . '/core/Listener/AddMissingIndicesListener.php', 'OC\\Core\\Listener\\AddMissingPrimaryKeyListener' => __DIR__ . '/../../..' . '/core/Listener/AddMissingPrimaryKeyListener.php', + 'OC\\Core\\Listener\\AvatarVersionListener' => __DIR__ . '/../../..' . '/core/Listener/AvatarVersionListener.php', 'OC\\Core\\Listener\\BeforeMessageLoggedEventListener' => __DIR__ . '/../../..' . '/core/Listener/BeforeMessageLoggedEventListener.php', 'OC\\Core\\Listener\\BeforeTemplateRenderedListener' => __DIR__ . '/../../..' . '/core/Listener/BeforeTemplateRenderedListener.php', 'OC\\Core\\Listener\\FeedBackHandler' => __DIR__ . '/../../..' . '/core/Listener/FeedBackHandler.php', diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 2f5008a7b21fb..13343ce9dec9e 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -22,6 +22,7 @@ use OCP\IAvatarManager; use OCP\IConfig; use OCP\IL10N; +use OCP\IUser; use OCP\IUserSession; use OCP\User\Exceptions\UserNotFoundException; use Psr\Log\LoggerInterface; @@ -40,6 +41,7 @@ public function __construct( private IAccountManager $accountManager, private KnownUserService $knownUserService, private ICloudIdManager $cloudIdManager, + private AvatarVersion $avatarVersion, ) { } @@ -79,31 +81,54 @@ public function getAvatar(string $userId): IAvatar { $folder = $this->appData->newFolder($userId); } - try { - $account = $this->accountManager->getAccount($user); - $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); - $avatarScope = $avatarProperties->getScope(); - } catch (PropertyDoesNotExistException $e) { - $avatarScope = ''; - } + $avatarScope = $this->getAvatarScope($user); switch ($avatarScope) { // v2-private scope hides the avatar from public access and from unknown users case IAccountManager::SCOPE_PRIVATE: if ($requestingUser !== null && $this->knownUserService->isKnownToUser($requestingUser->getUID(), $userId)) { - return new UserAvatar($folder, $this->l, $user, $this->logger, $this->config); + return new UserAvatar($folder, $this->l, $user, $this->logger, $this->config, $this->avatarVersion); } break; case IAccountManager::SCOPE_LOCAL: case IAccountManager::SCOPE_FEDERATED: case IAccountManager::SCOPE_PUBLISHED: - return new UserAvatar($folder, $this->l, $user, $this->logger, $this->config); + return new UserAvatar($folder, $this->l, $user, $this->logger, $this->config, $this->avatarVersion); default: // use a placeholder avatar which caches the generated images - return new PlaceholderAvatar($folder, $user, $this->config, $this->logger); + return new PlaceholderAvatar($folder, $user, $this->config, $this->logger, $this->avatarVersion); + } + + return new PlaceholderAvatar($folder, $user, $this->config, $this->logger, $this->avatarVersion); + } + + private function getAvatarScope(IUser $user): string { + try { + return $this->accountManager->getAccount($user) + ->getProperty(IAccountManager::PROPERTY_AVATAR) + ->getScope(); + } catch (PropertyDoesNotExistException $e) { + return ''; + } + } + + /** + * `SCOPE_PRIVATE` resolves through `isKnownToUser()` in {@see getAvatar()}, so + * one URL gives two viewers different bytes and no per-user version tracks that. + */ + public function canCacheAvatarLongTerm(string $userId): bool { + $user = $this->userManager->get($userId); + if ($user === null) { + // Federated avatar fetched from another instance, or nothing at all. + return false; + } + + if (!$user->isEnabled()) { + // Serves a guest avatar, and re-enabling bumps no version. + return false; } - return new PlaceholderAvatar($folder, $user, $this->config, $this->logger); + return $this->getAvatarScope($user) !== IAccountManager::SCOPE_PRIVATE; } /** diff --git a/lib/private/Avatar/AvatarVersion.php b/lib/private/Avatar/AvatarVersion.php new file mode 100644 index 0000000000000..8a228ebf37a91 --- /dev/null +++ b/lib/private/Avatar/AvatarVersion.php @@ -0,0 +1,33 @@ +userConfig->setValueInt( + $userId, + 'avatar', + 'version', + $this->userConfig->getValueInt($userId, 'avatar', 'version') + 1, + ); + } +} diff --git a/lib/private/Avatar/PlaceholderAvatar.php b/lib/private/Avatar/PlaceholderAvatar.php index 705cbb754d406..0e1669ccf3211 100644 --- a/lib/private/Avatar/PlaceholderAvatar.php +++ b/lib/private/Avatar/PlaceholderAvatar.php @@ -32,6 +32,7 @@ public function __construct( private User $user, IConfig $config, LoggerInterface $logger, + private AvatarVersion $avatarVersion, ) { parent::__construct($config, $logger); } @@ -64,6 +65,8 @@ public function set($data): void { public function remove(bool $silent = false): void { $avatars = $this->folder->getDirectoryListing(); + $this->avatarVersion->bump($this->user->getUID()); + foreach ($avatars as $avatar) { $avatar->delete(); } diff --git a/lib/private/Avatar/UserAvatar.php b/lib/private/Avatar/UserAvatar.php index ebf4fee4e0538..493c6e13dfa83 100644 --- a/lib/private/Avatar/UserAvatar.php +++ b/lib/private/Avatar/UserAvatar.php @@ -32,6 +32,7 @@ public function __construct( protected User $user, LoggerInterface $logger, IConfig $config, + private AvatarVersion $avatarVersion, ) { parent::__construct($config, $logger); } @@ -157,8 +158,7 @@ private function validateAvatar(IImage $avatar): void { public function remove(bool $silent = false): void { $avatars = $this->folder->getDirectoryListing(); - $this->config->setUserValue($this->user->getUID(), 'avatar', 'version', - (string)((int)$this->config->getUserValue($this->user->getUID(), 'avatar', 'version', '0') + 1)); + $this->avatarVersion->bump($this->user->getUID()); foreach ($avatars as $avatar) { $avatar->delete(); diff --git a/lib/private/Server.php b/lib/private/Server.php index c1aae75d9fb48..412dcfb295841 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -610,7 +610,8 @@ public function __construct( $c->get(IConfig::class), $c->get(IAccountManager::class), $c->get(KnownUserService::class), - $c->get(ICloudIdManager::class) + $c->get(ICloudIdManager::class), + $c->get(\OC\Avatar\AvatarVersion::class), ); }); diff --git a/openapi.json b/openapi.json index 6d5e2907c4b91..ac6fef57be0bf 100644 --- a/openapi.json +++ b/openapi.json @@ -13700,6 +13700,15 @@ "type": "boolean", "default": false } + }, + { + "name": "v", + "in": "query", + "description": "Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { @@ -13803,6 +13812,15 @@ "type": "boolean", "default": false } + }, + { + "name": "v", + "in": "query", + "description": "Avatar version, which lets the response be cached for longer. A stale version still returns the current avatar", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { diff --git a/tests/Core/Controller/AvatarControllerTest.php b/tests/Core/Controller/AvatarControllerTest.php index 125c015a30335..c0e31d0242d10 100644 --- a/tests/Core/Controller/AvatarControllerTest.php +++ b/tests/Core/Controller/AvatarControllerTest.php @@ -19,6 +19,7 @@ function is_uploaded_file($filename) { namespace Tests\Core\Controller; use OC\AppFramework\Utility\TimeFactory; +use OC\Avatar\AvatarManager; use OC\Core\Controller\AvatarController; use OC\Core\Controller\GuestAvatarController; use OCP\AppFramework\Http; @@ -28,7 +29,6 @@ function is_uploaded_file($filename) { use OCP\Files\NotPermittedException; use OCP\Files\SimpleFS\ISimpleFile; use OCP\IAvatar; -use OCP\IAvatarManager; use OCP\IL10N; use OCP\IRequest; use OCP\IUser; @@ -52,7 +52,7 @@ class AvatarControllerTest extends \Test\TestCase { private $userMock; /** @var ISimpleFile|\PHPUnit\Framework\MockObject\MockObject */ private $avatarFile; - /** @var IAvatarManager|\PHPUnit\Framework\MockObject\MockObject */ + /** @var AvatarManager|\PHPUnit\Framework\MockObject\MockObject */ private $avatarManager; /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ private $l; @@ -71,7 +71,7 @@ class AvatarControllerTest extends \Test\TestCase { protected function setUp(): void { parent::setUp(); - $this->avatarManager = $this->getMockBuilder('OCP\IAvatarManager')->getMock(); + $this->avatarManager = $this->createMock(AvatarManager::class); $this->l = $this->getMockBuilder(IL10N::class)->getMock(); $this->l->method('t')->willReturnArgument(0); $this->userManager = $this->getMockBuilder(IUserManager::class)->getMock(); @@ -188,6 +188,25 @@ public function testGetAvatarNoUser(): void { $this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus()); } + public static function dataCacheWindow(): array { + return [ + 'no version, so the client has nothing that tracks changes' => ['', true, 'private, max-age=86400, immutable'], + 'version, and the avatar is the same for every viewer' => ['7', true, 'private, max-age=2592000, immutable'], + 'version, but the avatar depends on the viewer' => ['7', false, 'private, max-age=86400, immutable'], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataCacheWindow')] + public function testCacheWindow(string $version, bool $cacheable, string $expected): void { + $this->avatarMock->method('getFile')->willReturn($this->avatarFile); + $this->avatarManager->method('getAvatar')->with('userId')->willReturn($this->avatarMock); + $this->avatarManager->method('canCacheAvatarLongTerm')->willReturn($cacheable); + + $response = $this->avatarController->getAvatar('userId', 64, false, $version); + + $this->assertEquals($expected, $response->getHeaders()['Cache-Control']); + } + public function testGetAvatarSize64(): void { $this->avatarMock->expects($this->once()) ->method('getFile') diff --git a/tests/Core/Listener/AvatarVersionListenerTest.php b/tests/Core/Listener/AvatarVersionListenerTest.php new file mode 100644 index 0000000000000..8188b1ca395bb --- /dev/null +++ b/tests/Core/Listener/AvatarVersionListenerTest.php @@ -0,0 +1,62 @@ +avatarVersion = $this->createMock(AvatarVersion::class); + $this->listener = new AvatarVersionListener($this->avatarVersion); + } + + public function testBumpsTheVersionWhenTheAccountChanges(): void { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + + $this->avatarVersion->expects($this->once())->method('bump')->with('alice'); + + $this->listener->handle(new UserUpdatedEvent($user, [])); + } + + public function testBumpsTheVersionWhenTheAccountIsDisabled(): void { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + + $this->avatarVersion->expects($this->once())->method('bump')->with('alice'); + + $this->listener->handle(new UserChangedEvent($user, 'enabled', false, true)); + } + + public function testIgnoresUnrelatedUserChanges(): void { + $user = $this->createMock(IUser::class); + + $this->avatarVersion->expects($this->never())->method('bump'); + + $this->listener->handle(new UserChangedEvent($user, 'quota', '1 GB', '2 GB')); + } + + public function testIgnoresOtherEvents(): void { + $this->avatarVersion->expects($this->never())->method('bump'); + + $this->listener->handle(new Event()); + } +} diff --git a/tests/lib/Avatar/AvatarManagerTest.php b/tests/lib/Avatar/AvatarManagerTest.php index 63011e40bd15d..cc32875ed1986 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -9,6 +9,7 @@ namespace Test\Avatar; use OC\Avatar\AvatarManager; +use OC\Avatar\AvatarVersion; use OC\Avatar\PlaceholderAvatar; use OC\Avatar\RemoteAvatar; use OC\Avatar\UserAvatar; @@ -51,6 +52,7 @@ class AvatarManagerTest extends \Test\TestCase { /** @var KnownUserService | \PHPUnit\Framework\MockObject\MockObject */ private $knownUserService; private ICloudIdManager&\PHPUnit\Framework\MockObject\MockObject $cloudIdManager; + private AvatarVersion&\PHPUnit\Framework\MockObject\MockObject $avatarVersion; #[\Override] protected function setUp(): void { @@ -65,6 +67,7 @@ protected function setUp(): void { $this->accountManager = $this->createMock(IAccountManager::class); $this->knownUserService = $this->createMock(KnownUserService::class); $this->cloudIdManager = $this->createMock(ICloudIdManager::class); + $this->avatarVersion = $this->createMock(AvatarVersion::class); $this->avatarManager = new AvatarManager( $this->userSession, @@ -75,7 +78,8 @@ protected function setUp(): void { $this->config, $this->accountManager, $this->knownUserService, - $this->cloudIdManager + $this->cloudIdManager, + $this->avatarVersion, ); } @@ -130,7 +134,7 @@ public function testGetAvatarForSelf(): void { ->with('valid-user') ->willReturn($folder); - $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config); + $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config, $this->avatarVersion); $this->assertEquals($expected, $this->avatarManager->getAvatar('valid-user')); } @@ -177,7 +181,7 @@ public function testGetAvatarValidUserDifferentCasing(): void { ->method('getScope') ->willReturn(IAccountManager::SCOPE_FEDERATED); - $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config); + $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config, $this->avatarVersion); $this->assertEquals($expected, $this->avatarManager->getAvatar('vaLid-USER')); } @@ -263,13 +267,44 @@ public function testGetAvatarScopes($avatarScope, $isPublicCall, $isKnownUser, $ } if ($expectedPlaceholder) { - $expected = new PlaceholderAvatar($folder, $user, $this->config, $this->logger); + $expected = new PlaceholderAvatar($folder, $user, $this->config, $this->logger, $this->avatarVersion); } else { - $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config); + $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config, $this->avatarVersion); } $this->assertEquals($expected, $this->avatarManager->getAvatar('valid-user')); } + public static function dataCanCacheAvatarLongTerm(): array { + return [ + 'federated is the same for everyone' => [IAccountManager::SCOPE_FEDERATED, true, true], + 'no scope resolves to one placeholder' => ['', true, true], + 'private depends on the viewer' => [IAccountManager::SCOPE_PRIVATE, true, false], + 'disabled' => [IAccountManager::SCOPE_FEDERATED, false, false], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataCanCacheAvatarLongTerm')] + public function testCanCacheAvatarLongTerm(string $scope, bool $enabled, bool $expected): void { + $user = $this->createMock(User::class); + $user->method('getUID')->willReturn('valid-user'); + $user->method('isEnabled')->willReturn($enabled); + $this->userManager->method('get')->with('valid-user')->willReturn($user); + + $property = $this->createMock(IAccountProperty::class); + $property->method('getScope')->willReturn($scope); + $account = $this->createMock(IAccount::class); + $account->method('getProperty')->with(IAccountManager::PROPERTY_AVATAR)->willReturn($property); + $this->accountManager->method('getAccount')->with($user)->willReturn($account); + + $this->assertEquals($expected, $this->avatarManager->canCacheAvatarLongTerm('valid-user')); + } + + public function testCannotCacheAnAvatarForAnUnknownUser(): void { + $this->userManager->method('get')->with('nobody')->willReturn(null); + + $this->assertFalse($this->avatarManager->canCacheAvatarLongTerm('nobody')); + } + public function testGetAvatarInvalidUser(): void { $this->expectException(\Exception::class); $this->expectExceptionMessage('user does not exist'); diff --git a/tests/lib/Avatar/PlaceholderAvatarTest.php b/tests/lib/Avatar/PlaceholderAvatarTest.php new file mode 100644 index 0000000000000..290995a1545dd --- /dev/null +++ b/tests/lib/Avatar/PlaceholderAvatarTest.php @@ -0,0 +1,53 @@ +folder = $this->createMock(ISimpleFolder::class); + $this->user = $this->createMock(User::class); + $this->user->method('getUID')->willReturn('alice'); + $this->avatarVersion = $this->createMock(AvatarVersion::class); + + $this->avatar = new PlaceholderAvatar( + $this->folder, + $this->user, + $this->createMock(IConfig::class), + $this->createMock(LoggerInterface::class), + $this->avatarVersion, + ); + } + + public function testRemoveBumpsTheVersion(): void { + $generated = $this->createMock(ISimpleFile::class); + $generated->expects($this->once())->method('delete'); + $this->folder->method('getDirectoryListing')->willReturn([$generated]); + + $this->avatarVersion->expects($this->once())->method('bump')->with('alice'); + + $this->avatar->remove(); + } +} diff --git a/tests/lib/Avatar/UserAvatarTest.php b/tests/lib/Avatar/UserAvatarTest.php index acbc9488e8c93..8f089792cd262 100644 --- a/tests/lib/Avatar/UserAvatarTest.php +++ b/tests/lib/Avatar/UserAvatarTest.php @@ -8,6 +8,7 @@ namespace Test\Avatar; +use OC\Avatar\AvatarVersion; use OC\Avatar\UserAvatar; use OC\Files\SimpleFS\SimpleFolder; use OC\User\User; @@ -26,6 +27,7 @@ class UserAvatarTest extends \Test\TestCase { private UserAvatar $avatar; private SimpleFolder&MockObject $folder; private IConfig&MockObject $config; + private AvatarVersion&MockObject $avatarVersion; private User&MockObject $user; #[\Override] @@ -36,6 +38,7 @@ protected function setUp(): void { // abcdefghi is a convenient name that our algorithm convert to our nextcloud blue 0082c9 $this->user = $this->getUserWithDisplayName('abcdefghi'); $this->config = $this->createMock(IConfig::class); + $this->avatarVersion = $this->createMock(AvatarVersion::class); $this->avatar = $this->getUserAvatar($this->user); } @@ -221,10 +224,11 @@ public function testSetAvatar(): void { ->method('putContent') ->with($image->data()); - $this->config->expects($this->exactly(3)) + $this->config->expects($this->exactly(2)) ->method('setUserValue'); - $this->config->expects($this->once()) - ->method('getUserValue'); + $this->avatarVersion->expects($this->once()) + ->method('bump') + ->with($this->user->getUID()); $this->user->expects($this->exactly(1))->method('triggerChange'); @@ -289,7 +293,8 @@ private function getUserAvatar($user) { $l, $user, $this->createMock(LoggerInterface::class), - $this->config + $this->config, + $this->avatarVersion, ); } }