prole/docs/plans/onboarding-tdd-phase-a.md
chrisfu 00f0ebec07 Merge claude/crazy-bose-fec256 into main
Bringing the long-running session-feature branch back into main in one
deliberate sweep. The branch carried the cluster work that's been live for
weeks (cross-cluster CNPG metrics, Grafana w/ Google OAuth, supabase
oauth2-proxy, cluster recovery, pg.0.knoe.dev + per-engineer onboarding,
GCS-backed CNPG backups via Workload Identity, the env-contamination
guard, the Junie brief queue, the cnpg-grafana CSRF + memory-request
fixes from today), while main accumulated Junie's parallel knoe-auth
Phase 2 OIDC work (full provider surface: discovery, authorize, token,
userinfo, JWKS, RS256 signing, code exchange, session services).

Key decision: the two branches did COMPETING rebrands off the same
starting point (5ba9b63, 2026-04-27):

  - claude branch (commit b355855, earlier): org.prole.authority.* →
                                              dev.knoe.auth.*
                                              (artifact renamed to
                                              knoe-auth.jar)
  - main (commit 9daa94b, recent): org.prole.authority.* →
                                    dev.knoe.authority.*
                                    (kept "authority" artifact name)

dev.knoe.auth wins: cluster runs from this name, the Maven artifact is
already knoe-auth.jar, and the broader rename is the documented
namespace direction (per ~/.claude/projects/-Users-chrisfu-dev-knoe-db/
memory/MEMORY.md). All of main's recent Phase 2 OIDC content was ported
from authority/src/.../dev/knoe/authority/ into
authority/src/.../dev/knoe/auth/ with package declarations rewritten.

== File-level resolution summary ==

Textual conflicts (4):

  authority/pom.xml
    - Took our artifactId="auth"
    - Took our branch's removal of spring-security-kerberos-client
      (verified: Junie's Phase 2 OIDC code does not import it; the dep
      was already-dead config)

  docs/pipeline-phases.md
    - Took our branch's "Phase 1 not started" status. Main had a
      misplaced "✅ Complete" with a knoe-auth-Phase-1 commit ref
      in the autobuild Phase 1 section — different domain.

  docs/plans/knoe-auth-round-1.md
    - Took our branch's dev.knoe.auth file table (vs main's
      dev.knoe.authority listing). Pure rename mismatch.

  supabase/helm/knoe-supabase/templates/kong/config.yaml
    - Took our branch's onboard route + plain dashboard wiring.
      Main had an oauth2proxy.enabled toggle that put oauth2-proxy as
      a Kong upstream — but the deployed architecture (commit 25f1b2e)
      has oauth2-proxy in FRONT of Kong, not behind. Main's wrapper
      reflected an architecture that was never deployed.
    - Took our branch's removal of basic-auth from dashboard route
      (queue #15 brief still tracks the matching values.yaml /
      kong/deployment.yaml cleanup).

Java tree reconciliation (44 file-pairs):

  20 dual-path source files + 2 dual-path tests
    Body-identical between main's authority/ and our branch's auth/
    after stripping package decls — main's commit 9daa94b was a pure
    rebrand. Took our branch's auth/ version for all 22.

  8 main-only source files (Phase 2 OIDC), ported into auth/:
    web/JwksController.java
    web/OidcAuthorizeController.java
    web/OidcDiscoveryController.java
    web/OidcTokenController.java
    web/OidcUserInfoController.java
    session/OidcCodeService.java
    session/OidcTokenService.java
    session/SessionService.java

  12 main-only test files, ported into auth/:
    HealthControllerTest.java
    enroll/EnrollValueTypesTest.java
    enroll/EnrollmentControllerTest.java
    enroll/TotpServiceTest.java
    kerberos/KadminClientTest.java
    kerberos/KerberosSpnegoResultTest.java
    web/LoginControllerTest.java
    admin/AdminControllerTest.java
    user/PrincipalNormalizerTest.java
    regression/IdentityRegressionTest.java
    session/OidcCodeServiceTest.java
    session/SessionServiceTest.java

  Port mechanics: read main:authority/...<file> via git show, then sed
  rewrite `package dev.knoe.authority` → `package dev.knoe.auth` and
  `import dev.knoe.authority` → `import dev.knoe.auth`. Body content
  unchanged.

  authority/src/main/java/dev/knoe/authority/ — DELETED (duplicate)
  authority/src/test/java/dev/knoe/authority/  — DELETED (duplicate)

