fix: OIDC login crash when a Keycloak group name collides with an exi…

CHANGES.md

@@ -1,7 +1,9 @@
 ## 1.9.3 (unreleased)
 
 
-- Nothing changed yet.
+- Fix OIDC login crash when a Keycloak group name collides with an
+  existing Plone user id (e.g. the `imio` IdP-link alias): skip the unmappable
+  group instead of dereferencing `None` in `_create_update_groups`. [remdub]
 
 
 ## 1.9.2 (2026-07-09)

src/pas/plugins/kimug/plugin/__init__.py

@@ -6,6 +6,7 @@
 from pas.plugins.kimug.interfaces import IKimugPlugin
 from pas.plugins.kimug.utils import is_log_active
 from pas.plugins.oidc.plugins import OIDCPlugin
+from pas.plugins.oidc.plugins import safe_write
 from plone import api
 from Products.PageTemplates.PageTemplateFile import PageTemplateFile
 from Products.PluggableAuthService.interfaces import plugins as pas_interfaces
@@ -139,6 +140,57 @@ def getRolesForPrincipal(self, user, request=None):
             logger.info(f"getRolesForPrincipal: assigned roles={tuple(roles)}")
         return tuple(roles)
 
+    @security.private
+    def _create_update_groups(self, user, user_id, userinfo):
+        """Sync the token's group claim into Plone groups.
+
+        Overrides ``OIDCPlugin._create_update_groups`` to guard against a group
+        id that cannot be created as a Plone group. ``api.group.create`` returns
+        ``None`` when the id collides with an existing principal: Keycloak group
+        names (e.g. the ``imio`` IdP link alias) can match an existing Plone
+        *user* id, in which case ``Products.PlonePAS`` refuses the group
+        (``searchPrincipals`` finds the user) and ``getGroupById`` returns
+        ``None``. The upstream method then dereferences ``None`` and raises
+        ``AttributeError: 'NoneType' object has no attribute 'getTool'``, turning
+        an interactive login into an HTTP 500. Here we skip the unmappable id
+        (logging a warning) instead of crashing.
+        """
+        groupid_property = self.getProperty("user_property_as_groupid")
+        group_ids = userinfo.get(groupid_property)
+        if isinstance(group_ids, str):
+            group_ids = [group_ids]
+
+        if isinstance(group_ids, list):
+            with safe_write(self.REQUEST):
+                oidc = self.getId()
+                groups = user.getGroups()
+                # Remove group memberships
+                for gid in groups:
+                    group = api.group.get(gid)
+                    if group is None:
+                        continue
+                    is_managed = group.getProperty("type") == oidc.upper()
+                    if is_managed and gid not in group_ids:
+                        api.group.remove_user(group=group, username=user_id)
+                # Add group memberships
+                for gid in group_ids:
+                    if gid not in groups:
+                        group = api.group.get(gid) or api.group.create(gid, title=gid)
+                        if group is None:
+                            logger.warning(
+                                "Skipping group '%s' from token: it collides "
+                                "with an existing principal id and cannot be "
+                                "created as a Plone group.",
+                                gid,
+                            )
+                            continue
+                        # Tag managed groups with "type" of plugin id
+                        if not group.getTool().hasProperty("type"):
+                            group.getTool()._setProperty("type", "", "string")
+                        group.setGroupProperties({"type": oidc.upper()})
+                        api.group.add_user(group=group, username=user_id)
+        return user.getGroups()
+
     @security.private
     def extractCredentials(self, request):
         """Extract an OAuth2 bearer access token from the request.

tests/plugin/test_plugin.py

@@ -80,6 +80,38 @@ def test_create_user(self, browser_layers):
         assert pas.getUserById("kimug") is not None
         assert api.user.get_users()[0].getUserId() == "kimug"
 
+    def test_create_update_groups_skips_user_id_collision(self, portal):
+        """A token group whose id collides with an existing Plone *user* id must
+        be skipped, not crash the login.
+
+        Regression: the ``imio`` IdP-link group name shipped in the token matched
+        an existing ``imio`` Plone user, so ``api.group.create`` returned ``None``
+        (PlonePAS refuses a group whose id is already a principal) and the
+        upstream ``_create_update_groups`` raised
+        ``AttributeError: 'NoneType' object has no attribute 'getTool'``.
+        """
+        plugin = portal.acl_users.oidc
+        pas = portal.acl_users
+        with api.env.adopt_roles(["Manager"]):
+            # Existing user whose id equals a group name carried by the token.
+            api.user.create(username="imio", email="imio@example.com")
+            # The user that is logging in.
+            api.user.create(username="login-uid", email="login@example.com")
+            user = pas.getUserById("login-uid")
+
+            # Must not raise despite the "imio" user/group collision.
+            plugin._create_update_groups(
+                user, "login-uid", {"groups": ["imio", "real-group"]}
+            )
+
+            user = pas.getUserById("login-uid")
+            # The colliding id is skipped: no group created, user not joined.
+            assert api.group.get("imio") is None
+            assert "imio" not in user.getGroups()
+            # A normal group is still created and assigned.
+            assert api.group.get("real-group") is not None
+            assert "real-group" in user.getGroups()
+
     def test_ensure_user_exists_creates_user(self, portal):
         """_ensure_user_exists should create a Plone user with email from payload."""
         plugin = portal.acl_users.oidc