Skip to content

[PAM-190] PAM access audit: API - #8507

Merged
patriksvensson merged 7 commits into
mainfrom
pam/access-audit-api
Oct 7, 2026
Merged

patriksvensson merged 7 commits into
mainfrom
pam/access-audit-api

Conversation

@Hinton

@Hinton Hinton commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

🎟️ Tracking

PAM-190 — bundle A6 of the pam/uat → main extraction, tracked under PAM-173. Builds on the access-audit store from #8230.

Originating tickets on pam/uat: PM-39047, PM-42480, PM-42614, PM-42814, PM-42816 and PM-43606.

📔 Objective

#8230 landed the append-only AccessAuditEvent store with nothing calling it. This PR writes to it and reads it back: PAM commands now record what they do, and organization admins can page through the trail.

Recording. AccessAuditEventEmitter is the write side. Each action emits an Attempt before its point of no return and an Outcome after it, sharing a correlation ID. An Attempt with no Outcome is how the trail shows an action that may not have landed. The emitter is wired into the nine commands on main:

Area Commands
Access rules create, update, delete
Access requests submit, decide, cancel, activate
Leases extend, revoke

An access-rule delete now receives the calling user, because the audit event is the only record of who deleted the rule.

Reading. A new /organizations/{orgId}/audit endpoint group, backed by ListAccessAuditTrailQuery and ListAccessAuditItemsQuery. Paging is keyset-based through AccessAuditTrailContinuationToken. The requested range is clamped to the 90-day history window by AccessHistoryWindow.ResolveRange. Kinds go over the wire as the string vocabulary in AccessAuditEventKindNames, which a test pins against the web client's copy.

Kill switch. PamDisableSqlAuditLogging stops the store writes and takes the read endpoint down with them, so the trail is never served as a complete record of a period it only partly covers. Off is both the default when the flag is absent and the only state self-host ever sees.

AccessAuditEventKind also gains the rotation and fleet kinds. Nothing on main emits them yet, but the wire vocabulary covers them, so they land with it.

Scope

Every file matches pam/uat exactly or differs only by omission. Deliberately left out, each returning with its own bundle:

  • The organization event log fan-out in the emitter (IEventService, the EventType mapping) and its tests go with B1, which needs Dirt review.
  • Push and mail notifiers in the request and lease commands, and their assertions, go with A7.
  • The rotation access-end hook in lease revoke goes with A12.
  • The pre-rename daemon* kind names that pam/uat still accepts in the trail filter are dropped. main never served this endpoint, so no client sends them.

This PR also takes the pieces A4 (#8494) deferred here: the acting-user argument to the access-rule delete, and AuditEndpointsHandler registered in the endpoint test hosts.

Bundle A6 of the pam/uat -> main extraction (PAM-173). Puts the
access-audit store from #8230 to use: commands record what they do,
and admins can read the trail back.

- AccessAuditEventEmitter writes an Attempt before each action's point
  of no return and an Outcome after it. Wired into the nine commands on
  main: access rule create/update/delete, request submit/decide/cancel/
  activate, and lease extend/revoke. The access-rule delete now takes
  the calling user so the event can name the actor.
- ListAccessAuditTrailQuery and ListAccessAuditItemsQuery behind a new
  /organizations/{orgId}/audit endpoint group, with keyset continuation
  tokens and a range clamped to the 90-day history window.
- PamDisableSqlAuditLogging kill switch: stops the writes and takes the
  read endpoint down with them.
- AccessAuditEventKind gains the rotation and fleet kinds so the wire
  vocabulary is pinned in full.

Content matches pam/uat except for omissions: the organization event
log fan-out (B1), the push and mail notifiers (A7), and the rotation
access-end hook in revoke (A12).
@Hinton Hinton added the t:feature Change Type - Feature Development label Oct 5, 2026
@codecov

codecov Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.63702% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.98%. Comparing base (3ff73a5) to head (1a8765b).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...m/Api/Models/Response/AccessAuditEventKindNames.cs 96.42% 1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (3ff73a5) and HEAD (1a8765b). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (3ff73a5) HEAD (1a8765b)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8507      +/-   ##
==========================================
- Coverage   71.35%   65.98%   -5.38%     
==========================================
  Files        2547     2575      +28     
  Lines      109310   110572    +1262     
  Branches     9940    10054     +114     
==========================================
- Hits        77999    72957    -5042     
- Misses      28816    35235    +6419     
+ Partials     2495     2380     -115     

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

@Hinton Hinton added the ai-review Request a Claude code review label Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed the full diff at 1a8765b62, covering the AccessAuditEventEmitter write path wired into the nine PAM commands, the /organizations/{orgId}/audit read group with its keyset paging and 90-day clamp, the PamDisableSqlAuditLogging kill switch, and the additive AccessAuditEventKind values. The two commits since the last pass hold up: [FromRoute] on orgId pins the binding the route pattern already implied, and AccessRuleEndpointsHandler resolving the editor from the ClaimsPrincipal matches the GetProperUserId pattern used across the codebase, with IDeleteAccessRuleCommand.DeleteAsync having exactly one call site so the signature change is contained.

Traced the paths the earlier threads opened and found them closed in code: the automatic approval records its own Attempt before CreateAutoApprovedAsync, the emitter swallows store failures for Outcome only with the interface doc matching, and SubmitAccessRequestCommandTests / RevokeAccessLeaseCommandTests assert the emissions they were missing. Also checked that TOP (@PageSize) sits above the Attempt/Outcome collapse in AccessAuditEvent_ReadPageByOrganizationId, so the events.Count >= PageSize token heuristic is sound; that Detail is NVARCHAR(MAX) and RuleName matches the rule's own 256 limit, so no emission can truncate; that AccessAuditTrailRequirement excludes providers by leaving isProviderUserForOrg uncalled, which both the unit and integration tests pin; and that PamValidationEndpointFilter reaches the [AsParameters] filter model, so an unknown kind or a forged continuation token is a 400 rather than a throw from ToQueryOptions. AccessHistoryWindow.ResolveRange rejecting a span wider than retention rather than clamping it is deliberate and pinned by GetTrailAsync_RangeWiderThanRetention_ThrowsBadRequest.

No findings.

Comment thread bitwarden_license/src/Services/Pam/Services/IAccessAuditEventEmitter.cs Outdated
@Hinton
Hinton marked this pull request as ready for review October 6, 2026 09:16
@Hinton
Hinton requested a review from a team as a code owner October 6, 2026 09:16

@patriksvensson patriksvensson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

Left a minor comment, but not 100% sure about it and if this is something we need to fix.

await _accessAuditEventEmitter.EmitAsync(
audit with
{
Kind = AccessAuditEventKind.RequestDenied,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should set the ActorId to null here, otherwise I think the audit log will look like something like (and I'm paraphrasing) "User X denied extension for user X". Not sure if it something we need to fix now though.

@patriksvensson
patriksvensson merged commit c1b36a4 into main Oct 7, 2026
48 of 49 checks passed
@patriksvensson
patriksvensson deleted the pam/access-audit-api branch October 7, 2026 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants