From 5cfbd4c53577ce1b713a3d50ef574e4ecd13fdb2 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Tue, 23 May 2023 17:51:44 +0200 Subject: [PATCH] fix(carddav): Check enumeration settings for all SAB methods Signed-off-by: Christoph Wurst --- apps/dav/lib/CardDAV/SystemAddressbook.php | 70 ++++- .../unit/CardDAV/SystemAddressBookTest.php | 242 +++++++++++++++++- 2 files changed, 309 insertions(+), 3 deletions(-) diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php index c936ad2344ff7..48ec533353a94 100644 --- a/apps/dav/lib/CardDAV/SystemAddressbook.php +++ b/apps/dav/lib/CardDAV/SystemAddressbook.php @@ -44,7 +44,9 @@ 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; class SystemAddressbook extends AddressBook { public const URI_SHARED = 'z-server-generated--system'; @@ -90,7 +92,7 @@ public function getChildren() { // Should never happen because we don't allow anonymous access return []; } - if (!$shareEnumeration || !$shareEnumerationGroup && $shareEnumerationPhone) { + if (!$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) { $name = SyncService::getCardUri($user); try { return [parent::getChild($name)]; @@ -130,6 +132,41 @@ public function getChildren() { * @throws NotFound */ 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'; + if (!$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) { + $user = $this->userSession->getUser(); + // No user or cards with no access + if ($user === null || !in_array(SyncService::getCardUri($user), $paths, true)) { + return []; + } + // Only return the own card + try { + return [parent::getChild(SyncService::getCardUri($user))]; + } catch (NotFound $e) { + return []; + } + } + if ($shareEnumerationGroup) { + $user = $this->userSession->getUser(); + 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); } @@ -159,6 +196,37 @@ public function getMultipleChildren($paths): array { * @throws Forbidden */ public function getChild($name): Card { + $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'; + if (!$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) { + $currentUser = $this->userSession->getUser(); + $ownName = $currentUser !== null ? SyncService::getCardUri($currentUser) : null; + if ($ownName === $name) { + return parent::getChild($name); + } + throw new Forbidden(); + } + if ($shareEnumerationGroup) { + $user = $this->userSession->getUser(); + 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); + } + } + } + throw new Forbidden(); + } if (!$this->isFederation()) { return parent::getChild($name); } diff --git a/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php b/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php index a753a1c5a7342..325b1120e8b27 100644 --- a/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php +++ b/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php @@ -26,21 +26,26 @@ namespace OCA\DAV\Tests\unit\CardDAV; use OC\AppFramework\Http\Request; +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 PHPUnit\Framework\MockObject\MockObject; use Sabre\CardDAV\Backend\BackendInterface; +use Sabre\DAV\Exception\Forbidden; +use Sabre\DAV\Exception\NotFound; use Sabre\VObject\Component\VCard; use Sabre\VObject\Reader; use Test\TestCase; class SystemAddressBookTest extends TestCase { - private MockObject|BackendInterface $cardDavBackend; private array $addressBookInfo; private IL10N|MockObject $l10n; @@ -49,6 +54,7 @@ class SystemAddressBookTest extends TestCase { private IRequest|MockObject $request; private array $server; private TrustedServers|MockObject $trustedServers; + private IGroupManager|MockObject $groupManager; private SystemAddressbook $addressBook; protected function setUp(): void { @@ -70,6 +76,7 @@ protected function setUp(): void { ]; $this->request->method('__get')->with('server')->willReturn($this->server); $this->trustedServers = $this->createMock(TrustedServers::class); + $this->groupManager = $this->createMock(IGroupManager::class); $this->addressBook = new SystemAddressbook( $this->cardDavBackend, @@ -79,11 +86,18 @@ protected function setUp(): void { $this->userSession, $this->request, $this->trustedServers, - null, + $this->groupManager, ); } 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->trustedServers->expects(self::once()) ->method('getServers') ->willReturn([ @@ -125,4 +139,228 @@ 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->trustedServers->expects(self::once()) + ->method('getServers') + ->willReturn([ + [ + 'shared_secret' => 'shared123', + ], + ]); + $this->expectException(NotFound::class); + + $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'], + ]); + $this->expectException(Forbidden::class); + + $this->addressBook->getChild("LDAP:user.vcf"); + } + + 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'], + ]); + $user = $this->createMock(IUser::class); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->expects(self::once()) + ->method('getUser') + ->willReturn($user); + $otherUser = $this->createMock(IUser::class); + $user->method('getBackendClassName')->willReturn('LDAP'); + $otherUser->method('getUID')->willReturn('other'); + $group = $this->createMock(IGroup::class); + $group->expects(self::once()) + ->method('getUsers') + ->willReturn([$otherUser]); + $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 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'], + ]); + $user = $this->createMock(IUser::class); + $user->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession->expects(self::once()) + ->method('getUser') + ->willReturn($user); + $this->expectException(Forbidden::class); + + $this->addressBook->getChild("LDAP:user.vcf"); + } + + 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'], + ]); + $user = $this->createMock(IUser::class); + $user->method('getBackendClassName')->willReturn('LDAP'); + $user->method('getUID')->willReturn('user'); + $this->userSession->expects(self::once()) + ->method('getUser') + ->willReturn($user); + $cardData = <<cardDavBackend->expects(self::once()) + ->method('getCard') + ->with($this->addressBookInfo['id'], 'LDAP:user.vcf') + ->willReturn([ + 'id' => 123, + 'carddata' => $cardData, + ]); + + $this->addressBook->getChild("LDAP:user.vcf"); + } + + 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'], + ]); + $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'); + $other3 = $this->createMock(IUser::class); + $other3->method('getUID')->willReturn('other3'); + $other3->method('getBackendClassName')->willReturn('LDAP'); + $this->userSession + ->method('getUser') + ->willReturn($user); + $group1 = $this->createMock(IGroup::class); + $group1 + ->method('getUsers') + ->willReturn([$user, $other1]); + $group2 = $this->createMock(IGroup::class); + $group2 + ->method('getUsers') + ->willReturn([$other1, $other2, $user]); + $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), // Omitted to test that it's not returned as stray + SyncService::getCardUri($other3), // No overlapping group with this one + ]); + + self::assertCount(2, $cards); + } + + 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->trustedServers + ->method('getServers') + ->willReturn([ + [ + 'shared_secret' => 'shared123', + ], + ]); + $cardData = <<cardDavBackend->expects(self::once()) + ->method('getMultipleCards') + ->with($this->addressBookInfo['id'], ['Database:user1.vcf', 'LDAP:user2.vcf']) + ->willReturn([ + [ + 'id' => 123, + 'carddata' => $cardData, + ], + [ + 'id' => 321, + 'carddata' => $cardData, + ], + ]); + + $cards = $this->addressBook->getMultipleChildren(['Database:user1.vcf', 'LDAP:user2.vcf']); + + self::assertCount(2, $cards); + } }