Skip to content

feat(oidc): add enterprise SSO login with Authorization Code + PKCE - #59

Open
bonashen wants to merge 1 commit into
OpenBMB:mainfrom
bonashen:main
Open

feat(oidc): add enterprise SSO login with Authorization Code + PKCE#59
bonashen wants to merge 1 commit into
OpenBMB:mainfrom
bonashen:main

Conversation

@bonashen

@bonashen bonashen commented Aug 1, 2026

Copy link
Copy Markdown
  • /api/auth/oidc/{config,authorize,callback} endpoints with state + PKCE S256 + nonce; state rows deleted before validation to prevent replay

  • JWT verification via JWKS with issuer/audience/exp checks, clock skew tolerance, optional nonce check

  • user mapping by (tenant_id, oidc_sub), auto-provision on first login; configurable display name (OIDC_NAME)

  • login page SSO button, callback token consumption from URL fragment, session bootstrap with oidc-pending placeholder

  • keep original Host header in the vite /api proxy so redirect_uri derivation stays correct

  • 9 backend tests + 5 frontend tests; E2E validated against Keycloak in WSL docker compose

- /api/auth/oidc/{config,authorize,callback} endpoints with state + PKCE S256 + nonce; state rows deleted before validation to prevent replay

- JWT verification via JWKS with issuer/audience/exp checks, clock skew tolerance, optional nonce check

- user mapping by (tenant_id, oidc_sub), auto-provision on first login; configurable display name (OIDC_NAME)

- login page SSO button, callback token consumption from URL fragment, session bootstrap with oidc-pending placeholder

- keep original Host header in the vite /api proxy so redirect_uri derivation stays correct

- 9 backend tests + 5 frontend tests; E2E validated against Keycloak in WSL docker compose
@hm1229
hm1229 requested a review from fadeoreo August 3, 2026 02:31
@fadeoreo

fadeoreo commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

I tested this PR locally against the latest origin/main.

Merge result:

  • Merges cleanly into current main
  • backend/tests/test_oidc_auth.py: 9 passed
  • backend/tests/test_desktop_launcher.py: 20 passed
  • frontend-enterprise/src/oidcCallback.test.ts: 5 passed
  • npm run build: passed

The core OIDC flow looks solid overall: Authorization Code + PKCE, state/nonce handling, JWKS-based ID token validation, issuer/audience/expiration checks, one-time state consumption, and stable user mapping by OIDC sub.

That said, I think there are a few points worth addressing before treating this as production-ready enterprise SSO:

  1. Auto-provisioning is enabled by default

    OIDC authenticates who the user is, but it does not by itself decide whether that user should be allowed into StaffDeck. With OIDC_AUTO_PROVISION=true by default, any user who can authenticate through the configured IdP can be auto-created as a StaffDeck member.

    For enterprise SSO, I would prefer one of:

    • default OIDC_AUTO_PROVISION=false
    • support allowed domains / allowed groups / required claims
    • require pre-existing StaffDeck users and bind OIDC identities explicitly
  2. redirect_uri inference depends on request Host/base URL

    When OIDC_REDIRECT_URI is empty, the callback URL is derived from request.base_url. This can be fragile behind reverse proxies or public deployments unless Host / forwarded headers are very carefully controlled.

    I suggest either:

    • requiring explicit OIDC_REDIRECT_URI when OIDC is enabled in production
    • adding a PUBLIC_BASE_URL
    • storing the computed redirect_uri in the state row during authorize and reusing that exact value during callback
  3. Database migration appears SQLite-focused

    The migration adds users.oidc_sub and a partial unique index in the SQLite migration path. If StaffDeck is deployed with a non-SQLite database, existing deployments may not get the new column/index automatically.

    This matters for enterprise SSO deployments, which are more likely to use an external DB.

  4. OIDC identity mapping should include issuer/provider

    sub is only guaranteed to be stable within a given issuer. The current mapping uses (tenant_id, oidc_sub). For long-term safety, especially if multiple IdPs or issuer changes are possible, I would prefer:

    tenant_id + oidc_issuer + oidc_sub
    

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