From 5f516a0fbbfbe921384828b3d50603f145cd5576 Mon Sep 17 00:00:00 2001 From: tegwick Date: Wed, 9 Sep 2026 23:23:48 +0200 Subject: [PATCH] Make the static-registration precondition a checked condition informed-decision asked for the caveat under which a registration-bound human tenant is safe to be a condition of the capability rather than reasoning in a message, so a future change to registration policy has to confront it. They were right that it was only prose: the contract stated it, a separate test asserted registration_endpoint is absent, and nothing connected the two -- so a session adding dynamic registration would have seen a test about discovery metadata, not a warning about relabelling users. The test asserts both halves together: that a client-declared tenant is issued, and that dynamic registration is not advertised. Whichever is removed first, the failure points at the other. The message carries the reasoning rather than the observation -- anyone able to register a client could relabel the users who log in through it -- and names the two ways out. Verified in both directions rather than assumed: advertising a registration_endpoint fails it with the escalation message, and neutering humanTenant fails it with the message saying to remove the guard along with the capability it protects. This is not a vote on the tenant question. It makes one option's precondition checkable; if option 1 lands, the capability and this guard are removed together. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NV9oijZukGyGbRQGGKnK4P Assistant: claude-code Assistant-Model: opus Assistant-Process: 713576@bnt-lap001 Assistant-Session: 384c511d-9bce-4cb8-a676-2aef6c0c8df6 --- .../server/oidc/tenant_precondition_test.go | 66 +++++++++++++++++ .../KEY-WP-0030-tenant-precondition-guard.md | 70 +++++++++++++++++++ 2 files changed, 136 insertions(+) create mode 100644 src/internal/server/oidc/tenant_precondition_test.go create mode 100644 workplans/KEY-WP-0030-tenant-precondition-guard.md diff --git a/src/internal/server/oidc/tenant_precondition_test.go b/src/internal/server/oidc/tenant_precondition_test.go new file mode 100644 index 0000000..f5bb870 --- /dev/null +++ b/src/internal/server/oidc/tenant_precondition_test.go @@ -0,0 +1,66 @@ +package oidc_test + +import ( + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "keycape/internal/domain" + "keycape/internal/server/oidc" +) + +// KEY-WP-0030. A client registration may declare the tenant its users' tokens +// carry (humanTenant). That is safe for exactly one reason: registrations here +// are static and deployment-owned. A self-service client able to name its users' +// tenant would be a straightforward escalation — register a client, declare the +// landlord zone, and any user who logs in through it is labelled with it. +// +// informed-decision asked for that caveat to be a condition of the capability +// rather than a paragraph, so that a future change to registration policy has to +// confront it. This test is that condition: it fails if the exclusion is ever +// lifted while the capability is present, and its failure message says what to do +// about it rather than merely reporting the endpoint. +// +// The two halves are asserted together on purpose. Whichever is removed first, +// the test points at the other. + +func TestRegistrationBoundTenantRequiresStaticRegistration(t *testing.T) { + // Half one: the capability exists. If this stops holding, the precondition + // below is no longer load-bearing and this test can go with it. + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + h.ClientConfig["test-client"].Tenant = "tenant:platform" + verifier := "test-verifier" + code := seededSession(sessions, verifier) + w := httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, url.Values{ + "grant_type": {"authorization_code"}, "client_id": {"test-client"}, + "code": {code}, "code_verifier": {verifier}, "redirect_uri": {seededRedirectURI}, + })) + if w.Code != http.StatusOK { + t.Fatalf("client-declared tenant no longer issues (status %d): if the capability was "+ + "removed deliberately, remove this test too — the dynamic-registration "+ + "precondition below only exists to protect it", w.Code) + } + if got := parseJWTPayload(t, decodeTokenResponse(t, w.Body.String())["access_token"].(string))["tenant"]; got != "tenant:platform" { + t.Fatalf("client-declared tenant resolved to %v, want tenant:platform", got) + } + + // Half two: the precondition holds. Dynamic client registration must stay + // absent while any client can declare a tenant. + doc := discoveryDoc(t, oidc.DiscoveryConfig{ + Issuer: "https://auth.netkingdom.local", + AuthorizationEndpoint: "https://auth.netkingdom.local/authorize", + TokenEndpoint: "https://auth.netkingdom.local/token", + JWKSUri: "https://auth.netkingdom.local/jwks", + }) + if _, advertised := doc["registration_endpoint"]; advertised { + t.Fatal("dynamic client registration is advertised while a client registration " + + "may declare its users' tenant. That combination lets anyone who can register " + + "a client relabel the users who log in through it into a zone of their choosing. " + + "Resolve it deliberately: either drop the registration-bound tenant, or gate it " + + "so only statically configured registrations may declare one. See " + + "docs/tenant-claim-contract.md, 'How a human token's tenant is resolved'.") + } +} diff --git a/workplans/KEY-WP-0030-tenant-precondition-guard.md b/workplans/KEY-WP-0030-tenant-precondition-guard.md new file mode 100644 index 0000000..583961a --- /dev/null +++ b/workplans/KEY-WP-0030-tenant-precondition-guard.md @@ -0,0 +1,70 @@ +--- +id: KEY-WP-0030 +type: workplan +title: "Make the static-registration precondition a checked condition" +domain: infotech +repo: key-cape +status: finished +owner: claude +topic_slug: tenant-precondition-guard +created: "2026-09-09" +updated: "2026-09-09" +--- + +`informed-decision` replied on KEY-WP-0013-T05 and asked for one thing that is +ours to do: the caveat under which a registration-bound human tenant is safe +should be "a condition of the capability, so a future change to registration +policy has to confront it", not reasoning left in a message. + +They are right that it was only prose. `docs/tenant-claim-contract.md` states it, +and a separate test asserted `registration_endpoint` is absent — but nothing +connected the two, so a future session adding dynamic registration would see a +test about discovery metadata, not a warning about relabelling users. + +## Tie the exclusion to the capability it protects + +```task +id: KEY-WP-0030-T01 +status: done +priority: medium +``` + +`TestRegistrationBoundTenantRequiresStaticRegistration` asserts both halves +together: that a client-declared tenant is issued, and that dynamic client +registration is not advertised. Whichever is removed first, the failure points at +the other. + +The failure message carries the reasoning rather than the observation. Adding +dynamic registration while a registration may declare its users' tenant means +anyone able to register a client can relabel the users who log in through it into +a zone of their choosing; the message says that and names the two ways out, drop +the capability or gate it to statically configured registrations. + +Verified in both directions rather than assumed: advertising a +`registration_endpoint` fails it with the escalation message, and neutering +`humanTenant` fails it with the message saying to remove the guard along with the +capability it protects. + +This is deliberately not a vote on the tenant question. It makes the precondition +of one option checkable; it does not choose between them, and if option 1 lands +the capability and this guard are removed together. + +## What stays with the owners + +```task +id: KEY-WP-0030-T02 +status: done +priority: medium +``` + +`informed-decision` prefers option 2 and explicitly declines to treat that as the +answer, flagging it to gate-house on GH-DEC-2026-012 and naming approval-engine's +stake. KeyCape leans the same way, which is a reason for more care rather than +less: the code already implements option 2 (`329e48f`), so the doctrine question +is being decided against a working implementation. That is worth saying out loud +to everyone waiting on it, and was. + +Their scope set `[openid, approval:read, approval:approve]` is confirmed and +already published. `client_id` and callback follow at their T07, deliberately not +before the tenant question resolves — registering a client that fails closed at +first use is the failure this exchange existed to avoid.