Mirrors
/
nextcloud-server
mirror of https://github.com/nextcloud/server
3
0
Fork 0
Code Releases Activity
Browse Source

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

Signed-off-by: provokateurin <kate@provokateurin.de>
pull/55134/head
provokateurin 4 months ago
parent
fab035e3c0
commit
bfb4fe8f86
Failed to extract signature
5 changed files with 12 additions and 369 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 3
      apps/settings/src/components/PersonalInfo/AvatarSection.vue
  2. 128
      build/integration/features/avatar.feature
  3. 53
      build/integration/features/bootstrap/Avatar.php
  4. 79
      core/Controller/AvatarController.php
  5. 118
      tests/Core/Controller/AvatarControllerTest.php

3
apps/settings/src/components/PersonalInfo/AvatarSection.vue
View File

@ -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)

128
build/integration/features/avatar.feature
View File

@ -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

53
build/integration/features/bootstrap/Avatar.php
View File

@ -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
*/

79
core/Controller/AvatarController.php
View File

@ -8,7 +8,6 @@
namespace OC\Core\Controller;
use OC\AppFramework\Utility\TimeFactory;
use OC\NotSquareException;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\FrontpageRoute;
@ -16,7 +15,6 @@ use OCP\AppFramework\Http\Attribute\NoAdminRequired;
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;
@ -24,7 +22,6 @@ use OCP\Files\File;
use OCP\Files\IRootFolder;
use OCP\Files\NotPermittedException;
use OCP\IAvatarManager;
use OCP\ICache;
use OCP\IL10N;
use OCP\Image;
use OCP\IRequest;
@ -41,7 +38,6 @@ class AvatarController extends Controller {
string $appName,
IRequest $request,
protected IAvatarManager $avatarManager,
protected ICache $cache,
protected IL10N $l10n,
protected IUserManager $userManager,
protected IRootFolder $rootFolder,
@ -202,8 +198,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 = [
@ -250,8 +245,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']);
@ -259,9 +252,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 {
@ -288,71 +280,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 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 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 (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);
}
}
}

118
tests/Core/Controller/AvatarControllerTest.php
View File

@ -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(): 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.
*/
@ -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(): void {
$this->cache->expects($this->once())
->method('set')
->willThrowException(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(): void {
$response = $this->avatarController->postCroppedAvatar([]);
$this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus());
}
/**
* Test no tmp avatar to crop
*/
public function testPostCroppedAvatarNoTmpAvatar(): void {
$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(): void {
$this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.jpg'));
$this->avatarMock->method('set')->willThrowException(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(): void {
$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(): void {
$this->cache->method('get')->willReturn(file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.jpg'));
$this->avatarMock->method('set')->willThrowException(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
*/

Write Preview
Loading…
Cancel
Save