[PM-40526] Access Leasing: persistence & schema#8002
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the PAM access-request/lease persistence layer across both ORMs: the SSDT tables, 22 stored procedures, the consolidated MSSQL migration, the Dapper and EF Core repositories, the EF One correctness divergence between the two ORMs remains open (see below). Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var decisionEntity = Mapper.Map<EfDecision>(decision); | ||
| decisionEntity.DeciderKind = AccessDeciderKind.Automatic; | ||
| decisionEntity.ApproverId = null; | ||
| decisionEntity.ConditionKind = null; | ||
| decisionEntity.Verdict = AccessDecisionVerdict.Approve; | ||
| decisionEntity.Comment = null; | ||
| decisionEntity.EvaluationContext = null; | ||
| decisionEntity.CreationDate = now; |
There was a problem hiding this comment.
AccessRequestId, diverging from the MSSQL proc and risking an FK violation.
Details and fix
The MSSQL AccessRequest_CreateApprovedExtension proc inserts the AccessDecision with @AccessRequestId (the new extension request's Id). Here the decision entity is mapped from decision but AccessRequestId is never assigned — unlike CreateAutoApprovedAsync (line 39: decisionEntity.AccessRequestId = request.Id).
The integration test's BuildAutoDecision leaves AccessRequestId at Guid.Empty, so on the EF providers the decision row gets AccessRequestId = Guid.Empty, which references no AccessRequest and violates FK_AccessDecision_AccessRequest at SaveChangesAsync. Unlike the mint path, this method has no DbUpdateException catch, so the extension throws on EF while succeeding on MSSQL — a cross-provider parity break.
var decisionEntity = Mapper.Map<EfDecision>(decision);
decisionEntity.AccessRequestId = requestEntity.Id;
decisionEntity.DeciderKind = AccessDeciderKind.Automatic;
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## pam/access-leasing-domain #8002 +/- ##
=============================================================
- Coverage 66.68% 61.73% -4.96%
=============================================================
Files 2313 2320 +7
Lines 100411 101200 +789
Branches 9068 9120 +52
=============================================================
- Hits 66959 62475 -4484
- Misses 31172 36544 +5372
+ Partials 2280 2181 -99 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
49577aa to
6124c0d
Compare
d15334c to
b7eebb4
Compare
…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.
6124c0d to
b9fe7d1
Compare
b7eebb4 to
3c540d8
Compare
| foreach (var detail in details) | ||
| { | ||
| if (decisionsByRequest.TryGetValue(detail.Id, out var decisions)) | ||
| { | ||
| detail.Decisions = decisions; | ||
| } | ||
| } |
…AM tests CoreHelpers.GenerateComb() is obsolete; PAM code standardizes on Bit.Core.Utilities.CombGuid.Generate() (already used by the domain entities).
a615a81 to
b8f1900
Compare
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40526
📔 Objective
Persistence & schema for the PAM access-request/lease domain (introduced in #8001), across both ORMs:
AccessRequest,AccessDecision, andAccessLeaseSSDT tables; the 22AccessRequest_*/AccessLease_*stored procedures; the DapperAccessRequestRepositoryandAccessLeaseRepository; 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.DatabaseContextconfiguration 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.Collection_SetAccessRuleAssociationsto 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
Serializabletransaction 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.