-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Email the lease holder when an operator revokes their access #8292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
maxkpower
wants to merge
7
commits into
pam/PM-42817/mail-request-decided
from
pam/PM-42817/mail-lease-revoked
Draft
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
7f765de
Notify the lease holder by email when an operator revokes their access
304eed3
Simplify the lease-revocation notification
452a467
Drop narrating comments from the mail-lease-revoked changes
94293ec
Point the revocation mail link at the pam route rather than privilegeβ¦
2a0a450
Gate the revocation mail on the existing PAM flag rather than its own
469b5b4
Say revoked rather than ended in the revocation email
a231580
Inherit the shared mail view in the revocation email
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
31 changes: 31 additions & 0 deletions
31
bitwarden_license/src/Services/Pam/Services/ILeaseRevokedMailNotifier.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| ο»Ώusing Bit.Pam.Entities; | ||
| using Bit.Pam.Enums; | ||
|
|
||
| namespace Bit.Services.Pam.Services; | ||
|
|
||
| /// <summary> | ||
| /// Emails a lease holder that an operator ended their active access: the out-of-band twin of the | ||
| /// <c>RefreshAccessRequest</c> push from <see cref="IRequesterNotifier" /> on the same path. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// A courtesy, not a security control. The lease is dead server-side before anything here runs, so nothing this | ||
| /// type does or fails to do changes who holds access. | ||
| /// | ||
| /// Like <see cref="IAccessMailNotifier" />, it never throws: the call sits at the end of the command that ended | ||
| /// the lease, and a mail outage must not fail a revocation that has already been written. | ||
| /// </remarks> | ||
| public interface ILeaseRevokedMailNotifier | ||
| { | ||
| /// <summary> | ||
| /// Tells <paramref name="lease" />'s holder that their access ended, but only when | ||
| /// <paramref name="endAction" /> is <see cref="AccessLeaseAction.Revoked" />. The name is neutral because this | ||
| /// is handed every early end, so the rule that a holder is never mailed about their own action lives here. | ||
| /// </summary> | ||
| /// <param name="lease">The lease just ended. Its <c>Action</c> may not be stamped yet at the call site.</param> | ||
| /// <param name="endAction"> | ||
| /// How it ended, passed rather than read from <paramref name="lease" /> for that reason: | ||
| /// <see cref="AccessLeaseAction.Revoked" /> for an operator, <see cref="AccessLeaseAction.Cancelled" /> for the | ||
| /// holder ending their own access. | ||
| /// </param> | ||
| Task NotifyLeaseEndedAsync(AccessLease lease, AccessLeaseAction endAction); | ||
| } |
75 changes: 75 additions & 0 deletions
75
bitwarden_license/src/Services/Pam/Services/LeaseRevokedMailNotifier.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| ο»Ώusing Bit.Core; | ||
| using Bit.Core.Pam.Models.Mail.AccessLeaseRevoked; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Settings; | ||
| using Bit.Pam.Entities; | ||
| using Bit.Pam.Enums; | ||
| using Bitwarden.Server.Sdk.Features; | ||
|
|
||
| namespace Bit.Services.Pam.Services; | ||
|
|
||
| public class LeaseRevokedMailNotifier : ILeaseRevokedMailNotifier | ||
| { | ||
| private readonly IAccessMailNotifier _accessMailNotifier; | ||
| private readonly IOrganizationRepository _organizationRepository; | ||
| private readonly IGlobalSettings _globalSettings; | ||
| private readonly IFeatureService _featureService; | ||
| private readonly ILogger<LeaseRevokedMailNotifier> _logger; | ||
|
|
||
| public LeaseRevokedMailNotifier( | ||
| IAccessMailNotifier accessMailNotifier, | ||
| IOrganizationRepository organizationRepository, | ||
| IGlobalSettings globalSettings, | ||
| IFeatureService featureService, | ||
| ILogger<LeaseRevokedMailNotifier> logger) | ||
| { | ||
| _accessMailNotifier = accessMailNotifier; | ||
| _organizationRepository = organizationRepository; | ||
| _globalSettings = globalSettings; | ||
| _featureService = featureService; | ||
| _logger = logger; | ||
| } | ||
|
|
||
| public async Task NotifyLeaseEndedAsync(AccessLease lease, AccessLeaseAction endAction) | ||
| { | ||
| if (endAction != AccessLeaseAction.Revoked) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Duplicates the guard inside IAccessMailNotifier to keep the organization read off every revocation in the | ||
| // flag-off state, which is every revocation on self-host. | ||
| if (!_featureService.IsEnabled(FeatureFlagKeys.Pam)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| var organization = await _organizationRepository.GetByIdAsync(lease.OrganizationId); | ||
| if (organization is null) | ||
| { | ||
| _logger.LogWarning( | ||
| "PAM lease-revoked mail for lease {AccessLeaseId}: organization could not be resolved; nothing sent.", | ||
| lease.Id); | ||
| return; | ||
| } | ||
|
|
||
| var view = new AccessLeaseRevokedView | ||
| { | ||
| WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, | ||
| AccessRequestId = lease.AccessRequestId, | ||
| OrganizationName = organization.Name, | ||
| NotAfter = lease.NotAfter, | ||
| }; | ||
|
|
||
| await _accessMailNotifier.SendToUserAsync( | ||
| lease.RequesterId, email => new AccessLeaseRevokedMail { ToEmails = [email], View = view }); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Ids only: not the holder's address, and never the reason the operator gave for revoking. | ||
| _logger.LogError(ex, "PAM lease-revoked mail for lease {AccessLeaseId} could not be sent.", lease.Id); | ||
| } | ||
| } | ||
| } | ||
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
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
149 changes: 149 additions & 0 deletions
149
bitwarden_license/test/Services/Pam.Test/Services/LeaseRevokedMailNotifierTests.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| ο»Ώusing Bit.Core; | ||
| using Bit.Core.AdminConsole.Entities; | ||
| using Bit.Core.Pam.Models.Mail.AccessLeaseRevoked; | ||
| using Bit.Core.Platform.Mail.Mailer; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Settings; | ||
| using Bit.Pam.Entities; | ||
| using Bit.Pam.Enums; | ||
| using Bit.Services.Pam.Services; | ||
| using Bit.Test.Common.AutoFixture; | ||
| using Bit.Test.Common.AutoFixture.Attributes; | ||
| using Bitwarden.Server.Sdk.Features; | ||
| using NSubstitute; | ||
| using NSubstitute.ExceptionExtensions; | ||
| using Xunit; | ||
|
|
||
| namespace Bit.Services.Pam.Test.Services; | ||
|
|
||
| public class LeaseRevokedMailNotifierTests | ||
| { | ||
| private const string _vaultUrl = "https://vault.example.com/#"; | ||
| private const string _organizationName = "Contoso"; | ||
|
|
||
| [Theory, BitAutoData] | ||
| public async Task NotifyLeaseEndedAsync_Revoked_MailsTheHolderWithTheWindowItCutShort(AccessLease lease) | ||
| { | ||
| lease.NotAfter = new DateTime(2026, 9, 1, 17, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| var sutProvider = Setup(); | ||
| SetupOrganization(sutProvider, lease); | ||
| var sent = RecordMail(sutProvider); | ||
|
|
||
| await sutProvider.Sut.NotifyLeaseEndedAsync(lease, AccessLeaseAction.Revoked); | ||
|
|
||
| var (recipientId, mail) = Assert.Single(sent); | ||
| Assert.Equal(lease.RequesterId, recipientId); | ||
| Assert.Equal("Your access was revoked", mail.Subject); | ||
| Assert.Equal(_organizationName, mail.View.OrganizationName); | ||
| Assert.Equal("1 Sep 2026 at 17:00 UTC", mail.View.ScheduledEnd); | ||
| Assert.Equal($"{_vaultUrl}/pam/requests/{lease.AccessRequestId}", mail.View.Url); | ||
| // One message per recipient: the holder is never named alongside anyone else. | ||
| await sutProvider.GetDependency<IAccessMailNotifier>().DidNotReceiveWithAnyArgs() | ||
| .SendToUsersAsync(default!, (Func<string, BaseMail<AccessLeaseRevokedView>>)default!); | ||
| } | ||
|
|
||
| [Theory] | ||
| [BitAutoData(AccessLeaseAction.Cancelled)] | ||
| [BitAutoData(AccessLeaseAction.None)] | ||
| public async Task NotifyLeaseEndedAsync_NotRevoked_ReadsNothingAndSendsNothing( | ||
| AccessLeaseAction endAction, AccessLease lease) | ||
| { | ||
| var sutProvider = Setup(); | ||
| SetupOrganization(sutProvider, lease); | ||
| var sent = RecordMail(sutProvider); | ||
|
|
||
| await sutProvider.Sut.NotifyLeaseEndedAsync(lease, endAction); | ||
|
|
||
| Assert.Empty(sent); | ||
| await sutProvider.GetDependency<IAccessMailNotifier>().DidNotReceiveWithAnyArgs() | ||
| .SendToUserAsync(default, (Func<string, BaseMail<AccessLeaseRevokedView>>)default!); | ||
| await sutProvider.GetDependency<IOrganizationRepository>().DidNotReceiveWithAnyArgs().GetByIdAsync(default); | ||
| } | ||
|
|
||
| [Theory, BitAutoData] | ||
| public async Task NotifyLeaseEndedAsync_FlagOff_ReadsNothingAndSendsNothing(AccessLease lease) | ||
| { | ||
| var sutProvider = Setup(flagOn: false); | ||
|
|
||
| await sutProvider.Sut.NotifyLeaseEndedAsync(lease, AccessLeaseAction.Revoked); | ||
|
|
||
| await sutProvider.GetDependency<IOrganizationRepository>().DidNotReceiveWithAnyArgs().GetByIdAsync(default); | ||
| await sutProvider.GetDependency<IAccessMailNotifier>().DidNotReceiveWithAnyArgs() | ||
| .SendToUserAsync(default, (Func<string, BaseMail<AccessLeaseRevokedView>>)default!); | ||
| } | ||
|
|
||
| [Theory, BitAutoData] | ||
| public async Task NotifyLeaseEndedAsync_UnknownOrganization_SendsNothing(AccessLease lease) | ||
| { | ||
| var sutProvider = Setup(); | ||
| sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(lease.OrganizationId) | ||
| .Returns((Organization?)null); | ||
| var sent = RecordMail(sutProvider); | ||
|
|
||
| await sutProvider.Sut.NotifyLeaseEndedAsync(lease, AccessLeaseAction.Revoked); | ||
|
|
||
| Assert.Empty(sent); | ||
| } | ||
|
|
||
| [Theory, BitAutoData] | ||
| public async Task NotifyLeaseEndedAsync_SendFails_DoesNotPropagate(AccessLease lease) | ||
| { | ||
| var sutProvider = Setup(); | ||
| SetupOrganization(sutProvider, lease); | ||
| sutProvider.GetDependency<IAccessMailNotifier>() | ||
| .SendToUserAsync(Arg.Any<Guid>(), Arg.Any<Func<string, BaseMail<AccessLeaseRevokedView>>>()) | ||
| .ThrowsAsync(new InvalidOperationException("delivery service unavailable")); | ||
|
|
||
| var exception = await Record.ExceptionAsync( | ||
| () => sutProvider.Sut.NotifyLeaseEndedAsync(lease, AccessLeaseAction.Revoked)); | ||
|
|
||
| Assert.Null(exception); | ||
| } | ||
|
|
||
| [Theory, BitAutoData] | ||
| public async Task NotifyLeaseEndedAsync_OrganizationReadFails_DoesNotPropagate(AccessLease lease) | ||
| { | ||
| var sutProvider = Setup(); | ||
| sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(lease.OrganizationId) | ||
| .ThrowsAsync(new TimeoutException("database unavailable")); | ||
|
|
||
| var exception = await Record.ExceptionAsync( | ||
| () => sutProvider.Sut.NotifyLeaseEndedAsync(lease, AccessLeaseAction.Revoked)); | ||
|
|
||
| Assert.Null(exception); | ||
| } | ||
|
|
||
| private static SutProvider<LeaseRevokedMailNotifier> Setup(bool flagOn = true) | ||
| { | ||
| var sutProvider = new SutProvider<LeaseRevokedMailNotifier>().Create(); | ||
|
|
||
| sutProvider.GetDependency<IFeatureService>() | ||
| .IsEnabled(FeatureFlagKeys.Pam) | ||
| .Returns(flagOn); | ||
| sutProvider.GetDependency<IGlobalSettings>().BaseServiceUri.VaultWithHash.Returns(_vaultUrl); | ||
|
|
||
| return sutProvider; | ||
| } | ||
|
|
||
| private static void SetupOrganization(SutProvider<LeaseRevokedMailNotifier> sutProvider, AccessLease lease) => | ||
| sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(lease.OrganizationId) | ||
| .Returns(new Organization { Id = lease.OrganizationId, Name = _organizationName }); | ||
|
|
||
| private static List<(Guid RecipientId, AccessLeaseRevokedMail Mail)> RecordMail( | ||
| SutProvider<LeaseRevokedMailNotifier> sutProvider) | ||
| { | ||
| List<(Guid RecipientId, AccessLeaseRevokedMail Mail)> sent = []; | ||
|
|
||
| sutProvider.GetDependency<IAccessMailNotifier>() | ||
| .When(x => x.SendToUserAsync( | ||
| Arg.Any<Guid>(), | ||
| Arg.Any<Func<string, BaseMail<AccessLeaseRevokedView>>>())) | ||
| .Do(call => sent.Add(( | ||
| call.Arg<Guid>(), | ||
| (AccessLeaseRevokedMail)call.Arg<Func<string, BaseMail<AccessLeaseRevokedView>>>()( | ||
| "holder@example.com")))); | ||
|
|
||
| return sent; | ||
| } | ||
| } |
44 changes: 44 additions & 0 deletions
44
src/Core/MailTemplates/Mjml/emails/Pam/AccessLeaseRevokedView.mjml
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| <mjml> | ||
| <mj-head> | ||
| <mj-include path="../../components/head.mjml" /> | ||
| </mj-head> | ||
| <mj-body> | ||
| <!-- Blue Header Section --> | ||
| <mj-wrapper css-class="border-fix" padding="20px 24px 0px 24px"> | ||
| <mj-bw-simple-hero /> | ||
| </mj-wrapper> | ||
|
|
||
| <!-- Main Content --> | ||
| <mj-wrapper padding="0px 25px"> | ||
| <mj-section background-color="#fff" padding="24px 0px"> | ||
| <mj-column padding="0px 25px"> | ||
| <mj-text padding="0px 0px 16px 0px" font-weight="600" font-size="18px" line-height="28px"> | ||
| Your access was revoked | ||
| </mj-text> | ||
| <mj-text padding="0px 0px 24px 0px" font-weight="400" line-height="24px"> | ||
| Someone who manages this collection revoked your access in <b>{{OrganizationName}}</b>, which was due to | ||
| run until {{ScheduledEnd}}. The item is locked again, and this access cannot be resumed. | ||
| </mj-text> | ||
| <mj-button | ||
| padding="0px 0px 24px 0px" | ||
| inner-padding="12px 24px" | ||
| border-radius="20px" | ||
| font-weight="600" | ||
| align="left" | ||
| href="{{{Url}}}" | ||
| >View the request</mj-button | ||
| > | ||
| <mj-text padding="0px" font-size="12px" font-weight="400" line-height="24px"> | ||
| If you still need access, ask for it again with a new request. Any reason given for revoking this one is | ||
| on the request, along with the item it covered; neither is included in this email. | ||
| </mj-text> | ||
| </mj-column> | ||
| </mj-section> | ||
| </mj-wrapper> | ||
|
|
||
| <!-- Footer --> | ||
| <mj-wrapper padding-top="15px"> | ||
| <mj-include path="../../components/footer.mjml" /> | ||
| </mj-wrapper> | ||
| </mj-body> | ||
| </mjml> |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs null checks, such as
_foo = foo ?? throw new ArgumentNullException(nameof(foo)).