Merge pull request #29559 from nextcloud/feat/28139/profile-respect-user-enumeration

Respect user enumeration settings on profile

apps/files_sharing/tests/CapabilitiesTest.php

@@ -28,6 +28,7 @@
  */
 namespace OCA\Files_Sharing\Tests;
 
+use OC\KnownUser\KnownUserService;
 use OC\Share20\Manager;
 use OCA\Files_Sharing\Capabilities;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -94,7 +95,8 @@ class CapabilitiesTest extends \Test\TestCase {
 			$this->createMock(IURLGenerator::class),
 			$this->createMock(\OC_Defaults::class),
 			$this->createMock(IEventDispatcher::class),
-			$this->createMock(IUserSession::class)
+			$this->createMock(IUserSession::class),
+			$this->createMock(KnownUserService::class)
 		);
 		$cap = new Capabilities($config, $shareManager);
 		$result = $this->getFilesSharingPart($cap->getCapabilities());

core/Controller/ProfilePageController.php

@@ -26,14 +26,18 @@ declare(strict_types=1);
 
 namespace OC\Core\Controller;
 
+use OC\KnownUser\KnownUserService;
+use OC\Profile\ProfileManager;
 use OCP\Accounts\IAccountManager;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
+use OCP\IGroupManager;
 use OCP\IRequest;
+use OCP\IUser;
 use OCP\IUserManager;
 use OCP\IUserSession;
