diff --git a/apps/files_sharing/tests/CapabilitiesTest.php b/apps/files_sharing/tests/CapabilitiesTest.php index eb10c9271406a..01ef13a3e6c0d 100644 --- a/apps/files_sharing/tests/CapabilitiesTest.php +++ b/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 @@ private function getResults(array $map) { $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()); diff --git a/core/Controller/ProfilePageController.php b/core/Controller/ProfilePageController.php index a7ceb404fbc24..e4064370d9c7d 100644 --- a/core/Controller/ProfilePageController.php +++ b/core/Controller/ProfilePageController.php @@ -26,14 +26,18 @@ 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 @@ public function __construct( IInitialState $initialStateService, IAccountManager $accountManager, ProfileManager $profileManager, + IShareManager $shareManager, + IGroupManager $groupManager, + KnownUserService $knownUserService, IUserManager $userManager, IUserSession $userSession, IUserStatusManager $userStatusManager @@ -71,6 +87,9 @@ public function __construct( $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 @@ public function __construct( * @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(), diff --git a/lib/private/Server.php b/lib/private/Server.php index 635bd80d4f8ce..baebbe7558d92 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1253,7 +1253,8 @@ public function __construct($webRoot, \OC\Config $config) { $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; diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index ccc2d454a94de..1891e3a128337 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -43,6 +43,7 @@ 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 @@ public function __construct( IURLGenerator $urlGenerator, \OC_Defaults $defaults, IEventDispatcher $dispatcher, - IUserSession $userSession + IUserSession $userSession, + KnownUserService $knownUserService ) { $this->logger = $logger; $this->config = $config; @@ -160,6 +165,7 @@ public function __construct( $this->defaults = $defaults; $this->dispatcher = $dispatcher; $this->userSession = $userSession; + $this->knownUserService = $knownUserService; } /** @@ -1909,6 +1915,42 @@ public function allowEnumerationFullMatch(): bool { 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 * diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index 77a9980a8946f..8b1f5144b9aa3 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -32,6 +32,7 @@ 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 @@ public function limitEnumerationToPhone(): bool; */ 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 * diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index de8dc9fcc862d..f1a53200f5f36 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/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\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 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $this->defaultProvider = $this->createMock(DefaultShareProvider::class); @@ -165,7 +172,7 @@ protected function setUp(): void { * @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 @@ private function createManagerMock() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ]); } @@ -2696,7 +2704,8 @@ public function testGetShareByToken() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $share = $this->createMock(IShare::class); @@ -2741,7 +2750,8 @@ public function testGetShareByTokenRoom() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $share = $this->createMock(IShare::class); @@ -2793,7 +2803,8 @@ public function testGetShareByTokenWithException() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $share = $this->createMock(IShare::class); @@ -4132,7 +4143,8 @@ public function testShareProviderExists($shareType, $expected) { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $this->assertSame($expected, $manager->shareProviderExists($shareType) @@ -4166,7 +4178,8 @@ public function testGetSharesInFolder() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $factory->setProvider($this->defaultProvider); @@ -4231,7 +4244,8 @@ public function testGetAccessList() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $factory->setProvider($this->defaultProvider); @@ -4348,7 +4362,8 @@ public function testGetAccessListWithCurrentAccess() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $factory->setProvider($this->defaultProvider); @@ -4474,7 +4489,8 @@ public function testGetAllShares() { $this->urlGenerator, $this->defaults, $this->dispatcher, - $this->userSession + $this->userSession, + $this->knownUserService ); $factory->setProvider($this->defaultProvider); @@ -4506,6 +4522,83 @@ public function testGetAllShares() { $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 {