feat: offboard-by-owner endpoint — deactivate a human's agents and cascade-revoke (INV-IDN-010) - #280
Merged
Merged
Conversation
…scade-revoke (INV-IDN-010) Slice 1 of the UC8 offboarding trigger (ADR 0028 D5): the internal endpoint admin's SCIM deactivation worker calls when the org IdP deactivates a human. POST /identities/offboard-by-owner (admin surface, tenant from context — the body cannot steer tenancy) composes two existing primitives in order: 1. Deactivate every not-yet-deactivated identity the human owns in the account — across ALL projects, mirroring migration 041's (owner_user_id, account_id) scoping — via the shared DeactivateIdentity path, which sweeps linked API keys, cascade-revokes that identity's credentials, and emits the retirement CAE signal. Credential revocation alone would be cosmetic: a surviving zid_sk_* service key just re-exchanges for a fresh token via the api_key grant. 2. RevokeAllActiveForOwner — the atomic owner-scoped sweep that also catches delegated descendants owned by other humans (parent_jti chain) and anything issued while step 1 looped. Failure semantics are retry-oriented: per-identity errors are recorded and the loop continues, the cascade runs regardless, and a partial application returns 502 so the outbox worker retries the idempotent operation until 200. The empty-owner guard mirrors the credential cascade's (a blank owner matches every ownerless identity in the account) and short-circuits before any dependency call. New repo method ListByOwnerForOffboard is account-wide by design (List is project-scoped) and excludes only already-deactivated rows — every other status transitions legally to deactivated and must not survive an offboarding. No migration: pure composition of deployed primitives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Security review of #280 found two issues, both fixed here: 1. Whitespace bypass → silent no-op success. A whitespace-only or padded owner (" ", "alice ") passed every == "" guard, matched NO stored row (ownerless identities store exactly ""; VARCHAR equality is exact), and returned 200 with zero counts — the SCIM worker would record a successful offboarding while the departing human's fleet kept running. Guards at all four layers are now trim-aware and reject untrimmed input loudly: OffboardOwner + RevokeAllActiveForOwner (service), ListByOwnerForOffboard (repo), and migration 043 (SQL backstop, btrim mirror of 042's pattern). Guard rejections now wrap a typed sentinel (ErrInvalidOwnerArgument) that mapErr surfaces as 400, so the outbox worker treats malformed events as data bugs to surface, not transient faults to retry forever. 2. 502 leaked the raw wrapped error chain (including Postgres driver text) to the client, deviating from this surface's redaction convention (mapErr: generic messages, details logged server-side). The partial-failure 502 now carries a fixed message; the detail stays in the existing server-side log line. Also from the review's hardening list: ctx.Err() checked at the top of the deactivation loop (a canceled request on a large fleet no longer burns one error log + one failure append per remaining identity), and reason is capped at maxLength 256 (it fans out into revoke_reason on every revoked row and every RevocationNotifier event). Guard tests extended with the whitespace/padded variants and a typed- sentinel assertion, in both the offboard and credential-cascade suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
migration-analyzer caught that one-argument btrim(text) strips spaces only — 'alice\n' or a tab-padded owner would pass the guard and reproduce the silent-no-op the migration exists to close, and trailing-newline artifacts from shell/file input are the classic case for exactly the direct-to-SQL callers the backstop covers. Both guards now use btrim(x, E' \t\n\r\f\v') (ASCII whitespace), and the header states honestly that the SQL backstop is the ASCII subset while the Go guards (strings.TrimSpace) additionally cover Unicode whitespace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jalbrethsen-highflame
approved these changes
Aug 10, 2026
…s the straggler sweep, not the total Deep-audit finding: per-identity deactivation cleanup already cascade- revokes each identity's credentials AND delegated descendants (031), so the final owner-scoped sweep usually finds nothing and credentials_revoked is ~0 on a healthy run. The old field docs implied it was the total, which would misread a healthy offboard as a failed one (and a zero-alert keyed on it would false-alarm). Evidence and zero-alerts key on identities_deactivated; a non-zero credentials_revoked means stragglers. Also corrects the no-ops-not-counted claim, which was inaccurate under a list-vs-deactivate race. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Slice 1 of the UC8 offboarding trigger (ADR 0028 D5, spec INV-IDN-010): the internal endpoint admin's SCIM deactivation outbox worker will call when the org IdP deactivates a human.
POST /identities/offboard-by-owner(admin surface;account_idfrom tenant context, never the body) composes two deployed primitives, in order:ListByOwnerForOffboard; the existingListis project-scoped) — via the sharedDeactivateIdentitypath: sweeps linked API keys, cascade-revokes that identity's credentials, emits the retirement CAE signal. This closes the hole where credential revocation alone is cosmetic: a survivingzid_sk_*key would just re-exchange for a fresh token (api_keygrant).RevokeAllActiveForOwner(migration 041) — the atomic owner-scoped sweep catching delegated descendants owned by other humans via theparent_jtichain, plus anything issued mid-loop.Semantics
ctxcancellation stops the loop early and reports partial.== ""but matches no stored row (ownerless identities store exactly""), which would make the offboarding a silent 200 no-op — the worst outcome for an irreversible operation. Guards inOffboardOwner,RevokeAllActiveForOwner,ListByOwnerForOffboard, and migration 043 (SQL backstop, extending 042's pattern withbtrim) all reject untrimmed input, wrapping a typed sentinel thatmapErrmaps to 400 — the worker surfaces malformed SCIM events as data bugs instead of retrying a 500 forever.CREATE OR REPLACE FUNCTIONonly (no table DDL, no locks, no backfill);downrestores the 042 body verbatim.Security review
Full pass done pre-merge (tenancy steering, guard bypass, blast radius, info leaks, DoS). Both High findings fixed in the second commit (whitespace bypass; 502 error-chain leak) plus the cheap hardenings (
ctx.Err()loop check,maxLengthon reason). Verified clean: body cannot steer tenancy; all queries Bun-parameterized; cross-project sweep is documented intent; logs carry only caller-tenant identifiers; route registered before/identities/{id}.Tests
@pytest.mark.capability("INV-IDN-010")regression (SCIM-deactivate → owned agent's token denied AND its key can't mint), which flips the spec entry toimplemented.Verification
go build,go vet,go test(service/store/handler),golangci-lint(0 issues) — all green under CI'sGOEXPERIMENT=jsonv2.Next slice: admin SCIM deactivation receiver (2 tables + outbox worker) targeting this endpoint.
🤖 Generated with Claude Code