From 272d6141cae1c3a507e042a51fa1bd2d8524ee26 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 8 Jan 2026 00:00:01 +0100 Subject: [PATCH] fix: improve handling updated storages Signed-off-by: Robin Appelman --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../lib/Lib/ApplicableHelper.php | 114 ++++++++++++ apps/files_external/lib/Lib/StorageConfig.php | 2 +- .../lib/Service/DBConfigService.php | 11 +- .../lib/Service/MountCacheService.php | 91 +++------- .../tests/ApplicableHelperTest.php | 170 ++++++++++++++++++ 7 files changed, 313 insertions(+), 77 deletions(-) create mode 100644 apps/files_external/lib/Lib/ApplicableHelper.php create mode 100644 apps/files_external/tests/ApplicableHelperTest.php diff --git a/apps/files_external/composer/composer/autoload_classmap.php b/apps/files_external/composer/composer/autoload_classmap.php index 8526237f829..da5e7329e0f 100644 --- a/apps/files_external/composer/composer/autoload_classmap.php +++ b/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', diff --git a/apps/files_external/composer/composer/autoload_static.php b/apps/files_external/composer/composer/autoload_static.php index b5b99235640..adba91ec553 100644 --- a/apps/files_external/composer/composer/autoload_static.php +++ b/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', diff --git a/apps/files_external/lib/Lib/ApplicableHelper.php b/apps/files_external/lib/Lib/ApplicableHelper.php new file mode 100644 index 00000000000..1c603ec2c64 --- /dev/null +++ b/apps/files_external/lib/Lib/ApplicableHelper.php @@ -0,0 +1,114 @@ + + */ + 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 + */ + 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; + } + } + } + } + } + } +} diff --git a/apps/files_external/lib/Lib/StorageConfig.php b/apps/files_external/lib/Lib/StorageConfig.php index 3f8a318b280..133c5392ec7 100644 --- a/apps/files_external/lib/Lib/StorageConfig.php +++ b/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; } diff --git a/apps/files_external/lib/Service/DBConfigService.php b/apps/files_external/lib/Service/DBConfigService.php index 207bb1b79b2..e9d4cbc89e7 100644 --- a/apps/files_external/lib/Service/DBConfigService.php +++ b/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, config: array, options: array} + * @psalm-type ApplicableConfig = array{type: int, value: string} + * @psalm-type StorageConfigData = array{type: int, priority: int, applicable: list, config: array, options: array, ...} */ 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> [$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> [$id => [['type' => $type, 'value' => $value], ...], ...] */ - public function getApplicableForMounts($mountIds) { + public function getApplicableForMounts(array $mountIds): array { return $this->selectForMounts('external_applicable', ['type', 'value'], $mountIds); } diff --git a/apps/files_external/lib/Service/MountCacheService.php b/apps/files_external/lib/Service/MountCacheService.php index 9b4b594efe8..2be17b9d598 100644 --- a/apps/files_external/lib/Service/MountCacheService.php +++ b/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 - */ - 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 $oldApplicable */ - $oldApplicable = iterator_to_array($this->getUsersForStorage($oldStorage)); - /** @var array $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)); } } } diff --git a/apps/files_external/tests/ApplicableHelperTest.php b/apps/files_external/tests/ApplicableHelperTest.php new file mode 100644 index 00000000000..3085cf71d42 --- /dev/null +++ b/apps/files_external/tests/ApplicableHelperTest.php @@ -0,0 +1,170 @@ + */ + private array $users = []; + /** @var array> */ + 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); + } +}