From ec492eadfa157a765e6ddf7fb9b64963cf64ef54 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 14:14:14 +0100 Subject: [PATCH] Add known user check in avatar when v2-private scope Signed-off-by: Vincent Petry --- lib/private/Avatar/AvatarManager.php | 35 ++++--- lib/private/KnownUser/KnownUserService.php | 4 + lib/private/Server.php | 4 +- tests/lib/Avatar/AvatarManagerTest.php | 111 +++++++++++++++++---- 4 files changed, 119 insertions(+), 35 deletions(-) diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 92cd502dacb..04d3a721022 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -34,6 +34,7 @@ declare(strict_types=1); namespace OC\Avatar; +use OC\KnownUser\KnownUserService; use OC\User\Manager; use OC\User\NoUserException; use OCP\Accounts\IAccountManager; @@ -73,6 +74,9 @@ class AvatarManager implements IAvatarManager { /** @var IAccountManager */ private $accountManager; + /** @var KnownUserService */ + private $knownUserService; + /** * AvatarManager constructor. * @@ -90,7 +94,9 @@ class AvatarManager implements IAvatarManager { IL10N $l, ILogger $logger, IConfig $config, - IAccountManager $accountManager) { + IAccountManager $accountManager, + KnownUserService $knownUserService + ) { $this->userSession = $userSession; $this->userManager = $userManager; $this->appData = $appData; @@ -98,6 +104,7 @@ class AvatarManager implements IAvatarManager { $this->logger = $logger; $this->config = $config; $this->accountManager = $accountManager; + $this->knownUserService = $knownUserService; } /** @@ -128,17 +135,21 @@ class AvatarManager implements IAvatarManager { $folder = $this->appData->newFolder($userId); } - // requesting in public page - if ($requestingUser === null) { - $account = $this->accountManager->getAccount($user); - $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); - $avatarScope = $avatarProperties->getScope(); - - // v2-private scope hides the avatar from public access - if ($avatarScope === IAccountManager::SCOPE_PRIVATE) { - // use a placeholder avatar which caches the generated images - return new PlaceholderAvatar($folder, $user, $this->logger); - } + $account = $this->accountManager->getAccount($user); + $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); + $avatarScope = $avatarProperties->getScope(); + + if ( + // v2-private scope hides the avatar from public access and from unknown users + $avatarScope === IAccountManager::SCOPE_PRIVATE + && ( + // accessing from public link + $requestingUser === null + // logged in, but unknown to user + || !$this->knownUserService->isKnownToUser($requestingUser->getUID(), $userId) + )) { + // use a placeholder avatar which caches the generated images + return new PlaceholderAvatar($folder, $user, $this->logger); } return new UserAvatar($folder, $this->l, $user, $this->logger, $this->config); diff --git a/lib/private/KnownUser/KnownUserService.php b/lib/private/KnownUser/KnownUserService.php index 96af21c836f..1f300a9f8e4 100644 --- a/lib/private/KnownUser/KnownUserService.php +++ b/lib/private/KnownUser/KnownUserService.php @@ -74,6 +74,10 @@ class KnownUserService { * @return bool */ public function isKnownToUser(string $knownTo, string $contactUserId): bool { + if ($knownTo === $contactUserId) { + return true; + } + if (!isset($this->knownUsers[$knownTo])) { $entities = $this->mapper->getKnownUsers($knownTo); $this->knownUsers[$knownTo] = []; diff --git a/lib/private/Server.php b/lib/private/Server.php index 93ad3b38997..26c76125e56 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -104,6 +104,7 @@ use OC\IntegrityCheck\Checker; use OC\IntegrityCheck\Helpers\AppLocator; use OC\IntegrityCheck\Helpers\EnvironmentHelper; use OC\IntegrityCheck\Helpers\FileAccessHelper; +use OC\KnownUser\KnownUserService; use OC\Lock\DBLockingProvider; use OC\Lock\MemcacheLockingProvider; use OC\Lock\NoopLockingProvider; @@ -726,7 +727,8 @@ class Server extends ServerContainer implements IServerContainer { $c->getL10N('lib'), $c->get(ILogger::class), $c->get(\OCP\IConfig::class), - $c->get(IAccountManager::class) + $c->get(IAccountManager::class), + $c->get(KnownUserService::class) ); }); $this->registerAlias(IAvatarManager::class, AvatarManager::class); diff --git a/tests/lib/Avatar/AvatarManagerTest.php b/tests/lib/Avatar/AvatarManagerTest.php index 1b06e786fb1..d3bc60efb59 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -27,6 +27,7 @@ namespace Test\Avatar; use OC\Avatar\AvatarManager; use OC\Avatar\PlaceholderAvatar; use OC\Avatar\UserAvatar; +use OC\KnownUser\KnownUserService; use OC\User\Manager; use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; @@ -59,6 +60,8 @@ class AvatarManagerTest extends \Test\TestCase { private $accountManager; /** @var AvatarManager | \PHPUnit\Framework\MockObject\MockObject */ private $avatarManager; + /** @var KnownUserService | \PHPUnit\Framework\MockObject\MockObject */ + private $knownUserService; protected function setUp(): void { parent::setUp(); @@ -70,6 +73,7 @@ class AvatarManagerTest extends \Test\TestCase { $this->logger = $this->createMock(ILogger::class); $this->config = $this->createMock(IConfig::class); $this->accountManager = $this->createMock(IAccountManager::class); + $this->knownUserService = $this->createMock(KnownUserService::class); $this->avatarManager = new AvatarManager( $this->userSession, @@ -78,7 +82,8 @@ class AvatarManagerTest extends \Test\TestCase { $this->l10n, $this->logger, $this->config, - $this->accountManager + $this->accountManager, + $this->knownUserService ); } @@ -95,26 +100,45 @@ class AvatarManagerTest extends \Test\TestCase { $this->avatarManager->getAvatar('invalidUser'); } - public function testGetAvatarValidUser() { - // requesting user - $this->userSession->expects($this->once()) - ->method('getUser') - ->willReturn($this->createMock(IUser::class)); - - // we skip account scope check for logged in user - $this->accountManager->expects($this->never()) - ->method('getAccount'); - + public function testGetAvatarForSelf() { $user = $this->createMock(IUser::class); $user - ->expects($this->once()) + ->expects($this->any()) ->method('getUID') ->willReturn('valid-user'); + + // requesting user + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($user); + $this->userManager ->expects($this->once()) ->method('get') ->with('valid-user') ->willReturn($user); + + $account = $this->createMock(IAccount::class); + $this->accountManager->expects($this->once()) + ->method('getAccount') + ->with($user) + ->willReturn($account); + + $property = $this->createMock(IAccountProperty::class); + $account->expects($this->once()) + ->method('getProperty') + ->with(IAccountManager::PROPERTY_AVATAR) + ->willReturn($property); + + $property->expects($this->once()) + ->method('getScope') + ->willReturn(IAccountManager::SCOPE_PRIVATE); + + $this->knownUserService->expects($this->any()) + ->method('isKnownToUser') + ->with('valid-user', 'valid-user') + ->willReturn(true); + $folder = $this->createMock(ISimpleFolder::class); $this->appData ->expects($this->once()) @@ -148,7 +172,36 @@ class AvatarManagerTest extends \Test\TestCase { $this->assertEquals($expected, $this->avatarManager->getAvatar('vaLid-USER')); } - public function testGetAvatarPrivateScope() { + public function knownUnknownProvider() { + return [ + [IAccountManager::SCOPE_LOCAL, false, false, false], + [IAccountManager::SCOPE_LOCAL, true, false, false], + + // public access cannot see real avatar + [IAccountManager::SCOPE_PRIVATE, true, false, true], + // unknown users cannot see real avatar + [IAccountManager::SCOPE_PRIVATE, false, false, true], + // known users can see real avatar + [IAccountManager::SCOPE_PRIVATE, false, true, false], + ]; + } + + /** + * @dataProvider knownUnknownProvider + */ + public function testGetAvatarScopes($avatarScope, $isPublicCall, $isKnownUser, $expectedPlaceholder) { + if ($isPublicCall) { + $requestingUser = null; + } else { + $requestingUser = $this->createMock(IUser::class); + $requestingUser->method('getUID')->willReturn('requesting-user'); + } + + // requesting user + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($requestingUser); + $user = $this->createMock(IUser::class); $user ->expects($this->once()) @@ -160,13 +213,6 @@ class AvatarManagerTest extends \Test\TestCase { ->with('valid-user') ->willReturn($user); - $folder = $this->createMock(ISimpleFolder::class); - $this->appData - ->expects($this->once()) - ->method('getFolder') - ->with('valid-user') - ->willReturn($folder); - $account = $this->createMock(IAccount::class); $this->accountManager->expects($this->once()) ->method('getAccount') @@ -181,9 +227,30 @@ class AvatarManagerTest extends \Test\TestCase { $property->expects($this->once()) ->method('getScope') - ->willReturn(IAccountManager::SCOPE_PRIVATE); + ->willReturn($avatarScope); + + $folder = $this->createMock(ISimpleFolder::class); + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('valid-user') + ->willReturn($folder); + + if (!$isPublicCall) { + $this->knownUserService->expects($this->any()) + ->method('isKnownToUser') + ->with('requesting-user', 'valid-user') + ->willReturn($isKnownUser); + } else { + $this->knownUserService->expects($this->never()) + ->method('isKnownToUser'); + } - $expected = new PlaceholderAvatar($folder, $user, $this->createMock(ILogger::class)); + if ($expectedPlaceholder) { + $expected = new PlaceholderAvatar($folder, $user, $this->createMock(ILogger::class)); + } else { + $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config); + } $this->assertEquals($expected, $this->avatarManager->getAvatar('valid-user')); } } -- 2.39.5