From 06c96a96c543b70045c2e96423859588c1db37ce Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Thu, 20 Feb 2025 15:38:59 -0500 Subject: [PATCH] [PM-17449] Add logic to handle email updates for managed users. (#5422) --- .../Auth/Controllers/AccountsController.cs | 12 ---- .../Services/Implementations/UserService.cs | 35 +++++++++++ .../Controllers/AccountsControllerTest.cs | 59 +------------------ .../Controllers/AccountsControllerTests.cs | 30 ---------- test/Core.Test/Services/UserServiceTests.cs | 2 + 5 files changed, 38 insertions(+), 100 deletions(-) diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index 1cd9292386..f3af49e6c3 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -149,12 +149,6 @@ public class AccountsController : Controller throw new BadRequestException("MasterPasswordHash", "Invalid password."); } - // If Account Deprovisioning is enabled, we need to check if the user is managed by any organization. - if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) - && await _userService.IsManagedByAnyOrganizationAsync(user.Id)) - { - throw new BadRequestException("Cannot change emails for accounts owned by an organization. Contact your organization administrator for additional details."); - } await _userService.InitiateEmailChangeAsync(user, model.NewEmail); } @@ -173,12 +167,6 @@ public class AccountsController : Controller throw new BadRequestException("You cannot change your email when using Key Connector."); } - // If Account Deprovisioning is enabled, we need to check if the user is managed by any organization. - if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) - && await _userService.IsManagedByAnyOrganizationAsync(user.Id)) - { - throw new BadRequestException("Cannot change emails for accounts owned by an organization. Contact your organization administrator for additional details."); - } var result = await _userService.ChangeEmailAsync(user, model.MasterPasswordHash, model.NewEmail, model.NewMasterPasswordHash, model.Token, model.Key); diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index e04290a686..47637d0f75 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -49,6 +49,7 @@ public class UserService : UserManager, IUserService, IDisposable private readonly ICipherRepository _cipherRepository; private readonly IOrganizationUserRepository _organizationUserRepository; private readonly IOrganizationRepository _organizationRepository; + private readonly IOrganizationDomainRepository _organizationDomainRepository; private readonly IMailService _mailService; private readonly IPushNotificationService _pushService; private readonly IdentityErrorDescriber _identityErrorDescriber; @@ -81,6 +82,7 @@ public class UserService : UserManager, IUserService, IDisposable ICipherRepository cipherRepository, IOrganizationUserRepository organizationUserRepository, IOrganizationRepository organizationRepository, + IOrganizationDomainRepository organizationDomainRepository, IMailService mailService, IPushNotificationService pushService, IUserStore store, @@ -127,6 +129,7 @@ public class UserService : UserManager, IUserService, IDisposable _cipherRepository = cipherRepository; _organizationUserRepository = organizationUserRepository; _organizationRepository = organizationRepository; + _organizationDomainRepository = organizationDomainRepository; _mailService = mailService; _pushService = pushService; _identityOptions = optionsAccessor?.Value ?? new IdentityOptions(); @@ -521,6 +524,13 @@ public class UserService : UserManager, IUserService, IDisposable return IdentityResult.Failed(_identityErrorDescriber.PasswordMismatch()); } + var managedUserValidationResult = await ValidateManagedUserAsync(user, newEmail); + + if (!managedUserValidationResult.Succeeded) + { + return managedUserValidationResult; + } + if (!await base.VerifyUserTokenAsync(user, _identityOptions.Tokens.ChangeEmailTokenProvider, GetChangeEmailTokenPurpose(newEmail), token)) { @@ -586,6 +596,31 @@ public class UserService : UserManager, IUserService, IDisposable return IdentityResult.Success; } + private async Task ValidateManagedUserAsync(User user, string newEmail) + { + var managingOrganizations = await GetOrganizationsManagingUserAsync(user.Id); + + if (!managingOrganizations.Any()) + { + return IdentityResult.Success; + } + + var newDomain = CoreHelpers.GetEmailDomain(newEmail); + + var verifiedDomains = await _organizationDomainRepository.GetVerifiedDomainsByOrganizationIdsAsync(managingOrganizations.Select(org => org.Id)); + + if (verifiedDomains.Any(verifiedDomain => verifiedDomain.DomainName == newDomain)) + { + return IdentityResult.Success; + } + + return IdentityResult.Failed(new IdentityError + { + Code = "EmailDomainMismatch", + Description = "Your new email must match your organization domain." + }); + } + public async Task ChangePasswordAsync(User user, string masterPassword, string newMasterPassword, string passwordHint, string key) { diff --git a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs index 6dd7f42c63..277f558566 100644 --- a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs +++ b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs @@ -1,6 +1,4 @@ -using System.Net; -using System.Net.Http.Headers; -using Bit.Api.Auth.Models.Request.Accounts; +using System.Net.Http.Headers; using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; using Bit.Api.Models.Response; @@ -45,61 +43,6 @@ public class AccountsControllerTest : IClassFixture Assert.NotNull(content.SecurityStamp); } - [Fact] - public async Task PostEmailToken_WhenAccountDeprovisioningEnabled_WithManagedAccount_ThrowsBadRequest() - { - var email = await SetupOrganizationManagedAccount(); - - var tokens = await _factory.LoginAsync(email); - var client = _factory.CreateClient(); - - var model = new EmailTokenRequestModel - { - NewEmail = $"{Guid.NewGuid()}@example.com", - MasterPasswordHash = "master_password_hash" - }; - - using var message = new HttpRequestMessage(HttpMethod.Post, "/accounts/email-token") - { - Content = JsonContent.Create(model) - }; - message.Headers.Authorization = new AuthenticationHeaderValue("Bearer", tokens.Token); - var response = await client.SendAsync(message); - - Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - var content = await response.Content.ReadAsStringAsync(); - Assert.Contains("Cannot change emails for accounts owned by an organization", content); - } - - [Fact] - public async Task PostEmail_WhenAccountDeprovisioningEnabled_WithManagedAccount_ThrowsBadRequest() - { - var email = await SetupOrganizationManagedAccount(); - - var tokens = await _factory.LoginAsync(email); - var client = _factory.CreateClient(); - - var model = new EmailRequestModel - { - NewEmail = $"{Guid.NewGuid()}@example.com", - MasterPasswordHash = "master_password_hash", - NewMasterPasswordHash = "master_password_hash", - Token = "validtoken", - Key = "key" - }; - - using var message = new HttpRequestMessage(HttpMethod.Post, "/accounts/email") - { - Content = JsonContent.Create(model) - }; - message.Headers.Authorization = new AuthenticationHeaderValue("Bearer", tokens.Token); - var response = await client.SendAsync(message); - - Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - var content = await response.Content.ReadAsStringAsync(); - Assert.Contains("Cannot change emails for accounts owned by an organization", content); - } - private async Task SetupOrganizationManagedAccount() { _factory.SubstituteService(featureService => diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index 33b7e764d4..8bdb14bf78 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -181,22 +181,6 @@ public class AccountsControllerTests : IDisposable ); } - [Fact] - public async Task PostEmailToken_WithAccountDeprovisioningEnabled_WhenUserIsManagedByAnOrganization_ShouldThrowBadRequestException() - { - var user = GenerateExampleUser(); - ConfigureUserServiceToReturnValidPrincipalFor(user); - ConfigureUserServiceToAcceptPasswordFor(user); - _featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(true); - _userService.IsManagedByAnyOrganizationAsync(user.Id).Returns(true); - - var result = await Assert.ThrowsAsync( - () => _sut.PostEmailToken(new EmailTokenRequestModel()) - ); - - Assert.Equal("Cannot change emails for accounts owned by an organization. Contact your organization administrator for additional details.", result.Message); - } - [Fact] public async Task PostEmail_ShouldChangeUserEmail() { @@ -248,20 +232,6 @@ public class AccountsControllerTests : IDisposable ); } - [Fact] - public async Task PostEmail_WithAccountDeprovisioningEnabled_WhenUserIsManagedByAnOrganization_ShouldThrowBadRequestException() - { - var user = GenerateExampleUser(); - ConfigureUserServiceToReturnValidPrincipalFor(user); - _featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(true); - _userService.IsManagedByAnyOrganizationAsync(user.Id).Returns(true); - - var result = await Assert.ThrowsAsync( - () => _sut.PostEmail(new EmailRequestModel()) - ); - - Assert.Equal("Cannot change emails for accounts owned by an organization. Contact your organization administrator for additional details.", result.Message); - } [Fact] public async Task PostVerifyEmail_ShouldSendEmailVerification() diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 880e79500d..7f1dd37b6b 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -248,6 +248,7 @@ public class UserServiceTests sutProvider.GetDependency(), sutProvider.GetDependency(), sutProvider.GetDependency(), + sutProvider.GetDependency(), sutProvider.GetDependency(), sutProvider.GetDependency(), sutProvider.GetDependency>(), @@ -829,6 +830,7 @@ public class UserServiceTests sutProvider.GetDependency(), sutProvider.GetDependency(), sutProvider.GetDependency(), + sutProvider.GetDependency(), sutProvider.GetDependency(), sutProvider.GetDependency(), sutProvider.GetDependency>(),