Build SAML login (enterprise/internal/loginhandler), mirroring OIDC

Adds GET /auth/saml/login + POST /auth/saml/acs alongside the existing
OIDC pair, both converging on the same upsert-user/resolve-tenant/
issue-session path. loginhandler.New now takes an optional
*saml.ServiceProvider, RegisterRoutes registers each protocol's routes
independently so either, both, or neither can be configured. SAML's
replay/unsolicited-response defense (InResponseTo, standing in for
OIDC's state) is carried via a SameSite=None sentry_saml_request cookie
-- None because the ACS endpoint receives a cross-site POST from the
IdP's origin, which SameSite=Lax cookies are never sent on.
enterprise-auth's main.go now fetches+parses SAML_IDP_METADATA_URL at
startup (samlsp.FetchMetadata) and wires the result through.

Verified to the same bar as OIDC: a real fake IdP
(crewjam/saml/samlidp, genuine XML signing/verification) drives the
full login->ACS->session-cookie round trip and negative paths (bad
InResponseTo, missing request cookie, missing email/NameID, no/multiple
tenant memberships), all in loginhandler/saml_test.go, no Docker
needed. The login-form HTML is bypassed by pre-seeding a saml.Session
directly into samlidp's session store and presenting the matching
`session` cookie -- an IdP-supported shortcut (confirmed by reading
GetSession), the same "skip the UI, keep the crypto real" approach
oidctest gave the OIDC tests.

Writing that test caught two real bugs in internal/saml.ParseResponse,
both fixed here: it never called r.ParseForm() before reading the
POSTed SAMLResponse field, so every real ACS POST would have silently
decoded an empty response; and its email-attribute matching missed
urn:oid:0.9.2342.19200300.100.1.3 (the standard LDAP "mail" OID), which
is what an IdP sends by default absent an explicit
AttributeConsumingService request for "email" -- exactly what
samlidp's own DefaultAssertionMaker does, and plausibly what real IdPs'
default SAML app templates do too.

Docs (CLAUDE.md, threat-model.md, architecture.md, enterprise/README.md,
phase-4-runbook.md, docker-compose.yml's enterprise-auth comment)
updated in lockstep: SAML login moves from "protocol mechanics only" to
"built, verified with a real fake IdP, not yet tried against a real
external IdP or a running enterprise-auth container" -- the same
disclosed gap OIDC already carried.

enterprise/README.md

@@ -86,12 +86,27 @@ section for exactly what "not yet run" means here and why. Don't read
   same "offline action, not a network endpoint" shape as
   `enterprise-auth -mint-service-token`.
 
+OIDC and SAML login are both now fully wired: `internal/loginhandler`
+serves `GET /auth/oidc/login`+`GET /auth/oidc/callback` and
+`GET /auth/saml/login`+`POST /auth/saml/acs`, converging on the same
+upsert-user/resolve-tenant/issue-session path. Both are verified the
+same way -- a real fake IdP with genuine cryptographic signing and
+verification (`coreos/go-oidc`'s `oidctest` for OIDC,
+`crewjam/saml/samlidp` for SAML), no Docker needed, every test in
+`loginhandler_test.go`/`saml_test.go` passing including the full login
+round trip and negative paths (bad state/`InResponseTo`, expired/missing
+credential, no/multiple tenant memberships). Writing the SAML test
+caught two real bugs in `internal/saml.ParseResponse`, both fixed:
+missing `r.ParseForm()` before reading the POSTed `SAMLResponse` field,
+and email-attribute matching that missed the standard LDAP "mail" OID
+(`urn:oid:0.9.2342.19200300.100.1.3`) that IdPs send by default absent
+an explicit `AttributeConsumingService` request for "email" -- exactly
+what `samlidp`'s own default assertion builder does. Neither protocol
+has been tried against a real external IdP or a running
+`enterprise-auth` container -- see `/docs/phase-4-runbook.md` §3a/§3b.
+
 **Deliberately deferred, not half-built** -- named explicitly rather than
 silently left out:
-- SAML's login handler (the ACS endpoint) -- `internal/saml` does the
-  protocol mechanics (AuthnRequest generation, assertion validation);
-  nothing calls it from an HTTP handler, following `internal/
-  loginhandler`'s now-built OIDC pattern once someone builds it.
 - A tenant-picker UI/flow for an identity with more than one
   `tenant_memberships` row -- `loginhandler` refuses these logins
   outright rather than guessing (`ErrMultipleMemberships`).
@@ -123,7 +138,7 @@ internal/oidc/           coreos/go-oidc wiring: discovery, login redirect, code
 internal/saml/            crewjam/saml wiring: SP setup, login redirect, response parsing/validation
 internal/session/          issues/validates signed session + RoleService tokens
 internal/authhandler/       POST /internal/authorize, GET /auth/features
-internal/loginhandler/       GET /auth/oidc/login, GET /auth/oidc/callback -- the human login flow
+internal/loginhandler/       GET /auth/oidc/{login,callback} + GET /auth/saml/login + POST /auth/saml/acs -- the human login flow
 internal/rbacstore/          users/tenants/tenant_memberships/data_sources CRUD (pgx against sentry_metadata)
 internal/tenantprovision/     real ClickHouse CREATE DATABASE/USER/GRANT
 internal/chrunner/             tenant-scoped api/querylang/executor.SQLRunner
@@ -134,9 +149,9 @@ internal/apiconfig/       enterprise-api's own env-var config
 internal/config/          enterprise-auth's env-var config
 ```
 
-Future additions: SAML's login handler, `dashboard_permissions` CRUD,
-ingest tenant-awareness (undesigned), and real deployment-topology
-wiring for `enterprise-api` -- see "Status" above.
+Future additions: `dashboard_permissions` CRUD, ingest tenant-awareness
+(undesigned), and real deployment-topology wiring for `enterprise-api`
+-- see "Status" above.
 
 ## Why OIDC and SAML aren't hand-rolled
 
@@ -246,7 +261,7 @@ edit today, not a supported flag.
 | `OIDC_REDIRECT_URL` | (empty — must be `<enterprise-auth base URL>/auth/oidc/callback`, registered with the IdP) |
 | `SAML_ENTITY_ID` | (empty) |
 | `SAML_ACS_URL` | (empty) |
-| `SAML_IDP_METADATA_URL` | (empty — presence only feeds `GET /auth/features`; not yet fetched/parsed) |
+| `SAML_IDP_METADATA_URL` | (empty — SAML disabled if unset; if set, fetched and parsed at startup via `samlsp.FetchMetadata`, same trust level as `OIDC_ISSUER_URL`'s discovery fetch) |
 | `ENTERPRISE_SESSION_SIGNING_KEY` | **required**, min 32 bytes |
 | `POST_LOGIN_REDIRECT_URL` | `http://localhost:3000` — where the browser lands after `internal/loginhandler` sets a session cookie |
 

enterprise/cmd/enterprise-auth/main.go

@@ -4,15 +4,16 @@
 //
 // Wires session issuance/validation (internal/session), the
 // POST /internal/authorize endpoint api/authz.HTTPAuthorizer calls (the
-// piece that turns on RBAC enforcement in /api), and -- since
-// internal/loginhandler -- the real GET /auth/oidc/login and
-// GET /auth/oidc/callback handlers that issue a *human* session after
-// an actual IdP round trip, resolving tenant/role via internal/rbacstore.
-// Still deliberately missing: SAML's equivalent (ACS endpoint) -- same
-// shape, not yet built, following internal/loginhandler's OIDC pattern
-// once it is. What's fully wired: -mint-service-token (the RoleService
-// credential /alerting presents) and, when OIDC_ISSUER_URL is
-// configured, a real human login flow.
+// piece that turns on RBAC enforcement in /api), and --
+// internal/loginhandler -- the real GET /auth/{oidc,saml}/login and
+// GET /auth/oidc/callback + POST /auth/saml/acs handlers that issue a
+// *human* session after an actual IdP round trip, resolving tenant/role
+// via internal/rbacstore. Both protocols are now fully wired -- OIDC via
+// discovery, SAML via fetching+parsing SAML_IDP_METADATA_URL at startup
+// (crewjam/saml's samlsp.FetchMetadata; a trusted operator-supplied URL,
+// same trust level as OIDC_ISSUER_URL's discovery fetch, not
+// end-user-controlled input). Also fully wired: -mint-service-token
+// (the RoleService credential /alerting presents).
 package main
 
 import (
@@ -21,12 +22,14 @@ import (
 	"fmt"
 	"log/slog"
 	"net/http"
+	"net/url"
 	"os"
 	"os/signal"
 	"strings"
 	"syscall"
 	"time"
 
+	"github.com/crewjam/saml/samlsp"
 	"github.com/jackc/pgx/v5/pgxpool"
 
 	"github.com/sentry/sentry/enterprise/internal/authhandler"
@@ -34,6 +37,7 @@ import (
 	"github.com/sentry/sentry/enterprise/internal/loginhandler"
 	"github.com/sentry/sentry/enterprise/internal/oidc"
 	"github.com/sentry/sentry/enterprise/internal/rbacstore"
+	samlpkg "github.com/sentry/sentry/enterprise/internal/saml"
 	"github.com/sentry/sentry/enterprise/internal/session"
 )
 
@@ -114,6 +118,33 @@ func main() {
 		logger.Info("OIDC not configured (OIDC_ISSUER_URL unset) -- skipping discovery, /auth/oidc/* routes disabled")
 	}
 
+	// samlProvider stays nil (loginhandler.RegisterRoutes then registers
+	// nothing) unless SAML is actually configured -- same shape as OIDC
+	// above.
+	var samlProvider *samlpkg.ServiceProvider
+	if cfg.SAML.IDPMetadataURL != "" {
+		metadataURL, err := url.Parse(cfg.SAML.IDPMetadataURL)
+		if err != nil {
+			logger.Error("parsing SAML_IDP_METADATA_URL", "error", err)
+			os.Exit(1)
+		}
+		idpMetadata, err := samlsp.FetchMetadata(ctx, http.DefaultClient, *metadataURL)
+		if err != nil {
+			logger.Error("fetching SAML IdP metadata", "error", err)
+			os.Exit(1)
+		}
+		samlProvider, err = samlpkg.New(samlpkg.Config{
+			EntityID: cfg.SAML.EntityID, ACSURL: cfg.SAML.ACSURL, IDPMetadata: idpMetadata,
+		})
+		if err != nil {
+			logger.Error("constructing SAML service provider", "error", err)
+			os.Exit(1)
+		}
+		logger.Info("SAML provider configured", "idp_metadata_url", cfg.SAML.IDPMetadataURL)
+	} else {
+		logger.Info("SAML not configured (SAML_IDP_METADATA_URL unset) -- /auth/saml/* routes disabled")
+	}
+
 	mux := http.NewServeMux()
 	mux.HandleFunc("GET /healthz", func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(http.StatusOK)
@@ -123,7 +154,7 @@ func main() {
 		SAMLEnabled: cfg.SAML.IDPMetadataURL != "",
 	}
 	authhandler.New(logger, sessionManager, features).RegisterRoutes(mux)
-	loginhandler.New(logger, oidcProvider, sessionManager, rbac, cfg.PostLoginRedirectURL).RegisterRoutes(mux)
+	loginhandler.New(logger, oidcProvider, samlProvider, sessionManager, rbac, cfg.PostLoginRedirectURL).RegisterRoutes(mux)
 
 	srv := &http.Server{Addr: cfg.HTTPListenAddr, Handler: mux}
 

enterprise/go.mod

@@ -37,6 +37,7 @@ require (
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/go-faster/city v1.0.1 // indirect
 	github.com/go-faster/errors v0.7.1 // indirect
+	github.com/golang-jwt/jwt/v4 v4.5.2 // indirect
 	github.com/jackc/pgpassfile v1.0.0 // indirect
 	github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect
 	github.com/jackc/puddle/v2 v2.2.2 // indirect

enterprise/internal/config/config.go

@@ -37,13 +37,11 @@ type OIDCConfig struct {
 	RedirectURL  string
 }
 
-// SAMLConfig is likewise optional. Note this only records *presence* --
-// enough for /auth/features (internal/authhandler) to report
-// saml_enabled -- it does not itself fetch/parse IDPMetadataURL into the
-// *saml.EntityDescriptor internal/saml.New requires; that fetch (and the
-// login/ACS HTTP handlers that would use it) is deferred, same as OIDC's
-// login/callback handlers -- see cmd/enterprise-auth/main.go's doc
-// comment.
+// SAMLConfig is likewise optional. cmd/enterprise-auth/main.go fetches
+// and parses IDPMetadataURL into the *saml.EntityDescriptor
+// internal/saml.New requires at startup (crewjam/saml's
+// samlsp.FetchMetadata) -- this struct just carries the raw config
+// values this package's job (env-var loading) is scoped to.
 type SAMLConfig struct {
 	EntityID       string
 	ACSURL         string

enterprise/internal/loginhandler/loginhandler.go

@@ -2,19 +2,19 @@
 // the actual HTTP login/callback flow that issues a *human* session,
 // not just /alerting's RoleService credential (-mint-service-token) or
 // the RBAC-enforcement plumbing that assumes a session already exists.
-// enterprise/internal/oidc does the OAuth2/OIDC protocol mechanics
-// (discovery, the auth-code redirect, code exchange, ID token
-// verification); this package is the two HTTP handlers that drive it
-// and decide what happens with a verified identity: look up or create a
-// users row, resolve which tenant/role that user belongs to, and issue
-// a session.Manager-signed session cookie.
+// enterprise/internal/oidc and enterprise/internal/saml do the protocol
+// mechanics (discovery/AuthnRequest generation, code exchange/assertion
+// parsing, signature verification); this package is the HTTP handlers
+// that drive them and decide what happens with a verified identity: look
+// up or create a users row, resolve which tenant/role that user belongs
+// to, and issue a session.Manager-signed session cookie. Both protocols
+// share that decision (resolveIdentity below) -- only how the identity
+// gets verified differs.
 //
-// Deliberately out of scope here: SAML's equivalent (ACS endpoint) --
-// same shape, not yet built, following this package's pattern once it
-// is. Multi-tenant users (one identity with memberships in more than
-// one tenant) are refused with a clear error rather than guessing which
-// tenant to log them into -- a tenant-selection step is real,
-// undesigned future work, not silently approximated.
+// Multi-tenant users (one identity with memberships in more than one
+// tenant) are refused with a clear error rather than guessing which
+// tenant to log them into -- a tenant-selection step is real, undesigned
+// future work, not silently approximated.
 package loginhandler
 
 import (
@@ -28,20 +28,28 @@ import (
 	"github.com/sentry/sentry/enterprise/internal/authhandler"
 	"github.com/sentry/sentry/enterprise/internal/oidc"
 	"github.com/sentry/sentry/enterprise/internal/rbacstore"
+	"github.com/sentry/sentry/enterprise/internal/saml"
 	"github.com/sentry/sentry/enterprise/internal/session"
 )
 
-// stateCookieName carries the CSRF-protection state value between the
-// login redirect and the callback -- a short-lived, scoped-to-the-
+// oidcStateCookieName carries OIDC's CSRF-protection state value between
+// the login redirect and the callback -- a short-lived, scoped-to-the-
 // callback-path cookie (the "double-submit cookie" pattern) rather than
 // server-side state, since this service otherwise has no per-browser
 // session store to put it in before a session exists.
-const stateCookieName = "sentry_oidc_state"
+const oidcStateCookieName = "sentry_oidc_state"
 
-// stateCookieTTL bounds how long a user has to complete the IdP round
+// samlRequestCookieName is SAML's analog -- carries the AuthnRequest ID
+// LoginURL generated, so the ACS handler can pass it back to
+// ParseResponse's possibleRequestIDs (SAML's actual replay/unsolicited-
+// response defense -- see saml.ServiceProvider.LoginURL's doc comment).
+const samlRequestCookieName = "sentry_saml_request"
+
+// loginCookieTTL bounds how long a user has to complete the IdP round
 // trip -- generous enough for a real login form, short enough that a
-// stale state cookie isn't a long-lived CSRF token sitting in a browser.
-const stateCookieTTL = 10 * time.Minute
+// stale cookie isn't a long-lived CSRF token sitting in a browser.
+// Shared by both protocols' cookies.
+const loginCookieTTL = 10 * time.Minute
 
 // userStore is the narrow interface Handler depends on -- *rbacstore.Store
 // is the production implementation; tests use a fake, same pattern used
@@ -60,9 +68,16 @@ type oidcProvider interface {
 	Exchange(ctx context.Context, code string) (*oidc.Claims, error)
 }
 
+// samlProvider mirrors oidcProvider's reasoning for SAML.
+type samlProvider interface {
+	LoginURL(relayState string) (redirectURL, requestID string, err error)
+	ParseResponse(r *http.Request, possibleRequestIDs []string) (*saml.Claims, error)
+}
+
 type Handler struct {
 	logger  *slog.Logger
 	oidc    oidcProvider // nil if OIDC isn't configured -- RegisterRoutes registers nothing in that case
+	saml    samlProvider // nil if SAML isn't configured -- same
 	session *session.Manager
 	users   userStore
 	// postLoginRedirectURL is where the browser lands after a session
@@ -70,36 +85,46 @@ type Handler struct {
 	postLoginRedirectURL string
 }
 
-// New takes a concrete *oidc.Provider (nilable), not the oidcProvider
-// interface directly -- a nil *oidc.Provider assigned straight into an
-// interface-typed field would produce a non-nil interface wrapping a
-// nil pointer (Go's classic typed-nil trap), which would silently break
-// RegisterRoutes'/handleLogin's `h.oidc == nil` checks the moment a
-// caller (enterprise-auth's main.go) passes a `var p *oidc.Provider`
-// that's legitimately still nil because OIDC isn't configured. Checking
-// the concrete pointer here, before it ever becomes the interface
-// field, is what keeps that check meaningful.
-func New(logger *slog.Logger, provider *oidc.Provider, sessionManager *session.Manager, users userStore, postLoginRedirectURL string) *Handler {
+// New takes concrete *oidc.Provider/*saml.ServiceProvider (both
+// nilable), not the narrower interfaces directly -- assigning a nil
+// pointer straight into an interface-typed field would produce a
+// non-nil interface wrapping a nil pointer (Go's classic typed-nil
+// trap), which would silently break RegisterRoutes'/the handlers'
+// `h.oidc == nil`/`h.saml == nil` checks the moment a caller
+// (enterprise-auth's main.go) passes a `var p *oidc.Provider` that's
+// legitimately still nil because that protocol isn't configured.
+// Checking the concrete pointers here, before they ever become the
+// interface fields, is what keeps those checks meaningful -- see
+// loginhandler_test.go's TestRegisterRoutesNoOpWithTypedNilProviderVariable
+// for the regression test that caught this the first time (OIDC; SAML
+// follows the same fix from day one).
+func New(logger *slog.Logger, oidcProvider *oidc.Provider, samlProvider *saml.ServiceProvider, sessionManager *session.Manager, users userStore, postLoginRedirectURL string) *Handler {
 	h := &Handler{logger: logger, session: sessionManager, users: users, postLoginRedirectURL: postLoginRedirectURL}
-	if provider != nil {
-		h.oidc = provider
+	if oidcProvider != nil {
+		h.oidc = oidcProvider
+	}
+	if samlProvider != nil {
+		h.saml = samlProvider
 	}
 	return h
 }
 
-// RegisterRoutes registers OIDC's two routes only if OIDC is actually
-// configured (h.oidc != nil) -- matches the "absent, not broken" default
+// RegisterRoutes registers each protocol's routes only if that protocol
+// is actually configured -- matches the "absent, not broken" default
 // every other optional-config path in this codebase follows (e.g.
 // api/authz.RequireRole's nil-authorizer no-op).
 func (h *Handler) RegisterRoutes(mux *http.ServeMux) {
-	if h.oidc == nil {
-		return
+	if h.oidc != nil {
+		mux.HandleFunc("GET /auth/oidc/login", h.handleOIDCLogin)
+		mux.HandleFunc("GET /auth/oidc/callback", h.handleOIDCCallback)
+	}
+	if h.saml != nil {
+		mux.HandleFunc("GET /auth/saml/login", h.handleSAMLLogin)
+		mux.HandleFunc("POST /auth/saml/acs", h.handleSAMLACS)
 	}
-	mux.HandleFunc("GET /auth/oidc/login", h.handleLogin)
-	mux.HandleFunc("GET /auth/oidc/callback", h.handleCallback)
 }
 
-func (h *Handler) handleLogin(w http.ResponseWriter, r *http.Request) {
+func (h *Handler) handleOIDCLogin(w http.ResponseWriter, r *http.Request) {
 	state, err := oidc.NewState()
 	if err != nil {
 		h.logger.Error("generating oidc state", "error", err)
@@ -107,29 +132,29 @@ func (h *Handler) handleLogin(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	http.SetCookie(w, &http.Cookie{
-		Name: stateCookieName, Value: state, Path: "/auth/oidc/callback",
+		Name: oidcStateCookieName, Value: state, Path: "/auth/oidc/callback",
 		HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode,
-		MaxAge: int(stateCookieTTL.Seconds()),
+		MaxAge: int(loginCookieTTL.Seconds()),
 	})
 	http.Redirect(w, r, h.oidc.AuthCodeURL(state), http.StatusFound)
 }
 
-// clearStateCookie is called on every path out of handleCallback --
-// the state cookie is single-use regardless of whether the login
-// ultimately succeeds, same reasoning a CSRF token gets discarded after
-// one use rather than left around for reuse.
-func clearStateCookie(w http.ResponseWriter, r *http.Request) {
+// clearCookie is called on every path out of the two callback handlers
+// below -- the state/request cookie is single-use regardless of whether
+// the login ultimately succeeds, same reasoning a CSRF token gets
+// discarded after one use rather than left around for reuse.
+func clearCookie(w http.ResponseWriter, r *http.Request, name, path string) {
 	http.SetCookie(w, &http.Cookie{
-		Name: stateCookieName, Value: "", Path: "/auth/oidc/callback",
+		Name: name, Value: "", Path: path,
 		HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode,
 		MaxAge: -1,
 	})
 }
 
-func (h *Handler) handleCallback(w http.ResponseWriter, r *http.Request) {
-	defer clearStateCookie(w, r)
+func (h *Handler) handleOIDCCallback(w http.ResponseWriter, r *http.Request) {
+	defer clearCookie(w, r, oidcStateCookieName, "/auth/oidc/callback")
 
-	stateCookie, err := r.Cookie(stateCookieName)
+	stateCookie, err := r.Cookie(oidcStateCookieName)
 	if err != nil || stateCookie.Value == "" {
 		http.Error(w, "missing or expired login state -- start over at /auth/oidc/login", http.StatusBadRequest)
 		return
@@ -156,9 +181,79 @@ func (h *Handler) handleCallback(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	identity, status, err := h.resolveIdentity(r.Context(), claims)
+	h.finishLogin(w, r, claims.Subject, claims.Email)
+}
+
+func (h *Handler) handleSAMLLogin(w http.ResponseWriter, r *http.Request) {
+	// relayState isn't used to carry anything here (postLoginRedirectURL
+	// is a fixed server-side config, not per-request) -- still generated
+	// fresh per login and round-tripped, since crewjam/saml's API expects
+	// one and an empty/constant value would be a needless deviation from
+	// how a real SP-initiated flow looks.
+	relayState, err := oidc.NewState() // same random-value generator, protocol-agnostic despite the package name
 	if err != nil {
-		h.logger.Error("resolving identity after oidc login", "error", err, "email", claims.Email)
+		h.logger.Error("generating saml relay state", "error", err)
+		http.Error(w, "internal error", http.StatusInternalServerError)
+		return
+	}
+	redirectURL, requestID, err := h.saml.LoginURL(relayState)
+	if err != nil {
+		h.logger.Error("building saml login url", "error", err)
+		http.Error(w, "internal error", http.StatusInternalServerError)
+		return
+	}
+	// SameSiteNoneMode, not Lax like OIDC's state cookie: SAML's
+	// HTTP-POST binding means the browser POSTs to /auth/saml/acs
+	// *from the IdP's origin* -- a cross-site POST, which SameSite=Lax
+	// cookies are never sent on (Lax only exempts top-level GET
+	// navigations, which is what OIDC's redirect-based callback is, but
+	// SAML's response delivery isn't). SameSite=None requires Secure
+	// per the cookie spec, so this genuinely needs the deployment to be
+	// on HTTPS -- realistic for any real SAML IdP integration (they
+	// require it too), but worth stating plainly: unlike OIDC, SAML
+	// login will not work correctly over plain HTTP.
+	http.SetCookie(w, &http.Cookie{
+		Name: samlRequestCookieName, Value: requestID, Path: "/auth/saml/acs",
+		HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteNoneMode,
+		MaxAge: int(loginCookieTTL.Seconds()),
+	})
+	http.Redirect(w, r, redirectURL, http.StatusFound)
+}
+
+func (h *Handler) handleSAMLACS(w http.ResponseWriter, r *http.Request) {
+	defer clearCookie(w, r, samlRequestCookieName, "/auth/saml/acs")
+
+	requestCookie, err := r.Cookie(samlRequestCookieName)
+	if err != nil || requestCookie.Value == "" {
+		http.Error(w, "missing or expired login state -- start over at /auth/saml/login", http.StatusBadRequest)
+		return
+	}
+
+	claims, err := h.saml.ParseResponse(r, []string{requestCookie.Value})
+	if err != nil {
+		h.logger.Error("parsing saml response", "error", err)
+		http.Error(w, "login failed", http.StatusUnauthorized)
+		return
+	}
+	if claims.Email == "" {
+		http.Error(w, "identity provider did not return an email attribute", http.StatusUnauthorized)
+		return
+	}
+	if claims.NameID == "" {
+		http.Error(w, "identity provider did not return a NameID", http.StatusUnauthorized)
+		return
+	}
+
+	h.finishLogin(w, r, claims.NameID, claims.Email)
+}
+
+// finishLogin is the point both protocols converge on: a verified
+// (subject, email) pair, still needing tenant/role resolution and a
+// session cookie -- everything from here down is protocol-agnostic.
+func (h *Handler) finishLogin(w http.ResponseWriter, r *http.Request, subject, email string) {
+	identity, status, err := h.resolveIdentity(r.Context(), subject, email)
+	if err != nil {
+		h.logger.Error("resolving identity after login", "error", err, "email", email)
 		http.Error(w, err.Error(), status)
 		return
 	}
@@ -181,7 +276,7 @@ var (
 	// ErrNoMembership and ErrMultipleMemberships are exported so tests
 	// (and any future caller that wants to distinguish these outcomes,
 	// e.g. to render a real tenant-picker UI instead of a flat error
-	// page) don't have to string-match handleCallback's HTTP error body.
+	// page) don't have to string-match the HTTP error body.
 	ErrNoMembership        = errors.New("loginhandler: this identity has no tenant membership -- contact your administrator")
 	ErrMultipleMemberships = errors.New("loginhandler: this identity belongs to multiple tenants -- tenant selection is not supported yet")
 )
@@ -193,12 +288,14 @@ type resolvedIdentity struct {
 }
 
 // resolveIdentity is the policy decision this whole package exists to
-// make: given a verified external identity, which tenant/role does it
-// map to. Deliberately conservative -- exactly one tenant_memberships
-// row is the only case handled; zero or multiple both refuse rather
-// than guess (see this package's doc comment).
-func (h *Handler) resolveIdentity(ctx context.Context, claims *oidc.Claims) (resolvedIdentity, int, error) {
-	user, err := h.users.UpsertUserBySSO(ctx, claims.Subject, claims.Email, claims.Email)
+// make: given a verified external identity (subject, email -- OIDC's
+// "sub"/"email" claims or SAML's NameID/email attribute, already
+// protocol-normalized by the caller), which tenant/role does it map to.
+// Deliberately conservative -- exactly one tenant_memberships row is the
+// only case handled; zero or multiple both refuse rather than guess
+// (see this package's doc comment).
+func (h *Handler) resolveIdentity(ctx context.Context, subject, email string) (resolvedIdentity, int, error) {
+	user, err := h.users.UpsertUserBySSO(ctx, subject, email, email)
 	if err != nil {
 		return resolvedIdentity{}, http.StatusInternalServerError, fmt.Errorf("loginhandler: upserting user: %w", err)
 	}

enterprise/internal/loginhandler/loginhandler_test.go

@@ -128,7 +128,7 @@ func newTestSessionManager(t *testing.T) *session.Manager {
 
 func TestHandleLoginRedirectsAndSetsStateCookie(t *testing.T) {
 	idp := newTestIdP(t)
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
 	mux := http.NewServeMux()
 	h.RegisterRoutes(mux)
 
@@ -144,7 +144,7 @@ func TestHandleLoginRedirectsAndSetsStateCookie(t *testing.T) {
 	cookies := rec.Result().Cookies()
 	var stateCookie *http.Cookie
 	for _, c := range cookies {
-		if c.Name == stateCookieName {
+		if c.Name == oidcStateCookieName {
 			stateCookie = c
 		}
 	}
@@ -168,7 +168,7 @@ func fullLoginFlow(t *testing.T, h *Handler, idp *testIdP) *httptest.ResponseRec
 	mux.ServeHTTP(loginRec, httptest.NewRequest(http.MethodGet, "/auth/oidc/login", nil))
 	var stateCookie *http.Cookie
 	for _, c := range loginRec.Result().Cookies() {
-		if c.Name == stateCookieName {
+		if c.Name == oidcStateCookieName {
 			stateCookie = c
 		}
 	}
@@ -188,7 +188,7 @@ func TestFullLoginFlowIssuesSessionForSingleMembership(t *testing.T) {
 	store := newFakeUserStore()
 	store.memberships["user-user-1"] = []rbacstore.Membership{{TenantID: "acme", UserID: "user-user-1", Role: rbacstore.RoleEditor}}
 	sessionManager := newTestSessionManager(t)
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), sessionManager, store, "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, sessionManager, store, "http://web/")
 
 	idp.setNextIDToken(t, "user-1", "[email protected]", true, time.Now().Add(time.Hour))
 	rec := fullLoginFlow(t, h, idp)
@@ -220,7 +220,7 @@ func TestFullLoginFlowIssuesSessionForSingleMembership(t *testing.T) {
 
 func TestFullLoginFlowRefusesNoMembership(t *testing.T) {
 	idp := newTestIdP(t)
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
 
 	idp.setNextIDToken(t, "user-2", "[email protected]", true, time.Now().Add(time.Hour))
 	rec := fullLoginFlow(t, h, idp)
@@ -237,7 +237,7 @@ func TestFullLoginFlowRefusesMultipleMemberships(t *testing.T) {
 		{TenantID: "acme", UserID: "user-user-3", Role: rbacstore.RoleViewer},
 		{TenantID: "globex", UserID: "user-user-3", Role: rbacstore.RoleAdmin},
 	}
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), store, "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), store, "http://web/")
 
 	idp.setNextIDToken(t, "user-3", "[email protected]", true, time.Now().Add(time.Hour))
 	rec := fullLoginFlow(t, h, idp)
@@ -249,12 +249,12 @@ func TestFullLoginFlowRefusesMultipleMemberships(t *testing.T) {
 
 func TestCallbackRejectsStateMismatch(t *testing.T) {
 	idp := newTestIdP(t)
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
 	mux := http.NewServeMux()
 	h.RegisterRoutes(mux)
 
 	req := httptest.NewRequest(http.MethodGet, "/auth/oidc/callback?state=wrong&code=test-code", nil)
-	req.AddCookie(&http.Cookie{Name: stateCookieName, Value: "correct"})
+	req.AddCookie(&http.Cookie{Name: oidcStateCookieName, Value: "correct"})
 	rec := httptest.NewRecorder()
 	mux.ServeHTTP(rec, req)
 
@@ -265,7 +265,7 @@ func TestCallbackRejectsStateMismatch(t *testing.T) {
 
 func TestCallbackRejectsMissingStateCookie(t *testing.T) {
 	idp := newTestIdP(t)
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
 	mux := http.NewServeMux()
 	h.RegisterRoutes(mux)
 
@@ -279,7 +279,7 @@ func TestCallbackRejectsMissingStateCookie(t *testing.T) {
 
 func TestCallbackRejectsExpiredIDToken(t *testing.T) {
 	idp := newTestIdP(t)
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
 
 	idp.setNextIDToken(t, "user-4", "[email protected]", true, time.Now().Add(-time.Hour)) // already expired
 	rec := fullLoginFlow(t, h, idp)
@@ -290,7 +290,7 @@ func TestCallbackRejectsExpiredIDToken(t *testing.T) {
 }
 
 func TestRegisterRoutesNoOpWhenOIDCNotConfigured(t *testing.T) {
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
 	mux := http.NewServeMux()
 	h.RegisterRoutes(mux)
 
@@ -314,7 +314,7 @@ func TestRegisterRoutesNoOpWhenOIDCNotConfigured(t *testing.T) {
 // the trap, only passing a nil-valued typed variable does.
 func TestRegisterRoutesNoOpWithTypedNilProviderVariable(t *testing.T) {
 	var provider *oidc.Provider // stays nil -- exactly main.go's shape when OIDC_ISSUER_URL is unset
-	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), provider, newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), provider, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
 	mux := http.NewServeMux()
 	h.RegisterRoutes(mux)
 

enterprise/internal/loginhandler/saml_test.go

@@ -0,0 +1,423 @@
+// Mirrors loginhandler_test.go's OIDC approach: exercise the full SAML
+// login flow against a real fake IdP rather than mocking anything.
+// crewjam/saml ships samlidp, a genuine SAML identity provider (real XML
+// signing, real assertion construction) meant for exactly this kind of
+// testing. To avoid driving its HTML login form, a valid saml.Session is
+// seeded directly into the IdP's session store and presented via the
+// `session` cookie GetSession already accepts -- confirmed by reading
+// samlidp's own GetSession implementation, the same "skip the UI, keep
+// the crypto real" shortcut oidctest gives the OIDC tests above.
+package loginhandler
+
+import (
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/tls"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/xml"
+	"fmt"
+	"html"
+	"io"
+	"log/slog"
+	"math/big"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"regexp"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/crewjam/saml"
+	"github.com/crewjam/saml/samlidp"
+
+	"github.com/sentry/sentry/enterprise/internal/rbacstore"
+	samlpkg "github.com/sentry/sentry/enterprise/internal/saml"
+)
+
+const (
+	testSAMLEntityID = "https://sentry-test.example.com/saml/metadata"
+	testSAMLACSURL   = "https://sentry-test.example.com/auth/saml/acs"
+)
+
+// testSAMLIdP bundles a real samlidp.Server with the SP key/cert it was
+// registered against, enough to drive a full SP-initiated login.
+type testSAMLIdP struct {
+	server *samlidp.Server
+	store  *samlidp.MemoryStore
+	spKey  *rsa.PrivateKey
+	spCert *x509.Certificate
+}
+
+func genSelfSignedCert(t *testing.T, commonName string) (*rsa.PrivateKey, *x509.Certificate) {
+	t.Helper()
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("generating RSA key: %v", err)
+	}
+	serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128))
+	if err != nil {
+		t.Fatalf("generating serial number: %v", err)
+	}
+	template := x509.Certificate{
+		SerialNumber:          serial,
+		Subject:               pkix.Name{CommonName: commonName},
+		NotBefore:             time.Now().Add(-time.Hour),
+		NotAfter:              time.Now().Add(24 * time.Hour),
+		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
+		BasicConstraintsValid: true,
+	}
+	der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key)
+	if err != nil {
+		t.Fatalf("creating certificate: %v", err)
+	}
+	cert, err := x509.ParseCertificate(der)
+	if err != nil {
+		t.Fatalf("parsing certificate: %v", err)
+	}
+	return key, cert
+}
+
+// newTestSAMLIdP starts a real samlidp.Server and registers Sentry's SP
+// metadata with it directly via the IdP's own PUT /services/{id}
+// endpoint -- the same mechanism a real IdP admin uses, not a shortcut
+// that reaches into samlidp's unexported state.
+func newTestSAMLIdP(t *testing.T) *testSAMLIdP {
+	t.Helper()
+	idpKey, idpCert := genSelfSignedCert(t, "sentry-test-idp")
+	spKey, spCert := genSelfSignedCert(t, "sentry-test-sp")
+
+	store := &samlidp.MemoryStore{}
+	idpServer, err := samlidp.New(samlidp.Options{
+		Key:         idpKey,
+		Certificate: idpCert,
+		Store:       store,
+		URL:         url.URL{Scheme: "http", Host: "idp.example.com"},
+	})
+	if err != nil {
+		t.Fatalf("samlidp.New: %v", err)
+	}
+
+	entityIDURL, err := url.Parse(testSAMLEntityID)
+	if err != nil {
+		t.Fatalf("parsing test entity id: %v", err)
+	}
+	acsURL, err := url.Parse(testSAMLACSURL)
+	if err != nil {
+		t.Fatalf("parsing test acs url: %v", err)
+	}
+	// A throwaway saml.ServiceProvider built from the same
+	// EntityID/ACSURL/cert samlpkg.New below uses -- Metadata() is a
+	// pure function of those exported fields, so this stays consistent
+	// with the real samlpkg.ServiceProvider without needing access to
+	// its unexported inner sp field.
+	spForRegistration := saml.ServiceProvider{
+		Key:         spKey,
+		Certificate: spCert,
+		MetadataURL: *entityIDURL,
+		AcsURL:      *acsURL,
+	}
+	spMetadataXML, err := xml.Marshal(spForRegistration.Metadata())
+	if err != nil {
+		t.Fatalf("marshaling sp metadata: %v", err)
+	}
+	putReq := httptest.NewRequest(http.MethodPut, "/services/sentry-test-sp", strings.NewReader(string(spMetadataXML)))
+	putRec := httptest.NewRecorder()
+	idpServer.ServeHTTP(putRec, putReq)
+	if putRec.Code != http.StatusNoContent {
+		t.Fatalf("registering sp metadata with fake idp: status = %d, body = %s", putRec.Code, putRec.Body.String())
+	}
+
+	return &testSAMLIdP{server: idpServer, store: store, spKey: spKey, spCert: spCert}
+}
+
+// serviceProvider builds the samlpkg.ServiceProvider loginhandler uses,
+// trusting idp's metadata.
+func (idp *testSAMLIdP) serviceProvider(t *testing.T) *samlpkg.ServiceProvider {
+	t.Helper()
+	sp, err := samlpkg.New(samlpkg.Config{
+		EntityID:    testSAMLEntityID,
+		ACSURL:      testSAMLACSURL,
+		IDPMetadata: idp.server.IDP.Metadata(),
+		Certificate: &tls.Certificate{
+			Certificate: [][]byte{idp.spCert.Raw},
+			PrivateKey:  idp.spKey,
+		},
+	})
+	if err != nil {
+		t.Fatalf("samlpkg.New: %v", err)
+	}
+	return sp
+}
+
+// seedSession pre-authenticates a user directly in the fake IdP's store,
+// bypassing its login-form HTML entirely. Confirmed viable by reading
+// samlidp's GetSession: a valid, non-expired saml.Session at
+// /sessions/<id> plus a matching `session` cookie is exactly what a real
+// login-form POST would have produced -- this is the IdP's own supported
+// shortcut, not an abuse of internals.
+func (idp *testSAMLIdP) seedSession(t *testing.T, nameID, email string) *http.Cookie {
+	t.Helper()
+	sessionID := fmt.Sprintf("test-session-%d", time.Now().UnixNano())
+	session := &saml.Session{
+		ID:         sessionID,
+		NameID:     nameID,
+		CreateTime: saml.TimeNow(),
+		ExpireTime: saml.TimeNow().Add(time.Hour),
+		Index:      sessionID,
+		UserEmail:  email,
+	}
+	if err := idp.store.Put(fmt.Sprintf("/sessions/%s", sessionID), session); err != nil {
+		t.Fatalf("seeding idp session: %v", err)
+	}
+	return &http.Cookie{Name: "session", Value: sessionID}
+}
+
+var samlResponseFieldRe = regexp.MustCompile(`name="(SAMLResponse|RelayState)" value="([^"]*)"`)
+
+// extractSAMLResponseForm pulls the hidden form fields out of the IdP's
+// auto-submitting HTML response -- what a real browser's inline <script>
+// reads before POSTing to the SP's ACS endpoint.
+func extractSAMLResponseForm(t *testing.T, body string) (samlResponse, relayState string) {
+	t.Helper()
+	for _, m := range samlResponseFieldRe.FindAllStringSubmatch(body, -1) {
+		switch m[1] {
+		case "SAMLResponse":
+			samlResponse = html.UnescapeString(m[2])
+		case "RelayState":
+			relayState = html.UnescapeString(m[2])
+		}
+	}
+	if samlResponse == "" {
+		t.Fatalf("no SAMLResponse field found in idp response html: %s", body)
+	}
+	return samlResponse, relayState
+}
+
+// fullSAMLLoginFlow drives handleSAMLLogin, the fake IdP's /sso, and
+// handleSAMLACS end to end, exactly the way a browser + IdP round trip
+// would, and returns the final response so callers can assert on it.
+func fullSAMLLoginFlow(t *testing.T, h *Handler, idp *testSAMLIdP, nameID, email string) *httptest.ResponseRecorder {
+	t.Helper()
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	loginRec := httptest.NewRecorder()
+	mux.ServeHTTP(loginRec, httptest.NewRequest(http.MethodGet, "/auth/saml/login", nil))
+	if loginRec.Code != http.StatusFound {
+		t.Fatalf("GET /auth/saml/login: status = %d, body = %s", loginRec.Code, loginRec.Body.String())
+	}
+	redirectURL := loginRec.Header().Get("Location")
+	var requestCookie *http.Cookie
+	for _, c := range loginRec.Result().Cookies() {
+		if c.Name == samlRequestCookieName {
+			requestCookie = c
+		}
+	}
+	if requestCookie == nil {
+		t.Fatal("no saml request cookie from /auth/saml/login")
+	}
+
+	ssoReq := httptest.NewRequest(http.MethodGet, redirectURL, nil)
+	ssoReq.AddCookie(idp.seedSession(t, nameID, email))
+	ssoRec := httptest.NewRecorder()
+	idp.server.ServeHTTP(ssoRec, ssoReq)
+	if ssoRec.Code != http.StatusOK {
+		t.Fatalf("idp GET /sso: status = %d, body = %s", ssoRec.Code, ssoRec.Body.String())
+	}
+	samlResponse, relayState := extractSAMLResponseForm(t, ssoRec.Body.String())
+
+	form := url.Values{"SAMLResponse": {samlResponse}, "RelayState": {relayState}}
+	acsReq := httptest.NewRequest(http.MethodPost, "/auth/saml/acs", strings.NewReader(form.Encode()))
+	acsReq.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	acsReq.AddCookie(requestCookie)
+	acsRec := httptest.NewRecorder()
+	mux.ServeHTTP(acsRec, acsReq)
+	return acsRec
+}
+
+func TestHandleSAMLLoginRedirectsAndSetsRequestCookie(t *testing.T) {
+	idp := newTestSAMLIdP(t)
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/saml/login", nil))
+
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, want 302", rec.Code)
+	}
+	if loc := rec.Header().Get("Location"); loc == "" {
+		t.Fatal("expected a Location header redirecting to the IdP")
+	}
+	var requestCookie *http.Cookie
+	for _, c := range rec.Result().Cookies() {
+		if c.Name == samlRequestCookieName {
+			requestCookie = c
+		}
+	}
+	if requestCookie == nil || requestCookie.Value == "" {
+		t.Fatal("expected a non-empty saml request cookie to be set")
+	}
+	if !requestCookie.HttpOnly {
+		t.Fatal("expected the saml request cookie to be HttpOnly")
+	}
+	if requestCookie.SameSite != http.SameSiteNoneMode {
+		t.Fatalf("SameSite = %v, want SameSiteNoneMode -- the acs POST is cross-site from the idp's origin", requestCookie.SameSite)
+	}
+}
+
+func TestFullSAMLLoginFlowIssuesSessionForSingleMembership(t *testing.T) {
+	idp := newTestSAMLIdP(t)
+	store := newFakeUserStore()
+	store.memberships["user-saml-user-1"] = []rbacstore.Membership{{TenantID: "acme", UserID: "user-saml-user-1", Role: rbacstore.RoleEditor}}
+	sessionManager := newTestSessionManager(t)
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), sessionManager, store, "http://web/")
+
+	rec := fullSAMLLoginFlow(t, h, idp, "saml-user-1", "[email protected]")
+
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, want 302; body=%s", rec.Code, rec.Body.String())
+	}
+	if loc := rec.Header().Get("Location"); loc != "http://web/" {
+		t.Fatalf("Location = %q, want http://web/", loc)
+	}
+
+	var sessionCookie *http.Cookie
+	for _, c := range rec.Result().Cookies() {
+		if c.Name == "sentry_session" {
+			sessionCookie = c
+		}
+	}
+	if sessionCookie == nil || sessionCookie.Value == "" {
+		t.Fatal("expected a sentry_session cookie to be set")
+	}
+	claims, err := sessionManager.Validate(sessionCookie.Value)
+	if err != nil {
+		t.Fatalf("validating issued session: %v", err)
+	}
+	if claims.TenantID != "acme" || claims.Role != "editor" || claims.UserID != "user-saml-user-1" {
+		t.Fatalf("unexpected session claims: %+v", claims)
+	}
+}
+
+func TestFullSAMLLoginFlowRefusesNoMembership(t *testing.T) {
+	idp := newTestSAMLIdP(t)
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+
+	rec := fullSAMLLoginFlow(t, h, idp, "saml-user-2", "[email protected]")
+
+	if rec.Code != http.StatusForbidden {
+		t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String())
+	}
+}
+
+func TestFullSAMLLoginFlowRefusesMultipleMemberships(t *testing.T) {
+	idp := newTestSAMLIdP(t)
+	store := newFakeUserStore()
+	store.memberships["user-saml-user-3"] = []rbacstore.Membership{
+		{TenantID: "acme", UserID: "user-saml-user-3", Role: rbacstore.RoleViewer},
+		{TenantID: "globex", UserID: "user-saml-user-3", Role: rbacstore.RoleAdmin},
+	}
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), store, "http://web/")
+
+	rec := fullSAMLLoginFlow(t, h, idp, "saml-user-3", "[email protected]")
+
+	if rec.Code != http.StatusNotImplemented {
+		t.Fatalf("status = %d, want 501; body=%s", rec.Code, rec.Body.String())
+	}
+}
+
+func TestFullSAMLLoginFlowRefusesMissingEmail(t *testing.T) {
+	idp := newTestSAMLIdP(t)
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+
+	rec := fullSAMLLoginFlow(t, h, idp, "saml-user-4", "") // no email attribute in the assertion
+
+	if rec.Code != http.StatusUnauthorized {
+		t.Fatalf("status = %d, want 401; body=%s", rec.Code, rec.Body.String())
+	}
+}
+
+func TestSAMLACSRejectsMissingRequestCookie(t *testing.T) {
+	idp := newTestSAMLIdP(t)
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	// No prior GET /auth/saml/login, so no sentry_saml_request cookie --
+	// simulates an attacker POSTing a captured/forged response directly
+	// at the ACS endpoint with no matching request state.
+	form := url.Values{"SAMLResponse": {"irrelevant"}, "RelayState": {""}}
+	req := httptest.NewRequest(http.MethodPost, "/auth/saml/acs", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400", rec.Code)
+	}
+}
+
+func TestSAMLACSRejectsWrongRequestID(t *testing.T) {
+	idp := newTestSAMLIdP(t)
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	loginRec := httptest.NewRecorder()
+	mux.ServeHTTP(loginRec, httptest.NewRequest(http.MethodGet, "/auth/saml/login", nil))
+	redirectURL := loginRec.Header().Get("Location")
+
+	ssoReq := httptest.NewRequest(http.MethodGet, redirectURL, nil)
+	ssoReq.AddCookie(idp.seedSession(t, "saml-user-5", "[email protected]"))
+	ssoRec := httptest.NewRecorder()
+	idp.server.ServeHTTP(ssoRec, ssoReq)
+	samlResponse, relayState := extractSAMLResponseForm(t, ssoRec.Body.String())
+
+	// Present the genuine, correctly-signed response but with a
+	// tampered request cookie -- the InResponseTo check must still
+	// reject it. This is SAML's replay/unsolicited-response defense,
+	// the mechanism samlRequestCookieName exists for.
+	form := url.Values{"SAMLResponse": {samlResponse}, "RelayState": {relayState}}
+	acsReq := httptest.NewRequest(http.MethodPost, "/auth/saml/acs", strings.NewReader(form.Encode()))
+	acsReq.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	acsReq.AddCookie(&http.Cookie{Name: samlRequestCookieName, Value: "some-other-request-id"})
+	acsRec := httptest.NewRecorder()
+	mux.ServeHTTP(acsRec, acsReq)
+
+	if acsRec.Code != http.StatusUnauthorized {
+		t.Fatalf("status = %d, want 401; body=%s", acsRec.Code, acsRec.Body.String())
+	}
+}
+
+func TestRegisterRoutesNoOpWhenSAMLNotConfigured(t *testing.T) {
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/saml/login", nil))
+	if rec.Code != http.StatusNotFound {
+		t.Fatalf("status = %d, want 404 (no routes should be registered when saml is nil)", rec.Code)
+	}
+}
+
+// TestRegisterRoutesNoOpWithTypedNilSAMLProviderVariable is SAML's
+// equivalent of TestRegisterRoutesNoOpWithTypedNilProviderVariable in
+// loginhandler_test.go -- see that test's doc comment for the Go
+// typed-nil-interface trap this guards against.
+func TestRegisterRoutesNoOpWithTypedNilSAMLProviderVariable(t *testing.T) {
+	var provider *samlpkg.ServiceProvider // stays nil -- main.go's shape when SAML_IDP_METADATA_URL is unset
+	h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, provider, newTestSessionManager(t), newFakeUserStore(), "http://web/")
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/saml/login", nil))
+	if rec.Code != http.StatusNotFound {
+		t.Fatalf("status = %d, want 404 (a typed-nil *samlpkg.ServiceProvider must still result in saml routes being disabled)", rec.Code)
+	}
+}

enterprise/internal/saml/saml.go

@@ -89,17 +89,24 @@ func New(cfg Config) (*ServiceProvider, error) {
 // round-trips through the IdP and comes back with the response --
 // typically where to send the browser after login completes, validated
 // by the caller the same way OIDC's state parameter is (this package
-// doesn't store it).
-func (s *ServiceProvider) LoginURL(relayState string) (string, error) {
+// doesn't store it). Also returns the AuthnRequest's ID: the caller must
+// persist it (e.g. a short-lived cookie, the same pattern
+// enterprise/internal/loginhandler uses for OIDC's state) and pass it
+// back via ParseResponse's possibleRequestIDs -- SAML's actual replay/
+// unsolicited-response defense, standing in for OIDC's simpler state
+// check. Skipping this (e.g. passing nil to ParseResponse) is exactly
+// the "don't validate you asked for this response" mistake that would
+// let an attacker replay a captured assertion.
+func (s *ServiceProvider) LoginURL(relayState string) (redirectURL, requestID string, err error) {
 	req, err := s.sp.MakeAuthenticationRequest(s.sp.GetSSOBindingLocation(saml.HTTPRedirectBinding), saml.HTTPRedirectBinding, saml.HTTPPostBinding)
 	if err != nil {
-		return "", fmt.Errorf("saml: building authentication request: %w", err)
+		return "", "", fmt.Errorf("saml: building authentication request: %w", err)
 	}
-	redirectURL, err := req.Redirect(relayState, &s.sp)
+	redirect, err := req.Redirect(relayState, &s.sp)
 	if err != nil {
-		return "", fmt.Errorf("saml: building redirect URL: %w", err)
+		return "", "", fmt.Errorf("saml: building redirect URL: %w", err)
 	}
-	return redirectURL.String(), nil
+	return redirect.String(), req.ID, nil
 }
 
 // Claims is the subset of an assertion Sentry uses -- same "extend
@@ -114,6 +121,17 @@ type Claims struct {
 // the step that actually establishes trust -- crewjam/saml's
 // ParseResponse does the XML signature verification, not this package.
 func (s *ServiceProvider) ParseResponse(r *http.Request, possibleRequestIDs []string) (*Claims, error) {
+	// crewjam/saml's ServiceProvider.ParseResponse reads req.PostForm
+	// directly rather than parsing the body itself -- net/http only
+	// populates PostForm once something calls ParseForm, which the
+	// stdlib server never does on its own. Skipping this turns every
+	// real ACS POST into an empty SAMLResponse (silently failing at the
+	// base64-decode step), a bug this package's own real-fake-IdP test
+	// caught immediately since it drives a genuine POST body.
+	if err := r.ParseForm(); err != nil {
+		return nil, fmt.Errorf("saml: parsing ACS POST body: %w", err)
+	}
+
 	assertion, err := s.sp.ParseResponse(r, possibleRequestIDs)
 	if err != nil {
 		return nil, fmt.Errorf("saml: parsing/validating response: %w", err)
@@ -125,7 +143,16 @@ func (s *ServiceProvider) ParseResponse(r *http.Request, possibleRequestIDs []st
 	}
 	for _, stmt := range assertion.AttributeStatements {
 		for _, attr := range stmt.Attributes {
-			if attr.Name == "email" || attr.Name == "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress" {
+			switch attr.Name {
+			// "email" and the ADFS claims URI are what an IdP admin gets
+			// by explicitly naming the attribute that way in the SAML
+			// app's attribute-statement config. urn:oid:0.9.2342.19200300.100.1.3
+			// is the standard LDAP "mail" OID -- what crewjam's own
+			// DefaultAssertionMaker (and many real IdPs' default
+			// templates) send when nothing more specific was requested,
+			// found by tracing the actual assertion-building code this
+			// test exercises rather than assuming "email" covers it.
+			case "email", "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress", "urn:oid:0.9.2342.19200300.100.1.3":
 				if len(attr.Values) > 0 {
 					claims.Email = attr.Values[0].Value
 				}

enterprise/internal/saml/saml_test.go

@@ -48,11 +48,14 @@ func TestLoginURLBuildsAgainstRealIDPMetadata(t *testing.T) {
 		t.Fatalf("New: %v", err)
 	}
 
-	redirectURL, err := sp.LoginURL("relay-state-123")
+	redirectURL, requestID, err := sp.LoginURL("relay-state-123")
 	if err != nil {
 		t.Fatalf("LoginURL: %v", err)
 	}
 	if redirectURL == "" {
 		t.Fatalf("expected a non-empty redirect URL")
 	}
+	if requestID == "" {
+		t.Fatalf("expected a non-empty AuthnRequest ID -- callers need this for ParseResponse's possibleRequestIDs")
+	}
 }