From 8f6eb989f7fdb2723d3688bd4f758af7d4b3e2ba Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 10 Jul 2024 10:33:00 +0200 Subject: [PATCH] enh(GroupMigration): add command to run member transfer Signed-off-by: Arthur Schiwon --- appinfo/info.xml | 1 + lib/Command/ConfigDelete.php | 2 +- lib/Command/GroupMigrationCopyIncomplete.php | 93 +++++++++++++++++ lib/Jobs/MigrateGroups.php | 51 +--------- lib/Service/GroupMigration.php | 101 +++++++++++++++++++ tests/stub.phpstub | 41 ++++++++ 6 files changed, 241 insertions(+), 48 deletions(-) create mode 100644 lib/Command/GroupMigrationCopyIncomplete.php create mode 100644 lib/Service/GroupMigration.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 9df4fb24..88c8155f 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -53,6 +53,7 @@ While theoretically any other authentication provider implementing either one of OCA\User_SAML\Command\ConfigGet OCA\User_SAML\Command\ConfigSet OCA\User_SAML\Command\GetMetadata + OCA\User_SAML\Command\GroupMigrationCopyIncomplete OCA\User_SAML\Settings\Admin diff --git a/lib/Command/ConfigDelete.php b/lib/Command/ConfigDelete.php index 0d5e04d3..eba270cc 100644 --- a/lib/Command/ConfigDelete.php +++ b/lib/Command/ConfigDelete.php @@ -44,7 +44,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->samlSettings->delete($pId); $output->writeln('Provider deleted.'); } catch (Exception $e) { - $output->writeln('Provider with id: ' . $providerId . ' does not exist.'); + $output->writeln('Provider with id: ' . $pId . ' does not exist.'); return 1; } return 0; diff --git a/lib/Command/GroupMigrationCopyIncomplete.php b/lib/Command/GroupMigrationCopyIncomplete.php new file mode 100644 index 00000000..7d746245 --- /dev/null +++ b/lib/Command/GroupMigrationCopyIncomplete.php @@ -0,0 +1,93 @@ +setName('saml:group-migration:copy-incomplete-members'); + $this->setDescription('Transfers remaining group members from old local to current SAML groups'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $groupsToTreat = $this->groupMigration->findGroupsWithLocalMembers(); + if (empty($groupsToTreat)) { + if ($output->isVerbose()) { + $output->writeln('No pending group member transfer'); + } + return 0; + } + + if (!$this->doMemberTransfer($groupsToTreat, $output)) { + if (!$output->isQuiet()) { + $output->writeln('Not all group members could be transferred completely. Rerun this command or check the Nextcloud log.'); + } + return 1; + } + + if (!$output->isQuiet()) { + $output->writeln('All group members could be transferred completely.'); + } + return 0; + } + + /** + * @param string[]|array $groups + * @param OutputInterface $output + * @return bool + */ + protected function doMemberTransfer(array $groups, OutputInterface $output): bool { + $errorOccurred = false; + for ($i = 0; $i < 2; $i++) { + $retry = []; + foreach ($groups as $gid) { + try { + $isComplete = $this->groupMigration->migrateGroupUsers($gid); + if (!$isComplete) { + $retry[] = $gid; + } else { + $this->groupMigration->cleanUpOldGroupUsers($gid); + if ($output->isVerbose()) { + $output->writeln(sprintf('Members transferred successfully for group %s', $gid)); + } + } + } catch (Throwable $e) { + $errorOccurred = true; + if (!$output->isQuiet()) { + $output->writeln(sprintf('Failed to transfer users from group %s: %s', $gid, $e->getMessage())); + } + $this->logger->warning('Error while transferring group members of {gid}', ['gid' => $gid, 'exception' => $e]); + } + } + if (empty($retry)) { + return true; + } + /** @var string[]|array $groups */ + $groups = $retry; + } + if (!empty($groups) && !$output->isQuiet()) { + $output->writeln(sprintf( + 'Members not or incompletely transferred for groups: %s', + implode(', ', $groups) + )); + } + return empty($groups) && !$errorOccurred; + } +} diff --git a/lib/Jobs/MigrateGroups.php b/lib/Jobs/MigrateGroups.php index 875d38b5..f573d95e 100644 --- a/lib/Jobs/MigrateGroups.php +++ b/lib/Jobs/MigrateGroups.php @@ -10,6 +10,7 @@ use OCA\User_SAML\GroupBackend; use OCA\User_SAML\GroupManager; +use OCA\User_SAML\Service\GroupMigration; use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\QueuedJob; @@ -17,7 +18,6 @@ use OCP\IConfig; use OCP\IDBConnection; use OCP\IGroupManager; -use OCP\IUser; use Psr\Log\LoggerInterface; use Throwable; @@ -44,6 +44,7 @@ class MigrateGroups extends QueuedJob { private $logger; public function __construct( + protected GroupMigration $groupMigration, IConfig $config, IGroupManager $groupManager, IDBConnection $dbc, @@ -97,7 +98,7 @@ protected function migrateGroup(string $gid): bool { $isMigrated = false; $allUsersInserted = false; try { - $allUsersInserted = $this->migrateGroupUsers($gid); + $allUsersInserted = $this->groupMigration->migrateGroupUsers($gid); $this->dbc->beginTransaction(); @@ -121,7 +122,7 @@ protected function migrateGroup(string $gid): bool { if ($allUsersInserted && $isMigrated) { try { - $this->cleanUpOldGroupUsers($gid); + $this->groupMigration->cleanUpOldGroupUsers($gid); } catch (Exception $e) { $this->logger->warning('Error while cleaning up group members in (oc_)group_user of group (gid) {gid}', [ 'app' => 'user_saml', @@ -134,50 +135,6 @@ protected function migrateGroup(string $gid): bool { return $isMigrated; } - /** - * @throws Exception - */ - protected function cleanUpOldGroupUsers(string $gid): void { - $cleanup = $this->dbc->getQueryBuilder(); - $cleanup->delete('group_user') - ->where($cleanup->expr()->eq('gid', $cleanup->createNamedParameter($gid))); - $cleanup->executeStatement(); - } - - /** - * @returns bool true when all users were migrated, when they were only partly migrated - * @throws Exception - * @throws Throwable - */ - protected function migrateGroupUsers(string $gid): bool { - $originalGroup = $this->groupManager->get($gid); - $members = $originalGroup?->getUsers(); - - $areAllInserted = true; - foreach (array_chunk($members ?? [], self::BATCH_SIZE) as $userBatch) { - $areAllInserted = ($this->atomic(function () use ($userBatch, $gid) { - /** @var IUser $user */ - foreach ($userBatch as $user) { - $this->dbc->insertIgnoreConflict( - GroupBackend::TABLE_MEMBERS, - [ - 'gid' => $gid, - 'uid' => $user->getUID(), - ] - ); - } - return true; - }, $this->dbc) === true) && $areAllInserted; - } - if (!$areAllInserted) { - $this->logger->warning('Partial migration of users from local group {gid} to SAML.', [ - 'app' => 'user_saml', - 'gid' => $gid, - ]); - } - return $areAllInserted; - } - protected function getGroupsToMigrate(array $samlGroups, array $pool): array { return array_filter($samlGroups, function (string $gid) use ($pool) { if (!in_array($gid, $pool)) { diff --git a/lib/Service/GroupMigration.php b/lib/Service/GroupMigration.php new file mode 100644 index 00000000..ce3b7a26 --- /dev/null +++ b/lib/Service/GroupMigration.php @@ -0,0 +1,101 @@ +dbc->getQueryBuilder(); + $qb->selectDistinct('gid') + ->from('group_user') + ->where($qb->expr()->in('gid', $qb->createParameter('gidList'))); + + $allOwnedGroups = $this->ownGroupBackend->getGroups(); + foreach (array_chunk($allOwnedGroups, self::CHUNK_SIZE) as $groupsChunk) { + $qb->setParameter('gidList', $groupsChunk, IQueryBuilder::PARAM_STR_ARRAY); + $result = $qb->executeQuery(); + while ($gid = $result->fetchOne()) { + $foundGroups[] = $gid; + } + $result->closeCursor(); + } + + return $foundGroups; + } + + /** + * @returns bool true when all users were migrated, when they were only partly migrated + * @throws Exception + * @throws Throwable + */ + public function migrateGroupUsers(string $gid): bool { + $originalGroup = $this->groupManager->get($gid); + $members = $originalGroup?->getUsers(); + + $areAllInserted = true; + foreach (array_chunk($members ?? [], (int)floor(self::CHUNK_SIZE / 2)) as $userBatch) { + $areAllInserted = ($this->atomic(function () use ($userBatch, $gid) { + /** @var IUser $user */ + foreach ($userBatch as $user) { + $this->dbc->insertIgnoreConflict( + GroupBackend::TABLE_MEMBERS, + [ + 'gid' => $gid, + 'uid' => $user->getUID(), + ] + ); + } + return true; + }, $this->dbc) === true) && $areAllInserted; + } + if (!$areAllInserted) { + $this->logger->warning('Partial migration of users from local group {gid} to SAML.', [ + 'app' => 'user_saml', + 'gid' => $gid, + ]); + } + return $areAllInserted; + } + + /** + * @throws Exception + */ + public function cleanUpOldGroupUsers(string $gid): void { + $cleanup = $this->dbc->getQueryBuilder(); + $cleanup->delete('group_user') + ->where($cleanup->expr()->eq('gid', $cleanup->createNamedParameter($gid))); + $cleanup->executeStatement(); + } + +} diff --git a/tests/stub.phpstub b/tests/stub.phpstub index c2345130..352cb702 100644 --- a/tests/stub.phpstub +++ b/tests/stub.phpstub @@ -9,6 +9,47 @@ namespace OCA\Files\Event { } } +namespace OC\Core\Command { + use Symfony\Component\Console\Command\Command; + use Symfony\Component\Console\Input\InputInterface; + use Symfony\Component\Console\Output\OutputInterface; + + class Base { + public function __construct() {} + protected function configure(): void {} + public function run(InputInterface $input, OutputInterface $output): int {} + public function setName(string $name): self {} + public function setDescription(string $description): self {} + public function addOption(string $name, $shortcut = null, ?int $mode = null, string $description = '', $default = null): self; + public function addArgument(string $name, int $mode = null, string $description = '', $default = null): self; + protected function writeArrayInOutputFormat(InputInterface $input, OutputInterface $output, array $items, string $prefix = ' - '): void; + } +} + +namespace Symfony\Component\Console\Output { + class OutputInterface { + public function isVerbose(): bool; + public function isQuiet(): bool; + public function writeln($messages, int $options = 0): void; + } +} + +namespace Symfony\Component\Console\Input { + class InputInterface { + public function getArgument(string $name): mixed; + public function getOptions(): array; + public function getOption(string $name): mixed; + } + + class InputArgument { + public const REQUIRED = 1; + } + + class InputOption { + public const VALUE_REQUIRED = 2; + } +} + namespace OC\Group { abstract class Database extends \OCP\Group\Backend\ABackend implements \OCP\Group\Backend\IAddToGroupBackend,