# 2026-05-15 — Universe Onboarding PR1

Resumed `/plans/universe-onboarding.md` at PR1 (Broker config foundations: `WorldConfig.PublicURL`).

## Changes

- `tools/demarkus-broker/internal/broker/config.go` — added `WorldConfig.PublicURL string` with a doc comment explaining the field belongs on the broker (single source of truth for the client-facing address), is optional, and that a blank value means "skip in /me/install."
- `deploy/helm/demarkus-broker/values.yaml` — documented `publicURL` in the commented worlds[] example with the rationale for setting it.
- `deploy/helm/demarkus-broker/templates/secret-config.yaml` — render `publicURL: "<value-or-empty>"` for every world. Always emitted so the YAML shape is stable across set/unset, and operators can grep for the field.
- `tools/demarkus-broker/internal/broker/config_test.go` — two new subtests: empty default round-trip + explicit `mark://` value round-trip.
- `deploy/helm/demarkus-broker/tests/secret-config_test.yaml` — two new cases: blank renders as `publicURL: ""`, operator-supplied value renders verbatim.

## Verification

- `go test ./internal/broker/ -run TestLoadConfig` → 29/29 pass (2 new).
- `helm unittest -f tests/secret-config_test.yaml .` → 13/13 pass (2 new).
- `bash pre-commit.sh` → format, vet, lint clean across protocol/server/client/tools.

## Scope discipline

No validation in PR1 — plan calls for "optional; validation only requires it when used." Shape validation (`mark://` URL, port present, etc.) lives with the `/me/install` consumer in PR5.

`KnownFields(true)` means older broker binaries would reject the new chart's rendered config because of the unknown `publicURL` line. Chart appVersion gates broker upgrade as usual; not new for this PR.

## Next

PR2 — broker discovery doc (`GET /.well-known/openid-configuration` proxying the IdP discovery with device-endpoint overrides). Resume at `tools/demarkus-broker/internal/broker/discovery.go` (new file) + route registration in `server.go`.

Pending review: should PR1 ship as a standalone PR or be folded into PR2? Plan calls for independent reviewability per PR, so default is standalone.


## PR2 — Broker discovery doc

