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 9ee344f7ed800..45c0daece0282 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 @@ public function __construct($appName, IRootFolder $rootFolder, ILogger $logger, $userId, - TimeFactory $timeFactory) { + TimeFactory $timeFactory, + IAccountManager $accountManager) { parent::__construct($appName, $request); $this->avatarManager = $avatarManager; @@ -109,6 +113,7 @@ public function __construct($appName, $this->logger = $logger; $this->userId = $userId; $this->timeFactory = $timeFactory; + $this->accountManager = $accountManager; } @@ -130,6 +135,19 @@ public function getAvatar($userId, $size) { $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 @@ public function getAvatar($userId, $size) { ['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 @@ public function getAvatar($userId, $size) { 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 45c0daece0282..28416264eb3c0 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 @@ public function getAvatar($userId, $size) { $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 @@ public function getAvatar($userId, $size) { 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 @@ public function getAvatar($userId, $size) { ['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 @@ public function getAvatar($userId, $size) { 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 5fce8fc6359f4..9441bfc12460d 100644 --- a/tests/Core/Controller/AvatarControllerTest.php +++ b/tests/Core/Controller/AvatarControllerTest.php @@ -33,6 +33,9 @@ function is_uploaded_file($filename) { 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 @@ function is_uploaded_file($filename) { 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 @@ protected function setUp() { $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 @@ protected function setUp() { $this->rootFolder, $this->logger, 'userid', - $this->timeFactory + $this->timeFactory, + $this->accountManager ); // Configure userMock @@ -137,6 +145,39 @@ public function testGetAvatarNoAvatar() { $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 */