[PM-40336] Access Rules: domain, persistence & schema - #7981
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Hinton
changed the title
|
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 67.48% 67.57% +0.08%
==========================================
Files 2314 2319 +5
Lines 100435 100661 +226
Branches 9035 9044 +9
==========================================
+ Hits 67774 68017 +243
+ Misses 30381 30364 -17
Partials 2280 2280 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the domain and persistence layer for org-scoped PAM Access Rules: the new Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
3c05d0f to
95d575b
Compare
d680dcf to
5b920b2
Compare
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Generally LGTM aside from the 1 question below.
Collection_SetAccessRuleAssociations is written as a collections sproc and its SSDT definition is already AdminConsole-owned, so keeping the method on IAccessRuleRepository split ownership: AC owned the MSSQL sproc while PAM owned the EF implementation. Both implementations now live on ICollectionRepository alongside the sproc they wrap. Addresses review feedback on #7981.
patriksvensson
left a comment
patriksvensson
left a comment
There was a problem hiding this comment.
Some minor things that probably would be good to fix, but does not block IMO.
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.
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/**.
…lder Collection sprocs stay together under the table's owning team (AC), per PR review.
The AccessRule migrations were generated on 2026-07-14, before the migrations that have since landed on main, so the chronological-order check fails. Re-date them to the end of the chain. The MSSQL script is a pure rename; its contents are unchanged and every statement is already guarded, so re-running it on a database that applied it under the old name is a no-op. The EF migrations are regenerated rather than renamed, so each provider's Designer snapshot targets the model as of its new position in the chain. The Up/Down bodies are byte-identical to the originals and DatabaseContextModelSnapshot is unchanged.
Infrastructure.Dapper and Infrastructure.EntityFramework now reference Pam.Domain directly, so every project that transitively depends on them (Api, Admin, Identity, the Pam service, tests, etc.) needs the reference recorded in its own lock file. Force-evaluated a full-solution restore to bring every lock file back in sync with the actual dependency graph.
Collection_SetAccessRuleAssociations is written as a collections sproc and its SSDT definition is already AdminConsole-owned, so keeping the method on IAccessRuleRepository split ownership: AC owned the MSSQL sproc while PAM owned the EF implementation. Both implementations now live on ICollectionRepository alongside the sproc they wrap. Addresses review feedback on #7981.
The method moved onto ICollectionRepository so that AC owns both the sproc and its callers, but it landed without direct coverage — the only exercise it got was an incidental call inside the AccessRule delete test. Each case pins a clause the MSSQL sproc and the EF reimplementation have to agree on: the clear pass is scoped to the rule being written, both passes are scoped to the organization, clear runs before assign, and assign is unqualified so a collection can move between rules. Also covers the account revision bump and the empty-set no-op the commands rely on. Addresses review feedback on #7981.
ReplaceAsync is the standard collection-edit path and knows nothing about PAM, but Collection_Update assigned [AccessRuleId] = @AccessRuleId unconditionally. The three Dapper wrapper classes it routes through copy Collection's properties by hand and stopped at DefaultUserCollectionEmail, so Dapper bound the inherited parameter as NULL and every admin-console collection edit erased the governing rule on SQL Server. EF preserved it, so the two stacks disagreed. Rather than teach the wrappers to carry the value — which fixes this instance and leaves the next new column to regress the same way — take the column out of the generic write paths entirely. Collection_Create and Collection_Update still accept @AccessRuleId, because Dapper binds every property on the entity and dropping it would raise "too many arguments specified", but neither assigns it. EF excludes it from tracked inserts and updates via PropertySaveBehavior. The association is now writable only by SetAccessRuleAssociationsAsync and the clear inside AccessRuleRepository.DeleteAsync, both of which use ExecuteUpdate and so bypass the change tracker. That makes an accidental erasure structurally impossible instead of merely fixed, and stops an ordinary edit forging an association it was never granted. Tests pin the invariant in both directions across all four ReplaceAsync group/user branches. No schema change, so no EF migration; the AccessRule migration is unshipped and carries the same sproc bodies as SSDT. Addresses review feedback on #7981.
Both passes of Collection_SetAccessRuleAssociations qualify the collections by organization, but only the clear pass qualified the rule -- it matches on AccessRuleId, so it cannot touch another rule's collections. The assign pass had no such qualifier, and the foreign key only proves the rule exists. A caller could therefore govern its own collections with another organization's rule, handing that organization control of the conditions gating access to data it cannot see. Both implementations now require the rule to belong to the organization, refusing the assignment rather than erroring, which is how a collection from another organization is already treated. The MSSQL procedure also opened a transaction without XACT_ABORT and without TRY/CATCH. A run-time error in the assign pass aborts only that statement, so execution reached COMMIT and committed the clear on its own, detaching collections that were meant to be reassigned. EF was already atomic here -- its transaction rolls back when the scope exits without committing -- so this was also a divergence between the two stacks. Addresses review feedback on #7981.
Organization cascades to both Collection and AccessRule, while Collection.AccessRuleId -> AccessRule does not cascade, so deleting an organization that still had a governed collection depended on which of those two cascade paths the database applied first. EF's OrganizationRepository.DeleteAsync deletes neither table explicitly -- it relies on the cascade when the organization row is removed -- so nothing made that order deterministic. On MySQL it is reliably the wrong order: InnoDB reaches the AccessRule rows while the collections still reference them and the delete fails outright with "Cannot delete or update a parent row" on FK_Collection_AccessRule_AccessRuleId. Organization deletion was simply broken on MySQL for any organization with an active rule. PostgreSQL and SQLite happen to survive because the Collection foreign key predates the AccessRule one and its trigger fires first, which is not a guarantee worth resting on -- pg_dump recreates constraints in name order, and FK_AccessRule_ sorts before FK_Collection_. Detaching the collections and removing the rules explicitly, alongside the twenty other tables this method already clears by hand, makes the outcome identical on all four databases. Note this could not have been fixed by relaxing the foreign key to NO ACTION: InnoDB treats NO ACTION as a synonym for RESTRICT, so the only affected provider would have been unaffected by the change. Verified against SQL Server, PostgreSQL, MySQL and SQLite.
GetManyByOrganizationIdAsync and GetManyDetailsByOrganizationIdAsync had no coverage on either stack. Both are org-scoped reads and the two implementations assemble their results differently -- MSSQL returns the governed collection IDs as a second result set while EF groups them in memory -- so each test pins a clause both have to agree on: the scoping, the grouping of collections onto the right rule, and an ungoverning rule coming back with an empty list rather than null or another rule's collections. Also pins that every AccessRule column survives the round trip. Dapper maps sproc columns onto AccessRuleDetails directly while EF goes through AutoMapper, so a column missing from one side is a silent divergence rather than a failure. Addresses review feedback on #7981.
IX_Collection_OrganizationId_IncludeAll exists so that a seek on Collection.OrganizationId is covering. UserCollectionDetails drives the sync path -- it seeks Collection on OrganizationId and projects C.* -- so adding AccessRuleId to the table without adding it to the index costs a key lookup per collection on every sync. The migration guards on the included column rather than the index name: the index has existed since 2018, so only its column list distinguishes a database that has had this applied from one that has not. Recreating it with DROP_EXISTING keeps the index available while it is rebuilt.
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
One concern below, but if you can & want to address it in a follow-up PR that's OK.
| -- governed; the association is established only by [dbo].[Collection_SetAccessRuleAssociations], so | ||
| -- [AccessRuleId] is left to its NULL default here. Retained because Dapper binds every property on | ||
| -- the Collection entity. | ||
| @AccessRuleId UNIQUEIDENTIFIER = NULL |
There was a problem hiding this comment.
This isn't ideal. Any caller would assume that the whole object is being saved. Could you call EXEC [dbo].[Collection_SetAccessRuleAssociations] if AccessRuleId is not null - just to honour the interface contract? Alternatively, we could pass a data model (or interface) that is a subset of the Collection entity, but that may have broader ripples through the codebase.
Open to other ideas, this can also be handled in a follow-up PR if possible.
There was a problem hiding this comment.
That is also a bit sub-optimal though. Setting an access rule would work, but not removing it.
| -- Accepted and deliberately ignored. [AccessRuleId] has a single writer, | ||
| -- [dbo].[Collection_SetAccessRuleAssociations] (cleared by [dbo].[AccessRule_DeleteById]), so this | ||
| -- procedure must never assign it: callers pass whole-entity updates that know nothing about PAM, and | ||
| -- assigning it here erases the association. The parameter is retained because Dapper binds every | ||
| -- property on the Collection entity, so dropping it would raise "too many arguments specified". | ||
| @AccessRuleId UNIQUEIDENTIFIER = NULL |
4 participants
🎟️ 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.)