fix: improve handling updated storages

Signed-off-by: Robin Appelman <robin@icewind.nl>

apps/files_external/composer/composer/autoload_classmap.php

@@ -40,6 +40,7 @@ return array(
     'OCA\\Files_External\\Event\\StorageCreatedEvent' => $baseDir . '/../lib/Event/StorageCreatedEvent.php',
     'OCA\\Files_External\\Event\\StorageDeletedEvent' => $baseDir . '/../lib/Event/StorageDeletedEvent.php',
     'OCA\\Files_External\\Event\\StorageUpdatedEvent' => $baseDir . '/../lib/Event/StorageUpdatedEvent.php',
+    'OCA\\Files_External\\Lib\\ApplicableHelper' => $baseDir . '/../lib/Lib/ApplicableHelper.php',
     'OCA\\Files_External\\Lib\\Auth\\AmazonS3\\AccessKey' => $baseDir . '/../lib/Lib/Auth/AmazonS3/AccessKey.php',
     'OCA\\Files_External\\Lib\\Auth\\AuthMechanism' => $baseDir . '/../lib/Lib/Auth/AuthMechanism.php',
     'OCA\\Files_External\\Lib\\Auth\\Builtin' => $baseDir . '/../lib/Lib/Auth/Builtin.php',

apps/files_external/composer/composer/autoload_static.php

@@ -55,6 +55,7 @@ class ComposerStaticInitFiles_External
         'OCA\\Files_External\\Event\\StorageCreatedEvent' => __DIR__ . '/..' . '/../lib/Event/StorageCreatedEvent.php',
         'OCA\\Files_External\\Event\\StorageDeletedEvent' => __DIR__ . '/..' . '/../lib/Event/StorageDeletedEvent.php',
         'OCA\\Files_External\\Event\\StorageUpdatedEvent' => __DIR__ . '/..' . '/../lib/Event/StorageUpdatedEvent.php',
+        'OCA\\Files_External\\Lib\\ApplicableHelper' => __DIR__ . '/..' . '/../lib/Lib/ApplicableHelper.php',
         'OCA\\Files_External\\Lib\\Auth\\AmazonS3\\AccessKey' => __DIR__ . '/..' . '/../lib/Lib/Auth/AmazonS3/AccessKey.php',
         'OCA\\Files_External\\Lib\\Auth\\AuthMechanism' => __DIR__ . '/..' . '/../lib/Lib/Auth/AuthMechanism.php',
         'OCA\\Files_External\\Lib\\Auth\\Builtin' => __DIR__ . '/..' . '/../lib/Lib/Auth/Builtin.php',

apps/files_external/lib/Lib/ApplicableHelper.php

@@ -0,0 +1,114 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Files_External\Lib;
+
+use OC\User\LazyUser;
+use OCP\IGroupManager;
+use OCP\IUser;
+use OCP\IUserManager;
+
+class ApplicableHelper {
+	public function __construct(
+		private readonly IUserManager $userManager,
+		private readonly IGroupManager $groupManager,
+	) {
+	}
+
+	/**
+	 * Get all users that have access to a storage
+	 *
+	 * @return \Iterator<string, IUser>
+	 */
+	public function getUsersForStorage(StorageConfig $storage): \Iterator {
+		$yielded = [];
+		if (count($storage->getApplicableUsers()) + count($storage->getApplicableGroups()) === 0) {
+			yield from $this->userManager->getSeenUsers();
+		}
+		foreach ($storage->getApplicableUsers() as $userId) {
+			$yielded[$userId] = true;
+			yield $userId => new LazyUser($userId, $this->userManager);
+		}
+		foreach ($storage->getApplicableGroups() as $groupId) {
+			$group = $this->groupManager->get($groupId);
+			if ($group !== null) {
+				foreach ($group->getUsers() as $user) {
+					if (!isset($yielded[$user->getUID()])) {
+						$yielded[$user->getUID()] = true;
+						yield $user->getUID() => $user;
+					}
+				}
+			}
+		}
+	}
+
+	public function isApplicableForUser(StorageConfig $storage, IUser $user): bool {
+		if (count($storage->getApplicableUsers()) + count($storage->getApplicableGroups()) === 0) {
+			return true;
+		}
+		if (in_array($user->getUID(), $storage->getApplicableUsers())) {
+			return true;
+		}
+		$groupIds = $this->groupManager->getUserGroupIds($user);
+		foreach ($groupIds as $groupId) {
+			if (in_array($groupId, $storage->getApplicableGroups())) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	/**
+	 * Return all users that are applicable for storage $a, but not for $b
+	 *
+	 * @return \Iterator<IUser>
+	 */
+	public function diffApplicable(StorageConfig $a, StorageConfig $b): \Iterator {
+		$aIsAll = count($a->getApplicableUsers()) + count($a->getApplicableGroups()) === 0;
+		$bIsAll = count($b->getApplicableUsers()) + count($b->getApplicableGroups()) === 0;
+		if ($bIsAll) {
+			return;
+		}
+
+		if ($aIsAll) {
+			foreach ($this->getUsersForStorage($a) as $user) {
+				if (!$this->isApplicableForUser($b, $user)) {
+					yield $user;
+				}
+			}
+		} else {
+			$yielded = [];
+			foreach ($a->getApplicableGroups() as $groupId) {
+				if (!in_array($groupId, $b->getApplicableGroups())) {
+					$group = $this->groupManager->get($groupId);
+					if ($group) {
+						foreach ($group->getUsers() as $user) {
+							if (!$this->isApplicableForUser($b, $user)) {
+								if (!isset($yielded[$user->getUID()])) {
+									$yielded[$user->getUID()] = true;
+									yield $user;
+								}
+							}
+						}
+					}
+				}
+			}
+			foreach ($a->getApplicableUsers() as $userId) {
+				if (!in_array($userId, $b->getApplicableUsers())) {
+					$user = $this->userManager->get($userId);
+					if ($user && !$this->isApplicableForUser($b, $user)) {
+						if (!isset($yielded[$user->getUID()])) {
+							$yielded[$user->getUID()] = true;
+							yield $user;
+						}
+					}
+				}
+			}
+		}
+	}
+}

apps/files_external/lib/Lib/StorageConfig.php

@@ -441,7 +441,7 @@ class StorageConfig implements \JsonSerializable {
 		return '/' . $user->getUID() . '/files/' . trim($this->mountPoint, '/') . '/';
 	}
 
-	public function __clone(): void {
+	public function __clone() {
 		$this->backend = clone $this->backend;
 		$this->authMechanism = clone $this->authMechanism;
 	}

apps/files_external/lib/Service/DBConfigService.php

@@ -16,7 +16,8 @@ use OCP\Security\ICrypto;
 /**
  * Stores the mount config in the database
  *
- * @psalm-type StorageConfigData = array{type: int, priority: int, applicable: list<array{type: mixed, value: mixed}>, config: array, options: array}
+ * @psalm-type ApplicableConfig = array{type: int, value: string}
+ * @psalm-type StorageConfigData = array{type: int, priority: int, applicable: list<ApplicableConfig>, config: array, options: array, ...<string, mixed>}
  */
 class DBConfigService {
 	public const MOUNT_TYPE_ADMIN = 1;
@@ -451,9 +452,9 @@ class DBConfigService {
 	 * @param string $table
 	 * @param string[] $fields
 	 * @param int[] $mountIds
-	 * @return array [$mountId => [['field1' => $value1, ...], ...], ...]
+	 * @return array<int, list<array>> [$mountId => [['field1' => $value1, ...], ...], ...]
 	 */
-	private function selectForMounts($table, array $fields, array $mountIds) {
+	private function selectForMounts(string $table, array $fields, array $mountIds): array {
 		if (count($mountIds) === 0) {
 			return [];
 		}
@@ -485,9 +486,9 @@ class DBConfigService {
 
 	/**
 	 * @param int[] $mountIds
-	 * @return array [$id => [['type' => $type, 'value' => $value], ...], ...]
+	 * @return array<int, list<ApplicableConfig>> [$id => [['type' => $type, 'value' => $value], ...], ...]
 	 */
-	public function getApplicableForMounts($mountIds) {
+	public function getApplicableForMounts(array $mountIds): array {
 		return $this->selectForMounts('external_applicable', ['type', 'value'], $mountIds);
 	}
 

apps/files_external/lib/Service/MountCacheService.php

@@ -10,11 +10,11 @@ namespace OCA\Files_External\Service;
 
 use OC\Files\Cache\CacheEntry;
 use OC\Files\Storage\FailedStorage;
-use OC\User\LazyUser;
 use OCA\Files_External\Config\ConfigAdapter;
 use OCA\Files_External\Event\StorageCreatedEvent;
 use OCA\Files_External\Event\StorageDeletedEvent;
 use OCA\Files_External\Event\StorageUpdatedEvent;
+use OCA\Files_External\Lib\ApplicableHelper;
 use OCA\Files_External\Lib\StorageConfig;
 use OCP\Cache\CappedMemoryCache;
 use OCP\EventDispatcher\Event;
@@ -25,9 +25,7 @@ use OCP\Group\Events\BeforeGroupDeletedEvent;
 use OCP\Group\Events\UserAddedEvent;
 use OCP\Group\Events\UserRemovedEvent;
 use OCP\IGroup;
-use OCP\IGroupManager;
 use OCP\IUser;
-use OCP\IUserManager;
 use OCP\User\Events\PostLoginEvent;
 use OCP\User\Events\UserCreatedEvent;
 
@@ -42,9 +40,8 @@ class MountCacheService implements IEventListener {
 	public function __construct(
 		private readonly IUserMountCache $userMountCache,
 		private readonly ConfigAdapter $configAdapter,
-		private readonly IUserManager $userManager,
-		private readonly IGroupManager $groupManager,
 		private readonly GlobalStoragesService $storagesService,
+		private readonly ApplicableHelper $applicableHelper,
 	) {
 		$this->storageRootCache = new CappedMemoryCache();
 	}
@@ -76,63 +73,24 @@ class MountCacheService implements IEventListener {
 		}
 	}
 
-
-	/**
-	 * Get all users that have access to a storage, either directly or through a group
-	 *
-	 * @param StorageConfig $storage
-	 * @return \Iterator<string, IUser>
-	 */
-	private function getUsersForStorage(StorageConfig $storage): \Iterator {
-		$yielded = [];
-		if (count($storage->getApplicableUsers()) + count($storage->getApplicableGroups()) === 0) {
-			yield from $this->userManager->getSeenUsers();
-		}
-		foreach ($storage->getApplicableUsers() as $userId) {
-			$yielded[$userId] = true;
-			yield $userId => new LazyUser($userId, $this->userManager);
-		}
-		foreach ($storage->getApplicableGroups() as $groupId) {
-			$group = $this->groupManager->get($groupId);
-			if ($group !== null) {
-				foreach ($group->searchUsers('') as $user) {
-					if (!isset($yielded[$user->getUID()])) {
-						$yielded[$user->getUID()] = true;
-						yield $user->getUID() => $user;
-					}
-				}
-			}
-		}
-	}
-
 	public function handleDeletedStorage(StorageConfig $storage): void {
-		foreach ($this->getUsersForStorage($storage) as $user) {
+		foreach ($this->applicableHelper->getUsersForStorage($storage) as $user) {
 			$this->userMountCache->removeMount($storage->getMountPointForUser($user));
 		}
 	}
 
 	public function handleAddedStorage(StorageConfig $storage): void {
-		foreach ($this->getUsersForStorage($storage) as $user) {
+		foreach ($this->applicableHelper->getUsersForStorage($storage) as $user) {
 			$this->registerForUser($user, $storage);
 		}
 	}
 
 	public function handleUpdatedStorage(StorageConfig $oldStorage, StorageConfig $newStorage): void {
-		/** @var array<string, IUser> $oldApplicable */
-		$oldApplicable = iterator_to_array($this->getUsersForStorage($oldStorage));
-		/** @var array<string, IUser> $newApplicable */
-		$newApplicable = iterator_to_array($this->getUsersForStorage($newStorage));
-
-		foreach ($oldApplicable as $oldUser) {
-			if (!isset($newApplicable[$oldUser->getUID()])) {
-				$this->userMountCache->removeMount($oldStorage->getMountPointForUser($oldUser));
-			}
+		foreach ($this->applicableHelper->diffApplicable($oldStorage, $newStorage) as $user) {
+			$this->userMountCache->removeMount($oldStorage->getMountPointForUser($user));
 		}
-
-		foreach ($newApplicable as $newUser) {
-			if (!isset($oldApplicable[$newUser->getUID()])) {
-				$this->registerForUser($newUser, $newStorage);
-			}
+		foreach ($this->applicableHelper->diffApplicable($newStorage, $oldStorage) as $user) {
+			$this->registerForUser($user, $newStorage);
 		}
 	}
 
@@ -194,26 +152,10 @@ class MountCacheService implements IEventListener {
 		);
 	}
 
-	private function isApplicableForUser(StorageConfig $storage, IUser $user): bool {
-		if (count($storage->getApplicableUsers()) + count($storage->getApplicableGroups()) === 0) {
-			return true;
-		}
-		if (in_array($user->getUID(), $storage->getApplicableUsers())) {
-			return true;
-		}
-		$groupIds = $this->groupManager->getUserGroupIds($user);
-		foreach ($groupIds as $groupId) {
-			if (in_array($groupId, $storage->getApplicableGroups())) {
-				return true;
-			}
-		}
-		return false;
-	}
-
 	private function handleUserRemoved(IGroup $group, IUser $user): void {
 		$storages = $this->storagesService->getAllStoragesForGroup($group);
 		foreach ($storages as $storage) {
-			if (!$this->isApplicableForUser($storage, $user)) {
+			if (!$this->applicableHelper->isApplicableForUser($storage, $user)) {
 				$this->userMountCache->removeMount($storage->getMountPointForUser($user));
 			}
 		}
@@ -229,10 +171,17 @@ class MountCacheService implements IEventListener {
 	private function handleGroupDeleted(IGroup $group): void {
 		$storages = $this->storagesService->getAllStoragesForGroup($group);
 		foreach ($storages as $storage) {
-			foreach ($group->searchUsers('') as $user) {
-				if (!$this->isApplicableForUser($storage, $user)) {
-					$this->userMountCache->removeMount($storage->getMountPointForUser($user));
-				}
+			$this->removeGroupFromStorage($storage, $group);
+		}
+	}
+
+	/**
+	 * Remove mounts from users in a group, if they don't have access to the storage trough other means
+	 */
+	private function removeGroupFromStorage(StorageConfig $storage, IGroup $group): void {
+		foreach ($group->searchUsers('') as $user) {
+			if (!$this->applicableHelper->isApplicableForUser($storage, $user)) {
+				$this->userMountCache->removeMount($storage->getMountPointForUser($user));
 			}
 		}
 	}

apps/files_external/tests/ApplicableHelperTest.php

@@ -0,0 +1,170 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Files_External\Tests;
+
+use OCA\Files_External\Lib\ApplicableHelper;
+use OCA\Files_External\Lib\StorageConfig;
+use OCP\IGroup;
+use OCP\IGroupManager;
+use OCP\IUser;
+use OCP\IUserManager;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\MockObject\MockObject;
+use Test\TestCase;
+
+class ApplicableHelperTest extends TestCase {
+	private IUserManager|MockObject $userManager;
+	private IGroupManager|MockObject $groupManager;
+
+	/** @var list<string> */
+	private array $users = [];
+	/** @var array<string, list<string>> */
+	private array $groups = [];
+
+	private ApplicableHelper $applicableHelper;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->userManager = $this->createMock(IUserManager::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
+
+		$this->userManager->method('get')
+			->willReturnCallback(function (string $id) {
+				$user = $this->createMock(IUser::class);
+				$user->method('getUID')->willReturn($id);
+				return $user;
+			});
+		$this->userManager->method('getSeenUsers')
+			->willReturnCallback(fn () => new \ArrayIterator(array_map($this->userManager->get(...), $this->users)));
+		$this->groupManager->method('get')
+			->willReturnCallback(function (string $id) {
+				$group = $this->createMock(IGroup::class);
+				$group->method('getGID')->willReturn($id);
+				$group->method('getUsers')
+					->willReturn(array_map($this->userManager->get(...), $this->groups[$id] ?: []));
+				return $group;
+			});
+		$this->groupManager->method('getUserGroupIds')
+			->willReturnCallback(function (IUser $user) {
+				$groups = [];
+				foreach ($this->groups as $group => $users) {
+					if (in_array($user->getUID(), $users)) {
+						$groups[] = $group;
+					}
+				}
+				return $groups;
+			});
+
+		$this->applicableHelper = new ApplicableHelper($this->userManager, $this->groupManager);
+
+		$this->users = ['user1', 'user2', 'user3', 'user4'];
+		$this->groups = [
+			'group1' => ['user1', 'user2'],
+			'group2' => ['user3'],
+		];
+	}
+
+	public static function usersForStorageProvider(): array {
+		return [
+			[[], [], ['user1', 'user2', 'user3', 'user4']],
+			[['user1'], [], ['user1']],
+			[['user1', 'user3'], [], ['user1', 'user3']],
+			[['user1'], ['group1'], ['user1', 'user2']],
+			[['user1'], ['group2'], ['user1', 'user3']],
+		];
+	}
+
+	#[DataProvider('usersForStorageProvider')]
+	public function testGetUsersForStorage(array $applicableUsers, array $applicableGroups, array $expected) {
+		$storage = $this->createMock(StorageConfig::class);
+		$storage->method('getApplicableUsers')
+			->willReturn($applicableUsers);
+		$storage->method('getApplicableGroups')
+			->willReturn($applicableGroups);
+
+		$result = iterator_to_array($this->applicableHelper->getUsersForStorage($storage));
+		$result = array_map(fn (IUser $user) => $user->getUID(), $result);
+		sort($result);
+		sort($expected);
+		$this->assertEquals($expected, $result);
+	}
+
+	public static function applicableProvider(): array {
+		return [
+			[[], [], 'user1', true],
+			[['user1'], [], 'user1', true],
+			[['user1'], [], 'user2', false],
+			[['user1', 'user3'], [], 'user1', true],
+			[['user1', 'user3'], [], 'user2', false],
+			[['user1'], ['group1'], 'user1', true],
+			[['user1'], ['group1'], 'user2', true],
+			[['user1'], ['group1'], 'user3', false],
+			[['user1'], ['group1'], 'user4', false],
+			[['user1'], ['group2'], 'user1', true],
+			[['user1'], ['group2'], 'user2', false],
+			[['user1'], ['group2'], 'user3', true],
+			[['user1'], ['group1'], 'user4', false],
+		];
+	}
+
+	#[DataProvider('applicableProvider')]
+	public function testIsApplicable(array $applicableUsers, array $applicableGroups, string $user, bool $expected) {
+		$storage = $this->createMock(StorageConfig::class);
+		$storage->method('getApplicableUsers')
+			->willReturn($applicableUsers);
+		$storage->method('getApplicableGroups')
+			->willReturn($applicableGroups);
+
+		$this->assertEquals($expected, $this->applicableHelper->isApplicableForUser($storage, $this->userManager->get($user)));
+	}
+
+	public static function diffProvider(): array {
+		return [
+			[[], [], [], [], []], // both all
+			[['user1'], [], [], [], []], // all added
+			[[], [], ['user1'], [], ['user2', 'user3', 'user4']], // all removed
+			[[], [], [], ['group1'], ['user3', 'user4']], // all removed
+			[[], [], ['user3'], ['group1'], ['user4']], // all removed
+			[['user1'], [], ['user1'], [], []],
+			[['user1'], [], ['user1', 'user2'], [], []],
+			[['user1'], [], ['user2'], [], ['user1']],
+			[['user1'], [], [], ['group1'], []],
+			[['user1'], [], [], ['group2'], ['user1']],
+			[[], ['group1'], [], ['group2'], ['user1', 'user2']],
+			[[], ['group1'], ['user1'], [], ['user2']],
+			[['user1'], ['group1'], ['user1'], [], ['user2']],
+			[['user1'], ['group1'], [], ['group1'], []],
+			[['user1'], ['group1'], [], ['group2'], ['user1', 'user2']],
+			[['user1'], ['group1'], ['user1'], ['group2'], ['user2']],
+		];
+	}
+
+	#[DataProvider('diffProvider')]
+	public function testDiff(array $applicableUsersA, array $applicableGroupsA, array $applicableUsersB, array $applicableGroupsB, array $expected) {
+		$storageA = $this->createMock(StorageConfig::class);
+		$storageA->method('getApplicableUsers')
+			->willReturn($applicableUsersA);
+		$storageA->method('getApplicableGroups')
+			->willReturn($applicableGroupsA);
+
+		$storageB = $this->createMock(StorageConfig::class);
+		$storageB->method('getApplicableUsers')
+			->willReturn($applicableUsersB);
+		$storageB->method('getApplicableGroups')
+			->willReturn($applicableGroupsB);
+
+		$result = iterator_to_array($this->applicableHelper->diffApplicable($storageA, $storageB));
+		$result = array_map(fn (IUser $user) => $user->getUID(), $result);
+		sort($result);
+		sort($expected);
+		$this->assertEquals($expected, $result);
+	}
+}