[PM-40336] Access Rules: domain, persistence & schema#7981
Conversation
|
Reviewed the automated code-quality suggestions. Decisions below: 1. 2. Dapper 3. 4. EF Net: keeping (1), (2), and (4) as-is by design; (3) is an optional cosmetic tweak. |
f5a81b6 to
74777a5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7981 +/- ##
===========================================
- Coverage 66.58% 15.00% -51.58%
===========================================
Files 2277 1391 -886
Lines 99452 60449 -39003
Branches 8983 4806 -4177
===========================================
- Hits 66220 9071 -57149
- Misses 30963 51224 +20261
+ Partials 2269 154 -2115 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The audit-event kinds, payload fields, and emitter doc referenced request, lease, credential, and system-control events that no action emits yet. Reduce AccessAuditEventKind to the rule group (RuleCreated/Updated/Deleted, 0-9) and AccessAuditEventData to the fields the rule commands set, keeping the emitter seam for the read side that lands later.
Wrap the EF delete (clear collection links, then remove the rule) in an explicit transaction so a partial delete cannot leave ungoverned collections. Read rule details via ReadFirstOrDefaultAsync and make the Dapper collection-mapping properties settable.
The audit-event kinds, phases, payload, and emitter seam were incomplete and need proper design before they can be relied on (notably a read-side pairing key). Drop AccessAuditEventKind, AccessAuditEventPhase, AccessAuditEventData, IAccessAuditEventEmitter, and NoopAccessAuditEventEmitter, and remove the comments that referenced those events. Audit will be reintroduced as its own piece of work.
There was a problem hiding this comment.
@eliykat Do you think this sproc belongs to AC or PAM?
There was a problem hiding this comment.
I could go either way. PAM will be the ones most likely to update it in the future. But it's probably best that all Collection sprocs stay together and that the owner of the table (AC) have some oversight for the sprocs that change it. Overall I would say AC should own it, but let me know if you disagree.
Replace the per-directory PAM entries with one **/Pam* glob covering the Pam service, Pam.Domain, the Infrastructure Pam repos, and the PAM integration tests. Re-assert dept-dbops on src/Sql/dbo/Pam so PAM DB schema keeps its DBOps review, consistent with src/Sql/**.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the domain and persistence layer for org-scoped PAM Access Rules: the new Code Review Details
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40336
📔 Objective
Add the domain and persistence layer for org-scoped PAM Access Rules — the
reusable policy objects that gate PAM credential leasing. This is the first of two
stacked PRs; the commercial CRUD API that consumes this layer follows in #7983
(stacked on this branch).
Access-rule data (name, description, conditions) is organization configuration
metadata, not Vault Data — no zero-knowledge surface is touched. Nothing here is
reachable at runtime until the API lands and the
pm-37044-pam-v-0(
FeatureFlagKeys.Pam) flag is enabled.What's included
Domain — new
Pam.Domainlibrary (src/Pam.Domain)AccessRuleentity: org-scoped rule with lease semantics —SingleActiveLease,DefaultLeaseDurationSeconds,MaxLeaseDurationSeconds,Enabled,AllowsExtensions/MaxExtensionDurationSeconds, andLastEditedBy.Conditionsstored as a JSONAccessConditiontree (validated in [PM-40336] Access Rules: API #7983).AccessRuleDetails(rule + governed collection IDs) andIAccessRuleRepository.Persistence
Infrastructure.Dapper) and EF Core (Infrastructure.EntityFramework)AccessRuleRepositoryimplementations, registered in DI.AccessRuletable + stored procedures (_Create,_Update,_DeleteById,_ReadById,_ReadByOrganizationId,_ReadDetailsById,_ReadDetailsByOrganizationId,Collection_SetAccessRuleAssociations).2026-07-14_01_AddAccessRule.sql) and EF migrations +snapshots for Postgres/MySQL/SQLite.
Collection association
Collection.AccessRuleIdFK (null = leasing disabled for that collection);updated
Collection_*stored procedures and theCollectiontable.(Touches AdminConsole-owned files — flagging for that team's review.)