From 1fd72c9fe80e1729dbabd258859d11e0991b1036 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 1 Jul 2025 10:23:43 +0200 Subject: [PATCH] fix(core): Stop abusing the cache for avatar upload Signed-off-by: provokateurin --- .../components/PersonalInfo/AvatarSection.vue | 3 +- build/integration/features/avatar.feature | 128 +----------------- .../integration/features/bootstrap/Avatar.php | 53 +------- core/Controller/AvatarController.php | 78 +---------- .../Core/Controller/AvatarControllerTest.php | 118 ---------------- 5 files changed, 12 insertions(+), 368 deletions(-) diff --git a/apps/settings/src/components/PersonalInfo/AvatarSection.vue b/apps/settings/src/components/PersonalInfo/AvatarSection.vue index 79eba665cd4..e2a062bf7fc 100644 --- a/apps/settings/src/components/PersonalInfo/AvatarSection.vue +++ b/apps/settings/src/components/PersonalInfo/AvatarSection.vue @@ -182,8 +182,7 @@ export default { if (data.status === 'success') { this.handleAvatarUpdate(false) } else if (data.data === 'notsquare') { - const tempAvatar = generateUrl('/avatar/tmp') + '?requesttoken=' + encodeURIComponent(OC.requestToken) + '#' + Math.floor(Math.random() * 1000) - this.$refs.cropper.replace(tempAvatar) + this.$refs.cropper.replace(data.image) this.showCropper = true } else { showError(data.data.message) diff --git a/build/integration/features/avatar.feature b/build/integration/features/avatar.feature index 4c8c37fb98c..45d0e1e157a 100644 --- a/build/integration/features/avatar.feature +++ b/build/integration/features/avatar.feature @@ -21,32 +21,9 @@ Feature: avatar And last avatar is a square of size 512 And last avatar is not a single color - - - Scenario: get temporary non-square user avatar before cropping it - Given Logging in using web as "user0" - And logged in user posts temporary avatar from file "data/coloured-pattern-non-square.png" - When logged in user gets temporary avatar - Then The following headers should be set - | Content-Type | image/png | - # "last avatar" also includes the last temporary avatar - And last avatar is not a square - And last avatar is not a single color - - Scenario: get non-square user avatar before cropping it - Given Logging in using web as "user0" - And logged in user posts temporary avatar from file "data/coloured-pattern-non-square.png" - # Avatar needs to be cropped to finish setting it - When user "user0" gets avatar for user "user0" - Then The following headers should be set - | Content-Type | image/png | - | X-NC-IsCustomAvatar | 0 | - And last avatar is a square of size 512 - And last avatar is not a single color - Scenario: set square user avatar from file Given Logging in using web as "user0" - When logged in user posts temporary avatar from file "data/green-square-256.png" + When logged in user posts avatar from file "data/green-square-256.png" And user "user0" gets avatar for user "user0" And The following headers should be set | Content-Type | image/png | @@ -64,7 +41,7 @@ Feature: avatar Scenario: set square user avatar from internal path Given user "user0" uploads file "data/green-square-256.png" to "/internal-green-square-256.png" And Logging in using web as "user0" - When logged in user posts temporary avatar from internal path "internal-green-square-256.png" + When logged in user posts avatar from internal path "internal-green-square-256.png" And user "user0" gets avatar for user "user0" with size "64" And The following headers should be set | Content-Type | image/png | @@ -78,82 +55,21 @@ Feature: avatar And last avatar is a square of size 64 And last avatar is a single "#00FF00" color - Scenario: set non-square user avatar from file - Given Logging in using web as "user0" - When logged in user posts temporary avatar from file "data/coloured-pattern-non-square.png" - And logged in user crops temporary avatar - | x | 384 | - | y | 256 | - | w | 128 | - | h | 128 | - Then logged in user gets temporary avatar with 404 - And user "user0" gets avatar for user "user0" - And The following headers should be set - | Content-Type | image/png | - | X-NC-IsCustomAvatar | 1 | - And last avatar is a square of size 512 - And last avatar is a single "#FF0000" color - And user "anonymous" gets avatar for user "user0" - And The following headers should be set - | Content-Type | image/png | - | X-NC-IsCustomAvatar | 1 | - And last avatar is a square of size 512 - And last avatar is a single "#FF0000" color - - Scenario: set non-square user avatar from internal path - Given user "user0" uploads file "data/coloured-pattern-non-square.png" to "/internal-coloured-pattern-non-square.png" - And Logging in using web as "user0" - When logged in user posts temporary avatar from internal path "internal-coloured-pattern-non-square.png" - And logged in user crops temporary avatar - | x | 704 | - | y | 320 | - | w | 64 | - | h | 64 | - Then logged in user gets temporary avatar with 404 - And user "user0" gets avatar for user "user0" with size "64" - And The following headers should be set - | Content-Type | image/png | - | X-NC-IsCustomAvatar | 1 | - And last avatar is a square of size 64 - And last avatar is a single "#00FF00" color - And user "anonymous" gets avatar for user "user0" with size "64" - And The following headers should be set - | Content-Type | image/png | - | X-NC-IsCustomAvatar | 1 | - And last avatar is a square of size 64 - And last avatar is a single "#00FF00" color - - Scenario: cropped user avatar needs to be squared - Given Logging in using web as "user0" - And logged in user posts temporary avatar from file "data/coloured-pattern-non-square.png" - When logged in user crops temporary avatar with 400 - | x | 384 | - | y | 256 | - | w | 192 | - | h | 128 | - - - Scenario: delete user avatar Given Logging in using web as "user0" - And logged in user posts temporary avatar from file "data/coloured-pattern-non-square.png" - And logged in user crops temporary avatar - | x | 384 | - | y | 256 | - | w | 128 | - | h | 128 | + And logged in user posts avatar from file "data/green-square-256.png" And user "user0" gets avatar for user "user0" And The following headers should be set | Content-Type | image/png | | X-NC-IsCustomAvatar | 1 | And last avatar is a square of size 512 - And last avatar is a single "#FF0000" color + And last avatar is a single "#00FF00" color And user "anonymous" gets avatar for user "user0" And The following headers should be set | Content-Type | image/png | | X-NC-IsCustomAvatar | 1 | And last avatar is a square of size 512 - And last avatar is a single "#FF0000" color + And last avatar is a single "#00FF00" color When logged in user deletes the user avatar Then user "user0" gets avatar for user "user0" And The following headers should be set @@ -168,40 +84,6 @@ Feature: avatar And last avatar is a square of size 512 And last avatar is not a single color - - - Scenario: get user avatar with a larger size than the original one - Given Logging in using web as "user0" - And logged in user posts temporary avatar from file "data/coloured-pattern-non-square.png" - And logged in user crops temporary avatar - | x | 384 | - | y | 256 | - | w | 128 | - | h | 128 | - When user "user0" gets avatar for user "user0" with size "192" - Then The following headers should be set - | Content-Type | image/png | - | X-NC-IsCustomAvatar | 1 | - And last avatar is a square of size 512 - And last avatar is a single "#FF0000" color - - Scenario: get user avatar with a smaller size than the original one - Given Logging in using web as "user0" - And logged in user posts temporary avatar from file "data/coloured-pattern-non-square.png" - And logged in user crops temporary avatar - | x | 384 | - | y | 256 | - | w | 128 | - | h | 128 | - When user "user0" gets avatar for user "user0" with size "96" - Then The following headers should be set - | Content-Type | image/png | - | X-NC-IsCustomAvatar | 1 | - And last avatar is a square of size 512 - And last avatar is a single "#FF0000" color - - - Scenario: get default guest avatar When user "user0" gets avatar for guest "guest0" Then The following headers should be set diff --git a/build/integration/features/bootstrap/Avatar.php b/build/integration/features/bootstrap/Avatar.php index c7d70ff1507..0a639e14ef6 100644 --- a/build/integration/features/bootstrap/Avatar.php +++ b/build/integration/features/bootstrap/Avatar.php @@ -4,7 +4,6 @@ * SPDX-FileCopyrightText: 2020 Nextcloud GmbH and Nextcloud contributors * SPDX-License-Identifier: AGPL-3.0-or-later */ -use Behat\Gherkin\Node\TableNode; use PHPUnit\Framework\Assert; require __DIR__ . '/../../vendor/autoload.php'; @@ -68,30 +67,11 @@ trait Avatar { } /** - * @When logged in user gets temporary avatar - */ - public function loggedInUserGetsTemporaryAvatar() { - $this->loggedInUserGetsTemporaryAvatarWith('200'); - } - - /** - * @When logged in user gets temporary avatar with :statusCode - * - * @param string $statusCode - */ - public function loggedInUserGetsTemporaryAvatarWith(string $statusCode) { - $this->sendingAToWithRequesttoken('GET', '/index.php/avatar/tmp'); - $this->theHTTPStatusCodeShouldBe($statusCode); - - $this->getLastAvatar(); - } - - /** - * @When logged in user posts temporary avatar from file :source + * @When logged in user posts avatar from file :source * * @param string $source */ - public function loggedInUserPostsTemporaryAvatarFromFile(string $source) { + public function loggedInUserPostsAvatarFromFile(string $source) { $file = \GuzzleHttp\Psr7\Utils::streamFor(fopen($source, 'r')); $this->sendingAToWithRequesttoken('POST', '/index.php/avatar', @@ -107,40 +87,15 @@ trait Avatar { } /** - * @When logged in user posts temporary avatar from internal path :path + * @When logged in user posts avatar from internal path :path * * @param string $path */ - public function loggedInUserPostsTemporaryAvatarFromInternalPath(string $path) { + public function loggedInUserPostsAvatarFromInternalPath(string $path) { $this->sendingAToWithRequesttoken('POST', '/index.php/avatar?path=' . $path); $this->theHTTPStatusCodeShouldBe('200'); } - /** - * @When logged in user crops temporary avatar - * - * @param TableNode $crop - */ - public function loggedInUserCropsTemporaryAvatar(TableNode $crop) { - $this->loggedInUserCropsTemporaryAvatarWith('200', $crop); - } - - /** - * @When logged in user crops temporary avatar with :statusCode - * - * @param string $statusCode - * @param TableNode $crop - */ - public function loggedInUserCropsTemporaryAvatarWith(string $statusCode, TableNode $crop) { - $parameters = []; - foreach ($crop->getRowsHash() as $key => $value) { - $parameters[] = 'crop[' . $key . ']=' . $value; - } - - $this->sendingAToWithRequesttoken('POST', '/index.php/avatar/cropped?' . implode('&', $parameters)); - $this->theHTTPStatusCodeShouldBe($statusCode); - } - /** * @When logged in user deletes the user avatar */ diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index 0e5a8b2e408..cdc3fe427b9 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -14,14 +14,12 @@ use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; use OCP\AppFramework\Http\Attribute\PublicPage; -use OCP\AppFramework\Http\DataDisplayResponse; use OCP\AppFramework\Http\FileDisplayResponse; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\Response; use OCP\Files\File; use OCP\Files\IRootFolder; use OCP\IAvatarManager; -use OCP\ICache; use OCP\IL10N; use OCP\IRequest; use OCP\IUserManager; @@ -37,7 +35,6 @@ class AvatarController extends Controller { string $appName, IRequest $request, protected IAvatarManager $avatarManager, - protected ICache $cache, protected IL10N $l10n, protected IUserManager $userManager, protected IRootFolder $rootFolder, @@ -196,8 +193,7 @@ class AvatarController extends Controller { Http::STATUS_BAD_REQUEST ); } - $this->cache->set('avatar_upload', file_get_contents($files['tmp_name'][0]), 7200); - $content = $this->cache->get('avatar_upload'); + $content = file_get_contents($files['tmp_name'][0]); unlink($files['tmp_name'][0]); } else { $phpFileUploadErrors = [ @@ -244,8 +240,6 @@ class AvatarController extends Controller { try { $avatar = $this->avatarManager->getAvatar($this->userId); $avatar->set($image); - // Clean up - $this->cache->remove('tmpAvatar'); return new JSONResponse(['status' => 'success']); } catch (\Throwable $e) { $this->logger->error($e->getMessage(), ['exception' => $e, 'app' => 'core']); @@ -253,9 +247,8 @@ class AvatarController extends Controller { } } - $this->cache->set('tmpAvatar', $image->data(), 7200); return new JSONResponse( - ['data' => 'notsquare'], + ['data' => 'notsquare', 'image' => 'data:' . $mimeType . ';base64,' . base64_encode($image->data())], Http::STATUS_OK ); } else { @@ -282,71 +275,4 @@ class AvatarController extends Controller { return new JSONResponse(['data' => ['message' => $this->l10n->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST); } } - - /** - * @return JSONResponse|DataDisplayResponse - */ - #[NoAdminRequired] - #[FrontpageRoute(verb: 'GET', url: '/avatar/tmp')] - public function getTmpAvatar() { - $tmpAvatar = $this->cache->get('tmpAvatar'); - if (is_null($tmpAvatar)) { - return new JSONResponse(['data' => [ - 'message' => $this->l10n->t('No temporary profile picture available, try again') - ]], - Http::STATUS_NOT_FOUND); - } - - $image = new \OCP\Image(); - $image->loadFromData($tmpAvatar); - - $resp = new DataDisplayResponse( - $image->data() ?? '', - Http::STATUS_OK, - ['Content-Type' => $image->mimeType()]); - - $resp->setETag((string) crc32($image->data() ?? '')); - $resp->cacheFor(0); - $resp->setLastModified(new \DateTime('now', new \DateTimeZone('GMT'))); - return $resp; - } - - #[NoAdminRequired] - #[FrontpageRoute(verb: 'POST', url: '/avatar/cropped')] - public function postCroppedAvatar(?array $crop = null): JSONResponse { - if (is_null($crop)) { - return new JSONResponse(['data' => ['message' => $this->l10n->t('No crop data provided')]], - Http::STATUS_BAD_REQUEST); - } - - if (!isset($crop['x'], $crop['y'], $crop['w'], $crop['h'])) { - return new JSONResponse(['data' => ['message' => $this->l10n->t('No valid crop data provided')]], - Http::STATUS_BAD_REQUEST); - } - - $tmpAvatar = $this->cache->get('tmpAvatar'); - if (is_null($tmpAvatar)) { - return new JSONResponse(['data' => [ - 'message' => $this->l10n->t('No temporary profile picture available, try again') - ]], - Http::STATUS_BAD_REQUEST); - } - - $image = new \OCP\Image(); - $image->loadFromData($tmpAvatar); - $image->crop($crop['x'], $crop['y'], (int) round($crop['w']), (int) round($crop['h'])); - try { - $avatar = $this->avatarManager->getAvatar($this->userId); - $avatar->set($image); - // Clean up - $this->cache->remove('tmpAvatar'); - return new JSONResponse(['status' => 'success']); - } catch (\OC\NotSquareException $e) { - return new JSONResponse(['data' => ['message' => $this->l10n->t('Crop is not square')]], - Http::STATUS_BAD_REQUEST); - } catch (\Exception $e) { - $this->logger->error($e->getMessage(), ['exception' => $e, 'app' => 'core']); - return new JSONResponse(['data' => ['message' => $this->l10n->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST); - } - } } diff --git a/tests/Core/Controller/AvatarControllerTest.php b/tests/Core/Controller/AvatarControllerTest.php index 57375e08d42..0194ae1a339 100644 --- a/tests/Core/Controller/AvatarControllerTest.php +++ b/tests/Core/Controller/AvatarControllerTest.php @@ -29,7 +29,6 @@ use OCP\Files\NotPermittedException; use OCP\Files\SimpleFS\ISimpleFile; use OCP\IAvatar; use OCP\IAvatarManager; -use OCP\ICache; use OCP\IL10N; use OCP\IRequest; use OCP\IUser; @@ -55,8 +54,6 @@ class AvatarControllerTest extends \Test\TestCase { private $avatarFile; /** @var IAvatarManager|\PHPUnit\Framework\MockObject\MockObject */ private $avatarManager; - /** @var ICache|\PHPUnit\Framework\MockObject\MockObject */ - private $cache; /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ private $l; /** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */ @@ -74,8 +71,6 @@ class AvatarControllerTest extends \Test\TestCase { parent::setUp(); $this->avatarManager = $this->getMockBuilder('OCP\IAvatarManager')->getMock(); - $this->cache = $this->getMockBuilder('OCP\ICache') - ->disableOriginalConstructor()->getMock(); $this->l = $this->getMockBuilder(IL10N::class)->getMock(); $this->l->method('t')->willReturnArgument(0); $this->userManager = $this->getMockBuilder(IUserManager::class)->getMock(); @@ -98,7 +93,6 @@ class AvatarControllerTest extends \Test\TestCase { 'core', $this->request, $this->avatarManager, - $this->cache, $this->l, $this->userManager, $this->rootFolder, @@ -298,25 +292,6 @@ class AvatarControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $this->avatarController->deleteAvatar()); } - /** - * Trying to get a tmp avatar when it is not available. 404 - */ - public function testTmpAvatarNoTmp() { - $response = $this->avatarController->getTmpAvatar(); - $this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus()); - } - - /** - * Fetch tmp avatar - */ - public function testTmpAvatarValid() { - $this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT.'/tests/data/testimage.jpg')); - - $response = $this->avatarController->getTmpAvatar(); - $this->assertEquals(Http::STATUS_OK, $response->getStatus()); - } - - /** * When trying to post a new avatar a path or image should be posted. */ @@ -335,9 +310,6 @@ class AvatarControllerTest extends \Test\TestCase { $copyRes = copy(\OC::$SERVERROOT.'/tests/data/testimage.jpg', $fileName); $this->assertTrue($copyRes); - //Create file in cache - $this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT.'/tests/data/testimage.jpg')); - //Create request return $reqRet = ['error' => [0], 'tmp_name' => [$fileName], 'size' => [filesize(\OC::$SERVERROOT.'/tests/data/testimage.jpg')]]; $this->request->method('getUploadedFile')->willReturn($reqRet); @@ -373,9 +345,6 @@ class AvatarControllerTest extends \Test\TestCase { $copyRes = copy(\OC::$SERVERROOT.'/tests/data/testimage.gif', $fileName); $this->assertTrue($copyRes); - //Create file in cache - $this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT.'/tests/data/testimage.gif')); - //Create request return $reqRet = ['error' => [0], 'tmp_name' => [$fileName], 'size' => [filesize(\OC::$SERVERROOT.'/tests/data/testimage.gif')]]; $this->request->method('getUploadedFile')->willReturn($reqRet); @@ -464,93 +433,6 @@ class AvatarControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $this->avatarController->postAvatar('avatar.jpg')); } - /** - * Test what happens if the upload of the avatar fails - */ - public function testPostAvatarException() { - $this->cache->expects($this->once()) - ->method('set') - ->will($this->throwException(new \Exception('foo'))); - $file = $this->getMockBuilder('OCP\Files\File') - ->disableOriginalConstructor()->getMock(); - $file->expects($this->once()) - ->method('getContent') - ->willReturn(file_get_contents(\OC::$SERVERROOT.'/tests/data/testimage.jpg')); - $file->expects($this->once()) - ->method('getMimeType') - ->willReturn('image/jpeg'); - $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); - $this->rootFolder->method('getUserFolder')->with('userid')->willReturn($userFolder); - $userFolder->method('get')->willReturn($file); - - $this->logger->expects($this->once()) - ->method('error') - ->with('foo', ['exception' => new \Exception('foo'), 'app' => 'core']); - $expectedResponse = new Http\JSONResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_OK); - $this->assertEquals($expectedResponse, $this->avatarController->postAvatar('avatar.jpg')); - } - - - /** - * Test invalid crop argument - */ - public function testPostCroppedAvatarInvalidCrop() { - $response = $this->avatarController->postCroppedAvatar([]); - - $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); - } - - /** - * Test no tmp avatar to crop - */ - public function testPostCroppedAvatarNoTmpAvatar() { - $response = $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 10]); - - $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); - } - - /** - * Test with non square crop - */ - public function testPostCroppedAvatarNoSquareCrop() { - $this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT.'/tests/data/testimage.jpg')); - - $this->avatarMock->method('set')->will($this->throwException(new \OC\NotSquareException)); - $this->avatarManager->method('getAvatar')->willReturn($this->avatarMock); - $response = $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 11]); - - $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); - } - - /** - * Check for proper reply on proper crop argument - */ - public function testPostCroppedAvatarValidCrop() { - $this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT.'/tests/data/testimage.jpg')); - $this->avatarManager->method('getAvatar')->willReturn($this->avatarMock); - $response = $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 10]); - - $this->assertEquals(Http::STATUS_OK, $response->getStatus()); - $this->assertEquals('success', $response->getData()['status']); - } - - /** - * Test what happens if the cropping of the avatar fails - */ - public function testPostCroppedAvatarException() { - $this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT.'/tests/data/testimage.jpg')); - - $this->avatarMock->method('set')->will($this->throwException(new \Exception('foo'))); - $this->avatarManager->method('getAvatar')->willReturn($this->avatarMock); - - $this->logger->expects($this->once()) - ->method('error') - ->with('foo', ['exception' => new \Exception('foo'), 'app' => 'core']); - $expectedResponse = new Http\JSONResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_BAD_REQUEST); - $this->assertEquals($expectedResponse, $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 11])); - } - - /** * Check for proper reply on proper crop argument */