Repository navigation
feat(audit): synchronous audit writes and super-admin auth mode (Phase 1) - #806
Merged
Merged
Conversation
Contributor
Author
|
CI note: the 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 — Evidence:
Fix is in #807 (otel 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
force-pushed
the
feat/audit-sync-and-actor-mode
branch
from
October 6, 2026 10:22
eb1a6e4 to
c334d7f
Compare
This was referenced Oct 6, 2026
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
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.
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.LogEventkeeps its exact signature and fire-and-forget semantics, so logins and token issuance never gain a synchronous dependency on the audit table. Both share onebuildAuditLog, so the two paths cannot drift on record shape.token.Provider.AdminAuthMode(gc) string— records how a super-admin authenticated (admin_sessionvsshared_secret). Super-admin is one sharedAdminSecretwith no per-admin identity, so an audit record can never name a person; recording the credential mode is the honest substitute.IsSuperAdminis nowreturn p.AdminAuthMode(gc) != "", so the admit decision and the recorded mode cannot disagree.scim.Dependencies.AuditProvider+ a nil-safelogAuditSyncwrapper. 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
IsSuperAdminrewrite was reviewed for admit-equivalence branch by branch across all six inputs (valid cookie,DisableAdminHeaderAuth, emptyAdminSecret, empty header, wrong secret, correct secret). It is identical:err == nil && token != ""is the originalreturn token != ""written explicitly, becauseGetAdminAuthTokennever returns a nil error alongside an empty token. Branch ordering, the empty-secret reject, and theVerifyAdminSecretthrottle/lockout all fire in the original order with the original call counts.Verification
go build ./...andgo vet ./...clean.make testexit 0 — 44 packages, 0 failures.make smokepassed including thescim_provisioningsubtest, which is the only check that proves thecmd/root.gowiring 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:
admin_sessionbranch ofAdminAuthModehad no test — the two mode constants could have been swapped with nothing failing. Fixed indfb39b25, and mutation-tested (constant swapped, test confirmed failing, restored).audit.Providerandtoken.Providerleft them compiling but nil-panicking at runtime on any un-overridden method. Two bit during development (inviteAudit,stubTokenProvider);eb1a6e4fcloses the remaining three before Phase 2 adds the callers.Carried into Phase 2 (recorded in the design doc)
Do not read
Principal.AuthModedirectly. The gRPC interceptor is its only writer; GraphQL and REST construct noPrincipal, which is exactly whyrequireSuperAdminfalls back toTokenProvider.IsSuperAdmin(gc). A Phase 2 that reads the field would recordauth_mode: ""for a super-admin acting from the dashboard — the most common admin path. Phase 2 must derive the mode at the service gate viaAdminAuthMode(gc), treatingPrincipal.AuthModeas a gRPC fast path. Found by the whole-branch review; spec amended in authorizerdev/docs@d6f4d69.LogEventSyncpasses 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 iscontext.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-engineerreview — this touches admin auth context and the audit path.