== Verification ==

- grep -rln '<<<<<<<' across .java/.md/.yaml/.yml/.sh/.xml/.tpl: clean
- find authority/src -path '*/dev/knoe/authority*': empty (subtree gone)
- grep 'package dev.knoe.authority' across repo: clean
- bash -n install.sh deploy.sh etc/preflight_kubecontext.sh: clean
- git ls-files -u | wc -l: 0 unmerged paths
- helm lint supabase/helm/knoe-supabase: pre-existing failure on
  studioIngress.enabled undefined in values.yaml (introduced by Junie
  on main; unrelated to this merge — flagging as follow-up).

== Followups (carried into TODO ranked queue or noted here) ==

  - helm lint failure: studioIngress block in values.yaml is missing
    enable flag; templates/studio/{ingress,oauth2proxy-deployment,
    oauth2proxy-service}.yaml all reference studioIngress.enabled with
    no default. Pre-existing on main; not introduced by this merge.
  - The five Junie briefs filed on this branch are now reachable from
    main at docs/plans/junie/{02,06,07,13,15}-*.md. Junie can pick them
    up in any order.
  - knoe-auth Phase 2 OIDC source (now at dev.knoe.auth.*) is not yet
    deployed to the cluster. Deployment is its own task.
  - The branch claude/crazy-bose-fec256 stays in place (worktree at
    .claude/worktrees/crazy-bose-fec256 may have ongoing context for
    Claude Code sessions). Safe to delete once next session starts
    cleanly from main.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-01 16:39:10 -07:00

323 lines
15 KiB
Markdown

