From b00f11fc43c0d5f20a8e224a8f9e3fc4dc23e60e Mon Sep 17 00:00:00 2001 From: Ike <137194738+ike-kottlowski@users.noreply.github.com> Date: Fri, 21 Feb 2025 11:12:31 -0500 Subject: [PATCH] [PM-17645] : update email for new email multi factor tokens (#5428) * feat(newDeviceVerification) : Initial update to email * fix : email copying over extra whitespace when using keyboard short cuts * test : Fixing tests for new device verificaiton email format --- .../Auth/Controllers/TwoFactorController.cs | 7 ++- .../Handlebars/Auth/TwoFactorEmail.html.hbs | 46 ++++++++++---- .../Handlebars/Auth/TwoFactorEmail.text.hbs | 15 ++++- .../Mail/TwoFactorEmailTokenViewModel.cs | 25 ++++++++ ...=> UserVerificationEmailTokenViewModel.cs} | 2 +- src/Core/Services/IMailService.cs | 2 +- src/Core/Services/IUserService.cs | 16 ++++- .../Implementations/HandlebarsMailService.cs | 20 ++++-- .../Services/Implementations/UserService.cs | 29 +++++++-- .../NoopImplementations/NoopMailService.cs | 2 +- .../RequestValidators/DeviceValidator.cs | 10 ++- test/Core.Test/Services/UserServiceTests.cs | 62 +++++++++++++++++-- .../Endpoints/IdentityServerTwoFactorTests.cs | 14 ++++- .../IdentityServer/DeviceValidatorTests.cs | 2 +- 14 files changed, 214 insertions(+), 38 deletions(-) create mode 100644 src/Core/Models/Mail/TwoFactorEmailTokenViewModel.cs rename src/Core/Models/Mail/{EmailTokenViewModel.cs => UserVerificationEmailTokenViewModel.cs} (54%) diff --git a/src/Api/Auth/Controllers/TwoFactorController.cs b/src/Api/Auth/Controllers/TwoFactorController.cs index c7d39f64b0..83490f1c2f 100644 --- a/src/Api/Auth/Controllers/TwoFactorController.cs +++ b/src/Api/Auth/Controllers/TwoFactorController.cs @@ -288,12 +288,17 @@ public class TwoFactorController : Controller return response; } + /// + /// This endpoint is only used to set-up email two factor authentication. + /// + /// secret verification model + /// void [HttpPost("send-email")] public async Task SendEmail([FromBody] TwoFactorEmailRequestModel model) { var user = await CheckAsync(model, false, true); model.ToUser(user); - await _userService.SendTwoFactorEmailAsync(user); + await _userService.SendTwoFactorEmailAsync(user, false); } [AllowAnonymous] diff --git a/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.html.hbs b/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.html.hbs index be51c4e9f3..27a222f1de 100644 --- a/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.html.hbs +++ b/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.html.hbs @@ -1,14 +1,38 @@ {{#>FullHtmlLayout}} - - - - - - + + + + + + + + +
- Your two-step verification code is: {{Token}} -
- Use this code to complete logging in with Bitwarden. -
+ To finish {{EmailTotpAction}}, enter this verification code: {{Token}} +
+
+ If this was not you, take these immediate steps to secure your account in the web app: +
    +
  • Deauthorize unrecognized devices
    • +
  • Change your master password
    • +
  • Turn on two-step login
    • +
+
+
+
+
+ Account: + {{AccountEmail}} +
+ Date: + {{TheDate}} at {{TheTime}} {{TimeZone}} +
+ IP: + {{DeviceIp}} +
+ DeviceType: + {{DeviceType}} +
-{{/FullHtmlLayout}} +{{/FullHtmlLayout}} \ No newline at end of file diff --git a/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.text.hbs b/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.text.hbs index c7e64e5da2..211a870d6a 100644 --- a/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.text.hbs +++ b/src/Core/MailTemplates/Handlebars/Auth/TwoFactorEmail.text.hbs @@ -1,5 +1,16 @@ {{#>BasicTextLayout}} -Your two-step verification code is: {{Token}} +To finish {{EmailTotpAction}}, enter this verification code: {{Token}} -Use this code to complete logging in with Bitwarden. +If this was not you, take these immediate steps to secure your account in the web app: + +Deauthorize unrecognized devices + +Change your master password + +Turn on two-step login + +Account : {{AccountEmail}} +Date : {{TheDate}} at {{TheTime}} {{TimeZone}} +IP : {{DeviceIp}} +Device Type : {{DeviceType}} {{/BasicTextLayout}} \ No newline at end of file diff --git a/src/Core/Models/Mail/TwoFactorEmailTokenViewModel.cs b/src/Core/Models/Mail/TwoFactorEmailTokenViewModel.cs new file mode 100644 index 0000000000..dbd47af35a --- /dev/null +++ b/src/Core/Models/Mail/TwoFactorEmailTokenViewModel.cs @@ -0,0 +1,25 @@ +namespace Bit.Core.Models.Mail; + +/// +/// This view model is used to set-up email two factor authentication, to log in with email two factor authentication, +/// and for new device verification. +/// +public class TwoFactorEmailTokenViewModel : BaseMailModel +{ + public string Token { get; set; } + /// + /// This view model is used to also set-up email two factor authentication. We use this property to communicate + /// the purpose of the email, since it can be used for logging in and for setting up. + /// + public string EmailTotpAction { get; set; } + /// + /// When logging in with email two factor the account email may not be the same as the email used for two factor. + /// we want to show the account email in the email, so the user knows which account they are logging into. + /// + public string AccountEmail { get; set; } + public string TheDate { get; set; } + public string TheTime { get; set; } + public string TimeZone { get; set; } + public string DeviceIp { get; set; } + public string DeviceType { get; set; } +} diff --git a/src/Core/Models/Mail/EmailTokenViewModel.cs b/src/Core/Models/Mail/UserVerificationEmailTokenViewModel.cs similarity index 54% rename from src/Core/Models/Mail/EmailTokenViewModel.cs rename to src/Core/Models/Mail/UserVerificationEmailTokenViewModel.cs index 561df580e8..b8850b5f00 100644 --- a/src/Core/Models/Mail/EmailTokenViewModel.cs +++ b/src/Core/Models/Mail/UserVerificationEmailTokenViewModel.cs @@ -1,6 +1,6 @@ namespace Bit.Core.Models.Mail; -public class EmailTokenViewModel : BaseMailModel +public class UserVerificationEmailTokenViewModel : BaseMailModel { public string Token { get; set; } } diff --git a/src/Core/Services/IMailService.cs b/src/Core/Services/IMailService.cs index 92d05ddb7d..3492ada838 100644 --- a/src/Core/Services/IMailService.cs +++ b/src/Core/Services/IMailService.cs @@ -23,7 +23,7 @@ public interface IMailService Task SendCannotDeleteManagedAccountEmailAsync(string email); Task SendChangeEmailAlreadyExistsEmailAsync(string fromEmail, string toEmail); Task SendChangeEmailEmailAsync(string newEmailAddress, string token); - Task SendTwoFactorEmailAsync(string email, string token); + Task SendTwoFactorEmailAsync(string email, string accountEmail, string token, string deviceIp, string deviceType, bool authentication = true); Task SendNoMasterPasswordHintEmailAsync(string email); Task SendMasterPasswordHintEmailAsync(string email, string hint); diff --git a/src/Core/Services/IUserService.cs b/src/Core/Services/IUserService.cs index d1c61e4418..2ac7796547 100644 --- a/src/Core/Services/IUserService.cs +++ b/src/Core/Services/IUserService.cs @@ -21,7 +21,21 @@ public interface IUserService Task CreateUserAsync(User user); Task CreateUserAsync(User user, string masterPasswordHash); Task SendMasterPasswordHintAsync(string email); - Task SendTwoFactorEmailAsync(User user); + /// + /// Used for both email two factor and email two factor setup. + /// + /// user requesting the action + /// this controls if what verbiage is shown in the email + /// void + Task SendTwoFactorEmailAsync(User user, bool authentication = true); + /// + /// Calls the same email implementation but instead it sends the token to the account email not the + /// email set up for two-factor, since in practice they can be different. + /// + /// user attepting to login with a new device + /// void + Task SendNewDeviceVerificationEmailAsync(User user); + Task VerifyTwoFactorEmailAsync(User user, string token); Task StartWebAuthnRegistrationAsync(User user); Task DeleteWebAuthnKeyAsync(User user, int id); Task CompleteWebAuthRegistrationAsync(User user, int value, string name, AuthenticatorAttestationRawResponse attestationResponse); diff --git a/src/Core/Services/Implementations/HandlebarsMailService.cs b/src/Core/Services/Implementations/HandlebarsMailService.cs index d18a29b13a..44be3bfdf4 100644 --- a/src/Core/Services/Implementations/HandlebarsMailService.cs +++ b/src/Core/Services/Implementations/HandlebarsMailService.cs @@ -146,7 +146,7 @@ public class HandlebarsMailService : IMailService public async Task SendChangeEmailEmailAsync(string newEmailAddress, string token) { var message = CreateDefaultMessage("Your Email Change", newEmailAddress); - var model = new EmailTokenViewModel + var model = new UserVerificationEmailTokenViewModel { Token = token, WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, @@ -158,14 +158,22 @@ public class HandlebarsMailService : IMailService await _mailDeliveryService.SendEmailAsync(message); } - public async Task SendTwoFactorEmailAsync(string email, string token) + public async Task SendTwoFactorEmailAsync(string email, string accountEmail, string token, string deviceIp, string deviceType, bool authentication = true) { - var message = CreateDefaultMessage("Your Two-step Login Verification Code", email); - var model = new EmailTokenViewModel + var message = CreateDefaultMessage("Your Bitwarden Verification Code", email); + var requestDateTime = DateTime.UtcNow; + var model = new TwoFactorEmailTokenViewModel { Token = token, + EmailTotpAction = authentication ? "logging in" : "setting up two-step login", + AccountEmail = accountEmail, + TheDate = requestDateTime.ToLongDateString(), + TheTime = requestDateTime.ToShortTimeString(), + TimeZone = _utcTimeZoneDisplay, + DeviceIp = deviceIp, + DeviceType = deviceType, WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, - SiteName = _globalSettings.SiteName + SiteName = _globalSettings.SiteName, }; await AddMessageContentAsync(message, "Auth.TwoFactorEmail", model); message.MetaData.Add("SendGridBypassListManagement", true); @@ -1012,7 +1020,7 @@ public class HandlebarsMailService : IMailService public async Task SendOTPEmailAsync(string email, string token) { var message = CreateDefaultMessage("Your Bitwarden Verification Code", email); - var model = new EmailTokenViewModel + var model = new UserVerificationEmailTokenViewModel { Token = token, WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash, diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index 47637d0f75..2374d8f4e1 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -1,4 +1,6 @@ -using System.Security.Claims; +using System.ComponentModel.DataAnnotations; +using System.Reflection; +using System.Security.Claims; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data; @@ -350,7 +352,7 @@ public class UserService : UserManager, IUserService, IDisposable await _mailService.SendMasterPasswordHintEmailAsync(email, user.MasterPasswordHint); } - public async Task SendTwoFactorEmailAsync(User user) + public async Task SendTwoFactorEmailAsync(User user, bool authentication = true) { var provider = user.GetTwoFactorProvider(TwoFactorProviderType.Email); if (provider == null || provider.MetaData == null || !provider.MetaData.ContainsKey("Email")) @@ -361,7 +363,26 @@ public class UserService : UserManager, IUserService, IDisposable var email = ((string)provider.MetaData["Email"]).ToLowerInvariant(); var token = await base.GenerateTwoFactorTokenAsync(user, CoreHelpers.CustomProviderName(TwoFactorProviderType.Email)); - await _mailService.SendTwoFactorEmailAsync(email, token); + + var deviceType = _currentContext.DeviceType?.GetType().GetMember(_currentContext.DeviceType?.ToString()) + .FirstOrDefault()?.GetCustomAttribute()?.GetName() ?? "Unknown Browser"; + + await _mailService.SendTwoFactorEmailAsync( + email, user.Email, token, _currentContext.IpAddress, deviceType, authentication); + } + + public async Task SendNewDeviceVerificationEmailAsync(User user) + { + ArgumentNullException.ThrowIfNull(user); + + var token = await base.GenerateUserTokenAsync(user, TokenOptions.DefaultEmailProvider, + "otp:" + user.Email); + + var deviceType = _currentContext.DeviceType?.GetType().GetMember(_currentContext.DeviceType?.ToString()) + .FirstOrDefault()?.GetCustomAttribute()?.GetName() ?? "Unknown Browser"; + + await _mailService.SendTwoFactorEmailAsync( + user.Email, user.Email, token, _currentContext.IpAddress, deviceType); } public async Task VerifyTwoFactorEmailAsync(User user, string token) @@ -1519,7 +1540,7 @@ public class UserService : UserManager, IUserService, IDisposable if (await VerifySecretAsync(user, secret)) { - await SendOTPAsync(user); + await SendNewDeviceVerificationEmailAsync(user); } } diff --git a/src/Core/Services/NoopImplementations/NoopMailService.cs b/src/Core/Services/NoopImplementations/NoopMailService.cs index d6b330294d..9984f8ee90 100644 --- a/src/Core/Services/NoopImplementations/NoopMailService.cs +++ b/src/Core/Services/NoopImplementations/NoopMailService.cs @@ -87,7 +87,7 @@ public class NoopMailService : IMailService public Task SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(string organizationName, string email) => Task.CompletedTask; - public Task SendTwoFactorEmailAsync(string email, string token) + public Task SendTwoFactorEmailAsync(string email, string accountEmail, string token, string deviceIp, string deviceType, bool authentication = true) { return Task.FromResult(0); } diff --git a/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs b/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs index fee10e10ff..17d16f5949 100644 --- a/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs @@ -79,7 +79,7 @@ public class DeviceValidator( BuildDeviceErrorResult(validationResult); if (validationResult == DeviceValidationResultType.NewDeviceVerificationRequired) { - await _userService.SendOTPAsync(context.User); + await _userService.SendNewDeviceVerificationEmailAsync(context.User); } return false; } @@ -163,6 +163,14 @@ public class DeviceValidator( return DeviceValidationResultType.NewDeviceVerificationRequired; } + /// + /// Sends an email whenever the user logs in from a new device. Will not send to a user who's account + /// is less than 10 minutes old. We assume an account that is less than 10 minutes old is new and does + /// not need an email stating they just logged in. + /// + /// user logging in + /// current device being approved to login + /// void private async Task SendNewDeviceLoginEmail(User user, Device requestDevice) { // Ensure that the user doesn't receive a "new device" email on the first login diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 7f1dd37b6b..3158c1595c 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -96,6 +96,9 @@ public class UserServiceTests { var email = user.Email.ToLowerInvariant(); var token = "thisisatokentocompare"; + var authentication = true; + var IpAddress = "1.1.1.1"; + var deviceType = "Android"; var userTwoFactorTokenProvider = Substitute.For>(); userTwoFactorTokenProvider @@ -105,6 +108,10 @@ public class UserServiceTests .GenerateAsync("TwoFactor", Arg.Any>(), user) .Returns(Task.FromResult(token)); + var context = sutProvider.GetDependency(); + context.DeviceType = DeviceType.Android; + context.IpAddress = IpAddress; + sutProvider.Sut.RegisterTokenProvider("Custom_Email", userTwoFactorTokenProvider); user.SetTwoFactorProviders(new Dictionary @@ -119,7 +126,7 @@ public class UserServiceTests await sutProvider.GetDependency() .Received(1) - .SendTwoFactorEmailAsync(email, token); + .SendTwoFactorEmailAsync(email, user.Email, token, IpAddress, deviceType, authentication); } [Theory, BitAutoData] @@ -160,6 +167,44 @@ public class UserServiceTests await Assert.ThrowsAsync("No email.", () => sutProvider.Sut.SendTwoFactorEmailAsync(user)); } + [Theory, BitAutoData] + public async Task SendNewDeviceVerificationEmailAsync_ExceptionBecauseUserNull(SutProvider sutProvider) + { + await Assert.ThrowsAsync(() => sutProvider.Sut.SendNewDeviceVerificationEmailAsync(null)); + } + + [Theory] + [BitAutoData(DeviceType.UnknownBrowser, "Unknown Browser")] + [BitAutoData(DeviceType.Android, "Android")] + public async Task SendNewDeviceVerificationEmailAsync_DeviceMatches(DeviceType deviceType, string deviceTypeName, SutProvider sutProvider, User user) + { + SetupFakeTokenProvider(sutProvider, user); + var context = sutProvider.GetDependency(); + context.DeviceType = deviceType; + context.IpAddress = "1.1.1.1"; + + await sutProvider.Sut.SendNewDeviceVerificationEmailAsync(user); + + await sutProvider.GetDependency() + .Received(1) + .SendTwoFactorEmailAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), deviceTypeName, Arg.Any()); + } + + [Theory, BitAutoData] + public async Task SendNewDeviceVerificationEmailAsync_NullDeviceTypeShouldSendUnkownBrowserType(SutProvider sutProvider, User user) + { + SetupFakeTokenProvider(sutProvider, user); + var context = sutProvider.GetDependency(); + context.DeviceType = null; + context.IpAddress = "1.1.1.1"; + + await sutProvider.Sut.SendNewDeviceVerificationEmailAsync(user); + + await sutProvider.GetDependency() + .Received(1) + .SendTwoFactorEmailAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), "Unknown Browser", Arg.Any()); + } + [Theory, BitAutoData] public async Task HasPremiumFromOrganization_Returns_False_If_No_Orgs(SutProvider sutProvider, User user) { @@ -577,7 +622,7 @@ public class UserServiceTests } [Theory, BitAutoData] - public async Task ResendNewDeviceVerificationEmail_UserNull_SendOTPAsyncNotCalled( + public async Task ResendNewDeviceVerificationEmail_UserNull_SendTwoFactorEmailAsyncNotCalled( SutProvider sutProvider, string email, string secret) { sutProvider.GetDependency() @@ -588,11 +633,11 @@ public class UserServiceTests await sutProvider.GetDependency() .DidNotReceive() - .SendOTPEmailAsync(Arg.Any(), Arg.Any()); + .SendTwoFactorEmailAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } [Theory, BitAutoData] - public async Task ResendNewDeviceVerificationEmail_SecretNotValid_SendOTPAsyncNotCalled( + public async Task ResendNewDeviceVerificationEmail_SecretNotValid_SendTwoFactorEmailAsyncNotCalled( SutProvider sutProvider, string email, string secret) { sutProvider.GetDependency() @@ -603,7 +648,7 @@ public class UserServiceTests await sutProvider.GetDependency() .DidNotReceive() - .SendOTPEmailAsync(Arg.Any(), Arg.Any()); + .SendTwoFactorEmailAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } [Theory, BitAutoData] @@ -637,6 +682,10 @@ public class UserServiceTests .GetByEmailAsync(user.Email) .Returns(user); + var context = sutProvider.GetDependency(); + context.DeviceType = DeviceType.Android; + context.IpAddress = "1.1.1.1"; + // HACK: SutProvider is being weird about not injecting the IPasswordHasher that I configured var sut = RebuildSut(sutProvider); @@ -644,7 +693,8 @@ public class UserServiceTests await sutProvider.GetDependency() .Received(1) - .SendOTPEmailAsync(user.Email, Arg.Any()); + .SendTwoFactorEmailAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } [Theory] diff --git a/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs b/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs index 4e598c436d..289f321512 100644 --- a/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs @@ -67,7 +67,12 @@ public class IdentityServerTwoFactorTests : IClassFixture(mailService => { - mailService.SendTwoFactorEmailAsync(Arg.Any(), Arg.Do(t => emailToken = t)) + mailService.SendTwoFactorEmailAsync( + Arg.Any(), + Arg.Any(), + Arg.Do(t => emailToken = t), + Arg.Any(), + Arg.Any()) .Returns(Task.CompletedTask); }); @@ -273,7 +278,12 @@ public class IdentityServerTwoFactorTests : IClassFixture(mailService => { - mailService.SendTwoFactorEmailAsync(Arg.Any(), Arg.Do(t => emailToken = t)) + mailService.SendTwoFactorEmailAsync( + Arg.Any(), + Arg.Any(), + Arg.Do(t => emailToken = t), + Arg.Any(), + Arg.Any()) .Returns(Task.CompletedTask); }); diff --git a/test/Identity.Test/IdentityServer/DeviceValidatorTests.cs b/test/Identity.Test/IdentityServer/DeviceValidatorTests.cs index 6e6406f16b..fddcf2005d 100644 --- a/test/Identity.Test/IdentityServer/DeviceValidatorTests.cs +++ b/test/Identity.Test/IdentityServer/DeviceValidatorTests.cs @@ -574,7 +574,7 @@ public class DeviceValidatorTests var result = await _sut.ValidateRequestDeviceAsync(request, context); // Assert - await _userService.Received(1).SendOTPAsync(context.User); + await _userService.Received(1).SendNewDeviceVerificationEmailAsync(context.User); await _deviceService.Received(0).SaveAsync(Arg.Any()); Assert.False(result);