Add known user check in avatar when v2-private scope

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

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();
+		$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);
-			}
+		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);

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] = [];

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

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() {
+	public function testGetAvatarForSelf() {
+		$user = $this->createMock(IUser::class);
+		$user
+			->expects($this->any())
+			->method('getUID')
+			->willReturn('valid-user');
+
 		// requesting user
 		$this->userSession->expects($this->once())
 			->method('getUser')
-			->willReturn($this->createMock(IUser::class));
+			->willReturn($user);
 
-		// we skip account scope check for logged in user
-		$this->accountManager->expects($this->never())
-			->method('getAccount');
-
-		$user = $this->createMock(IUser::class);
-		$user
-			->expects($this->once())
-			->method('getUID')
-			->willReturn('valid-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);
 
-		$expected = new PlaceholderAvatar($folder, $user, $this->createMock(ILogger::class));
+		$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');
+		}
+
+		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'));
 	}
 }