Merge pull request #38423 from nextcloud/fix/carddav/check-system-address-book-enumeration

fix(carddav): Check enumeration settings for all SAB methods

2 files changed, 309 insertions, 3 deletions

diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php
index c936ad2344f..48ec533353a 100644
--- a/apps/dav/lib/CardDAV/SystemAddressbook.php
+++ b/apps/dav/lib/CardDAV/SystemAddressbook.php
@@ -44,7 +44,9 @@ use Sabre\DAV\Exception\NotFound;
 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 @@ class SystemAddressbook extends AddressBook {
 			// 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 @@ class SystemAddressbook extends AddressBook {
 	 * @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 @@ class SystemAddressbook extends AddressBook {
 	 * @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 a753a1c5a73..325b1120e8b 100644
--- a/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php
+++ b/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php
@@ -26,21 +26,26 @@ declare(strict_types=1);
 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 @@ class SystemAddressBookTest extends TestCase {
 		];
 		$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 @@ class SystemAddressBookTest extends TestCase {
 			$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 @@ VCF;
 		}
 	}
 
+	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 = <<<VCF
+BEGIN:VCARD
+VERSION:3.0
+PRODID:-//Sabre//Sabre VObject 4.4.2//EN
+UID:admin
+FN;X-NC-SCOPE=v2-federated:other
+END:VCARD
+VCF;
+		$this->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 = <<<VCF
+BEGIN:VCARD
+VERSION:3.0
+PRODID:-//Sabre//Sabre VObject 4.4.2//EN
+UID:admin
+FN;X-NC-SCOPE=v2-federated:user
+END:VCARD
+VCF;
+		$this->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 = <<<VCF
+BEGIN:VCARD
+VERSION:3.0
+PRODID:-//Sabre//Sabre VObject 4.4.2//EN
+UID:admin
+FN;X-NC-SCOPE=v2-federated:user
+END:VCARD
+VCF;
+		$this->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);
+	}
 }