[ADMINAPI-1479] [ADMINAPI-1327] Missing Audit Logging Across Critical Operations

[jagudelo-gap]

This pull request introduces a comprehensive audit logging infrastructure to the codebase, including middleware for capturing mutating API actions, event recording, background processing with retry/fallback logic, and configuration for enabling/disabling audit logging. It also adds unit tests to ensure the reliability of each component. The most important changes are grouped below.

Audit Logging Core Implementation

• Added AuditActionLoggingMiddleware to capture and record mutating HTTP actions (POST, PUT, PATCH, DELETE) as audit events, including client ID, IP address, HTTP verb, URL, and status code. Handles both successful and failed requests.
• Introduced AuditEventRecorder and IAuditEventRecorder for recording audit events, supporting tenant-aware and fallback connection string resolution, and fail-open error handling. [1] [2]
• Defined AuditEvent and AuditEventType to standardize audit event data and event types (authentication success/failure and actions). [1] [2]
• Added AuditLogBackgroundService to process audit events from a bounded channel, with retry logic and fallback logging on repeated failures.
• Implemented AuditLogChannel for thread-safe, bounded, single-reader/multi-writer event queuing.
• Added configuration class AuditLoggingSettings to enable/disable audit logging.
• Added AuditLog entity for persistence mapping of audit events.

Test Coverage

• Added unit tests for AuditActionLoggingMiddleware, AuditEventRecorder, and AuditLogBackgroundService to verify correct event recording, connection string resolution, error handling, and retry/fallback behavior. [1] [2] [3]

Assembly Configuration

• Updated AssemblyInfo.cs to allow internal visibility for unit tests.

[github-actions]

Test Results

   15 files     15 suites   1m 12s ⏱️
1 815 tests 1 801 ✅ 14 💤 0 ❌
4 267 runs  4 229 ✅ 38 💤 0 ❌

Results for commit 4a72f49.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

Adds an audit trail logging subsystem to Admin API so authentication outcomes and mutating administrative actions can be recorded to a database-backed adminapi.AuditLogs table (with background processing and “fail-open” behavior), plus supporting docs and tests.

Changes:

• Introduces audit capture points (middleware + auth hooks) and an async write pipeline (bounded channel + hosted background service + per-version EF writer).
• Adds DbUp migrations + EF mappings for the new AuditLogs table across SQL Server and PostgreSQL, plus DB/integration tests and local DB test runner scripts.
• Updates developer documentation to describe the audit logging feature and configuration.

