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

[remdub]

…sting Plone user id

Summary by CodeRabbit

• Bug Fixes

  • Fixed an OIDC login crash that could occur when a group name matched an existing user ID.
  • Group syncing now safely skips unmappable groups instead of failing during sign-in.
  • Existing group memberships continue to be updated normally for valid groups.

• Tests

  • Added coverage for the collision case to ensure login remains stable and valid groups are still assigned.

• Documentation

  • Updated the release notes with the latest fix.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a _create_update_groups override in KimugPlugin to safely synchronize OIDC token groups into Plone, skipping group creation when a token group id collides with an existing user principal and returns None, instead of crashing. Includes a regression test and a changelog entry.

Changes

Group sync collision fix

Layer / File(s)	Summary
Group sync override implementation src/pas/plugins/kimug/plugin/__init__.py	Imports safe_write and adds _create_update_groups, which removes stale group memberships, creates/joins token groups, and skips a group when api.group.create returns None due to an id collision, logging a warning instead of crashing.
Regression test and changelog tests/plugin/test_plugin.py, CHANGES.md	Adds a test confirming a colliding group id ("imio") is skipped without error while a valid group ("real-group") is still created and assigned, and documents the fix as an unreleased 1.9.3 changelog entry.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OIDCLogin
  participant KimugPlugin
  participant PloneGroupAPI

  OIDCLogin->>KimugPlugin: _create_update_groups(user, token groups)
  KimugPlugin->>PloneGroupAPI: remove stale group memberships
  KimugPlugin->>PloneGroupAPI: api.group.create(missing group id)
  PloneGroupAPI-->>KimugPlugin: None (id collision with user)
  KimugPlugin->>KimugPlugin: log warning, skip group
  KimugPlugin->>PloneGroupAPI: api.group.create(real-group)
  PloneGroupAPI-->>KimugPlugin: group object
  KimugPlugin->>PloneGroupAPI: join user to real-group
  KimugPlugin-->>OIDCLogin: user.getGroups()

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main fix: an OIDC login crash caused by a Keycloak group/user id collision.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/oidc-group-user-id-collision

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/pas/plugins/kimug/plugin/__init__.py (1)

143-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Guard logic is correct; note the duplication risk with upstream.

The None-check correctly prevents the AttributeError crash on id collisions, and the test confirms the expected behavior (skip colliding group, still create/join non-colliding ones).

The one thing worth flagging: this override re-implements the entire upstream _create_update_groups body rather than delegating to it, so any future behavioral change in pas.plugins.oidc's implementation (e.g. new group properties, different removal semantics) will silently diverge from this fork unless someone remembers to resync it manually. Consider adding a short comment noting the coupling to the specific upstream version, or a test that pins/exercises the pieces this override intentionally mirrors, so a future upstream upgrade doesn't quietly regress.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pas/plugins/kimug/plugin/__init__.py` around lines 143 - 192, The
override in `_create_update_groups` duplicates the upstream
`OIDCPlugin._create_update_groups` implementation, so it can silently drift from
future `pas.plugins.oidc` behavior. Add a short note near
`_create_update_groups` (or a focused test around this method) that explicitly
documents the dependency on the current upstream implementation and the parts
intentionally mirrored, so future upgrades of `OIDCPlugin` can be checked
against this forked logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/pas/plugins/kimug/plugin/__init__.py`:
- Around line 143-192: The override in `_create_update_groups` duplicates the
upstream `OIDCPlugin._create_update_groups` implementation, so it can silently
drift from future `pas.plugins.oidc` behavior. Add a short note near
`_create_update_groups` (or a focused test around this method) that explicitly
documents the dependency on the current upstream implementation and the parts
intentionally mirrored, so future upgrades of `OIDCPlugin` can be checked
against this forked logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0588be7c-4742-40fb-a1be-b17ac879467c

📥 Commits

Reviewing files that changed from the base of the PR and between 635420b and 267a455.

📒 Files selected for processing (3)

• CHANGES.md
• src/pas/plugins/kimug/plugin/__init__.py
• tests/plugin/test_plugin.py