feat(auth): enterprise OIDC SSO login with JIT provisioning (security-hardened) - #74
Merged
Conversation
Implements OAuth 2.0 Authorization Code Flow + PKCE for federated authentication. Adds sso_providers table (Enterprise-tier gated), OIDC provider CRUD admin API, and public SSO login flow with ID token validation and just-in-time user provisioning. Key features: - Authorization Code + PKCE flow (state single-use ≤10min, nonce validation) - ID token signature verification via JWKS (RS256/ES256 only, HS256 rejected) - JIT provisioning: matches by SSO subject + email, creates Viewer on new users - SSO login bypasses local TOTP MFA (IdP owns MFA) - Fernet-encrypted client secrets at rest (HKDF-SHA256 derived key) - License-gated admin endpoints (Enterprise tier only) - HTTPS validation on all IdP endpoint URLs - No PII logged, sanitized error messages Deferred: SAML 2.0 support (separate feature, same table). Adds comprehensive test coverage: secret encryption, PKCE, state tokens, ID token validation (nonce/aud/alg/sig), JIT provisioning (create/match), provider listing, authorization URL generation, and license gating. 188 existing tests still passing (schema test expects new table, 2 tests need JWKS mock refinement, 1 needs scope-aware token factory). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add sso:read, sso:write, sso:admin scopes to SystemAdmin role - Update sso_admin.py to use sso:read/write scopes (not admin:read/write) - Fix ID token JWKS validation test: patch PyJWKClient at module level - Fix admin endpoint tests: use SystemAdmin role (has sso:write scope) - Update test_schema.py to expect sso_providers table All 192 tests now passing (0 failures).
[CRITICAL] Finding 1: Code verifier in state JWT - Replace JWT state with opaque random tokens (not decodable) - Store code_verifier server-side in sso_login_attempts table - State sent to client is only random opaque token - Tests: state is random ~43 chars, not JWT-decodable, no verifier inside [CRITICAL] Finding 2: Email account takeover - Match existing users by (sso_provider, sso_subject) ONLY - Refuse auto-link if local account exists with same email (403) - Require email_verified=true for JIT provisioning (add claim to ValidatedIDToken) - Tests: local account refused; unverified email refused; returning user matched by subject [HIGH] Finding 3: Login CSRF via browser binding - Set httpOnly, Secure, SameSite=Lax cookie at /authorize - Store SHA-256(binding_cookie) in attempt row - Callback validates cookie hash matches; reject if missing/mismatched - Tests: binding mismatch rejected; missing cookie rejected [MEDIUM] Finding 4: Nonce is a no-op - Persist nonce in sso_login_attempts row - Pass expected_nonce to validate_id_token from stored attempt - ID token nonce claim must match exactly - Tests: wrong nonce rejected [MEDIUM] Finding 5: Redirect URI from query param - Drop query param override; always use server config OIDC_REDIRECT_URI - Prevents open redirect via redirect_uri= parameter [LOW] Finding 6: Placeholder password hash is weak - SSO users: password_hash = NULL (not placeholder '*'*64) - AuthService.verify_password short-circuits before bcrypt if hash is falsy - Local login attempt on SSO user → clean 401, no exception - Tests: SSO user created with NULL; verify_password returns False for NULL New schema: sso_login_attempts table + password_hash nullable Migrations: 010 (sso_login_attempts), 011 (password_hash nullable) All 182 tests passing (11 new SSO security tests).
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @PenguinzTech, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
… test coverage The real OIDC ID-token signature/claims validation path in SSOService.validate_id_token() crashed at runtime with NameError: name 'jwt' is not defined because the file only imported from jwt.exceptions and jwt.PyJWKClient but never imported jwt itself for the jwt.decode() call. This bug went undetected through 182 tests because existing tests either mocked SSOService.validate_id_token wholesale at too high a level (in callback handlers), or when they did test ID token validation (test_wrong_nonce_rejected), they never exercised the success path or other failure modes. The happy path had zero real coverage. Flake8's undefined-name check (F821) would have caught this instantly, but the repo's flake8 pre-commit hook was also broken (YAML flow-sequence args not quoted). Fixes: 1. Add missing `import jwt` (line 30) 2. Add comprehensive TestIDTokenValidation class with 5 real unit tests: - test_valid_id_token_accepted: happy path with correct nonce, all claims - test_wrong_audience_rejected: aud mismatch → None - test_expired_token_rejected: exp in past → None - test_algorithm_confusion_hs256_rejected: HS256 attack blocked by ALLOWED_ALGS - test_email_verified_false_ignored_in_validation: token accepted, JIT layer rejects 3. Fix .pre-commit-config.yaml flake8 args quoting (YAML issue) 4. Bump hadolint rev v2.13.0 → v2.13.1 (v2.13.0 was never a real tag) 5. Add .gitleaks.toml with sensible allowlist for docs, tests, fixtures All tests now pass: 187 green. Flake8 clean (F821 + E9,F63,F7 checks). Pre-commit hooks pass end-to-end. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This was referenced Jul 27, 2026
- .gitleaks.toml: both branches independently created this file (add/add conflict). Kept origin/v2.1.x's version -- accumulated and already validated across #69/#71/#73/#77's resolutions. - .pre-commit-config.yaml: same flake8/hadolint divergence resolved identically to prior merges this session (single .flake8 source of truth + flake8-bugbear; hadolint v2.14.0). - app/schema.py: auth_user gained sso_provider/sso_subject (this branch) alongside external_id (#73 SCIM, already merged) -- both independent new columns, combined. - app/services/scopes.py: sso:write/sso:admin (this branch) combined with audit:read (already merged, SystemAdmin-only). - tests/test_schema.py: expected-tables set now covers sso_providers/ sso_login_attempts alongside scim_tokens/machine_client/ oidc_trust_anchor/dpop_replay/audit_event (all already merged). - alembic: SSO's own chain (008_add_mfa_fields -> 009_add_sso_providers -> 010_add_sso_login_attempts -> 011_allow_null_password_hash) and SCIM's chain (008_add_mfa_fields -> 010_add_scim_provisioning) both forked from the same parent, authored independently off feature/mfa-totp. Re-chained 009_add_sso_providers to depend on 010_add_scim_provisioning (SCIM's already-merged migration) instead, restoring a single linear head. Full manager suite: 339/339 passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lated to this PR) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PenguinzTech
added a commit
that referenced
this pull request
Jul 27, 2026
- .gitleaks.toml: another add/add conflict (same file independently created on this branch and on v2.1.x). Took v2.1.x's canonical version, validated across every prior merge this session. - .pre-commit-config.yaml: same flake8/hadolint divergence resolved identically to every other merge in this batch. - app/schema.py: saml_provider/saml_assertion_id tables inserted cleanly (origin/v2.1.x had no competing content at this exact insertion point -- its own new tables live elsewhere in the file). - requirements.in/.txt: pysaml2 (this branch) combined with boto3 + opentelemetry-* (already merged). pysaml2==7.4.2 confirmed already present in the shared venv at the correct pinned version. - tests/test_schema.py: saml_providers/saml_assertion_ids combined with scim_tokens/machine_client/oidc_trust_anchor/dpop_replay/ audit_event (all already merged). - tests/test_sso.py: TestIDTokenValidation (the real signature-path test coverage added when fixing #74's missing `jwt` import) existed only on origin/v2.1.x -- this branch predates that fix. Kept it, nothing on this side to preserve at that insertion point. - alembic: no chain fix needed -- SAML's own migrations (012_add_saml_providers -> 013_add_saml_assertion_ids) already correctly chain from 011_allow_null_password_hash (SSO's actual tip when this branch was authored off feature/sso-oidc-login). Single linear head confirmed across the full 19-migration graph. Full manager suite: 361/361 passing -- the final combined total across every merged PR in this session's enterprise-hardening effort. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PenguinzTech
added a commit
that referenced
this pull request
Jul 27, 2026
Process error: after merging #74 (SSO) into v2.1.x, I deleted my LOCAL feature/sso-oidc-login branch ref but never deleted or retargeted the REMOTE branch/PR. #76 (SAML)'s base silently remained feature/sso-oidc-login instead of v2.1.x, so `gh pr merge 76` merged SAML into that orphaned remote branch -- completely disconnected from v2.1.x. GitHub still shows PR #76 as MERGED, but v2.1.x itself never received any of it (confirmed: alembic versions/ was missing 012_add_saml_providers.py and 013_add_saml_assertion_ids.py entirely). Nothing was lost -- the fully-resolved SAML merge commit (d7ffab1, carrying all of #76's conflict resolution work) still existed on the orphaned branch. Merged it into the actual v2.1.x tip here: clean, no conflicts (SAML's changes are manager/backend-only; v2.1.x's only change since d7ffab1's parent was #72's Helm-only work). Verified: 19 alembic migrations, single linear head (013_add_saml_assertion_ids). Full manager suite: 361/361 passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Enterprise OIDC SSO login (Authorization Code + PKCE) with JIT provisioning — license-gated Enterprise tier; SAML 2.0 deliberately deferred. Admin CRUD for providers (Fernet-encrypted client secrets, HTTPS-only endpoints, new
sso:read/sso:writescopes in the SystemAdmin bundle), public provider list exposing name+display_name only, server-side code exchange, ID-token validation via JWKS (RS256/ES256 only), JIT-created users land as Viewer with no local password. SSO users bypass local TOTP (the IdP owns MFA — documented).Security hardening (adversarial review ran against this branch; all 6 findings fixed and test-covered):
sso_login_attempts) — no PKCE verifier or anything decodable crosses the front channelemail_verifiedrequired for new accounts; existing local-email accounts are never auto-linked (explicit refusal)Tests: 182 green, independently re-verified (including replayed-state, cookie-mismatch, unverified-email, wrong-nonce, and local-login-on-SSO-user rejection paths). Migrations 009–011.
Stack note: stacks on #62 (
feature/mfa-totp) alongside #73 (SCIM); auto-retargets as the stack merges. Parallel migration-numbering caveat applies (SCIM also mints a010).🤖 Generated with Claude Code