Skip to content

Commit

Permalink
[PM-11405] Account Management: Prevent a verified user from changing …
Browse files Browse the repository at this point in the history
…their email address (#4875)

* Add check for managed user before purging account

* Rename IOrganizationRepository.GetByClaimedUserDomainAsync to GetByVerifiedUserEmailDomainAsync and refactor to return a list. Remove ManagedByOrganizationId from ProfileResponseMode. Add ManagesActiveUser to ProfileOrganizationResponseModel

* Rename the property ManagesActiveUser to UserIsManagedByOrganization

* Remove whole class #nullable enable and add it to specific places

* [PM-11405] Account Deprovisioning: Prevent a verified user from changing their email address

* Remove unnecessary .ToList()

* Refactor IUserService methods GetOrganizationsManagingUserAsync and IsManagedByAnyOrganizationAsync to not return nullable objects. Update ProfileOrganizationResponseModel.UserIsManagedByOrganization to not be nullable

* Update error message when unable to purge vault for managed account

* Update error message when unable to change email for managed account

* Update expected error messages on unit tests

* Add TestFeatureService to Api.IntegrationTest.Helpers and use it on ApiApplicationFactory to be able to enable specific features for each test

* Add CreateVerifiedDomainAsync method to OrganizationTestHelpers

* Add tests to AccountsControllerTest to prevent changing email for managed accounts

* Remove setting the feature flag value in ApiApplicationFactory and set it on AccountsControllerTest

* Remove TestFeatureService class from Api.IntegrationTest.Helpers
  • Loading branch information
r-tome authored and cyprain-okeke committed Oct 30, 2024
1 parent 246b7fe commit 6297e24
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 1 deletion.
14 changes: 14 additions & 0 deletions src/Api/Auth/Controllers/AccountsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,13 @@ public async Task PostEmailToken([FromBody] EmailTokenRequestModel model)
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);
}

Expand All @@ -165,6 +172,13 @@ public async Task PostEmail([FromBody] EmailRequestModel model)
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);
if (result.Succeeded)
Expand Down
89 changes: 88 additions & 1 deletion test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
using System.Net.Http.Headers;
using System.Net;
using System.Net.Http.Headers;
using Bit.Api.Auth.Models.Request.Accounts;
using Bit.Api.IntegrationTest.Factories;
using Bit.Api.IntegrationTest.Helpers;
using Bit.Api.Models.Response;
using Bit.Core;
using Bit.Core.Billing.Enums;
using Bit.Core.Enums;
using Bit.Core.Models.Data;
using Bit.Core.Services;
using NSubstitute;
using Xunit;

namespace Bit.Api.IntegrationTest.Controllers;
Expand Down Expand Up @@ -35,4 +44,82 @@ public async Task GetPublicKey()
Assert.Null(content.PrivateKey);
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<string> SetupOrganizationManagedAccount()
{
_factory.SubstituteService<IFeatureService>(featureService =>
featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(true));

// Create the owner account
var ownerEmail = $"{Guid.NewGuid()}@bitwarden.com";
await _factory.LoginWithNewAccount(ownerEmail);

// Create the organization
var (_organization, _) = await OrganizationTestHelpers.SignUpAsync(_factory, plan: PlanType.EnterpriseAnnually2023,
ownerEmail: ownerEmail, passwordManagerSeats: 10, paymentMethod: PaymentMethodType.Card);

// Create a new organization member
var (email, orgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, _organization.Id,
OrganizationUserType.Custom, new Permissions { AccessReports = true, ManageScim = true });

// Add a verified domain
await OrganizationTestHelpers.CreateVerifiedDomainAsync(_factory, _organization.Id, "bitwarden.com");

return email;
}
}
18 changes: 18 additions & 0 deletions test/Api.IntegrationTest/Helpers/OrganizationTestHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,22 @@ public static async Task<OrganizationUser> CreateUserAsync<T>(

return (email, organizationUser);
}

/// <summary>
/// Creates a VerifiedDomain for the specified organization.
/// </summary>
public static async Task CreateVerifiedDomainAsync(ApiApplicationFactory factory, Guid organizationId, string domain)
{
var organizationDomainRepository = factory.GetService<IOrganizationDomainRepository>();

var verifiedDomain = new OrganizationDomain
{
OrganizationId = organizationId,
DomainName = domain,
Txt = "btw+test18383838383"
};
verifiedDomain.SetVerifiedDate();

await organizationDomainRepository.CreateAsync(verifiedDomain);
}
}
62 changes: 62 additions & 0 deletions test/Api.Test/Auth/Controllers/AccountsControllerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Bit.Api.Auth.Validators;
using Bit.Api.Tools.Models.Request;
using Bit.Api.Vault.Models.Request;
using Bit.Core;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.AdminConsole.Services;
using Bit.Core.Auth.Entities;
Expand Down Expand Up @@ -143,6 +144,21 @@ public async Task PostEmailToken_ShouldInitiateEmailChange()
await _userService.Received(1).InitiateEmailChangeAsync(user, newEmail);
}

[Fact]
public async Task PostEmailToken_WithAccountDeprovisioningEnabled_WhenUserIsNotManagedByAnOrganization_ShouldInitiateEmailChange()
{
var user = GenerateExampleUser();
ConfigureUserServiceToReturnValidPrincipalFor(user);
ConfigureUserServiceToAcceptPasswordFor(user);
_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(true);
_userService.IsManagedByAnyOrganizationAsync(user.Id).Returns(false);
var newEmail = "[email protected]";

await _sut.PostEmailToken(new EmailTokenRequestModel { NewEmail = newEmail });

await _userService.Received(1).InitiateEmailChangeAsync(user, newEmail);
}

[Fact]
public async Task PostEmailToken_WhenNotAuthorized_ShouldThrowUnauthorizedAccessException()
{
Expand All @@ -165,6 +181,22 @@ await Assert.ThrowsAsync<BadRequestException>(
);
}

[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<BadRequestException>(
() => _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()
{
Expand All @@ -178,6 +210,21 @@ public async Task PostEmail_ShouldChangeUserEmail()
await _userService.Received(1).ChangeEmailAsync(user, default, default, default, default, default);
}

[Fact]
public async Task PostEmail_WithAccountDeprovisioningEnabled_WhenUserIsNotManagedByAnOrganization_ShouldChangeUserEmail()
{
var user = GenerateExampleUser();
ConfigureUserServiceToReturnValidPrincipalFor(user);
_userService.ChangeEmailAsync(user, default, default, default, default, default)
.Returns(Task.FromResult(IdentityResult.Success));
_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(true);
_userService.IsManagedByAnyOrganizationAsync(user.Id).Returns(false);

await _sut.PostEmail(new EmailRequestModel());

await _userService.Received(1).ChangeEmailAsync(user, default, default, default, default, default);
}

[Fact]
public async Task PostEmail_WhenNotAuthorized_ShouldThrownUnauthorizedAccessException()
{
Expand All @@ -201,6 +248,21 @@ await Assert.ThrowsAsync<BadRequestException>(
);
}

[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<BadRequestException>(
() => _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()
{
Expand Down

0 comments on commit 6297e24

Please sign in to comment.