diff --git a/core/AppInfo/Application.php b/core/AppInfo/Application.php index 1b9e36c5a3413..31b79d4e85213 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, -10); $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 034d30c684ddf..997c082cd5b33 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\IUserFolder; use OCP\Files\NotPermittedException; -use OCP\IAvatarManager; use OCP\IL10N; use OCP\Image; use OCP\IRequest; @@ -36,10 +36,26 @@ * @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 this + * is also how long a missed version bump stays wrong. + */ + private const CACHE_VERSIONED = 60 * 60 * 24 * 60; + + /** + * In some situations everyone fetches everyone else's avatar at once, so a fixed + * window expires the whole set together and the next call pays for all of + * it. Offsetting per user spreads those refetches over later calls. + */ + private const CACHE_SPREAD = 60 * 60 * 24 * 30; + public function __construct( string $appName, IRequest $request, - protected IAvatarManager $avatarManager, + protected AvatarManager $avatarManager, protected IL10N $l10n, protected IUserManager $userManager, protected ?IUserFolder $userFolder, @@ -57,6 +73,7 @@ public function __construct( * @param non-empty-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 +85,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 +113,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 +123,7 @@ public function getAvatarDark(string $userId, int $size, bool $guestFallback = f * @param non-empty-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 +135,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 +163,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 + (crc32($userId) % self::CACHE_SPREAD); + } + + return self::CACHE_DEFAULT; + } + /** * @param ?non-empty-string $path */ diff --git a/core/Listener/AvatarVersionListener.php b/core/Listener/AvatarVersionListener.php new file mode 100644 index 0000000000000..473959d7eb916 --- /dev/null +++ b/core/Listener/AvatarVersionListener.php @@ -0,0 +1,47 @@ + + */ +class AvatarVersionListener implements IEventListener { + public function __construct( + private IUserConfig $userConfig, + ) { + } + + #[\Override] + public function handle(Event $event): void { + $accountChanged = $event instanceof UserUpdatedEvent; + $enabledChanged = $event instanceof UserChangedEvent && $event->getFeature() === 'enabled'; + if (!$accountChanged && !$enabledChanged) { + return; + } + + $userId = $event->getUser()->getUID(); + $this->userConfig->setValueInt($userId, 'avatar', 'version', + $this->userConfig->getValueInt($userId, 'avatar', 'version') + 1); + } +} diff --git a/core/openapi-full.json b/core/openapi-full.json index a68b14c3420a0..2445eb4e8613e 100644 --- a/core/openapi-full.json +++ b/core/openapi-full.json @@ -9352,6 +9352,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": { @@ -9456,6 +9465,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 259376f32ad7c..57cc79b769c53 100644 --- a/core/openapi.json +++ b/core/openapi.json @@ -9352,6 +9352,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": { @@ -9456,6 +9465,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 70ebb77e1aaa6..255465c0ffb2e 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1638,6 +1638,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 75746428ed294..046d2cd20b693 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1679,6 +1679,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..ddee10bc28586 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -13,6 +13,7 @@ use OC\User\Manager; use OCP\Accounts\IAccountManager; use OCP\Accounts\PropertyDoesNotExistException; +use OCP\Config\IUserConfig; use OCP\Federation\ICloudIdManager; use OCP\Files\IAppData; use OCP\Files\NotFoundException; @@ -22,6 +23,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 +42,7 @@ public function __construct( private IAccountManager $accountManager, private KnownUserService $knownUserService, private ICloudIdManager $cloudIdManager, + private IUserConfig $userConfig, ) { } @@ -79,31 +82,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->userConfig); } 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->userConfig); 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->userConfig); + } + + return new PlaceholderAvatar($folder, $user, $this->config, $this->logger, $this->userConfig); + } + + 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 those went out with the short window. + return false; } - return new PlaceholderAvatar($folder, $user, $this->config, $this->logger); + return $this->getAvatarScope($user) !== IAccountManager::SCOPE_PRIVATE; } /** diff --git a/lib/private/Avatar/PlaceholderAvatar.php b/lib/private/Avatar/PlaceholderAvatar.php index 705cbb754d406..caf9acffa51fa 100644 --- a/lib/private/Avatar/PlaceholderAvatar.php +++ b/lib/private/Avatar/PlaceholderAvatar.php @@ -11,6 +11,7 @@ use OC\NotSquareException; use OC\User\User; +use OCP\Config\IUserConfig; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; use OCP\Files\SimpleFS\ISimpleFile; @@ -32,6 +33,7 @@ public function __construct( private User $user, IConfig $config, LoggerInterface $logger, + private IUserConfig $userConfig, ) { parent::__construct($config, $logger); } @@ -64,6 +66,10 @@ public function set($data): void { public function remove(bool $silent = false): void { $avatars = $this->folder->getDirectoryListing(); + $userId = $this->user->getUID(); + $this->userConfig->setValueInt($userId, 'avatar', 'version', + $this->userConfig->getValueInt($userId, 'avatar', 'version') + 1); + foreach ($avatars as $avatar) { $avatar->delete(); } diff --git a/lib/private/Avatar/UserAvatar.php b/lib/private/Avatar/UserAvatar.php index ebf4fee4e0538..245f338bc11b7 100644 --- a/lib/private/Avatar/UserAvatar.php +++ b/lib/private/Avatar/UserAvatar.php @@ -11,6 +11,7 @@ use OC\NotSquareException; use OC\User\User; +use OCP\Config\IUserConfig; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; use OCP\Files\SimpleFS\ISimpleFile; @@ -32,6 +33,7 @@ public function __construct( protected User $user, LoggerInterface $logger, IConfig $config, + private IUserConfig $userConfig, ) { parent::__construct($config, $logger); } @@ -157,8 +159,9 @@ 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)); + $userId = $this->user->getUID(); + $this->userConfig->setValueInt($userId, 'avatar', 'version', + $this->userConfig->getValueInt($userId, 'avatar', 'version') + 1); foreach ($avatars as $avatar) { $avatar->delete(); diff --git a/lib/private/Server.php b/lib/private/Server.php index ed0ba5c04f741..92b2b4bebbb43 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -561,7 +561,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(IUserConfig::class), ); }); diff --git a/openapi.json b/openapi.json index eea7f6a7906ad..b9b1bee0f782a 100644 --- a/openapi.json +++ b/openapi.json @@ -13728,6 +13728,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": { @@ -13832,6 +13841,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 38718eab67786..5a0a812431618 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; @@ -27,7 +28,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; @@ -47,7 +47,7 @@ class AvatarControllerTest extends \Test\TestCase { private IAvatar&MockObject $avatarMock; private IUser&MockObject $userMock; private ISimpleFile&MockObject $avatarFile; - private IAvatarManager&MockObject $avatarManager; + private AvatarManager&MockObject $avatarManager; private IL10N&MockObject $l; private IUserManager&MockObject $userManager; private IUserFolder&MockObject $userFolder; @@ -59,7 +59,7 @@ class AvatarControllerTest extends \Test\TestCase { protected function setUp(): void { parent::setUp(); - $this->avatarManager = $this->createMock(IAvatarManager::class); + $this->avatarManager = $this->createMock(AvatarManager::class); $this->l = $this->createMock(IL10N::class); $this->l->method('t')->willReturnArgument(0); $this->userManager = $this->createMock(IUserManager::class); @@ -176,6 +176,28 @@ 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'], + // 60 days plus this user's share of the 30 day spread + 'version, and the avatar is the same for every viewer' => ['7', true, 'private, max-age=7461068, 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')->with('userId')->willReturn($cacheable); + + $light = $this->avatarController->getAvatar('userId', 64, false, $version); + $dark = $this->avatarController->getAvatarDark('userId', 64, false, $version); + + $this->assertEquals($expected, $light->getHeaders()['Cache-Control'], 'light avatar'); + $this->assertEquals($expected, $dark->getHeaders()['Cache-Control'], 'dark avatar'); + } + 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..4580d5421cab2 --- /dev/null +++ b/tests/Core/Listener/AvatarVersionListenerTest.php @@ -0,0 +1,64 @@ +userConfig = $this->createMock(IUserConfig::class); + $this->listener = new AvatarVersionListener($this->userConfig); + } + + public function testBumpsTheVersionWhenTheAccountChanges(): void { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + + $this->userConfig->expects($this->once())->method('setValueInt') + ->with('alice', 'avatar', 'version', 1); + + $this->listener->handle(new UserUpdatedEvent($user, [])); + } + + public function testBumpsTheVersionWhenTheAccountIsDisabled(): void { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + + $this->userConfig->expects($this->once())->method('setValueInt') + ->with('alice', 'avatar', 'version', 1); + + $this->listener->handle(new UserChangedEvent($user, 'enabled', false, true)); + } + + public function testIgnoresUnrelatedUserChanges(): void { + $user = $this->createMock(IUser::class); + + $this->userConfig->expects($this->never())->method('setValueInt'); + + $this->listener->handle(new UserChangedEvent($user, 'quota', '1 GB', '2 GB')); + } + + public function testIgnoresOtherEvents(): void { + $this->userConfig->expects($this->never())->method('setValueInt'); + + $this->listener->handle(new Event()); + } +} diff --git a/tests/lib/Avatar/AvatarManagerTest.php b/tests/lib/Avatar/AvatarManagerTest.php index 63011e40bd15d..61fabf58d5bf3 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -18,6 +18,7 @@ use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; use OCP\Accounts\IAccountProperty; +use OCP\Config\IUserConfig; use OCP\Federation\ICloudId; use OCP\Federation\ICloudIdManager; use OCP\Files\IAppData; @@ -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 IUserConfig&\PHPUnit\Framework\MockObject\MockObject $userConfig; #[\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->userConfig = $this->createMock(IUserConfig::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->userConfig, ); } @@ -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->userConfig); $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->userConfig); $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->userConfig); } else { - $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config); + $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config, $this->userConfig); } $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..ef0af363040ed --- /dev/null +++ b/tests/lib/Avatar/PlaceholderAvatarTest.php @@ -0,0 +1,54 @@ +folder = $this->createMock(ISimpleFolder::class); + $this->user = $this->createMock(User::class); + $this->user->method('getUID')->willReturn('alice'); + $this->userConfig = $this->createMock(IUserConfig::class); + + $this->avatar = new PlaceholderAvatar( + $this->folder, + $this->user, + $this->createMock(IConfig::class), + $this->createMock(LoggerInterface::class), + $this->userConfig, + ); + } + + public function testRemoveBumpsTheVersion(): void { + $generated = $this->createMock(ISimpleFile::class); + $generated->expects($this->once())->method('delete'); + $this->folder->method('getDirectoryListing')->willReturn([$generated]); + + $this->userConfig->expects($this->once())->method('setValueInt') + ->with('alice', 'avatar', 'version', 1); + + $this->avatar->remove(); + } +} diff --git a/tests/lib/Avatar/UserAvatarTest.php b/tests/lib/Avatar/UserAvatarTest.php index acbc9488e8c93..b785796ec8a2b 100644 --- a/tests/lib/Avatar/UserAvatarTest.php +++ b/tests/lib/Avatar/UserAvatarTest.php @@ -12,6 +12,7 @@ use OC\Files\SimpleFS\SimpleFolder; use OC\User\User; use OCP\Color; +use OCP\Config\IUserConfig; use OCP\Files\File; use OCP\Files\NotFoundException; use OCP\Files\SimpleFS\ISimpleFile; @@ -26,6 +27,7 @@ class UserAvatarTest extends \Test\TestCase { private UserAvatar $avatar; private SimpleFolder&MockObject $folder; private IConfig&MockObject $config; + private IUserConfig&MockObject $userConfig; 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->userConfig = $this->createMock(IUserConfig::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->userConfig->expects($this->once()) + ->method('setValueInt') + ->with($this->user->getUID(), 'avatar', 'version', 1); $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->userConfig, ); } }