diff --git a/CLAUDE.md b/CLAUDE.md index 5e31526..9a5e06c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -221,11 +221,21 @@ caller of ClickHouse/`rbacstore`, and (via a new its real result into the CRD — a real credential Secret, not the previous placeholder that authenticated against nothing, and status fields the reconciler derives `Phase`/`Ready` from instead of -independently guessing "Active" the moment a Tenant object exists. What -still keeps this phase from being done: ingest itself has no tenant -concept for either storage engine (every record lands in the one shared -ClickHouse database and Tantivy index no matter what — undesigned, not -just unbuilt). Full accounting: +independently guessing "Active" the moment a Tenant object exists. The +tenant-picker's backend protocol is built too: an identity with more +than one `tenant_memberships` row now gets a real `GET +/auth/memberships`/`POST /auth/select-tenant` round trip (a short-lived +pending-login token, distinct from a real session by both Go type and +JWT claim name — a real token-confusion bug this design's own tests +caught before it shipped) instead of the flat refusal Phase 4 shipped +with earlier. What still keeps this phase from being done: the actual +tenant-picker *page* doesn't exist (`web` has no session/cookie-handling +code at all yet, and `enterprise-auth` has no CORS middleware for a +cross-origin `fetch` with credentials — both real, separately-scoped +frontend gaps), and ingest itself has no tenant concept for either +storage engine (every record lands in the one shared ClickHouse database +and Tantivy index no matter what — undesigned, not just unbuilt). Full +accounting: `/docs/security/threat-model.md`; step-by-step verification procedure (not yet run against a live cluster in this environment): `/docs/phase-4-runbook.md`. The rest of this section describes the exit diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index 3e48e26..fe7d2b2 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -521,6 +521,39 @@ the full loop (does the operator's watch actually re-trigger a reconcile after `-provision-tenant`'s external status write the way controller- runtime's default predicate is expected to). +## 12. Tenant-picker backend protocol (no frontend yet, no Docker needed) + +Like §9, this needs nothing but a local Go toolchain -- the real +fake-IdP tests already exercise the full login → pending-login cookie → +`GET /auth/memberships` → `POST /auth/select-tenant` → real session +round trip: + +```sh +cd enterprise +go test ./internal/session/... -run PendingLogin -v +# real JWT signing/verification: issues a pending-login token, validates +# it, and proves the two negative regressions that matter most -- +# a real session token must not validate as a pending login (they share +# a signing key but PendingLoginClaims uses a disjoint json field name, +# see that type's doc comment for the bug this caught in its own tests), +# and a pending-login token must not validate as a real session either. + +go test ./internal/loginhandler/... -run 'Memberships|SelectTenant|MultipleMemberships' -v +# full round trip against the real fake OIDC/SAML IdPs: multi-membership +# login sets a pending cookie and redirects (not a 501 anymore), GET +# /auth/memberships lists the real tenant options with display names, +# POST /auth/select-tenant re-derives role server-side and refuses a +# tenant_id outside the identity's actual memberships. +``` + +**Not built, and explicitly not attempted here**: the frontend page. +`web` has no session/cookie-handling code anywhere in it today (checked +while designing this), and `enterprise-auth` has no CORS middleware at +all -- a cross-origin `fetch` with credentials from `web`'s origin to +`enterprise-auth`'s would need it, and doesn't work today. Building the +actual picker UI is real, separately-scoped frontend work; this section +only closes the backend half. + ## Known gaps (do not treat this phase as done without reading these) Full accounting: `/docs/security/threat-model.md`. Headline items: @@ -553,8 +586,13 @@ Full accounting: `/docs/security/threat-model.md`. Headline items: - **Human SSO login now works for both OIDC (§3a) and SAML (§3b)** -- each verified with a real fake IdP (genuine cryptographic signing and verification), not yet a real external IdP or a running - `enterprise-auth` container. No tenant-picker UI for a multi-membership - identity either (refused outright) for either protocol. + `enterprise-auth` container. **The tenant-picker backend protocol is + now built too** (§12) -- `GET /auth/memberships`/ + `POST /auth/select-tenant`, backed by a short-lived pending-login + token distinct from a real session -- but nothing in `web` calls it + yet, so a multi-membership identity still can't actually finish + logging in through a browser today, just through direct HTTP calls + (which is what §12's verification does). - No admin UI to create a `tenant_memberships` row, but §3a/§3b's manual SQL bootstrap is gone -- `enterprise-auth -create-tenant`/ `-grant-membership-*`/`-revoke-membership-*`/`-list-memberships-tenant` diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index c613df3..c811143 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -176,11 +176,23 @@ container against a *real* external IdP (Google/Okta/etc.) — that needs real IdP credentials and a reachable callback/ACS URL neither of which this environment has; see `/docs/phase-4-runbook.md`. -A user with zero or more than one `tenant_memberships` row is refused -outright (403 / 501 respectively) rather than guessed at — a -tenant-selection UI for the multi-membership case is real, undesigned -future work, not silently approximated, for either protocol. -`GET /auth/features` (`enterprise/internal/authhandler`) reports whether +A user with zero `tenant_memberships` rows is refused outright (403). +More than one no longer guesses or refuses: `finishLogin` issues a +short-lived `session.Manager` "pending login" token (a distinct Go/JWT +type from a real session, with its own disjoint claim name so a real +session token can't double as one — a real bug this design's own test +suite caught before it shipped, see `session.PendingLoginClaims`'s doc +comment) and redirects to a not-yet-served URL instead, backed by two +new endpoints (`GET /auth/memberships`, `POST /auth/select-tenant`) that +list the identity's real tenant options and, on selection, re-derive the +role for the chosen tenant server-side (never trusting a client-supplied +role) before issuing the real session. This is the *backend protocol* +for tenant selection, verified with the same real-fake-IdP tests as the +rest of `internal/loginhandler` — the frontend page that would call it +doesn't exist (`web` has no session/cookie-handling code at all today, +and `enterprise-auth` has no CORS middleware for a cross-origin `fetch` +with credentials to work), both real, separately-scoped gaps, not +silently approximated. `GET /auth/features` (`enterprise/internal/authhandler`) reports whether OIDC/SAML are *configured*, for `/web`'s settings page to conditionally render — independent of whether a login button actually exists yet in the UI (it doesn't; only the HTTP endpoints do). @@ -413,7 +425,7 @@ terms: | Deployment actually routing traffic to `enterprise-api` (docker-compose) | **Enforced** — `api`/`enterprise-api` are mutually exclusive via `COMPOSE_PROFILES`, same flag choice as Helm's `enterprise.enabled`; verified via `docker compose config`, not an actual `docker compose up` in this environment | | Human SSO login — OIDC | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | | Human SSO login — SAML | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | -| Multi-tenant-membership login (tenant picker) | **Not implemented** — refused with a clear error, not guessed | +| Multi-tenant-membership login (tenant picker) | **Backend protocol built and verified** (`GET /auth/memberships`, `POST /auth/select-tenant`, a pending-login token distinct from a real session) — no frontend page calls it yet | | Per-resource dashboard grants (`own/granted`) | **Built, unit-tested against a fake store; live-Postgres integration tests written, not run in this environment** (only when `enterprise-api` serves traffic — plain `api` falls back to own/Admin only) | | Query audit logging (routine queries) | **Enforced**, fail-open, and now wired to a real writer via `enterprise-api` (`audit.QueryAPILogger`) | | Audit log tamper detection (hash chain) | **Enforced**, verified live | diff --git a/enterprise/README.md b/enterprise/README.md index f48806d..48d6fb2 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -55,9 +55,12 @@ section for exactly what "not yet run" means here and why. Don't read -- the actual human login flow, previously entirely missing. Redirects to the configured IdP with CSRF-protection state in a short-lived cookie, exchanges the code, verifies the ID token via `internal/oidc`, - upserts a `users` row, resolves tenant/role from exactly one - `tenant_memberships` row (refuses with a clear error on zero or - multiple -- no tenant-picker UI yet), and issues a session cookie. + upserts a `users` row, resolves tenant/role from `tenant_memberships` + (refuses with a clear error on zero rows; more than one starts a real + tenant-selection round trip -- `GET /auth/memberships` + + `POST /auth/select-tenant`, backed by a short-lived + `session.Manager.IssuePendingLogin` token -- rather than guessing; see + "Tenant selection" below), and issues a session cookie. **This one genuinely is verified**, unlike the ClickHouse pieces above: `loginhandler_test.go` runs the full flow against a real fake IdP (`coreos/go-oidc`'s own `oidctest` package, real RS256 signing and @@ -134,11 +137,51 @@ manage-grants regression); real integration tests exist in environment, same disclosed gap as the rest of this package's Postgres-backed pieces. +## Tenant selection (multi-membership identities) + +The backend protocol for choosing a tenant is built and tested; the +frontend page that would actually call it is deliberately not (see +"Deliberately deferred" below). When `resolveIdentity` finds more than +one `tenant_memberships` row for a logged-in identity, `finishLogin` +issues a `session.Manager.IssuePendingLogin` token (a distinct Go/JWT +type from a real session -- see that type's doc comment for a real bug +this design caught in its own tests: a shared JSON key would have let a +full session token double as a pending login) as a +`sentry_pending_login` cookie (`Path=/auth`) and redirects to +`SELECT_TENANT_REDIRECT_URL` (defaults to +`{POST_LOGIN_REDIRECT_URL}/select-tenant`) instead of completing the +login. From there: + +- `GET /auth/memberships` -- lists the pending identity's tenants + (`tenant_id`, `tenant_display_name`, `role`) to choose between. +- `POST /auth/select-tenant` with `{"tenant_id": "..."}` -- re-derives + the role for that specific tenant server-side (never trusts a + client-supplied role, only checks the claimed `tenant_id` against the + identity's actual memberships, refusing with 403 otherwise), issues + the real session cookie, and responds with + `{"redirect_url": "..."}` for the caller to navigate to -- JSON, not a + redirect, since this is a POST/fetch call a frontend page should + control the navigation for itself. + +Verified with the same real-fake-IdP tests as the rest of this package +(`loginhandler_test.go`/`saml_test.go`): the full login -> pending +cookie -> `GET /auth/memberships` -> `POST /auth/select-tenant` -> real +session round trip, plus negative paths (missing/expired/wrong-type +pending cookie, a `tenant_id` outside the identity's actual +memberships, a real session token rejected when presented as a pending +login). + **Deliberately deferred, not half-built** -- named explicitly rather than silently left out: -- A tenant-picker UI/flow for an identity with more than one - `tenant_memberships` row -- `loginhandler` refuses these logins - outright rather than guessing (`ErrMultipleMemberships`). +- **The actual tenant-picker page** -- nothing in `web/` calls the + endpoints above yet. Building it is genuinely different, larger scope + than the backend protocol: `web` has zero session/cookie-handling + code today (confirmed by reading it end to end while designing this), + so a real picker page means adding that from scratch, plus CORS + wiring (`enterprise-auth` has no CORS middleware at all right now -- + a cross-origin `fetch` with credentials from `web`'s origin needs + it), neither of which is verifiable in this environment without a + live backend and a browser session to exercise. - **Ingest tenant-awareness, for either storage engine** -- `chrunner`/ `searchclient` prove read isolation given tenant-scoped data exists, but nothing writes it: every record `ingest` produces still lands in @@ -344,6 +387,7 @@ to, see `tenantprovision.ProvisionClickHouse`'s doc comment). | `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 | +| `SELECT_TENANT_REDIRECT_URL` | `{POST_LOGIN_REDIRECT_URL}/select-tenant` — where the browser lands for a multi-membership identity instead; nothing serves this route yet, see "Tenant selection" above | ## Environment variables (`enterprise-api`) diff --git a/enterprise/cmd/enterprise-auth/main.go b/enterprise/cmd/enterprise-auth/main.go index 47c722a..52a4dbb 100644 --- a/enterprise/cmd/enterprise-auth/main.go +++ b/enterprise/cmd/enterprise-auth/main.go @@ -188,7 +188,7 @@ func main() { SAMLEnabled: cfg.SAML.IDPMetadataURL != "", } authhandler.New(logger, sessionManager, features).RegisterRoutes(mux) - loginhandler.New(logger, oidcProvider, samlProvider, sessionManager, rbac, cfg.PostLoginRedirectURL).RegisterRoutes(mux) + loginhandler.New(logger, oidcProvider, samlProvider, sessionManager, rbac, cfg.PostLoginRedirectURL, cfg.SelectTenantRedirectURL).RegisterRoutes(mux) srv := &http.Server{Addr: cfg.HTTPListenAddr, Handler: mux} diff --git a/enterprise/internal/config/config.go b/enterprise/internal/config/config.go index 0e1cd5c..4d0c3d2 100644 --- a/enterprise/internal/config/config.go +++ b/enterprise/internal/config/config.go @@ -17,6 +17,16 @@ type Config struct { // internal/loginhandler sets a session cookie -- web's base URL in // a real deployment. PostLoginRedirectURL string + // SelectTenantRedirectURL is where the browser lands after a login + // resolves to more than one tenant_memberships row -- + // internal/loginhandler issues a pending-login cookie and sends the + // browser here instead of straight to PostLoginRedirectURL. Nothing + // serves this route yet (a real tenant-picker page is undesigned + // frontend work -- see internal/loginhandler's package doc comment); + // the backend protocol (GET /auth/memberships, POST + // /auth/select-tenant) is complete and independently testable via + // HTTP regardless of what, if anything, is listening here today. + SelectTenantRedirectURL string } type PostgresConfig struct { @@ -70,6 +80,12 @@ func Load() (Config, error) { }, PostLoginRedirectURL: getenv("POST_LOGIN_REDIRECT_URL", "http://localhost:3000"), } + // Defaults relative to PostLoginRedirectURL if not set explicitly -- + // computed after cfg.PostLoginRedirectURL above so a caller + // overriding just POST_LOGIN_REDIRECT_URL still gets a sensible + // SelectTenantRedirectURL without also having to set the new + // variable. + cfg.SelectTenantRedirectURL = getenv("SELECT_TENANT_REDIRECT_URL", cfg.PostLoginRedirectURL+"/select-tenant") // Required, unlike OIDC/SAML above: every enterprise-auth deployment // issues and validates session/service tokens (internal/session), diff --git a/enterprise/internal/loginhandler/loginhandler.go b/enterprise/internal/loginhandler/loginhandler.go index 3b4da5e..797c4cf 100644 --- a/enterprise/internal/loginhandler/loginhandler.go +++ b/enterprise/internal/loginhandler/loginhandler.go @@ -12,13 +12,20 @@ // gets verified differs. // // 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. +// tenant) get a real tenant-selection step, not a guess: finishLogin +// issues a short-lived session.Manager "pending login" token (proves +// who they are, commits to no tenant yet) and redirects to +// selectTenantRedirectURL instead of issuing a session outright. +// GET /auth/memberships and POST /auth/select-tenant complete the round +// trip. The backend protocol is complete and independently testable via +// HTTP; the frontend page that would actually call it doesn't exist yet +// (a real tenant-picker UI is undesigned, separately-scoped frontend +// work -- see config.SelectTenantRedirectURL's doc comment). package loginhandler import ( "context" + "encoding/json" "errors" "fmt" "log/slog" @@ -45,6 +52,12 @@ const oidcStateCookieName = "sentry_oidc_state" // response defense -- see saml.ServiceProvider.LoginURL's doc comment). const samlRequestCookieName = "sentry_saml_request" +// pendingLoginCookieName carries a PendingLoginClaims token from +// finishLogin's multi-membership branch through GET /auth/memberships +// and POST /auth/select-tenant -- Path "/auth" (not "/") so it's never +// sent on ordinary requests, only the two routes that need it. +const pendingLoginCookieName = "sentry_pending_login" + // 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 cookie isn't a long-lived CSRF token sitting in a browser. @@ -57,6 +70,9 @@ const loginCookieTTL = 10 * time.Minute type userStore interface { UpsertUserBySSO(ctx context.Context, ssoSubject, email, displayName string) (*rbacstore.User, error) ListMembershipsForUser(ctx context.Context, userID string) ([]rbacstore.Membership, error) + // ListMembershipsWithTenantForUser backs GET /auth/memberships -- + // the one caller that needs tenant display names, not just IDs/roles. + ListMembershipsWithTenantForUser(ctx context.Context, userID string) ([]rbacstore.MembershipWithTenant, error) } // oidcProvider is the narrow slice of *oidc.Provider Handler needs -- @@ -83,6 +99,10 @@ type Handler struct { // postLoginRedirectURL is where the browser lands after a session // cookie is set -- web's base URL in a real deployment. postLoginRedirectURL string + // selectTenantRedirectURL is where the browser lands instead, when + // the identity has more than one tenant_memberships row -- see this + // package's doc comment. + selectTenantRedirectURL string } // New takes concrete *oidc.Provider/*saml.ServiceProvider (both @@ -98,8 +118,8 @@ type Handler struct { // 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} +func New(logger *slog.Logger, oidcProvider *oidc.Provider, samlProvider *saml.ServiceProvider, sessionManager *session.Manager, users userStore, postLoginRedirectURL, selectTenantRedirectURL string) *Handler { + h := &Handler{logger: logger, session: sessionManager, users: users, postLoginRedirectURL: postLoginRedirectURL, selectTenantRedirectURL: selectTenantRedirectURL} if oidcProvider != nil { h.oidc = oidcProvider } @@ -122,6 +142,10 @@ func (h *Handler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("GET /auth/saml/login", h.handleSAMLLogin) mux.HandleFunc("POST /auth/saml/acs", h.handleSAMLACS) } + if h.oidc != nil || h.saml != nil { + mux.HandleFunc("GET /auth/memberships", h.handleListMemberships) + mux.HandleFunc("POST /auth/select-tenant", h.handleSelectTenant) + } } func (h *Handler) handleOIDCLogin(w http.ResponseWriter, r *http.Request) { @@ -248,17 +272,30 @@ func (h *Handler) handleSAMLACS(w http.ResponseWriter, r *http.Request) { } // 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. +// (subject, email) pair, still needing tenant/role resolution -- +// 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) + outcome, 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 } - token, err := h.session.IssueUserSession(identity.tenantID, identity.userID, identity.role) + if outcome.ambiguous { + h.startTenantSelection(w, r, outcome.userID) + return + } + h.issueSessionAndRedirect(w, r, outcome.identity.tenantID, outcome.identity.userID, outcome.identity.role) +} + +// issueSessionAndRedirect is finishLogin's single-membership path and +// handleSelectTenant's success path converging on the same "issue a +// real session, set the cookie, send the browser on" logic -- the only +// difference between the two callers is how they got a +// (tenantID, userID, role) tuple to issue a session for. +func (h *Handler) issueSessionAndRedirect(w http.ResponseWriter, r *http.Request, tenantID, userID, role string) { + token, err := h.session.IssueUserSession(tenantID, userID, role) if err != nil { h.logger.Error("issuing session", "error", err) http.Error(w, "internal error", http.StatusInternalServerError) @@ -272,13 +309,142 @@ func (h *Handler) finishLogin(w http.ResponseWriter, r *http.Request, subject, e http.Redirect(w, r, h.postLoginRedirectURL, http.StatusFound) } +// startTenantSelection issues a pending-login token for an identity +// with more than one tenant_memberships row and sends the browser to +// selectTenantRedirectURL instead of completing the login -- the real +// tenant-selection step named as a gap throughout Phase 4's docs, not a +// refusal anymore (see this package's doc comment). +func (h *Handler) startTenantSelection(w http.ResponseWriter, r *http.Request, userID string) { + token, err := h.session.IssuePendingLogin(userID) + if err != nil { + h.logger.Error("issuing pending login", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + http.SetCookie(w, &http.Cookie{ + Name: pendingLoginCookieName, Value: token, Path: "/auth", + HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + MaxAge: int(session.PendingLoginTTL.Seconds()), + }) + http.Redirect(w, r, h.selectTenantRedirectURL, http.StatusFound) +} + +// pendingUserID validates the pending-login cookie GET /auth/memberships +// and POST /auth/select-tenant both require, writing an error response +// and returning ok=false if it's missing, expired, or forged. +func (h *Handler) pendingUserID(w http.ResponseWriter, r *http.Request) (userID string, ok bool) { + cookie, err := r.Cookie(pendingLoginCookieName) + if err != nil || cookie.Value == "" { + http.Error(w, "missing or expired pending login -- start over at /auth/oidc/login or /auth/saml/login", http.StatusBadRequest) + return "", false + } + userID, err = h.session.ValidatePendingLogin(cookie.Value) + if err != nil { + http.Error(w, "missing or expired pending login -- start over at /auth/oidc/login or /auth/saml/login", http.StatusUnauthorized) + return "", false + } + return userID, true +} + +// membershipOption is one GET /auth/memberships response entry. +type membershipOption struct { + TenantID string `json:"tenant_id"` + TenantDisplayName string `json:"tenant_display_name"` + Role string `json:"role"` +} + +// handleListMemberships lists the pending login's tenants to choose +// from -- a tenant-picker UI's first call, once one exists (see this +// package's doc comment). +func (h *Handler) handleListMemberships(w http.ResponseWriter, r *http.Request) { + userID, ok := h.pendingUserID(w, r) + if !ok { + return + } + memberships, err := h.users.ListMembershipsWithTenantForUser(r.Context(), userID) + if err != nil { + h.logger.Error("listing memberships with tenant", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + options := make([]membershipOption, 0, len(memberships)) + for _, m := range memberships { + options = append(options, membershipOption{TenantID: m.TenantID, TenantDisplayName: m.TenantDisplayName, Role: string(m.Role)}) + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(options) +} + +type selectTenantRequest struct { + TenantID string `json:"tenant_id"` +} + +type selectTenantResponse struct { + RedirectURL string `json:"redirect_url"` +} + +// handleSelectTenant completes the tenant-selection round trip: given a +// still-valid pending login and a chosen tenant_id, re-derives the +// membership/role for that specific tenant server-side (never trusting +// a client-supplied role -- only which tenant they claim to want, +// checked against their actual memberships) and issues the real +// session. Responds with JSON, not a redirect: this is a POST a +// tenant-picker page would call via fetch, which should decide for +// itself how to navigate afterward, not have a redirect response +// imposed on it the way the GET-based OIDC/SAML callbacks reasonably +// can. +func (h *Handler) handleSelectTenant(w http.ResponseWriter, r *http.Request) { + userID, ok := h.pendingUserID(w, r) + if !ok { + return + } + var body selectTenantRequest + if err := json.NewDecoder(r.Body).Decode(&body); err != nil || body.TenantID == "" { + http.Error(w, `invalid request body -- expected {"tenant_id": "..."}`, http.StatusBadRequest) + return + } + + memberships, err := h.users.ListMembershipsForUser(r.Context(), userID) + if err != nil { + h.logger.Error("listing memberships", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + var role string + found := false + for _, m := range memberships { + if m.TenantID == body.TenantID { + role = string(m.Role) + found = true + break + } + } + if !found { + http.Error(w, "no membership in the requested tenant", http.StatusForbidden) + return + } + + clearCookie(w, r, pendingLoginCookieName, "/auth") + token, err := h.session.IssueUserSession(body.TenantID, userID, role) + if err != nil { + h.logger.Error("issuing session", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + http.SetCookie(w, &http.Cookie{ + Name: authhandler.SessionCookieName, Value: token, Path: "/", + HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + MaxAge: int(session.HumanSessionTTL.Seconds()), + }) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(selectTenantResponse{RedirectURL: h.postLoginRedirectURL}) +} + 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 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") + // ErrNoMembership is exported so tests (and any future caller that + // wants to distinguish this from other failures) don't have to + // string-match the HTTP error body. + ErrNoMembership = errors.New("loginhandler: this identity has no tenant membership -- contact your administrator") ) type resolvedIdentity struct { @@ -287,29 +453,40 @@ type resolvedIdentity struct { role string } +// identityOutcome is resolveIdentity's result: exactly one membership +// resolves identity directly; more than one sets ambiguous (userID is +// always populated so the caller can start tenant selection without a +// second lookup). +type identityOutcome struct { + identity resolvedIdentity + userID string + ambiguous bool +} + // resolveIdentity is the policy decision this whole package exists to // 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) { +// Zero memberships refuses outright (ErrNoMembership) -- there's +// nothing to choose between. More than one is no longer a refusal (see +// this package's doc comment): finishLogin routes an ambiguous outcome +// into tenant selection instead of erroring. +func (h *Handler) resolveIdentity(ctx context.Context, subject, email string) (identityOutcome, 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) + return identityOutcome{}, http.StatusInternalServerError, fmt.Errorf("loginhandler: upserting user: %w", err) } memberships, err := h.users.ListMembershipsForUser(ctx, user.ID) if err != nil { - return resolvedIdentity{}, http.StatusInternalServerError, fmt.Errorf("loginhandler: listing memberships: %w", err) + return identityOutcome{}, http.StatusInternalServerError, fmt.Errorf("loginhandler: listing memberships: %w", err) } switch len(memberships) { case 0: - return resolvedIdentity{}, http.StatusForbidden, ErrNoMembership + return identityOutcome{}, http.StatusForbidden, ErrNoMembership case 1: - return resolvedIdentity{tenantID: memberships[0].TenantID, userID: user.ID, role: string(memberships[0].Role)}, 0, nil + return identityOutcome{identity: resolvedIdentity{tenantID: memberships[0].TenantID, userID: user.ID, role: string(memberships[0].Role)}, userID: user.ID}, 0, nil default: - return resolvedIdentity{}, http.StatusNotImplemented, ErrMultipleMemberships + return identityOutcome{userID: user.ID, ambiguous: true}, 0, nil } } diff --git a/enterprise/internal/loginhandler/loginhandler_test.go b/enterprise/internal/loginhandler/loginhandler_test.go index 0d63cfb..9ef45df 100644 --- a/enterprise/internal/loginhandler/loginhandler_test.go +++ b/enterprise/internal/loginhandler/loginhandler_test.go @@ -17,6 +17,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -34,12 +35,17 @@ const testKeyID = "test-key-1" // SSO subject -- enough to drive resolveIdentity's logic without a real // Postgres. type fakeUserStore struct { - usersBySubject map[string]*rbacstore.User - memberships map[string][]rbacstore.Membership // by user ID + usersBySubject map[string]*rbacstore.User + memberships map[string][]rbacstore.Membership // by user ID + tenantDisplayNames map[string]string // by tenant ID, defaults to the ID itself if unset } func newFakeUserStore() *fakeUserStore { - return &fakeUserStore{usersBySubject: map[string]*rbacstore.User{}, memberships: map[string][]rbacstore.Membership{}} + return &fakeUserStore{ + usersBySubject: map[string]*rbacstore.User{}, + memberships: map[string][]rbacstore.Membership{}, + tenantDisplayNames: map[string]string{}, + } } func (f *fakeUserStore) UpsertUserBySSO(_ context.Context, ssoSubject, email, displayName string) (*rbacstore.User, error) { @@ -56,6 +62,19 @@ func (f *fakeUserStore) ListMembershipsForUser(_ context.Context, userID string) return f.memberships[userID], nil } +func (f *fakeUserStore) ListMembershipsWithTenantForUser(_ context.Context, userID string) ([]rbacstore.MembershipWithTenant, error) { + memberships := f.memberships[userID] + out := make([]rbacstore.MembershipWithTenant, 0, len(memberships)) + for _, m := range memberships { + displayName := f.tenantDisplayNames[m.TenantID] + if displayName == "" { + displayName = m.TenantID + } + out = append(out, rbacstore.MembershipWithTenant{TenantID: m.TenantID, TenantDisplayName: displayName, Role: m.Role}) + } + return out, nil +} + // testIdP bundles a real oidctest.Server (discovery + JWKS) with a local // /token handler, and knows how to mint a validly-signed ID token for a // given subject/email -- everything a test needs to drive a real login @@ -128,7 +147,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), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -188,7 +207,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), nil, sessionManager, store, "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, sessionManager, store, "http://web/", "http://web/select-tenant") idp.setNextIDToken(t, "user-1", "person@acme.example", true, time.Now().Add(time.Hour)) rec := fullLoginFlow(t, h, idp) @@ -220,7 +239,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), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") idp.setNextIDToken(t, "user-2", "nobody@acme.example", true, time.Now().Add(time.Hour)) rec := fullLoginFlow(t, h, idp) @@ -230,26 +249,282 @@ func TestFullLoginFlowRefusesNoMembership(t *testing.T) { } } -func TestFullLoginFlowRefusesMultipleMemberships(t *testing.T) { +// TestFullLoginFlowStartsTenantSelectionForMultipleMemberships is the +// regression test for the real tenant-picker protocol (see this +// package's doc comment): an identity with more than one +// tenant_memberships row must get a pending-login cookie and a redirect +// to selectTenantRedirectURL, not a flat refusal and not a session +// cookie for either tenant guessed at. +func TestFullLoginFlowStartsTenantSelectionForMultipleMemberships(t *testing.T) { idp := newTestIdP(t) store := newFakeUserStore() store.memberships["user-user-3"] = []rbacstore.Membership{ {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), nil, newTestSessionManager(t), store, "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), store, "http://web/", "http://web/select-tenant") idp.setNextIDToken(t, "user-3", "multi@example.com", true, time.Now().Add(time.Hour)) rec := fullLoginFlow(t, h, idp) - if rec.Code != http.StatusNotImplemented { - t.Fatalf("status = %d, want 501; body=%s", rec.Code, rec.Body.String()) + 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/select-tenant" { + t.Fatalf("Location = %q, want http://web/select-tenant", loc) + } + var pendingCookie, sessionCookie *http.Cookie + for _, c := range rec.Result().Cookies() { + switch c.Name { + case pendingLoginCookieName: + pendingCookie = c + case "sentry_session": + sessionCookie = c + } + } + if pendingCookie == nil || pendingCookie.Value == "" { + t.Fatal("expected a non-empty pending-login cookie") + } + if sessionCookie != nil { + t.Fatal("must not issue a real session cookie before a tenant is actually chosen") + } +} + +// startTenantSelection drives a full OIDC login for an identity that +// resolves to more than one membership, returning the mux (so callers +// can drive GET /auth/memberships and POST /auth/select-tenant +// afterward, exactly like a picker page would) and the pending-login +// cookie the login flow set. +func startTenantSelection(t *testing.T, h *Handler, idp *testIdP) (*http.ServeMux, *http.Cookie) { + t.Helper() + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + loginRec := httptest.NewRecorder() + mux.ServeHTTP(loginRec, httptest.NewRequest(http.MethodGet, "/auth/oidc/login", nil)) + var stateCookie *http.Cookie + for _, c := range loginRec.Result().Cookies() { + if c.Name == oidcStateCookieName { + stateCookie = c + } + } + if stateCookie == nil { + t.Fatal("no state cookie from /auth/oidc/login") + } + + callbackReq := httptest.NewRequest(http.MethodGet, "/auth/oidc/callback?state="+stateCookie.Value+"&code=test-code", nil) + callbackReq.AddCookie(stateCookie) + callbackRec := httptest.NewRecorder() + mux.ServeHTTP(callbackRec, callbackReq) + if callbackRec.Code != http.StatusFound { + t.Fatalf("callback status = %d, want 302; body=%s", callbackRec.Code, callbackRec.Body.String()) + } + var pendingCookie *http.Cookie + for _, c := range callbackRec.Result().Cookies() { + if c.Name == pendingLoginCookieName { + pendingCookie = c + } + } + if pendingCookie == nil { + t.Fatal("expected a pending-login cookie") + } + return mux, pendingCookie +} + +func newMultiMembershipStore() *fakeUserStore { + store := newFakeUserStore() + store.memberships["user-user-multi"] = []rbacstore.Membership{ + {TenantID: "acme", UserID: "user-user-multi", Role: rbacstore.RoleViewer}, + {TenantID: "globex", UserID: "user-user-multi", Role: rbacstore.RoleAdmin}, + } + store.tenantDisplayNames["acme"] = "Acme Corp" + store.tenantDisplayNames["globex"] = "Globex Corporation" + return store +} + +func TestListMembershipsReturnsTenantOptions(t *testing.T) { + idp := newTestIdP(t) + store := newMultiMembershipStore() + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), store, "http://web/", "http://web/select-tenant") + idp.setNextIDToken(t, "user-multi", "multi@example.com", true, time.Now().Add(time.Hour)) + mux, pendingCookie := startTenantSelection(t, h, idp) + + req := httptest.NewRequest(http.MethodGet, "/auth/memberships", nil) + req.AddCookie(pendingCookie) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var options []membershipOption + if err := json.Unmarshal(rec.Body.Bytes(), &options); err != nil { + t.Fatalf("decoding response: %v", err) + } + if len(options) != 2 { + t.Fatalf("got %d options, want 2: %+v", len(options), options) + } + byTenant := map[string]membershipOption{} + for _, o := range options { + byTenant[o.TenantID] = o + } + if byTenant["acme"].TenantDisplayName != "Acme Corp" || byTenant["acme"].Role != "viewer" { + t.Fatalf("unexpected acme option: %+v", byTenant["acme"]) + } + if byTenant["globex"].TenantDisplayName != "Globex Corporation" || byTenant["globex"].Role != "admin" { + t.Fatalf("unexpected globex option: %+v", byTenant["globex"]) + } +} + +func TestListMembershipsRejectsMissingPendingCookie(t *testing.T) { + idp := newTestIdP(t) + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/memberships", nil)) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +func TestSelectTenantIssuesSessionForChosenTenant(t *testing.T) { + idp := newTestIdP(t) + store := newMultiMembershipStore() + sessionManager := newTestSessionManager(t) + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, sessionManager, store, "http://web/", "http://web/select-tenant") + idp.setNextIDToken(t, "user-multi", "multi@example.com", true, time.Now().Add(time.Hour)) + mux, pendingCookie := startTenantSelection(t, h, idp) + + req := httptest.NewRequest(http.MethodPost, "/auth/select-tenant", strings.NewReader(`{"tenant_id": "globex"}`)) + req.AddCookie(pendingCookie) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var sessionCookie, clearedPendingCookie *http.Cookie + for _, c := range rec.Result().Cookies() { + switch c.Name { + case "sentry_session": + sessionCookie = c + case pendingLoginCookieName: + clearedPendingCookie = c + } + } + if sessionCookie == nil || sessionCookie.Value == "" { + t.Fatal("expected a sentry_session cookie to be set") + } + if clearedPendingCookie == nil || clearedPendingCookie.MaxAge >= 0 { + t.Fatalf("expected the pending-login cookie to be cleared (MaxAge < 0), got %+v", clearedPendingCookie) + } + claims, err := sessionManager.Validate(sessionCookie.Value) + if err != nil { + t.Fatalf("validating issued session: %v", err) + } + if claims.TenantID != "globex" || claims.Role != "admin" || claims.UserID != "user-user-multi" { + t.Fatalf("unexpected session claims: %+v", claims) + } + + var body selectTenantResponse + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decoding response: %v", err) + } + if body.RedirectURL != "http://web/" { + t.Fatalf("RedirectURL = %q, want http://web/", body.RedirectURL) + } +} + +// TestSelectTenantRejectsTenantOutsideMembership is the regression test +// for handleSelectTenant re-deriving role server-side rather than +// trusting the request: a client claiming a tenant_id the pending +// identity doesn't actually belong to must be refused, not silently +// granted whatever role happened to be in the request. +func TestSelectTenantRejectsTenantOutsideMembership(t *testing.T) { + idp := newTestIdP(t) + store := newMultiMembershipStore() + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), store, "http://web/", "http://web/select-tenant") + idp.setNextIDToken(t, "user-multi", "multi@example.com", true, time.Now().Add(time.Hour)) + mux, pendingCookie := startTenantSelection(t, h, idp) + + req := httptest.NewRequest(http.MethodPost, "/auth/select-tenant", strings.NewReader(`{"tenant_id": "not-a-real-membership"}`)) + req.AddCookie(pendingCookie) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) + } + for _, c := range rec.Result().Cookies() { + if c.Name == "sentry_session" { + t.Fatal("must not issue a session cookie for a tenant outside the identity's memberships") + } + } +} + +func TestSelectTenantRejectsMissingBody(t *testing.T) { + idp := newTestIdP(t) + store := newMultiMembershipStore() + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), store, "http://web/", "http://web/select-tenant") + idp.setNextIDToken(t, "user-multi", "multi@example.com", true, time.Now().Add(time.Hour)) + mux, pendingCookie := startTenantSelection(t, h, idp) + + req := httptest.NewRequest(http.MethodPost, "/auth/select-tenant", strings.NewReader(`{}`)) + req.AddCookie(pendingCookie) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +func TestSelectTenantRejectsMissingPendingCookie(t *testing.T) { + idp := newTestIdP(t) + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/auth/select-tenant", strings.NewReader(`{"tenant_id": "acme"}`))) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +// TestSelectTenantRejectsSessionCookieAsPendingCookie is the regression +// test for PendingLoginClaims being a distinct type from Claims: a real +// session token must not work as a pending-login cookie, even though +// both are signed by the same key. +func TestSelectTenantRejectsSessionCookieAsPendingCookie(t *testing.T) { + idp := newTestIdP(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), nil, sessionManager, store, "http://web/", "http://web/select-tenant") + + realSessionToken, err := sessionManager.IssueUserSession("acme", "user-user-1", "editor") + if err != nil { + t.Fatalf("IssueUserSession: %v", err) + } + + mux := http.NewServeMux() + h.RegisterRoutes(mux) + req := httptest.NewRequest(http.MethodPost, "/auth/select-tenant", strings.NewReader(`{"tenant_id": "acme"}`)) + req.AddCookie(&http.Cookie{Name: pendingLoginCookieName, Value: realSessionToken}) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 (a real session token must not validate as a pending login)", rec.Code) } } func TestCallbackRejectsStateMismatch(t *testing.T) { idp := newTestIdP(t) - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -265,7 +540,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), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -279,7 +554,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), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") idp.setNextIDToken(t, "user-4", "person@example.com", true, time.Now().Add(-time.Hour)) // already expired rec := fullLoginFlow(t, h, idp) @@ -290,7 +565,7 @@ func TestCallbackRejectsExpiredIDToken(t *testing.T) { } func TestRegisterRoutesNoOpWhenOIDCNotConfigured(t *testing.T) { - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -314,7 +589,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, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), provider, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) diff --git a/enterprise/internal/loginhandler/saml_test.go b/enterprise/internal/loginhandler/saml_test.go index 563808b..59a0ac6 100644 --- a/enterprise/internal/loginhandler/saml_test.go +++ b/enterprise/internal/loginhandler/saml_test.go @@ -239,7 +239,7 @@ func fullSAMLLoginFlow(t *testing.T, h *Handler, idp *testSAMLIdP, nameID, email 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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -274,7 +274,7 @@ func TestFullSAMLLoginFlowIssuesSessionForSingleMembership(t *testing.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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), sessionManager, store, "http://web/", "http://web/select-tenant") rec := fullSAMLLoginFlow(t, h, idp, "saml-user-1", "person@acme.example") @@ -305,7 +305,7 @@ func TestFullSAMLLoginFlowIssuesSessionForSingleMembership(t *testing.T) { 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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") rec := fullSAMLLoginFlow(t, h, idp, "saml-user-2", "nobody@acme.example") @@ -314,25 +314,42 @@ func TestFullSAMLLoginFlowRefusesNoMembership(t *testing.T) { } } -func TestFullSAMLLoginFlowRefusesMultipleMemberships(t *testing.T) { +// TestFullSAMLLoginFlowStartsTenantSelectionForMultipleMemberships is +// SAML's side of the tenant-picker regression test -- see +// loginhandler_test.go's OIDC equivalent for the full reasoning; both +// protocols converge on the same finishLogin/resolveIdentity, so the +// behavior must match exactly. +func TestFullSAMLLoginFlowStartsTenantSelectionForMultipleMemberships(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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), store, "http://web/", "http://web/select-tenant") rec := fullSAMLLoginFlow(t, h, idp, "saml-user-3", "multi@example.com") - if rec.Code != http.StatusNotImplemented { - t.Fatalf("status = %d, want 501; body=%s", rec.Code, rec.Body.String()) + 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/select-tenant" { + t.Fatalf("Location = %q, want http://web/select-tenant", loc) + } + var pendingCookie *http.Cookie + for _, c := range rec.Result().Cookies() { + if c.Name == pendingLoginCookieName { + pendingCookie = c + } + } + if pendingCookie == nil || pendingCookie.Value == "" { + t.Fatal("expected a non-empty pending-login cookie") } } 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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") rec := fullSAMLLoginFlow(t, h, idp, "saml-user-4", "") // no email attribute in the assertion @@ -343,7 +360,7 @@ func TestFullSAMLLoginFlowRefusesMissingEmail(t *testing.T) { 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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -363,7 +380,7 @@ func TestSAMLACSRejectsMissingRequestCookie(t *testing.T) { 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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -394,7 +411,7 @@ func TestSAMLACSRejectsWrongRequestID(t *testing.T) { } func TestRegisterRoutesNoOpWhenSAMLNotConfigured(t *testing.T) { - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -411,7 +428,7 @@ func TestRegisterRoutesNoOpWhenSAMLNotConfigured(t *testing.T) { // 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/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, provider, newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") mux := http.NewServeMux() h.RegisterRoutes(mux) diff --git a/enterprise/internal/rbacstore/rbacstore.go b/enterprise/internal/rbacstore/rbacstore.go index eecf49e..cab8031 100644 --- a/enterprise/internal/rbacstore/rbacstore.go +++ b/enterprise/internal/rbacstore/rbacstore.go @@ -360,6 +360,46 @@ func (s *Store) ListMembershipsForUser(ctx context.Context, userID string) ([]Me return out, rows.Err() } +// MembershipWithTenant is ListMembershipsWithTenantForUser's result row +// -- Membership plus the tenant's display name, the shape a +// tenant-picker UI needs (a bare tenant_id/Role isn't enough to show a +// human something recognizable to choose between). +type MembershipWithTenant struct { + TenantID string + TenantDisplayName string + Role Role +} + +// ListMembershipsWithTenantForUser is ListMembershipsForUser plus a join +// against tenants -- used by loginhandler's multi-membership +// tenant-selection step (GET /auth/memberships), which is the one +// caller that actually needs to show a human "here are your tenants," +// not just resolve a single membership programmatically. +func (s *Store) ListMembershipsWithTenantForUser(ctx context.Context, userID string) ([]MembershipWithTenant, error) { + rows, err := s.pool.Query(ctx, ` + SELECT m.tenant_id, t.display_name, m.role + FROM tenant_memberships m + JOIN tenants t ON t.id = m.tenant_id + WHERE m.user_id = $1 + ORDER BY t.display_name`, userID) + if err != nil { + return nil, fmt.Errorf("rbacstore: listing memberships with tenant: %w", err) + } + defer rows.Close() + + var out []MembershipWithTenant + for rows.Next() { + var m MembershipWithTenant + var role string + if err := rows.Scan(&m.TenantID, &m.TenantDisplayName, &role); err != nil { + return nil, fmt.Errorf("rbacstore: scanning membership with tenant: %w", err) + } + m.Role = Role(role) + out = append(out, m) + } + return out, rows.Err() +} + // DataSource is one tenant's data-plane location -- today, exactly one // ClickHouse database + one Tantivy index per tenant (see // /docs/phase-4-rbac-design.md's "data_sources" extension-point diff --git a/enterprise/internal/session/session.go b/enterprise/internal/session/session.go index 5e95215..5d9f0c7 100644 --- a/enterprise/internal/session/session.go +++ b/enterprise/internal/session/session.go @@ -44,6 +44,12 @@ const ( // Rotation is by redeploying alerting with a freshly issued token, // not automatic refresh. ServiceTokenTTL = 24 * 365 * time.Hour + // PendingLoginTTL is intentionally short -- a pending login only + // bridges the gap between "the IdP round trip proved who you are" + // and "you picked which tenant to act as" for a multi-membership + // identity (see loginhandler's package doc comment), a single-page + // interaction, not a session lifetime. + PendingLoginTTL = 10 * time.Minute // MinSigningKeyBytes: HS256 wants a key at least as long as its // output (32 bytes/256 bits) to not weaken the MAC. MinSigningKeyBytes = 32 @@ -115,6 +121,72 @@ func (m *Manager) IssueServiceToken(subject string) (string, error) { return jwt.Signed(m.signer).Claims(claims).Serialize() } +// PendingLoginClaims carries a proven-but-not-yet-tenant-scoped identity +// through the multi-membership tenant-selection round trip -- see +// loginhandler.startTenantSelection/handleSelectTenant. Deliberately a +// different Go type from Claims, not the same struct with an empty +// TenantID/Role: a pending token's JSON body never has those keys at +// all, so there's no field a caller could mistake for a real session's +// tenant/role, and no risk of this token type ever satisfying a +// role-gated check by accident (see ValidatePendingLogin -- callers +// that mistakenly feed a pending token to Validate instead just get a +// Claims with an empty Role, which authz.Role.Satisfies already treats +// as satisfying nothing). +// +// The json tag is deliberately "pending_user_id", not the "user_id" a +// real Claims token also carries -- found while writing this package's +// own test for "a real session token must not work as a pending +// login": go-jose's Claims() unmarshal is happy to populate any struct +// field whose json tag matches a key present in the token, key overlap +// included, so reusing "user_id" would have let a full session token +// parse successfully as a PendingLoginClaims too (extracting UserID +// from the session's own user_id field) -- exactly the token-type +// confusion this type's separate-Go-type design was supposed to +// prevent. A disjoint field name closes that regardless of which +// fields either struct happens to add later. +type PendingLoginClaims struct { + UserID string `json:"pending_user_id"` + jwt.Claims +} + +// IssuePendingLogin issues a short-lived token proving userID's identity +// (already resolved by resolveIdentity's UpsertUserBySSO) without +// committing to a tenant yet -- called only when that identity has more +// than one tenant_memberships row. +func (m *Manager) IssuePendingLogin(userID string) (string, error) { + now := time.Now() + claims := PendingLoginClaims{ + UserID: userID, + Claims: jwt.Claims{ + Subject: userID, + IssuedAt: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(PendingLoginTTL)), + }, + } + return jwt.Signed(m.signer).Claims(claims).Serialize() +} + +// ValidatePendingLogin is IssuePendingLogin's counterpart -- same +// signature/expiry checks as Validate, collapsed to ErrInvalidToken for +// the same reasoning (see that method's doc comment). +func (m *Manager) ValidatePendingLogin(token string) (userID string, err error) { + parsed, err := jwt.ParseSigned(token, []josev4.SignatureAlgorithm{josev4.HS256}) + if err != nil { + return "", ErrInvalidToken + } + var claims PendingLoginClaims + if err := parsed.Claims(m.key, &claims); err != nil { + return "", ErrInvalidToken + } + if err := claims.Claims.Validate(jwt.Expected{}); err != nil { + return "", ErrInvalidToken + } + if claims.UserID == "" { + return "", ErrInvalidToken + } + return claims.UserID, nil +} + // Validate verifies signature and expiry and returns the token's claims. // Every failure mode collapses to ErrInvalidToken -- see its doc comment. func (m *Manager) Validate(token string) (Claims, error) { diff --git a/enterprise/internal/session/session_test.go b/enterprise/internal/session/session_test.go index 315559c..f4b3ed7 100644 --- a/enterprise/internal/session/session_test.go +++ b/enterprise/internal/session/session_test.go @@ -95,6 +95,107 @@ func TestValidateRejectsWrongKey(t *testing.T) { } } +func TestIssueAndValidatePendingLogin(t *testing.T) { + m, err := NewManager(testKey()) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + token, err := m.IssuePendingLogin("u1") + if err != nil { + t.Fatalf("IssuePendingLogin: %v", err) + } + userID, err := m.ValidatePendingLogin(token) + if err != nil { + t.Fatalf("ValidatePendingLogin: %v", err) + } + if userID != "u1" { + t.Fatalf("userID = %q, want u1", userID) + } +} + +func TestValidatePendingLoginRejectsTamperedToken(t *testing.T) { + m, err := NewManager(testKey()) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + token, err := m.IssuePendingLogin("u1") + if err != nil { + t.Fatalf("IssuePendingLogin: %v", err) + } + parts := strings.Split(token, ".") + if len(parts) != 3 { + t.Fatalf("expected a 3-segment JWT, got %d segments", len(parts)) + } + tampered := parts[0] + "." + parts[1] + "x" + "." + parts[2] + if _, err := m.ValidatePendingLogin(tampered); err != ErrInvalidToken { + t.Fatalf("ValidatePendingLogin(tampered) error = %v, want ErrInvalidToken", err) + } +} + +// TestValidatePendingLoginRejectsRealSessionToken is the regression test +// for PendingLoginClaims.UserID's "pending_user_id" json tag (see that +// field's doc comment): a real user-session token must not parse as a +// valid pending login just because both structs happen to be signed by +// the same key. +func TestValidatePendingLoginRejectsRealSessionToken(t *testing.T) { + m, err := NewManager(testKey()) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + sessionToken, err := m.IssueUserSession("acme", "u1", "editor") + if err != nil { + t.Fatalf("IssueUserSession: %v", err) + } + if _, err := m.ValidatePendingLogin(sessionToken); err != ErrInvalidToken { + t.Fatalf("ValidatePendingLogin(a real session token) error = %v, want ErrInvalidToken", err) + } +} + +// TestValidateRejectsPendingLoginToken is the same regression in the +// other direction: a pending-login token must not validate as a usable +// session either (it carries no tenant_id/role at all, so it would be +// inert even if it somehow parsed, but this proves that directly rather +// than relying on downstream role checks alone). +func TestValidateRejectsPendingLoginToken(t *testing.T) { + m, err := NewManager(testKey()) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + pendingToken, err := m.IssuePendingLogin("u1") + if err != nil { + t.Fatalf("IssuePendingLogin: %v", err) + } + claims, err := m.Validate(pendingToken) + if err != nil { + t.Fatalf("Validate(pending token): %v", err) + } + if claims.TenantID != "" || claims.UserID != "" || claims.Role != "" { + t.Fatalf("a pending-login token must not carry any tenant/user/role claims when read as a session, got %+v", claims) + } +} + +func TestValidatePendingLoginRejectsExpiredToken(t *testing.T) { + m, err := NewManager(testKey()) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + now := time.Now() + claims := PendingLoginClaims{ + UserID: "u1", + Claims: jwt.Claims{ + IssuedAt: jwt.NewNumericDate(now.Add(-1 * time.Hour)), + Expiry: jwt.NewNumericDate(now.Add(-30 * time.Minute)), + }, + } + token, err := jwt.Signed(m.signer).Claims(claims).Serialize() + if err != nil { + t.Fatalf("building an already-expired pending token: %v", err) + } + if _, err := m.ValidatePendingLogin(token); err != ErrInvalidToken { + t.Fatalf("ValidatePendingLogin(expired) error = %v, want ErrInvalidToken", err) + } +} + func TestValidateRejectsExpiredToken(t *testing.T) { m, err := NewManager(testKey()) if err != nil {