Skip to content

[PM-40526] Access Leasing: domain#8001

Open
Hinton wants to merge 3 commits into
pam/access-rule-enginefrom
pam/access-leasing-domain
Open

[PM-40526] Access Leasing: domain#8001
Hinton wants to merge 3 commits into
pam/access-rule-enginefrom
pam/access-leasing-domain

Conversation

@Hinton

@Hinton Hinton commented Jul 17, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

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

📔 Objective

Extract the pure domain for the PAM access-request/lease flow into Pam.Domain, alongside the existing AccessRule domain:

  • The AccessRequest, AccessDecision, and AccessLease entities and their status/kind/verdict/outcome enums.
  • The request projection models (AccessRequestDetails, AccessRequestDecision).
  • The IAccessRequestRepository and IAccessLeaseRepository contracts.

Requests and leases ship as one feature because their persistence is coupled (AccessRequest.ExtensionOfLeaseId FKs AccessLease; the request reads project lease status). No audit surface is included (deferred, as on the rule slice).

Stacked PR — targets pam/access-rule-engine. Review/merge the engine PR (#7992) first. Persistence & schema for these entities follows in the data PR stacked on this branch.

@Hinton
Hinton requested a review from a team as a code owner July 17, 2026 10:32
@Hinton Hinton added ai-review Request a Claude code review t:feature Change Type - Feature Development labels Jul 17, 2026
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the pure-domain slice extracting the PAM access-request/lease flow into Pam.Domain: the AccessRequest, AccessDecision, and AccessLease entities, their status/kind/verdict/outcome enums, the AccessRequestDetails/AccessRequestDecision projections, and the IAccessRequestRepository/IAccessLeaseRepository contracts. The entities follow the established repo conventions (ITableObject<Guid>, CombGuid.Generate(), UTC construction stamps) consistent with the sibling AccessRule, and the two outcome enums correctly use signed int backing to carry stored-proc return codes including -1. No persistence, DI, or runtime behavior ships here, so there is no logic to unit-test in this slice.

Code Review Details
  • 🎨 : <see cref="Models.Conditions.AccessCondition"/> references a type that does not exist; the sibling AccessRule.cs uses plain <c> markup for the same concept. Doc-only, no build impact.
    • src/Pam.Domain/Enums/AccessConditionKind.cs:5

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.68%. Comparing base (f4a2254) to head (3c540d8).

Files with missing lines Patch % Lines
src/Pam.Domain/Models/AccessRequestDetails.cs 0.00% 18 Missing ⚠️
src/Pam.Domain/Entities/AccessRequest.cs 0.00% 16 Missing ⚠️
src/Pam.Domain/Entities/AccessLease.cs 0.00% 15 Missing ⚠️
src/Pam.Domain/Entities/AccessDecision.cs 0.00% 12 Missing ⚠️
src/Pam.Domain/Models/AccessRequestDecision.cs 0.00% 7 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           pam/access-rule-engine    #8001      +/-   ##
==========================================================
+ Coverage                   62.20%   66.68%   +4.47%     
==========================================================
  Files                        2308     2313       +5     
  Lines                      100343   100411      +68     
  Branches                     9068     9068              
==========================================================
+ Hits                        62422    66959    +4537     
+ Misses                      35740    31172    -4568     
- Partials                     2181     2280      +99     

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

@Hinton
Hinton force-pushed the pam/access-leasing-domain branch from d15334c to b7eebb4 Compare July 17, 2026 13:36
@Hinton
Hinton force-pushed the pam/access-rule-engine branch from fcbbb6d to 504efaf Compare July 17, 2026 13:36
Hinton added 3 commits July 17, 2026 15:44
Extract the pure domain for the PAM access-request/lease flow into Pam.Domain,
alongside the existing AccessRule domain: the AccessRequest and AccessDecision
and AccessLease entities, their status/kind/verdict/outcome enums, the request
projection models (AccessRequestDetails, AccessRequestDecision), and the
IAccessRequestRepository and IAccessLeaseRepository contracts.

Requests and leases ship as one feature because their persistence is coupled
(AccessRequest.ExtensionOfLeaseId FKs AccessLease; the request reads project
lease status). No audit surface is included (deferred, as on the rule slice).
Add XML doc comments to the AccessRequest/AccessLease/AccessDecision
entities, their status/verdict/kind enum members, and the denormalized
identity fields on the AccessRequestDetails read model. Documents the
non-obvious semantics (nullability, lifecycle, granted vs requested
window) while leaving plain ids and straight projections bare.
Use <inheritdoc cref> on the six read-model fields that project
AccessRequest verbatim, so their documentation tracks the entity
instead of duplicating it.
@Hinton
Hinton force-pushed the pam/access-rule-engine branch from 504efaf to f4a2254 Compare July 17, 2026 13:45
@Hinton
Hinton force-pushed the pam/access-leasing-domain branch from b7eebb4 to 3c540d8 Compare July 17, 2026 13:45

/// <summary>
/// The kind of access condition that produced an automatic <see cref="Entities.AccessDecision"/>. Mirrors the
/// <c>kind</c> discriminator on <see cref="Models.Conditions.AccessCondition"/>, which remains the JSON wire format

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 SUGGESTED: Models.Conditions.AccessCondition does not resolve

Details and fix

No AccessCondition type exists under Bit.Pam.Models.Conditions (or anywhere in the codebase), so this <see cref> will not link. The sibling AccessRule.cs documents the same concept with plain code markup precisely because the type isn't referenceable from this project:

/// <c>kind</c> discriminator on <c>AccessCondition</c>, which remains the JSON wire format

Switching to <c>AccessCondition</c> keeps the two files consistent and avoids a dangling reference. (Doc-only; no build impact since GenerateDocumentationFile is not set.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants