fix(Groups): drop groups with mixed users from transition list

this avoids testing one group all over again and again, which might be costly when large groups have to be tested (user facing request!). Signed-off-by: Arthur Schiwon <[email protected]> Co-authored-by: Christoph Wurst <[email protected]>
nextcloud · Sep 18, 2024 · dc10c9c · dc10c9c
1 parent 1e871e0
commit dc10c9c
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 26 deletions.
diff --git a/lib/GroupManager.php b/lib/GroupManager.php
@@ -70,6 +70,9 @@ public function __construct(
 	private function getGroupsToRemove(array $samlGroupNames, array $assignedGroups): array {
 		$groupsToRemove = [];
 		foreach ($assignedGroups as $group) {
+			if (in_array($group->getGID(), $samlGroupNames, true)) {
+				continue;
+			}
 			// if group is not supplied by SAML and group has SAML backend
 			if (!in_array($group->getGID(), $samlGroupNames) && $this->hasSamlBackend($group)) {
 				$groupsToRemove[] = $group->getGID();
@@ -286,11 +289,35 @@ protected function hasGroupForeignMembers(IGroup $group): bool {
 	 * allowed only for groups owned by the SAML backend.
 	 */
 	protected function mayModifyGroup(?IGroup $group): bool {
-		return
+		$isInTransition =
 			$group !== null
 			&& $group->getGID() !== 'admin'
 			&& in_array('Database', $group->getBackendNames())
-			&& $this->isGroupInTransitionList($group->getGID())
-			&& !$this->hasGroupForeignMembers($group);
+			&& $this->isGroupInTransitionList($group->getGID());
+
+		if ($isInTransition) {
+			$hasOnlySamlUsers = !$this->hasGroupForeignMembers($group);
+			if (!$hasOnlySamlUsers) {
+				$this->updateCandidatePool([$group->getGID()]);
+			}
+		}
+		return $isInTransition && $hasOnlySamlUsers;
+	}
+
+	public function updateCandidatePool(array $migratedGroups): void {
+		$candidateInfo = $this->config->getAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
+		if ($candidateInfo === '' || $candidateInfo === self::STATE_MIGRATION_PHASE_EXPIRED) {
+			return;
+		}
+		$candidateInfo = \json_decode($candidateInfo, true);
+		if (!isset($candidateInfo['dropAfter']) || !isset($candidateInfo['groups'])) {
+			return;
+		}
+		$candidateInfo['groups'] = array_diff($candidateInfo['groups'], $migratedGroups);
+		$this->config->setAppValue(
+			'user_saml',
+			self::LOCAL_GROUPS_CHECK_FOR_MIGRATION,
+			json_encode($candidateInfo)
+		);
 	}
 }
diff --git a/lib/Jobs/MigrateGroups.php b/lib/Jobs/MigrateGroups.php
@@ -66,29 +66,12 @@ protected function run($argument) {
 			$candidates = $this->getMigratableGroups();
 			$toMigrate = $this->getGroupsToMigrate($argument['gids'], $candidates);
 			$migrated = $this->migrateGroups($toMigrate);
-			$this->updateCandidatePool($migrated);
+			$this->ownGroupManager->updateCandidatePool($migrated);
 		} catch (\RuntimeException $e) {
 			return;
 		}
 	}
 
-	protected function updateCandidatePool(array $migratedGroups): void {
-		$candidateInfo = $this->config->getAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION, '');
-		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'], $migratedGroups);
-		$this->config->setAppValue(
-			'user_saml',
-			GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION,
-			json_encode($candidateInfo)
-		);
-	}
-
 	protected function migrateGroups(array $toMigrate): array {
 		return array_filter($toMigrate, function ($gid) {
 			return $this->migrateGroup($gid);

diff --git a/tests/integration/features/Shibboleth.feature b/tests/integration/features/Shibboleth.feature
@@ -420,6 +420,7 @@ Feature: Shibboleth
     Then the group backend of "Mixed Bag Of Sweets" should be "Database"
     And Then the group backend of "Mixed Bag Of Sweets" must not be "user_saml"
     And The user value "groups" should be "Mixed Bag Of Sweets,SAML_Astrophysics,SAML_Students"
+    And The setting "localGroupsCheckForMigration" is currently '{"dropAfter":9223372036854775807,"groups":["Astrophysics","Students"]}'
 
   Scenario: Not migrating a group with only SAML members that is not in the migration list
     Given The setting "type" is set to "saml"

diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php
@@ -101,12 +101,8 @@ public function after() {
 
 	/**
 	 * @Given The setting :settingName is set to :value
-	 *
-	 * @param string $settingName
-	 * @param string $value
 	 */
-	public function theSettingIsSetTo($settingName,
-		$value) {
+	public function theSettingIsSetTo(string $settingName, string $value): void {
 		if (in_array($settingName, [
 			'type',
 			'general-require_provisioned_account',
@@ -138,6 +134,44 @@ public function theSettingIsSetTo($settingName,
 		);
 	}
 
+	/**
+	 * @Then The setting :settingName is currently :expectedValue
+	 */
+	public function theSettingIsCurrently(string $settingName, string $expectedValue): void {
+		if (in_array($settingName, [
+			'type',
+			'general-require_provisioned_account',
+			'general-allow_multiple_user_back_ends',
+			'localGroupsCheckForMigration',
+		])) {
+			$this->changedSettings[] = $settingName;
+			$value = shell_exec(
+				sprintf(
+					'%s %s config:app:get user_saml %s',
+					PHP_BINARY,
+					__DIR__ . '/../../../../../../occ',
+					$settingName
+				)
+			);
+		} else {
+			$value = shell_exec(
+				sprintf(
+					'%s %s saml:config:set --"%s" %d',
+					PHP_BINARY,
+					__DIR__ . '/../../../../../../occ',
+					$settingName,
+					1
+				)
+			);
+		}
+		$value = rtrim($value); // remove trailing newline from shell output
+		if ($value !== $expectedValue) {
+			throw new UnexpectedValueException(
+				sprintf('Config value for %s is %s, but expected was %s', $settingName, $value, $expectedValue)
+			);
+		}
+	}
+
 	/**
 	 * @When I send a GET request to :url
 	 */