# Phase A — TDD design brief for the onboarding system
> **For Junie in Sonnet mode.** This is the design phase. You produce a
> design doc (`docs/plans/onboarding-tdd.md`) plus one hard test fixture.
> Phase B (Codex) consumes the design and writes the bulk of the test bodies.
> **Do not write production-quality test files in Phase A** — write the spec
> they'll be written from.
---
## 1. Why we're slowing down
Five commits landed in rapid succession over the last sitting, none with
automated test coverage:
| Commit | Surface |
|---|---|
| [`f805404`](#) | `oauth2-proxy --skip-auth-route` for Supabase API surfaces (unifies `db.0.knoe.dev` for Studio + SDK) |
| [`79a3f76`](#) | `GRANT anon, authenticated, service_role TO supabase_storage_admin`; oauth2-proxy `--pass/set-authorization-header` removed; `/support` 302→mailto stop-gap |
| [`8c2956b`](#) | `pg_stat_statements` moved from `public` to `extensions` schema (durable across all 4 deploy modes) |
| [`317b20b`](#) | TCP LB `pg.0.knoe.dev` (34.106.156.196), CNPG cert SAN, `knoe_developer` role, `chrisfu` + `ron` engineer roles, pg_hba tightened |
| [`313ee88`](#) | Reusable engineer onboarding: `etc/onboard_engineer.sh`, knoe-onboard nginx pod, `/onboard.html` Kong route, membership-based pg_hba (`+knoe_developer`), committed CA cert |
About to onboard the second human engineer (`ron`) against this stack.
Standing decision was: **don't add a user, even of one, before we have tests
covering the surfaces a user touches**. This brief is that decision being
acted on.
---
## 2. What you produce in Phase A
Three artifacts, in order:
1. **The f-string fix at `knoe/core/ops/cloudnative_pg.py:1372`** committed
first. 30 test-collection errors block the Phase 1 pipeline; until that
lands, no test we write has anywhere to run. See top of
[`docs/TODO.md`](../TODO.md). Trivial: extract `field.replace('.', '\\.')`
to a variable before the f-string. **One commit, message starts
`fix(test):`**.
2. **`docs/plans/onboarding-tdd.md`** — the design doc. This is the main
deliverable. It must be detailed enough that Codex (Sonnet's successor in
Phase B, no shared memory) can write every test body from it without
needing to make architectural calls. Sections required:
- Test taxonomy (Tier 1 / Tier 2 / Tier 3) with one-line acceptance per
test — not bash code, but a precise spec of what's being asserted.
- Fixture architecture decision (k3d-per-run vs. shared persistent vs.
hybrid) with the tradeoff analysis recorded.
- Browser-test framework decision (Playwright vs. Puppeteer vs. ...) with
why.
- Directory layout under `tests/` and how it slots into the existing
pytest structure (read [`tests/test_render_supabase_hostname.py`](../../tests/test_render_supabase_hostname.py)
and [`tests/test_repair_update_and_supabase_flags.py`](../../tests/test_repair_update_and_supabase_flags.py)
for the conventions to match).
- CI integration — these tests slot into Phase 1 of
[`docs/pipeline-phases.md`](../pipeline-phases.md). Document the new run
config(s), and whether the integration tier needs its own marker (e.g.
`pytest -m integration`) so unit tests stay fast for local dev.
- Definition of done per tier — what passes count as the boundary covered
vs. work-in-progress.
3. **The first hard fixture, working.** Specifically: a deterministic way to
simulate "external client connecting to the cluster" inside CI. This is
the single hardest design call in Phase A; everything in Tier 1 hangs off
it. Get it functional and verified — even if it's just a 30-line
`conftest.py` helper — so Codex can rely on it from day one. **One commit,
message starts `test(infra):`**.
You do **not** write test bodies in Phase A. You write the spec and the
hardest fixture. Resist the temptation to land a few "easy" tests — Codex
gets a much cleaner working environment if Phase A's output is purely
design + fixture.
---
## 3. Test taxonomy (the spec to flesh out)
This is what the conversation that produced this brief settled on. Use it as
the skeleton for `docs/plans/onboarding-tdd.md` Section "Test taxonomy", but
you're expected to refine it: add edge cases, kill duplicates, surface gaps.
### Tier 1 — Security boundaries (must-have before more onboards)
The wall protecting the database from the public internet. **No test = no
proof.** Hard-gating on the team's deploy pipeline.
1. **pg_hba rejection matrix.** For each axis cross-product:
- **Source**: external IP, internal cluster IP (10.0.0.0/8 simulated)
- **Encryption**: TLS, plaintext
- **Role**: `chrisfu`, `ron`, a fresh-onboarded `knoe_developer` member,
`postgres`, `supabase_admin`, `anon`, a non-member of `knoe_developer`
with valid LOGIN + password
- The matrix has 36 cells minimum. Acceptance: each cell asserts the
expected `connect / reject / auth-fail` outcome.
2. **`+knoe_developer` membership semantics is load-bearing.** A role granted
`knoe_developer` is reachable externally; revoking the grant immediately
removes external reachability. A role with LOGIN + password but NOT in the
group cannot connect externally even with valid creds. (This is the
reusability lynchpin from commit `313ee88`; a regression here means
"everyone with a postgres password is reachable from the public internet".)
3. **oauth2-proxy `--skip-auth-route` regex tightness.** For each declared
route (`/auth/v1/.*`, `/rest/v1/.*`, `/realtime/v1/.*`, `/storage/v1/.*`,
`/functions/v1/.*`, `/graphql/v1/.*`, `/pg/.*`, `/onboard.html`, `/support`):
- The exact path skips Google sign-in.
- Variant paths that *should* be gated (e.g. `/api/profile`,
`/Auth/v1/foo`, double-slash, URL-encoded traversal probes) **do** get
redirected to Google.
- Each declared route is exercised with at least one negative test that
would fail if the regex is loosened by accident.
4. **`hostnossl reject` precedence.** A property test: any pg_hba rule order
that places a permissive rule (e.g. `host all all 10.0.0.0/8`) before a
`hostnossl ... reject` line, and would let an external plaintext
connection match the permissive rule, must FAIL the test. This is the
regression detector for the `externalTrafficPolicy: Cluster -> Local` bug
we hit in Phase 1 — preserve the lesson.
### Tier 2 — Onboarding flow correctness
CI-gating (block merges) but not deploy-gating.
5. **`onboard_engineer.sh` is idempotent.** Run twice in a row → one role
exists with the second run's password. `--rotate` on a non-existent user
→ clear error, no partial state. `--revoke` on a connected session → role
dropped, sessions terminated cleanly.
6. **`onboard_engineer.sh` input validation.** Username regex; email is
`*@knoey.com`; rejection of shell-injection / SQL-injection probes; clear
error messages.
7. **Reveal page renders correctly.** Headless-browser tests:
- `onboard.html#user=test&pw=Zm9v&exp=2099-01-01T00:00:00Z` → password
decoded and shown.
- `exp` in the past → expired-state UI.
- Missing required fields → error UI.
- Copy buttons populate clipboard.
- On render, `history.replaceState` strips the fragment from the URL.
- The `pw` is base64-decoded correctly even with URL-safe `-_` chars.
8. **CA cert in repo matches live CA.** SHA256 fingerprint of
`etc/knoe-db-ca.crt` matches `kubectl get secret knoe-db-ca`. When CNPG
rotates, this test fails loudly instead of engineers silently 502'ing.
### Tier 3 — Adjacent surfaces we touched
Regression coverage. CI-gating.
9. **Studio internal paths still gate via Google.** `/`, `/api/profile`,
`/api/database/*` → 302 to `accounts.google.com`.
10. **`db.0.knoe.dev/storage/v1/bucket` reachable as service-role.** The
GRANT we landed in `79a3f76` makes this work; pin it.
11. **`pg_stat_statements` lives in `extensions` post-bootstrap.** PG18
docker fixture for k3d/k3s/min; live-cluster check for gke. Database
advisor's `public.pg_stat_statements` warning stays cleared.
12. **`api.0.knoe.dev` alias still works** until [`docs/TODO.md`](../TODO.md)
follow-up #14 explicitly retires it.
---
## 4. Decisions you must make in Phase A
These are the architectural calls Codex needs nailed down before writing
test bodies. Sonnet's reasoning bandwidth is concentrated here.
### 4a. Fixture architecture for "external client"
Tier 1 needs to assert "an external client cannot connect with X". Options:
| Option | Pros | Cons |
|---|---|---|
| **k3d cluster + sidecar pod with `externalTrafficPolicy: Local` LB** | Closest to prod (real LB IP, real pg_hba) | Slow to spin up; LB IP allocation in k3d is non-trivial |
| **k3d cluster + iptables rule simulating "external" via a labeled namespace's pod IP space** | Faster; deterministic | Diverges from prod; risk that test passes but real LB doesn't |
| **Live cluster (gke `knoe-dev-cnpg-0`) + ephemeral test roles** | Highest fidelity | Pollutes prod with test roles; risk of leaving them around; can't run in CI without GCP creds |
| **Hybrid**: k3d for most assertions; one nightly job against live cluster for the fidelity-sensitive few | Best of both | Two test rigs to maintain |
You decide. Document the tradeoff. Note that CLAUDE.md establishes k3d as
the canonical local mode, so the bias should lean k3d unless you find a
real reason otherwise.
### 4b. Browser-test framework
Tier 2.7 needs headless-browser support. Candidates: Playwright (Node),
Puppeteer (Node), Selenium (multi-language), Cypress (Node, e2e-focused).
Read [`tests/`](../../tests/) — current Python pytest-only. Adding a Node
toolchain just for browser tests is a real cost. Consider:
- Are there ~5 browser-tested behaviors total? Then a dedicated framework
may be overkill — a pytest test that uses `selenium` (Python) + a headless
Chrome may be enough.
- Are there ~25? Dedicated framework worth the cost.
### 4c. k3d-per-run vs. shared persistent local cluster
| | Per-run | Shared persistent |
|---|---|---|
| **Speed** | Slow (60-90s spinup) | Fast (~0s) |
| **Determinism** | High | Low (tests can pollute each other) |
| **Local dev UX** | Friction | Zero friction |
| **CI** | Required for parallel runs | Doesn't work for parallel |
You decide. Docs/pipeline-phases.md would benefit from your call being
recorded explicitly, since Phase 2 of that pipeline will face the same
question.
### 4d. Acceptance criteria per tier
What counts as "Tier 1 done"? Specifically:
- Coverage threshold (e.g. all 36 pg_hba cells; all 9+ skip-auth-route
cases)?
- Mutation testing pass rate?
- Just "the spec items are all green"?
Document the answer per tier. **Tier 1's bar should be the highest** — it's
the security wall.
### 4e. Retroactive coverage policy
The five commits (`f805404` through `313ee88`) landed without tests. We can:
- **Cover them all in this round.** The Tier 1 + 2 + 3 spec already covers
most of the surface area, but not all (e.g. `pg_stat_statements` schema
move's behavior on a NEW deploy isn't directly tested by the spec; the
storage GRANT's effect on bucket listing isn't directly tested).
- **Cover only the onboarding system in this round; queue the rest for
follow-up.**
I lean toward "cover them all" — the test debt only grows otherwise — but
the call is yours given you can see the actual scope better than I drafted.
---
## 5. Files to read first
In order:
1. [`docs/plans/README.md`](README.md) — the team's plan-doc conventions.
2. [`docs/pipeline-phases.md`](../pipeline-phases.md) — where these tests
slot into the autobuild pipeline.
3. [`docs/TODO.md`](../TODO.md) — current queue and the f-string blocker
you'll fix first.
4. [`tests/test_render_supabase_hostname.py`](../../tests/test_render_supabase_hostname.py)
and [`tests/test_repair_update_and_supabase_flags.py`](../../tests/test_repair_update_and_supabase_flags.py)
— existing test patterns to follow.
5. [`docs/db-access.md`](../db-access.md) and [`docs/onboarding.md`](../onboarding.md)
— what the onboarding system actually does.
6. [`etc/onboard_engineer.sh`](../../etc/onboard_engineer.sh) — the script
under test.
7. [`deploy/gcp/gke/knoe-onboard.yaml`](../../deploy/gcp/gke/knoe-onboard.yaml)
— the static page (HTML embedded in the ConfigMap).
8. [`deploy/gcp/gke/knoe-db.yaml`](../../deploy/gcp/gke/knoe-db.yaml) —
pg_hba block (the surface most needing test coverage).
9. [`deploy/gcp/gke/oauth2-proxy-deployment.yaml`](../../deploy/gcp/gke/oauth2-proxy-deployment.yaml)
— `--skip-auth-route` args.
10. [`CLAUDE.md`](../../CLAUDE.md) §"install.sh / deploy.sh pre-flight" —
env-contamination warning is a category-mate of the boundary tests
you're designing.
---
## 6. Definition of done for Phase A
When all four are true:
- [ ] `knoe/core/ops/cloudnative_pg.py:1372` f-string fix committed; pytest
collection succeeds repo-wide (`pytest --collect-only` returns clean).
- [ ] `docs/plans/onboarding-tdd.md` exists, addresses every section in
Sec. 2.2 above, and is reviewable by chrisfu.
- [ ] The first hard fixture (Sec. 4a's choice, implemented) is committed
with at least one Tier 1 test that uses it green.
- [ ] You have **not** committed bodies for any other tests. Codex's job.
---
## 7. Out of scope for Phase A (Codex picks up)
- Writing test bodies for Tiers 1, 2, 3 (except the one Tier 1 test that
validates the fixture from Sec. 4a is functional).
- CI wiring beyond what's needed for the f-string fix.
- Mutation-testing setup, if your acceptance criteria call for it.
- Infrastructure for live-cluster integration tests (only relevant if your
Sec. 4a decision calls for one).
- Updating engineer-facing docs to mention the test suite. Codex updates
`docs/onboarding.md` and `docs/db-access.md` after Phase B lands.
---
## 8. Handoff format to Phase B
Codex's brief (separate doc, will be drafted by chrisfu after this lands)
will reference:
- `docs/plans/onboarding-tdd.md` — the spec Codex implements.
- The fixture you committed in Sec. 4a — Codex's tests use it.
- Your acceptance criteria per tier — Codex knows when each tier is done.
- The directory layout you defined — Codex's PRs land in the right places.
So `docs/plans/onboarding-tdd.md` needs to be **prescriptive**, not
"considerations" or "options". Codex isn't going to make architectural
calls; it'll execute against what you wrote. Land the design.
---
## 9. Working notes
- **You can ask chrisfu questions.** If you hit a decision that genuinely
needs human input (e.g. "should we let CI provision GCP credentials" —
policy call, not technical), surface it rather than guessing. But the
questions in Sec. 4 are yours to answer; don't punt them upward.
- **The plan in this brief is also wrong somewhere.** The taxonomy is my
best draft from a conversation; you may find that some Tier 1 items are
redundant with Tier 2, or that there's a Tier 0 ("does the cluster even
start?") missing. Calling that out IN the design doc is part of the
deliverable.
- **Slow is smooth.** This brief deliberately scopes Phase A to design +
one fixture so the design isn't rushed. Resist scope creep into Phase B.
You're starting fresh in a Sonnet context. Read the files in Sec. 5, then
the existing plans in `docs/plans/`, then come back here. Good design lands.