Skip to content

OpenConceptLab/ocl_online#482 | SSO sign-in matches a legacy mixed-case username on its verified email - #958

Merged
snyaggarwal merged 2 commits into
masterfrom
ocl_online#482
Oct 27, 2026
Merged

snyaggarwal merged 2 commits into
masterfrom
ocl_online#482

Conversation

@snyaggarwal

Copy link
Copy Markdown
Contributor

Fixes OpenConceptLab/ocl_online#482.

Keycloak sends a lowercase preferred_username, so a legacy mixed-case account (e.g. GDolin) wasn't matched and SSO sign-in created a case twin (gdolin).

filter_users_by_claims now matches, after the sub and exact-username lookups, an account whose username equals the claim ignoring case, but only when the claims carry that account's verified email (the rule deactivated accounts already use). Otherwise the sign-in is refused with a new SSOUsernameCaseMismatchRefused (401) and logged; no account is created. Several case-only matches are refused too, and an account linked to another Keycloak user still raises SSOIdentityMismatchRefused.

The three refusal exceptions now share a SSOSignInRefused base, caught by the backend and the auth middleware.

Existing case twins are left as they are; the exact (lowercase) username still wins over a case variant.

Tests: case-insensitive match on verified email, refusal for each bad email claim, multiple case-only matches, sub mismatch, reactivation of a deactivated mixed-case account, exact match preferred, end-to-end get_or_create_user, and the middleware's 401.

🤖 Generated with Claude Code

…se username on its verified email instead of creating a case twin
@snyaggarwal
snyaggarwal requested a review from paynejd October 23, 2026 08:34
@snyaggarwal snyaggarwal self-assigned this Oct 23, 2026

@snyaggarwal snyaggarwal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blocking findings.

I found no actionable correctness or security issues in the SSO username matching and refusal-path changes. The case-insensitive fallback is gated by a verified email match, exact username matches remain preferred when applicable, ambiguous case-only matches are refused, and the refusal base class is propagated through the middleware with coverage for the new 401 path.

Verification:

  • PR checks run 99: successful.
  • git diff --check: passed.
  • Python syntax compilation for all changed files: passed.
  • The available API container was on an older commit and could not run the PR’s targeted test classes.

GitHub does not allow the PR author to approve their own pull request, so this was posted as a comment review.

@paynejd

paynejd commented Oct 27, 2026

Copy link
Copy Markdown
Member

Codex review, pass 1 (codex-cli 0.164.0, commit 386b49f)

Verdict: clean. No findings on git diff 3259b83...386b49f5, reviewed against ocl_online#482 and its acceptance criteria, with extra attention on wrong-account linking, refusals that leave a session or token behind, and the browser and bearer-token paths.

Locally, core.common.tests.OCLOIDCAuthenticationBackendTest and core.middlewares.tests pass at 386b49f (50 tests, OK). The failing CI test, TranslateMappingsWithRegistryTest setUpClass, fails on master at 3259b83 too.

findings.json
{
  "reviewer": "codex-cli 0.164.0",
  "perspective": "defects",
  "mode": "full",
  "commit": "386b49f5",
  "verdict": "clean",
  "prior": [],
  "findings": []
}

@paynejd paynejd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Codex came back clean (see above), the PR's 50 backend and middleware tests pass locally, and CI's only failure comes from master.

An adversarial security pass found nothing P0 or P1 here. The case-insensitive path needs the verified email, and every sign-in path (browser, bearer token, middleware, GraphQL) refuses cleanly without leaving a token behind. The notes, and the cleanup steps for the existing twins, are on ocl_online#482. Merge when CI is green on a fresh master.

@snyaggarwal
snyaggarwal merged commit ded8383 into master Oct 27, 2026
2 of 3 checks passed
@snyaggarwal
snyaggarwal deleted the ocl_online#482 branch October 27, 2026 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants