allow local group modifications in some cases

- must not be about admin group - must be about local group backend - must be in allow migration list - must not have users from mixed backends Signed-off-by: Arthur Schiwon <[email protected]>
nextcloud · Dec 7, 2022 · 0d72089 · 0d72089
1 parent 50509bc
commit 0d72089
Showing 1 changed file with 69 additions and 16 deletions.
diff --git a/lib/GroupManager.php b/lib/GroupManager.php
@@ -86,40 +86,51 @@ public function __construct(
 		$this->settings = $settings;
 	}
 
-	private function getGroupsToRemove(array $samlGroups, array $assignedGroups): array {
+	/**
+	 * @param string[] $samlGroupNames
+	 * @param IGroup[] $assignedGroups
+	 * @return string[]
+	 */
+	private function getGroupsToRemove(array $samlGroupNames, array $assignedGroups): array {
 		$groupsToRemove = [];
+		$this->config->getAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, null);
 		foreach ($assignedGroups as $group) {
 			// if group is not supplied by SAML and group has SAML backend
-			if (!in_array($group->getGID(), $samlGroups) && $this->hasSamlBackend($group)) {
+			if (!in_array($group->getGID(), $samlGroupNames) && $this->hasSamlBackend($group)) {
+				$groupsToRemove[] = $group->getGID();
+			} else if ($this->mayModifyGroup($group)) {
 				$groupsToRemove[] = $group->getGID();
 			}
 		}
 		return $groupsToRemove;
 	}
 
-	private function getGroupsToAdd(array $samlGroups, array $assignedGroupIds): array {
+	private function getGroupsToAdd(array $samlGroupNames, array $assignedGroupIds): array {
 		$groupsToAdd = [];
-		foreach ($samlGroups as $group) {
-			// if user is not assigend to the group or the provided group has a non SAML backend
-			if (!in_array($group, $assignedGroupIds) || !$this->hasSamlBackend($this->groupManager->get($group))) {
-				$groupsToAdd[] = $group;
+		foreach ($samlGroupNames as $groupName) {
+			$group = $this->groupManager->get($groupName);
+			// if user is not assigned to the group or the provided group has a non SAML backend
+			if (!in_array($groupName, $assignedGroupIds) || !$this->hasSamlBackend($group)) {
+				$groupsToAdd[] = $groupName;
+			} else if ($this->mayModifyGroup($group)) {
+				$groupsToAdd[] = $groupName->getGID();
 			}
 		}
 		return $groupsToAdd;
 	}
 
-	public function replaceGroups(string $uid, array $samlGroups): void {
+	public function replaceGroups(string $uid, array $samlGroupNames): void {
 		$user = $this->userManager->get($uid);
 		if ($user === null) {
 			return;
 		}
-		$this->translateGroupToIds($samlGroups);
+		$this->translateGroupToIds($samlGroupNames);
 		$assignedGroups = $this->groupManager->getUserGroups($user);
 		$assignedGroupIds = array_map(function (IGroup $group) {
 			return $group->getGID();
 		}, $assignedGroups);
-		$groupsToRemove = $this->getGroupsToRemove($samlGroups, $assignedGroups);
-		$groupsToAdd = $this->getGroupsToAdd($samlGroups, $assignedGroupIds);
+		$groupsToRemove = $this->getGroupsToRemove($samlGroupNames, $assignedGroups);
+		$groupsToAdd = $this->getGroupsToAdd($samlGroupNames, $assignedGroupIds);
 		$this->removeGroups($user, $groupsToRemove);
 		$this->addGroups($user, $groupsToAdd);
 	}
@@ -144,11 +155,16 @@ public function removeGroup(IUser $user, string $gid): void {
 		if ($group === null) {
 			return;
 		}
-		$this->ownGroupBackend->removeFromGroup($user->getUID(), $group->getGID());
-		if ($this->ownGroupBackend->countUsersInGroup($gid) === 0) {
-			$this->groupManager->emit('\OC\Group', 'preDelete', [$group]);
-			$this->ownGroupBackend->deleteGroup($group->getGID());
-			$this->groupManager->emit('\OC\Group', 'postDelete', [$group]);
+
+		if ($this->hasSamlBackend($group)) {
+			$this->ownGroupBackend->removeFromGroup($user->getUID(), $group->getGID());
+			if ($this->ownGroupBackend->countUsersInGroup($gid) === 0) {
+				$this->groupManager->emit('\OC\Group', 'preDelete', [$group]);
+				$this->ownGroupBackend->deleteGroup($group->getGID());
+				$this->groupManager->emit('\OC\Group', 'postDelete', [$group]);
+			}
+		} else {
+			$group->removeUser($user);
 		}
 	}
 
@@ -239,4 +255,41 @@ public function evaluateGroupMigrations(array $groups): void {
 
 		$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, null);
+		if ($candidateInfo === null) {
+			return false;
+		}
+		$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;
+		}
+
+		return in_array($groupId, $candidateInfo['groups']);
+	}
+
+	protected function hasGroupForeignMembers(IGroup $group): bool {
+		foreach ($group->getUsers() as $user) {
+			if ($user->getBackendClassName() !== 'user_saml') {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	/**
+	 * In the transition phase, update group memberships of local groups only
+	 * under very specific conditions. Otherwise, membership modifications are
+	 * allowed only for groups owned by the SAML backend.
+	 */
+	protected function mayModifyGroup(?IGroup $group): bool {
+		return
+			$group !== null
+			&& $group->getGID() !== 'admin'
+			&& in_array('Database', $group->getBackendNames())
+			&& $this->isGroupInTransitionList($group->getGID())
+			&& !$this->hasGroupForeignMembers($group);
+	}
 }