Merge pull request #38448 from nextcloud/fix/carddav/no-sab-guest-users

fix(carddav): Don't show system address book cards to guests
nextcloud · May 25, 2023 · cd5bd11 · cd5bd11
2 parents 9780472 + e524631
commit cd5bd11
Show file tree

Hide file tree

Showing 2 changed files with 84 additions and 8 deletions.
diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php
@@ -92,7 +92,7 @@ public function getChildren() {
 			// Should never happen because we don't allow anonymous access
 			return [];
 		}
-		if (!$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
+		if ($user->getBackendClassName() === 'Guests' || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
 			$name = SyncService::getCardUri($user);
 			try {
 				return [parent::getChild($name)];
@@ -135,8 +135,8 @@ 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();
+		$user = $this->userSession->getUser();
+		if (($user !== null && $user->getBackendClassName() === 'Guests') || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
 			// No user or cards with no access
 			if ($user === null || !in_array(SyncService::getCardUri($user), $paths, true)) {
 				return [];
@@ -149,7 +149,6 @@ public function getMultipleChildren($paths): array {
 			}
 		}
 		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 [];
@@ -196,19 +195,18 @@ public function getMultipleChildren($paths): array {
 	 * @throws Forbidden
 	 */
 	public function getChild($name): Card {
+		$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';
-		if (!$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
-			$currentUser = $this->userSession->getUser();
-			$ownName = $currentUser !== null ? SyncService::getCardUri($currentUser) : null;
+		if (($user !== null && $user->getBackendClassName() === 'Guests') || !$shareEnumeration || (!$shareEnumerationGroup && $shareEnumerationPhone)) {
+			$ownName = $user !== null ? SyncService::getCardUri($user) : 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();

diff --git a/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php b/apps/dav/tests/unit/CardDAV/SystemAddressBookTest.php
@@ -90,6 +90,46 @@ protected function setUp(): void {
 		);
 	}
 
+	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'],
+			]);
+		$user = $this->createMock(IUser::class);
+		$user->method('getUID')->willReturn('user');
+		$user->method('getBackendClassName')->willReturn('Guests');
+		$this->userSession->expects(self::once())
+			->method('getUser')
+			->willReturn($user);
+		$vcfWithScopes = <<<VCF
+BEGIN:VCARD
+VERSION:3.0
+PRODID:-//Sabre//Sabre VObject 4.4.2//EN
+UID:admin
+FN;X-NC-SCOPE=v2-federated:admin
+N;X-NC-SCOPE=v2-federated:admin;;;;
+ADR;TYPE=OTHER;X-NC-SCOPE=v2-local:Testing test test test;;;;;;
+EMAIL;TYPE=OTHER;X-NC-SCOPE=v2-federated:[email protected]
+TEL;TYPE=OTHER;X-NC-SCOPE=v2-local:+435454454544
+CLOUD:admin@http://localhost
+END:VCARD
+VCF;
+		$originalCard = [
+			'carddata' => $vcfWithScopes,
+		];
+		$this->cardDavBackend->expects(self::once())
+			->method('getCard')
+			->with(123, 'Guests:user.vcf')
+			->willReturn($originalCard);
+
+		$children = $this->addressBook->getChildren();
+
+		self::assertCount(1, $children);
+	}
+
 	public function testGetFilteredChildForFederation(): void {
 		$this->config->expects(self::exactly(3))
 			->method('getAppValue')
@@ -172,6 +212,24 @@ public function testGetChildWithoutEnumeration(): void {
 		$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'],
+			]);
+		$user = $this->createMock(IUser::class);
+		$user->method('getBackendClassName')->willReturn('Guests');
+		$this->userSession->expects(self::once())
+			->method('getUser')
+			->willReturn($user);
+		$this->expectException(Forbidden::class);
+
+		$this->addressBook->getChild("LDAP:user.vcf");
+	}
+
 	public function testGetChildWithGroupEnumerationRestriction(): void {
 		$this->config->expects(self::exactly(3))
 			->method('getAppValue')
@@ -322,6 +380,26 @@ 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'],
+			]);
+		$user = $this->createMock(IUser::class);
+		$user->method('getUID')->willReturn('user');
+		$user->method('getBackendClassName')->willReturn('Guests');
+		$this->userSession->expects(self::once())
+			->method('getUser')
+			->willReturn($user);
+
+		$cards = $this->addressBook->getMultipleChildren(['Database:user1.vcf', 'LDAP:user2.vcf']);
+
+		self::assertEmpty($cards);
+	}
+
 	public function testGetMultipleChildren(): void {
 		$this->config
 			->method('getAppValue')