fix(GroupMigration): run job to remember old groups only once

Signed-off-by: Arthur Schiwon <[email protected]>
nextcloud · Jul 11, 2024 · 69e423a · 69e423a
1 parent 9fde08d
commit 69e423a
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 25 deletions.
diff --git a/lib/GroupManager.php b/lib/GroupManager.php
@@ -23,6 +23,7 @@
 
 class GroupManager {
 	public const LOCAL_GROUPS_CHECK_FOR_MIGRATION = 'localGroupsCheckForMigration';
+	public const STATE_MIGRATION_PHASE_EXPIRED = 'EXPIRED';
 
 	/**
 	 * @var IDBConnection $db
@@ -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)) {
@@ -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);
 		}
@@ -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 {

diff --git a/lib/Jobs/MigrateGroups.php b/lib/Jobs/MigrateGroups.php
@@ -45,12 +45,13 @@ class MigrateGroups extends QueuedJob {
 
 	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;
@@ -73,7 +74,7 @@ protected function run($argument) {
 
 	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);
@@ -162,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'];

diff --git a/lib/Migration/RememberLocalGroupsForPotentialMigrations.php b/lib/Migration/RememberLocalGroupsForPotentialMigrations.php
@@ -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);