From 267a4556aeb3531bfaa91be77056db34e97d191b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Dubois?= Date: Thu, 9 Jul 2026 13:59:45 +0200 Subject: [PATCH] fix: OIDC login crash when a Keycloak group name collides with an existing Plone user id --- CHANGES.md | 4 +- src/pas/plugins/kimug/plugin/__init__.py | 52 ++++++++++++++++++++++++ tests/plugin/test_plugin.py | 32 +++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index fe62ba1..5678d08 100644 --- a/CHANGES.md +++ b/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) diff --git a/src/pas/plugins/kimug/plugin/__init__.py b/src/pas/plugins/kimug/plugin/__init__.py index a8dce3f..0188395 100644 --- a/src/pas/plugins/kimug/plugin/__init__.py +++ b/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. diff --git a/tests/plugin/test_plugin.py b/tests/plugin/test_plugin.py index bcae0a5..17a06a7 100644 --- a/tests/plugin/test_plugin.py +++ b/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