-
Notifications
You must be signed in to change notification settings - Fork 329
feat(auth): Organization-level SAML SSO #2057
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
f9bd254 to
6a4c6c4
Compare
6a4c6c4 to
9907664
Compare
There was a problem hiding this comment.
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: 9907664131
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ) | ||
| return | ||
|
|
||
| org_id = await resolve_auth_organization_id(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exempt SAML callbacks from request-org lookup
In EE multi-tenant mode, this lookup now requires ?org= or tracecat-org-id on every SAML route, but /auth/saml/acs gets org context from RelayState inside sso_acs rather than query/cookie. Since app.py mounts the whole SAML router behind require_auth_type_enabled(AuthType.SAML), ACS requests from the IdP can fail with 428 Organization selection required before relay-state parsing, which blocks SAML sign-in for users who do not already have an org override cookie.
Useful? React with 👍 / 👎.
| if (!showOidcAuth) { | ||
| throw new Error("OIDC login is not enabled") | ||
| } | ||
| await startOidcLogin(orgSlug, returnUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use discovered org slug when starting OIDC flow
The discovery response now includes organization_slug, but the OIDC path still uses only the page-level orgSlug. For users landing on /sign-in without ?org=..., discovery can resolve the org from email domain, yet this call still omits that org and triggers /auth/oauth/... without org context; with the new auth-type dependency checks, multi-tenant setups can reject the request before login is initiated.
Useful? React with 👍 / 👎.
| providerLabel={oidcProviderLabel} | ||
| providerIcon={oidcProviderIcon} | ||
| {discoveredMethod === "basic" ? ( | ||
| <BasicLoginForm orgSlug={orgSlug} initialEmail={discoveredEmail} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Carry discovered org slug into password login
After domain discovery selects basic, the password form is still bound to the URL orgSlug instead of the discovered organization_slug. In multi-tenant mode, users who start from /sign-in without an org query will submit /auth/login without org context and hit the new auth-type guard (428), so domain-discovered basic-auth users cannot complete sign-in.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 issues found across 19 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/auth/sign-in.tsx">
<violation number="1" location="frontend/src/components/auth/sign-in.tsx:176">
P2: Unexpected auth discovery methods fall through to SAML login. If the server returns an unknown `method` value, the code will attempt a SAML login instead of showing an error. Add an explicit check for `data.method === "saml"` and handle unknown methods gracefully.</violation>
</file>
<file name="tracecat/auth/org_context.py">
<violation number="1" location="tracecat/auth/org_context.py:15">
P2: Duplicated `ORG_OVERRIDE_COOKIE` constant. This is already defined in `tracecat/auth/credentials.py:97`. Import it from there (or move it to a shared location like `tracecat/auth/constants.py` or `tracecat/auth/types.py`) to avoid silent divergence.</violation>
</file>
<file name="frontend/src/components/auth/oauth-buttons.tsx">
<violation number="1" location="frontend/src/components/auth/oauth-buttons.tsx:15">
P3: `orgSlug` will leak to the DOM via `...props` in `GithubOAuthButton`. The component destructures `returnUrl` but not the newly added `orgSlug`, so it passes through to the underlying `<Button>` element. Destructure it alongside `returnUrl` to prevent a React DOM warning.</violation>
</file>
<file name="frontend/src/hooks/use-auth.ts">
<violation number="1" location="frontend/src/hooks/use-auth.ts:44">
P2: Login error discards HTTP status and response body, throwing a generic `Error("Login failed")`. This loses valuable diagnostic information (e.g., why the login was rejected) and breaks the `instanceof ApiError` error-handling branch in the sign-up flow. Consider parsing the response body and including the status/detail in the thrown error.</violation>
</file>
<file name="frontend/src/components/organization/org-settings-sso.tsx">
<violation number="1" location="frontend/src/components/organization/org-settings-sso.tsx:62">
P2: Guard saml_enforced so it is only submitted when SAML is allowed and enabled; otherwise it can persist true despite the toggle being disabled.</violation>
</file>
<file name="tracecat/api/app.py">
<violation number="1" location="tracecat/api/app.py:597">
P2: The /info endpoint now falls back to the default org for any HTTPException from resolve_auth_organization_id, including invalid org slugs (400). This hides errors and can leak default-org SAML settings when a caller supplies an invalid org. Only fall back when org context is missing (428) and re-raise other HTTP exceptions.</violation>
</file>
<file name="tracecat/auth/discovery.py">
<violation number="1" location="tracecat/auth/discovery.py:62">
P2: When `org_slug` is provided but not found (org doesn't exist or is inactive), the code silently falls through to email-domain-based discovery. This could route the user to a different organization's auth method than intended. Consider returning an error or at minimum including a signal in the response (e.g., `organization_slug=None` with a distinguishable method) so the client knows the specified org wasn't resolved.</violation>
</file>
<file name="tracecat/auth/dependencies.py">
<violation number="1" location="tracecat/auth/dependencies.py:149">
P2: Bug: `_check_any_auth_type_enabled` does not implement "any" semantics — an exception from `verify_auth_type` on the first matching candidate aborts the loop instead of falling through to try the next candidate. Since `verify_auth_type` can now raise for all auth types (SAML enforcement check, org resolution), the first failure prevents any subsequent type from being attempted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 13 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tracecat/auth/discovery.py">
<violation number="1" location="tracecat/auth/discovery.py:65">
P2: Avoid raising HTTPException from the service layer. Per the project’s error-handling guidance, services should raise a domain/validation exception (e.g., ValueError/TracecatValidationError) and let routers or a global handler translate it to an HTTP response.
(Based on your team's feedback about error handling posture.) [FEEDBACK_USED]</violation>
</file>
<file name="tracecat/api/app.py">
<violation number="1" location="tracecat/api/app.py:507">
P1: Removing the `require_auth_type_enabled(AuthType.SAML)` dependency from the SAML router makes the `/auth/saml/*` endpoints reachable even when SAML is disabled or not allowed. Since the router/ACS endpoint doesn't add its own auth-type dependency, this bypasses the org-level SAML enablement check and can expose SAML endpoints unintentionally. Reapply the auth-type dependency when including the router (or add the dependency at the router/endpoint level).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
25aa856 to
436c0fe
Compare
436c0fe to
877fb60
Compare

Summary by cubic
Adds org-level SAML SSO with an email-first sign-in that routes by email or org to basic, OIDC, or SAML. Enforces SAML per org, restores the SAML login guard, and makes discovery and /info org-aware with safe defaults.
Written for commit 877fb60. Summary will update on new commits.