From 9490f1ecdf58cc57a2633c2900abd6a17bcd5e1e Mon Sep 17 00:00:00 2001 From: SebastianKrupinski Date: Tue, 18 Aug 2026 11:31:29 -0400 Subject: [PATCH] fix: system address book card restriction Signed-off-by: SebastianKrupinski --- apps/dav/lib/CardDAV/SystemAddressbook.php | 237 ++++--- apps/dav/lib/CardDAV/UserAddressBooks.php | 6 +- .../unit/CardDAV/SystemAddressBookTest.php | 652 +++++++++++++++--- build/psalm-baseline.xml | 13 - 4 files changed, 713 insertions(+), 195 deletions(-) diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php index 6984298d1c2cb..8bdb24ee6b454 100644 --- a/apps/dav/lib/CardDAV/SystemAddressbook.php +++ b/apps/dav/lib/CardDAV/SystemAddressbook.php @@ -10,11 +10,13 @@ use OCA\Federation\TrustedServers; use OCP\Accounts\IAccountManager; -use OCP\IConfig; +use OCP\IGroup; use OCP\IGroupManager; use OCP\IL10N; use OCP\IRequest; +use OCP\IUser; use OCP\IUserSession; +use OCP\Share\IManager as IShareManager; use Sabre\CardDAV\Backend\BackendInterface; use Sabre\CardDAV\Backend\SyncSupport; use Sabre\CardDAV\Card; @@ -23,7 +25,6 @@ use Sabre\VObject\Component\VCard; use Sabre\VObject\Reader; use function array_filter; -use function array_intersect; use function array_unique; use function in_array; @@ -34,7 +35,7 @@ public function __construct( BackendInterface $carddavBackend, array $addressBookInfo, IL10N $l10n, - private IConfig $config, + private IShareManager $shareManager, private IUserSession $userSession, private ?IRequest $request = null, private ?TrustedServers $trustedServers = null, @@ -52,12 +53,14 @@ public function __construct( * 'Allow username autocompletion in share dialog' + 'Allow username autocompletion to users within the same groups' -> show only users in intersecting groups * 'Allow username autocompletion in share dialog' + 'Allow username autocompletion to users based on phone number integration' -> show only the same user * 'Allow username autocompletion in share dialog' + 'Allow username autocompletion to users within the same groups' + 'Allow username autocompletion to users based on phone number integration' -> show only users in intersecting groups + * 'Restrict users to only share with users in their groups' -> show only users in intersecting groups, not counting the groups excluded from that check */ #[\Override] public function getChildren() { - $shareEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; - $shareEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; - $shareEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes'; + $shareEnumeration = $this->shareManager->allowEnumeration(); + $shareEnumerationGroup = $this->shareManager->limitEnumerationToGroups(); + $shareEnumerationPhone = $this->shareManager->limitEnumerationToPhone(); + $restrictToOwnGroups = $this->shareManager->shareWithGroupMembersOnly(); $user = $this->userSession->getUser(); if (!$user) { // Should never happen because we don't allow anonymous access @@ -71,23 +74,12 @@ public function getChildren() { return []; } } - if ($shareEnumerationGroup) { + if ($shareEnumerationGroup || $restrictToOwnGroups) { if ($this->groupManager === null) { // Group manager is not available, so we can't determine which data is safe return []; } - $groups = $this->groupManager->getUserGroups($user); - $names = []; - foreach ($groups as $group) { - $users = $group->getUsers(); - foreach ($users as $groupUser) { - if ($groupUser->getBackendClassName() === 'Guests') { - continue; - } - $names[] = SyncService::getCardUri($groupUser); - } - } - return parent::getMultipleChildren(array_unique($names)); + return parent::getMultipleChildren($this->getCardsForUsersGroups($user)); } $children = parent::getChildren(); @@ -104,9 +96,33 @@ public function getChildren() { */ #[\Override] public function getMultipleChildren($paths): array { - $shareEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; - $shareEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; - $shareEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes'; + // Trusted servers have no session user, so they get no cards while group restrictions apply and bypass the others + if ($this->isFederation()) { + if ($this->shareManager->limitEnumerationToGroups() || $this->shareManager->shareWithGroupMembersOnly()) { + return []; + } + $objs = $this->carddavBackend->getMultipleCards($this->addressBookInfo['id'], $paths); + $children = []; + /** @var array $obj */ + foreach ($objs as $obj) { + if (empty($obj)) { + continue; + } + $carddata = $this->extractCarddata($obj); + if (empty($carddata)) { + continue; + } else { + $obj['carddata'] = $carddata; + } + $children[] = new Card($this->carddavBackend, $this->addressBookInfo, $obj); + } + return $children; + } + + $shareEnumeration = $this->shareManager->allowEnumeration(); + $shareEnumerationGroup = $this->shareManager->limitEnumerationToGroups(); + $shareEnumerationPhone = $this->shareManager->limitEnumerationToPhone(); + $restrictToOwnGroups = $this->shareManager->shareWithGroupMembersOnly(); $user = $this->userSession->getUser(); if (($user !== null && $user->getBackendClassName() === 'Guests') || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) { // No user or cards with no access @@ -120,44 +136,14 @@ public function getMultipleChildren($paths): array { return []; } } - if ($shareEnumerationGroup) { + if ($shareEnumerationGroup || $restrictToOwnGroups) { if ($this->groupManager === null || $user === null) { // Group manager or user is not available, so we can't determine which data is safe return []; } - $groups = $this->groupManager->getUserGroups($user); - $allowedNames = []; - foreach ($groups as $group) { - $users = $group->getUsers(); - foreach ($users as $groupUser) { - if ($groupUser->getBackendClassName() === 'Guests') { - continue; - } - $allowedNames[] = SyncService::getCardUri($groupUser); - } - } - return parent::getMultipleChildren(array_intersect($paths, $allowedNames)); - } - if (!$this->isFederation()) { - return parent::getMultipleChildren($paths); - } - - $objs = $this->carddavBackend->getMultipleCards($this->addressBookInfo['id'], $paths); - $children = []; - /** @var array $obj */ - foreach ($objs as $obj) { - if (empty($obj)) { - continue; - } - $carddata = $this->extractCarddata($obj); - if (empty($carddata)) { - continue; - } else { - $obj['carddata'] = $carddata; - } - $children[] = new Card($this->carddavBackend, $this->addressBookInfo, $obj); + return parent::getMultipleChildren($this->filterAccessibleCardUris($user, $paths)); } - return $children; + return parent::getMultipleChildren($paths); } /** @@ -168,10 +154,29 @@ public function getMultipleChildren($paths): array { */ #[\Override] public function getChild($name): Card { + // Trusted servers have no session user, so they get no cards while group restrictions apply and bypass the others + if ($this->isFederation()) { + if ($this->shareManager->limitEnumerationToGroups() || $this->shareManager->shareWithGroupMembersOnly()) { + throw new Forbidden(); + } + $obj = $this->carddavBackend->getCard($this->addressBookInfo['id'], $name); + if (!$obj) { + throw new NotFound('Card not found'); + } + $carddata = $this->extractCarddata($obj); + if (empty($carddata)) { + throw new Forbidden(); + } else { + $obj['carddata'] = $carddata; + } + return new Card($this->carddavBackend, $this->addressBookInfo, $obj); + } + $user = $this->userSession->getUser(); - $shareEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; - $shareEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; - $shareEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes'; + $shareEnumeration = $this->shareManager->allowEnumeration(); + $shareEnumerationGroup = $this->shareManager->limitEnumerationToGroups(); + $shareEnumerationPhone = $this->shareManager->limitEnumerationToPhone(); + $restrictToOwnGroups = $this->shareManager->shareWithGroupMembersOnly(); if (($user !== null && $user->getBackendClassName() === 'Guests') || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) { $ownName = $user !== null ? SyncService::getCardUri($user) : null; if ($ownName === $name) { @@ -179,40 +184,17 @@ public function getChild($name): Card { } throw new Forbidden(); } - if ($shareEnumerationGroup) { + if ($shareEnumerationGroup || $restrictToOwnGroups) { if ($user === null || $this->groupManager === null) { // Group manager is not available, so we can't determine which data is safe throw new Forbidden(); } - $groups = $this->groupManager->getUserGroups($user); - foreach ($groups as $group) { - foreach ($group->getUsers() as $groupUser) { - if ($groupUser->getBackendClassName() === 'Guests') { - continue; - } - $otherName = SyncService::getCardUri($groupUser); - if ($otherName === $name) { - return parent::getChild($name); - } - } + if ($this->isCardAccessible($user, $name)) { + return parent::getChild($name); } throw new Forbidden(); } - if (!$this->isFederation()) { - return parent::getChild($name); - } - - $obj = $this->carddavBackend->getCard($this->addressBookInfo['id'], $name); - if (!$obj) { - throw new NotFound('Card not found'); - } - $carddata = $this->extractCarddata($obj); - if (empty($carddata)) { - throw new Forbidden(); - } else { - $obj['carddata'] = $carddata; - } - return new Card($this->carddavBackend, $this->addressBookInfo, $obj); + return parent::getChild($name); } #[\Override] public function getChanges($syncToken, $syncLevel, $limit = null) { @@ -289,6 +271,91 @@ private function isFederation(): bool { return true; } + /** + * Groups excluded from the 'only share with group members' restriction are not considered. + * + * @return IGroup[] groups of $user whose members' cards $user can access + */ + private function getCardAccessGroups(IUser $user): array { + $excludedGroupIds = $this->shareManager->shareWithGroupMembersOnlyExcludeGroupsList(); + $groups = []; + foreach ($this->groupManager->getUserGroups($user) as $group) { + if (!in_array($group->getGID(), $excludedGroupIds, true)) { + $groups[] = $group; + } + } + return $groups; + } + + /** + * @return string[] card URIs of $user and of non-guest users sharing at least one group with $user + */ + private function getCardsForUsersGroups(IUser $user): array { + $names = [SyncService::getCardUri($user)]; + foreach ($this->getCardAccessGroups($user) as $group) { + foreach ($group->getUsers() as $groupUser) { + if ($groupUser->getBackendClassName() === 'Guests') { + continue; + } + $names[] = SyncService::getCardUri($groupUser); + } + } + return array_values(array_unique($names)); + } + + private function isCardAccessible(IUser $user, string $cardUri): bool { + if ($cardUri === SyncService::getCardUri($user)) { + return true; + } + $uid = $this->extractUserId($cardUri); + if ($uid === null) { + return false; + } + foreach ($this->getCardAccessGroups($user) as $group) { + if ($this->groupManager->isInGroup($uid, $group->getGID())) { + return true; + } + } + return false; + } + + /** + * Lists group members once, so the number of queries depends on neither the + * number of requested cards nor the size of the groups. + * + * @param string[] $cardUris + * @return string[] the card URIs $user can access + */ + private function filterAccessibleCardUris(IUser $user, array $cardUris): array { + $members = []; + foreach ($this->getCardAccessGroups($user) as $group) { + $members += $group->searchUsers(''); + } + $ownCardUri = SyncService::getCardUri($user); + $accessibleCardUris = []; + foreach ($cardUris as $cardUri) { + $uid = $this->extractUserId($cardUri); + if ($cardUri === $ownCardUri || ($uid !== null && isset($members[$uid]))) { + $accessibleCardUris[] = $cardUri; + } + } + return $accessibleCardUris; + } + + /** + * Reads the user id from a card URI as built by {@see SyncService::getCardUri()}. + * Only the URI is used, because loading the user would cost a lookup per card. + * + * @return string|null the user id, or null for guests and other URIs + */ + private function extractUserId(string $cardUri): ?string { + $separator = strpos($cardUri, ':'); + if ($separator === false || !str_ends_with($cardUri, '.vcf') || str_starts_with($cardUri, 'Guests:')) { + return null; + } + return substr($cardUri, $separator + 1, -strlen('.vcf')); + } + /** * If the validation doesn't work the card is "not found" so we * return empty carddata even if the carddata might exist in the local backend. diff --git a/apps/dav/lib/CardDAV/UserAddressBooks.php b/apps/dav/lib/CardDAV/UserAddressBooks.php index 00ee75b79920c..2ae2238131ef5 100644 --- a/apps/dav/lib/CardDAV/UserAddressBooks.php +++ b/apps/dav/lib/CardDAV/UserAddressBooks.php @@ -16,13 +16,13 @@ use OCA\DAV\ConfigLexicon; use OCA\Federation\TrustedServers; use OCP\IAppConfig; -use OCP\IConfig; use OCP\IGroupManager; use OCP\IL10N; use OCP\IRequest; use OCP\IUser; use OCP\IUserSession; use OCP\Server; +use OCP\Share\IManager as IShareManager; use OCP\Util; use Psr\Container\ContainerExceptionInterface; use Psr\Container\NotFoundExceptionInterface; @@ -34,7 +34,6 @@ class UserAddressBooks extends \Sabre\CardDAV\AddressBookHome { protected IL10N $l10n; - protected IConfig $config; protected IAppConfig $appConfig; public function __construct( @@ -47,7 +46,6 @@ public function __construct( parent::__construct($carddavBackend, $principalUri); $this->l10n = Util::getL10N('dav'); - $this->config = Server::get(IConfig::class); $this->appConfig = Server::get(IAppConfig::class); } @@ -91,7 +89,7 @@ public function getChildren() { $this->carddavBackend, $addressBook, $this->l10n, - $this->config, + Server::get(IShareManager::class), Server::get(IUserSession::class), $request, $trustedServers, diff --git a/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php b/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php index 1c163f4ea83b8..d469e753bdc28 100644 --- a/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php +++ b/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php @@ -10,17 +10,19 @@ namespace OCA\DAV\Tests\unit\CardDAV; use OC\AppFramework\Http\Request; +use OCA\DAV\CardDAV\CardDavBackend; use OCA\DAV\CardDAV\SyncService; use OCA\DAV\CardDAV\SystemAddressbook; use OCA\Federation\TrustedServers; use OCP\Accounts\IAccountManager; -use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\IL10N; use OCP\IRequest; use OCP\IUser; use OCP\IUserSession; +use OCP\Share\IManager as IShareManager; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use Sabre\CardDAV\Backend\BackendInterface; use Sabre\DAV\Exception\Forbidden; @@ -33,7 +35,7 @@ class SystemAddressBookTest extends TestCase { private BackendInterface&MockObject $cardDavBackend; private array $addressBookInfo; private IL10N&MockObject $l10n; - private IConfig&MockObject $config; + private IShareManager&MockObject $shareManager; private IUserSession&MockObject $userSession; private IRequest&MockObject $request; private array $server; @@ -51,14 +53,11 @@ protected function setUp(): void { 'principaluri' => 'principals/system/system', ]; $this->l10n = $this->createMock(IL10N::class); - $this->config = $this->createMock(IConfig::class); + $this->shareManager = $this->createMock(IShareManager::class); $this->userSession = $this->createMock(IUserSession::class); $this->request = $this->createMock(Request::class); - $this->server = [ - 'PHP_AUTH_USER' => 'system', - 'PHP_AUTH_PW' => 'shared123', - ]; - $this->request->method('__get')->with('server')->willReturn($this->server); + $this->server = []; + $this->request->method('__get')->with('server')->willReturnCallback(fn () => $this->server); $this->trustedServers = $this->createMock(TrustedServers::class); $this->groupManager = $this->createMock(IGroupManager::class); @@ -66,7 +65,7 @@ protected function setUp(): void { $this->cardDavBackend, $this->addressBookInfo, $this->l10n, - $this->config, + $this->shareManager, $this->userSession, $this->request, $this->trustedServers, @@ -74,14 +73,23 @@ protected function setUp(): void { ); } + private function mockFederatedRequest(): void { + $this->server = [ + 'PHP_AUTH_USER' => 'system', + 'PHP_AUTH_PW' => 'shared123', + ]; + } + + private function mockShareSettings(bool $enumeration, bool $group, bool $phone, bool $ownGroups, array $excludedGroups = []): void { + $this->shareManager->method('allowEnumeration')->willReturn($enumeration); + $this->shareManager->method('limitEnumerationToGroups')->willReturn($enumeration && $group); + $this->shareManager->method('limitEnumerationToPhone')->willReturn($enumeration && $phone); + $this->shareManager->method('shareWithGroupMembersOnly')->willReturn($ownGroups); + $this->shareManager->method('shareWithGroupMembersOnlyExcludeGroupsList')->willReturn($ownGroups ? $excludedGroups : []); + } + public function testGetChildrenAsGuest(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], - ]); + $this->mockShareSettings(true, false, false, false); $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user'); $user->method('getBackendClassName')->willReturn('Guests'); @@ -115,13 +123,9 @@ public function testGetChildrenAsGuest(): void { } public function testGetFilteredChildForFederation(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], - ]); + $this->mockFederatedRequest(); + $this->shareManager->expects(self::never()) + ->method('allowEnumeration'); $this->trustedServers->expects(self::once()) ->method('getServers') ->willReturn([ @@ -164,13 +168,9 @@ public function testGetFilteredChildForFederation(): void { } public function testGetChildNotFound(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], - ]); + $this->mockFederatedRequest(); + $this->shareManager->expects(self::never()) + ->method('allowEnumeration'); $this->trustedServers->expects(self::once()) ->method('getServers') ->willReturn([ @@ -183,27 +183,169 @@ public function testGetChildNotFound(): void { $this->addressBook->getChild('LDAP:user.vcf'); } - public function testGetChildWithoutEnumeration(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], + public static function federationRestrictionProvider(): array { + return [ + 'enumeration disabled' => [false, false, false, false], + 'enumeration limited to phone' => [true, false, true, false], + ]; + } + + public static function federationGroupRestrictionProvider(): array { + return [ + 'enumeration limited to groups' => [true, true, false, false], + 'enumeration limited to groups and phone' => [true, true, true, false], + 'only share with group members' => [true, false, false, true], + 'only share with group members, enumeration disabled' => [false, false, false, true], + ]; + } + + private function mockFederationWithRestrictions(bool $enumeration, bool $group, bool $phone, bool $ownGroups): void { + $this->mockFederatedRequest(); + $this->mockShareSettings($enumeration, $group, $phone, $ownGroups); + $this->trustedServers + ->method('getServers') + ->willReturn([ + [ + 'shared_secret' => 'shared123', + ], + ]); + $this->userSession + ->method('getUser') + ->willReturn(null); + } + + private function getFederatedCard(): array { + return [ + 'id' => 123, + 'carddata' => <<mockFederationWithRestrictions($enumeration, $group, $phone, $ownGroups); + $this->cardDavBackend->expects(self::once()) + ->method('getCard') + ->with(123, 'LDAP:user.vcf') + ->willReturn($this->getFederatedCard()); + + $card = $this->addressBook->getChild('LDAP:user.vcf'); + + self::assertStringContainsString('FN', $card->get()); + } + + #[DataProvider('federationRestrictionProvider')] + public function testGetMultipleChildrenForFederationBypassesRestrictions(bool $enumeration, bool $group, bool $phone, bool $ownGroups): void { + $this->mockFederationWithRestrictions($enumeration, $group, $phone, $ownGroups); + $this->cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with(123, ['LDAP:user.vcf']) + ->willReturn([$this->getFederatedCard()]); + + $cards = $this->addressBook->getMultipleChildren(['LDAP:user.vcf']); + + self::assertCount(1, $cards); + } + + #[DataProvider('federationRestrictionProvider')] + public function testGetChangesForFederationBypassesRestrictions(bool $enumeration, bool $group, bool $phone, bool $ownGroups): void { + $this->mockFederationWithRestrictions($enumeration, $group, $phone, $ownGroups); + $backend = $this->createMock(CardDavBackend::class); + $backend->method('getChangesForAddressBook') + ->willReturn([ + 'syncToken' => 2, + 'added' => ['LDAP:added.vcf'], + 'modified' => ['LDAP:modified.vcf'], + 'deleted' => ['LDAP:deleted.vcf'], ]); + $backend->method('getCard') + ->willReturn($this->getFederatedCard()); + $addressBook = new SystemAddressbook( + $backend, + $this->addressBookInfo, + $this->l10n, + $this->shareManager, + $this->userSession, + $this->request, + $this->trustedServers, + $this->groupManager, + ); + + $changes = $addressBook->getChanges(1, 1); + + self::assertSame(['LDAP:added.vcf'], $changes['added']); + self::assertSame(['LDAP:modified.vcf'], $changes['modified']); + self::assertSame(['LDAP:deleted.vcf'], $changes['deleted']); + } + + #[DataProvider('federationGroupRestrictionProvider')] + public function testGetChildForFederationForbiddenWithGroupRestrictions(bool $enumeration, bool $group, bool $phone, bool $ownGroups): void { + $this->mockFederationWithRestrictions($enumeration, $group, $phone, $ownGroups); + $this->cardDavBackend->expects(self::never()) + ->method('getCard'); $this->expectException(Forbidden::class); $this->addressBook->getChild('LDAP:user.vcf'); } - public function testGetChildAsGuest(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], + #[DataProvider('federationGroupRestrictionProvider')] + public function testGetMultipleChildrenForFederationEmptyWithGroupRestrictions(bool $enumeration, bool $group, bool $phone, bool $ownGroups): void { + $this->mockFederationWithRestrictions($enumeration, $group, $phone, $ownGroups); + $this->cardDavBackend->expects(self::never()) + ->method('getMultipleCards'); + + $cards = $this->addressBook->getMultipleChildren(['LDAP:user.vcf']); + + self::assertSame([], $cards); + } + + #[DataProvider('federationGroupRestrictionProvider')] + public function testGetChangesForFederationReportsDeletedWithGroupRestrictions(bool $enumeration, bool $group, bool $phone, bool $ownGroups): void { + $this->mockFederationWithRestrictions($enumeration, $group, $phone, $ownGroups); + $backend = $this->createMock(CardDavBackend::class); + $backend->method('getChangesForAddressBook') + ->willReturn([ + 'syncToken' => 2, + 'added' => ['LDAP:added.vcf'], + 'modified' => ['LDAP:modified.vcf'], + 'deleted' => ['LDAP:deleted.vcf'], ]); + $backend->expects(self::never()) + ->method('getCard'); + $addressBook = new SystemAddressbook( + $backend, + $this->addressBookInfo, + $this->l10n, + $this->shareManager, + $this->userSession, + $this->request, + $this->trustedServers, + $this->groupManager, + ); + + $changes = $addressBook->getChanges(1, 1); + + self::assertSame([], $changes['added']); + self::assertSame([], $changes['modified']); + self::assertSame(['LDAP:deleted.vcf', 'LDAP:added.vcf', 'LDAP:modified.vcf'], $changes['deleted']); + } + + public function testGetChildWithoutEnumeration(): void { + $this->mockShareSettings(false, false, false, false); + $this->expectException(Forbidden::class); + + $this->addressBook->getChild('LDAP:user.vcf'); + } + + public function testGetChildAsGuest(): void { + $this->mockShareSettings(true, false, false, false); $user = $this->createMock(IUser::class); $user->method('getBackendClassName')->willReturn('Guests'); $this->userSession->expects(self::once()) @@ -215,13 +357,7 @@ public function testGetChildAsGuest(): void { } public function testGetChildWithGroupEnumerationRestriction(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], - ]); + $this->mockShareSettings(true, true, false, false); $user = $this->createMock(IUser::class); $user->method('getBackendClassName')->willReturn('LDAP'); $this->userSession->expects(self::once()) @@ -231,9 +367,11 @@ public function testGetChildWithGroupEnumerationRestriction(): void { $user->method('getBackendClassName')->willReturn('LDAP'); $otherUser->method('getUID')->willReturn('other'); $group = $this->createMock(IGroup::class); - $group->expects(self::once()) - ->method('getUsers') - ->willReturn([$otherUser]); + $group->method('getGID')->willReturn('sales'); + $this->groupManager->expects(self::once()) + ->method('isInGroup') + ->with('other', 'sales') + ->willReturn(true); $this->groupManager->expects(self::once()) ->method('getUserGroups') ->with($user) @@ -257,14 +395,217 @@ public function testGetChildWithGroupEnumerationRestriction(): void { $this->addressBook->getChild("{$otherUser->getBackendClassName()}:{$otherUser->getUID()}.vcf"); } - public function testGetChildWithPhoneNumberEnumerationRestriction(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'yes'], + public function testGetChildWithOwnGroupsOnlyRestriction(): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->expects(self::once()) + ->method('getUser') + ->willReturn($user); + $otherUser = $this->createMock(IUser::class); + $otherUser->method('getUID')->willReturn('other'); + $otherUser->method('getBackendClassName')->willReturn('LDAP'); + $group = $this->createMock(IGroup::class); + $group->method('getGID')->willReturn('sales'); + $this->groupManager->expects(self::once()) + ->method('isInGroup') + ->with('other', 'sales') + ->willReturn(true); + $this->groupManager->expects(self::once()) + ->method('getUserGroups') + ->with($user) + ->willReturn([$group]); + $cardData = <<cardDavBackend->expects(self::once()) + ->method('getCard') + ->with($this->addressBookInfo['id'], "{$otherUser->getBackendClassName()}:{$otherUser->getUID()}.vcf") + ->willReturn([ + 'id' => 123, + 'carddata' => $cardData, ]); + + $this->addressBook->getChild("{$otherUser->getBackendClassName()}:{$otherUser->getUID()}.vcf"); + } + + public function testGetChildForbiddenWithOwnGroupsOnlyRestriction(): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->expects(self::once()) + ->method('getUser') + ->willReturn($user); + $this->groupManager->expects(self::once()) + ->method('getUserGroups') + ->with($user) + ->willReturn([]); + $this->expectException(Forbidden::class); + + $this->addressBook->getChild('LDAP:other.vcf'); + } + + public function testGetChildForbiddenThroughExcludedGroupWithOwnGroupsOnlyRestriction(): void { + $this->mockShareSettings(true, false, false, true, ['everyone']); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->expects(self::once()) + ->method('getUser') + ->willReturn($user); + $excludedGroup = $this->createMock(IGroup::class); + $excludedGroup->method('getGID')->willReturn('everyone'); + $this->groupManager->expects(self::never()) + ->method('isInGroup'); + $this->groupManager->expects(self::once()) + ->method('getUserGroups') + ->with($user) + ->willReturn([$excludedGroup]); + $this->cardDavBackend->expects(self::never()) + ->method('getCard'); + $this->expectException(Forbidden::class); + + $this->addressBook->getChild('LDAP:other.vcf'); + } + + public function testGetChildWithGroupEnumerationAndOwnGroupsOnlyRestrictionIgnoresExcludedGroup(): void { + $this->mockShareSettings(true, true, false, true, ['everyone']); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->expects(self::once()) + ->method('getUser') + ->willReturn($user); + $other = $this->createMock(IUser::class); + $other->method('getUID')->willReturn('other'); + $other->method('getBackendClassName')->willReturn('LDAP'); + $excludedGroup = $this->createMock(IGroup::class); + $excludedGroup->method('getGID')->willReturn('everyone'); + $this->groupManager->expects(self::never()) + ->method('isInGroup'); + $this->groupManager->expects(self::once()) + ->method('getUserGroups') + ->with($user) + ->willReturn([$excludedGroup]); + $this->cardDavBackend->expects(self::never()) + ->method('getCard'); + $this->expectException(Forbidden::class); + + $this->addressBook->getChild(SyncService::getCardUri($other)); + } + + public function testGetChildDoesNotCheckGroupsForOwnCard(): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->method('getUser')->willReturn($user); + $this->groupManager->expects(self::never())->method('isInGroup'); + $this->cardDavBackend->expects(self::once()) + ->method('getCard') + ->with(123, 'LDAP:user.vcf') + ->willReturn($this->getFederatedCard()); + + $this->addressBook->getChild('LDAP:user.vcf'); + } + + public static function inaccessibleCardUriProvider(): array { + return [ + 'guest' => ['Guests:other.vcf'], + 'no backend separator' => ['other.vcf'], + 'not a card' => ['LDAP:other.txt'], + ]; + } + + #[DataProvider('inaccessibleCardUriProvider')] + public function testGetChildForbiddenForInaccessibleCardUri(string $cardUri): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->method('getUser')->willReturn($user); + $this->groupManager->method('isInGroup')->willReturn(true); + $group = $this->createMock(IGroup::class); + $group->method('getGID')->willReturn('sales'); + $this->groupManager->method('getUserGroups')->willReturn([$group]); + $this->cardDavBackend->expects(self::never())->method('getCard'); + $this->expectException(Forbidden::class); + + $this->addressBook->getChild($cardUri); + } + + #[DataProvider('inaccessibleCardUriProvider')] + public function testGetMultipleChildrenOmitsInaccessibleCardUri(string $cardUri): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->method('getUser')->willReturn($user); + $group = $this->createMock(IGroup::class); + $group->method('searchUsers')->willReturn(['other' => $this->createMock(IUser::class)]); + $this->groupManager->method('getUserGroups')->willReturn([$group]); + $this->cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with(123, []) + ->willReturn([]); + + self::assertSame([], $this->addressBook->getMultipleChildren([$cardUri])); + } + + public static function memberCardUriProvider(): array { + return [ + 'numeric user id' => ['Database:123.vcf', '123'], + 'user id with separator' => ['Database:a:b.vcf', 'a:b'], + ]; + } + + #[DataProvider('memberCardUriProvider')] + public function testGetMultipleChildrenMatchesUserIdFromCardUri(string $cardUri, string $uid): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->method('getUser')->willReturn($user); + $group = $this->createMock(IGroup::class); + $group->method('searchUsers')->with('')->willReturn([$uid => $this->createMock(IUser::class)]); + $this->groupManager->method('getUserGroups')->willReturn([$group]); + $this->cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with(123, [$cardUri]) + ->willReturn([$this->getFederatedCard()]); + + self::assertCount(1, $this->addressBook->getMultipleChildren([$cardUri])); + } + + #[DataProvider('memberCardUriProvider')] + public function testGetChildMatchesUserIdFromCardUri(string $cardUri, string $uid): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->method('getUser')->willReturn($user); + $group = $this->createMock(IGroup::class); + $group->method('getGID')->willReturn('sales'); + $this->groupManager->method('getUserGroups')->willReturn([$group]); + $this->groupManager->expects(self::once()) + ->method('isInGroup') + ->with($uid, 'sales') + ->willReturn(true); + $this->cardDavBackend->expects(self::once()) + ->method('getCard') + ->with(123, $cardUri) + ->willReturn($this->getFederatedCard()); + + $this->addressBook->getChild($cardUri); + } + + public function testGetChildWithPhoneNumberEnumerationRestriction(): void { + $this->mockShareSettings(true, false, true, false); $user = $this->createMock(IUser::class); $user->method('getBackendClassName')->willReturn('LDAP'); $this->userSession->expects(self::once()) @@ -276,13 +617,7 @@ public function testGetChildWithPhoneNumberEnumerationRestriction(): void { } public function testGetOwnChildWithPhoneNumberEnumerationRestriction(): void { - $this->config->expects(self::exactly(3)) - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'yes'], - ]); + $this->mockShareSettings(true, false, true, false); $user = $this->createMock(IUser::class); $user->method('getBackendClassName')->willReturn('LDAP'); $user->method('getUID')->willReturn('user'); @@ -309,13 +644,7 @@ public function testGetOwnChildWithPhoneNumberEnumerationRestriction(): void { } public function testGetMultipleChildrenWithGroupEnumerationRestriction(): void { - $this->config - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], - ]); + $this->mockShareSettings(true, true, false, false); $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user'); $user->method('getBackendClassName')->willReturn('LDAP'); @@ -333,12 +662,14 @@ public function testGetMultipleChildrenWithGroupEnumerationRestriction(): void { ->willReturn($user); $group1 = $this->createMock(IGroup::class); $group1 - ->method('getUsers') - ->willReturn([$user, $other1]); + ->method('searchUsers') + ->with('') + ->willReturn(['user' => $user, 'other1' => $other1]); $group2 = $this->createMock(IGroup::class); $group2 - ->method('getUsers') - ->willReturn([$other1, $other2, $user]); + ->method('searchUsers') + ->with('') + ->willReturn(['other1' => $other1, 'other2' => $other2, 'user' => $user]); $this->groupManager ->method('getUserGroups') ->with($user) @@ -364,14 +695,154 @@ public function testGetMultipleChildrenWithGroupEnumerationRestriction(): void { self::assertCount(2, $cards); } - public function testGetMultipleChildrenAsGuest(): void { - $this->config - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], + public function testGetMultipleChildrenWithOwnGroupsOnlyRestriction(): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $other1 = $this->createMock(IUser::class); + $other1->method('getUID')->willReturn('other1'); + $other1->method('getBackendClassName')->willReturn('LDAP'); + $other2 = $this->createMock(IUser::class); + $other2->method('getUID')->willReturn('other2'); + $other2->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession + ->method('getUser') + ->willReturn($user); + $group1 = $this->createMock(IGroup::class); + $group1 + ->method('searchUsers') + ->with('') + ->willReturn(['user' => $user, 'other1' => $other1]); + $this->groupManager + ->method('getUserGroups') + ->with($user) + ->willReturn([$group1]); + $this->cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with($this->addressBookInfo['id'], [ + SyncService::getCardUri($user), + SyncService::getCardUri($other1), + ]) + ->willReturn([ + [], + [], + ]); + + $cards = $this->addressBook->getMultipleChildren([ + SyncService::getCardUri($user), + SyncService::getCardUri($other1), + SyncService::getCardUri($other2), // No overlapping group with this one + ]); + + self::assertCount(2, $cards); + } + + public function testGetMultipleChildrenWithOwnGroupsOnlyRestrictionIgnoresExcludedGroup(): void { + $this->mockShareSettings(true, false, false, true, ['everyone']); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $colleague = $this->createMock(IUser::class); + $colleague->method('getUID')->willReturn('colleague'); + $colleague->method('getBackendClassName')->willReturn('LDAP'); + $stranger = $this->createMock(IUser::class); + $stranger->method('getUID')->willReturn('stranger'); + $stranger->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession + ->method('getUser') + ->willReturn($user); + $excludedGroup = $this->createMock(IGroup::class); + $excludedGroup->method('getGID')->willReturn('everyone'); + $excludedGroup->expects(self::never())->method('searchUsers'); + $sharedGroup = $this->createMock(IGroup::class); + $sharedGroup->method('getGID')->willReturn('sales'); + $sharedGroup->method('searchUsers')->with('')->willReturn(['user' => $user, 'colleague' => $colleague]); + $this->groupManager + ->method('getUserGroups') + ->with($user) + ->willReturn([$excludedGroup, $sharedGroup]); + $this->cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with($this->addressBookInfo['id'], [ + SyncService::getCardUri($user), + SyncService::getCardUri($colleague), + ]) + ->willReturn([ + [], + [], ]); + + $cards = $this->addressBook->getMultipleChildren([ + SyncService::getCardUri($user), + SyncService::getCardUri($colleague), + SyncService::getCardUri($stranger), // Only shares the excluded group + ]); + + self::assertCount(2, $cards); + } + + public function testGetChildrenWithOwnGroupsOnlyRestrictionKeepsOwnCardWhenAllGroupsExcluded(): void { + $this->mockShareSettings(true, false, false, true, ['everyone']); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession + ->method('getUser') + ->willReturn($user); + $excludedGroup = $this->createMock(IGroup::class); + $excludedGroup->method('getGID')->willReturn('everyone'); + $this->groupManager + ->method('getUserGroups') + ->with($user) + ->willReturn([$excludedGroup]); + $this->cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with($this->addressBookInfo['id'], [SyncService::getCardUri($user)]) + ->willReturn([[]]); + + $cards = $this->addressBook->getChildren(); + + self::assertCount(1, $cards); + } + + public function testGetChildrenWithOwnGroupsOnlyRestriction(): void { + $this->mockShareSettings(true, false, false, true); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $user->method('getBackendClassName')->willReturn('LDAP'); + $other1 = $this->createMock(IUser::class); + $other1->method('getUID')->willReturn('other1'); + $other1->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession + ->method('getUser') + ->willReturn($user); + $group1 = $this->createMock(IGroup::class); + $group1 + ->method('getUsers') + ->willReturn([$user, $other1]); + $this->groupManager + ->method('getUserGroups') + ->with($user) + ->willReturn([$group1]); + $this->cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with($this->addressBookInfo['id'], [ + SyncService::getCardUri($user), + SyncService::getCardUri($other1), + ]) + ->willReturn([ + [], + [], + ]); + + $cards = $this->addressBook->getChildren(); + + self::assertCount(2, $cards); + } + + public function testGetMultipleChildrenAsGuest(): void { + $this->mockShareSettings(true, false, false, false); $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user'); $user->method('getBackendClassName')->willReturn('Guests'); @@ -385,13 +856,8 @@ public function testGetMultipleChildrenAsGuest(): void { } public function testGetMultipleChildren(): void { - $this->config - ->method('getAppValue') - ->willReturnMap([ - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'], - ['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'], - ['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'], - ]); + $this->mockFederatedRequest(); + $this->mockShareSettings(true, false, false, false); $this->trustedServers ->method('getServers') ->willReturn([ diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 7141a5694f457..0629444d210f0 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -612,19 +612,6 @@ UID]]> - - - - - - - - - - - - - principalUri]]>