PAM UAT - #8216
Draft
patriksvensson wants to merge 26 commits into
Draft
Conversation
patriksvensson
changed the base branch from
main
to
pam/cipher-partial-data-witness
August 14, 2026 10:56
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## pam/cipher-partial-data-witness #8216 +/- ##
===================================================================
+ Coverage 63.32% 69.10% +5.78%
===================================================================
Files 2387 2425 +38
Lines 104060 105811 +1751
Branches 9422 9551 +129
===================================================================
+ Hits 65891 73118 +7227
+ Misses 35914 30311 -5603
- Partials 2255 2382 +127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The witness test was written three ways — `fullAccess.Authorizes(id)`, `fullAccess?.Authorizes(id) == true`, and `is not null && Authorizes(id)` — at the branch that decides whether secrets go out. One of those getting inverted leaks, and only the path it sits on would notice. A static `From` on each abstract base now owns that test, so it appears once per family and nowhere else. Call sites state what they want and get the shape the witness permits; the five casts to the base type and the controller's `BuildCipherMiniResponse` helper are gone with it. Paths authorized out of band still construct their `Full*` directly, which keeps `_cipherLeaseGate.Unrestricted()` visible at the admin and export call sites rather than hidden behind a factory.
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>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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>
The collection browser renders a "Controlled access" column, but a collection row had nothing to put in it: no collection response model carried the PAM association, so a client could only learn that a collection is governed by reading the organization's access rules itself. Providers cannot do that -- the access-rules endpoint requires organization membership, deliberately, because rules gate who may lease credentials out of an organization and that is not a provider's to read. Their column stays empty however the client is written. Surface the fact on the collection instead, as HasEnabledAccessRule on the responses that feed sync and the organization listing. The flag is derived rather than the stored association. Collection.AccessRuleId records which rule governs a collection, but a rule that is switched off gates nothing, so reporting the association alone would start badging collections whose rule is disabled. Each read path therefore joins AccessRule and reports whether it is enabled: MSSQL in Collection_ReadByUserId, Collection_ReadByIdWithPermissions and Collection_ReadSharedCollectionsByOrganizationIdWithPermissions, Entity Framework via an EXISTS subquery in UserCollectionDetailsQuery and CollectionAdminDetailsQuery (Collection has no AccessRule navigation property to traverse) carried through the GROUP BY key of all three repository reads. Collection_ReadByUserId needed its columns qualified: joining AccessRule makes Id, OrganizationId and Name ambiguous against the UserCollectionDetails table function. Nothing writes the column and no request model gains a field -- SetAccessRuleAssociationsAsync remains the single writer of the association. Covered by CollectionRepositoryHasEnabledAccessRuleTests, which runs the same truth table -- ungoverned, governed by an enabled rule, governed by a disabled rule -- against every read path on both implementations.
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>
GetManagingUserIdsAsync admits four kinds of manager and the tests exercised three: direct Manage, Manage through a group, and an Owner with and without organization-wide admin access. The Custom member holding EditAnyCollection went untested. That is the branch where the two implementations are most likely to drift, because it is the only one that reads the Permissions column rather than a typed one. MSSQL asks the column directly with JSON_VALUE(..., '$.editAnyCollection'), while Entity Framework deserializes the blob and reads the property, so the two agree only as long as the serializer keeps writing camelCase keys. Nothing in either implementation states that dependency, and a change to the serializer would break MSSQL alone. Turn admin access off in the new case so that EditAnyCollection is the only thing that can grant Manage, and pair the Custom member holding it against one holding ManageGroups, so the assertion distinguishes the permission from the member type.
CreateTestUserAsync names the user "{identifier}-{guid}", and User.Name is 50
characters, so an identifier longer than 13 overflows it. "editanycollection" is 17.
Only Postgres reports this. The MSSQL path passes the name to a procedure whose
parameter is declared NVARCHAR(50), and SQL Server truncates silently on assignment
rather than raising, while SQLite does not enforce declared lengths at all -- so the
test passed on both while inserting a name it had quietly cut short.
Collection_ReadManagingUserIds and the three HasEnabledAccessRule read paths go out together, so they are one deployment step rather than two dated scripts. Re-dated to the day the pair lands. Also drops the note above Collection_ReadByUserId's SELECT explaining why its columns carry the UCD prefix.
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.
GetManagingUserIdsAsync loaded every confirmed member of the organization -- including the Permissions column, an NVARCHAR(MAX) -- and then resolved the four Manage routes in memory against a dictionary. Cost scaled with the size of the organization rather than with the collection's access list, which is the shape the read was added to avoid, and the blob came back for members that could never need it. Each route is a query now, unioned in the database, in the shape CollectionCipherRepository.GetUserIdsByCollectionIdsAsync already uses to resolve access from the same three assignment sources. AllowAdminAccessToAllCollectionItems becomes a join predicate rather than a separate read and a branch in C#. Five round trips become two, and neither scales with member count. Permissions is still parsed in memory, but only for Custom members, who are the only ones EditAnyCollection can apply to. It stays in memory because EF maps the column as a plain string: nothing in the model says it holds JSON, so there is nothing to translate. Querying it would mean mapping the column as an owned or complex type, which hands EF's serializer the format contract that Collection_ReadManagingUserIds reads with JSON_VALUE(..., '$.editAnyCollection') and CoreHelpers.ClassToJsonData writes -- a serializer change on either side would silently break the procedure. There is no portable alternative: the SQL Server provider exposes no JSON function at all, Npgsql and Pomelo expose their own, and the issue tracking a provider-neutral one (dotnet/efcore#29306) is still open. The one place the EF layer does filter a JSON blob in the database, OrganizationReportRepository.ReadLatestByOrganizationIdAsync, does it with a substring match under a comment warning that it fails silently. Cover the two mechanisms this changes: the confirmed-status filter, which moved from a dictionary lookup into the joins, and deduplication, which moved from a HashSet to Union and Distinct. Both cases run against the procedure and the EF path, so they also hold the two implementations to the same answer.
The [HasEnabledAccessRule] alias already says what the column holds. The rest of the note argued that MAX() is safe over a group keyed by [AccessRuleId], which is a case made to a reader of the diff rather than to a reader of the procedure. Removed from all three read procedures and from their copies in the dated script.
The script was dated 2026-08-21 while the newest script on main is 2026-08-14_04. Anything that merges before this PR does will claim a later date, leaving this one to sort ahead of scripts that were already applied, and 2026-08-2x_00 is a slot another PR could just as easily take. Renamed to 2026-08-25_00, unused on main and on pam/uat. Contents are unchanged and nothing refers to the filename -- Migrator embeds DbScripts\**\*.sql by wildcard.
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.
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
3 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.
No description provided.