Reviewed changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
eng/run-db-tests.ps1	Adds a local helper script to run DBTests against containerized DB engines and apply migrations.
eng/db-tests-compose.yml	Adds bare SQL Server/PostgreSQL containers for DBTests/migration verification.
docs/developer.md	Links audit logging docs from the main developer guide.
docs/design/plan-b-log4net-to-serilog-migration.md	Removes an obsolete design plan document.
docs/design/2026-07-28-audit-trail-logging.md	Adds the technical design write-up for audit trail logging.
docs/audit-logging.md	Adds operator/developer documentation for enabling audit logging and understanding captured events/table schema.
Application/EdFi.Ods.AdminApi/Program.cs	Registers the audit action logging middleware in the request pipeline.
Application/EdFi.Ods.AdminApi/Infrastructure/WebApplicationBuilderExtensions.cs	Wires up audit logging services (settings, channel, recorder, writer, hosted service).
Application/EdFi.Ods.AdminApi/Infrastructure/Security/SecurityExtensions.cs	Records authentication success/failure audit events (token response handler + 401 challenge).
Application/EdFi.Ods.AdminApi/Infrastructure/Audit/AdminApiAuditLogWriter.cs	Adds V2 audit log writer implementation using EF Core.
Application/EdFi.Ods.AdminApi/Infrastructure/AdminApiDbContext.cs	Adds DbSet<AuditLog> and mapping for the AuditLogs table (V2).
Application/EdFi.Ods.AdminApi/Artifacts/PgSql/Structure/Admin/00007-CreateAuditLogs.sql	Adds PostgreSQL DbUp migration to create AuditLogs (V2).
Application/EdFi.Ods.AdminApi/Artifacts/MsSql/Structure/Admin/00007-CreateAuditLogs.sql	Adds SQL Server DbUp migration to create AuditLogs (V2).
Application/EdFi.Ods.AdminApi/appsettings.json	Adds AuditLogging:Enabled configuration flag (V2 host appsettings).
Application/EdFi.Ods.AdminApi.V3/Infrastructure/Audit/AdminApiAuditLogWriter.cs	Adds V3 audit log writer implementation using EF Core.
Application/EdFi.Ods.AdminApi.V3/Infrastructure/AdminApiDbContext.cs	Adds DbSet<AuditLog> and mapping for the AuditLogs table (V3).
Application/EdFi.Ods.AdminApi.V3/Artifacts/PgSql/Structure/Admin/00007-CreateAuditLogs.sql	Adds PostgreSQL DbUp migration to create AuditLogs (V3).
Application/EdFi.Ods.AdminApi.V3/Artifacts/MsSql/Structure/Admin/00007-CreateAuditLogs.sql	Adds SQL Server DbUp migration to create AuditLogs (V3).
Application/EdFi.Ods.AdminApi.V3/appsettings.json	Adds AuditLogging:Enabled configuration flag (V3 appsettings file).
Application/EdFi.Ods.AdminApi.V3.DBTests/Services/Jobs/JobStatusServiceTests.cs	Adjusts namespace aliasing for clarity in V3 DB tests.
Application/EdFi.Ods.AdminApi.V3.DBTests/Infrastructure/Audit/AdminApiAuditLogWriterTests.cs	Adds DB test validating audit log persistence for V3 writer.
Application/EdFi.Ods.AdminApi.V3.DBTests/Database/QueryTests/GetResourceClaimsQueryTests.cs	Adjusts namespace aliasing in V3 DB tests.
Application/EdFi.Ods.AdminApi.V3.DBTests/Database/QueryTests/GetResourceClaimsAsFlatListQueryTests.cs	Adjusts namespace aliasing in V3 DB tests.
Application/EdFi.Ods.AdminApi.UnitTests/Infrastructure/Security/SecurityExtensionsAuditTests.cs	Adds unit tests for auth audit event recording behavior.
Application/EdFi.Ods.AdminApi.DBTests/Services/Jobs/JobStatusServiceTests.cs	Adjusts DbContext aliasing for clarity in V2 DB tests.
Application/EdFi.Ods.AdminApi.DBTests/Infrastructure/Audit/AdminApiAuditLogWriterTests.cs	Adds DB test validating audit log persistence for V2 writer.
Application/EdFi.Ods.AdminApi.DBTests/Database/QueryTests/GetResourceClaimsQueryTests.cs	Adjusts namespace qualification in V2 DB tests.
Application/EdFi.Ods.AdminApi.DBTests/Database/QueryTests/GetResourceClaimsAsFlatListQueryTests.cs	Adjusts namespace qualification in V2 DB tests.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/IAuditLogWriter.cs	Adds interface contract for audit log persistence.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/IAuditEventRecorder.cs	Adds interface contract for recording audit events.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditLoggingSettings.cs	Adds configuration binding object for AuditLogging.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditLogChannel.cs	Adds bounded channel wrapper for audit event buffering.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditLogBackgroundService.cs	Adds background service to drain channel and write events with retry/fallback.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditLog.cs	Adds EF entity for persisted audit logs.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditEventType.cs	Adds enum defining audit event types.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditEventRecorder.cs	Adds recorder that builds/enqueues events and resolves tenant connection string.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditEvent.cs	Adds in-memory audit event DTO used by the channel/writer.
Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditActionLoggingMiddleware.cs	Adds middleware capturing mutating HTTP action audit events.
Application/EdFi.Ods.AdminApi.Common/AssemblyInfo.cs	Grants internal visibility to common unit tests.
Application/EdFi.Ods.AdminApi.Common.UnitTests/Infrastructure/Audit/AuditLogBackgroundServiceTests.cs	Adds unit tests for background service retry/fallback behavior.
Application/EdFi.Ods.AdminApi.Common.UnitTests/Infrastructure/Audit/AuditEventRecorderTests.cs	Adds unit tests for recorder enable/disable and connection string resolution.
Application/EdFi.Ods.AdminApi.Common.UnitTests/Infrastructure/Audit/AuditActionLoggingMiddlewareTests.cs	Adds unit tests for middleware verb filtering and exception handling.

