Skip to content

Add the PAM access-audit event store - #8230

Draft
patriksvensson wants to merge 3 commits into
mainfrom
psvensson/pam-audit-log-scaffolding
Draft

Add the PAM access-audit event store#8230
patriksvensson wants to merge 3 commits into
mainfrom
psvensson/pam-audit-log-scaffolding

Conversation

@patriksvensson

Copy link
Copy Markdown
Member

🎟️ Tracking

PM-39047

📔 Objective

Adds the append-only store behind the PAM access-audit trail: the AccessAuditEvent
table, its two stored procedures, migrations for MSSQL and the three EF providers,
and the Dapper and EF implementations of IAccessAuditEventRepository.

Nothing calls it yet, and that's deliberate. The emitter that writes events and the
endpoint that reads the trail back are separate PRs. Landing the persistence layer on
its own keeps the schema reviewable without a feature's worth of code wrapped around
it. The parts DB Ops care about are the whole diff here, not a corner of it.

Two design decisions look like mistakes if you don't know the intent:

Rows are self-contained. AccessAuditEvent_Create snapshots the actor, requester,
cipher, collection, and rule display names into the row at write time. Reading the
trail then touches no other table, and a later rename can't rewrite history. The
subject ids are deliberately not foreign keyed for the same reason: an audit event
has to outlive what it references. Only OrganizationId is, so the rows are removed
with the organization.

The EF path resolves those names in C# instead of in the query. The stored
procedure pulls the cipher name out of its encrypted Data document with JSON_VALUE,
which has no portable translation across MySQL, Postgres, and SQLite.

Five DatabaseTheory integration tests cover the round trip. Two of them pin the
design above: GetManyByOrganizationId_SnapshotName_SurvivesEntityDeletion and
..._IsNotRewrittenByRename.

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.84946% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.62%. Comparing base (7f38699) to head (60f33d0).

Files with missing lines Patch % Lines
...ork/Pam/Repositories/AccessAuditEventRepository.cs 97.50% 0 Missing and 2 partials ⚠️
...ure.EntityFramework/Pam/Models/AccessAuditEvent.cs 95.45% 1 Missing ⚠️
src/Pam.Domain/Models/AccessAuditEvent.cs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8230      +/-   ##
==========================================
+ Coverage   68.57%   68.62%   +0.05%     
==========================================
  Files        2401     2406       +5     
  Lines      104043   104229     +186     
  Branches     9426     9431       +5     
==========================================
+ Hits        71349    71530     +181     
- Misses      30341    30344       +3     
- Partials     2353     2355       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread src/Sql/dbo/Pam/Stored Procedures/AccessAuditEvent_Create.sql
Comment thread src/Infrastructure.EntityFramework/Pam/Models/AccessAuditEvent.cs Outdated
@patriksvensson
patriksvensson force-pushed the psvensson/pam-audit-log-scaffolding branch from 7c202c4 to 60f33d0 Compare August 24, 2026 11:56
patriksvensson and others added 3 commits August 24, 2026 17:41
The append-only store the audit trail is written to and read back from: the
AccessAuditEvent table and its two stored procedures, a consolidated migration
for MSSQL plus generated ones for the EF providers, and the Dapper and EF
repositories behind IAccessAuditEventRepository.

Rows are self-contained. AccessAuditEvent_Create snapshots the actor,
requester, cipher, collection, and rule display names into the row at write
time, so the trail read touches no other table and a later rename or delete
cannot rewrite history. The subject ids are deliberately not foreign keyed for
the same reason -- an event outlives what it references. Only OrganizationId
is, so the rows go when the organization does. The EF path resolves those names
in C#, because JSON_VALUE -- which the procedure uses to read the cipher name
out of its encrypted Data document -- has no portable EF translation.

This is the persistence layer only; nothing consumes it yet. The emitter that
writes to it and the trail endpoint that reads from it are separate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit d06d9fa)
Remove CipherName and CollectionName from AccessAuditEvent end-to-end:
the table, both stored procedures (AccessAuditEvent_Create loses its
JSON_VALUE cipher-name read and its Collection join), the domain and EF
models, the EF repository's two name resolvers, and all three EF
migrations. The subject cipher and collection are now recorded by id
only, so no vault data lands in the audit store -- every snapshotted
name that remains is plaintext.

Page AccessAuditEvent_ReadManyByOrganizationId with @Skip/@take
(defaulting to 0/25), matching the paging idiom the rest of the
repository already uses. OccurredAt is not unique -- an action's Attempt
and Outcome are written with the same timestamp -- so the read orders by
[OccurredAt] DESC, [Id] DESC and the index takes Id as a third key;
without a total order an OFFSET page boundary could serve a row on two
pages or on neither, silently dropping an event from the trail.
Aligns the audit repositories and their tests with the rest of PAM: the
AccessRule, AccessRequest, AccessLease and AccessDecision entities, and
the other PAM integration tests, already generate identifiers with
CombGuid.Generate rather than CoreHelpers.GenerateComb.
@patriksvensson
patriksvensson force-pushed the psvensson/pam-audit-log-scaffolding branch from 60f33d0 to a3135ff Compare August 24, 2026 15:41
@patriksvensson
patriksvensson marked this pull request as ready for review August 24, 2026 15:49
@patriksvensson
patriksvensson requested review from a team as code owners August 24, 2026 15:49
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the net-new AccessAuditEvent persistence layer: the SSDT table and two stored procedures, the consolidated MSSQL migration, the three generated EF provider migrations, the Dapper and EF repository implementations, the domain models/enums, and the five DatabaseTheory integration tests. Dual-ORM parity holds — column types, nullability, the Organization cascade FK, and the (OrganizationId ASC, OccurredAt DESC, Id DESC) index all match between the MSSQL script and the EF model/migrations, and the migration script is idempotent, dated newest, and uses CREATE OR ALTER for both procedures. The snapshot-at-write-time design and the deliberate absence of foreign keys on the subject ids are documented in the schema, the models, and pinned by two dedicated tests; no vault data reaches the audit store, and the snapshotted name/email columns hold data that is already plaintext in [dbo].[User]. One finding, on the pagination strategy.

Code Review Details
  • ♻️ : OFFSET @Skip paging over an append-only log re-serves rows as events are appended, and cost grows with page depth; Event_ReadPageByOrganizationId uses a keyset cursor for the same reason
    • src/Sql/dbo/Pam/Stored Procedures/AccessAuditEvent_ReadManyByOrganizationId.sql:46

Comment on lines +46 to +48
ORDER BY [OccurredAt] DESC, [Id] DESC
OFFSET @Skip ROWS
FETCH NEXT @Take ROWS ONLY

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ DEBT: OFFSET @Skip paging over an append-only log re-serves rows as new events arrive, and gets slower with depth.

Details and fix

The [Id] tiebreaker fixes the equal-OccurredAt tie, but not the two problems that come from the store being append-only and (per the discussion on this file) potentially very large:

  1. Duplicate rows across pages. Under ORDER BY [OccurredAt] DESC, every event appended between two page requests enters at the front of the result set and shifts the whole window down by one. A client walking skip=0, 25, 50… while PAM actions are still emitting will see the same event on consecutive pages. The comment above claims the [Id] key prevents "double-serve", which holds for a static snapshot but not for a live trail.
  2. Cost grows with @Skip. Even with the covering index, SQL Server must read and discard @Skip index rows before returning the page. At page 400 of a busy org that is 10,000 discarded rows per request.

Bitwarden's existing event log solves both with a keyset cursor rather than an offset — see Event_ReadPageByOrganizationId, which takes a @BeforeDate continuation token and uses OFFSET 0 ROWS FETCH NEXT @PageSize. The equivalent here would be a @Before OccurredAt + @BeforeId pair:

WHERE [OrganizationId] = @OrganizationId
    AND [OccurredAt] >= @Since
    AND (@BeforeOccurredAt IS NULL
         OR [OccurredAt] < @BeforeOccurredAt
         OR ([OccurredAt] = @BeforeOccurredAt AND [Id] < @BeforeId))
ORDER BY [OccurredAt] DESC, [Id] DESC
OFFSET 0 ROWS
FETCH NEXT @Take ROWS ONLY

That seeks straight into IX_AccessAuditEvent_OrganizationId_OccurredAt_Id at the cursor, so every page costs the same, and appends can no longer shift the window.

This also affects the interface shape (IAccessAuditEventRepository.GetManyByOrganizationIdAsync(organizationId, since, skip, take)) and the EF .Skip(skip).Take(take), so it is cheaper to settle now than after the read endpoint lands in the follow-up PR. Note the read model Bit.Pam.Models.AccessAuditEvent would also need to expose Id for the caller to build the cursor.

@withinfocus

Copy link
Copy Markdown
Contributor

What's the plan here for non-relational storage of these events? We don't store events in the relational database ourselves and it's a self-host fallback. We cannot launch with the assumption that we can use this for our cloud-hosted deployments.

@patriksvensson
patriksvensson marked this pull request as draft August 24, 2026 15:58
@Hinton

Hinton commented Aug 24, 2026

Copy link
Copy Markdown
Member

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.

4 participants