Skip to content

Commit

Permalink
Merge pull request #863 from nextcloud/fix/862/migrate-members-also
Browse files Browse the repository at this point in the history
fix(Groups): take over members during migration
  • Loading branch information
blizzz committed Jul 18, 2024
2 parents 8ff3d56 + 244abb4 commit 12304d9
Show file tree
Hide file tree
Showing 11 changed files with 592 additions and 34 deletions.
2 changes: 2 additions & 0 deletions appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ While theoretically any other authentication provider implementing either one of
</post-migration>
<live-migration>
<step>OCA\User_SAML\Migration\CleanupRemovedConfig</step>
<step>OCA\User_SAML\Migration\TransferGroupMembers</step>
</live-migration>
</repair-steps>
<commands>
Expand All @@ -53,6 +54,7 @@ While theoretically any other authentication provider implementing either one of
<command>OCA\User_SAML\Command\ConfigGet</command>
<command>OCA\User_SAML\Command\ConfigSet</command>
<command>OCA\User_SAML\Command\GetMetadata</command>
<command>OCA\User_SAML\Command\GroupMigrationCopyIncomplete</command>
</commands>
<settings>
<admin>OCA\User_SAML\Settings\Admin</admin>
Expand Down
2 changes: 1 addition & 1 deletion lib/Command/ConfigDelete.php
Original file line number Diff line number Diff line change
Expand Up @@ -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('<error>Provider with id: ' . $providerId . ' does not exist.</error>');
$output->writeln('<error>Provider with id: ' . $pId . ' does not exist.</error>');
return 1;
}
return 0;
Expand Down
93 changes: 93 additions & 0 deletions lib/Command/GroupMigrationCopyIncomplete.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OCA\User_SAML\Command;

use OC\Core\Command\Base;
use OCA\User_SAML\Service\GroupMigration;
use Psr\Log\LoggerInterface;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Throwable;

class GroupMigrationCopyIncomplete extends Base {
public function __construct(
protected GroupMigration $groupMigration,
protected LoggerInterface $logger,
) {
parent::__construct();
}
protected function configure(): void {
$this->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('<info>No pending group member transfer</info>');
}
return 0;
}

if (!$this->doMemberTransfer($groupsToTreat, $output)) {
if (!$output->isQuiet()) {
$output->writeln('<comment>Not all group members could be transferred completely. Rerun this command or check the Nextcloud log.</comment>');
}
return 1;
}

if (!$output->isQuiet()) {
$output->writeln('<info>All group members could be transferred completely.</info>');
}
return 0;
}

/**
* @param string[]|array<empty> $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('<info>Members transferred successfully for group %s</info>', $gid));
}
}
} catch (Throwable $e) {
$errorOccurred = true;
if (!$output->isQuiet()) {
$output->writeln(sprintf('<error>Failed to transfer users from group %s: %s</error>', $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<empty> $groups */
$groups = $retry;
}
if (!empty($groups) && !$output->isQuiet()) {
$output->writeln(sprintf(
'<comment>Members not or incompletely transferred for groups: %s</comment>',
implode(', ', $groups)
));
}
return empty($groups) && !$errorOccurred;
}
}
36 changes: 21 additions & 15 deletions lib/GroupManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

class GroupManager {
public const LOCAL_GROUPS_CHECK_FOR_MIGRATION = 'localGroupsCheckForMigration';
public const STATE_MIGRATION_PHASE_EXPIRED = 'EXPIRED';

/**
* @var IDBConnection $db
Expand Down Expand Up @@ -68,8 +69,6 @@ public function __construct(
*/
private function getGroupsToRemove(array $samlGroupNames, array $assignedGroups): array {
$groupsToRemove = [];
// FIXME: Seems unused
$this->config->getAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
foreach ($assignedGroups as $group) {
// if group is not supplied by SAML and group has SAML backend
if (!in_array($group->getGID(), $samlGroupNames) && $this->hasSamlBackend($group)) {
Expand Down Expand Up @@ -201,7 +200,7 @@ protected function findGroup(string $gid): IGroup {
$strictBackendCheck = $migrationAllowListRaw === '';

$migrationAllowList = null;
if ($migrationAllowListRaw !== '') {
if ($migrationAllowListRaw !== '' || $migrationAllowListRaw !== self::STATE_MIGRATION_PHASE_EXPIRED) {
/** @var array{dropAfter: int, groups: string[]} $migrationAllowList */
$migrationAllowList = \json_decode($migrationAllowListRaw, true);
}
Expand Down Expand Up @@ -238,31 +237,38 @@ protected function hasSamlBackend(IGroup $group): bool {
}

protected function evaluateGroupMigrations(array $groups): void {
$candidateInfo = $this->config->getAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
if ($candidateInfo === '') {
return;
}
$candidateInfo = \json_decode($candidateInfo, true);
if (!isset($candidateInfo['dropAfter']) || $candidateInfo['dropAfter'] < time()) {
$this->config->deleteAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION);
$candidateInfo = $this->getCandidateInfoIfValid();
if ($candidateInfo === null) {
return;
}

$this->jobList->add(MigrateGroups::class, ['gids' => $groups]);
}

protected function isGroupInTransitionList(string $groupId): bool {
$candidateInfo = $this->config->getAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
if ($candidateInfo === '') {
$candidateInfo = $this->getCandidateInfoIfValid();
if (!$candidateInfo) {
return false;
}
return in_array($groupId, $candidateInfo['groups'], true);
}

/**
* @return array{dropAfter: int, groups: string[]}|null
*/
public function getCandidateInfoIfValid(): ?array {
$candidateInfo = $this->config->getAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
if ($candidateInfo === '' || $candidateInfo === self::STATE_MIGRATION_PHASE_EXPIRED) {
return null;
}
/** @var array{dropAfter: int, groups: string[]} $candidateInfo */
$candidateInfo = \json_decode($candidateInfo, true);
if (!isset($candidateInfo['dropAfter']) || $candidateInfo['dropAfter'] < time()) {
$this->config->deleteAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION);
return false;
$this->config->setAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, self::STATE_MIGRATION_PHASE_EXPIRED);
return null;
}

return in_array($groupId, $candidateInfo['groups']);
return $candidateInfo;
}

protected function hasGroupForeignMembers(IGroup $group): bool {
Expand Down
52 changes: 35 additions & 17 deletions lib/Jobs/MigrateGroups.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,16 @@

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;
use OCP\DB\Exception;
use OCP\IConfig;
use OCP\IDBConnection;
use OCP\IGroupManager;
use Psr\Log\LoggerInterface;
use Throwable;

/**
* Class MigrateGroups
Expand All @@ -24,6 +28,9 @@
* @todo: remove this, when dropping Nextcloud 29 support
*/
class MigrateGroups extends QueuedJob {
use TTransactional;

protected const BATCH_SIZE = 1000;

/** @var IConfig */
private $config;
Expand All @@ -37,12 +44,14 @@ class MigrateGroups extends QueuedJob {
private $logger;

public function __construct(
protected GroupMigration $groupMigration,
protected GroupManager $ownGroupManager,
IConfig $config,
IGroupManager $groupManager,
IDBConnection $dbc,
GroupBackend $ownGroupBackend,
LoggerInterface $logger,
ITimeFactory $timeFactory
ITimeFactory $timeFactory,
) {
parent::__construct($timeFactory);
$this->config = $config;
Expand All @@ -63,16 +72,16 @@ protected function run($argument) {
}
}

protected function updateCandidatePool($migrateGroups) {
protected function updateCandidatePool(array $migratedGroups): void {
$candidateInfo = $this->config->getAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
if ($candidateInfo === null || $candidateInfo === '') {
if ($candidateInfo === '' || $candidateInfo === GroupManager::STATE_MIGRATION_PHASE_EXPIRED) {
return;
}
$candidateInfo = \json_decode($candidateInfo, true);
if (!isset($candidateInfo['dropAfter']) || !isset($candidateInfo['groups'])) {
return;
}
$candidateInfo['groups'] = array_diff($candidateInfo['groups'], $migrateGroups);
$candidateInfo['groups'] = array_diff($candidateInfo['groups'], $migratedGroups);
$this->config->setAppValue(
'user_saml',
GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION,
Expand All @@ -87,7 +96,11 @@ protected function migrateGroups(array $toMigrate): array {
}

protected function migrateGroup(string $gid): bool {
$isMigrated = false;
$allUsersInserted = false;
try {
$allUsersInserted = $this->groupMigration->migrateGroupUsers($gid);

$this->dbc->beginTransaction();

$qb = $this->dbc->getQueryBuilder();
Expand All @@ -97,20 +110,30 @@ protected function migrateGroup(string $gid): bool {
if ($affected === 0) {
throw new \RuntimeException('Could not delete group from local backend');
}

if (!$this->ownGroupBackend->createGroup($gid)) {
throw new \RuntimeException('Could not create group in SAML backend');
}

$this->dbc->commit();

return true;
} catch (\Exception $e) {
$isMigrated = true;
} catch (Throwable $e) {
$this->dbc->rollBack();
$this->logger->warning($e->getMessage(), ['app' => 'user_saml', 'exception' => $e]);
}

return false;
if ($allUsersInserted && $isMigrated) {
try {
$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',
'gid' => $gid,
'exception' => $e,
]);
}
}

return $isMigrated;
}

protected function getGroupsToMigrate(array $samlGroups, array $pool): array {
Expand Down Expand Up @@ -140,14 +163,9 @@ protected function getGroupsToMigrate(array $samlGroups, array $pool): array {
}

protected function getMigratableGroups(): array {
$candidateInfo = $this->config->getAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
if ($candidateInfo === null || $candidateInfo === '') {
throw new \RuntimeException('No migration of groups to SAML backend anymore');
}
$candidateInfo = \json_decode($candidateInfo, true);
if (!isset($candidateInfo['dropAfter']) || !isset($candidateInfo['groups']) || $candidateInfo['dropAfter'] < time()) {
$this->config->deleteAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION);
throw new \RuntimeException('Period for migration groups is over');
$candidateInfo = $this->ownGroupManager->getCandidateInfoIfValid();
if ($candidateInfo === null) {
throw new \RuntimeException('No migration tasks of groups to SAML backend');
}

return $candidateInfo['groups'];
Expand Down
5 changes: 5 additions & 0 deletions lib/Migration/RememberLocalGroupsForPotentialMigrations.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ public function getName(): string {
* @since 9.1.0
*/
public function run(IOutput $output) {
$candidateInfo = $this->config->getAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
if ($candidateInfo !== '') {
return;
}

try {
$backend = $this->findBackend();
$groupIds = $this->findGroupIds($backend);
Expand Down
52 changes: 52 additions & 0 deletions lib/Migration/TransferGroupMembers.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OCA\User_SAML\Migration;

use OCA\User_SAML\Service\GroupMigration;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;
use Psr\Log\LoggerInterface;
use Throwable;

class TransferGroupMembers implements IRepairStep {

public function __construct(
private GroupMigration $groupMigration,
private LoggerInterface $logger,
) {
}

public function getName(): string {
return 'Move potential left members from old local groups to SAML groups';
}

public function run(IOutput $output): void {
$groupsToTreat = $this->groupMigration->findGroupsWithLocalMembers();
if (empty($groupsToTreat)) {
return;
}
$hasError = false;
$output->startProgress(count($groupsToTreat));
foreach ($groupsToTreat as $gid) {
try {
if ($this->groupMigration->migrateGroupUsers($gid)) {
$this->groupMigration->cleanUpOldGroupUsers($gid);
}
} catch (Throwable $e) {
$hasError = true;
$this->logger->warning('Error while transferring group members of {gid}', ['gid' => $gid, 'exception' => $e]);
} finally {
$output->advance();
}
}
$output->finishProgress();
if ($hasError) {
$output->warning('There were errors while transferring group members to SAML groups. You may try later `occ saml:group-migration:copy-incomplete-members` later and check your nextcloud.log.');
}
}
}
Loading

0 comments on commit 12304d9

Please sign in to comment.