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.