From 1fab02abd5ad9d0908e4085165d37087eeda8171 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 13 Aug 2026 23:00:35 -0700 Subject: [PATCH] Phase 4: real OIDC human login (enterprise/internal/loginhandler) Closes the other major named gap from this phase: until now, there was no way for a human to actually log in -- only /alerting's RoleService credential could be minted. GET /auth/oidc/login and GET /auth/oidc/callback drive the real coreos/go-oidc flow already wired in enterprise/internal/oidc: CSRF state in a short-lived cookie, code exchange, ID token verification, upserting a users row, resolving tenant/role from exactly one tenant_memberships row (refusing outright on zero or more than one, rather than guessing), and issuing a real session cookie. Unlike everything else built this phase, this one is genuinely verified end to end: the tests spin up coreos/go-oidc's own oidctest fake IdP, which signs real RS256 ID tokens, and drive the full login->callback-> session-cookie round trip through actual signature verification -- no live database or Docker needed, so nothing here is asserted without having actually been run in this session. Also fixes a real bug caught while wiring this into enterprise-auth's main.go: assigning a nil *oidc.Provider to the handler's interface field would have produced a non-nil interface wrapping a nil pointer (Go's classic typed-nil trap), silently breaking the "OIDC not configured" no-op path -- New() now takes the concrete pointer type and checks it before ever converting to the interface, with a regression test pinning the fix down. Still missing: SAML's equivalent (ACS endpoint), a tenant-picker UI for multi-membership identities, and any admin UI to actually create a tenant_memberships row (today that's manual SQL, documented in the runbook's new bootstrap walkthrough). --- CLAUDE.md | 29 +- docker-compose.yml | 13 + docs/architecture.md | 2 +- docs/phase-4-runbook.md | 81 ++++- docs/security/threat-model.md | 48 ++- enterprise/README.md | 29 +- enterprise/cmd/enterprise-auth/main.go | 51 ++- enterprise/internal/config/config.go | 5 + .../internal/loginhandler/loginhandler.go | 218 ++++++++++++ .../loginhandler/loginhandler_test.go | 326 ++++++++++++++++++ 10 files changed, 752 insertions(+), 50 deletions(-) create mode 100644 enterprise/internal/loginhandler/loginhandler.go create mode 100644 enterprise/internal/loginhandler/loginhandler_test.go diff --git a/CLAUDE.md b/CLAUDE.md index bc22479..519e3ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -152,17 +152,24 @@ access partway through the phase, so only the audit-logging guarantees were actually confirmed against a live database; the rest is untested beyond "compiles, and skips cleanly when no live database is configured" (see `/docs/phase-4-runbook.md`'s verification-status -section). Two things still keep this phase from being done: SSO login -(OIDC/SAML protocol wiring exists, no -HTTP login handler calls it), and Tantivy/free-text queries have no -per-tenant index routing at all (`enterprise-api` closes the ClickHouse -half of tenant isolation, not the Tantivy half) — plus a deployment gap -worth naming explicitly: nothing yet forces or even flags whether a -given deployment is actually running the isolated binary -(`enterprise-api`) versus the plain single-tenant one (`api`); both -still exist and nothing currently prevents mixing them up. Full -accounting: `/docs/security/threat-model.md`; step-by-step verification -procedure (not yet run against a live cluster in this environment): +section). Human OIDC login is now built too +(`enterprise/internal/loginhandler`: `GET /auth/oidc/login` + +`GET /auth/oidc/callback`, issuing a real session cookie after resolving +tenant/role from `tenant_memberships`) — genuinely verified, unlike the +ClickHouse pieces, via a real fake IdP that signs and verifies actual +RS256 tokens (`loginhandler_test.go`, all passing), though never tried +against a real external IdP or through a running `enterprise-auth` +container. Two things still keep this phase from being done: SAML login +(protocol wiring exists, no ACS handler calls it, following OIDC's now +-built pattern), and Tantivy/free-text queries have no per-tenant index +routing at all (`enterprise-api` closes the ClickHouse half of tenant +isolation, not the Tantivy half) — plus a deployment gap worth naming +explicitly: nothing yet forces or even flags whether a given deployment +is actually running the isolated binary (`enterprise-api`) versus the +plain single-tenant one (`api`); both still exist and nothing currently +prevents mixing them up. 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 bar this phase is aiming at, not a completed state. diff --git a/docker-compose.yml b/docker-compose.yml index 98bede7..4b8ad29 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -252,12 +252,25 @@ services: context: enterprise dockerfile: Dockerfile container_name: sentry-enterprise-auth + depends_on: + metadata-migrate: + condition: service_completed_successfully ports: - "8082:8082" environment: # Dev-only, same framing as CLICKHOUSE_PASSWORD above -- not a real # secret. Must be at least 32 bytes (see internal/config.Load). ENTERPRISE_SESSION_SIGNING_KEY: "sentry-dev-only-session-signing-key-32bytes+" + POSTGRES_ADDR: "metadata-postgres:5432" + POSTGRES_DATABASE: "sentry_metadata" + POSTGRES_USERNAME: "sentry" + POSTGRES_PASSWORD: "sentry-dev-only" + # Where the browser lands after internal/loginhandler sets a + # session cookie -- web's mapped host port (see web's build args + # for why this is localhost:3000, not the compose network's + # service DNS name: the browser resolves this, not a sibling + # container). + POST_LOGIN_REDIRECT_URL: "http://localhost:3000" healthcheck: test: ["CMD", "/enterprise-auth", "-healthcheck"] interval: 5s diff --git a/docs/architecture.md b/docs/architecture.md index c6b5af9..95c0dac 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -79,7 +79,7 @@ This split is not to be changed without discussion — see CLAUDE.md. | `search` (Rust, Phase 1) | Consumes the same Redpanda topic `ingest` does (own offset tracking), builds a Tantivy full-text index over `message`, serves matches over gRPC. One shared index for every tenant today — see "Tenant isolation" below. | | `api` (Go) | gRPC + REST gateway. `POST /query` compiles pipe-syntax or raw SQL to one IR, executed across ClickHouse/Tantivy (`/docs/query-language-design.md`). `internal/dashboards` is CRUD only — panel query execution happens client-side, reusing `/query`. `internal/authz` (Phase 4) enforces RBAC via a network call to `enterprise-auth`, never an import. | | `alerting` (Go, Phase 3) | Evaluates alert rules on an interval, calls `api`'s `POST /query` (via a `RoleService` credential once Phase 4 auth is configured — see `/docs/phase-4-isolation-design.md`'s alerting↔api gap), delivers firing/resolved notifications (webhook/Slack/PagerDuty). | -| `enterprise` (Go, commercial license, Phase 4) | SSO (OIDC/SAML protocol mechanics), RBAC storage (`internal/rbacstore`), session/service-token issuance (`internal/session`), the append-only audit log (`internal/audit`), `enterprise-auth`'s HTTP surface (`/internal/authorize`, `/auth/features`), per-tenant ClickHouse provisioning (`internal/tenantprovision`) and query routing (`internal/chrunner`), and `cmd/enterprise-api` — a second binary combining core's `api/queryapi`/`api/dashboards` handlers with these tenant-aware implementations. Never imported by core — see "Licensing boundary" below. Does **not** yet include per-tenant Tantivy routing or the OIDC/SAML login HTTP handlers — see `/docs/security/threat-model.md`. | +| `enterprise` (Go, commercial license, Phase 4) | OIDC login (`internal/loginhandler`'s `/auth/oidc/login`+`/auth/oidc/callback`, real IdP round trip, verified with a fake IdP but not a real external one), RBAC storage (`internal/rbacstore`), session/service-token issuance (`internal/session`), the append-only audit log (`internal/audit`), `enterprise-auth`'s HTTP surface (`/internal/authorize`, `/auth/features`), per-tenant ClickHouse provisioning (`internal/tenantprovision`) and query routing (`internal/chrunner`), and `cmd/enterprise-api` — a second binary combining core's `api/queryapi`/`api/dashboards` handlers with these tenant-aware implementations. Never imported by core — see "Licensing boundary" below. Does **not** yet include per-tenant Tantivy routing or SAML's login ACS handler (protocol mechanics only) — see `/docs/security/threat-model.md`. | | `web` (SvelteKit, static build) | Query bar, dashboards, alerts, and (Phase 4) a settings page that renders SSO status via a runtime capability check (`GET /auth/features`) rather than bundling enterprise-licensed components. | | `cli` (`sentryctl`) | `ping`, `query`, `dashboards` (list/get/apply), `alerts` (list/get/apply). `$SENTRYCTL_TOKEN`, if set, is forwarded as a Bearer credential (Phase 4). | | `deploy` | A Helm chart covering every `docker-compose.yml` service, plus (Phase 4) a small Go Operator managing one CRD (`Tenant`) that provisions a per-tenant ClickHouse credential Secret. Never applied to a live cluster in the environment this was built in — see `/deploy/README.md`'s verification section before trusting it. | diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index 71a4e5c..9ea7a02 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -11,14 +11,19 @@ stack**, not asserted. This one is different, and says so plainly rather than papering over it: for the great majority of this phase's work, **there was no working Docker daemon access and no reachable Kubernetes cluster**, so most of what follows is a *procedure to run*, not a report -of what was already run and passed. One genuine exception, verified live -against a real Postgres earlier in this phase's work (see its own doc -comments for the exact `docker run` invocations, and note this was -before the environment lost Docker access, not a claim about this -runbook's own session): +of what was already run and passed. Two genuine exceptions: - `enterprise/internal/audit`'s hash-chain, tamper-detection, and - concurrent-write guarantees (task 4). + concurrent-write guarantees (task 4) -- verified live against a real + Postgres earlier in this phase's work (see its own doc comments for + the exact `docker run` invocations), before the environment lost + Docker access. +- `enterprise/internal/loginhandler`'s full OIDC login flow (§3a) -- + verified with real cryptography (a fake IdP that signs and verifies + genuine RS256 tokens) *without* needing Docker or a live database at + all, so this one was actually run in this runbook's own session, not + just an earlier one. What's still unverified is wiring it into a real + running `enterprise-auth` container against a real external IdP. Everything else — `internal/rbacstore`'s CRUD, the auth-enforcement walkthrough, the dashboards tenant-scoping fix, the Helm chart, the @@ -85,6 +90,61 @@ curl -s http://localhost:8082/auth/features # (no OIDC_ISSUER_URL/SAML_IDP_METADATA_URL set in this compose file) ``` +## 3a. `enterprise-auth`: human login via OIDC (new -- unlike everything +else in this runbook, the underlying flow *was* verified live in this +session, just not against a real running `enterprise-auth` container or +a real external IdP) + +`enterprise/internal/loginhandler`'s tests already prove the mechanism +works end to end against a real fake IdP (`go test +./internal/loginhandler/... -v` from `enterprise/`, no Docker needed -- +see `enterprise/README.md`). What's still unverified is wiring it into +this actual running stack. To try that for real, point +`docker-compose.yml`'s `enterprise-auth` service at a real OIDC IdP +(a free Auth0/Okta developer tenant, or any IdP you control): + +```sh +# Add to enterprise-auth's environment in docker-compose.yml (or a +# docker-compose.override.yml): +# OIDC_ISSUER_URL: "https://your-tenant.example.com/" +# OIDC_CLIENT_ID: "..." +# OIDC_CLIENT_SECRET: "..." +# OIDC_REDIRECT_URL: "http://localhost:8082/auth/oidc/callback" +# Register that same redirect URL with the IdP's application config. + +docker compose up -d --build enterprise-auth +curl -s http://localhost:8082/auth/features +# expect: {"sso_configured":true,"oidc_enabled":true,"saml_enabled":false} +``` + +Before a login can succeed, the logging-in identity needs a +`tenant_memberships` row -- there's no admin UI for this yet, so insert +one directly: + +```sh +docker run --rm --network sentry_default postgres:16-alpine psql \ + "postgres://sentry:sentry-dev-only@metadata-postgres:5432/sentry_metadata" -c \ + "INSERT INTO tenants (id, display_name, status) VALUES ('acme', 'Acme Corp', 'active') ON CONFLICT DO NOTHING;" +# The users row is created automatically on first login (UpsertUserBySSO) +# -- but tenant_memberships needs the user's ID, which doesn't exist +# until after a first login attempt fails with 403. Log in once (it'll +# fail with "no tenant membership"), then: +docker run --rm --network sentry_default postgres:16-alpine psql \ + "postgres://sentry:sentry-dev-only@metadata-postgres:5432/sentry_metadata" -c \ + "SELECT id, email FROM users;" +docker run --rm --network sentry_default postgres:16-alpine psql \ + "postgres://sentry:sentry-dev-only@metadata-postgres:5432/sentry_metadata" -c \ + "INSERT INTO tenant_memberships (id, tenant_id, user_id, role) VALUES (gen_random_uuid(), 'acme', '', 'viewer');" +``` + +Then visit `http://localhost:8082/auth/oidc/login` in a real browser, +complete the IdP's login, and confirm you land on +`POST_LOGIN_REDIRECT_URL` (`http://localhost:3000` by default) with a +`sentry_session` cookie set. This whole bootstrap sequence (manual SQL +to create the first tenant membership) is exactly the kind of rough +edge an admin UI would smooth over -- named as real future work, not +hidden. + ## 4. Turn on RBAC enforcement and prove it actually blocks/allows Without touching the main stack's `api` container (so step 2's baseline @@ -236,8 +296,13 @@ Full accounting: `/docs/security/threat-model.md`. Headline items: - **No Tantivy/free-text isolation at all**, regardless of which binary serves the request -- `enterprise/internal/searchclient` (chrunner's Tantivy-side sibling) doesn't exist. -- **No human SSO login.** OIDC/SAML protocol wiring exists; - the HTTP login/callback handlers that would use it don't. +- **Human SSO login now works for OIDC** (§3a) -- verified with a real + fake IdP, not yet a real external one or a running `enterprise-auth` + container. **SAML login still doesn't exist** -- protocol wiring only, + no ACS handler. No tenant-picker UI for a multi-membership identity + either (refused outright). +- No admin UI to create a `tenant_memberships` row -- §3a's manual SQL + bootstrap is the only way to grant a logged-in identity access today. - **No per-resource dashboard grants** (`dashboard_permissions` has a schema, no handler reads it). - Two of the four adversarial ClickHouse/Tantivy probes named in diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 71dc7b3..5acb00b 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -87,6 +87,10 @@ api/alerting ──▶ enterprise-auth (POST /internal/authorize, HTTP only — no Go import edge, see "Module boundary" below) +Browser ──▶ enterprise-auth (GET /auth/oidc/login, /auth/oidc/callback) + └─▶ external IdP (OIDC authorization code flow) + └─▶ Postgres (rbacstore: users, tenant_memberships) + sentryctl ──▶ api, alerting (Bearer token when SENTRYCTL_TOKEN is set) ``` @@ -113,15 +117,37 @@ to `enterprise-auth` — see "Deployment/network assumptions" below. ## Authentication -**Not implemented for human users.** `enterprise/internal/oidc` and -`enterprise/internal/saml` wire `coreos/go-oidc`/`crewjam/saml` for the -protocol mechanics (discovery, AuthnRequest generation, token/assertion -validation), but no HTTP handler calls them — there is no -`/auth/oidc/login`, `/auth/oidc/callback`, or SAML ACS endpoint. A human -cannot log in today. `GET /auth/features` (`enterprise/internal/ -authhandler`) reports whether OIDC/SAML are *configured* (for `/web`'s -settings page to conditionally render), which is independent of whether -login actually works. +**Implemented for OIDC, still missing for SAML.** +`enterprise/internal/loginhandler` serves `GET /auth/oidc/login` +(redirects to the configured IdP, with a short-lived HttpOnly cookie +carrying CSRF-protection state) and `GET /auth/oidc/callback` +(validates state, exchanges the code, verifies the ID token via +`enterprise/internal/oidc`'s real `coreos/go-oidc` wiring, upserts a +`users` row keyed by SSO subject, resolves tenant/role from +`tenant_memberships`, and issues a `session.Manager`-signed session +cookie). Verified end-to-end with real cryptography, not mocked: the +tests spin up a real fake IdP (`coreos/go-oidc`'s own `oidctest` +package) that signs genuine RS256 ID tokens, and +`enterprise/internal/loginhandler`'s handler verifies them for real via +the same code path production uses — every test in +`loginhandler_test.go` passes, including the full login→callback→ +session-cookie round trip. **Not yet verified**: wiring this into a +running `enterprise-auth` container against a *real* external IdP +(Google/Okta/etc.) — that needs real IdP credentials and a reachable +callback 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. `enterprise/internal/saml` still +only does the protocol mechanics (AuthnRequest generation, assertion +validation) with no ACS HTTP handler calling it — SAML login remains +unimplemented, following `loginhandler`'s OIDC pattern once it is built. +`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 two HTTP endpoints do). **Implemented for the one machine caller.** `/alerting`'s evaluator is the sole service-to-service caller (`POST /query`, to evaluate rule @@ -327,7 +353,9 @@ terms: | `system.*` ClickHouse metadata isolation | **Built, not live-verified** — same caveat as above | | Tantivy/free-text tenant isolation | **Not implemented** — no per-tenant index routing at all | | Deployment actually routing traffic to `enterprise-api` | **Not implemented** — no Helm service, no default wiring | -| Human SSO login (OIDC/SAML) | **Not implemented** | +| Human SSO login — OIDC | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | +| Human SSO login — SAML | **Not implemented** | +| Multi-tenant-membership login (tenant picker) | **Not implemented** — refused with a clear error, not guessed | | Per-resource dashboard grants (`own/granted`) | **Not implemented** | | 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 29f18e9..77125a2 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -51,6 +51,19 @@ section for exactly what "not yet run" means here and why. Don't read grants). - `internal/audit.QueryAPILogger`: the real `api/queryapi.AuditLogger` implementation -- wired into `enterprise-api`, no longer `nil`. +- `internal/loginhandler`: `GET /auth/oidc/login` + `GET /auth/oidc/callback` + -- 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. + **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 + verification, no live database or Docker needed) and every test + passes. Not yet tried against a real external IdP or a running + `enterprise-auth` container. - `cmd/enterprise-api`: a second binary (alongside `api/cmd/api`, unchanged) importing *both* `api`'s handler packages and the tenant-aware implementations above -- see its own doc comment for why @@ -62,11 +75,13 @@ section for exactly what "not yet run" means here and why. Don't read **Deliberately deferred, not half-built** -- named explicitly rather than silently left out: -- The actual OIDC/SAML login/callback HTTP handlers that would issue a - *human* session after a real IdP round trip (`internal/oidc`/ - `internal/saml` do the protocol mechanics; nothing calls them from an - HTTP handler yet). `-mint-service-token` is the only way to get a - token today, and it only mints `RoleService` credentials. +- 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`). - `dashboard_permissions` CRUD (schema exists, `metadata/migrations/0024`; no caller reads per-resource grants yet -- `dashboards`' handler enforces tenant-baseline role only, not @@ -91,6 +106,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/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 @@ -209,11 +225,12 @@ edit today, not a supported flag. | `OIDC_ISSUER_URL` | (empty — OIDC discovery skipped if unset) | | `OIDC_CLIENT_ID` | (empty) | | `OIDC_CLIENT_SECRET` | (empty) | -| `OIDC_REDIRECT_URL` | (empty) | +| `OIDC_REDIRECT_URL` | (empty — must be `/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) | | `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 | ## Environment variables (`enterprise-api`) diff --git a/enterprise/cmd/enterprise-auth/main.go b/enterprise/cmd/enterprise-auth/main.go index 873a603..b9fe76a 100644 --- a/enterprise/cmd/enterprise-auth/main.go +++ b/enterprise/cmd/enterprise-auth/main.go @@ -2,17 +2,17 @@ // service (commercial license, not AGPL) -- see // /docs/phase-4-isolation-design.md and /docs/phase-4-rbac-design.md. // -// Phase 4 task 5 adds session issuance/validation (internal/session) and -// the POST /internal/authorize endpoint api/authz.HTTPAuthorizer -// calls -- the piece that actually turns on RBAC enforcement in /api. -// Still deliberately missing: the OIDC/SAML login/callback HTTP handlers -// that would issue a *human* session after a real IdP round trip, and -// internal/rbacstore (the org/tenant/user/role Postgres storage those -// handlers need to look up a role from). Both depend on RBAC storage -// that wasn't built in task 3's scope and are called out as deferred -// rather than half-built -- see the task 5 summary. What IS wired end to -// end: minting and validating the RoleService credential /alerting -// presents, via -mint-service-token below. +// 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. package main import ( @@ -27,9 +27,13 @@ import ( "syscall" "time" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/sentry/sentry/enterprise/internal/authhandler" "github.com/sentry/sentry/enterprise/internal/config" + "github.com/sentry/sentry/enterprise/internal/loginhandler" "github.com/sentry/sentry/enterprise/internal/oidc" + "github.com/sentry/sentry/enterprise/internal/rbacstore" "github.com/sentry/sentry/enterprise/internal/session" ) @@ -78,18 +82,36 @@ func main() { ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() + pgDSN := fmt.Sprintf("postgres://%s:%s@%s/%s", cfg.Postgres.Username, cfg.Postgres.Password, cfg.Postgres.Addr, cfg.Postgres.Database) + pgPool, err := pgxpool.New(ctx, pgDSN) + if err != nil { + logger.Error("opening postgres pool", "error", err) + os.Exit(1) + } + defer pgPool.Close() + if err := pgPool.Ping(ctx); err != nil { + logger.Error("pinging postgres", "error", err) + os.Exit(1) + } + rbac := rbacstore.NewStore(pgPool) + + // oidcProvider stays nil (loginhandler.RegisterRoutes then registers + // nothing) unless OIDC is actually configured -- matches every other + // optional-config path in this codebase. + var oidcProvider *oidc.Provider if cfg.OIDC.IssuerURL != "" { - if _, err := oidc.New(ctx, oidc.Config{ + oidcProvider, err = oidc.New(ctx, oidc.Config{ IssuerURL: cfg.OIDC.IssuerURL, ClientID: cfg.OIDC.ClientID, ClientSecret: cfg.OIDC.ClientSecret, RedirectURL: cfg.OIDC.RedirectURL, Scopes: []string{"email", "profile"}, - }); err != nil { + }) + if err != nil { logger.Error("discovering OIDC issuer", "error", err) os.Exit(1) } logger.Info("OIDC provider configured", "issuer", cfg.OIDC.IssuerURL) } else { - logger.Info("OIDC not configured (OIDC_ISSUER_URL unset) -- skipping discovery") + logger.Info("OIDC not configured (OIDC_ISSUER_URL unset) -- skipping discovery, /auth/oidc/* routes disabled") } mux := http.NewServeMux() @@ -101,6 +123,7 @@ func main() { SAMLEnabled: cfg.SAML.IDPMetadataURL != "", } authhandler.New(logger, sessionManager, features).RegisterRoutes(mux) + loginhandler.New(logger, oidcProvider, sessionManager, rbac, cfg.PostLoginRedirectURL).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 6c687b3..8cb98e3 100644 --- a/enterprise/internal/config/config.go +++ b/enterprise/internal/config/config.go @@ -13,6 +13,10 @@ type Config struct { OIDC OIDCConfig SAML SAMLConfig SessionSigningKey []byte + // PostLoginRedirectURL is where the browser lands after + // internal/loginhandler sets a session cookie -- web's base URL in + // a real deployment. + PostLoginRedirectURL string } type PostgresConfig struct { @@ -66,6 +70,7 @@ func Load() (Config, error) { ACSURL: getenv("SAML_ACS_URL", ""), IDPMetadataURL: getenv("SAML_IDP_METADATA_URL", ""), }, + PostLoginRedirectURL: getenv("POST_LOGIN_REDIRECT_URL", "http://localhost:3000"), } // Required, unlike OIDC/SAML above: every enterprise-auth deployment diff --git a/enterprise/internal/loginhandler/loginhandler.go b/enterprise/internal/loginhandler/loginhandler.go new file mode 100644 index 0000000..96aab97 --- /dev/null +++ b/enterprise/internal/loginhandler/loginhandler.go @@ -0,0 +1,218 @@ +// Package loginhandler is the piece named as missing throughout Phase 4: +// 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. +// +// 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. +package loginhandler + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net/http" + "time" + + "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/session" +) + +// stateCookieName carries the 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" + +// stateCookieTTL 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 + +// userStore is the narrow interface Handler depends on -- *rbacstore.Store +// is the production implementation; tests use a fake, same pattern used +// throughout this codebase (api/dashboards, api/queryapi). +type userStore interface { + UpsertUserBySSO(ctx context.Context, ssoSubject, email, displayName string) (*rbacstore.User, error) + ListMembershipsForUser(ctx context.Context, userID string) ([]rbacstore.Membership, error) +} + +// oidcProvider is the narrow slice of *oidc.Provider Handler needs -- +// letting tests substitute a provider pointed at a fake IdP without +// needing real OIDC discovery against something Handler's own tests +// would have to stand up twice. +type oidcProvider interface { + AuthCodeURL(state string) string + Exchange(ctx context.Context, code string) (*oidc.Claims, error) +} + +type Handler struct { + logger *slog.Logger + oidc oidcProvider // nil if OIDC isn't configured -- RegisterRoutes registers nothing in that case + session *session.Manager + users userStore + // postLoginRedirectURL is where the browser lands after a session + // cookie is set -- web's base URL in a real deployment. + 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 { + h := &Handler{logger: logger, session: sessionManager, users: users, postLoginRedirectURL: postLoginRedirectURL} + if provider != nil { + h.oidc = provider + } + return h +} + +// RegisterRoutes registers OIDC's two routes only if OIDC is actually +// configured (h.oidc != nil) -- 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 + } + 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) { + state, err := oidc.NewState() + if err != nil { + h.logger.Error("generating oidc state", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + http.SetCookie(w, &http.Cookie{ + Name: stateCookieName, Value: state, Path: "/auth/oidc/callback", + HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + MaxAge: int(stateCookieTTL.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) { + http.SetCookie(w, &http.Cookie{ + Name: stateCookieName, Value: "", Path: "/auth/oidc/callback", + 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) + + stateCookie, err := r.Cookie(stateCookieName) + if err != nil || stateCookie.Value == "" { + http.Error(w, "missing or expired login state -- start over at /auth/oidc/login", http.StatusBadRequest) + return + } + if r.URL.Query().Get("state") != stateCookie.Value { + http.Error(w, "state mismatch -- possible CSRF, start over at /auth/oidc/login", http.StatusBadRequest) + return + } + + code := r.URL.Query().Get("code") + if code == "" { + http.Error(w, "missing code parameter", http.StatusBadRequest) + return + } + + claims, err := h.oidc.Exchange(r.Context(), code) + if err != nil { + h.logger.Error("exchanging oidc code", "error", err) + http.Error(w, "login failed", http.StatusUnauthorized) + return + } + if claims.Email == "" { + http.Error(w, "identity provider did not return an email claim", http.StatusUnauthorized) + return + } + + identity, status, err := h.resolveIdentity(r.Context(), claims) + if err != nil { + h.logger.Error("resolving identity after oidc login", "error", err, "email", claims.Email) + http.Error(w, err.Error(), status) + return + } + + token, err := h.session.IssueUserSession(identity.tenantID, identity.userID, identity.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()), + }) + http.Redirect(w, r, h.postLoginRedirectURL, http.StatusFound) +} + +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. + 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") +) + +type resolvedIdentity struct { + tenantID string + userID string + role string +} + +// 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) + if err != nil { + return resolvedIdentity{}, 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) + } + switch len(memberships) { + case 0: + return resolvedIdentity{}, http.StatusForbidden, ErrNoMembership + case 1: + return resolvedIdentity{tenantID: memberships[0].TenantID, userID: user.ID, role: string(memberships[0].Role)}, 0, nil + default: + return resolvedIdentity{}, http.StatusNotImplemented, ErrMultipleMemberships + } +} diff --git a/enterprise/internal/loginhandler/loginhandler_test.go b/enterprise/internal/loginhandler/loginhandler_test.go new file mode 100644 index 0000000..28ab588 --- /dev/null +++ b/enterprise/internal/loginhandler/loginhandler_test.go @@ -0,0 +1,326 @@ +// Uses coreos/go-oidc's own oidctest package (a real fake OIDC IdP -- +// serves discovery + JWKS, signs real RS256 ID tokens) plus a small +// local /token handler (oidctest doesn't implement the OAuth2 code +// exchange itself, only ID token signing/verification) to exercise the +// FULL login flow -- login redirect, a real signed-and-verified ID +// token round trip, user upsert, tenant/role resolution, and session +// cookie issuance -- with real cryptographic verification, not mocked. +package loginhandler + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "encoding/json" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/coreos/go-oidc/v3/oidc/oidctest" + + "github.com/sentry/sentry/enterprise/internal/oidc" + "github.com/sentry/sentry/enterprise/internal/rbacstore" + "github.com/sentry/sentry/enterprise/internal/session" +) + +const testClientID = "sentry-test-client" +const testKeyID = "test-key-1" + +// fakeUserStore is an in-memory stand-in for *rbacstore.Store, keyed by +// 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 +} + +func newFakeUserStore() *fakeUserStore { + return &fakeUserStore{usersBySubject: map[string]*rbacstore.User{}, memberships: map[string][]rbacstore.Membership{}} +} + +func (f *fakeUserStore) UpsertUserBySSO(_ context.Context, ssoSubject, email, displayName string) (*rbacstore.User, error) { + if u, ok := f.usersBySubject[ssoSubject]; ok { + u.Email, u.DisplayName = email, displayName + return u, nil + } + u := &rbacstore.User{ID: "user-" + ssoSubject, Email: email, DisplayName: displayName, SSOSubject: ssoSubject} + f.usersBySubject[ssoSubject] = u + return u, nil +} + +func (f *fakeUserStore) ListMembershipsForUser(_ context.Context, userID string) ([]rbacstore.Membership, error) { + return f.memberships[userID], 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 +// round trip. +type testIdP struct { + srv *httptest.Server + priv *rsa.PrivateKey + nextIDToken string +} + +func newTestIdP(t *testing.T) *testIdP { + t.Helper() + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generating RSA key: %v", err) + } + idp := &testIdP{priv: priv} + + oidcSrv := &oidctest.Server{ + PublicKeys: []oidctest.PublicKey{{PublicKey: priv.Public(), KeyID: testKeyID, Algorithm: "RS256"}}, + } + mux := http.NewServeMux() + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "test-access-token", + "id_token": idp.nextIDToken, + "token_type": "Bearer", + }) + }) + mux.Handle("/", oidcSrv) + + idp.srv = httptest.NewServer(mux) + oidcSrv.SetIssuer(idp.srv.URL) + t.Cleanup(idp.srv.Close) + return idp +} + +// setNextIDToken configures what /token returns on the next exchange -- +// a real RS256-signed JWT, verified for real by oidc.Provider.Exchange. +func (idp *testIdP) setNextIDToken(t *testing.T, subject, email string, emailVerified bool, expiry time.Time) { + t.Helper() + claims := fmt.Sprintf(`{ + "iss": %q, "aud": %q, "sub": %q, "email": %q, "email_verified": %v, + "iat": %d, "exp": %d + }`, idp.srv.URL, testClientID, subject, email, emailVerified, time.Now().Unix(), expiry.Unix()) + idp.nextIDToken = oidctest.SignIDToken(idp.priv, testKeyID, "RS256", claims) +} + +func newTestOIDCProvider(t *testing.T, idp *testIdP) *oidc.Provider { + t.Helper() + p, err := oidc.New(context.Background(), oidc.Config{ + IssuerURL: idp.srv.URL, ClientID: testClientID, ClientSecret: "secret", + RedirectURL: "http://sentry-test/auth/oidc/callback", + }) + if err != nil { + t.Fatalf("oidc.New: %v", err) + } + return p +} + +func newTestSessionManager(t *testing.T) *session.Manager { + t.Helper() + m, err := session.NewManager([]byte("this-is-a-32-byte-test-signing-key!")) + if err != nil { + t.Fatalf("session.NewManager: %v", err) + } + return m +} + +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/") + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/oidc/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") + } + cookies := rec.Result().Cookies() + var stateCookie *http.Cookie + for _, c := range cookies { + if c.Name == stateCookieName { + stateCookie = c + } + } + if stateCookie == nil || stateCookie.Value == "" { + t.Fatal("expected a non-empty state cookie to be set") + } + if !stateCookie.HttpOnly { + t.Fatal("expected the state cookie to be HttpOnly") + } +} + +// fullLoginFlow drives handleLogin then handleCallback end to end, +// exactly the way a browser + IdP round trip would, and returns the +// final response so callers can assert on it. +func fullLoginFlow(t *testing.T, h *Handler, idp *testIdP) *httptest.ResponseRecorder { + 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 == stateCookieName { + 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) + return callbackRec +} + +func TestFullLoginFlowIssuesSessionForSingleMembership(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), sessionManager, store, "http://web/") + + idp.setNextIDToken(t, "user-1", "person@acme.example", true, time.Now().Add(time.Hour)) + rec := fullLoginFlow(t, h, idp) + + 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-user-1" { + t.Fatalf("unexpected session claims: %+v", claims) + } +} + +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/") + + idp.setNextIDToken(t, "user-2", "nobody@acme.example", true, time.Now().Add(time.Hour)) + rec := fullLoginFlow(t, h, idp) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestFullLoginFlowRefusesMultipleMemberships(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), newTestSessionManager(t), store, "http://web/") + + 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()) + } +} + +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/") + 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"}) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +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/") + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/oidc/callback?state=whatever&code=test-code", nil)) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +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/") + + idp.setNextIDToken(t, "user-4", "person@example.com", true, time.Now().Add(-time.Hour)) // already expired + rec := fullLoginFlow(t, h, idp) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestRegisterRoutesNoOpWhenOIDCNotConfigured(t *testing.T) { + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/oidc/login", nil)) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (no routes should be registered when oidc is nil)", rec.Code) + } +} + +// TestRegisterRoutesNoOpWithTypedNilProviderVariable is the regression +// test for Go's typed-nil-interface trap: enterprise-auth's main.go +// holds a `var provider *oidc.Provider` that stays nil when OIDC isn't +// configured, then passes that *variable* (not a nil literal) into New. +// If New ever goes back to assigning that pointer straight into the +// oidcProvider interface field, this test starts failing -- the +// interface would become non-nil (type=*oidc.Provider, value=nil) even +// though the variable itself is nil, and RegisterRoutes' `h.oidc == nil` +// check would stop working. TestRegisterRoutesNoOpWhenOIDCNotConfigured +// above doesn't catch this: passing a nil literal directly never hits +// 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/") + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/oidc/login", nil)) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (a typed-nil *oidc.Provider must still result in oidc routes being disabled)", rec.Code) + } +}