From 822ff04f5559640c9b5126a7aa97bb732d3e34c8 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 28 Sep 2026 12:10:52 +0530 Subject: [PATCH 1/5] docs: design for authorization change evidence (before/after snapshots) --- ...2026-09-28-authz-change-evidence-design.md | 343 ++++++++++++++++++ 1 file changed, 343 insertions(+) create mode 100644 specs/2026-09-28-authz-change-evidence-design.md diff --git a/specs/2026-09-28-authz-change-evidence-design.md b/specs/2026-09-28-authz-change-evidence-design.md new file mode 100644 index 0000000..bf96028 --- /dev/null +++ b/specs/2026-09-28-authz-change-evidence-design.md @@ -0,0 +1,343 @@ +# Authorization Change Evidence — Design + +**Date:** 2026-09-28 +**Status:** Design, awaiting review +**Scope:** FGA authorization changes (tuples, model, reset) +**Repo:** `authorizerdev/authorizer` (server) + +## Problem + +A verification pass against `main` (891f58fd) asked whether Authorizer can evidence +**one real authorization change** — the state before it, the change itself, the state +after, and the audit record. Findings, each confirmed in code: + +| Requirement | Today | +|---|---| +| The change happened | **Yes** — audit row with action, resource type, IP, UA, protocol, timestamp | +| State **before** | **No** — never captured | +| The change itself | **Partial** — FGA tuple write/delete records `count=N` only (`admin_fga.go:116`, `:149`); model write records the new id only (`:82`); `FgaReset` records nothing (`:290-297`) | +| State **after** | **No** — never captured | +| Audit evidence | **Partial** — best-effort, no actor identity, `ResourceID` unset on tuple events | + +Supporting detail: + +- **No before/after anywhere.** No write path snapshots prior state. The engine SPI has + no `ReadChanges` and no read-by-version; `ReadModel` returns only the active model + (`internal/authorization/engine/engine.go:164`), even though `Reset`'s doc (`:180`) + confirms OpenFGA retains prior versions. +- **`ResourceID` is unset** on tuple write/delete, so `_audit_logs(resource_id:)` cannot + locate a specific change. +- **Best-effort evidence.** `LogEvent` is fire-and-forget via `asyncutil.Go`; a storage + failure is logged at Debug and dropped (`internal/audit/provider.go:101-103`) while the + mutation returns success. +- **No actor identity on admin ops.** Every FGA event is `ActorType: admin` with an empty + `ActorID`. Super-admin is a single shared `AdminSecret` or a session cookie derived from + it (`internal/token/admin_token.go:41-63`) — there is no per-admin identity to record. +- **Two storage parity bugs** make the record unfindable on some backends (see Phase 0). + +## Locked decisions + +Agreed with the maintainer before this spec was written: + +1. **Scope: FGA only** — tuples, model, reset. User roles, org membership, and other + authz-adjacent admin surfaces are explicitly out of scope for this spec. +2. **Explicit before/after snapshots**, not replay-derivable deltas. Literal evidence, + accepting the extra engine reads and the payload-bounding work. +3. **Actor identity: record what exists.** No new auth model. Named admin accounts remain + a separate future feature. +4. **Synchronous audit for authorization changes only.** Logins and token issuance stay + asynchronous. +5. **On audit-write failure: return an error, do not compensate.** The change stays + applied. + +## Non-goals + +- Named per-admin identities (would need its own auth-model spec and migration). +- Making non-FGA authz changes (roles, org membership) evidential — same mechanism will + apply later, but not here. +- Exposing OpenFGA's `ReadChanges` as an admin API. It exists on the embedded server + (`openfga@v1.18.1/pkg/server/read_changes.go:19`) and Authorizer inherits + `ChangelogHorizonOffset = 0` so it would return changes immediately — noted as a future + option for independent cross-checking, deliberately not built now. +- Audit log retention/rotation. `DeleteAuditLogsBefore` exists on the storage interface + (`internal/storage/provider.go:306`) with zero callers; snapshots will grow the table + faster, so retention becomes worth revisiting — tracked, not solved here. + +--- + +## Phase 0 — Make the record findable (storage parity) + +Setting `ResourceID` is pointless if callers cannot filter on it. Two backends accept +filters the GraphQL API advertises and silently ignore them: + +| Backend | `action` | `actor_id` | `resource_type` | `resource_id` | `from`/`to_timestamp` | +|---|---|---|---|---|---| +| SQL, MongoDB, ArangoDB, DynamoDB | yes | yes | yes | yes | yes | +| Couchbase | yes | yes | yes | yes | **ignored** | +| Cassandra/ScyllaDB | yes | yes | **ignored** | **ignored** | **ignored** | + +Cassandra: `internal/storage/db/cassandradb/audit_log.go:39-61`. + +**Change** + +- Add `resource_id` and `resource_type` secondary indexes beside the existing three + (`cassandradb/provider.go:408-419`) and apply both filters as indexed equality, matching + the existing comment's constraint that count queries must not need `ALLOW FILTERING` + (Scylla builds secondary indexes as materialized views). +- Timestamp range on a non-key column cannot use a secondary index. Use `ALLOW FILTERING` + for the range predicate only, with a `ponytail:` comment naming the materialized-view + upgrade path. This is an admin-only, rare query; a full scan is acceptable until it is + measurably not. +- Couchbase: apply `from_timestamp` / `to_timestamp` in the N1QL predicate. + +**Verification.** Storage-layer change, so per `AGENTS.md` step 4 SQLite alone is +insufficient: `make test-scylladb` and `make test-couchbase` must both pass. + +**Ships as its own PR** — it is an independent bug fix and is useful without the rest. + +--- + +## Phase 1 — Audit plumbing + +### 1.1 Synchronous audit writes + +Add to `audit.Provider` (`internal/audit/provider.go`): + +```go +// LogEventSync records an audit log entry synchronously and returns the storage +// error. Callers that treat the audit record as part of the operation's contract +// (authorization changes) use this; everything else uses LogEvent. +LogEventSync(ctx context.Context, event Event) error +``` + +`LogEvent` keeps its current fire-and-forget behaviour so logins and token issuance stay +off the audit DB's hot path. Both share one internal `buildAuditLog(event)` so the +protocol-folding in `metadataWithProtocol` (`provider.go:63-79`) is not duplicated. + +Blast radius is small: the only test double is `inviteAudit` +(`internal/service/admin_access_invite_test.go:90`), which embeds `audit.Provider` and so +satisfies a widened interface without edits. + +### 1.2 Failure semantics + +Ordering is forced: before-snapshot → engine write → after-snapshot → audit write. When +the audit write fails **the authorization change has already persisted**. + +Per locked decision 5, no compensation: + +```go +if err := p.AuditProvider.LogEventSync(ctx, ev); err != nil { + log.Error().Err(err).Str("action", ev.Action).Str("object", obj). + Msg("authorization change applied but NOT audited") + return nil, nil, err +} +``` + +Note the log level: `Error`, not the `Debug` used everywhere else in the audit path. The +documented meaning of this error is **"the change was applied but is unevidenced — verify +the live state with `_fga_read_tuples`"**, and the GraphQL/gRPC/REST docs must say so, +because the natural reading of an error is "nothing happened." + +Known consequence, accepted: a naïve client retry of `_fga_write_tuples` may then fail on +duplicate tuples. Rejected alternatives and why: + +- *Compensating rollback* — a failed compensation leaves **two** unaudited changes, and + `FgaReset` has no compensation at all. +- *Succeed and flag* — reintroduces exactly the silent evidence gap this spec exists to + close. + +### 1.3 Actor identity + +Record what the system already knows; invent no new identity model. + +- Add `AuthMode string` to `authctx.Principal` (`internal/authctx/principal.go:42`), + values `"admin_session"` or `"shared_secret"`. Set it at + `internal/grpcsrv/interceptors/auth.go:138-139`, which today constructs + `Principal{IsSuperAdmin: true}` and nothing else. Derive the same value through the gin + shim on the GraphQL/REST path. +- Emit it as `auth_mode` in the event metadata. +- **Never record the admin session id.** `GetAdminAuthToken` returns a live bearer + credential and the dashboard renders this table. If per-session correlation is ever + needed, store a non-reversible hash — not in this spec. +- The shared actor-resolution helper populates `ActorID` / `ActorEmail` whenever a real + user id is available via `callerTokenData` (`internal/service/caller.go:15`). Every FGA + operation is super-admin-gated, so in this spec's scope that path never fires and FGA + rows carry `auth_mode` only. The helper is written to handle it so the org-admin lanes + get it for free when they are brought in scope later; no org-admin call sites are + changed here. + +This does not make a super-admin action attributable to a person. It records honestly +that it was not. Stated as a known limitation in the docs. + +--- + +## Phase 2 — FGA snapshots + +### 2.1 Grain: one audit row per distinct object touched + +A write of `[(alice, viewer, doc:1), (bob, editor, doc:2)]` emits **two** audit rows, with +`ResourceID` = `document:1` and `document:2`. + +Rationale: + +- `_audit_logs(resource_id: "document:1")` answers the question an auditor actually asks — + "what happened to this object?" — which a single batch row cannot. +- Each row's payload is bounded by the fan-in of one object, not the whole request. +- No invented batch-correlation id. + +Rejected alternative: one row per request with a batch id. Smaller write amplification, +but pushes correlation onto the reader and leaves `ResourceID` unusable. + +### 2.2 Record shape + +Tuple write/delete: + +```json +{ + "protocol": "graphql", + "auth_mode": "shared_secret", + "op": "write", + "object": "document:1", + "delta": [ + {"user": "user:alice", "relation": "viewer", "object": "document:1"} + ], + "before": {"tuples": [...], "count": 3, "truncated": false}, + "after": {"tuples": [...], "count": 4, "truncated": false}, + "concurrent_modification": false +} +``` + +Model write: + +```json +{ + "op": "model_write", + "auth_mode": "admin_session", + "before": {"model_id": "01ABC...", "dsl": "model\n schema 1.1\n..."}, + "after": {"model_id": "01XYZ...", "dsl": "model\n schema 1.1\n..."} +} +``` + +`before` is `null` when no model existed (`engine.ErrNoModel`). + +The **delta is always recorded** alongside the snapshots. This is not a hedge against +decision 2 — it is the only field guaranteed complete when a snapshot truncates, and +§2.4 depends on it. + +### 2.3 Bounding + +DynamoDB's 400KB item limit is the hard ceiling; Cassandra degrades near 100KB. Cells are +`text` / unbounded elsewhere. + +- **32KB budget** for the serialized metadata of one row. +- **250 tuples per snapshot side.** A serialized tuple runs ~80-120 bytes, so 2 x 250 + leaves headroom inside the 32KB budget for the delta and envelope. On overflow emit + `"truncated": true` with the true `count` so the reader knows what they are missing. The + budget is enforced as the real limit — the tuple cap is the cheap pre-check, and a row + still over 32KB after it truncates further. +- Snapshot reads **must page** — `ReadTuples` caps at `maxFgaReadPageSize = 100` + (`admin_fga.go:25`) — looping until the cap or exhaustion. +- **Object fan-out cap.** `maxFgaTuplesPerWrite = 100` (`admin_fga.go:20`) bounds a request + to 100 tuples and therefore up to 100 distinct objects. Worst case that is 200 engine + reads plus 100 synchronous inserts for one mutation. Above **20 distinct objects**, + degrade to a single summary row carrying the full delta and + `"snapshot_skipped": "too_many_objects"`. Typical writes touch one or two objects. +- Model DSLs: if both DSLs exceed the budget, fall back to `before_model_id` alone. OpenFGA + retains prior model versions, so the id stays a stable pointer even though nothing + exposes it for reading yet. + +### 2.4 The read-write-read race + +OpenFGA offers no transaction spanning before-read → write → after-read. A concurrent +write from another replica pollutes the after-snapshot. + +A process mutex does not fix this — multi-replica deployments share one SQL FGA store +(`internal/authorization/engine/openfga/datastore_sql.go`). + +Instead, make the record self-checking: compute `expected_after = before ⊕ delta`, compare +against the observed after-snapshot, and set `"concurrent_modification": true` when they +disagree. The auditor sees a flagged row instead of a silent lie. Cost is a set comparison +over already-loaded data. + +### 2.5 Per-operation behaviour + +| Operation | before | after | Rows | +|---|---|---|---| +| `FgaWriteTuples` (`admin_fga.go:93`) | tuples on each object | tuples on each object | one per object | +| `FgaDeleteTuples` (`:126`) | tuples on each object | tuples on each object | one per object | +| `FgaWriteModel` (`:60`) | active model id + DSL, or `null` | new model id + DSL | one | +| `FgaReset` (`:267`) | active model id + DSL | empty | one, written **before** execution | + +`FgaReset` writes its audit row before executing because it cannot be compensated and its +after-state is empty by construction. It already reads tuples for its safety gate +(`:278-283`, which refuses while any tuple exists) — reuse that read rather than adding +another. + +### 2.6 Code layout + +FGA-specific snapshot and metadata shaping goes in a new +`internal/service/admin_fga_audit.go`, keeping `admin_fga.go` readable and the audit +package generic. + +**Constraint:** `admin_gate_test.go:108` statically asserts every admin function calls +`requireSuperAdmin` at top level. Any helper extracted from these functions must preserve +that shape. + +--- + +## Phase 3 — Tests + +Per `AGENTS.md`, integration tests use SQLite via `getTestConfig()`. + +**Integration** (`internal/integration_tests/`) — assert row *content*, not just presence: + +1. Tuple write on a fresh object: exactly one row, `ResourceID` = the object, + `before.tuples` empty, `after.tuples` contains the new tuple, `delta` matches. +2. Tuple delete: `before` contains it, `after` does not. +3. Multi-object write: one row per distinct object, each scoped to its own object. +4. Object fan-out over the cap: single summary row with `snapshot_skipped`. +5. Snapshot over the tuple cap: `truncated: true` with an accurate `count`. +6. Model write over an existing model: both DSLs present; and over an empty store: + `before: null`. +7. `FgaReset`: row present with the prior model DSL. +8. Audit-write failure (injected): mutation returns an error **and** the tuples are still + present via `_fga_read_tuples` — the documented no-compensation contract. +9. `auth_mode` recorded correctly for both header-secret and admin-session auth. + +**Storage** (`internal/storage/`): `resource_id`, `resource_type` and timestamp-range +filters return correct results on every backend — the Phase 0 regression test. + +**Concurrency:** a unit-level test of the `expected_after` comparison with a synthetic +divergent after-set; a genuine race is not reliably reproducible in CI. + +**Full gate before any PR** (`AGENTS.md`): `go build ./...`, `go vet ./...`, `make test`, +at least one non-SQL backend (`make test-scylladb`, `make test-couchbase` for Phase 0), +and `make lint`. + +--- + +## Rollout + +Four PRs, each on its own feature branch, never to `main`: + +1. `fix/audit-log-filter-parity` — Phase 0. +2. `feat/audit-sync-and-actor-mode` — Phase 1. +3. `feat/fga-change-snapshots` — Phase 2 + Phase 3. +4. Docs: the `_audit_logs` metadata shape, the no-compensation error contract, and the + stated limitation that super-admin actions are not attributable to a person. + +`security-engineer` reviews PRs 2 and 3 — both touch admin auth context and audit +integrity. + +Per the established rollout order, after the server ships: dashboard (render the +before/after diff on the audit log page), SDKs, then the docs site. + +## Open risks + +- **Audit table growth.** Snapshots are far larger than today's `count=N` rows. Retention + (`DeleteAuditLogsBefore`, currently uncalled) becomes worth wiring. Out of scope; flagged. +- **Write amplification.** Up to 20 synchronous inserts for one multi-object mutation. + Bounded by the fan-out cap, but it makes admin FGA writes measurably slower. Acceptable + for an admin-only path. +- **Super-admin remains unattributable.** This spec records *that* it was a shared secret, + not *who* held it. Genuinely closing this needs named admin accounts. From ab376df8b73277b009974b6cead1cfcd695c85af Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 29 Sep 2026 11:32:59 +0530 Subject: [PATCH 2/5] docs: rescope authz change evidence to two defensible claims Scoping to internal/service/admin_fga.go covered 4 of 8 FGA tuple mutation sites. SCIM group membership is stored as FGA tuples and writes them directly; purgeFgaTuplesForUser deletes them on user delete. Neither is audited, and scim.Dependencies has no AuditProvider at all. Rescope by claim rather than by file, add the static guard test that prevents a ninth site appearing unaudited, and bring roles/membership in. Infrastructure-config surfaces stay deferred. --- ...2026-09-28-authz-change-evidence-design.md | 178 ++++++++++++++++-- 1 file changed, 162 insertions(+), 16 deletions(-) diff --git a/specs/2026-09-28-authz-change-evidence-design.md b/specs/2026-09-28-authz-change-evidence-design.md index bf96028..429eedb 100644 --- a/specs/2026-09-28-authz-change-evidence-design.md +++ b/specs/2026-09-28-authz-change-evidence-design.md @@ -2,7 +2,8 @@ **Date:** 2026-09-28 **Status:** Design, awaiting review -**Scope:** FGA authorization changes (tuples, model, reset) +**Scope:** Two defensible claims — every FGA tuple change, and every change to a user's +roles or org membership (see Locked decisions 1) **Repo:** `authorizerdev/authorizer` (server) ## Problem @@ -39,8 +40,27 @@ Supporting detail: Agreed with the maintainer before this spec was written: -1. **Scope: FGA only** — tuples, model, reset. User roles, org membership, and other - authz-adjacent admin surfaces are explicitly out of scope for this spec. +1. **Scope is defined by the claim it makes true, not by a file.** + + An earlier revision scoped this to "FGA only", meaning `internal/service/admin_fga.go`. + That is not a coherent boundary: **half the FGA tuple mutations in the codebase happen + elsewhere** (see Phase 2.0). A spec limited to that file would ship, pass its tests, and + still let an auditor read the log across a window in which SCIM rewrote group + membership and conclude no authorization change occurred — replacing a known gap with + false confidence, which is worse. + + Two claims are in scope. Each is stated so a reader can falsify it: + + - **Claim 1 — every FGA tuple change is evidenced.** All 8 engine-mutation call sites, + plus a static guard test so a ninth cannot be added silently. + - **Claim 2 — every change to a user's roles or org membership is evidenced.** + `UpdateUser` roles, `RevokeAccess`/`EnableAccess`, `Add`/`RemoveOrgMember`, SCIM + org-membership creation, SCIM deactivate/reactivate. + + Deliberately outside both: clients, trusted issuers, org OIDC/SAML connections, SCIM + endpoints, org domains, SAML IdP keys and service providers (~22 sites). The line is + *"who can do what" is evidenced; "how the system is configured" is not yet* — and those + surfaces hold nearly all the credential material that makes redaction risky. 2. **Explicit before/after snapshots**, not replay-derivable deltas. Literal evidence, accepting the extra engine reads and the payload-bounding work. 3. **Actor identity: record what exists.** No new auth model. Named admin accounts remain @@ -52,9 +72,14 @@ Agreed with the maintainer before this spec was written: ## Non-goals -- Named per-admin identities (would need its own auth-model spec and migration). -- Making non-FGA authz changes (roles, org membership) evidential — same mechanism will - apply later, but not here. +- Infrastructure-configuration surfaces: clients, trusted issuers, org OIDC/SAML + connections, SCIM endpoints, org domains, SAML IdP keys/SPs. Same mechanism applies + later. They are deferred because they change rarely and because their rows carry an RSA + private key (`schemas/saml_idp_key.go:3`), a SCIM `TokenHash` (`scim_endpoint.go:33`) + and a bcrypt client secret (`client.go:20`) — the redaction allow-list guarding those is + the one part of this design that can cause a *new* incident, and it should be sized + against three simple resource types before it has to cover private keys. +- A named-admin identity model (see decision 3). - Exposing OpenFGA's `ReadChanges` as an admin API. It exists on the embedded server (`openfga@v1.18.1/pkg/server/read_changes.go:19`) and Authorizer inherits `ChangelogHorizonOffset = 0` so it would return changes immediately — noted as a future @@ -146,7 +171,19 @@ duplicate tuples. Rejected alternatives and why: - *Succeed and flag* — reintroduces exactly the silent evidence gap this spec exists to close. -### 1.3 Actor identity +### 1.3 SCIM audit wiring (prerequisite) + +`scim.Dependencies` (`internal/service/scim/scim.go:88-100`) holds `Log`, +`StorageProvider`, `MemoryStoreProvider`, `AuthzEngine` and `EventsProvider` — and **no +`AuditProvider`**. The SCIM package therefore cannot audit anything today; the absence is +structural, not an oversight at individual call sites. Add the field and wire it in +`cmd/root.go` alongside the other SCIM dependencies. + +Nil-safe: leave SCIM's audit calls no-ops when the provider is nil, matching the existing +convention for `EventsProvider` ("Nil when webhooks are not wired — event firing is then a +no-op"). + +### 1.4 Actor identity Record what the system already knows; invent no new identity model. @@ -171,7 +208,50 @@ that it was not. Stated as a known limitation in the docs. --- -## Phase 2 — FGA snapshots +## Phase 2 — Claim 1: every FGA tuple change is evidenced + +### 2.0 The eight call sites + +`AuthzEngine` mutation calls, enumerated from the tree (excluding `_test.go`): + +| Site | Operation | Audited today | +|---|---|---| +| `internal/service/admin_fga.go:72` | `WriteModel` | yes (id only) | +| `internal/service/admin_fga.go:106` | `WriteTuples` | yes (`count=N`) | +| `internal/service/admin_fga.go:139` | `DeleteTuples` | yes (`count=N`) | +| `internal/service/admin_fga.go:286` | `Reset` | yes (nothing) | +| `internal/service/scim/groups.go:303` | `DeleteTuples` (group delete) | **no** | +| `internal/service/scim/groups.go:376` | `WriteTuples` (group members added) | **no** | +| `internal/service/scim/groups.go:382` | `DeleteTuples` (group members removed) | **no** | +| `internal/service/fga.go:446` | `DeleteTuples` (`purgeFgaTuplesForUser`) | **no** | + +SCIM Group membership *is* FGA tuples — not a DB column (`scim.go:92-95`) — so an external +IdP rewriting a group is an authorization change that currently leaves no audit row at +all. `purgeFgaTuplesForUser` is called from `DeleteUser` (`admin_users.go:417`): the +deletion is audited, the grants it destroys are not enumerated, so "which access did this +removal actually revoke?" is unanswerable. + +The four unaudited sites differ from the admin ones in a way that shapes their treatment: + +- **SCIM sites are not super-admin actions.** Actor is the SCIM endpoint — + `ActorType: service_account`, `ActorID` = the endpoint id, no `auth_mode`. +- **`groups.go:303` is deliberately non-fatal** — a tuple-delete failure there is logged + and the group row is still removed. Its audit write must preserve that: log at `Error` + and continue, **not** the return-an-error contract of §1.2. Applying the synchronous + contract here would turn an accepted partial failure into a failed deprovision. +- **`purgeFgaTuplesForUser` has no natural per-object grain** — it deletes every tuple + naming one user, so it emits **one** row keyed on `ResourceID` = `user:`, with the + deleted tuples as the delta. The §2.1 per-object grain does not apply. + +### 2.0.1 Static guard test + +The gap above exists because nothing enforced it. Mirroring `admin_gate_test.go:108` +(which statically asserts every admin function calls `requireSuperAdmin` at top level), +add a test that parses the tree for `AuthzEngine.WriteTuples` / `DeleteTuples` / +`WriteModel` / `Reset` call sites and fails when one appears without an audit emission in +the enclosing function, against an explicit allow-list of known sites. + +This is the cheapest item in the spec and the only one that stops the gap reopening. ### 2.1 Grain: one audit row per distinct object touched @@ -285,6 +365,45 @@ that shape. --- +## Phase 2b — Claim 2: roles and org membership + +Row-shaped lanes. `before` is the loaded row, `after` is what the storage call returned — +**no extra reads, no paging, no fan-out cap, and no `expected_after` comparison**; none of +§2.1-2.4 applies. + +| Site | Note | +|---|---| +| `UpdateUser` (`admin_users.go:110`) | old roles already in memory at `:315`, discarded today | +| `RevokeAccess` / `EnableAccess` (`admin_access.go`) | | +| `AddOrgMember` (`admin_organizations.go:255`) | | +| `RemoveOrgMember` (`:317`) | a `before` of `{org_id, user_id, roles}` fixes today's dangling `membership.ID` | +| SCIM `AddOrgMembership` (`scim/scim.go:356`) | unaudited today | +| SCIM `deactivate` (`scim/scim.go:458-473`) | sets `RevokedTimestamp`, kills every session; unaudited today | +| SCIM reactivate (`scim/scim.go:412`) | clears `RevokedTimestamp`; unaudited today | + +Two hard rules: + +1. **Serialize before mutating.** Capture `beforeJSON` *before* any field assignment — not + by cloning the struct. These handlers mutate the loaded row in place, several fields + are `*string`, and a shallow copy shares slice backing arrays, so a clone helper would + silently produce `before == after` and pass a naive test. It is also already the stored + format. +2. **Snapshot the `schemas.*` row, never the `model.*` response.** Response objects carry + plaintext secrets exactly once (`CreateClient`, `RotateClientSecret`, + `RotateScimToken`); the DB row only ever holds the hash. This rule matters most for the + deferred lanes, but the helper is written now and must enforce it from the start. + +**Redaction allow-list**, opt-in per resource type — a deny-list would admit any newly +added secret field by default. In this phase it covers exactly three types: `user` +(excludes the password hash), `organization`, `org_membership`. A guard test enumerates +each schema's fields and fails when an unlisted one appears. + +**Lost-update race:** two admins updating one row concurrently means the loser's `before` +is stale. This is the same last-writer-wins semantics the API already has; the snapshot +records what this call saw and wrote. No machinery. + +--- + ## Phase 3 — Tests Per `AGENTS.md`, integration tests use SQLite via `getTestConfig()`. @@ -303,10 +422,24 @@ Per `AGENTS.md`, integration tests use SQLite via `getTestConfig()`. 8. Audit-write failure (injected): mutation returns an error **and** the tuples are still present via `_fga_read_tuples` — the documented no-compensation contract. 9. `auth_mode` recorded correctly for both header-secret and admin-session auth. +10. **SCIM group member add/remove** emits rows with `ActorType: service_account` and the + endpoint id as `ActorID`. +11. **SCIM group delete** with a failing audit write still deletes the group — the + non-fatal contract of §2.0, not §1.2. +12. **`DeleteUser`** emits a `purgeFgaTuplesForUser` row keyed `user:` listing the + destroyed tuples. +13. **Claim 2 lanes:** role change records old and new roles; `RemoveOrgMember` records + `{org_id, user_id, roles}`; SCIM deactivate records the `RevokedTimestamp` transition. +14. **Redaction:** a user snapshot contains no password hash; the allow-list guard test + fails when a new schema field is added without a decision. +15. **Serialize-before-mutate:** a role change produces `before != after` — the regression + test for the in-place-mutation trap. **Storage** (`internal/storage/`): `resource_id`, `resource_type` and timestamp-range filters return correct results on every backend — the Phase 0 regression test. +**Static:** the §2.0.1 guard test over `AuthzEngine` mutation call sites. + **Concurrency:** a unit-level test of the `expected_after` comparison with a synthetic divergent after-set; a genuine race is not reliably reproducible in CI. @@ -318,16 +451,21 @@ and `make lint`. ## Rollout -Four PRs, each on its own feature branch, never to `main`: - -1. `fix/audit-log-filter-parity` — Phase 0. -2. `feat/audit-sync-and-actor-mode` — Phase 1. -3. `feat/fga-change-snapshots` — Phase 2 + Phase 3. -4. Docs: the `_audit_logs` metadata shape, the no-compensation error contract, and the +Six PRs, each on its own feature branch, never to `main`: + +1. `fix/audit-log-filter-parity` — Phase 0. **Independent of everything else** — it is a + standalone bug (the API advertises filters two backends ignore) and ships first rather + than waiting on this design. +2. `feat/audit-sync-and-actor-mode` — Phase 1, including the SCIM `AuditProvider` wiring. +3. `feat/fga-change-evidence` — Phase 2, all 8 sites + the static guard test. +4. `feat/roles-membership-evidence` — Phase 2b. +5. Tests land with their phase; Phase 3 enumerates them in one place, it is not a + separate PR. +6. Docs: the `_audit_logs` metadata shape, the no-compensation error contract, and the stated limitation that super-admin actions are not attributable to a person. -`security-engineer` reviews PRs 2 and 3 — both touch admin auth context and audit -integrity. +`security-engineer` reviews PRs 2, 3 and 4 — admin auth context, audit integrity, and the +redaction allow-list. Per the established rollout order, after the server ships: dashboard (render the before/after diff on the audit log page), SDKs, then the docs site. @@ -339,5 +477,13 @@ before/after diff on the audit log page), SDKs, then the docs site. - **Write amplification.** Up to 20 synchronous inserts for one multi-object mutation. Bounded by the fan-out cap, but it makes admin FGA writes measurably slower. Acceptable for an admin-only path. +- **The audit table becomes a hard dependency of admin authorization changes.** + Synchronous writes mean an audit-table outage fails those mutations. In every default + deployment it is the same database the mutation already wrote to, so the added exposure + is small — but it is real, and it is the direct cost of decision 4. +- **Deferred surfaces stay unevidenced.** Clients, trusted issuers, org connections, SCIM + endpoints, domains and SAML IdP keys (~22 sites) keep today's partial records. The + two-claim framing must be stated plainly in the docs so nobody reads "authorization + changes are audited" more broadly than it is true. - **Super-admin remains unattributable.** This spec records *that* it was a shared secret, not *who* held it. Genuinely closing this needs named admin accounts. From ef9c977bbc64844516174eca4c515f795ad2904e Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 29 Sep 2026 11:59:45 +0530 Subject: [PATCH 3/5] =?UTF-8?q?docs:=20correct=20Phase=200=20table=20?= =?UTF-8?q?=E2=80=94=20couchbase=20drops=20four=20filters,=20not=20two?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified empirically: the three new storage subtests fail on couchbase before the fix. Earlier table read its SELECT column list as filter support. --- specs/2026-09-28-authz-change-evidence-design.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/specs/2026-09-28-authz-change-evidence-design.md b/specs/2026-09-28-authz-change-evidence-design.md index 429eedb..2576201 100644 --- a/specs/2026-09-28-authz-change-evidence-design.md +++ b/specs/2026-09-28-authz-change-evidence-design.md @@ -1,7 +1,7 @@ # Authorization Change Evidence — Design **Date:** 2026-09-28 -**Status:** Design, awaiting review +**Status:** Design approved; Phase 0 implemented **Scope:** Two defensible claims — every FGA tuple change, and every change to a user's roles or org membership (see Locked decisions 1) **Repo:** `authorizerdev/authorizer` (server) @@ -98,9 +98,15 @@ filters the GraphQL API advertises and silently ignore them: | Backend | `action` | `actor_id` | `resource_type` | `resource_id` | `from`/`to_timestamp` | |---|---|---|---|---|---| | SQL, MongoDB, ArangoDB, DynamoDB | yes | yes | yes | yes | yes | -| Couchbase | yes | yes | yes | yes | **ignored** | +| Couchbase | yes | yes | **ignored** | **ignored** | **ignored** | | Cassandra/ScyllaDB | yes | yes | **ignored** | **ignored** | **ignored** | +Both broken backends drop the same four filters. (An earlier revision of this table +credited Couchbase with `resource_type`/`resource_id` — that was a misreading of its +SELECT column list; its `WHERE` builder only ever handled `action` and `actor_id`. +Confirmed by running the Phase 0 tests against Couchbase before the fix: all three new +subtests failed.) + Cassandra: `internal/storage/db/cassandradb/audit_log.go:39-61`. **Change** @@ -113,7 +119,7 @@ Cassandra: `internal/storage/db/cassandradb/audit_log.go:39-61`. for the range predicate only, with a `ponytail:` comment naming the materialized-view upgrade path. This is an admin-only, rare query; a full scan is acceptable until it is measurably not. -- Couchbase: apply `from_timestamp` / `to_timestamp` in the N1QL predicate. +- Couchbase: apply all four missing filters in the N1QL predicate as named parameters. **Verification.** Storage-layer change, so per `AGENTS.md` step 4 SQLite alone is insufficient: `make test-scylladb` and `make test-couchbase` must both pass. From 40f1f381f108e953e00c8b955626df80f0f459ba Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 29 Sep 2026 12:16:11 +0530 Subject: [PATCH 4/5] docs: record the scylla combined-filter bug found in phase 0 --- specs/2026-09-28-authz-change-evidence-design.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/specs/2026-09-28-authz-change-evidence-design.md b/specs/2026-09-28-authz-change-evidence-design.md index 2576201..3aa5ece 100644 --- a/specs/2026-09-28-authz-change-evidence-design.md +++ b/specs/2026-09-28-authz-change-evidence-design.md @@ -1,7 +1,7 @@ # Authorization Change Evidence — Design **Date:** 2026-09-28 -**Status:** Design approved; Phase 0 implemented +**Status:** Design approved; Phase 0 shipped (authorizerdev/authorizer#802). Phases 1-3 pending review of this spec. **Scope:** Two defensible claims — every FGA tuple change, and every change to a user's roles or org membership (see Locked decisions 1) **Repo:** `authorizerdev/authorizer` (server) @@ -105,7 +105,16 @@ Both broken backends drop the same four filters. (An earlier revision of this ta credited Couchbase with `resource_type`/`resource_id` — that was a misreading of its SELECT column list; its `WHERE` builder only ever handled `action` and `actor_id`. Confirmed by running the Phase 0 tests against Couchbase before the fix: all three new -subtests failed.) +subtests failed. The four "yes" rows are likewise verified by running the tests against +MongoDB, ArangoDB and DynamoDB, not by reading the code.) + +A **second bug** surfaced while fixing this: on Scylla, two indexed equalities with no +`ALLOW FILTERING` are rejected outright, which is the query the pre-fix builder emitted. +So `_audit_logs(action:, actor_id:)` — the one combination Cassandra was believed to +support — already errored. No test combined two filters, and the audit integration tests +(`admin_audit_rest_test.go`, `admin_audit_grpc_test.go`) only ever pass `action`. That +absence is the root cause of both bugs, and is why Phase 3 asserts filter behaviour at the +storage layer across every backend rather than once over SQLite. Cassandra: `internal/storage/db/cassandradb/audit_log.go:39-61`. From dc5a3401c1f815ef6edd5ca493dc741dddd1ca72 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 2 Oct 2026 22:00:23 +0530 Subject: [PATCH 5/5] docs: phase 1 implementation plan for authz change evidence --- ...10-02-authz-change-evidence-phase1-plan.md | 712 ++++++++++++++++++ 1 file changed, 712 insertions(+) create mode 100644 specs/2026-10-02-authz-change-evidence-phase1-plan.md diff --git a/specs/2026-10-02-authz-change-evidence-phase1-plan.md b/specs/2026-10-02-authz-change-evidence-phase1-plan.md new file mode 100644 index 0000000..1c00a8a --- /dev/null +++ b/specs/2026-10-02-authz-change-evidence-phase1-plan.md @@ -0,0 +1,712 @@ +# Authorization Change Evidence — Phase 1 Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add the audit plumbing that Phase 2 and 2b need — a synchronous audit write that returns its error, a recorded super-admin authentication mode, and an `AuditProvider` reachable from the SCIM package. + +**Architecture:** Three additive changes, no behaviour change to any existing caller. `audit.Provider` gains `LogEventSync`; the existing fire-and-forget `LogEvent` is untouched and both share one record builder so they cannot diverge. `token.Provider` gains `AdminAuthMode`, and `IsSuperAdmin` is re-expressed in terms of it so the two can never disagree. `scim.Dependencies` gains a nil-safe `AuditProvider` field. + +**Tech Stack:** Go 1.26.6, zerolog, gin, testify, gqlgen, gocql/gocb/gorm storage providers. + +**Spec:** `specs/2026-09-28-authz-change-evidence-design.md` (this repo), §Phase 1. + +## Global Constraints + +- Go 1.26.6 per `go.mod`. Do not raise the floor. +- Never commit to `main`. Branch `feat/audit-sync-and-actor-mode`. +- Run `make fmt` before committing; CI runs `make lint`. +- golangci-lint is pinned to `v2.11.4`. The Makefile installs it **only when no `golangci-lint` is on PATH**, so a newer local binary silently takes precedence and reports findings CI does not. Verify lint against CI, not a local binary of a different version. +- Verification gate before PR (`AGENTS.md`): `go build ./...`, `go vet ./...`, `make test`, `make lint`. No storage provider is modified in this phase, so no non-SQL backend run is required. +- Admin credentials are never written to an audit record. Record the *mode* of authentication, never the session handle or the secret. +- `LogEvent` keeps its exact current signature and fire-and-forget semantics. Logins and token issuance must not acquire a synchronous dependency on the audit table. + +## Review Focus + +Inputs the spec implies but no task's happy path exercises. Each has its test assigned to the task that owns the code. + +1. **`LogEventSync` must fold `Protocol` into metadata exactly as `LogEvent` does.** If the two build their record separately, synchronous events silently lose the `protocol` key and audit queries filtering on it miss them. → Task 1, Step 9. +2. **`AdminAuthMode` must return empty when `AdminSecret` is unset**, even if a matching header is sent. An unconfigured secret must never authenticate, and must never be recorded as though it had. → Task 2, Step 7. +3. **`AdminAuthMode` must respect `DisableAdminHeaderAuth`.** With header auth disabled, a correct secret is not super-admin and must not report `shared_secret`. → Task 2, Step 7. +4. **`inviteAudit` embeds a nil `audit.Provider`.** Widening the interface compiles, but the first caller of `LogEventSync` under that test nil-panics. Phase 1 adds no caller; Phase 2 will. → Task 1, Step 7 adds the stub now. +5. **A nil `AuditProvider` in SCIM must be a no-op, not a panic.** `EventsProvider` already documents this convention; a deployment that does not wire audit must still serve SCIM. → Task 3, Step 1. + +--- + +### Task 1: `LogEventSync` on `audit.Provider` + +**Files:** +- Modify: `internal/audit/provider.go` +- Modify: `internal/service/admin_access_invite_test.go:90-92` +- Test: `internal/audit/provider_sync_test.go` (create) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `audit.Provider.LogEventSync(ctx context.Context, event Event) error` — returns the storage error unchanged, `nil` on success. Phase 2 call sites depend on this exact signature. + +- [ ] **Step 1: Write the failing test** + +Create `internal/audit/provider_sync_test.go`: + +```go +package audit + +import ( + "context" + "errors" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/storage" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// fakeAuditStore records what AddAuditLog received and can be told to fail. +// It embeds storage.Provider (nil) so it satisfies the interface without +// implementing the other ~200 methods; only AddAuditLog is ever called here. +type fakeAuditStore struct { + storage.Provider + got *schemas.AuditLog + err error +} + +func (f *fakeAuditStore) AddAuditLog(_ context.Context, l *schemas.AuditLog) error { + if f.err != nil { + return f.err + } + f.got = l + return nil +} + +func newTestProvider(st storage.Provider) Provider { + log := zerolog.Nop() + return New(&Dependencies{Log: &log, StorageProvider: st}) +} + +func TestLogEventSync_PersistsEventAndReturnsNil(t *testing.T) { + st := &fakeAuditStore{} + p := newTestProvider(st) + + err := p.LogEventSync(context.Background(), Event{ + Action: "admin.fga_tuples_written", + ActorType: "admin", + ActorID: "actor-1", + }) + + require.NoError(t, err) + require.NotNil(t, st.got) + assert.Equal(t, "admin.fga_tuples_written", st.got.Action) + assert.Equal(t, "actor-1", st.got.ActorID) +} + +func TestLogEventSync_ReturnsStorageError(t *testing.T) { + boom := errors.New("audit table unavailable") + p := newTestProvider(&fakeAuditStore{err: boom}) + + err := p.LogEventSync(context.Background(), Event{Action: "admin.fga_reset"}) + + require.Error(t, err) + assert.ErrorIs(t, err, boom, "the storage error must reach the caller unchanged") +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +go test ./internal/audit/ -run TestLogEventSync -v +``` + +Expected: compile failure — `p.LogEventSync undefined (type Provider has no field or method LogEventSync)`. + +- [ ] **Step 3: Extract the shared record builder** + +In `internal/audit/provider.go`, add below `metadataWithProtocol`: + +```go +// buildAuditLog converts an Event into its storage row. Shared by LogEvent and +// LogEventSync so the two can never disagree about how a record is shaped — +// in particular, both fold Protocol into Metadata via metadataWithProtocol. +func buildAuditLog(event Event) *schemas.AuditLog { + return &schemas.AuditLog{ + ActorID: event.ActorID, + ActorType: event.ActorType, + ActorEmail: event.ActorEmail, + Action: event.Action, + ResourceType: event.ResourceType, + ResourceID: event.ResourceID, + IPAddress: event.IPAddress, + UserAgent: event.UserAgent, + Metadata: metadataWithProtocol(event.Metadata, event.Protocol), + } +} +``` + +- [ ] **Step 4: Rewrite `LogEvent` to use the builder** + +Replace the body of `LogEvent` so it reads: + +```go +// LogEvent asynchronously records an audit log entry. +func (p *provider) LogEvent(event Event) { + asyncutil.Go(p.deps.Log, func() { + log := p.deps.Log.With().Str("func", "LogEvent").Logger() + if err := p.deps.StorageProvider.AddAuditLog(context.Background(), buildAuditLog(event)); err != nil { + log.Debug().Err(err).Str("action", event.Action).Msg("Failed to add audit log") + } + }) +} +``` + +- [ ] **Step 5: Add `LogEventSync` to the interface** + +In the `Provider` interface, below `LogEvent`: + +```go + // LogEventSync records an audit log entry synchronously and returns the + // storage error. + // + // For operations where the audit record is part of the contract, not a + // side effect: an authorization change that is not evidenced is a change + // nobody can account for. Everything else — logins, token issuance — + // keeps LogEvent, so the audit table stays off those hot paths. + // + // The caller decides what a failure means. For authorization changes the + // convention is to return the error and NOT compensate: by the time this + // is called the change has already been applied, so the error means + // "applied but unevidenced", not "nothing happened". + LogEventSync(ctx context.Context, event Event) error +``` + +- [ ] **Step 6: Implement `LogEventSync`** + +Below `LogEvent` in `internal/audit/provider.go`: + +```go +// LogEventSync records an audit log entry synchronously. +func (p *provider) LogEventSync(ctx context.Context, event Event) error { + return p.deps.StorageProvider.AddAuditLog(ctx, buildAuditLog(event)) +} +``` + +Note it takes the caller's `ctx`, unlike `LogEvent` which uses `context.Background()` because it outlives the request. + +- [ ] **Step 7: Add the stub to the existing test double** + +`internal/service/admin_access_invite_test.go` has `type inviteAudit struct{ audit.Provider }` with a nil embedded interface. Widening `Provider` still compiles, but the first caller of the new method under that test would nil-panic. Add the stub now, before Phase 2 introduces that caller. After line 92: + +```go +func (inviteAudit) LogEventSync(_ context.Context, _ audit.Event) error { return nil } +``` + +Add `"context"` to that file's imports if absent. + +- [ ] **Step 8: Run the tests to verify they pass** + +```bash +go build ./... && go test ./internal/audit/ -run TestLogEventSync -v +``` + +Expected: both tests PASS. + +- [ ] **Step 9: Add the protocol-folding regression test** (Review Focus 1) + +Append to `internal/audit/provider_sync_test.go`: + +```go +func TestLogEventSync_FoldsProtocolIntoMetadataLikeLogEvent(t *testing.T) { + st := &fakeAuditStore{} + p := newTestProvider(st) + + require.NoError(t, p.LogEventSync(context.Background(), Event{ + Action: "admin.fga_tuples_written", + Protocol: "grpc", + Metadata: `{"count":2}`, + })) + + require.NotNil(t, st.got) + // Both keys must survive: the protocol the caller set, and the metadata it + // already had. A separate builder for the sync path would drop one. + assert.JSONEq(t, `{"protocol":"grpc","count":2}`, st.got.Metadata) +} +``` + +- [ ] **Step 10: Run it** + +```bash +go test ./internal/audit/ -v +``` + +Expected: all PASS, including the pre-existing `metadata_test.go` tests. + +- [ ] **Step 11: Verify no existing caller changed behaviour** + +```bash +go build ./... && go vet ./... && make test +``` + +Expected: build and vet clean, 0 test failures. `LogEvent`'s observable behaviour is unchanged — this is the regression check for Step 4. + +- [ ] **Step 12: Commit** + +```bash +make fmt +git add internal/audit/provider.go internal/audit/provider_sync_test.go internal/service/admin_access_invite_test.go +git commit -m "feat(audit): add LogEventSync for evidence-critical events + +Authorization changes need the audit record to be part of the +operation's contract, not a side effect. LogEvent stays fire-and-forget +so logins and token issuance keep the audit table off their hot path. + +Both paths share buildAuditLog so they cannot diverge on record +shape — in particular on folding Protocol into Metadata." +``` + +--- + +### Task 2: Record how a super-admin authenticated + +**Files:** +- Modify: `internal/constants/audit_event.go` +- Modify: `internal/token/provider.go:76-77` +- Modify: `internal/token/admin_token.go:41-63` +- Modify: `internal/authctx/principal.go:10-30` +- Modify: `internal/grpcsrv/interceptors/auth.go:181-184` +- Test: `internal/token/admin_auth_mode_test.go` (create) + +**Interfaces:** +- Consumes: nothing from Task 1. +- Produces: + - `constants.AuditAuthModeAdminSession = "admin_session"`, `constants.AuditAuthModeSharedSecret = "shared_secret"` + - `token.Provider.AdminAuthMode(gc *gin.Context) string` — one of the two constants, or `""` when the caller is not a super admin. + - `authctx.Principal.AuthMode string` + +Phase 2 reads `Principal.AuthMode` to populate the `auth_mode` key in audit metadata. + +- [ ] **Step 1: Add the constants** + +At the end of the actor-type block in `internal/constants/audit_event.go`: + +```go +// Audit auth-mode constants record HOW a super-admin authenticated. +// +// Super-admin is a single shared AdminSecret, or a session cookie derived from +// it — there is no per-admin identity, so an audit record cannot name a person. +// Recording the mode is the honest alternative: it says which credential was +// used, and makes a shared-secret action distinguishable from a dashboard +// session without inventing an identity the system does not have. +// +// The session HANDLE is never recorded. It is a live bearer credential and the +// dashboard renders the audit table. +const ( + // AuditAuthModeAdminSession means the caller presented a valid admin + // session cookie (dashboard login). + AuditAuthModeAdminSession = "admin_session" + // AuditAuthModeSharedSecret means the caller presented the + // x-authorizer-admin-secret header. + AuditAuthModeSharedSecret = "shared_secret" +) +``` + +- [ ] **Step 2: Write the failing test** + +Create `internal/token/admin_auth_mode_test.go`. The existing `internal/token/admin_token_test.go` already provides `newGinCtx(header, value)` and `newProvider(adminSecret, disableHeaderAuth)` in this same package — reuse them, do not add parallel helpers. + +```go +package token + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/authorizerdev/authorizer/internal/constants" +) + +const adminSecretHeader = "x-authorizer-admin-secret" + +func TestAdminAuthMode_SharedSecretWhenHeaderMatches(t *testing.T) { + p := newProvider("correct-secret", false) + assert.Equal(t, constants.AuditAuthModeSharedSecret, + p.AdminAuthMode(newGinCtx(adminSecretHeader, "correct-secret"))) +} + +func TestAdminAuthMode_EmptyWhenNotSuperAdmin(t *testing.T) { + p := newProvider("correct-secret", false) + assert.Equal(t, "", p.AdminAuthMode(newGinCtx(adminSecretHeader, "wrong-secret"))) + assert.Equal(t, "", p.AdminAuthMode(newGinCtx(adminSecretHeader, ""))) + assert.Equal(t, "", p.AdminAuthMode(newGinCtx("", ""))) +} + +// IsSuperAdmin is re-expressed in terms of AdminAuthMode, so they must agree +// on every input. A divergence would mean a caller is admitted as super-admin +// while the audit record says they are not an admin at all. +func TestIsSuperAdmin_AgreesWithAdminAuthMode(t *testing.T) { + p := newProvider("correct-secret", false) + for _, secret := range []string{"correct-secret", "wrong-secret", ""} { + gc := newGinCtx(adminSecretHeader, secret) + assert.Equal(t, p.AdminAuthMode(gc) != "", p.IsSuperAdmin(gc), + "disagreement for secret %q", secret) + } +} +``` + +- [ ] **Step 3: Run the test to verify it fails** + +```bash +go test ./internal/token/ -run TestAdminAuthMode -v +``` + +Expected: compile failure — `p.AdminAuthMode undefined`. + +- [ ] **Step 4: Add the interface method** + +In `internal/token/provider.go`, directly below the `IsSuperAdmin` declaration at line 77: + +```go + // AdminAuthMode reports HOW the caller authenticated as super admin: + // constants.AuditAuthModeAdminSession, constants.AuditAuthModeSharedSecret, + // or "" when the caller is not a super admin at all. + // + // IsSuperAdmin is defined as AdminAuthMode(gc) != "", so the two cannot + // disagree about who is an admin. + AdminAuthMode(gc *gin.Context) string +``` + +- [ ] **Step 5: Implement it and re-express `IsSuperAdmin`** + +Replace `IsSuperAdmin` in `internal/token/admin_token.go` with: + +```go +// AdminAuthMode reports how the caller authenticated as super admin, or "" if +// they did not. This is the single place that decision is made; IsSuperAdmin +// is a thin predicate over it. +func (p *provider) AdminAuthMode(gc *gin.Context) string { + token, err := p.GetAdminAuthToken(gc) + if err == nil && token != "" { + return constants.AuditAuthModeAdminSession + } + if p.config.DisableAdminHeaderAuth { + return "" + } + // Reject header auth if no AdminSecret is configured — an unconfigured + // secret must never grant super-admin access. + if p.config.AdminSecret == "" { + return "" + } + secret := gc.Request.Header.Get("x-authorizer-admin-secret") + if secret == "" { + return "" + } + // Throttled: this header is an unauthenticated guess at the single + // highest-privilege credential in the system, and the only limiter in + // front of it used to be the shared 30rps budget ordinary traffic gets. + if valid, _ := p.VerifyAdminSecret(utils.GetIP(gc.Request), secret); valid { + return constants.AuditAuthModeSharedSecret + } + return "" +} + +// IsSuperAdmin checks if user is super admin +func (p *provider) IsSuperAdmin(gc *gin.Context) bool { + return p.AdminAuthMode(gc) != "" +} +``` + +Add `"github.com/authorizerdev/authorizer/internal/constants"` to the imports if absent. + +**Behaviour note:** the original returned `token != ""` when `GetAdminAuthToken` succeeded. `GetAdminAuthToken` already returns an error for an empty session id, so `err == nil && token != ""` is the same condition written explicitly. + +- [ ] **Step 6: Run the tests to verify they pass** + +```bash +go build ./... && go test ./internal/token/ -v +``` + +Expected: the new tests PASS **and** every pre-existing test in `admin_token_test.go` still passes — including `TestIsSuperAdmin_EmptyAdminSecretRejectsAllHeaderAuth` and `TestIsSuperAdmin_WrongSecretRejected`, which are the regression gate for this rewrite. + +- [ ] **Step 7: Add the config-edge tests** (Review Focus 2 and 3) + +Append to `internal/token/admin_auth_mode_test.go`: + +```go +// An unconfigured secret must never authenticate, and must never be recorded +// as though it had. +func TestAdminAuthMode_EmptyAdminSecretNeverReportsSharedSecret(t *testing.T) { + p := newProvider("", false) + assert.Equal(t, "", p.AdminAuthMode(newGinCtx(adminSecretHeader, ""))) + assert.Equal(t, "", p.AdminAuthMode(newGinCtx(adminSecretHeader, "anything"))) +} + +// With header auth disabled, even the correct secret is not super admin. +func TestAdminAuthMode_RespectsDisableAdminHeaderAuth(t *testing.T) { + p := newProvider("correct-secret", true) + assert.Equal(t, "", p.AdminAuthMode(newGinCtx(adminSecretHeader, "correct-secret"))) +} +``` + +- [ ] **Step 8: Run them** + +```bash +go test ./internal/token/ -run TestAdminAuthMode -v +``` + +Expected: all PASS. + +- [ ] **Step 9: Add `AuthMode` to `Principal`** + +In `internal/authctx/principal.go`, after the `ActorID` field: + +```go + // AuthMode records HOW a super-admin caller authenticated — one of + // constants.AuditAuthModeAdminSession or + // constants.AuditAuthModeSharedSecret. Empty for non-admin callers. + // + // Super-admin has no per-admin identity (one shared AdminSecret), so an + // audit record cannot name a person. This records which credential was + // used instead of leaving the question blank. Never the session handle + // itself. + AuthMode string +``` + +Do not import `constants` here — the field is a plain string and `authctx` is imported widely; keep it dependency-free. + +- [ ] **Step 10: Set it in the gRPC interceptor** + +In `internal/grpcsrv/interceptors/auth.go`, replace lines 181-184: + +```go + if mode := tp.AdminAuthMode(gc); mode != "" { + ctx = authctx.WithPrincipal(ctx, &authctx.Principal{IsSuperAdmin: true, AuthMode: mode}) + return handler(ctx, req) + } +``` + +This is the same decision as before — `AdminAuthMode(gc) != ""` is exactly `IsSuperAdmin(gc)` — but it carries the mode through instead of discarding it. + +- [ ] **Step 11: Verify the whole tree** + +```bash +go build ./... && go vet ./... && make test +``` + +Expected: build and vet clean, 0 test failures. The interceptor's admit/deny behaviour is unchanged; only the principal gained a field. + +- [ ] **Step 12: Commit** + +```bash +make fmt +git add internal/constants/audit_event.go internal/token/provider.go internal/token/admin_token.go internal/token/admin_auth_mode_test.go internal/authctx/principal.go internal/grpcsrv/interceptors/auth.go +git commit -m "feat(audit): record how a super-admin authenticated + +Super-admin is one shared AdminSecret with no per-admin identity, so an +audit record cannot name a person. Recording the credential MODE is the +honest alternative, and makes a shared-secret action distinguishable +from a dashboard session. + +IsSuperAdmin is now a predicate over AdminAuthMode so the admit decision +and the recorded mode cannot disagree. The session handle is never +recorded — it is a live bearer credential and the dashboard renders this +table." +``` + +--- + +### Task 3: Make `AuditProvider` reachable from SCIM + +**Files:** +- Modify: `internal/service/scim/scim.go:86-100` +- Modify: `cmd/root.go:815-821` +- Test: `internal/service/scim/audit_wiring_test.go` (create) + +**Interfaces:** +- Consumes: `audit.Provider` (unchanged by Task 1 for this purpose — only the field type matters). +- Produces: `scim.Dependencies.AuditProvider audit.Provider`, and `(*provider).logAuditSync(ctx, audit.Event) error` — a nil-safe wrapper Phase 2 calls from the SCIM group paths. + +**Why this is in Phase 1 rather than Phase 2:** the SCIM package has no `AuditProvider` *at all*, so it cannot audit anything. That absence is structural and blocks four of the eight call sites in Phase 2. Wiring it here keeps Phase 2 to one concern. It does mean Phase 1 ships a dependency with no caller yet — accepted deliberately, and Step 1's test pins the nil-safety contract so the field is not merely decorative. + +- [ ] **Step 1: Write the failing test** + +Create `internal/service/scim/audit_wiring_test.go`: + +```go +package scim + +import ( + "context" + "errors" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/audit" +) + +type recordingAudit struct { + audit.Provider + got audit.Event + err error +} + +func (r *recordingAudit) LogEventSync(_ context.Context, e audit.Event) error { + if r.err != nil { + return r.err + } + r.got = e + return nil +} + +// scim's concrete type embeds Dependencies BY VALUE (`type provider struct { +// Dependencies }`), so fields are reached as p.AuditProvider, not p.deps.X. +func newAuditTestProvider(ap audit.Provider) *provider { + log := zerolog.Nop() + return &provider{Dependencies: Dependencies{Log: &log, AuditProvider: ap}} +} + +// A deployment that does not wire audit must still serve SCIM, matching the +// documented EventsProvider convention. +func TestLogAuditSync_NilProviderIsNoOp(t *testing.T) { + p := newAuditTestProvider(nil) + assert.NotPanics(t, func() { + require.NoError(t, p.logAuditSync(context.Background(), audit.Event{Action: "x"})) + }) +} + +func TestLogAuditSync_ForwardsEvent(t *testing.T) { + ra := &recordingAudit{} + p := newAuditTestProvider(ra) + + require.NoError(t, p.logAuditSync(context.Background(), audit.Event{Action: "scim.group_members_added"})) + + assert.Equal(t, "scim.group_members_added", ra.got.Action) +} + +func TestLogAuditSync_PropagatesError(t *testing.T) { + boom := errors.New("audit unavailable") + p := newAuditTestProvider(&recordingAudit{err: boom}) + + assert.ErrorIs(t, p.logAuditSync(context.Background(), audit.Event{Action: "x"}), boom) +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +go test ./internal/service/scim/ -run TestLogAuditSync -v +``` + +Expected: compile failure — `unknown field AuditProvider` and `p.logAuditSync undefined`. + +- [ ] **Step 3: Add the dependency field** + +In `internal/service/scim/scim.go`, after `EventsProvider` in `Dependencies`: + +```go + // AuditProvider records authorization-relevant SCIM operations. SCIM is + // driven by an external IdP, so its writes are the least supervised + // authorization changes in the system — and until this field existed the + // package could not audit at all. + // + // Nil when audit is not wired — logging is then a no-op, matching the + // EventsProvider convention above. + AuditProvider audit.Provider +``` + +Add `"github.com/authorizerdev/authorizer/internal/audit"` to the imports. + +- [ ] **Step 4: Add the nil-safe wrapper** + +In `internal/service/scim/scim.go`, below the `Dependencies` struct: + +```go +// logAuditSync records an audit event synchronously, returning the storage +// error. A nil AuditProvider is a no-op returning nil. +// +// Callers decide what a failure means: the SCIM group paths that treat a tuple +// write as non-fatal must treat a failed audit the same way, or an accepted +// partial failure becomes a failed deprovision. +func (p *provider) logAuditSync(ctx context.Context, event audit.Event) error { + if p.AuditProvider == nil { + return nil + } + return p.AuditProvider.LogEventSync(ctx, event) +} +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +```bash +go build ./... && go test ./internal/service/scim/ -run TestLogAuditSync -v +``` + +Expected: all three PASS. + +- [ ] **Step 6: Wire it in `cmd/root.go`** + +At `cmd/root.go:815`, add to the `scim.Dependencies` literal: + +```go + AuditProvider: auditProvider, +``` + +`auditProvider` is already in scope — constructed at `cmd/root.go:704`. + +- [ ] **Step 7: Verify the whole tree** + +```bash +go build ./... && go vet ./... && make test +``` + +Expected: build and vet clean, 0 test failures. + +- [ ] **Step 8: Confirm the wiring actually reaches a running server** + +```bash +make smoke +``` + +Expected: PASS. `make smoke` builds the real binary and boots it, so it is the only check that proves `cmd/root.go` still wires a working server — a `Dependencies` literal mistake would not show up in unit tests. Per `AGENTS.md`, smoke is the right gate whenever `cmd/root.go` is touched. + +- [ ] **Step 9: Commit** + +```bash +make fmt +git add internal/service/scim/scim.go internal/service/scim/audit_wiring_test.go cmd/root.go +git commit -m "feat(scim): give the SCIM service an audit provider + +scim.Dependencies had no AuditProvider at all, so the package could not +audit anything — an external IdP could provision users, create org +memberships, revoke access and rewrite FGA group tuples leaving no audit +row. That absence is structural, and blocks half the call sites in the +next phase. + +Nil-safe, matching the EventsProvider convention. No events are emitted +yet; the call sites land with the phase that needs them." +``` + +--- + +## Deviations from the spec + +Two, both deliberate; a reviewer comparing plan to spec should see them called out rather than assume an omission. + +1. **Spec §1.4 says the shared actor-resolution helper that populates `ActorID`/`ActorEmail` is "written now".** This plan does not write it. Every FGA operation is super-admin gated, so in Phase 1's scope that helper would have zero callers — speculative code with no test that can fail meaningfully. Phase 2b brings the org-admin lanes that actually use it, and it should land there with its first consumer. Only `auth_mode` is implemented here, which is what Phase 2 consumes. + +2. **Spec §1.2's failure semantics are documented, not coded.** The "return the error, do not compensate" contract belongs to the *callers* of `LogEventSync`, and all of them arrive in Phase 2. It is captured in the interface doc comment on `LogEventSync` so the contract travels with the method rather than living only in the spec. + +**Honest framing for the PR:** this phase adds no new audit records and changes no observable behaviour. Its tests are unit-level by necessity — the consumers arrive in Phase 2. It is split out because Phase 2 is already 8 call sites plus snapshots plus a static guard test, and bundling the plumbing would make it unreviewable. + +## Final verification before opening the PR + +- [ ] `go build ./...` — clean +- [ ] `go vet ./...` — clean +- [ ] `make test` — 0 failures +- [ ] `make smoke` — PASS (required: `cmd/root.go` changed) +- [ ] `make lint` — clean. If a local `golangci-lint` of a version other than the pinned `v2.11.4` is on PATH, it will report findings CI does not; trust CI. +- [ ] No storage provider changed, so no non-SQL backend run is required. Confirm with `git diff --stat main -- internal/storage/` returning empty. +- [ ] Branch is `feat/audit-sync-and-actor-mode`, not `main`. +- [ ] PR body states plainly that this phase adds no new audit records — it is the plumbing Phase 2 consumes — so a reviewer does not go looking for behaviour changes that are not there. +- [ ] Request `security-engineer` review: this touches admin auth context and the audit path.