[PM-40526] Access Leasing: persistence & schema by Hinton · Pull Request #8002 · bitwarden/server

Hinton · 2026-07-17T10:32:31Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40526

📔 Objective

Persistence & schema for the PAM access-request/lease domain (introduced in #8001), across both ORMs:

MSSQL / Dapper — the AccessRequest, AccessDecision, and AccessLease SSDT tables; the 22 AccessRequest_* / AccessLease_* stored procedures; the Dapper AccessRequestRepository and AccessLeaseRepository; repository integration tests; and one consolidated migration (2026-07-16_00_AddAccessRequestAndLease.sql) that folds the POC's incremental scripts into a single create matching the final SSDT shape verbatim.
EF Core (PostgreSQL / MySQL / SQLite) — EF entity models + AutoMapper profiles, inline DatabaseContext configuration and DbSets, the EF repositories implementing the domain interfaces with LINQ matching the stored-procedure behaviour, DI registrations, and generated migrations for all three providers.
Relocates Collection_SetAccessRuleAssociations to the AdminConsole SSDT folder so collection sprocs stay under the table's owning team (per PR review).

The two race-guarded writes (lease mint, approved extension) use a Serializable transaction as the cross-provider stand-in for the MSSQL procs' UPDLOCK/HOLDLOCK; a losing concurrent writer may surface a provider serialization error rather than a clean outcome enum. Cross-provider parity is covered by the [DatabaseData] integration tests.

Stacked PR — targets pam/access-leasing-domain (#8001). Review/merge that first.

github-actions · 2026-07-17T10:33:20Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed the delta since the last pass (4bbcbdb0 compile fix, 635bf0e1 and its revert f8f6ba7e) and re-verified the previously addressed findings still hold at head. The AccessRequestDecision.Id → ApproverId rename is applied consistently across the Dapper DecisionRow.ToDecision() projection, the EF decision projection, and all integration test assertions; no stale references remain. The revert restored full EF migration bodies while keeping the AccessRequest.RuleId property reorder, and the two tracks are consistent — RuleId is the last column in the SSDT table, the MSSQL migration, and all three regenerated EF CreateTable calls. No new findings.

Code Review Details

Verified at head (no action needed)

The previously flagged empty Up/Down EF migrations are fixed. All three providers now carry full CreateTable / CreateIndex / AddForeignKey bodies (20260812123326 MySQL, 20260812123330 SQLite, 20260812123335 Postgres), each .Designer.cs model matches its provider's DatabaseContextModelSnapshot.cs exactly, and all three sort after AddAccessRule.
Decision-on-no-op gating (@@ROWCOUNT / rowsAffected), the authoritative CipherId lookup in the EF mint guard, the narrowed IsDuplicateKeyException, the org-delete FK ordering, the AccessRule detach, and the four new indexes are all still present and unchanged by the revert.

Existing open threads still valid at head (not re-posted inline)

🎨 : Mint precondition re-check does not exclude extension requests (ExtensionOfLeaseId IS NULL); the EF predicate matches, so both providers behave the same way.
- src/Sql/dbo/Pam/Stored Procedures/AccessLease_CreateFromApprovedRequest.sql:57
♻️ : No MSSQL OrganizationId-leading index behind the ON DELETE CASCADE FK_AccessLease_Organization; the EF providers get one by convention (IX_AccessLease_OrganizationId).
- src/Sql/dbo/Pam/Tables/AccessLease.sql:16

codecov · 2026-07-17T10:59:24Z

Codecov Report

❌ Patch coverage is 95.08393% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.41%. Comparing base (fc4c606) to head (f8f6ba7).

Files with missing lines	Patch %	Lines
...mework/Pam/Repositories/AccessRequestRepository.cs	93.23%	16 Missing and 6 partials ⚠️
...ramework/Pam/Repositories/AccessLeaseRepository.cs	91.17%	15 Missing ⚠️
...e.Dapper/Pam/Repositories/AccessLeaseRepository.cs	97.67%	2 Missing ⚠️
...tructure.EntityFramework/Pam/Models/AccessLease.cs	80.00%	1 Missing ⚠️
...ucture.EntityFramework/Pam/Models/AccessRequest.cs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8002      +/-   ##
==========================================
+ Coverage   68.12%   68.41%   +0.28%     
==========================================
  Files        2373     2380       +7     
  Lines      102846   103679     +833     
  Branches     9328     9386      +58     
==========================================
+ Hits        70068    70930     +862     
+ Misses      30454    30421      -33     
- Partials     2324     2328       +4

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

kspearrin · 2026-07-22T14:31:56Z

This is marked as a draft. Do you still need a review at this stage?

claude · 2026-08-11T19:06:13Z

+    WHERE
+        AR.[Id] = @AccessRequestId
+        AND AR.[RequesterId] = @RequesterId
+        AND AR.[Status] = 1 -- Approved
+        AND AR.[NotBefore] <= @Now
+        AND AR.[NotAfter] > @Now
+        AND NOT EXISTS (SELECT 1 FROM [dbo].[AccessLease] AL WHERE AL.[AccessRequestId] = AR.[Id])


🎨 SUGGESTED: The mint guard does not exclude extension requests, so activating one would mint a second lease alongside the extended parent.

Details and fix

AccessRequest_ReadActiveApprovedByRequesterIdCipherId deliberately excludes extension requests (AR.[ExtensionOfLeaseId] IS NULL) because "an approved extension pushes its parent lease's end out in place and never produces a lease of its own". This re-check block — which the comment above describes as re-checking every application-level precondition — omits that one, as does the EF CreateFromApprovedRequestAsync predicate.

An extension request created by AccessRequest_CreateApprovedExtension is Status = 1 (Approved), NotBefore = [old lease end], NotAfter = [new lease end], and has produced no lease of its own — so once the parent lease's original window lapses it satisfies every condition here and mints a second AccessLease for the same cipher, breaking the single-active-lease invariant the extension path exists to preserve. When the governing rule does not set SingleActiveLease, nothing downstream catches it either.

Today this is only reachable if a caller passes an extension request's id to CreateFromApprovedRequestAsync, and the command layer isn't in this stack yet — so this is defense in depth on the write that authorizes access to Vault Data, matching the read path's existing guard.

Suggested change

WHERE

AR.[Id] = @AccessRequestId

AND AR.[RequesterId] = @RequesterId

AND AR.[Status] = 1 -- Approved

AND AR.[NotBefore] <= @Now

AND AR.[NotAfter] > @Now

AND NOT EXISTS (SELECT 1 FROM [dbo].[AccessLease] AL WHERE AL.[AccessRequestId] = AR.[Id])

WHERE

AR.[Id] = @AccessRequestId

AND AR.[RequesterId] = @RequesterId

AND AR.[Status] = 1 -- Approved

AND AR.[ExtensionOfLeaseId] IS NULL -- an extension extends its parent lease; it never mints one

AND AR.[NotBefore] <= @Now

AND AR.[NotAfter] > @Now

AND NOT EXISTS (SELECT 1 FROM [dbo].[AccessLease] AL WHERE AL.[AccessRequestId] = AR.[Id])

The EF repository would need the matching r.ExtensionOfLeaseId == null clause.

claude · 2026-08-11T19:06:24Z

+    [CreationDate]      DATETIME2 (7)       NOT NULL,
+    CONSTRAINT [PK_AccessLease] PRIMARY KEY CLUSTERED ([Id] ASC),
+    CONSTRAINT [FK_AccessLease_AccessRequest] FOREIGN KEY ([AccessRequestId]) REFERENCES [dbo].[AccessRequest] ([Id]),
+    CONSTRAINT [FK_AccessLease_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE


♻️ DEBT: FK_AccessLease_Organization is ON DELETE CASCADE but no MSSQL index leads with OrganizationId, so org deletion scans the whole table.

Details and fix

All five indexes on this table lead with RequesterId, NotAfter, CollectionId, CipherId, or AccessRequestId. Organization_DeleteById relies on the database cascade for this table, which forces SQL Server to scan AccessLease to find the rows to delete. AccessLease grows monotonically — ended leases are retained for the governance history reads — so this gets worse over time.

This is also a dual-ORM divergence: EF's convention creates IX_AccessLease_OrganizationId on all three providers (util/PostgresMigrations/.../20260811180700_AddAccessRequestAndLease.cs:128 and the MySQL/SQLite equivalents), so only MSSQL is missing it. AccessRequest is already covered by IX_AccessRequest_OrganizationId_Status, and every other org-scoped table in src/Sql/dbo (Collection, SecurityTask, AccessRule, …) carries an OrganizationId-leading index.

CREATE NONCLUSTERED INDEX [IX_AccessLease_OrganizationId] ON [dbo].[AccessLease] ([OrganizationId] ASC); GO

Needs the matching guarded CREATE INDEX in util/Migrator/DbScripts/2026-08-11_01_AddAccessRequestAndLease.sql.

…Lease Extract the MSSQL/Dapper persistence for the PAM access-request/lease flow: the AccessRequest, AccessDecision, and AccessLease SSDT tables; the 22 AccessRequest_*/AccessLease_* stored procedures; the Dapper AccessRequestRepository and AccessLeaseRepository (registered in DapperServiceCollectionExtensions); the repository integration tests; and one consolidated migration (2026-07-16_00_AddAccessRequestAndLease.sql) that folds poc's incremental scripts into a single create, matching the final SSDT shape verbatim. EF Core parity for the non-MSSQL providers lands in the next commit.

Author from scratch the non-MSSQL (PostgreSQL/MySQL/SQLite) persistence for the PAM access-request/lease flow, since the POC was Dapper/MSSQL-only. Adds the EF entity models + AutoMapper profiles, inline DatabaseContext configuration and DbSets, the EF AccessRequestRepository and AccessLeaseRepository implementing the domain interfaces with LINQ matching the stored-procedure behaviour, the DI registrations, and generated migrations for all three providers (AddAccessRequestAndLease, matching the MSSQL migration name). The two race-guarded writes (lease mint, approved extension) use a Serializable transaction as the cross-provider stand-in for the MSSQL procs' UPDLOCK/HOLDLOCK; a losing concurrent writer may surface a provider serialization error rather than a clean outcome enum. Cross-provider parity is covered by the [DatabaseData] integration tests.

…AM tests CoreHelpers.GenerateComb() is obsolete; PAM code standardizes on Bit.Core.Utilities.CombGuid.Generate() (already used by the domain entities).

These have to apply after everything already on main. The EF timestamps for all three providers sort after AddAccessRule, and the MSSQL script now takes the 01 sequence for 2026-08-11 because main picked up AddKeyRegenerationStoredProcedures on that date, which already holds 00. DbUp runs DbScripts in name order, so sharing a prefix would have left the two ordering on the rest of their filenames instead of on the sequence number that exists to settle it.

Only record an AccessDecision when the guarded write actually transitioned the row. AccessRequest_ResolveWithDecision and AccessLease_Revoke inserted unconditionally, so a losing approver's verdict or a repeat revoke appended a decision to a row it never changed, leaving the decision log contradicting that row's status. AccessLease_Revoke now derives the request id from the ended lease via OUTPUT rather than trusting a caller-supplied @AccessRequestId, which also drops that parameter. Add SET XACT_ABORT ON to the four transactional procedures that lacked it. Without it a constraint violation aborts only the failing statement, execution falls through to COMMIT, and half the transaction is persisted alone. Bound both of AccessRequest_ReadManyByRequesterId's result sets to the same 250-row page; the decision set was previously unbounded across the requester's entire history. Replace the OUTER APPLY/TOP 1 produced-lease lookups with LEFT JOINs now that IX_AccessLease_AccessRequestId is unique. Mirror the two behavioural changes in the EF repositories, and set the extension decision's AccessRequestId from the request being created; it was left at the caller's value, which violated FK_AccessDecision_AccessRequest on the EF providers while MSSQL passed. Cover the no-op decision paths, the broader cancellable set, and the governance history ordering with integration tests.

Timestamps do not round-trip exactly on any provider: Dapper binds a plain DateTime as DbType.DateTime -- the legacy 3.33 ms type -- so MSSQL writes are rounded before they reach the DATETIME2(7) columns, and the EF providers store microseconds rather than ticks. Four assertions compared with exact equality and were failing on SqlServer. LaxDateTimeComparer.Default allows 2 ms, which covers datetime's worst-case rounding error of 1/600 s, so the comparison is bounded rather than merely empirical.

This branch adds FK_AccessRequest_AccessRule, which does not cascade (NO ACTION on SQL Server, RESTRICT on the EF providers). AccessRule_DeleteById and the EF AccessRuleRepository.DeleteAsync only detached Collection.AccessRuleId, so once any request had pinned a rule that rule became undeletable on all four databases: an admin creating a rule, a member submitting a request against it, and the admin then removing the rule was enough to hit a foreign-key violation. Detach AccessRequest.RuleId alongside the collection links. RuleId is provenance rather than authority -- the request's own window and decision log are the record of what was granted -- and the column is already nullable for requests that were never gated through a stored rule. AccessRule_DeleteById has shipped, so it is re-deployed via CREATE OR ALTER in this branch's migration, which is where the FK that requires the change is added.

@now

Four indexes, none of which the tables carried: IX_AccessLease_CipherId_Status — AccessLease_CreateFromApprovedRequest's per-cipher singleton guard filters on CipherId alone under UPDLOCK/HOLDLOCK. With no CipherId-leading index the optimizer had to take either a clustered scan or a seek on IX_AccessLease_NotAfter_Status for the open-ended NotAfter > @now range, so the range lock covered every currently-active and future lease and serialized unrelated organizations' activations against each other for the life of the transaction. IX_AccessRequest_CollectionId_Status — the approver inbox reads join the caller's manageable collection ids, which neither existing index could serve, so the hottest read on the table scanned it in full. IX_AccessRequest_ExtensionOfLeaseId — the extension cap check runs an EXISTS on this column while holding the parent lease's UPDLOCK, so the scan happened inside the lock window. Also indexes FK_AccessRequest_AccessLease. IX_AccessRequest_RuleId — indexes FK_AccessRequest_AccessRule so the rule detach added in the previous commit seeks rather than scans. The three EF migrations are regenerated rather than hand-edited, which also replaces the hand-re-dated pair from a1bb877 with tool-authored files. They still sort after AddAccessRule, and has-pending-model-changes reports no drift on any provider.

ResolveWithDecisionAsync and CancelWithDecisionAsync were the last two EF writes still taking the decision's AccessRequestId from the caller. The MSSQL procedures reuse a single @AccessRequestId for both the guarded UPDATE and the AccessDecision insert, so on SQL Server the verdict always lands on the request being resolved; on EF a decision naming a different request would file it there instead. Matches CreateAutoApprovedAsync, CreateApprovedExtensionAsync, and the lease RevokeAsync, so the PAM audit trail is now identical across all four providers regardless of what the command layer hands in.

Three fixes, all cases where the EF path behaved differently from the stored procedures: The per-cipher singleton guard resolved the cipher from the caller's copy of the lease rather than from the request. The procedure reads it authoritatively via WHERE [CipherId] = (SELECT [CipherId] FROM [dbo].[AccessRequest] ...), and this method already treats the caller's other copies as non-authoritative, so a lease whose CipherId disagreed with its AccessRequestId was checked for contention against the wrong cipher and could mint a second concurrent active lease. A request that does not exist yields no cipher and the guard is skipped, leaving the precondition check to report the failure, mirroring the procedure's NULL scalar subquery. The mint path caught every DbUpdateException and reported PreconditionFailed, so a genuine persistence failure was indistinguishable from losing the activation race. Narrowed to duplicate-key violations only. Note this needs different codes from EntityFrameworkCache.IsDuplicateKeyException: the backstop is a unique index, which reports 2601 on SQL Server and 2067 on SQLite rather than the primary-key 2627/1555 that helper looks for. Organization deletion could not complete on the EF providers once an organization held an extended lease. Organization cascades to both AccessRequest and AccessLease while the two reference each other under Restrict, so whichever cascade the provider fired first was blocked by the other. The delete now detaches the extension links, then removes leases, then requests, before the existing AccessRule removal — which also releases the requests' RuleId hold. MSSQL was unaffected: its FKs are NO ACTION, validated once the cascade completes. OrganizationDeleteAsync_WithExtendedLease_Succeeds pins the last of these; it fails with a foreign-key violation on SQLite without the fix.

Several callers seed a lease they expect a read to exclude. With the outcome discarded those assertions would pass vacuously if the mint had never happened, which matters most on the EF path where a failed mint returns an outcome rather than throwing. Matches the other call sites, which already assert Minted.

This reverts commit 635bf0e.

rkac-bw

We discussed some future work on error handling and possible race conditions, but no blocking issues
Jira Notes

Hinton requested review from a team as code owners July 17, 2026 10:32

Hinton added the ai-review Request a Claude code review label Jul 17, 2026

Hinton requested a review from a team as a code owner July 17, 2026 10:32

Hinton added the t:feature Change Type - Feature Development label Jul 17, 2026

Hinton requested a review from eliykat July 17, 2026 10:32

claude Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread src/Infrastructure.EntityFramework/Pam/Repositories/AccessRequestRepository.cs

Hinton marked this pull request as draft July 17, 2026 11:20

Hinton commented Jul 17, 2026

View reviewed changes

Comment thread src/Sql/dbo/AdminConsole/Stored Procedures/Collection_SetAccessRuleAssociations.sql

github-code-quality Bot found potential problems Jul 17, 2026

View reviewed changes

Hinton force-pushed the pam/access-leasing-data branch from 49577aa to 6124c0d Compare July 17, 2026 13:36

Hinton removed request for a team and eliykat July 17, 2026 13:43

Hinton force-pushed the pam/access-leasing-data branch from 6124c0d to b9fe7d1 Compare July 17, 2026 13:45

github-code-quality Bot found potential problems Jul 17, 2026

View reviewed changes

Hinton force-pushed the pam/access-leasing-data branch from a615a81 to b8f1900 Compare July 17, 2026 14:25

Hinton force-pushed the pam/access-leasing-data branch from b8f1900 to 9113dde Compare July 27, 2026 08:49

Hinton force-pushed the pam/access-leasing-data branch from 9113dde to 1e78704 Compare July 27, 2026 09:06

Hinton force-pushed the pam/access-leasing-data branch from 1e78704 to 6e566aa Compare July 27, 2026 13:38

Hinton force-pushed the pam/access-leasing-data branch from 6e566aa to 13f364d Compare July 27, 2026 15:22

claude Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread src/Sql/dbo/Pam/Tables/AccessRequest.sql

claude Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread src/Sql/dbo/Pam/Stored Procedures/AccessLease_ReadManyEndedByCollectionIds.sql

claude Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread src/Infrastructure.EntityFramework/Pam/Repositories/AccessRequestRepository.cs

Hinton force-pushed the pam/access-leasing-data branch from d5c3c17 to b8113f7 Compare August 11, 2026 18:54

Hinton marked this pull request as ready for review August 11, 2026 18:57

claude Bot reviewed Aug 11, 2026

View reviewed changes

Base automatically changed from pam/access-leasing-domain to main August 12, 2026 09:29

Hinton added 11 commits August 12, 2026 11:29

Use CombGuid.Generate() over obsolete CoreHelpers.GenerateComb() in P…

97fc3db

…AM tests CoreHelpers.GenerateComb() is obsolete; PAM code standardizes on Bit.Core.Utilities.CombGuid.Generate() (already used by the domain entities).

abergs force-pushed the pam/access-leasing-data branch from b8113f7 to ace6a05 Compare August 12, 2026 09:29

patriksvensson added 2 commits August 12, 2026 13:40

Fix compilation errors

4bbcbdb

Fix RuleId column order

635bf0e

claude Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread util/PostgresMigrations/Migrations/20260812115656_AddAccessRequestAndLease.cs Outdated

Revert "Fix RuleId column order"

f8f6ba7

This reverts commit 635bf0e.

rkac-bw approved these changes Aug 12, 2026

View reviewed changes

abergs approved these changes Aug 12, 2026

View reviewed changes

JimmyVo16 approved these changes Aug 12, 2026

View reviewed changes

abergs merged commit 643e3aa into main Aug 12, 2026
67 checks passed

abergs deleted the pam/access-leasing-data branch August 12, 2026 18:06

Uh oh!

Conversation

Hinton commented Jul 17, 2026

🎟️ Tracking

📔 Objective

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

Uh oh!

Uh oh!

codecov Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kspearrin commented Jul 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rkac-bw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions Bot commented Jul 17, 2026 •

edited

Loading

codecov Bot commented Jul 17, 2026 •

edited

Loading

rkac-bw left a comment •

edited

Loading