From 0bd1378f819915529d17a53147c0a43ff10c09f1 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 28 Oct 2019 13:48:34 +0100 Subject: [PATCH 1/2] Honor avatar visibility settings Fixes #5456 Only when an avatar is set to public should we show it to the public. For now this has an open question as to how to solve federated avatars. But I assume a dedicated paramter or endpooint would make sense there. Signed-off-by: Roeland Jago Douma --- core/Controller/AvatarController.php | 30 ++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index 9ee344f7ed..45c0daece0 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -28,6 +28,7 @@ namespace OC\Core\Controller; use OC\AppFramework\Utility\TimeFactory; +use OCP\Accounts\IAccountManager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataDisplayResponse; @@ -76,6 +77,8 @@ class AvatarController extends Controller { /** @var TimeFactory */ protected $timeFactory; + /** @var IAccountManager */ + private $accountManager; /** * @param string $appName @@ -98,7 +101,8 @@ class AvatarController extends Controller { IRootFolder $rootFolder, ILogger $logger, $userId, - TimeFactory $timeFactory) { + TimeFactory $timeFactory, + IAccountManager $accountManager) { parent::__construct($appName, $request); $this->avatarManager = $avatarManager; @@ -109,6 +113,7 @@ class AvatarController extends Controller { $this->logger = $logger; $this->userId = $userId; $this->timeFactory = $timeFactory; + $this->accountManager = $accountManager; } @@ -130,6 +135,19 @@ class AvatarController extends Controller { $size = 64; } + $user = $this->userManager->get($userId); + if ($user === null) { + return $this->return404(); + } + + $account = $this->accountManager->getAccount($user); + $scope = $account->getProperty(IAccountManager::PROPERTY_AVATAR)->getScope(); + + if ($scope !== IAccountManager::VISIBILITY_PUBLIC && $this->userId === null) { + // Public avatar access is not allowed + return $this->return404(); + } + try { $avatar = $this->avatarManager->getAvatar($userId); $avatarFile = $avatar->getFile($size); @@ -139,9 +157,7 @@ class AvatarController extends Controller { ['Content-Type' => $avatarFile->getMimeType()] ); } catch (\Exception $e) { - $resp = new Http\Response(); - $resp->setStatus(Http::STATUS_NOT_FOUND); - return $resp; + return $this->return404(); } // Cache for 30 minutes @@ -149,6 +165,12 @@ class AvatarController extends Controller { return $resp; } + private function return404(): Http\Response { + $resp = new Http\Response(); + $resp->setStatus(Http::STATUS_NOT_FOUND); + return $resp; + } + /** * @NoAdminRequired * From 54eb27dab2e3b5fe7a91b0fc388ae52de311d660 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 13 Nov 2019 20:43:20 +0100 Subject: [PATCH 2/2] Update tests Signed-off-by: Roeland Jago Douma --- core/Controller/AvatarController.php | 24 ++--------- .../Core/Controller/AvatarControllerTest.php | 43 ++++++++++++++++++- 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index 45c0daece0..28416264eb 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -80,18 +80,6 @@ class AvatarController extends Controller { /** @var IAccountManager */ private $accountManager; - /** - * @param string $appName - * @param IRequest $request - * @param IAvatarManager $avatarManager - * @param ICache $cache - * @param IL10N $l10n - * @param IUserManager $userManager - * @param IRootFolder $rootFolder - * @param ILogger $logger - * @param string $userId - * @param TimeFactory $timeFactory - */ public function __construct($appName, IRequest $request, IAvatarManager $avatarManager, @@ -137,7 +125,7 @@ class AvatarController extends Controller { $user = $this->userManager->get($userId); if ($user === null) { - return $this->return404(); + return new JSONResponse([], Http::STATUS_NOT_FOUND); } $account = $this->accountManager->getAccount($user); @@ -145,7 +133,7 @@ class AvatarController extends Controller { if ($scope !== IAccountManager::VISIBILITY_PUBLIC && $this->userId === null) { // Public avatar access is not allowed - return $this->return404(); + return new JSONResponse([], Http::STATUS_NOT_FOUND); } try { @@ -157,7 +145,7 @@ class AvatarController extends Controller { ['Content-Type' => $avatarFile->getMimeType()] ); } catch (\Exception $e) { - return $this->return404(); + return new JSONResponse([], Http::STATUS_NOT_FOUND); } // Cache for 30 minutes @@ -165,12 +153,6 @@ class AvatarController extends Controller { return $resp; } - private function return404(): Http\Response { - $resp = new Http\Response(); - $resp->setStatus(Http::STATUS_NOT_FOUND); - return $resp; - } - /** * @NoAdminRequired * diff --git a/tests/Core/Controller/AvatarControllerTest.php b/tests/Core/Controller/AvatarControllerTest.php index 5fce8fc635..9441bfc124 100644 --- a/tests/Core/Controller/AvatarControllerTest.php +++ b/tests/Core/Controller/AvatarControllerTest.php @@ -33,6 +33,9 @@ namespace Tests\Core\Controller; use OC\AppFramework\Utility\TimeFactory; use OC\Core\Controller\AvatarController; +use OCP\Accounts\IAccount; +use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; use OCP\AppFramework\Http; use OCP\ICache; use OCP\Files\File; @@ -46,6 +49,7 @@ use OCP\ILogger; use OCP\IRequest; use OCP\IUser; use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; /** * Class AvatarControllerTest @@ -78,6 +82,8 @@ class AvatarControllerTest extends \Test\TestCase { private $request; /** @var TimeFactory|\PHPUnit_Framework_MockObject_MockObject */ private $timeFactory; + /** @var IAccountManager|MockObject */ + private $accountManager; protected function setUp() { parent::setUp(); @@ -92,6 +98,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->rootFolder = $this->getMockBuilder('OCP\Files\IRootFolder')->getMock(); $this->logger = $this->getMockBuilder(ILogger::class)->getMock(); $this->timeFactory = $this->getMockBuilder('OC\AppFramework\Utility\TimeFactory')->getMock(); + $this->accountManager = $this->createMock(IAccountManager::class); $this->avatarMock = $this->getMockBuilder('OCP\IAvatar')->getMock(); $this->userMock = $this->getMockBuilder(IUser::class)->getMock(); @@ -106,7 +113,8 @@ class AvatarControllerTest extends \Test\TestCase { $this->rootFolder, $this->logger, 'userid', - $this->timeFactory + $this->timeFactory, + $this->accountManager ); // Configure userMock @@ -137,6 +145,39 @@ class AvatarControllerTest extends \Test\TestCase { $this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus()); } + public function testAvatarNotPublic() { + $account = $this->createMock(IAccount::class); + $this->accountManager->method('getAccount') + ->with($this->userMock) + ->willReturn($account); + + $property = $this->createMock(IAccountProperty::class); + $account->method('getProperty') + ->with(IAccountManager::PROPERTY_AVATAR) + ->willReturn($property); + + $property->method('getScope') + ->willReturn(IAccountManager::VISIBILITY_PRIVATE); + + $controller = new AvatarController( + 'core', + $this->request, + $this->avatarManager, + $this->cache, + $this->l, + $this->userManager, + $this->rootFolder, + $this->logger, + null, + $this->timeFactory, + $this->accountManager + ); + + $result = $controller->getAvatar('userId', 128); + + $this->assertEquals(Http::STATUS_NOT_FOUND, $result->getStatus()); + } + /** * Fetch the user's avatar */