Skip to content

Auth: OIDC / SAML SSO provider#733

Open
jaeyunha wants to merge 1 commit into
stagingfrom
issue-517-auth-oidc-saml-sso-provider
Open

Auth: OIDC / SAML SSO provider#733
jaeyunha wants to merge 1 commit into
stagingfrom
issue-517-auth-oidc-saml-sso-provider

Conversation

@jaeyunha

Copy link
Copy Markdown
Member

Summary

  • add workspace-scoped OIDC settings, capability reporting, and discovery/callback endpoints with PKCE, nonce, ID token validation, domain enforcement, member attach, and session creation
  • expose SAML/OIDC provider capabilities to the login UI and route enterprise SSO discovery through capabilities
  • add expn auth oidc bootstrap command plus OpenAPI/SDK contract updates

Verification

  • pnpm --filter @namuh-eng/expn-sdk build
  • pnpm --filter @namuh-eng/expn-cli typecheck && pnpm --filter @exponential/web typecheck
  • pnpm --filter @namuh-eng/expn-cli exec vitest run src/run.test.ts
  • pnpm --filter @exponential/web exec vitest run tests/login-page.test.tsx
  • make openapi-coverage

Blocked verification

  • go test ./internal/authproviders ./internal/workspaces and make check cannot complete in this lane because the go binary is not installed; make check passed typecheck and Biome before failing at Go API build with go: not found.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d316d17f49

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

$ref: '#/components/schemas/SamlDiscoveryResponse'
default:
$ref: '#/components/responses/Problem'
/auth/oidc/discovery:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Regenerate Go OpenAPI stubs for OIDC paths

Adding these OIDC operations to the public contract also requires regenerating apps/api/internal/openapi/openapi.gen.go; I checked that generated file and it still has no DiscoverOidcUrl, OidcCallback, or UpdateCurrentWorkspaceOidc symbols/routes. Any Go code or tests that rely on the checked-in strict-server contract will remain unaware of these endpoints, leaving the OpenAPI source and generated backend artifacts inconsistent.

Useful? React with 👍 / 👎.

}
return ssoSettingsConfigured(provider, raw)
}
rows, err := h.DB.Query(r.Context(), `select coalesce(settings,'{}'::jsonb) from workspace`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid scanning every workspace on login capability checks

When /login calls provider capabilities without a workspace callbackUrl, this branch reads the settings JSON for every workspace; ProviderCapabilities calls it separately for SAML and OIDC, so each anonymous login page load can do two unbounded table scans, especially bad when no matching SSO is found. In hosted deployments with many workspaces this makes login latency and DB load grow with total workspace count; consider combining/caching the checks or using indexed JSONB predicates instead of loading all rows.

Useful? React with 👍 / 👎.

@jaeyunha

Copy link
Copy Markdown
Member Author

Controller disposition for current head d316d17: validation-blocked; do not merge yet.

Current blocker:

  • PR Auth: OIDC / SAML SSO provider #733 implements OIDC discovery/callback, provider capabilities, CLI bootstrap, and login SSO routing, but it does not satisfy the linked Auth: OIDC / SAML SSO provider #517/spec-prep admin configuration + acceptance-test contract yet. There is no web workspace admin OIDC settings surface, and the Go test delta only covers settings parsing/claim validation; it does not cover duplicate-domain ambiguity, state/nonce/PKCE replay rejection, successful callback session creation, or SAML regression.

Controller evidence:

  • PR is MERGEABLE against staging, current-path diff only, and no GitHub checks are reported.
  • Passed: make check, go test ./internal/authproviders ./internal/workspaces, SDK build, CLI typecheck + src/run.test.ts, web typecheck + tests/login-page.test.tsx, and make openapi-coverage.
  • Broad make test is blocked by existing workspace-prefix expectations (exponential.app/ vs localhost:7015/) outside this PR's diff; not accepted as merge evidence.

Required before merge: add the OIDC admin configuration UI and the missing callback/replay/duplicate-domain/SAML regression coverage, then rerun focused gates.

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.

1 participant