feat(auth): add OpenID Connect (OIDC) SSO login#238
Open
pchopinet wants to merge 6 commits into
Open
Conversation
Add OIDC single sign-on alongside the existing username/password login, for self-hosted instances that plug into an external identity provider (Keycloak, Authentik, Google Workspace, Azure AD...). - WorkspaceOIDCBackend: JIT provisioning, optional email-domain allowlist, optional email_verified enforcement, and a configurable username claim with fallback and de-duplication. - Wired as a second auth backend only when OIDC is fully configured, so local username/password login keeps working when it is not. - Conditional "Sign in with <provider>" button on the login page. - oidc_discover management command prints the OIDC_OP_* endpoints from an issuer discovery document.
📝 WalkthroughWalkthroughThis PR implements OpenID Connect (OIDC) single sign-on (SSO) authentication, allowing users to log in via external identity providers while preserving local username/password authentication. The implementation includes the OIDC backend, identity tracking, login page integration, password protection for OIDC-managed users, and comprehensive deployment guidance. ChangesOIDC/SSO Authentication
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Make the identity provider authoritative for OIDC-provisioned accounts: - OIDCIdentity model marks a user as managed by the provider (created on first login, JIT or email match). - update_user refreshes first_name / last_name from the claims on each login, only when the claim is present so a missing claim never wipes a value. - ChangePasswordView returns 403 and the Security tab shows a managed-by- provider note instead of the password form for managed users.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
workspace/users/management/commands/oidc_discover.py (1)
40-42: 💤 Low valueChain the re-raised
CommandErrorto preserve the original cause.Ruff flags this (B904). Chaining keeps the underlying
httpx/JSON error in the traceback for easier diagnosis.♻️ Proposed change
except (httpx.HTTPError, ValueError) as exc: raise CommandError( - f'Failed to fetch discovery document from {scrub(url)}: {exc}') + f'Failed to fetch discovery document from {scrub(url)}: {exc}' + ) from exc🤖 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 `@workspace/users/management/commands/oidc_discover.py` around lines 40 - 42, The current except block in oidc_discover.py re-raises a CommandError without chaining the original exception; update the except handler that catches (httpx.HTTPError, ValueError) as exc to re-raise the CommandError using exception chaining (i.e., raise CommandError(f'Failed to fetch discovery document from {scrub(url)}: {exc}') from exc) so the original httpx/JSON exception is preserved in the traceback.
🤖 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.
Inline comments:
In `@workspace/urls.py`:
- Around line 62-64: The OIDC routes are always mounted; change the URL config
so the include('mozilla_django_oidc.urls') is only added when
settings.OIDC_ENABLED is truthy: check settings.OIDC_ENABLED in
workspace/urls.py and wrap or conditionally add the path('oidc/',
include('mozilla_django_oidc.urls')) entry (the same gating used for the login
button) so that when settings.OIDC_ENABLED is False the OIDC init/callback
endpoints are not registered and return 404.
In `@workspace/users/services/oidc.py`:
- Around line 96-106: In _link_identity, stop using
OIDCIdentity.objects.get_or_create blindly; instead first read the incoming sub
(as now) and return if empty, then (1) check for a different existing
OIDCIdentity for this user: if one exists verify its sub equals the incoming sub
and if not raise/return an error (i.e. fail closed) instead of silently keeping
the old link; (2) check for any OIDCIdentity with the incoming sub that is
linked to a different user
(OIDCIdentity.objects.filter(sub=sub).exclude(user=user).exists()) and fail with
an error if found; (3) only if no conflicts create the new OIDCIdentity for this
user (or use get_or_create after the collision checks) and handle any
IntegrityError to be safe; use the function name _link_identity and model
OIDCIdentity to locate where to implement these checks.
- Around line 122-129: The username-generation loop can never terminate when
base is already 150 chars because adding a numeric suffix then truncating with
f'{base}{suffix}'[:150] produces the same string; change the loop to reserve
space for the numeric suffix before truncating: compute suffix_str =
str(suffix), determine allowed_base_len = max(0, 150 - len(suffix_str)), build
username as base[:allowed_base_len] + suffix_str, and use that for the existence
check in the while loop (symbols to edit: base, username, suffix, and the
self.UserModel.objects.filter(...).exists() loop). Ensure suffix increments and
allowed_base_len is recomputed each iteration so the loop can progress and
terminate.
In `@workspace/users/tests/test_oidc.py`:
- Around line 94-97: Add two regression tests to users/tests/test_oidc.py: one
that verifies _generate_username handles a 150-character preferred_username
collision by returning a truncated/unique variant (reference
backend._generate_username and the existing test_dedupes_on_collision for
pattern), and another that verifies linking behavior when a user already exists
without a bound sub or with a different/changed sub/email (create an existing
user, simulate OIDC payloads with missing/changed 'sub' and changed 'email',
call the backend.link_user/associate method or whatever path triggers
sub-binding, and assert the correct refusal or re-linking behavior). Ensure each
test fails against the current buggy implementation before applying the fix.
---
Nitpick comments:
In `@workspace/users/management/commands/oidc_discover.py`:
- Around line 40-42: The current except block in oidc_discover.py re-raises a
CommandError without chaining the original exception; update the except handler
that catches (httpx.HTTPError, ValueError) as exc to re-raise the CommandError
using exception chaining (i.e., raise CommandError(f'Failed to fetch discovery
document from {scrub(url)}: {exc}') from exc) so the original httpx/JSON
exception is preserved in the traceback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a732d629-a2fc-4dd9-b394-7affdbbaae75
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.env.exampledocs/deployments/docker-compose/README.mddocs/deployments/docker-compose/docker-compose.ymldocs/deployments/kubernetes/secrets.yamlpyproject.tomlworkspace/settings.pyworkspace/urls.pyworkspace/users/management/__init__.pyworkspace/users/management/commands/__init__.pyworkspace/users/management/commands/oidc_discover.pyworkspace/users/migrations/0008_oidcidentity.pyworkspace/users/models.pyworkspace/users/services/oidc.pyworkspace/users/tests/test_oidc.pyworkspace/users/tests/test_oidc_discover.pyworkspace/users/ui/templates/users/ui/auth/login.htmlworkspace/users/ui/templates/users/ui/partials/settings_security.htmlworkspace/users/ui/views.pyworkspace/users/views.py
Two issues found in code review:
- _generate_username looped forever when a 150-char username already
existed: f'{base}{suffix}'[:150] truncated back to base, so the dedup
loop never advanced and the login request hung. Reserve room for the
numeric suffix before truncating.
- _link_identity treated the stored OIDC subject as a write-once marker:
a login whose sub disagreed with the stored one was silently accepted,
and a sub already bound to another account raised IntegrityError (500)
after the JIT user had already been created. It now fails closed on both
mismatch directions, and create_user wraps creation and linking in one
transaction so a conflict rolls the new user back instead of orphaning it.
Both ship with regression tests verified to fail against the previous code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds OpenID Connect (OIDC) single sign-on as a second login option next to the existing username/password form, for self-hosted instances that want to plug into an external identity provider (Keycloak, Authentik, Authelia, Google Workspace, Azure AD, ...).
How it works
WorkspaceOIDCBackend(subclass of mozilla-django-oidc's backend) holds all the custom rules:OIDC_ALLOWED_DOMAINS).email_verifiedenforcement (OIDC_REQUIRE_EMAIL_VERIFIED, off by default).OIDC_USERNAME_CLAIM, defaultpreferred_username), falling back to the email local-part thensub, sanitized with de-duplication.AUTHENTICATION_BACKENDSonly when OIDC is fully configured. The library backend raisesImproperlyConfiguredon missing OP endpoints (and on an RS/ES sign algo without a JWKS endpoint), and Django instantiates every backend on eachauthenticate()call, so wiring it unconditionally would break username/password login on any instance that has not configured OIDC.OIDC_ENABLEDgates this (and also requires the JWKS endpoint when the sign algo is RS/ES).oidc_discovermanagement command: prints theOIDC_OP_*endpoints from an issuer's.well-known/openid-configuration, so admins do not have to hand-copy them.No new database model, no migration.
Configuration (self-hosted)
then set in the environment:
IdP redirect URI to register:
{origin}/oidc/callback/.Tests
test_oidc.py,test_oidc_discover.py) covering claim verification, the domain allowlist,email_verifiedhandling, username generation / dedup / sanitization, the conditional login button, the settings wiring (OIDC backend absent when disabled so local login cannot break), and the discovery command.usersmodule: 276 tests pass, coverage 92.3% (floor 85%); the two new code files are at 100%.