Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Core/Services/Implementations/UserService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,10 @@
user.Email = newEmail;
user.EmailVerified = true;
user.RevisionDate = user.AccountRevisionDate = now;

// We need this to backfill the salt for now to keep the email and salt always in sync.
user.MasterPasswordSalt = newEmail;

user.LastEmailChangeDate = now;
await _userRepository.ReplaceAsync(user);

Expand All @@ -431,6 +435,10 @@
//if sync to strip fails, update email and securityStamp to previous
user.Key = previousState.Key;
user.Email = previousState.Email;

// We need this to backfill the salt for now to keep the email and salt always in sync.
user.MasterPasswordSalt = previousState.Email;

user.RevisionDate = user.AccountRevisionDate = DateTime.UtcNow;
user.MasterPassword = previousState.MasterPassword;
user.SecurityStamp = previousState.SecurityStamp;
Expand Down Expand Up @@ -732,7 +740,7 @@
{
if (!CoreHelpers.FixedTimeEquals(
user.TwoFactorRecoveryCode,
recoveryCode.Replace(" ", string.Empty).Trim().ToLower()))

Check warning on line 743 in src/Core/Services/Implementations/UserService.cs

View workflow job for this annotation

GitHub Actions / Build MSSQL migrator utility (win-x64)

The behavior of 'string.ToLower()' could vary based on the current user's locale settings. Replace this call in 'UserService.RecoverTwoFactorAsync(User, string)' with a call to 'string.ToLower(CultureInfo)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1304)

Check warning on line 743 in src/Core/Services/Implementations/UserService.cs

View workflow job for this annotation

GitHub Actions / Build Docker images (Identity, ./src, true)

The behavior of 'string.ToLower()' could vary based on the current user's locale settings. Replace this call in 'UserService.RecoverTwoFactorAsync(User, string)' with a call to 'string.ToLower(CultureInfo)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1304)

Check warning on line 743 in src/Core/Services/Implementations/UserService.cs

View workflow job for this annotation

GitHub Actions / Build Docker images (SeederApi, ./util, linux/amd64,linux/arm64, true)

The behavior of 'string.ToLower()' could vary based on the current user's locale settings. Replace this call in 'UserService.RecoverTwoFactorAsync(User, string)' with a call to 'string.ToLower(CultureInfo)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1304)
{
return false;
}
Expand Down
42 changes: 42 additions & 0 deletions test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Bit.Core.Auth.Enums;
using Bit.Core.Auth.Models.Data;
using Bit.Core.Auth.Repositories;
using Bit.Core.Billing.Services;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.KeyManagement.Models.Api.Request;
Expand Down Expand Up @@ -51,6 +52,7 @@ public class AccountsControllerTest : IClassFixture<ApiApplicationFactory>, IAsy
private readonly IUserSignatureKeyPairRepository _userSignatureKeyPairRepository;
private readonly IEventRepository _eventRepository;
private readonly IOrganizationUserRepository _organizationUserRepository;
private readonly IStripeSyncService _stripeSyncService;

private string _ownerEmail = null!;

Expand All @@ -59,6 +61,7 @@ public AccountsControllerTest(ApiApplicationFactory factory)
_factory = factory;
_factory.SubstituteService<IPushNotificationService>(_ => { });
_factory.SubstituteService<IFeatureService>(_ => { });
_factory.SubstituteService<IStripeSyncService>(_ => { });
_client = factory.CreateClient();
_loginHelper = new LoginHelper(_factory, _client);
_userRepository = _factory.GetService<IUserRepository>();
Expand All @@ -70,6 +73,7 @@ public AccountsControllerTest(ApiApplicationFactory factory)
_userSignatureKeyPairRepository = _factory.GetService<IUserSignatureKeyPairRepository>();
_eventRepository = _factory.GetService<IEventRepository>();
_organizationUserRepository = _factory.GetService<IOrganizationUserRepository>();
_stripeSyncService = _factory.GetService<IStripeSyncService>();
}

public async Task InitializeAsync()
Expand Down Expand Up @@ -949,6 +953,7 @@ public async Task PostEmail_Success_UpdatesEmailAndPassword()
Assert.Equal(_masterKeyWrappedUserKey, updatedUser.Key);
Assert.Equal(PasswordVerificationResult.Success,
_passwordHasher.VerifyHashedPassword(updatedUser, updatedUser.MasterPassword!, _newMasterPasswordHash));
Assert.Equal(newEmail, updatedUser.MasterPasswordSalt);
}

[Fact]
Expand Down Expand Up @@ -986,6 +991,43 @@ public async Task PostEmail_WhenInvalidMasterPassword_ReturnsBadRequest()
Assert.NotNull(unchangedUser);
}

[Fact]
public async Task PostEmail_WhenStripeSyncFails_MasterPasswordSaltIsRolledBack()
{
// Arrange
var newEmail = $"new-email-{Guid.NewGuid()}@bitwarden.com";
await _loginHelper.LoginAsync(_ownerEmail);

var user = await _userRepository.GetByEmailAsync(_ownerEmail);
Assert.NotNull(user);

// Set up the user as a Stripe customer to exercise the sync code path in ChangeEmailAsync
user.Gateway = GatewayType.Stripe;
user.GatewayCustomerId = "cus_test_stripe_fail";
await _userRepository.ReplaceAsync(user);

// Configure the substitute to simulate a Stripe sync failure after the DB write
_stripeSyncService
.UpdateCustomerEmailAddressAsync(Arg.Any<string>(), Arg.Any<string>())
.Returns(Task.FromException(new Exception("Stripe sync failure")));

var userManager = _factory.GetService<UserManager<User>>();
var token = await userManager.GenerateChangeEmailTokenAsync(user, newEmail);

// Act
var response = await PostEmailAsync(newEmail, token);

// Assert - Stripe failure is surfaced as a bad request
Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);

// Verify MasterPasswordSalt was rolled back to the original email, not left at newEmail.
// ChangeEmailAsync sets MasterPasswordSalt = newEmail and persists before attempting Stripe
// sync; on failure it must re-persist MasterPasswordSalt = previousState.Email.
var unchangedUser = await _userRepository.GetByEmailAsync(_ownerEmail);
Assert.NotNull(unchangedUser);
Assert.Equal(_ownerEmail, unchangedUser.MasterPasswordSalt);
}

private async Task<HttpResponseMessage> PostEmailAsync(string newEmail, string token)
{
var requestModel = new EmailRequestModel
Expand Down
Loading