-use OC\Profile\ProfileManager;
+use OCP\Share\IManager as IShareManager;
 use OCP\UserStatus\IManager as IUserStatusManager;
 
 class ProfilePageController extends Controller {
@@ -48,6 +52,15 @@ class ProfilePageController extends Controller {
 	/** @var ProfileManager */
 	private $profileManager;
 
+	/** @var IShareManager */
+	private $shareManager;
+
+	/** @var IGroupManager */
+	private $groupManager;
+
+	/** @var KnownUserService */
+	private $knownUserService;
+
 	/** @var IUserManager */
 	private $userManager;
 
@@ -63,6 +76,9 @@ class ProfilePageController extends Controller {
 		IInitialState $initialStateService,
 		IAccountManager $accountManager,
 		ProfileManager $profileManager,
+		IShareManager $shareManager,
+		IGroupManager $groupManager,
+		KnownUserService $knownUserService,
 		IUserManager $userManager,
 		IUserSession $userSession,
 		IUserStatusManager $userStatusManager
@@ -71,6 +87,9 @@ class ProfilePageController extends Controller {
 		$this->initialStateService = $initialStateService;
 		$this->accountManager = $accountManager;
 		$this->profileManager = $profileManager;
+		$this->shareManager = $shareManager;
+		$this->groupManager = $groupManager;
+		$this->knownUserService = $knownUserService;
 		$this->userManager = $userManager;
 		$this->userSession = $userSession;
 		$this->userStatusManager = $userStatusManager;
@@ -83,31 +102,34 @@ class ProfilePageController extends Controller {
 	 * @NoSubAdminRequired
 	 */
 	public function index(string $targetUserId): TemplateResponse {
-		if (!$this->userManager->userExists($targetUserId)) {
-			return new TemplateResponse(
-				'core',
-				'404-profile',
-				[],
-				TemplateResponse::RENDER_AS_GUEST,
-			);
-		}
+		$profileNotFoundTemplate = new TemplateResponse(
+			'core',
+			'404-profile',
+			[],
+			TemplateResponse::RENDER_AS_GUEST,
+		);
 
-		$visitingUser = $this->userSession->getUser();
 		$targetUser = $this->userManager->get($targetUserId);
+		if (!$targetUser instanceof IUser) {
+			return $profileNotFoundTemplate;
+		}
+		$visitingUser = $this->userSession->getUser();
 		$targetAccount = $this->accountManager->getAccount($targetUser);
 
 		if (!$this->isProfileEnabled($targetAccount)) {
-			return new TemplateResponse(
-				'core',
-				'404-profile',
-				[],
-				TemplateResponse::RENDER_AS_GUEST,
-			);
+			return $profileNotFoundTemplate;
+		}
+
+		// Run user enumeration checks only if viewing another user's profile
+		if ($targetUser !== $visitingUser) {
+			if (!$this->shareManager->currentUserCanEnumerateTargetUser($visitingUser, $targetUser)) {
+				return $profileNotFoundTemplate;
+			}
 		}
 
 		$userStatuses = $this->userStatusManager->getUserStatuses([$targetUserId]);
-		$status = array_shift($userStatuses);
-		if (!empty($status)) {
+		$status = $userStatuses[$targetUserId] ?? null;
+		if ($status !== null) {
 			$this->initialStateService->provideInitialState('status', [
 				'icon' => $status->getIcon(),
 				'message' => $status->getMessage(),

lib/private/Server.php

@@ -1253,7 +1253,8 @@ class Server extends ServerContainer implements IServerContainer {
 				$c->get(IURLGenerator::class),
 				$c->get('ThemingDefaults'),
 				$c->get(IEventDispatcher::class),
-				$c->get(IUserSession::class)
+				$c->get(IUserSession::class),
+				$c->get(KnownUserService::class)
 			);
 
 			return $manager;

lib/private/Share20/Manager.php

@@ -43,6 +43,7 @@ namespace OC\Share20;
 
 use OC\Cache\CappedMemoryCache;
 use OC\Files\Mount\MoveableMount;
+use OC\KnownUser\KnownUserService;
 use OC\Share20\Exception\ProviderException;
 use OCA\Files_Sharing\AppInfo\Application;
 use OCA\Files_Sharing\ISharedStorage;
@@ -118,7 +119,10 @@ class Manager implements IManager {
 	private $defaults;
 	/** @var IEventDispatcher */
 	private $dispatcher;
+	/** @var IUserSession */
 	private $userSession;
+	/** @var KnownUserService */
+	private $knownUserService;
 
 	public function __construct(
 		ILogger $logger,
@@ -137,7 +141,8 @@ class Manager implements IManager {
 		IURLGenerator $urlGenerator,
 		\OC_Defaults $defaults,
 		IEventDispatcher $dispatcher,
-		IUserSession $userSession
+		IUserSession $userSession,
+		KnownUserService $knownUserService
 	) {
 		$this->logger = $logger;
 		$this->config = $config;
@@ -160,6 +165,7 @@ class Manager implements IManager {
 		$this->defaults = $defaults;
 		$this->dispatcher = $dispatcher;
 		$this->userSession = $userSession;
+		$this->knownUserService = $knownUserService;
 	}
 
 	/**
@@ -1909,6 +1915,42 @@ class Manager implements IManager {
 		return $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
 	}
 
+	public function currentUserCanEnumerateTargetUser(?IUser $currentUser, IUser $targetUser): bool {
+		if ($this->allowEnumerationFullMatch()) {
+			return true;
+		}
+
+		if (!$this->allowEnumeration()) {
+			return false;
+		}
+
+		if (!$this->limitEnumerationToPhone() && !$this->limitEnumerationToGroups()) {
+			// Enumeration is enabled and not restricted: OK
+			return true;
+		}
+
+		if (!$currentUser instanceof IUser) {
+			// Enumeration restrictions require an account
+			return false;
+		}
+
+		// Enumeration is limited to phone match
+		if ($this->limitEnumerationToPhone() && $this->knownUserService->isKnownToUser($currentUser->getUID(), $targetUser->getUID())) {
+			return true;
+		}
+
+		// Enumeration is limited to groups
+		if ($this->limitEnumerationToGroups()) {
+			$currentUserGroupIds = $this->groupManager->getUserGroupIds($currentUser);
+			$targetUserGroupIds = $this->groupManager->getUserGroupIds($targetUser);
+			if (!empty(array_intersect($currentUserGroupIds, $targetUserGroupIds))) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * Copied from \OC_Util::isSharingDisabledForUser
 	 *

lib/public/Share/IManager.php

@@ -32,6 +32,7 @@ namespace OCP\Share;
 use OCP\Files\Folder;
 use OCP\Files\Node;
 
+use OCP\IUser;
 use OCP\Share\Exceptions\GenericShareException;
 use OCP\Share\Exceptions\ShareNotFound;
 
@@ -447,6 +448,16 @@ interface IManager {
 	 */
 	public function allowEnumerationFullMatch(): bool;
 
+	/**
+	 * Check if the current user can enumerate the target user
+	 *
+	 * @param IUser|null $currentUser
+	 * @param IUser $targetUser
+	 * @return bool
+	 * @since 23.0.0
+	 */
+	public function currentUserCanEnumerateTargetUser(?IUser $currentUser, IUser $targetUser): bool;
+
 	/**
 	 * Check if sharing is disabled for the given user
 	 *

tests/lib/Share20/ManagerTest.php

@@ -22,6 +22,7 @@
 namespace Test\Share20;
 
 use OC\Files\Mount\MoveableMount;
+use OC\KnownUser\KnownUserService;
 use OC\Share20\DefaultShareProvider;
 use OC\Share20\Exception;
 use OC\Share20\Manager;
@@ -53,6 +54,7 @@ use OCP\Security\IHasher;
 use OCP\Security\ISecureRandom;
 use OCP\Share\Exceptions\AlreadySharedException;
 use OCP\Share\Exceptions\ShareNotFound;
+use OCP\Share\IManager;
 use OCP\Share\IProviderFactory;
 use OCP\Share\IShare;
 use OCP\Share\IShareProvider;
@@ -105,7 +107,10 @@ class ManagerTest extends \Test\TestCase {
 	protected $urlGenerator;
 	/** @var  \OC_Defaults|MockObject */
 	protected $defaults;
+	/** @var IUserSession|MockObject  */
 	protected $userSession;
+	/** @var KnownUserService|MockObject  */
+	protected $knownUserService;
 
 	protected function setUp(): void {
 		$this->logger = $this->createMock(ILogger::class);
@@ -122,6 +127,7 @@ class ManagerTest extends \Test\TestCase {
 		$this->defaults = $this->createMock(\OC_Defaults::class);
 		$this->dispatcher = $this->createMock(IEventDispatcher::class);
 		$this->userSession = $this->createMock(IUserSession::class);
+		$this->knownUserService = $this->createMock(KnownUserService::class);
 
 		$this->l10nFactory = $this->createMock(IFactory::class);
 		$this->l = $this->createMock(IL10N::class);
@@ -153,7 +159,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$this->defaultProvider = $this->createMock(DefaultShareProvider::class);
@@ -165,7 +172,7 @@ class ManagerTest extends \Test\TestCase {
 	 * @return MockBuilder
 	 */
 	private function createManagerMock() {
-		return 	$this->getMockBuilder('\OC\Share20\Manager')
+		return $this->getMockBuilder(Manager::class)
 			->setConstructorArgs([
 				$this->logger,
 				$this->config,
@@ -183,7 +190,8 @@ class ManagerTest extends \Test\TestCase {
 				$this->urlGenerator,
 				$this->defaults,
 				$this->dispatcher,
-				$this->userSession
+				$this->userSession,
+				$this->knownUserService
 			]);
 	}
 
@@ -2696,7 +2704,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$share = $this->createMock(IShare::class);
@@ -2741,7 +2750,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$share = $this->createMock(IShare::class);
@@ -2793,7 +2803,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$share = $this->createMock(IShare::class);
@@ -4132,7 +4143,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 		$this->assertSame($expected,
 			$manager->shareProviderExists($shareType)
@@ -4166,7 +4178,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$factory->setProvider($this->defaultProvider);
@@ -4231,7 +4244,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$factory->setProvider($this->defaultProvider);
@@ -4348,7 +4362,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$factory->setProvider($this->defaultProvider);
@@ -4474,7 +4489,8 @@ class ManagerTest extends \Test\TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->dispatcher,
-			$this->userSession
+			$this->userSession,
+			$this->knownUserService
 		);
 
 		$factory->setProvider($this->defaultProvider);
@@ -4506,6 +4522,83 @@ class ManagerTest extends \Test\TestCase {
 
 		$this->assertSame($expects, $result);
 	}
+
+	public function dataCurrentUserCanEnumerateTargetUser(): array {
+		return [
+			'Full match guest' => [true, true, false, false, false, false, false, true],
+			'Full match user' => [false, true, false, false, false, false, false, true],
+			'Enumeration off guest' => [true, false, false, false, false, false, false, false],
+			'Enumeration off user' => [false, false, false, false, false, false, false, false],
+			'Enumeration guest' => [true, false, true, false, false, false, false, true],
+			'Enumeration user' => [false, false, true, false, false, false, false, true],
+
+			// Restricted enumerations guests never works
+			'Guest phone' => [true, false, true, true, false, false, false, false],
+			'Guest group' => [true, false, true, false, true, false, false, false],
+			'Guest both' => [true, false, true, true, true, false, false, false],
+
+			// Restricted enumerations users
+			'User phone but not known' => [false, false, true, true, false, false, false, false],
+			'User phone known' => [false, false, true, true, false, true, false, true],
+			'User group but no match' => [false, false, true, false, true, false, false, false],
+			'User group with match' => [false, false, true, false, true, false, true, true],
+		];
+	}
+
+	/**
+	 * @dataProvider dataCurrentUserCanEnumerateTargetUser
+	 * @param bool $expected
+	 */
+	public function testCurrentUserCanEnumerateTargetUser(bool $currentUserIsGuest, bool $allowEnumerationFullMatch, bool $allowEnumeration, bool $limitEnumerationToPhone, bool $limitEnumerationToGroups, bool $isKnownToUser, bool $haveCommonGroup, bool $expected): void {
+		/** @var IManager|MockObject $manager */
+		$manager = $this->createManagerMock()
+			->setMethods([
+				'allowEnumerationFullMatch',
+				'allowEnumeration',
+				'limitEnumerationToPhone',
+				'limitEnumerationToGroups',
+			])
+		->getMock();
+
+		$manager->method('allowEnumerationFullMatch')
+			->willReturn($allowEnumerationFullMatch);
+		$manager->method('allowEnumeration')
+			->willReturn($allowEnumeration);
+		$manager->method('limitEnumerationToPhone')
+			->willReturn($limitEnumerationToPhone);
+		$manager->method('limitEnumerationToGroups')
+			->willReturn($limitEnumerationToGroups);
+
+		$this->knownUserService->method('isKnownToUser')
+			->with('current', 'target')
+			->willReturn($isKnownToUser);
+
+		$currentUser = null;
+		if (!$currentUserIsGuest) {
+			$currentUser = $this->createMock(IUser::class);
+			$currentUser->method('getUID')
+				->willReturn('current');
+		}
+		$targetUser = $this->createMock(IUser::class);
+		$targetUser->method('getUID')
+			->willReturn('target');
+
+		if ($haveCommonGroup) {
+			$this->groupManager->method('getUserGroupIds')
+				->willReturnMap([
+					[$targetUser, ['gid1', 'gid2']],
+					[$currentUser, ['gid2', 'gid3']],
+				]);
+		} else {
+			$this->groupManager->method('getUserGroupIds')
+				->willReturnMap([
+					[$targetUser, ['gid1', 'gid2']],
+					[$currentUser, ['gid3', 'gid4']],
+				]);
+		}
+
+		$this->assertSame($expected, $manager->currentUserCanEnumerateTargetUser($currentUser, $targetUser));
+	}
 }
 
 class DummyFactory implements IProviderFactory {