[josephcampos-gap]

Review finding: ClientId will be null on every audited action in production

tl;dr: AuditActionLoggingMiddleware reads the client id from a claim named "client_id", but the token issued by TokenService never creates a claim with that name — it only adds sub (the OpenIddict Subject claim) and name. As written, every Action audit row this PR produces (every POST/PUT/PATCH/DELETE — the entire class of write this ticket exists to cover) will have ClientId = null in production. This silently defeats NFR-AUDIT-2 (log client_id for actions), which is the core Trustwave finding that opened ADMINAPI-1479: "users-global, teams-global, ownerships-global, and user-team-memberships services... perform create, update, and delete operations on sensitive resources without generating audit trails." Auth-event logging (NFR-AUDIT-1) is unaffected and correctly implemented — this only affects the action-logging side.

Why the existing test doesn't catch it

AuditActionLoggingMiddlewareTests.cs builds its test principal with a hand-picked claim:

context.User = new ClaimsPrincipal(new ClaimsIdentity(
    [new Claim("client_id", clientId)], "test"));

That claim name was invented for the test, not derived from how tokens are actually issued. Real principals never carry it — see TokenService.Handle, which only ever does:

identity.AddClaim(OpenIddictConstants.Claims.Subject, request.ClientId!, OpenIddictConstants.Destinations.AccessToken);
identity.AddClaim(OpenIddictConstants.Claims.Name, displayName!, OpenIddictConstants.Destinations.AccessToken);

OpenIddictConstants.Claims.Subject resolves to the standard JWT claim "sub". This is shared code — V3 uses the same TokenService//connect/token path — so the bug affects both API versions equally.

Suggested fix

1. Application/EdFi.Ods.AdminApi.Common/Infrastructure/Audit/AuditActionLoggingMiddleware.cs:25

-        var clientId = context.User.FindFirst("client_id")?.Value;
+        // TokenService issues the client id as the standard "sub" claim (OpenIddictConstants.Claims.Subject),
+        // never as a claim literally named "client_id".
+        var clientId = context.User.FindFirst("sub")?.Value;

(Using the literal "sub" instead of pulling in OpenIddictConstants.Claims.Subject avoids adding an OpenIddict.Abstractions package reference to EdFi.Ods.AdminApi.Common, which doesn't currently depend on it.)

2. Fix the existing test fixture so it matches reality instead of confirming itself — in AuditActionLoggingMiddlewareTests.cs:

         if (clientId != null)
         {
             context.User = new ClaimsPrincipal(new ClaimsIdentity(
-                [new Claim("client_id", clientId)], "test"));
+                [new Claim("sub", clientId)], "test"));
         }

This alone makes all four existing tests exercise the corrected code path.

3. Add a regression test that builds the principal the way TokenService.Handle actually does, so the claim-name assumption can't silently drift again:

[Test]
public async Task InvokeAsync_UsesClientIdFromRealIssuedPrincipal()
{
    // Mirrors TokenService.Handle's claim construction, not a hand-picked claim name.
    var identity = new ClaimsIdentity(JwtBearerDefaults.AuthenticationScheme);
    identity.AddClaim(OpenIddictConstants.Claims.Subject, "client-1", OpenIddictConstants.Destinations.AccessToken);
    identity.AddClaim(OpenIddictConstants.Claims.Name, "Test Client", OpenIddictConstants.Destinations.AccessToken);
    var principal = new ClaimsPrincipal(identity);

    var recorder = A.Fake<IAuditEventRecorder>();
    var middleware = new AuditActionLoggingMiddleware(_ => Task.CompletedTask, recorder);
    var context = new DefaultHttpContext { User = principal };
    context.Request.Method = "POST";
    context.Request.Path = "/v3/apiClients";
    context.Response.StatusCode = 201;

    await middleware.InvokeAsync(context);

    A.CallTo(() => recorder.Record(
        AuditEventType.Action, "client-1", A<string?>._, "POST", "/v3/apiClients", 201))
        .MustHaveHappenedOnceExactly();
}