PR1 (#135) merged. Continuing through plan PR2: broker-hosted `/.well-known/openid-configuration` that proxies the IdP's discovery doc with broker-hosted endpoint overrides.

### Changes

- `tools/demarkus-broker/internal/broker/discovery.go` (new) — `Discovery` type. Fetches `<idpIssuer>/.well-known/openid-configuration` once at `NewDiscovery`, parses into `map[string]any` so unknown fields proxy through unchanged, rewrites `issuer` + `device_authorization_endpoint` + `token_endpoint` to broker URLs, caches the rendered body with a 5-minute TTL. Lazy refresh on TTL expiry, stale-while-error on refresh failure. Constructor blocks on initial fetch so a misconfigured/unreachable IdP fails the broker at boot rather than at first `/soul-join`.
- `tools/demarkus-broker/internal/broker/discovery_test.go` (new) — 13 test cases via httptest fake-IdP: constructor validation (broker URL + issuer required), upstream-failure failure paths, override correctness, proxy of `jwks_uri` + `userinfo_endpoint` + `authorization_endpoint` + arrays, response headers, cache hit count within TTL, refresh after TTL, stale-while-error, recovery from transient failure, trailing-slash trim, malformed JSON rejection.
- `tools/demarkus-broker/internal/broker/server.go` — `Server` gains a `discovery *Discovery` field and `NewServer` takes it as a constructor arg. `Routes()` registers `GET /.well-known/openid-configuration` only when `discovery != nil`, so existing tests pass `nil` and skip the route cleanly.
- `tools/demarkus-broker/internal/broker/server_test.go` — `TestWellKnownDiscoveryRouteRegistered` (wires real Discovery into NewServer, asserts override applied through the mounted route) + `TestWellKnownDiscoveryRouteSkippedWhenNil` (404 when constructed nil).
- `tools/demarkus-broker/main.go` — `NewDiscovery` called between `NewVerifier` and `NewServer`; passes `cfg.Server.PublicURL` + `cfg.OIDC.Issuer`. Failure is fatal at startup, same shape as `NewVerifier`.
- `tools/demarkus-broker/internal/broker/config.go` — new `ServerConfig.PublicURL` field, required at load, trailing slash stripped once at load so every downstream consumer sees the canonical form. Doc comment notes it intentionally diverges from `OIDC.RedirectURL` (which is purpose-specific to the IdP redirect).
- `tools/demarkus-broker/internal/broker/config_test.go` — 2 new subtests: required-rejection + trailing-slash trim.
- `deploy/helm/demarkus-broker/values.yaml` — `server.publicURL: ""` with full operator-facing comment.
- `deploy/helm/demarkus-broker/templates/secret-config.yaml` — renders `publicURL: {{ required ... }}` so chart-render fails clearly when omitted (same shape as `oidc.redirectURL`).
- `deploy/helm/demarkus-broker/tests/secret-config_test.yaml` — 2 new cases (renders value, fail-render when blank) + added `server.publicURL` to the suite-wide `set:` block.
- `deploy/helm/demarkus-broker/tests/deployment_test.yaml` — same suite-wide `set:` addition (this suite also templates secret-config).
- `deploy/kind/values-broker.yaml`, `deploy/kind/values-broker-argo.yaml` — set `server.publicURL: "http://localhost:8080"` to mirror the existing redirectURL shape. Stage 2/4 harnesses don't exercise the well-known route but the broker's `LoadConfig` now requires the field.

### Verification

- `go test ./internal/broker/ -count=1` → all green (4.3s). 13 new Discovery tests + 2 new server route tests + 2 new config tests.
- `helm unittest .` → 55/55 pass across all 7 suites (was 52/52 before; 3 new).
- `bash pre-commit.sh` → format + vet + lint clean.

### Design notes (worth remembering)

- **Override shape is intentionally lossy at the seam.** Override `issuer` to broker URL but leave `jwks_uri` pointed at the IdP. ID tokens during PR2/PR3 are still IdP-signed and carry IdP's `iss`. Strict client validation against this discovery doc would fail; the plugin's device-flow client (PR6) only consumes `device_authorization_endpoint` + `token_endpoint` from this doc so the mismatch is invisible to it. PR4 closes the gap by minting broker-signed id_tokens with a broker-hosted `jwks_uri`. Doc comment in `discovery.go` calls this out explicitly so a future reader doesn't trip over it.
- **`map[string]any` proxy beats a typed struct.** Tried a typed `idpDiscovery` struct first; killed it because real IdPs ship 20+ fields each (Google's `code_challenge_methods_supported`, Auth0's `mfa_challenge_endpoint`, Okta's `request_object_signing_alg_values_supported`, etc.) and the broker has no business knowing or filtering them. Decode → mutate three keys → re-encode is the right shape; tests assert both the overridden fields AND a couple proxy fields so regressions surface.
- **Lazy refresh, not background goroutine.** Plan §Risks talks about "5-minute TTL on the discovery cache." Two implementation shapes: background ticker, or in-handler lazy. Picked lazy because (a) one fewer goroutine lifecycle to wire into the broker shutdown path, (b) the load is human-scale — even with N replicas behind a Service, the per-instance refresh rate caps at 1/5min, (c) thundering herd on simultaneous expiry is negligible at this rate (two replicas might both refresh once; the IdP doesn't notice). Stale-while-error keeps the well-known endpoint available during transient upstream blips.
- **Constructor signature pattern.** `NewDiscovery` takes a `DiscoveryConfig` struct, not a long positional arg list — per /guidelines.md "more than 4 parameters is a smell." Same pattern as the existing `Issuer`/`Verifier` constructors take typed configs.
- **Test isolation gotcha.** Existing `oidc_test.go` already had `fakeIdP` + `newFakeIdP`. Renamed mine to `fakeDiscoveryIdP` + `newFakeDiscoveryIdP` rather than touch the existing symbol — both are test-package-scope so the rename is purely local. Worth keeping in mind if a third test file ever needs a fake IdP: extract to a shared `testhelpers_test.go` helper instead of accumulating per-suite copies.
- **`http.NoBody`, not `nil`, on GET requests.** gocritic flagged the `nil` body on `http.NewRequestWithContext`; switched to `http.NoBody` (the idiomatic singleton for empty request bodies). Same fix in tests using `httptest.NewRequest`.

### Scope discipline

PR2 spec called for ~150 lines + tests. Final tally: ~210 lines of production code (discovery.go) + ~310 lines of tests + ~70 lines of config/chart plumbing + ~50 lines of dev-values updates. Over the 150-line estimate but every piece is load-bearing for PR3 (verification_uri uses brokerURL) and PR4 (broker-signed tokens use brokerURL as issuer).

### Next

PR3 — broker device flow (RFC 8628). New file `device.go` with `/device/authorize` + `/device` (HTML form + `/auth/login` redirect glue) + `/device/token` polling. ~600 lines + tests, ~2 days. Will exercise the route registration path that PR2 sets up; will also be the first time the broker holds short-lived per-flow state in-memory (device_code map with janitor goroutine).

### Open question to resolve before PR3

mock-oauth2-server device-code support — plan §Open Questions item 1. Need to check `navikt/mock-oauth2-server` v2.x source for `/device_authorization` endpoint before designing the kind Stage 5 harness in PR7. Not blocking PR3 itself (broker-side tests use a fake verifier), but if mock-oauth2-server doesn't support it, the kind end-to-end story diverges.


## End of session — PR2 merged, PR3 plan written

- PR2 (#136, broker discovery doc) merged after two rounds of CodeRabbit feedback. Final state included `Server.PublicURL` config field (required, trimmed, absolute-URL validated), `Discovery` type with 5-min TTL caching + refresh-mutex stampede protection + hard fetch deadline, route registration on `/.well-known/openid-configuration`, 16 broker tests + 5 helm-unittest cases.
- Coderabbit lessons worth saving:
  - `t.Fatalf` inside spawned goroutines panics that goroutine without releasing pending channel sends. Concurrency tests need an error-returning helper that surfaces failures through the channel; the main goroutine asserts after `<-`. Wrote `serveAndDecodeErr` as the canonical pattern for this kind of test.
  - When a constructor accepts an injected `*http.Client` via config struct, that client may not have a `Timeout` set. Wrap the fetch context with `context.WithTimeout` inside the operation so the deadline holds regardless of what the caller injected. Belt-and-suspenders on top of the default client's own timeout.
  - `helm install` regression tests in CI (`test-upgrade-wipe.sh` invocation) need explicit `--set` for every newly-required chart value. Adding a required field to `values.yaml` without updating CI passes helm-unittest but fails the actual install regression. Worth grepping `.github/workflows/` for `helm install` calls when adding a required field to any chart.
  - README install examples count as a test surface — coderabbit flagged the missing `--set server.publicURL` in the dev-install README block. Update README install commands alongside the chart change.
- PR3 plan published to `/plans/universe-onboarding-pr3.md` with full architecture, sub-task breakdown, open design questions, and tomorrow-morning resume steps. Index updated to surface it.
- Two design questions deferred to tomorrow's session:
  1. `Verifier.Exchange` signature change (lean: change it, three call sites, clean refactor) vs. parallel `ExchangeWithTokens` method. Latter is messier and we'd refactor anyway in PR4.
  2. RFC 8628 `slow_down` interval-increment policy. Lean: not for PR3 (RFC says MAY, not MUST); revisit if a customer reports IdP abuse.
- Tomorrow's first move: `mark_fetch /plans/universe-onboarding-pr3.md`, confirm the two open questions, branch from main, start at Step 1 (Verifier refactor) as its own commit.


## PR3 implementation — broker device flow (RFC 8628)

Picked up PR3 cold from `/plans/universe-onboarding-pr3.md`. Both deferred design questions confirmed:
1. `Verifier.Exchange` signature changed to return `ExchangeResult` (Claims + RawIDToken + AccessToken + Expiry) — three call sites touched.
2. `slow_down` policy is "enforce configured interval, no incremental bump" — pure minimum-interval check in `deviceStore.Poll`.

### Changes

- `tools/demarkus-broker/internal/broker/oidc.go` — added `ExchangeResult` struct; `Verifier.Exchange` returns it.
- `tools/demarkus-broker/internal/broker/oidc_test.go` — `fakeVerifier` gained `rawIDToken`/`accessToken`/`expiry` fields and the new return type.
- `tools/demarkus-broker/internal/broker/server.go` — `Server` gained `deviceStore`. `Routes()` registers `POST /device/authorize`, `GET /device`, `POST /device`, `POST /device/token`. The first and last sit behind `ipRateLimit`; `/device` form has no rate-limit middleware. `authCallback` gained a device-cookie early-return dispatch.
- `tools/demarkus-broker/internal/broker/device.go` (new) — four handlers + `deviceCallback` (the device branch of `/auth/callback`) + `RunDeviceJanitor` lifecycle + `clearDeviceCookie` helper. HTML templates embedded via `embed.FS`. ~280 lines.
- `tools/demarkus-broker/internal/broker/device_store.go` (new) — `deviceStore` state machine. Methods: `Authorize`, `LookupByUserCode`, `LookupByDeviceCode`, `Bind`, `Deny`, `Poll`, `Sweep`. User-code alphabet 30 chars (excludes `0 1 I L O U`); 8-char codes formatted with a hyphen for display, canonicalized to no-hyphen-uppercase at lookup. ~290 lines.
- `tools/demarkus-broker/internal/broker/templates/device_form.html`, `device_done.html` (new) — server-rendered, embedded into the binary.
- `tools/demarkus-broker/internal/broker/config.go` — `ServerConfig.DeviceCodeTTL` + `DevicePollInterval` with defaults (10m / 5s). Extracted `ServerConfig.applyDeviceFlowDefaults` to keep `Config.validate` under the gocyclo budget.
- `tools/demarkus-broker/main.go` — calls `srv.RunDeviceJanitor(sweepCtx)` in the existing shutdown WaitGroup so the goroutine tears down with the sweeper.
- `tools/demarkus-broker/internal/broker/device_store_test.go` (new) — Authorize, Lookup, Bind, Deny, Poll (incl. slow_down + expiry), Sweep, canonicalizer round-trip, alphabet invariants. ~330 lines.
- `tools/demarkus-broker/internal/broker/device_test.go` (new) — handler tests (authorize, form GET/POST, token states), `TestAuthCallbackUnchangedWithoutDeviceCookie` regression guard, full end-to-end happy-path integration test, IdP-deny integration test, janitor cancel test. ~510 lines.

### Verification

- `go test -race ./internal/broker/` → green (~6s) across all 30 existing + new tests.
- `helm unittest .` (broker chart) → 55/55 still green; no chart changes in PR3.
- `bash pre-commit.sh` → format + vet + lint clean. Initial run flagged 5 lint issues (gocritic hugeParam on `Bind`, gocyclo on `validate`, two `minmax`, one `rangeint`) — all addressed: `Bind` takes `*ExchangeResult`, validate extracted helper, `max()` replacements, `for range N` loop.

### Design decisions that landed differently from the plan

- **`deviceStore.Bind` takes `*ExchangeResult` (not value).** Plan showed it as value. gocritic's `hugeParam` flagged the 120-byte value at every call site. Pointer keeps the API as cheap as value semantics without changing ownership (`*result` is copied into `deviceCodeState.Result`). All call sites pass `&exchange` / `&ExchangeResult{...}`.
- **User-code alphabet is 30 characters, not 32.** Plan said "32-char alphabet (no I, L, O, 0, 1, U)" but 36 − 6 = 30. Used 30; collision space at TTL-aligned issue rates is still negligible (30^8 ≈ 6.6×10^11). Crockford base32's standard alphabet preserves `0` and `1`; the plan's exclusion list was the load-bearing constraint, not the alphabet size.
- **Cookie clearing must happen BEFORE `WriteHeader`.** First pass used `defer s.clearDeviceCookie(w)`. Integration test caught it: `WriteHeader` flushes headers, so the deferred SetCookie is silently dropped. Restructured to explicit `clearDeviceCookie(w)` calls at each early-return + before the render.
- **State cookie is also cleared on the device branch.** Plan mentioned device cookie only. Mirrored the browser path's state-cookie clear (Path=`/auth/callback`, MaxAge=-1) so a replay can't reuse the nonce.
- **Janitor is a plain `time.NewTicker` loop, no leader election.** Per-replica in-memory state means each replica sweeps its own store; no shared resource to elect on. Lifecycle shares `sweepCtx` with the Kubernetes-side Sweeper for a single shutdown signal.
- **Open Question 1 (user-denial branch): YES, translate.** `/auth/callback?error=...` is detected before the Exchange call. `deviceStore.Deny(deviceCode)` is invoked and the done page renders. Polling client sees `access_denied` on next poll instead of waiting for `expired_token`. ~10 lines.
- **Open Question 7 (client_id requirement on /device/authorize): accept-but-ignore.** `POST /device/authorize` reads `client_id` from the form (RFC-compliant client), logs at debug, but does not gate on it. Broker is the only relying party in this flow.

### Risks specific to PR3 that did NOT bite

- Cross-cookie path confusion (state cookie at `/auth/callback`, device cookie at `/`) — guarded by `TestAuthCallbackUnchangedWithoutDeviceCookie`, regression-safe.
- Verifier interface refactor breakage — `rg` confirmed only three call sites; refactor was a single commit's worth of mechanical changes.
- In-memory store loss on broker restart — documented in `deviceStore` doc comment; acceptable for 10-minute TTL.

### Next

PR3 ready to open. No commits made — Fritz handles those. Branch is still `main`; he'll cut a `feat-tools-broker-device-flow` (or similar) branch before committing. PR4 (broker-signed id_tokens + refresh tokens via `ExchangeResult` plumbing) is the natural next sub-plan.


## PR3 merged (#137) — review lessons

CodeRabbit ran two rounds before approving. Lessons worth saving:

### Security / correctness regressions caught
- **Stale-device-cookie dispatch hijack.** First-pass dispatch on `/auth/callback` keyed on the device cookie alone — a cookie that lived `DeviceCodeTTL` long. Abandoning the flow before `/auth/callback` left the cookie in the jar; a later legitimate browser `/auth/callback` would silently route through `deviceCallback` and bind the wrong grant. Fix: extended the signed `State` struct with `DeviceCode string`, consume the device cookie at `/auth/login`, dispatch on `state.DeviceCode` in `/auth/callback`. The cookie is now a one-shot pass-through; the State cookie is the authoritative signal. Pattern worth remembering: **never dispatch on an ambient cookie when a signed state value is available**.
- **Origin-only CSRF check passes scheme-mismatched origins.** `http://broker.example.com` matched `r.Host == "broker.example.com"` even though that's cross-origin. Fix: compare scheme+host, derive expected scheme from X-Forwarded-Proto / r.TLS. Pattern: **scheme is part of origin in the spec; host-only same-origin checks are broken by construction**.
- **`Deny` on Exchange failure.** Conflated user-denied-at-IdP with broker-side transient failure. A network blip during `Exchange` permanently marked the device_code as `access_denied` to the polling client. Fix: only the explicit `?error=` query branch maps to Deny; everything else leaves the grant pending so the client retries or sees truthful `expired_token`. Pattern: **terminal states must reflect what actually happened, not the closest convenient enum value**.
- **Bearer tokens cacheable by default.** `POST /device/token` success response didn't carry `Cache-Control: no-store`. Standard OAuth2 §5.1 posture. Easy miss when writing JSON helpers.
- **RFC 8628 `client_id` is REQUIRED, not optional.** The plan §Open Question 7 leaned "accept-but-ignore". CodeRabbit pushed back citing the spec; the right answer is **require presence, don't validate content** — filters malformed clients without putting weight on a useless registration check.

### Process lessons
- **Header writes after `WriteHeader` are silently dropped.** First-pass cookie clearing in `deviceCallback` used `defer s.clearDeviceCookie(w)` — the defer fired after `renderDeviceDone` had already flushed headers, so the Set-Cookie never landed. Integration test caught it. Always set headers BEFORE the first body write or `WriteHeader` call.
- **Sed-with-newlines breaks on multiline calls.** Used sed to rewrite `store.Bind(deviceCode, ExchangeResult{...})` to `store.Bind(deviceCode, &ExchangeResult{...})` and missed one site where the struct literal spanned multiple lines. Always grep after a sed-based refactor.
- **`gocritic hugeParam` flags 120-byte value parameters.** Took `ExchangeResult` by value at `deviceStore.Bind`; lint flagged. Switched to `*ExchangeResult` with a copy-into-state-on-store-side. Cleaner API anyway.
- **Test config divergence from production validation.** Tests construct `Config{}` directly without calling `validate()`, so new required fields (DeviceCodeTTL, DevicePollInterval) need fallback defaults in `NewServer` as well as `validate()`. Production-only validation paths surprise tests.
- **CSRF gate compatibility with Go's http.Client.** Go's client doesn't send `Origin` on POST by default. The check is "if Origin present, must match" — non-browser clients (curl, Go's client) hit the no-Origin branch and pass, which is correct (CSRF needs a puppeted user-agent).

### Final state
- 8 commits squashed-merged as `d0697b9`. ~1100 LOC production + ~870 LOC tests across `device.go`, `device_store.go`, `device_test.go`, `device_store_test.go`, templates, config, server, oidc, main.
- All four PRs of the universe-onboarding plan now merged in order: #135 (PR1), #136 (PR2), #137 (PR3). Plan §PR4 next.