fix(core): Stop abusing the cache for avatar upload

Signed-off-by: provokateurin <kate@provokateurin.de>

Author: provokateurin

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)

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

build/integration/features/bootstrap/Avatar.php

@@ -3,7 +3,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';
@@ -67,30 +66,11 @@ public function userGetsAvatarForGuest(string $user, string $guestAvatar) {
 	}
 
 	/**
-	 * @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',
@@ -106,40 +86,15 @@ public function loggedInUserPostsTemporaryAvatarFromFile(string $source) {
 	}
 
 	/**
-	 * @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
 	 */

core/Controller/AvatarController.php

@@ -15,14 +15,12 @@
 use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
 use OCP\AppFramework\Http\Attribute\OpenAPI;
 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;
@@ -38,7 +36,6 @@ public function __construct(
 		string $appName,
 		IRequest $request,
 		protected IAvatarManager $avatarManager,
-		protected ICache $cache,
 		protected IL10N $l10n,
 		protected IUserManager $userManager,
 		protected IRootFolder $rootFolder,
@@ -199,8 +196,7 @@ public function postAvatar(?string $path = null): JSONResponse {
 						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 = [
@@ -247,18 +243,15 @@ public function postAvatar(?string $path = null): JSONResponse {
 					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']);
 						return new JSONResponse(['data' => ['message' => $this->l10n->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST);
 					}
 				}
 
-				$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 {

tests/Core/Controller/AvatarControllerTest.php

@@ -28,7 +28,6 @@ function is_uploaded_file($filename) {
 use OCP\Files\SimpleFS\ISimpleFile;
 use OCP\IAvatar;
 use OCP\IAvatarManager;
-use OCP\ICache;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUser;
@@ -54,8 +53,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 */
@@ -73,8 +70,6 @@ protected function setUp(): void {
 		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();
@@ -97,7 +92,6 @@ protected function setUp(): void {
 			'core',
 			$this->request,
 			$this->avatarManager,
-			$this->cache,
 			$this->l,
 			$this->userManager,
 			$this->rootFolder,
@@ -297,25 +291,6 @@ public function testDeleteAvatarException(): void {
 		$this->assertEquals($expectedResponse, $this->avatarController->deleteAvatar());
 	}
 
-	/**
-	 * Trying to get a tmp avatar when it is not available. 404
-	 */
-	public function testTmpAvatarNoTmp(): void {
-		$response = $this->avatarController->getTmpAvatar();
-		$this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus());
-	}
-
-	/**
-	 * Fetch tmp avatar
-	 */
-	public function testTmpAvatarValid(): void {
-		$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.
 	 */
@@ -334,9 +309,6 @@ public function testPostAvatarFile(): void {
 		$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);
@@ -372,9 +344,6 @@ public function testPostAvatarFileGif(): void {
 		$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);