nextcloud · solracsf · Mar 31, 2026
@@ -80,6 +80,22 @@ class ShareAPIController extends OCSController {
 	private ?Node $lockedNode = null;
 	private array $trustedServerCache = [];
 
+	/**
+	 * Per-request user object cache to avoid repeated backend lookups when
+	 * formatting multiple shares for the same owner, sharer, or recipient.
+	 * Keyed by UID. Stores null to avoid re-querying a UID that doesn't exist.
+	 *
+	 * @var array<string, \OCP\IUser|null>
+	 */
+	private array $userObjectCache = [];
+
+	/**
+	 * Per-request group object cache for the same reason as $userObjectCache.
+	 *
+	 * @var array<string, \OCP\IGroup|null>
+	 */
+	private array $groupObjectCache = [];
+
 	/**
 	 * Share20OCS constructor.
 	 */
@@ -110,19 +126,47 @@ public function __construct(
 		parent::__construct($appName, $request);
 	}
 
+	/**
+	 * Fetches a user object, using the request-scoped cache to avoid repeated
+	 * backend calls for the same UID within a single API response.
+	 * Uses array_key_exists() so that UIDs resolving to null are cached and
+	 * not re-queried (e.g. shares from deleted users).
+	 *
+	 * @since 34.0.0
+	 * @internal
+	 */
+	private function getCachedUser(string $uid): ?\OCP\IUser {
+		if (!array_key_exists($uid, $this->userObjectCache)) {
+			$this->userObjectCache[$uid] = $this->userManager->get($uid);
+		}
+		return $this->userObjectCache[$uid];
+	}
+
+	/**
+	 * Fetches a group object, using the request-scoped cache to avoid repeated
+	 * backend calls for the same GID within a single API response.
+	 *
+	 * @since 34.0.0
+	 * @internal
+	 */
+	private function getCachedGroup(string $gid): ?\OCP\IGroup {
+		if (!array_key_exists($gid, $this->groupObjectCache)) {
+			$this->groupObjectCache[$gid] = $this->groupManager->get($gid);
+		}
+		return $this->groupObjectCache[$gid];
+	}
+
 	/**
 	 * Convert an IShare to an array for OCS output
 	 *
-	 * @param IShare $share
-	 * @param Node|null $recipientNode
 	 * @return Files_SharingShare
 	 * @throws NotFoundException In case the node can't be resolved.
 	 *
 	 * @suppress PhanUndeclaredClassMethod
 	 */
 	protected function formatShare(IShare $share, ?Node $recipientNode = null): array {
-		$sharedBy = $this->userManager->get($share->getSharedBy());
-		$shareOwner = $this->userManager->get($share->getShareOwner());
+		$sharedBy = $this->getCachedUser($share->getSharedBy());
+		$shareOwner = $this->getCachedUser($share->getShareOwner());
 
 		$isOwnShare = false;
 		if ($shareOwner !== null) {
@@ -240,7 +284,7 @@ protected function formatShare(IShare $share, ?Node $recipientNode = null): arra
 		}
 
 		if ($share->getShareType() === IShare::TYPE_USER) {
-			$sharedWith = $this->userManager->get($share->getSharedWith());
+			$sharedWith = $this->getCachedUser($share->getSharedWith());
 			$result['share_with'] = $share->getSharedWith();
 			$result['share_with_displayname'] = $sharedWith !== null ? $sharedWith->getDisplayName() : $share->getSharedWith();
 			$result['share_with_displayname_unique'] = $sharedWith !== null ? (
@@ -260,7 +304,7 @@ protected function formatShare(IShare $share, ?Node $recipientNode = null): arra
 				];
 			}
 		} elseif ($share->getShareType() === IShare::TYPE_GROUP) {
-			$group = $this->groupManager->get($share->getSharedWith());
+			$group = $this->getCachedGroup($share->getSharedWith());
 			$result['share_with'] = $share->getSharedWith();
 			$result['share_with_displayname'] = $group !== null ? $group->getDisplayName() : $share->getSharedWith();
 		} elseif ($share->getShareType() === IShare::TYPE_LINK) {

@@ -6,7 +6,6 @@
  */
 namespace OCA\Settings\Controller;
 
-use OC\Settings\AuthorizedGroup;
 use OCA\Settings\Service\AuthorizedGroupService;
 use OCA\Settings\Service\NotFoundException;
 use OCP\AppFramework\Controller;
@@ -26,37 +25,13 @@ public function __construct(
 	/**
 	 * @throws NotFoundException
 	 * @throws Exception
+	 * @throws \Throwable
 	 */
 	public function saveSettings(array $newGroups, string $class): DataResponse {
-		$currentGroups = $this->authorizedGroupService->findExistingGroupsForClass($class);
-
-		foreach ($currentGroups as $group) {
-			/** @var AuthorizedGroup $group */
-			$removed = true;
-			foreach ($newGroups as $groupData) {
-				if ($groupData['gid'] === $group->getGroupId()) {
-					$removed = false;
-					break;
-				}
-			}
-			if ($removed) {
-				$this->authorizedGroupService->delete($group->getId());
-			}
-		}
-
-		foreach ($newGroups as $groupData) {
-			$added = true;
-			foreach ($currentGroups as $group) {
-				/** @var AuthorizedGroup $group */
-				if ($groupData['gid'] === $group->getGroupId()) {
-					$added = false;
-					break;
-				}
-			}
-			if ($added) {
-				$this->authorizedGroupService->create($groupData['gid'], $class);
-			}
-		}
+		// Delegate the diff-and-apply logic to the service so that the cache
+		// is flushed exactly once after all mutations, regardless of how many
+		// groups were added or removed.
+		$this->authorizedGroupService->saveSettings($newGroups, $class);
 
 		return new DataResponse(['valid' => true]);
 	}

@@ -55,7 +55,74 @@ private function handleException(Throwable $e): void {
 	}
 
 	/**
-	 * Create a new AuthorizedGroup
+	 * Applies a bulk delegation update for a setting class in a single
+	 * transaction-like sequence, then invalidates the cache exactly once.
+	 *
+	 * @param list<array{gid: string}> $newGroups Desired final state
+	 * @throws Exception
+	 * @throws Throwable
+	 */
+	public function saveSettings(array $newGroups, string $class): void {
+		$currentGroups = $this->findExistingGroupsForClass($class);
+
+		foreach ($currentGroups as $group) {
+			$removed = true;
+			foreach ($newGroups as $groupData) {
+				if ($groupData['gid'] === $group->getGroupId()) {
+					$removed = false;
+					break;
+				}
+			}
+
+			if ($removed) {
+				try {
+					// $group is already a hydrated AuthorizedGroup entity
+					// returned by findExistingGroupsForClass()
+					$this->mapper->delete($group);
+				} catch (\Exception $exception) {
+					$this->handleException($exception);
+				}
+			}
+		}
+
+		// We attempt the insert unconditionally and treat a unique-
+		// constraint violation as idempotent — cheaper than a read-before-write
+		// and race-safe under concurrent saveSettings() calls.
+		foreach ($newGroups as $groupData) {
+			$added = true;
+			foreach ($currentGroups as $group) {
+				if ($groupData['gid'] === $group->getGroupId()) {
+					$added = false;
+					break;
+				}
+			}
+
+			if ($added) {
+				try {
+					$newGroup = new AuthorizedGroup();
+					$newGroup->setGroupId($groupData['gid']);
+					$newGroup->setClass($class);
+					$this->mapper->insert($newGroup);
+				} catch (Exception $e) {
+					// The DB unique constraint prevented the duplicate
+					// so treat as idempotent success.
+					if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+						throw $e;
+					}
+				}
+			}
+		}
+
+		$this->mapper->clearCache();
+	}
+
+	/**
+	 * Create a new AuthorizedGroup and invalidate the distributed cache.
+	 *
+	 * Adding a delegation may grant access to every current member of the
+	 * affected group. A full cache clear is used rather than per-user
+	 * invalidation to avoid an extra group-membership backend call at write
+	 * time.
 	 *
 	 * @throws Exception
 	 * @throws ConflictException
@@ -73,7 +140,14 @@ public function create(string $groupId, string $class): AuthorizedGroup {
 		$authorizedGroup = new AuthorizedGroup();
 		$authorizedGroup->setGroupId($groupId);
 		$authorizedGroup->setClass($class);
-		return $this->mapper->insert($authorizedGroup);
+
+		$result = $this->mapper->insert($authorizedGroup);
+
+		// Invalidate after successful insert so the next request re-evaluates
+		// all users' authorized classes.
+		$this->mapper->clearCache();
+
+		return $result;
 	}
 
 	/**
@@ -84,6 +158,8 @@ public function delete(int $id): void {
 		try {
 			$authorizedGroup = $this->mapper->find($id);
 			$this->mapper->delete($authorizedGroup);
+			// Revoking a delegation must take effect immediately.
+			$this->mapper->clearCache();
 		} catch (\Exception $exception) {
 			$this->handleException($exception);
 		}
@@ -107,6 +183,9 @@ public function findExistingGroupsForClass(string $class): array {
 	public function removeAuthorizationAssociatedTo(IGroup $group): void {
 		try {
 			$this->mapper->removeGroup($group->getGID());
+			// Group deletion removes all delegations for that GID
+			// all affected users need re-evaluation on their next request
+			$this->mapper->clearCache();
 		} catch (\Exception $exception) {
 			$this->handleException($exception);
 		}

@@ -6,7 +6,6 @@
  */
 namespace OCA\Settings\Tests\Controller\Admin;
 
-use OC\Settings\AuthorizedGroup;
 use OCA\Settings\Controller\AuthorizedGroupController;
 use OCA\Settings\Service\AuthorizedGroupService;
 use OCP\IRequest;
@@ -28,26 +27,14 @@ protected function setUp(): void {
 	}
 
 	public function testSaveSettings(): void {
-		$setting = 'MySecretSetting';
-		$oldGroups = [];
-		$oldGroups[] = AuthorizedGroup::fromParams(['groupId' => 'hello', 'class' => $setting]);
-		$goodbye = AuthorizedGroup::fromParams(['groupId' => 'goodbye', 'class' => $setting, 'id' => 42]);
-		$oldGroups[] = $goodbye;
-		$this->service->expects($this->once())
-			->method('findExistingGroupsForClass')
-			->with('MySecretSetting')
-			->willReturn($oldGroups);
-
-		$this->service->expects($this->once())
-			->method('delete')
-			->with(42);
+		$newGroups = [['gid' => 'hello'], ['gid' => 'world']];
 
+		// The controller delegates the entire diff-and-apply to the service.
 		$this->service->expects($this->once())
-			->method('create')
-			->with('world', 'MySecretSetting')
-			->willReturn(AuthorizedGroup::fromParams(['groupId' => 'world', 'class' => $setting]));
+			->method('saveSettings')
+			->with($newGroups, 'MySecretSetting');
 
-		$result = $this->controller->saveSettings([['gid' => 'hello'], ['gid' => 'world']], 'MySecretSetting');
+		$result = $this->controller->saveSettings($newGroups, 'MySecretSetting');
 
 		$this->assertEquals(['valid' => true], $result->getData());
 	}