[PM-40526] Access Leasing: domain

[Hinton]

🎟️ 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.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds the PAM access-request/lease domain (AccessRequest, AccessDecision, AccessLease, their enums, the AccessRequestDetails/AccessRequestDecision projections, and the two repository contracts) under src/Pam.Domain, and drops the four duplicated wire enums in favour of the domain ones. All added code is declarative — entities, enums, and interfaces with no behaviour — and the PAM endpoint handlers remain scaffolds that throw NotImplementedException, so there is no runtime path to exercise. Swapping the wire enums for the domain enums renumbers the serialized AccessRequestStatus (Denied 3→2, Cancelled 4→3, Expired 5→4, Activated removed) and adds AccessLeaseStatus.Cancelled; since nothing serves these endpoints yet this is a spec/SDK-codegen change rather than a live contract break, and it matches the direction already agreed in the earlier review threads. No new findings this pass.

Code Review Details

No new findings.

Five earlier threads on this PR are still unresolved and were intentionally not re-raised:

• src/Pam.Domain/Entities/AccessLease.cs — RevokedDate/RevokedBy docs vs. RevokeAsync's Cancelled path
• src/Pam.Domain/Entities/AccessDecision.cs — "exactly one decision per request"
• src/Pam.Domain/Models/AccessRequestDetails.cs — ProducedLeaseStatus / Decisions projection docs
• src/Pam.Domain/Models/AccessRequestDecision.cs — Id naming
• src/Pam.Domain/Repositories/IAccessRequestRepository.cs — CancelWithDecisionAsync naming

[codecov]

Codecov Report

❌ Patch coverage is 0% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.10%. Comparing base (63728c3) to head (47dd23a).

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 ⚠️
...els/Response/AccessRequestDecisionResponseModel.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8001      +/-   ##
==========================================
+ Coverage   63.89%   68.10%   +4.20%     
==========================================
  Files        2359     2371      +12     
  Lines      102319   102795     +476     
  Branches     9240     9320      +80     
==========================================
+ Hits        65379    70006    +4627     
+ Misses      34713    30469    -4244     
- Partials     2227     2320      +93     

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

The merge-base changed after approval.

[claude]

♻️ DEBT: RevokedDate/RevokedBy docs contradict RevokeAsync — both are also set for Cancelled

Details and fix

IAccessLeaseRepository.RevokeAsync in this same PR writes the revoked date and revoker for either end status:

setting its status to endStatus (Revoked when an operator ended it, Cancelled when the holder ended their own) along with its revoked date and revoker

and GetManyEndedByCollectionIdsAsync relies on that ("A revoked/cancelled lease's end is its revoked date"). So "NULL otherwise" / "NULL unless Status is Revoked" is wrong for cancellations, and RevokedBy is the holder — not an operator — in that path. This matters because the stacked schema PR reads these docs: a CHECK (RevokedDate IS NULL OR Status = 2) constraint derived from them would reject every self-cancel.

Suggested change

	/// <summary>
	/// Set when an operator revokes the lease (<see cref="AccessLeaseStatus.Revoked"/>). NULL otherwise.
	/// </summary>
	public DateTime? RevokedDate { get; set; }
	/// <summary>
	/// The operator who revoked the lease. NULL unless <see cref="Status"/> is <see cref="AccessLeaseStatus.Revoked"/>.
	/// </summary>
	public Guid? RevokedBy { get; set; }
	/// <summary>
	/// When the lease was ended early — set for both <see cref="AccessLeaseStatus.Revoked"/> (an operator ended it)
	/// and <see cref="AccessLeaseStatus.Cancelled"/> (the holder ended their own). NULL otherwise.
	/// </summary>
	public DateTime? RevokedDate { get; set; }
	/// <summary>
	/// Who ended the lease early: the operator who revoked it, or the holder who cancelled their own. NULL unless
	/// <see cref="Status"/> is <see cref="AccessLeaseStatus.Revoked"/> or <see cref="AccessLeaseStatus.Cancelled"/>.
	/// </summary>
	public Guid? RevokedBy { get; set; }

[patriksvensson]

I think this should be fixed as well.

[claude]

♻️ DEBT: Two projection docs are narrower than the states the repository contracts can produce

Details and fix

• ProducedLeaseStatus lists "Active/Expired/Revoked" but AccessLeaseStatus also has Cancelled, which RevokeAsync writes for holder-ended leases — an inbox row can carry it.
• Decisions says "Empty only while pending", yet two non-pending terminal states have no decision row: CancelAsync explicitly writes none ("No AccessDecision is written"), and ExpiredUnanswered is "the approval window lapsed with no decision recorded". A consumer that treats a non-pending request as guaranteed to have Decisions[0] would throw.

Suggested change

	/// <summary>
	/// The produced lease's current status (Active/Expired/Revoked), or null when the request has not produced a
	/// lease. Lets the inbox distinguish a still-live lease from one that has ended, so an ended lease is not offered
	/// for revocation.
	/// </summary>
	public AccessLeaseStatus? ProducedLeaseStatus { get; set; }
	/// <summary>
	/// Every decision recorded against this request, oldest first — one element per
	/// <see cref="Entities.AccessDecision"/> row (human or automatic; identity denormalized from the User join for
	/// human decisions). Empty only while pending (no decision recorded yet). The resolved reads return the decisions
	/// as a second result set that the repository groups onto this list; the constructed reads (decision result,
	/// cipher access-state snapshot) set it directly.
	/// </summary>
	public List<AccessRequestDecision> Decisions { get; set; } = new();
	/// <summary>
	/// The produced lease's current status (Active/Expired/Revoked/Cancelled), or null when the request has not
	/// produced a lease. Lets the inbox distinguish a still-live lease from one that has ended, so an ended lease is
	/// not offered for revocation.
	/// </summary>
	public AccessLeaseStatus? ProducedLeaseStatus { get; set; }
	/// <summary>
	/// Every decision recorded against this request, oldest first — one element per
	/// <see cref="Entities.AccessDecision"/> row (human or automatic; identity denormalized from the User join for
	/// human decisions). Empty while pending, and for the terminal states that record no verdict: a requester
	/// cancellation (<c>IAccessRequestRepository.CancelAsync</c>) and
	/// <see cref="AccessRequestStatus.ExpiredUnanswered"/>. The resolved reads return the decisions as a second result
	/// set that the repository groups onto this list; the constructed reads (decision result, cipher access-state
	/// snapshot) set it directly.
	/// </summary>
	public List<AccessRequestDecision> Decisions { get; set; } = new();

[claude]

🎨 SUGGESTED: Rename Id to ApproverId — on a class named AccessRequestDecision, Id reads as the decision's id

Details and fix

This projects an AccessDecision row, which has its own Id column, but here Id holds AccessDecision.ApproverId. The doc comment has to correct the name ("The human approver"), and the sibling Name/Email are also the approver's.

The concrete hazard is in the stacked data PR: Dapper maps by column name, so a read procedure that selects ad.Id (natural when projecting a decision row) binds the decision id into this property with no compiler or test signal. Naming it ApproverId makes that mismatch impossible and matches AccessDecision.ApproverId.

Suggested change

	public Guid? Id { get; set; }
	/// <summary>The human approver, or null for an automatic decision.</summary>
	public Guid? ApproverId { get; set; }

The class-level <see cref="Id"/> references above would need the same update.

[patriksvensson]

Just some minor things that Claude already complained about.

[patriksvensson]

I think this should be fixed as well.

[patriksvensson]

I think this should be fixed as well.