feat: SAML SSO client SDK, dashboard, demo, and e2e tests#1397
Closed
BilalG1 wants to merge 19 commits intopr/saml-backendfrom
Closed
feat: SAML SSO client SDK, dashboard, demo, and e2e tests#1397BilalG1 wants to merge 19 commits intopr/saml-backendfrom
BilalG1 wants to merge 19 commits intopr/saml-backendfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
This was referenced Apr 29, 2026
BilalG1
force-pushed
the
pr/saml-backend
branch
from
e60f439 to
2d1db7a
Compare
Three methods on StackClientApp that mirror signInWithOAuth:
- signInWithSaml({ connectionId, returnTo }) — explicit connection
selection. Calls /auth/saml/login/[connectionId] in stack_response_mode
=json so the SDK can intercept the redirect URL.
- signInWithSso({ email, returnTo }) — email-domain discovery via
/auth/saml/discover, then redirects through the matched connection.
Throws when no connection matches so callers can fall back to other
sign-in methods.
- getSamlConnectionForEmail(email) — pure lookup with no redirect, so
the customer's UI can render branding ("Sign in with Acme SSO")
before the user clicks.
Backed by getSamlUrl + authorizeSaml + discoverSamlConnection on
StackClientInterface (mirrors getOAuthUrl + authorizeOAuth pattern,
without provider_scope or bot challenge — SAML originates from a
corporate IdP, not a public form).
Generated via pnpm -w run generate-sdks; propagates from
packages/template into packages/js, packages/react, packages/stack.
examples/demo/src/app/saml-demo/page.tsx — manual end-to-end check for the SAML round-trip. Two flows: 1. Email-domain discovery: enter alice@acme.test, click "Sign in via SSO". The SDK calls getSamlConnectionForEmail then redirects via the matched connection. 2. Direct connection ID: per-tenant buttons that call signInWithSaml with explicit connectionId (the pattern most B2B login pages use when they brand each tenant separately). Page also shows the current signed-in state + an SDK snippet so a developer can see exactly what to copy. Pairs with seed-dummy-data's STACK_SEED_ENABLE_SAML=true block which pre-creates the matching acme + globex connections.
Three test files exercising the SAML routes that don't require a full IdP round-trip: - discover.test.ts: 5 cases for /auth/saml/discover — happy path, unknown domain (404), case-insensitivity, unknown project_id, cross-project isolation (project A's connection isn't visible from a query against project B's domain). - metadata.test.ts: 3 cases for /auth/saml/metadata — XML contains entityID + ACS URL embedded, 404 for unknown connection, 404 when connection exists but has no IdP cert (incomplete configuration). - login.test.ts: 5 cases for /auth/saml/login — JSON-mode returns the IdP redirect URL with SAMLRequest+RelayState, browser-redirect mode sets the stack-saml-inner- CSRF cookie, 404 unknown connection, 403 when allowSignIn=false, invalid client_id rejected. Test integrity: all tests drive the API only — no imports from apps/backend/src/saml/. SAML config is set via the standard config override endpoint (no test-only mutator), so the routes run through the same code path real customers would hit. Full SAML round-trip tests (login → mock IdP → ACS → session) deferred to a follow-up — they need a sequenced flow against the mock-saml-idp service that's separate from these endpoint-level tests.
apps/e2e/tests/backend/endpoints/api/v1/auth/saml/round-trip.test.ts
exercises the entire SP-initiated flow against the running mock IdP on
port 8115:
GET /auth/saml/login → IdP URL with SAMLRequest
POST mock /idp/[tenant]/login → auto-POST HTML with signed SAMLResponse
POST /auth/saml/acs → backend verifies + issues OAuth code
Five test cases:
1. Happy path: new user JIT-created, ACS responds with 303/307 + OAuth
code in the redirect.
2. Wrong audience: mock IdP misbehaves via /test-controls
{ kind: 'wrong-audience' }, backend rejects.
3. Bad signature (cross-tenant forgery): mock signs with another
tenant's key via { kind: 'bad-signature' }, backend rejects.
4. Expired assertion: NotOnOrAfter in the past via { kind: 'expired' },
backend rejects.
5. Replay: same SAMLResponse POSTed twice — second attempt rejected
because SamlOuterInfo was consumed by the first ACS call.
Fetches the mock IdP's cert at test setup time so the SAML
verification chain is real (the mock regenerates keys per startup, so
hardcoded certs would never match).
Test integrity reaffirmed: the test file imports only from helpers,
backend-helpers, and ports — NO imports from apps/backend/src/saml/.
Negative cases come from the mock deliberately misbehaving, never from
injecting bad data into the backend's own validator. Mock IdP uses
samlify; backend uses @node-saml/node-saml — different libraries on
each side mean a bug in either surfaces as a test failure rather than
canceling out.
Tests written and lint/typecheck clean; runtime verification needs the
backend + mock-saml-idp services up (CI workflow already wired).
Single page at /projects/[projectId]/sso for managing SAML connections. Lists existing connections (read from project.useConfig() — same source as the e2e tests and seed script use), with add + delete dialogs. Add dialog: ID, display name, optional email domain (for discovery), IdP entity ID, IdP SSO URL, and IdP signing certificate. PEM headers are stripped from the cert automatically before saving. Delete dialog warns that user accounts linked via the connection remain in the database — they just become unable to sign in until a connection with the same id is recreated. Each connection card surfaces the SP metadata URL + ACS URL so the customer's IT admin can copy them into the IdP console without manually composing the URLs. V1 limitations (planned follow-ups): - Edit happens by deleting + recreating; no in-place edit dialog yet. - No "paste IdP metadata XML" auto-fill (the backend metadata-parser exists; just needs to be wired up to a paste box). - No separate detail page; everything is on the list.
E2E tests + dashboard SSO page were writing per-field deep dot-keys like `auth.saml.connections.X.displayName`, which the config normalizer drops because the parent record entry doesn't yet exist when the connection is being created. Match the existing auth.oauth.providers convention: write the whole connection entry as a single value on create. Also fixed two test-harness issues uncovered while running the suite: - Round-trip ACS POST was using niceBackendFetch which always JSON.stringifies the body. Switched to plain niceFetch so URLSearchParams gets sent as application/x-www-form-urlencoded. - Mock IdP /metadata returns application/xml, which makes niceFetch return ArrayBuffer; added a TextDecoder pass before regex matching.
Surfaced while taking screenshots for the PR description: - yup.string().url() rejects http://localhost which breaks any local or test-env IdP setup. The backend SAML wrapper validates the URL on use anyway. Drop the client-side .url() check. - The form was set to pushable=true which routes through the GitHub-pushable config dialog. SAML connection fields (cert, IdP URLs) live at the environment level (not the branch level that's pushed to GitHub) — same as OAuth client secrets in the auth-methods page. Set pushable=false to write directly via env config override.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Register a new alpha-stage `saml-sso` app rather than exposing SAML to every project. Users opt in from the App Store; the dashboard SSO page, admin CRUD, and SDK routes all 400 with `SAML_SSO_NOT_ENABLED` until the app is installed. Alpha apps stay hidden in production via the existing NODE_ENV filter in `getAllAvailableAppIds`. - Add `saml-sso` to `ALL_APPS` + `ALL_APPS_FRONTEND` (icon, store copy, nav item pointing at the existing /sso route) - Wrap SSO page with `AppEnabledGuard` for the redirect-on-disabled UX - Backend: each SAML route checks `apps.installed["saml-sso"]?.enabled` and throws the new `KnownErrors.SamlSsoNotEnabled` - E2E: setup helpers enable the app on test projects; new discover test verifies the gate fires for unconfigured projects - Seed dummy data: enable saml-sso when `STACK_SEED_ENABLE_SAML=true` so the seeded acme/globex connections work without an extra click - Fix a pre-existing indent slip in the delete-dialog updateConfig call
The login route built the SP `callbackUrl` from `query.redirect_uri.origin`, which is the customer's app — not the backend. The IdP would then POST the assertion to e.g. `http://localhost:8103/api/v1/auth/saml/acs/acme` (the demo app), which 404s because the ACS handler only exists on the backend. Fix both login and ACS to derive `baseUrl` from the incoming request's own origin, matching what the metadata route already does. The e2e round-trip test didn't catch this because in tests the customer and backend run on the same host.
Previously wrote per-field deep dot-keys (`auth.saml.connections.X.displayName`, ...). When the parent record entry didn't yet exist, normalization with `onDotIntoNonObject="ignore"` silently dropped them — POST returned 200 but persisted nothing. Mirror the dashboard's create dialog (commit 5fa9629) by writing the whole connection object as a single overlay entry. Add a regression test that creates a connection from empty and verifies it round-trips through LIST + GET; covers the gate too.
…orkflows Without the mock IdP running, the SAML round-trip e2e test fails with ECONNREFUSED on port 8115 (or 6715 with the custom port prefix). The standard e2e workflow already starts the service; mirror that step here.
mantrakp04
requested changes
Apr 30, 2026
Collaborator
|
@greptile-ai review |
Contributor
Greptile SummaryThis PR adds the user-facing surface for SAML 2.0 SSO: three new Confidence Score: 5/5Safe to merge — no P0 or P1 findings; all previously flagged issues are resolved in this version Only P2 findings remain: the demo page type cast and discoverSamlConnection lacking _withFallback. Neither affects correctness or security. The backend feature gates, round-trip e2e coverage, and SDK patterns are solid. No files require special attention Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Customer App
participant SDK as StackClientApp (SDK)
participant API as Stack Backend API
participant IdP as Corporate IdP
Note over App,IdP: SP-Initiated SSO (signInWithSaml)
App->>SDK: signInWithSaml({ connectionId })
SDK->>API: GET /auth/saml/login/[connectionId]
API-->>SDK: { location: "https://idp.example.com/sso?SAMLRequest=..." }
SDK->>IdP: Redirect browser to IdP SSO URL
Note over App,IdP: IdP authenticates user
IdP-->>API: POST /auth/saml/acs/[connectionId] (SAMLResponse)
API->>API: Verify assertion (node-saml)
API-->>App: Redirect with OAuth code
Note over App,IdP: Email-Domain Discovery (signInWithSso)
App->>SDK: signInWithSso({ email })
SDK->>API: GET /auth/saml/discover?email=...&project_id=...
API-->>SDK: { connection_id, display_name }
SDK->>SDK: signInWithSaml({ connectionId })
Note over App,IdP: Dashboard SAML Management
App->>API: GET /api/v1/saml-connections (admin)
API-->>App: List of connections
App->>API: POST /api/v1/saml-connections (create)
App->>API: DELETE /api/v1/saml-connections (with body.id)
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
examples/demo/src/app/saml-demo/page.tsx:9-13
**Type assertion suggests SDK types may not be fully exported**
`useStackApp()` is cast with `as ... & { signInWithSaml, signInWithSso, getSamlConnectionForEmail }` to add the new SAML methods. If `pnpm generate-sdks` correctly propagated the new entries from `packages/template` → `packages/stack`, the intersection cast is unnecessary — and its presence here suggests the methods may not yet surface through the standard `useStackApp()` return type. The cast silently bypasses type-checking, so a mismatch between the asserted signature and the actual runtime object would go undetected. Worth verifying the generated types in `packages/stack` actually export these three methods before shipping the demo as a reference.
### Issue 2 of 2
packages/stack-shared/src/interface/client-interface.ts:1503-1535
**`discoverSamlConnection` uses bare `fetch` without `_withFallback`**
`discoverSamlConnection` issues a plain `fetch` against `getBestApiUrl()`, with no retry on the fallback URL if the primary is unavailable. This is the same gap as `authorizeSaml` in the same file — the OAuth equivalent (`_withFallback`) transparently retries on a secondary endpoint. Because `signInWithSso` calls `discoverSamlConnection` before delegating to `signInWithSaml`, a primary-URL outage would fail the discovery step with a network error rather than falling back, even though the rest of the sign-in would have succeeded. Wrapping in `_withFallback` would make the discovery call consistent with the rest of the interface layer.
Reviews (2): Last reviewed commit: "fix(saml): address PR review — absolute ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pull in PR review fixes from pr/saml-backend (5c9dab2, 1f79579): SP origin pinned to NEXT_PUBLIC_STACK_API_URL, branch_id query param on discover + metadata, retryTransaction-wrapped SAML account linking, NameID-as-email format restriction, POST-only metadata rejection, ACS allowSignIn re-check, atomic SamlOuterInfo consume, and Object.hasOwn guards on all `connections[id]` lookups. Conflict resolutions across the five SAML route handlers: - Kept HEAD's saml-sso app gate alongside the backend PR's `has()` checks. - Took the backend PR's NEXT_PUBLIC_STACK_API_URL approach for SP origin (deploy-time-stable, matches the OAuth provider convention) and dropped the now-unused `fullReq` arg from login + ACS handler signatures. - Took the backend PR's create/update branching in saml-connections POST so optional fields (domain, attributeMapping) are preserved on partial updates; removed a leftover overlay declaration from HEAD.
…, port-prefix-aware tests
- Dashboard SSO page now renders absolute SP metadata + ACS URLs from
NEXT_PUBLIC_BROWSER_STACK_API_URL so values are paste-ready into Okta
/ Azure AD / Google Workspace consoles.
- Trim + lowercase email domain on write so trailing whitespace can't
silently break discovery (matching is case-insensitive but not trim).
- Guard DeleteDialog displayName against stale deleteId after a config
refresh removed the entry.
- signInWithSso gains the same browser-only guard as signInWithSaml so
SSR callers get a coherent error from the right method.
- SAML e2e tests now derive redirect_uri from localhostUrl("01") and
the mock IdP base from suffix 42 (matches mock-saml-idp default port);
align both wait-on URLs in the workflows. Round-trip happy path now
exchanges the OAuth code, asserts is_new_user + the JIT-created
user's primary_email and display_name, and asserts the callback origin
matches the redirect_uri so a custom-port job can no longer pass while
wired to a non-running dashboard.
mantrakp04
approved these changes
May 1, 2026
mantrakp04
approved these changes
May 1, 2026
Previously the call would no-op the redirect and then await neverResolve(), hanging the caller forever. Throw an explicit error instead so the misuse surfaces immediately.
Collaborator
Author
|
closing for now, handling this post launch |
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
2 participants
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.
Third of 3 stacked PRs adding SAML 2.0 SSO. Stacked on: #1396 (backend) → #1395 (mock IdP).
Screenshots
The dashboard SSO management page is brand new — there's no "before" page to compare to. The screenshots below show the page in its three meaningful states (empty, mid-create, after-add) plus dark mode and the delete confirmation.
1. Empty state — no connections configured yet
2. "Add SAML connection" dialog (clean, before fill)
3. After adding an Acme connection — surfaces SP metadata URL + ACS URL for the IdP admin to copy
4. Same in dark mode
5. Delete confirmation — warns that linked user accounts remain in the DB
What this PR adds
The user-facing surface and verification for the backend SAML feature in #1396.
Client SDK
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.tsadds three methods onStackClientApp:signInWithSaml({ connectionId, returnTo })— explicit connection selection. MirrorssignInWithOAuth.signInWithSso({ email, returnTo })— email-domain discovery via/auth/saml/discover, then redirects through the matched connection. Throws if no connection matches so callers can fall back.getSamlConnectionForEmail(email)— pure lookup (no redirect) so the customer's UI can show "Sign in with Acme SSO" branding before the user clicks.Backed by
getSamlUrl+authorizeSaml+discoverSamlConnectiononStackClientInterface(mirrorsgetOAuthUrl+authorizeOAuth, without provider_scope or bot challenge — SAML originates from a corporate IdP, not a public form). Generated viapnpm -w run generate-sdks; propagates frompackages/templateintopackages/js,packages/react,packages/stack.Dashboard
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/sso/— single page for managing SAML connections on the current project. Reads fromproject.useConfig()(same source as the e2e tests + seed script), writes viauseUpdateConfig. PEM headers stripped from cert input automatically. Each connection card surfaces SP metadata URL + ACS URL for the customer's IT admin to copy.V1 limitations (planned follow-ups):
/projects/<id>/sso)Demo
examples/demo/src/app/saml-demo/page.tsx— exercises both SDK flows:alice@acme.test, click "Sign in via SSO"signInWithSaml({ connectionId })Pairs with the seed-dummy-data block from #1395 (gated on
STACK_SEED_ENABLE_SAML=true).E2E tests — 18 tests across 4 files, all passing
apps/e2e/tests/backend/endpoints/api/v1/auth/saml/:discover.test.tsmetadata.test.tslogin.test.tsround-trip.test.tsTest integrity self-check (verified)
grep -r \"from.*backend/src/saml\" apps/e2e/tests/→ 0 matches (tests don't import backend internals)samlify, backend uses@node-saml/node-saml— different libraries, so signature/canonicalization bugs would surface as failures rather than canceling out/test-controls, never from injecting bad data into the backend's own validatorTest plan
pnpm --filter @stackframe/template typecheck+lintpasspnpm -w run generate-sdksruns cleanly; new methods appear in packages/js, packages/react, packages/stackpnpm --filter @stackframe/example-demo-app typecheck+lintpasspnpm --filter @stackframe/dashboard typecheck+lintpass