Merge pull request #54426 from nextcloud/perf/prevent-fetching-account

perf: prevent fetching a principal's user account if the data is not needed

apps/dav/lib/CalDAV/CalDavBackend.php

@@ -3637,7 +3637,9 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 			$uri = $calendarInfo['principaluri'];
 		}
 
-		$principalInformation = $this->principalBackend->getPrincipalByPath($uri);
+		$principalInformation = $this->principalBackend->getPrincipalPropertiesByPath($uri, [
+			'{DAV:}displayname',
+		]);
 		if (isset($principalInformation['{DAV:}displayname'])) {
 			$calendarInfo[$displaynameKey] = $principalInformation['{DAV:}displayname'];
 		}

apps/dav/lib/CalDAV/CalendarRoot.php

@@ -9,11 +9,13 @@ namespace OCA\DAV\CalDAV;
 
 use OCA\DAV\CalDAV\Federation\FederatedCalendarFactory;
 use OCA\DAV\CalDAV\Federation\RemoteUserCalendarHome;
+use OCA\DAV\Connector\Sabre\Principal;
 use OCA\DAV\DAV\RemoteUserPrincipalBackend;
 use OCP\IConfig;
 use OCP\IL10N;
 use Psr\Log\LoggerInterface;
 use Sabre\CalDAV\Backend;