If pulling in OpenIddictConstants/JwtBearerDefaults isn't worth a new test-project dependency, hardcoding "sub" directly in the test with a comment tying it back to TokenService is an acceptable simplification — the important part is that the test derives from real issuance semantics, not an arbitrary claim name.

4. docs/audit-logging.md:63 — update the ClientId column description; it currently says the value comes from a client_id claim, which should read sub (the client id, per TokenService) once the fix lands.

Scope check

Everything else in this PR looked solid on review: the fail-open channel/background-writer design, migration parity across MsSql/PgSql for both v2 and v3 artifact copies (including the later ClientId width fix), the honestly-documented known gaps (double-logging on /connect/token, 403s not audited, natural-key stretch goal explicitly deferred), and auth-event logging (which correctly sources the client id from sub, not from a client_id claim). This is a scoped, single-line-plus-tests fix, not a rework — no changes needed to the channel, writer, retry logic, or migrations.

[josephcampos-gap]

Follow-up / partial retraction: my "ClientId will always be null" claim was wrong

Thanks for pushing back with the Docker screenshot — that's real evidence and it contradicts what I asserted earlier. Correcting the record:

What I got wrong: I traced the claim only through TokenService.Handle (Application/EdFi.Ods.AdminApi/Features/Connect/TokenService.cs:56-64), which explicitly adds sub and name — no claim literally named client_id. From that alone I concluded AuditActionLoggingMiddleware's FindFirst("client_id") must always miss. That was an overreach: static tracing of one method isn't the same as observing the actual issued token, and your screenshot shows clientid correctly populated for real mutating requests (POST /v3/vendors, POST /v3/claimSets).

I grepped the whole repo for anything that could be injecting a client_id claim onto the principal — IClaimsTransformation implementations, OnTokenValidated enrichment, any explicit AddClaim("client_id", ...) — and found nothing in app code. So if it's populating correctly in your environment, the claim is most likely coming from OpenIddict's own token/claim handling rather than anything this app's code does explicitly. I wasn't able to fully confirm that mechanism through static analysis of the OpenIddict packages, so I don't want to assert a "why" I can't back up with certainty.

Ask, so we can close this out with confidence: could you decode the actual JWT from one of the requests where clientid came through populated (e.g. paste the access token into jwt.io, or just base64-decode the payload segment) and confirm whether client_id is literally a claim key in the token itself? That tells us definitively whether:

• (a) my finding was simply wrong and no code change is needed here, or
• (b) it's working in this environment by some path we haven't identified, which would be worth understanding before relying on it everywhere.

What still stands from the original finding: the two NULL rows (/connect/register, /connect/token) are expected, not a bug — those are the auth endpoints themselves, hit with no bearer token yet, so context.User is anonymous when the middleware runs. That's consistent with the "double-logging on /connect/token" gap already called out in docs/audit-logging.md, not a new issue.

On "users-global, teams-global, ownerships-global, user-team-memberships" — good catch, and you're right to question it. I checked this repo's actual feature/route names (ApiClients, Applications, ClaimSets, Vendors, ResourceClaims, Tenants, Profiles, etc.) and none of the names quoted in the Jira ticket exist anywhere in ODS-Admin-API. Those almost certainly come from the Ed-Fi Admin App (a separate product with Users/Teams/Ownership screens) — the Trustwave finding text looks like it was quoted verbatim into this Admin API ticket even though it doesn't literally describe this codebase. That's worth flagging back to whoever triaged ADMINAPI-1479, but it isn't a gap in this PR: the generic mutating-verb middleware here audits every real Admin API route regardless of resource name, so the actual acceptance criteria (audit all creates/updates/deletes) is still met.

Sorry for the noise on the first point — appreciate you checking against real runtime behavior instead of taking the static claim at face value.

[josephcampos-gap]

LGTM