Skip to content

Email the lease holder when an operator revokes their access - #8292

Draft
maxkpower wants to merge 7 commits into
pam/PM-42817/mail-request-decidedfrom
pam/PM-42817/mail-lease-revoked
Draft

Email the lease holder when an operator revokes their access#8292
maxkpower wants to merge 7 commits into
pam/PM-42817/mail-request-decidedfrom
pam/PM-42817/mail-lease-revoked

Conversation

@maxkpower

Copy link
Copy Markdown

🎟️ Tracking

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

📔 Objective

Emails a lease holder when an operator revokes their active access. The push already re-locks their client; this tells them why.

  • Sent only when AccessLeaseAction.Revoked. A holder ending their own lease (Cancelled) receives nothing: mailing someone about their own action is what trains people to ignore the channel.
  • Says "revoked", matching pamStatusRevoked and pamAuditKindLeaseRevoked in the web vault. The product reserves "ended" for the holder's own action.
  • States that the access cannot be resumed and that regaining it needs a new request.
  • The revocation reason is not included and stays on the request.

This is a courtesy, not a control: the lease is already dead server side before the mail is composed.

Top of a four PR stack on pam/uat. Sits on the decision notification PR.

📸 Screenshots

@maxkpower maxkpower added the ai-review Request a Claude code review label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed LeaseRevokedMailNotifier and its interface, the RevokeAccessLeaseCommand call site and TryAddScoped registration, the AccessLeaseRevokedView/AccessLeaseRevokedMail pair, the MJML source and both compiled .hbs templates, and the two new test suites. The notifier mirrors the already-reviewed RequesterMailNotifier line for line — FeatureFlagKeys.Pam pre-guard ahead of the organization read, null-organization warning, swallow-and-log boundary so a mail outage cannot fail a revocation that is already written — and the AccessLeaseAction.Revoked gate correctly excludes Cancelled and None, which the command test now pins from both directions. I traced ScheduledEnd specifically: neither the Dapper AccessLease_Revoke procedure nor the EF ExecuteUpdateAsync writes NotAfter, so the in-memory lease still carries the original window when the view is built, and the IsLive precondition keeps that instant in the future as the doc comment claims. Zero-knowledge boundaries hold — only the organization name and scheduled end reach the body, the reason and cipher stay on the request, OrganizationName renders escaped with only the server-built Url triple-braced, and both catch blocks log ids only; the embedded-resource name derived from the view's namespace matches the committed .hbs files, which the Core.Test render specs exercise for real.

Code Review Details

No blocking findings.

PR Metadata Assessment

  • QUESTION: The Screenshots section is empty for a new customer-facing email — a rendered preview of AccessLeaseRevokedView would let reviewers check the copy and layout before it ships.

@maxkpower
maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from e1f25bc to f81d164 Compare August 31, 2026 16:33
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (pam/PM-42817/mail-request-decided@a5906d5). Learn more about missing BASE report.

Additional details and impacted files
@@                         Coverage Diff                          @@
##             pam/PM-42817/mail-request-decided    #8292   +/-   ##
====================================================================
  Coverage                                     ?   64.80%           
====================================================================
  Files                                        ?     2606           
  Lines                                        ?   112087           
  Branches                                     ?    10037           
====================================================================
  Hits                                         ?    72633           
  Misses                                       ?    37034           
  Partials                                     ?     2420           

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

@maxkpower
maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from f81d164 to 917d83d Compare August 31, 2026 20:26
@maxkpower
maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from 917d83d to 18ea011 Compare September 9, 2026 14:11
<tbody>
<tr>
<td align="center" bgcolor="#175ddc" role="presentation" style="border:none;border-radius:20px;cursor:auto;mso-padding-alt:12px 24px;background:#175ddc;" valign="middle">
<a href="{{{Url}}}" style="display:inline-block;background:#175ddc;color:#ffffff;font-family:'Helvetica Neue', Helvetica, Arial, sans-serif;font-size:16px;font-weight:600;line-height:120%;margin:0;text-decoration:none;text-transform:none;padding:12px 24px;mso-padding-alt:0px;border-radius:20px;" target="_blank">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rendering unescaped input in handlebar/mustache template can lead to XSS attacks - high severity
Using {{{ }}} renders an unescaped string into the template. This means the developer is responsible for allowlisting all HTML strings that can be passed in.

Show fix

Remediation: Avoid {{{ }}} usage. Ignore this issue only if the HTML content you feed into it is static. Defend in depth against XSS attacks by using a strict CSP policy. If you must use HTML, sanitize it with a library such as DOMPurify.

Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant