Add the PAM access-notification mailer - #8289
Draft
maxkpower wants to merge 40 commits into
Draft
Conversation
Stage 1 of two. Copies the final-state files from pam/poc-rebased to their pam/uat paths with no adaptation, so the next commit's diff shows every deviation from the POC. Does not compile on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… handlers
Stage 2 of two: adapts the POC blobs to pam/uat. Diffing this commit against its
parent shows every deviation from pam/poc-rebased.
The three handlers now orchestrate the ported queries and commands instead of
throwing NotImplementedException, so GET /leases/mine, /leases/active,
/access-requests/mine and /leases/ciphers/{id}/state return data rather than 500.
Adaptations:
- Restore the API enum shim as the wire contract. The domain's AccessRequestStatus
has no Activated member, so mapping it straight to the wire would decode a Denied
request as Activated. DomainEnumMapping derives Activated from ProducedLeaseId.
- Register a no-op IAccessAuditEventEmitter in the Pam service project. It cannot
live in Core, which has no Pam.Domain reference. Every command injects it, so the
registration is load-bearing even though it records nothing.
- Ship the two approver/requester notifiers as no-ops: the push types they would
send do not exist on this branch.
- Drop the approver email. Its ICollectionRepository.GetManagingUserIdsAsync sproc
and IMailService overload are both absent here.
- Widen AccessRequestResult with the automatic decision so the submit response
carries the decision log its published contract promises.
- Keep uat's response-model shape (parameterless ctor, mutable properties) and add
a domain-taking ctor alongside, preserving the generated OpenAPI schemas.
The only wire change is AccessRequestStatus gaining Activated and shifting the
values after Approved, which aligns the spec with the already-published
sdk-internal binding. All 18 PAM paths and the other 24 PAM schemas are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Write gating was deliberately out of scope for the UAT endpoint port, which left a leasing-gated cipher fully editable: a member holding no lease could edit, delete, restore, re-file, or attach to a credential whose secrets the read gate was already withholding from them. The read gate returns partial data for such a cipher, so a save would also have written the client's blanks over the fields the server suppressed. Add EnsureCanMutateAsync and EnsureCanMutateManyAsync to ICipherLeaseGate, no-op them in UnrestrictedCipherLeaseGate so OSS and flag-off behaviour is unchanged, and call them from the twelve CipherService mutation paths. Also gate CiphersController.PutPartial, which writes straight to the repository and so never reaches the service-level gate — leaving it open would let a caller re-file one gated cipher at a time while MoveManyAsync refused the batch. Brand-new ciphers, skipPermissionCheck, and org-admin paths stay ungated, for the same reason the read gate mints an unrestricted witness for a context already authorized out-of-band. Refusal is NotFound so a write attempt cannot confirm that a credential the caller cannot reach exists. The bulk decision honours leases where the bulk read deliberately does not: a read copies secrets into every client's local store for as long as that store lives, whereas a write copies no secret anywhere, so refusing the holder's own edit would withhold nothing. It refuses the whole batch when any cipher is gated, because DeleteManyAsync has no per-item result channel and a partial success would silently diverge from what the client believes happened. The bulk path resolves the governing rule per cipher, so a flag-on bulk mutation costs a query per cipher. A structural pre-filter over the caller's collections would remove that but is not sound: UserCollectionDetails filters on Organization.Enabled and CollectionCipher_ReadByUserId does not, so a disabled organization would clear a cipher the resolver gates. Batching belongs behind IGoverningRuleResolver instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Wires the commercial PAM library to the audit store: the emitter every
state-changing command already calls now appends to it, and the trail is read
back through GET organizations/{orgId}/audit, authorized by AccessEventLogs so
whoever can read the organization's event logs sees the whole trail regardless
of collection management.
The read collapses each action's before/after pair, which share a correlation
id, into one row -- the Outcome when it landed, otherwise the lone Attempt,
which the response flags as in-doubt rather than dropping. Emission is not
transactional by design, so an Attempt with no Outcome marks an interrupted
action instead of a silently lost event.
NoopAccessAuditEventEmitter goes with this: the interface is commercial-only, so
the placeholder had no remaining caller once the real emitter was registered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three access-rule commands never emitted, so creating, editing, or deleting a rule left no audit event -- even though RuleCreated/RuleUpdated/RuleDeleted were defined and the write payload carried AccessRuleId and RuleName for exactly this purpose. Unlike the expiry and credential-access kinds, these three were never marked deferred; they were simply unwired. Each command now emits the Attempt/Outcome pair the rest of the module does. Create and update take the actor from the LastEditedBy the handler already stamps. Delete had no actor at all, so DeleteAsync takes the caller's id -- the delete is hard, which makes the audit event the only surviving record of who did it and what the rule was called. That is also why RuleName is captured from the row before the delete rather than joined at write time. The create's Attempt cannot name the rule: Repository.CreateAsync assigns the id, so before the write there is no rule to name. Both create and update hold their Outcome until the collection links are written too, so an Attempt with no Outcome flags a half-applied change rather than reading as a clean one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AccessRule.DefaultLeaseDurationSeconds and MaxLeaseDurationSeconds were write-only: both round-tripped through create/update and rendered in the admin console's rule form, but nothing at request time ever read them. The only cap applied was the hardcoded 24h global in SubmitAccessRequestCommand, so a rule configured for 15 minutes granted a 1 hour lease in full (PM-39858). LeaseDurationBounds now owns the arithmetic that folds a rule's optional bounds together with the global ceiling. It has two callers that have to agree exactly -- the pre-check publishes the bounds so a client can shape its duration picker, and submit enforces them -- because a client narrowing to a cap the server does not enforce is how the rule's maximum came to be ignored in the first place. GoverningRule (and the resolver that builds it) now carry both fields; it previously copied only the extension-related ones, leaving the two lease-duration fields unreadable downstream. Submit applies the effective cap on both paths: the automatic duration, and the human-approval window, which is pinned at submit and so has no later gate of its own. AccessRuleWriteValidator additionally rejects non-positive durations and a default above the rule's own maximum. The edit form already couples its two pickers, but a write straight to the API bypassed that and could persist a rule whose every pre-filled request exceeded its own cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GoverningRuleResolver decided RequiresHumanApproval by evaluating the rule's whole condition list against the caller's current signals and asking whether the combined outcome was RequiresApproval. Combine gives deny precedence over requires-approval, so a single denying condition -- a source IP outside the rule's CIDR allowlist, a request outside its time windows -- short-circuits the fold to Deny and the flag comes back false. A rule carrying a HumanApprovalCondition was then handed downstream as though it had none (PM-42256). Nothing downstream recovers from that. SubmitAccessRequestCommand branches on the flag, takes the automatic path, re-evaluates the same conditions and rejects the request outright; AccessPreCheckQuery reports Automatic, so the client never offers the approval flow in the first place. The member is turned away at the door on precisely the rules whose purpose is that a human decides, and the approver never learns of it, because no request is created. The flag is now structural -- whether the rule's conditions contain a HumanApprovalCondition -- and no longer depends on signals. That matches how the governing rule is already chosen (oldest wins, on structure alone, never on how the conditions evaluate for the caller in front of us), and it makes the answer stable for a rule whose callers differ in IP or clock. Resolution no longer evaluates anything, so the resolver drops its IAccessRuleEngine dependency; the engine keeps its one real caller on the automatic path. One consequence is deliberate but worth naming: the approval path pins a window and routes to an approver without evaluating the rule's other conditions, so on a human-gated rule the IP and time-window checks now inform the approver's decision rather than pre-empting it. Routing these requests to a human is what the ticket asks for; that it also widens what a human-gated rule admits is a consequence of it, not only a routing fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restores what PM-40526 (#8001) did on main and 24e0e41 undid on this branch: delete the four Api/Models enum copies plus DomainEnumMapping, and let the request/response models carry Bit.Pam.Enums straight to the wire. The wire copy of AccessRequestStatus carried an extra Activated = 2, shifting Denied/Canceled/Expired to 3/4/5 while the generated bindings — which track main, where these files do not exist — decode 2/3/4 as Denied/Cancelled/Expired. Every terminal status therefore arrived one position early: an activated grant rendered as "Denied", a denial as "Canceled", a cancellation as "Expired", and a lapsed approval as "Unknown". Nothing consumed the member. DomainEnumMapping was its only producer, and activation is not a status of its own: the response already ships ProducedLeaseId and ProducedLeaseStatus, which is what the web client reads to label an activated request (my-access-row.ts historyDisplayStatus). The SDK's own bitwarden-pam type has no Activated variant either. AccessLeaseStatus, DeciderKind and AccessDecisionVerdict matched the domain member for member, so those three were pure duplication.
ListMyAccessRequestsQuery and ListMyActiveAccessLeasesQuery each forwarded one repository call, so the endpoint handlers now take the repository directly. The lease handler takes TimeProvider to stamp the active-window "now". Their unit tests only asserted the forwarding; the handler tests already cover the mapping.
The XML summaries on the access request models duplicated what the validation attributes and the generated OpenAPI spec already declare -- required-ness, string lengths, and enum wire values -- and prose copies of those only drift. The docs now cover meaning and behaviour alone, keeping genuinely semantic notes like the empty-Conditions contract and the rule-bound extension duration, and matching the style of the rotation slice's request models.
Access that had run out of time was still being reported as active, and never showed up in the history of ended access either, because nothing ever recorded the moment it lapsed. The server now works that out each time it reads, so access reads as expired everywhere once its time is up, appears in history where it used to vanish, and can no longer be revoked after it has already ended.
An extension is an AccessRequest row carrying ExtensionOfLeaseId. It applies in place when created -- AccessRequest_CreateApprovedExtension pushes the parent lease's NotAfter out -- and never mints a lease. The row survives only to carry the justification, anchor the automatic decision, and cap the parent at one extension. It is written Approved and stays Approved, and ActivateAsync never read ExtensionOfLeaseId, so every other check passed and activating an extension minted a second, independent lease over the extension window. Revoking the parent lease is what clears the single-active-lease guard, so a revoked requester could re-grant themselves the rest of the window -- a revocation bypass. API-only: the read path already excluded extensions, so no client offered the action. Guard ActivateAsync and DecideAsync, and add the exclusion to both mint paths (MSSQL sproc + EF) so the invariant holds of the data, not just the caller. DecideAsync is a backstop today -- extensions are born Approved -- but the spec models human-approved extensions, and this fails loudly rather than letting that work silently reopen the hole. BitAutoData fills ExtensionOfLeaseId, so both command test fixtures were silently extensions; pinned null in the setup helpers. PM-42530
…ation-wide The base change added an administrative stance to `ICipherLeaseGate`; this implements it in the commercial gate, which is where the fail-open it exists to close actually bites. Both member paths start from the caller. `GetGatedCipherIds` reads the collections they are assigned to, and `GoverningRuleResolver.ResolveAsync` starts from `GetManyByUserIdCipherIdAsync` — so for an administrator assigned to none of them it resolves no governing rule and reports the cipher ungated. Reusing either for the "/admin" endpoints would have released exactly the secrets leasing is meant to withhold. `AuthorizeAdminReadAsync` and `AuthorizeAdminReadManyAsync` therefore resolve which collections gate from the organization's own rules and collections. A gated cipher still yields to a valid active lease on the single read and never on the bulk one, matching the member asymmetry. Enabled-ness is derived by loading the organization's rules and filtering on `Enabled` rather than reading `CollectionDetails.HasEnabledAccessRule`, because the organization-scoped collection read returns `Collection`, which carries the association but not that computed projection — reading it there would have silently defaulted to false and gated nothing. `Unrestricted()` becomes `UnrestrictedForWholeVaultExport()` here and in the registration-test stub.
An organization cipher in no collection has no leasing-enabled collection to be reached through, so nothing gates it. The "/admin" endpoints reach these — Admin and Owner pass CanAccessUnassignedCiphersAsync — which makes it a case the administrative decision has to answer rather than one it never sees.
Adds a switch that stops PAM from recording its audit trail to the database. It is off by default, so nothing changes until someone turns it on, and while it is on the audit view is withheld rather than shown with gaps in it.
… a lease Gated-ness is the whole test for a write-return, so this stops where the single read goes on to look for a lease — making it the cheaper of the two as well as the stricter. Both stances are covered: the member decision resolves the governing rule, the administrative one resolves leasing status from the organization's collections, as their read counterparts do. Also corrects the remark that justified letting a lease widen the bulk mutation decision on the grounds that a write copies no secret anywhere. That is true of the request and not of the response, which is the gap this pair of commits closes. Carries the registration-test stub for the two new interface members: that test covers the DI swap for the commercial gate and so lives only here, above the extracted vault slice.
A member could see their own past access requests forever, while the approvers who decided those same requests lost sight of them after ninety days. Both views now reach back equally far, and anything a member can still act on stays visible however old it is.
Extending a lease that had already ended failed the call with a 409 and wrote nothing, so a requester whose lease ran out while the Extend dialog sat open got a generic error and no trace of what they had asked for. The extension now resolves the way the spec has always described it — a denied request carrying the window that was asked for, and an automatic verdict naming why the lease could not be extended. AccessRequest_CreateApprovedExtension becomes the single authority on whether there is anything left to extend. Its not-active branch records the denial and commits rather than rolling back; the outcome code it returns is unchanged, only its footprint. The command drops its own StatusAsOf pre-check, which was the second place that same verdict was computed and the reason a lost race behaved differently from the common case. The audit pair is closed out instead of left in doubt: the Outcome carries RequestDenied and the reason, mirroring how a refused activation reports LeaseActivationRejected against a LeaseActivated attempt. Only the requester is notified, since no collection-wide lease state changed. The denied row carries ExtensionOfLeaseId and so counts toward the one-extension cap. That is inert: a lease reaches this branch only once it is permanently un-extendable, so there is no later extension for it to consume. @DenialComment is optional, so a rolling deployment stays safe — an older server that predates the parameter records the denial without a comment rather than failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The endpoint filter attached exceptionMessage, exceptionStackTrace and innerExceptionMessage to every response it shaped whenever the host ran in Development, the 400/401/402/404/409 branches included. A modelled rejection is an answer the caller asked for rather than a defect, so its throw site diagnoses nothing: it only put the server's call stack and absolute source paths on the wire, where the SDK keeps the whole body as ApiError::Response and the PAM client re-logs it verbatim to the browser console. Those details now ride on the unhandled 500 branch alone, the one case where the throw site is the answer. That covers every PAM route, since the group runs under this filter. The MVC filters keep the old behaviour, so the CipherLeaseGate 404 raised from the ciphers controller still carries a stack trace. The divergence is deliberate and recorded in WithBasicExceptionHandling's remarks so it does not read as drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ad time The database stores facts -- what a party did to a record, and when; what a record means right now (its status) is computed against the clock at read time, so nothing stored can go stale. Finishes what PM-42355 started on the lease side and removes the stored claim it left behind. - AccessRequest.Status/ResolvedDate -> Action (None/Approved/Denied/ Cancelled) + ActionDate; AccessLease.Status/StatusAsOf -> Action (None/Revoked/Cancelled). Pending, Expired, and Active are now unrepresentable in storage; AccessStatusDerivation.ComputeStatus / ComputeLeaseStatus are the only producers of the derived enums, applied at the repository boundary (both ORMs) and in the response models. - Schema: metadata-only column/index renames (sp_rename + EF rename migrations per provider); no data migration. Every proc touching the columns is re-issued: reads return stored facts only (the ProducedLeaseStatus CASE projections come out), WHERE clauses carry plain clock comparisons (the pending inbox and active-pending reads gain @now, so lapsed rows leave the actionable set and stop blocking resubmission), and the cancel procs refuse a lapsed window so a row users saw as Expired can never restamp. ReadDetailsById keeps an accepted-and-unused @now for rolling-deploy compatibility. - DecideAccessRequestCommand now rejects both verdicts once the window has lapsed (409, like already-resolved), retiring the documented denial-closes-the-audit-trail behavior. - PM-42632 (late extension recorded as a denied request) and PM-42614 (the requester history retention window) are carried over onto the Action vocabulary. One consequence the PM-42614 exemption comment anticipated differently: a lapsed unanswered request is derived Expired -- history, not a live Pending row -- so it now ages out of the requester's own list with the rest of history instead of surviving forever. - Wire unchanged: status keeps its enum and numbers and always carries the derived value; resolvedAt maps from ActionDate. The always-null ExpiredAt response field is deleted. Verified: Pam.Test 473/473; Pam repository integration tests 63/63 against a freshly-migrated MSSQL database (full migration chain, including the two later-dated upstream scripts re-issuing procs this change re-renames) and 63/63 against SQLite via the EF rename migration.
* [PM-39040] Add PAM rotation domain layer
Entities, enums, models, and repository interfaces for credential
rotation: target systems, daemon registrations and assignments,
rotation configs, jobs (claim-lease semantics), and server-owned
attempts, per rotation-server.allium.
* [PM-39040] Extend PAM audit event store with rotation kinds
26 new AccessAuditEventKind values (rotation lifecycle 50-66, fleet and
target administration 70-78) plus nullable rotation subject columns
(target system, daemon, config, job, source, sync state) on the
append-only event store; names snapshotted at write like RuleName.
* [PM-39040] Add rotation SQL schema and stored procedures
Six tables and 38 stored procedures, including the concurrency-critical
paths: guarded job creation (one active job per config), atomic
first-claim-wins with an org/assignment eligibility join, the atomic
cipher-write capability check serialized against the sweeps via a job
row lock, retry-budget errored-attempt math, success-wins timeout and
lease-respecting release sweeps, and the lease natural-expiry sweep.
* [PM-39040] Add rotation Dapper repositories
MSSQL-only implementations of the four rotation repository interfaces
(matching the PAM POC's Dapper-only scope), plus
IAccessLeaseRepository.ExpireDueAsync backing the lease natural-expiry
sweep.
* [PM-39040] Wire rotation daemon identity on the generic ApiKey credential
Daemons authenticate via OAuth client-credentials exactly like Secrets
Manager machine accounts, reusing dbo.ApiKey as the credential store
(ServiceAccountId null; PamDaemon.ApiKeyId owns the link). The daemon.
client-id prefix resolves a new client provider that gates on daemon
enrollment and org Enabled/UsePam; every token response returns the
wrapped org key (encrypted_payload, zero-knowledge). Adds the
api.pam.rotation scope, RotationDaemon client type and claims parsing,
an exact-claims authorization policy, a LaunchDarkly context for daemon
principals, and the previously missing ApiKey_ReadById procedure.
* [PM-39040] Add rotation application layer
Commands and queries implementing rotation-server.allium's rules:
target/daemon/config administration with per-row org checks and
audit emission, OfferRotation as the single job-creation point, the
claim/report/cipher-write daemon paths with exact-complement rejection
auditing, the access-end trigger handler, cron scheduling via Quartz
(UTC, interval floor), and PamRotationOptions with spec defaults.
Feature-flagged under pm-39040-pam-rotation.
* [PM-39040] Map rotation admin and daemon API endpoints
Admin groups for daemons/target-systems/configs and daemon-facing
poll/claim/cipher/report routes behind the PamRotationDaemon policy.
Every daemon request re-checks enrollment and org Enabled/UsePam and
doubles as a conditional heartbeat. Also fixes
PamValidationEndpointFilter's exact-namespace match, which silently
skipped DataAnnotations validation for models outside the original
Pam.Api.Models.Request namespace.
* [PM-39040] Add rotation sweep jobs and access-end trigger
Two commercial-gated Quartz minute sweeps: rotation (due offers,
success-wins timeouts, lease-respecting releases of stale daemons'
claims) and lease natural expiry (flips Active leases past NotAfter,
emits the long-deferred LeaseExpired audit kind, fires the rotation
access-end trigger). RevokeAccessLeaseCommand fires the same trigger on
explicit lease ends, isolated so it can never fail the revoke.
* [PM-39040] Add rotation unit and integration tests
171 unit tests (guard matrices with mandatory cross-org 404 cases,
retry-budget and success-wins semantics, exact-complement rejections,
UTC cron evaluation, sweep phases, daemon filter and client provider)
plus 43 database integration tests covering the concurrency paths:
concurrent double-claim single-winner, forged cross-org assignment
refusal, atomic cipher-write vs release interleaving, and lease-expiry
idempotence. Lockfiles pick up the Pam.Domain reference and internal
version bump.
* Pam dev scripts
* PAM rotation: replace daemon revoke with enable/disable + delete
* PAM rotation: nest the rotation route groups
Map a single parent group per rotation surface and hang the child groups
off it, so the shared prefix and the auth/flag chain are each declared
once instead of repeated on every group.
* Run dotnet format
Using-directive ordering across the PAM slice, plus the file-scoped
namespace and whitespace conventions on the AddAccessAuditEvent EF
migrations, which were left as generated.
* PAM rotation: move admin authorization into the middleware
The rotation admin handlers each opened with an EnsureAdminAsync copied
from AccessRuleEndpointsHandler, so the org role check was restated
nineteen times and a new handler method was one forgotten line away from
being unauthenticated.
Replace it with ManageRotationRequirement, attached once to the parent
rotation admin group. It implements IOrganizationRequirement directly
rather than deriving from BasePermissionRequirement: that base's final
arm authorizes any provider managing the organization, and registering a
daemon hands it the organization key while rotation rewrites the
credentials inside the vault -- neither is a provider's to hold or
change. There is no custom-permission arm either; ManageAccessRules is
authority over who may lease a credential, not over the daemons that
rotate it.
Handlers keep resource scoping only -- the commands underneath still
re-verify every id argument belongs to the route organization, and still
404 rather than 403 so they are not an existence oracle.
Tests cover the requirement itself, the endpoint metadata (every
rotation admin route carries it; the daemon surface carries no
IOrganizationRequirement, since the daemon routes have no {orgId} for
the handler to read), and the denials over the real pipeline.
* PAM rotation: generate IDs with CombGuid.Generate()
* PAM rotation: address data-layer review findings
Bump the account revision date when a rotation writes a cipher.
PamRotationAttempt_AcceptCipherWrite updated Cipher.Data and RevisionDate
without the User_BumpAccountRevisionDateByCipherId call every other cipher
writer ends with, so a client that missed the (flag-gated, non-mobile) push
saw an unchanged AccountRevisionDate, skipped the sync, and kept serving the
pre-rotation password indefinitely.
Release a daemon's claimed jobs when it is deleted. ClaimedByDaemonId has no
FK to PamDaemon and PamRotationJob_ReleaseExpiredLeases inner joins that
table, so a job claimed at deletion became invisible to the release sweep and
blocked any replacement job for its config until the much later TimeoutDue.
Re-check the active-job guard inside PamRotationConfig_DeleteWithJobs, under
the same range lock PamRotationJob_Create takes. The caller's HasActiveJob
read happens in a separate transaction; a job claimed in that window was
hard-deleted mid-rotation, leaving the target rotated and the vault holding
the old secret with no attempt row to record the drift. The procedure now
reports whether it deleted, and the command surfaces a 400 when it did not.
Check the daemon is enabled in PamRotationJob_ReadManyClaimableByDaemonId,
which its own comment claimed to re-derive from PamRotationJob_Claim, and
correct the stale "Enrolled" comments left over from the enable/disable
rework.
Index OrganizationId on PamTargetSystem, PamDaemonTargetAssignment and
PamRotationConfig, plus TargetSystemId on PamRotationConfig. Each backs a
_ReadByOrganizationId procedure and an ON DELETE CASCADE FK; TargetSystemId
also carries the daemon poll's join.
Use CREATE OR ALTER for ApiKey_ReadById, matching the other scripts, instead
of a guarded EXECUTE wrapper that leaves a divergent definition in place.
* PAM rotation: address application-layer review findings
Scope the daemon report paths to the caller's own organization. The attempt
id is a bare route value and PamRotationAttempt_ReadById is unscoped, so
report-succeeded, report-failed and submit-cipher-update all loaded a foreign
attempt, let the guarded repository call reject it, and then emitted a
report-rejected event into the *other* organization's audit trail carrying
this daemon's name. The 409-for-foreign versus 404-for-missing split also told
a caller which attempt ids exist elsewhere. All three now resolve the attempt,
job, config and daemon up front and 404 unless the config and daemon share an
organization, matching what ClaimRotationJobCommand and GetRotationCipherQuery
already do.
Stop scheduling configs that have no schedule. The sweep's timeout phase and
the retry-budget-exhausted branch both wrote NextRotationAt unconditionally,
but only the success path runs it back through the schedule calculator, which
returns null for a null cron. A timed-out access-end job therefore left a
concrete NextRotationAt on an on-demand-only config; ReadManyDue does not
filter on the cron, so the config became permanently due, offering an hourly
job tagged Scheduled and -- once a daemon was assigned to its target --
rotating a live credential nobody asked to rotate.
Give the lease-expiry sweep's audit emit its own try block. It shared one with
the access-end rotation trigger, and ExpireDueAsync commits the whole batch
before the loop, so a transient audit-store failure permanently dropped the
RotateOnAccessEnd trigger for that lease with only a log line to show for it.
* PAM rotation: address API-surface review findings
Validate nested request models. Validator.TryValidateObject does not recurse
into complex properties, and PamPasswordPolicyRequestModel is only ever
reached as one -- so its range constraints and its IValidatableObject rules
never ran, and neither command re-checks them. An admin could persist an
unsatisfiable or unbounded generation policy, which the daemon then uses to
build a rotated privileged credential. The filter now walks nested request
models (and request-model collections) itself, guarding against cycles.
Refuse non-service-account ApiKey rows in SecretsManagerApiKeyProvider. The
daemon credential is the first dbo.ApiKey row with a null ServiceAccountId,
ApiKeyRepository always materializes ServiceAccountApiKeyDetails, and
ApiKeyDetailsView left joins ServiceAccount -- so a daemon key matched the
service-account arm with a defaulted organization id, and the null
organization was dereferenced. Any caller could turn that into an unhandled
500 on /connect/token, before secret validation, just by using the apikey
client-id prefix. Matching on ServiceAccountId and defaulting to null also
closes the bare-guid fallback path.
Project the target system id into the daemon poll. The handler re-read each
job's config purely to recover TargetSystemId, on a request every enabled
daemon makes several times a minute against an unpaged result set. The poll
query already joins PamRotationConfig for its eligibility check, so the column
comes back with the row as PamClaimableJob, which also removes the
config-disappeared-mid-poll case the loop had to special-case.
Drop the HttpContext.Items daemon stash, which nothing but a test read.
* PAM rotation: address test-coverage review findings
Compare round-tripped timestamps with LaxDateTimeComparer. Dapper has no
global DateTime type map, so a plain DateTime parameter binds as datetime and
is truncated to ~3.33ms precision client-side before SQL Server sees it. Every
assertion comparing an in-memory instant against one that made the round trip
was therefore unsatisfiable; the sibling AccessRequest tests already use the
comparer for exactly this.
Drive the daemon routes through their built RequestDelegate to prove
DaemonRequestEndpointFilter is attached. The filter is what re-checks that the
daemon is still enabled and its organization still licensed on every request,
and AddEndpointFilter<T>() leaves no metadata, so detaching it left the whole
suite green while a deleted or disabled daemon kept working for the remaining
lifetime of its token. Verified the test fails when the filter is removed.
Discover endpoint handlers by reflection instead of listing them by hand. The
[InlineData] list had drifted to four of the eleven handlers AddPamServices
registers, and the sibling every-dependency test cannot catch a dropped
registration because it only walks registrations that already exist.
Cover GetRotationConfigDetailsQuery's organization check, the one rotation
resource-scoping check with no test. ManageRotationRequirement only proves the
caller administers the organization named in the route, so this check is all
that stops an owner of one organization reading another's config and its full
job and attempt history.
* PAM rotation: add EF Core parity for the rotation repositories
The four rotation repositories were registered only on the Dapper path, so
every EF-backed deployment failed to resolve them: each rotation endpoint 500d,
daemon token issuance 500d, and Quartz threw once a minute constructing
PamRotationSweepJob regardless of the feature flag. It also failed 40
integration tests outright with "No service for type has been registered".
Adds the six persistence models, their DbContext mapping (keys, indexes and
delete behaviour mirroring src/Sql/dbo/Pam/Tables), the four repositories, and
the generated migrations for Postgres, MySQL and SQLite.
The MSSQL procedures lean on UPDLOCK/HOLDLOCK range locks and OUTPUT, none of
which is portable, so the guarded transitions are rebuilt from two primitives:
a serializable transaction wherever the procedure takes a range lock, and a
single ExecuteUpdate carrying the guard in its WHERE wherever the procedure
relies on a row lock. First-claim-wins, AtMostOneActiveJobPerConfig,
AtMostOneInFlightAttemptPerJob and VerifiedBeforeSuccess all hold the same way.
The release sweep computes its next-claimable time in memory rather than in the
UPDATE, since MySQL assigns left to right and would read the nulled ClaimedAt.
ReplaceAsync on the daemon repository is narrowed to the three columns
PamDaemon_Update writes. The generic whole-entity replace would persist a
caller-mutated OrganizationId or ApiKeyId, which would let an ordinary admin
edit move a daemon between organizations.
Also extends the EF AccessAuditEvent model and repository with the eight
rotation columns added to the table. The domain model and the Dapper repository
carried all eight; EF carried none, so on EF providers every rotation audit
event silently wrote and read back null target, daemon, config, job, source and
sync-state context -- latent only because rotation could not run there at all.
Three integration tests encoded behaviour that had never executed, since the
missing registrations failed them before they reached their assertions: the
delete-cascade test deleted a config out from under a live claim, the
disabled-target claim test expected NotEligible where the procedure documents
and returns NotClaimable, and the due-configs test asserted over the whole
result of a deliberately global sweep. All three now assert what the code means.
Verified against both ORMs: 100/100 PAM repository tests pass on SqlServer and
on SQLite, including the concurrent double-claim, release and timeout tests.
* PAM rotation: add the daemon detail endpoint (PM-42436)
The daemons tab links each daemon to a detail page, and that page could not be
opened. The client fetched GET rotation/daemons/{id}, which was never mapped --
the path existed for DELETE only, so the request came back 405, the component
caught it and navigated back to the list. Nothing surfaced the failure: the
client's own specs stub the API service, and the route carried no server test.
Adds the GET, returning the list shape flattened onto one object plus the
daemon's recent activity, which is what the surface already parses (it extends
its list-item response class and reads Jobs off the same object). Sourcing the
daemon from the already-loaded list was the alternative, but the list response
carries no activity and the detail page's history section needs it.
Membership in that activity is decided by PamRotationAttempt.ClaimedByDaemonId
rather than PamRotationJob.ClaimedByDaemonId: the job's claim fields are cleared
when it resolves, releases or times out, so the attempt is the only durable
record of which daemon worked it. Attempts come back narrowed to the daemon
being viewed as well -- a job released by one daemon and retaken by another
carries both daemons' attempts, and the history table renders no per-row
attribution, so the page would otherwise credit another daemon's work.
The read is capped at 50 jobs. A daemon accrues a job per rotation it executes
for the lifetime of the fleet, unlike a config's history, which is bounded by
that one config's rotations. IX_PamRotationAttempt_ClaimedByDaemonId_JobId
backs both result sets; without it every page view scans every attempt ever
recorded, across all organizations.
* PAM rotation: flatten the config detail response
The managed-credential detail page renders its header from blanks. The config
detail response nested the config under a Config property, while the surface
parses it the way it parses every other detail read -- by extending its
list-item response class, which reads Id, TargetSystemName, AccountIdentity and
the rest off the top level. Every one of them came back undefined, on the GET
and on the create, settings-update and account-update responses alike, since all
four return this model. No error anywhere; the page just renders empty fields.
Flattens it onto the list shape with Jobs alongside, matching what the surface
reads and the daemon detail response added alongside it. Nothing consumed the
nested Config.
Extends the detail wire-shape test to cover it. The client reading a property
name the server does not emit is the failure mode behind both of these defects,
and serializing the model is the only place it shows up on this side.
* PAM rotation: delete a daemon and its credential atomically (PM-42437)
Every daemon deletion returned a 500. DeleteDaemonCommand deleted the daemon's
dbo.ApiKey row through the generic repository, whose convention resolves to
dbo.ApiKey_DeleteById -- a procedure that has never existed; the SM table only
has the plural ApiKey_DeleteByIds. The unit tests mock IApiKeyRepository, so
nothing on this side ran the real path.
The partial delete is the worse half. The failure landed after the daemon row
was already gone, so each attempt left behind the ApiKey that authenticated it:
a working machine credential with no daemon governing it, and nothing left
pointing at it to find it by.
Folds the credential delete into PamDaemon_DeleteById and the EF repository's
existing delete transaction, next to the assignment clear and the job release
already there, and drops it from the command. The pair is now atomic in both
providers -- the daemon cannot go without its credential. Both read ApiKeyId
from the stored row rather than the passed entity, so a stale or forged value
cannot redirect the delete at another daemon's credential.
Covers it in PamDaemonRepositoryTests: the daemon, its credential and its
assignments all go, another daemon's credential in the same organization
survives, and the passed entity carries a foreign ApiKeyId to prove the delete
ignores it. The command's mocked tests could not see any of this, which is what
let the missing procedure through.
* PAM rotation: fix the flaky claim ExecuteBy assertion
ClaimAsync_ConcurrentDoubleClaim_ExactlyOneWinner compared a
DB-round-tripped ClaimedAt against the in-memory ExecuteBy the EF claim
computes as now + releaseDelay, so the two sides disagreed below the
microsecond. MySQL datetime(6) and Postgres timestamp truncate to
microseconds while DateTime carries 100ns ticks, so the assertion failed
on those providers under CI's Linux clock. MSSQL derives ExecuteBy in
T-SQL from the same @now param and Sqlite keeps the full text precision,
which is why both stayed green -- as did every local macOS run, where
DateTime.UtcNow is already microsecond-aligned.
Compare with LaxDateTimeComparer.Default, like the neighbouring timestamp
assertions in the same test.
* PAM rotation: record the daemon scope and claim in the discovery snapshot
WellKnownEndpoint_Success compares /.well-known/openid-configuration against
a checked-in snapshot, and wiring the daemon identity changed two lists the
document derives from. scopes_supported enumerates the registered ApiScopes,
so api.pam.rotation landed between api.secrets and api.send.access and shifted
everything after it -- the position-4 string mismatch CI reported.
claims_supported is the union of resource user claims, so declaring
Claims.Type on the api.pam.rotation resource appended type to it. That second
divergence was latent: the assertion stops at the first mismatch, so it only
surfaced once the scope was fixed.
Updates the snapshot to match on both counts. Every prior type claim is a
client claim minted per client, which never reaches discovery metadata, which
is why the document has not carried it before.
* PAM rotation: read the pass-through lists straight from the repository
ListTargetSystemsQuery, ListRotationConfigsQuery and ListClaimableJobsQuery
each forwarded one repository call, so the endpoint handlers now take the
repository directly. The claimable-jobs handler takes TimeProvider to stamp
the poll's "now", matching the other rotation handlers.
* PAM rotation: bound the daemon token's lifetime instead of re-checking per request
The daemon-facing filter re-read the daemon and its organization's PAM
licensing on every request, so a disable, a delete, or a license lapse
could not outlive an already-issued token. JWT lifetime is a product-wide
concern, and rotation was the only surface answering it with a bespoke
per-request check -- one that largely duplicated guards the work queries
already carry: the poll and claim queries join PamDaemon on Enabled and
on the config's organization, and deleting a daemon abandons its
in-flight attempts and re-pends its jobs.
Cut the daemon's access token lifetime to fifteen minutes instead, which
bounds the window at the token layer, and leave the filter with the one
job every daemon route needs: recording the heartbeat. Renamed to
DaemonHeartbeatEndpointFilter, since a name promising end-to-end
verification invites the next query to skip its own Enabled join.
A lapsed PAM license or a suspended organization is the one case no query
re-checks. That window is now the token lifetime rather than zero.
* PAM rotation: document the daemon's architecture and behaviour
The rotation path spans the domain entities, both data layers, the daemon's
OAuth client in Identity, the endpoint groups in this library and the Quartz
sweeps. The reasoning tying those together lived only in XML docs and stored
procedure comments, so answering "what does a daemon do, in what order, and
what happens when it stops" meant reading all of it in the right order.
Adds a README at the Pam service root as the component entry point, a README
under Rotation covering the actors, the four objects, the trigger sources and
the timing options, and three focused documents beneath it:
- daemon-protocol.md: the request-by-request contract, from registration
through claiming to reporting an outcome, and what each 404 and 409 means.
- job-lifecycle.md: the job and attempt state machines, the retry budget, the
three sweep phases, and what administrative changes do to work in flight.
- security-model.md: the trust boundary, the four places a daemon's
eligibility is established, why the responses avoid an existence oracle,
and the failure-reason contract.
Placement follows the documentation standard's lowest common ancestor rule --
an entry point per component, expanding into docs/ where one file would
sprawl -- and each document states the audience it is written for.
* PAM rotation: squash the daemon migrations
Six MSSQL scripts and two EF migrations per provider accumulated while
the daemon surface took shape, several of them revising what an earlier
one in the same unmerged branch had just created. Collapse each side into
one migration: 2026-08-25_01_PamRotationDaemon.sql, and a single
PamRotationDaemon migration per EF provider.
Only the final shape survives -- PamDaemon_DeleteById is declared once,
with the credential delete folded in, rather than created and then
replaced. Every procedure body in the squashed script is byte-identical
to its src/Sql definition, and the three EF model snapshots are unchanged,
so the model the migration lands on is the one it landed on before.
Verified by running all 527 scripts into an empty database: the squashed
script applies last, and the resulting PAM objects match a database built
from the six originals.
* PAM rotation: document the rotation request models
Give every rotation request model the property-level XML docs the
admin-side PAM models already carry, covering what each field means and
what it does downstream: what an assignment gates, how a null cron is
interpreted, which half of the daemon credential wraps which, and why
the failure report is bounded to daemon-defined tokens.
The docs deliberately do not restate [Required], lengths, or ranges --
the validation attributes and the generated OpenAPI spec are the source
of truth for those, and prose copies only drift.
* PAM rotation: raise the heartbeat write floor to a minute
Review feedback on the daemon docs: HeartbeatMinInterval only throttles
the server-side heartbeat write, and 15 seconds was tighter than anyone
needs -- raise it to a minute. The interval doubles as the poll floor
and a floor-polling daemon only records a heartbeat every two
intervals, so DaemonOfflineAfter moves to five minutes to keep the
margin against the offline threshold.
Also reword the lines the review flagged: the ambiguous "this library"
in the PAM invariant, and "connector kind" in the target system's
description.
* PAM rotation: port the expiry sweep to derived lease status
pam/uat retired stored lease status (PM-42355/PM-42653): a lease whose
window closes on its own keeps Action = None forever and Expired is the
read model's call, so the sweep can no longer flip Active -> Expired to
mark a lease as processed. Journal each swept lease in the new
PamLeaseExpirySweep table instead -- the insert is the once-only
arbiter, preserving the at-most-once guarantee the flip provided for
the LeaseExpired audit emission and the access-end rotation trigger.
The EF implementation drops its Serializable transaction along the way:
the journal's primary key already fails a racing sweep before it can
return anything, without range-locking the whole lease table against
concurrent mints.
Also re-dates the daemon migration to 2026-08-28_00: the rewritten
AccessLease_ExpireDue references [Action], which only exists once
2026-08-27_02_DerivePamStatusFromAction has run, so the script must
sort after it for fresh databases.
* Add pam spec
* PAM rotation: classify wide revision drift as a mismatch, not an error
MSSQL's DATEDIFF(MILLISECOND, ...) returns INT, so AcceptCipherWrite's
revision guard raised an overflow error once the daemon's last-known
revision sat more than ~24.8 days behind the cipher's current one.
That is the guard's own primary scenario: rotated credentials are
rarely hand-edited, so a concurrent user edit during rotation compares
a fresh revision date against a months-old one. The unmodeled
SqlException surfaced as a 500, skipped the write_rejected audit
emission, and left the attempt executing until the timeout sweep --
where the EF providers returned the contractual 409. DATEDIFF_BIG
restores parity; the new integration test pins sixty days of drift.
LastKnownRevisionDate also becomes nullable: [Required] never fires on
a value type, so an omitted field bound to DateTime.MinValue and rode
the same path. As DateTime? it fails validation as a 400 on every
provider before reaching the write.
Addresses the automated review finding on PR #7926.
---------
Co-authored-by: Anders Åberg <github@andersaberg.com>
--- Rebase onto main (#8279) ---
#8279 scaffolded this same surface on main under a different name and
shape -- Rotation/ became AccessConnector/, "daemon" became "access
connector", the routes moved to organizations/{orgId}/access-connectors
and access-connectors/rotation/..., the report enums became nullable
with [EnumDataType], and the four split PUTs (target-system name/policy,
config settings/account) collapsed into one PUT each. main's scaffold is
the reviewed wire contract clients generate against, so it wins wholesale
and the implementation from this commit is re-attached behind it:
- main's Api/ and Rotation/Api/ trees are kept verbatim; this commit's
Rotation/Api/ is dropped. Handler shells get the bodies from here, and
the response models get their domain-object constructors back with
main's property set and [get; set;] accessors untouched, so the
generated schema is unchanged.
- Commands/, Queries/, Jobs/ and Models/ move flat under AccessConnector/.
Fleet-facing types are renamed (RegisterDaemonCommand ->
RegisterAccessConnectorCommand, and so on); DaemonHeartbeatEndpointFilter
becomes AccessConnectorHeartbeatEndpointFilter.
- The merged PUTs fan out to the two existing commands. The stricter one
runs first -- account before settings, policy before rename -- so a
rejected write leaves the other half untouched. The automatic/manual
shape rule moves to the target-system handler, which is the first place
that knows the stored method the body no longer carries.
- The connector-facing group keeps this branch's real machine-credential
policy and heartbeat filter rather than main's Policies.Application
placeholder, and the salvaged heartbeat test moves onto main's
AccessConnectorMachineEndpointsTests.
- The persistence layer keeps its Daemon naming: PamDaemon and
PamDaemonTargetAssignment, their stored procedures and migrations,
PamDaemonClientProvider, and PamRotationJob.ClaimedByDaemonId are all
unchanged. Only PamDaemonStatus is dropped, for main's identical
PamAccessConnectorStatus, which the wire contract already names.
PAM recorded its audit trail only in its own dbo.AccessAuditEvent store, so none of it reached dbo.Event -- the organization event log, the events export, or the webhook/HEC integrations that read from it. The fan-out was never built; AccessAuditEventEmitter noted it as deferred. AccessAuditEventEmitter now projects each event into the organization event log after writing the PAM store. All 71 emit call sites are unchanged, since the emitter was already the single seam every state-changing command goes through. Five kinds are mapped (2600-2604), covering the access-request and lease lifecycle. The rest -- rotation and daemon lifecycle, rule administration -- stay PAM-only until each earns a place organization-wide. Event gains AccessRequestId and AccessLeaseId across both ORM tracks. The item and its gated collection ride the existing CipherId/CollectionId columns, which also files each PAM event under the item's own event history. Four decisions worth recording: - Only the Outcome half of an action crosses over. dbo.Event has no phase or correlation column, so writing the Attempt as well would double every action; the in-doubt attempt an interrupted action leaves behind stays visible in the PAM trail, which remains the system of record. - PamDisableSqlAuditLogging now guards the PAM store write alone. Its purpose is shedding audit-store inserts when that store is under pressure, and the organization event log is a separate sink with its own capacity, so the fan-out sits outside the guard. - EventSystemUser.Pam names PAM as the actor when PAM acts on its own. This is live rather than hypothetical: an auto-approved submit emits RequestApproved with a null ActorId, which would otherwise render with a blank member column. - The fan-out is best-effort and logged. The PAM store is already written by that point, so letting a failure in the event pipeline throw would undo nothing and would fail an access decision that actually succeeded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review DetailsNo findings. |
maxkpower
force-pushed
the
pam/PM-42817/mail-plumbing
branch
from
August 31, 2026 16:32
8a24f79 to
cc12bac
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## pam/uat #8289 +/- ##
===========================================
+ Coverage 64.32% 64.33% +0.01%
===========================================
Files 2582 2583 +1
Lines 111110 111170 +60
Branches 9925 9931 +6
===========================================
+ Hits 71468 71526 +58
- Misses 37272 37273 +1
- Partials 2370 2371 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
maxkpower
force-pushed
the
pam/PM-42817/mail-plumbing
branch
from
August 31, 2026 20:25
cc12bac to
23d0104
Compare
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
4 participants
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.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-42817
📔 Objective
Adds the delivery seam PAM's email notifications send through. No email is sent by this PR: it is plumbing the three notification PRs above it consume.
IAccessMailNotifierresolves recipient addresses and sends viaIMailer, gated onFeatureFlagKeys.Pam.Mailer.SendEmailreachesIMailDeliveryServicewith no queue in between, so a send that propagated would fail the enclosing access-request command: a mail outage would stop people getting access to their own vault items. Every send and every user read is caught and logged.ToEmails, so an organization's approvers are not disclosed to each other.IAccessMailNotifierhas no caller until the PR above this one. Registered, tested, and deliberately dormant.Bottom of a four PR stack on
pam/uat. Nothing sits below it.📸 Screenshots