+use Sabre\DAV\Exception\NotFound;
 use Sabre\DAVACL\PrincipalBackend;
 
 class CalendarRoot extends \Sabre\CalDAV\CalendarRoot {
@@ -70,4 +72,25 @@ class CalendarRoot extends \Sabre\CalDAV\CalendarRoot {
 	public function enableReturnCachedSubscriptions(string $principalUri): void {
 		$this->returnCachedSubscriptions['principals/users/' . $principalUri] = true;
 	}
+
+	public function childExists($name) {
+		if (!($this->principalBackend instanceof Principal)) {
+			return parent::childExists($name);
+		}
+
+		// Fetch the most shallow version of the principal just to determine if it exists
+		$principalInfo = $this->principalBackend->getPrincipalPropertiesByPath(
+			$this->principalPrefix . '/' . $name,
+			[],
+		);
+		if ($principalInfo === null) {
+			return false;
+		}
+
+		try {
+			return $this->getChildForPrincipal($principalInfo) !== null;
+		} catch (NotFound $e) {
+			return false;
+		}
+	}
 }

apps/dav/lib/Connector/Sabre/Principal.php

@@ -101,6 +101,21 @@ class Principal implements BackendInterface {
 	 * @return array
 	 */
 	public function getPrincipalByPath($path) {
+		return $this->getPrincipalPropertiesByPath($path);
+	}
+
+	/**
+	 * Returns a specific principal, specified by its path.
+	 * The returned structure should be the exact same as from
+	 * getPrincipalsByPrefix.
+	 *
+	 * It is possible to optionally filter retrieved properties in case only a limited set is
+	 * required. Note that the implementation might return more properties than requested.
+	 *
+	 * @param string $path The path of the principal
+	 * @param string[]|null $propertyFilter A list of properties to be retrieved or all if null. An empty array will cause a very shallow principal to be retrieved.
+	 */
+	public function getPrincipalPropertiesByPath($path, ?array $propertyFilter = null): ?array {
 		[$prefix, $name] = \Sabre\Uri\split($path);
 		$decodedName = urldecode($name);
 
@@ -127,7 +142,7 @@ class Principal implements BackendInterface {
 			$user = $this->userManager->get($decodedName);
 
 			if ($user !== null) {
-				return $this->userToPrincipal($user);
+				return $this->userToPrincipal($user, $propertyFilter);
 			}
 		} elseif ($prefix === 'principals/circles') {
 			if ($this->userSession->getUser() !== null) {
@@ -466,29 +481,44 @@ class Principal implements BackendInterface {
 
 	/**
 	 * @param IUser $user
+	 * @param string[]|null $propertyFilter
 	 * @return array
 	 * @throws PropertyDoesNotExistException
 	 */
-	protected function userToPrincipal($user) {
+	protected function userToPrincipal($user, ?array $propertyFilter = null) {
+		$wantsProperty = static function (string $name) use ($propertyFilter) {
+			if ($propertyFilter === null) {
+				return true;
+			}
+
+			return in_array($name, $propertyFilter, true);
+		};
+
 		$userId = $user->getUID();
 		$displayName = $user->getDisplayName();
 		$principal = [
 			'uri' => $this->principalPrefix . '/' . $userId,
 			'{DAV:}displayname' => is_null($displayName) ? $userId : $displayName,
 			'{urn:ietf:params:xml:ns:caldav}calendar-user-type' => 'INDIVIDUAL',
-			'{http://nextcloud.com/ns}language' => $this->languageFactory->getUserLanguage($user),
 		];
 
-		$account = $this->accountManager->getAccount($user);
-		$alternativeEmails = array_map(fn (IAccountProperty $property) => 'mailto:' . $property->getValue(), $account->getPropertyCollection(IAccountManager::COLLECTION_EMAIL)->getProperties());
+		if ($wantsProperty('{http://nextcloud.com/ns}language')) {
+			$principal['{http://nextcloud.com/ns}language'] = $this->languageFactory->getUserLanguage($user);
+		}
 
-		$email = $user->getSystemEMailAddress();
-		if (!empty($email)) {
-			$principal['{http://sabredav.org/ns}email-address'] = $email;
+		if ($wantsProperty('{http://sabredav.org/ns}email-address')) {
+			$email = $user->getSystemEMailAddress();
+			if (!empty($email)) {
+				$principal['{http://sabredav.org/ns}email-address'] = $email;
+			}
 		}
 
-		if (!empty($alternativeEmails)) {
-			$principal['{DAV:}alternate-URI-set'] = $alternativeEmails;
+		if ($wantsProperty('{DAV:}alternate-URI-set')) {
+			$account = $this->accountManager->getAccount($user);
+			$alternativeEmails = array_map(static fn (IAccountProperty $property) => 'mailto:' . $property->getValue(), $account->getPropertyCollection(IAccountManager::COLLECTION_EMAIL)->getProperties());
+			if (!empty($alternativeEmails)) {
+				$principal['{DAV:}alternate-URI-set'] = $alternativeEmails;
+			}
 		}
 
 		return $principal;

apps/dav/lib/DAV/Sharing/Backend.php

@@ -140,7 +140,10 @@ abstract class Backend {
 		$rows = $this->service->getShares($resourceId);
 		$shares = [];
 		foreach ($rows as $row) {
-			$p = $this->getPrincipalByPath($row['principaluri']);
+			$p = $this->getPrincipalByPath($row['principaluri'], [
+				'uri',
+				'{DAV:}displayname',
+			]);
 			$shares[] = [
 				'href' => "principal:{$row['principaluri']}",
 				'commonName' => isset($p['{DAV:}displayname']) ? (string)$p['{DAV:}displayname'] : '',
@@ -166,7 +169,10 @@ abstract class Backend {
 		$sharesByResource = array_fill_keys($resourceIds, []);
 		foreach ($rows as $row) {
 			$resourceId = (int)$row['resourceid'];
-			$p = $this->getPrincipalByPath($row['principaluri']);
+			$p = $this->getPrincipalByPath($row['principaluri'], [
+				'uri',
+				'{DAV:}displayname',
+			]);
 			$sharesByResource[$resourceId][] = [
 				'href' => "principal:{$row['principaluri']}",
 				'commonName' => isset($p['{DAV:}displayname']) ? (string)$p['{DAV:}displayname'] : '',
@@ -275,12 +281,15 @@ abstract class Backend {
 		return $this->service->getSharesByPrincipals([$principal]);
 	}
 
-	private function getPrincipalByPath(string $principalUri): ?array {
+	/**
+	 * @param string[]|null $propertyFilter A list of properties to be retrieved or all if null. Is not guaranteed to always be applied and might overfetch.
+	 */
+	private function getPrincipalByPath(string $principalUri, ?array $propertyFilter = null): ?array {
 		// Hacky code below ... shouldn't we check the whole (principal) root collection instead?
 		if (str_starts_with($principalUri, RemoteUserPrincipalBackend::PRINCIPAL_PREFIX)) {
 			return $this->remoteUserPrincipalBackend->getPrincipalByPath($principalUri);
 		}
 
-		return $this->principalBackend->getPrincipalByPath($principalUri);
+		return $this->principalBackend->getPrincipalPropertiesByPath($principalUri, $propertyFilter);
 	}
 }

apps/dav/tests/unit/CalDAV/AbstractCalDavBackend.php

@@ -85,9 +85,9 @@ abstract class AbstractCalDavBackend extends TestCase {
 				$this->createMock(IConfig::class),
 				$this->createMock(IFactory::class)
 			])
-			->onlyMethods(['getPrincipalByPath', 'getGroupMembership', 'findByUri'])
+			->onlyMethods(['getPrincipalPropertiesByPath', 'getGroupMembership', 'findByUri'])
 			->getMock();
-		$this->principal->expects($this->any())->method('getPrincipalByPath')
+		$this->principal->expects($this->any())->method('getPrincipalPropertiesByPath')
 			->willReturn([
 				'uri' => 'principals/best-friend',
 				'{DAV:}displayname' => 'User\'s displayname',

apps/dav/tests/unit/DAV/Sharing/BackendTest.php

@@ -304,8 +304,8 @@ class BackendTest extends TestCase {
 			->with($resourceId)
 			->willReturn($rows);
 		$this->principalBackend->expects(self::once())
-			->method('getPrincipalByPath')
-			->with($principal)
+			->method('getPrincipalPropertiesByPath')
+			->with($principal, ['uri', '{DAV:}displayname'])
 			->willReturn(['uri' => $principal, '{DAV:}displayname' => 'bob']);
 		$this->shareCache->expects(self::once())
 			->method('set')
@@ -354,8 +354,8 @@ class BackendTest extends TestCase {
 			->with($resourceId)
 			->willReturn($rows);
 		$this->principalBackend->expects(self::once())
-			->method('getPrincipalByPath')
-			->with($principal)
+			->method('getPrincipalPropertiesByPath')
+			->with($principal, ['uri', '{DAV:}displayname'])
 			->willReturn(['uri' => $principal, '{DAV:}displayname' => 'bob']);
 		$this->shareCache->expects(self::once())
 			->method('set')
@@ -392,7 +392,7 @@ class BackendTest extends TestCase {
 			->with($resourceIds)
 			->willReturn($rows);
 		$this->principalBackend->expects(self::exactly(2))
-			->method('getPrincipalByPath')
+			->method('getPrincipalPropertiesByPath')
 			->willReturnCallback(function (string $principal) use ($principalResults) {
 				switch ($principal) {
 					case 'principals/groups/bob':

build/psalm-baseline.xml

@@ -740,8 +740,6 @@
       <code><![CDATA[$results]]></code>
     </InvalidScalarArgument>
     <NullableReturnStatement>
-      <code><![CDATA[$this->circleToPrincipal($decodedName)
-					?: $this->circleToPrincipal($name)]]></code>
       <code><![CDATA[null]]></code>
       <code><![CDATA[null]]></code>
       <code><![CDATA[null]]></code>
@@ -832,11 +830,6 @@
       <code><![CDATA[null]]></code>
     </NullableReturnStatement>
   </file>
-  <file src="apps/dav/lib/DAV/Sharing/Backend.php">
-    <LessSpecificReturnType>
-      <code><![CDATA[?array]]></code>
-    </LessSpecificReturnType>
-  </file>
   <file src="apps/dav/lib/DAV/Sharing/Plugin.php">
     <DeprecatedMethod>
       <code><![CDATA[getAppValue]]></code>