Skip to content

feat(audit): synchronous audit writes and super-admin auth mode (Phase 1) - #806

Merged
lakhansamani merged 5 commits into
mainfrom
feat/audit-sync-and-actor-mode
Oct 6, 2026
Merged

lakhansamani merged 5 commits into
mainfrom
feat/audit-sync-and-actor-mode

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Phase 1 of the authorization-change-evidence work. Design: authorizerdev/docs#96 (§Phase 1).

What this is

Plumbing only. This branch adds no new audit records and changes no observable behaviour — its consumers arrive in Phase 2. It is split out because Phase 2 is already eight call sites plus snapshots plus a static guard test, and bundling the plumbing would make that unreviewable.

Three pieces:

  • audit.Provider.LogEventSync(ctx, Event) error — a synchronous audit write that returns its error, for operations where the audit record is part of the contract rather than a side effect. LogEvent keeps its exact signature and fire-and-forget semantics, so logins and token issuance never gain a synchronous dependency on the audit table. Both share one buildAuditLog, so the two paths cannot drift on record shape.
  • token.Provider.AdminAuthMode(gc) string — records how a super-admin authenticated (admin_session vs shared_secret). Super-admin is one shared AdminSecret with no per-admin identity, so an audit record can never name a person; recording the credential mode is the honest substitute. IsSuperAdmin is now return p.AdminAuthMode(gc) != "", so the admit decision and the recorded mode cannot disagree.
  • scim.Dependencies.AuditProvider + a nil-safe logAuditSync wrapper. The SCIM package previously had no audit provider at all — an external IdP could provision users, create org memberships and revoke access leaving no audit row. That absence is structural and blocks half of Phase 2's call sites.

Security notes

The admin session handle is a live bearer credential and the dashboard renders the audit table, so it is never recorded — only the two mode constants ever escape AdminAuthMode.

The IsSuperAdmin rewrite was reviewed for admit-equivalence branch by branch across all six inputs (valid cookie, DisableAdminHeaderAuth, empty AdminSecret, empty header, wrong secret, correct secret). It is identical: err == nil && token != "" is the original return token != "" written explicitly, because GetAdminAuthToken never returns a nil error alongside an empty token. Branch ordering, the empty-secret reject, and the VerifyAdminSecret throttle/lockout all fire in the original order with the original call counts.

Verification

go build ./... and go vet ./... clean. make test exit 0 — 44 packages, 0 failures. make smoke passed including the scim_provisioning subtest, which is the only check that proves the cmd/root.go wiring still boots.

Lint: 7 findings repo-wide, none in any file this branch touches. They come from a local golangci-lint v2.12.2; the repo pins v2.11.4, and the Makefile installs the pinned version only when none is already on PATH.

No storage provider changed, so no non-SQL backend run is required.

Review history

Built task-by-task with a fresh implementer and a fresh reviewer per task, then a whole-branch review. One Important finding in each stage, both fixed:

  1. The admin_session branch of AdminAuthMode had no test — the two mode constants could have been swapped with nothing failing. Fixed in dfb39b25, and mutation-tested (constant swapped, test confirmed failing, restored).
  2. Three test doubles embed an interface rather than implement it. Widening audit.Provider and token.Provider left them compiling but nil-panicking at runtime on any un-overridden method. Two bit during development (inviteAudit, stubTokenProvider); eb1a6e4f closes the remaining three before Phase 2 adds the callers.

Carried into Phase 2 (recorded in the design doc)

Do not read Principal.AuthMode directly. The gRPC interceptor is its only writer; GraphQL and REST construct no Principal, which is exactly why requireSuperAdmin falls back to TokenProvider.IsSuperAdmin(gc). A Phase 2 that reads the field would record auth_mode: "" for a super-admin acting from the dashboard — the most common admin path. Phase 2 must derive the mode at the service gate via AdminAuthMode(gc), treating Principal.AuthMode as a gRPC fast path. Found by the whole-branch review; spec amended in authorizerdev/docs@d6f4d69.

LogEventSync passes the request context straight through. A client disconnect between applying a change and writing its row yields the "applied but unevidenced" outcome — client-triggerable, not only a DB outage. Repo convention for detached work is context.WithoutCancel(ctx). Deliberately not changed here: it would alter the signature's meaning with no caller to validate against. Phase 2 should decide it once in the provider so all eight call sites inherit it.

Requesting security-engineer review — this touches admin auth context and the audit path.

@lakhansamani

Copy link
Copy Markdown
Contributor Author

CI note: the govulncheck failure here is not caused by this branch.

GO-2026-6505 (OpenTelemetry OTLP exporter can log endpoint URLs at info level) was published 2026-10-01. This PR was simply the first run after that date — main's last scan was 09-07 and #802's was 09-29, both green, both before publication.

Evidence:

  • This branch touches neither go.mod nor go.sum (git diff c9442a3f..HEAD -- go.mod go.sum is empty).
  • otel is v1.44.0 identically on main and here.
  • Every reported call trace runs through openfga, cassandradb and metrics — none of this branch's files.
  • A clean worktree of unmodified origin/main fails the same scan with exit 3.

Fix is in #807 (otel v1.44.0 → v1.45.0, indirect deps only, govulncheck exit 0, make test and make smoke both green). Once #807 merges this needs a rebase and nothing more.

The other three checks here — Lint, Go tests (SQLite), Release smoke — are all green.

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.
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.

Also updates the gRPC interceptor's stubTokenProvider test double, which
embeds token.Provider and overrides specific methods: it needed an
AdminAuthMode override to match, since the interceptor now calls that
instead of IsSuperAdmin.
The cookie-auth path (dashboard login) returning
constants.AuditAuthModeAdminSession had no test reaching it - every
existing test drove only the header/secret path, so the two mode
constants could have been swapped on that branch without anything
failing. That mapping is the one new behavior this task exists to
produce.

Adds a minimal memory_store.Provider fake (GetCache only, same
embed-and-override pattern as interceptors.stubTokenProvider) and a
session-backed provider fixture built on top of the existing
newProvider helper, then asserts both AdminAuthMode and IsSuperAdmin
agree on a valid session cookie.
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.
Three test doubles embed `token.Provider` or `audit.Provider` but override
only a subset of methods. Embedding an interface compiles fine, but calling
an overridden method on the unoverridden embedded pointer panics.

This branch added two new interface methods: token.AdminAuthMode and
audit.LogEventSync. Production code will call AdminAuthMode inside the
requireSuperAdmin guard and LogEvent inside SCIM paths. Add minimal
overrides to test doubles that would panic today:

- inviteToken in admin_access_invite_test: add AdminAuthMode override
  consistent with existing IsSuperAdmin behaviour (returns admin_session mode)
- recordingAudit in audit_wiring_test: add fire-and-forget LogEvent override
- mcp_auth_test comment: reflect actual AdminAuthMode call, not IsSuperAdmin

All three tests pass with these guards in place.
@lakhansamani
lakhansamani force-pushed the feat/audit-sync-and-actor-mode branch from eb1a6e4 to c334d7f Compare October 6, 2026 10:22
@lakhansamani
lakhansamani merged commit 0a52675 into main Oct 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant