Make the static-registration precondition a checked condition
All checks were successful
Build and Publish Container Image / build-and-push (push) Successful in 44s
All checks were successful
Build and Publish Container Image / build-and-push (push) Successful in 44s
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 <noreply@anthropic.com> 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
This commit is contained in:
parent
a4e2751ab5
commit
5f516a0fbb
2 changed files with 136 additions and 0 deletions
66
src/internal/server/oidc/tenant_precondition_test.go
Normal file
66
src/internal/server/oidc/tenant_precondition_test.go
Normal file
|
|
@ -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'.")
|
||||
}
|
||||
}
|
||||
70
workplans/KEY-WP-0030-tenant-precondition-guard.md
Normal file
70
workplans/KEY-WP-0030-tenant-precondition-guard.md
Normal file
|
|
@ -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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue