From 8aa39151e0128bd1d3063918ffe99eab6b5d27cd Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Fri, 4 Oct 2024 15:55:26 +0100 Subject: [PATCH 01/15] Add check for managed user before purging account --- src/Api/Vault/Controllers/CiphersController.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 769ba34a16d1..d8d918a2fce5 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -910,6 +910,13 @@ public async Task PostPurge([FromBody] SecretVerificationRequestModel model, str throw new BadRequestException(ModelState); } + // 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("Accounts managed by an organization cannot be purged."); + } + if (string.IsNullOrWhiteSpace(organizationId)) { await _cipherRepository.DeleteByUserIdAsync(user.Id); From fdb4a2cfc535af882cb70db4f905087ee1e4e3dc Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 8 Oct 2024 17:48:07 +0100 Subject: [PATCH 02/15] Rename IOrganizationRepository.GetByClaimedUserDomainAsync to GetByVerifiedUserEmailDomainAsync and refactor to return a list. Remove ManagedByOrganizationId from ProfileResponseMode. Add ManagesActiveUser to ProfileOrganizationResponseModel --- .../Controllers/OrganizationsController.cs | 6 +- .../ProfileOrganizationResponseModel.cs | 17 +- .../Auth/Controllers/AccountsController.cs | 32 ++-- .../Controllers/OrganizationsController.cs | 6 +- .../Models/Response/ProfileResponseModel.cs | 8 +- .../Vault/Controllers/CiphersController.cs | 2 +- src/Api/Vault/Controllers/SyncController.cs | 25 ++- .../Models/Response/SyncResponseModel.cs | 6 +- .../Repositories/IOrganizationRepository.cs | 4 +- src/Core/Services/IUserService.cs | 15 +- .../Services/Implementations/UserService.cs | 22 ++- .../Repositories/OrganizationRepository.cs | 4 +- .../Repositories/OrganizationRepository.cs | 4 +- test/Core.Test/Services/UserServiceTests.cs | 44 +++-- .../OrganizationRepositoryTests.cs | 163 +++++++++++++++++- 15 files changed, 288 insertions(+), 70 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationsController.cs b/src/Api/AdminConsole/Controllers/OrganizationsController.cs index e5dbcd10b1a1..03cc6a384216 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationsController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationsController.cs @@ -31,6 +31,8 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; +#nullable enable + namespace Bit.Api.AdminConsole.Controllers; [Route("organizations")] @@ -120,7 +122,9 @@ public async Task> GetUser() var userId = _userService.GetProperUserId(User).Value; var organizations = await _organizationUserRepository.GetManyDetailsByUserAsync(userId, OrganizationUserStatusType.Confirmed); - var responses = organizations.Select(o => new ProfileOrganizationResponseModel(o)); + var organizationManagingActiveUser = await _userService.GetOrganizationsManagingUserAsync(userId); + var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id); + var responses = organizations.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); return new ListResponseModel(responses); } diff --git a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs index 17ebfc095e28..f2ba8d564d98 100644 --- a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs +++ b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs @@ -9,13 +9,18 @@ using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Utilities; +#nullable enable + namespace Bit.Api.AdminConsole.Models.Response; public class ProfileOrganizationResponseModel : ResponseModel { public ProfileOrganizationResponseModel(string str) : base(str) { } - public ProfileOrganizationResponseModel(OrganizationUserOrganizationDetails organization) : this("profileOrganization") + public ProfileOrganizationResponseModel( + OrganizationUserOrganizationDetails organization, + IEnumerable? organizationIdsManagingActiveUser) + : this("profileOrganization") { Id = organization.OrganizationId; Name = organization.Name; @@ -64,6 +69,7 @@ public ProfileOrganizationResponseModel(OrganizationUserOrganizationDetails orga AccessSecretsManager = organization.AccessSecretsManager; LimitCollectionCreationDeletion = organization.LimitCollectionCreationDeletion; AllowAdminAccessToAllCollectionItems = organization.AllowAdminAccessToAllCollectionItems; + ManagesActiveUser = organizationIdsManagingActiveUser?.Contains(organization.OrganizationId); if (organization.SsoConfig != null) { @@ -122,4 +128,13 @@ public ProfileOrganizationResponseModel(OrganizationUserOrganizationDetails orga public bool AccessSecretsManager { get; set; } public bool LimitCollectionCreationDeletion { get; set; } public bool AllowAdminAccessToAllCollectionItems { get; set; } + /// + /// Indicates if the organization manages the active user. + /// + /// + /// An organization manages a user if the user's email domain is verified by the organization and the user is a member of it. + /// The organization must be enabled and able to have verified domains. + /// This property is null if the Account Deprovisioning feature flag is disabled. + /// + public bool? ManagesActiveUser { get; set; } } diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index cf74460fc15f..b1180883dffa 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -40,6 +40,8 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; +#nullable enable + namespace Bit.Api.Auth.Controllers; [Route("accounts")] @@ -443,11 +445,11 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user); - var managedByOrganizationId = await GetManagedByOrganizationIdAsync(user); + var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); var response = new ProfileResponseModel(user, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails, twoFactorEnabled, - hasPremiumFromOrg, managedByOrganizationId); + hasPremiumFromOrg, organizationIdsManagingActiveUser); return response; } @@ -457,7 +459,8 @@ public async Task> GetOrgani var userId = _userService.GetProperUserId(User); var organizationUserDetails = await _organizationUserRepository.GetManyDetailsByUserAsync(userId.Value, OrganizationUserStatusType.Confirmed); - var responseData = organizationUserDetails.Select(o => new ProfileOrganizationResponseModel(o)); + var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(userId.Value); + var responseData = organizationUserDetails.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); return new ListResponseModel(responseData); } @@ -475,9 +478,9 @@ public async Task PutProfile([FromBody] UpdateProfileReque var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user); - var managedByOrganizationId = await GetManagedByOrganizationIdAsync(user); + var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); - var response = new ProfileResponseModel(user, null, null, null, twoFactorEnabled, hasPremiumFromOrg, managedByOrganizationId); + var response = new ProfileResponseModel(user, null, null, null, twoFactorEnabled, hasPremiumFromOrg, organizationIdsManagingActiveUser); return response; } @@ -494,9 +497,9 @@ public async Task PutAvatar([FromBody] UpdateAvatarRequest var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); - var managedByOrganizationId = await GetManagedByOrganizationIdAsync(user); + var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); - var response = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, managedByOrganizationId); + var response = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); return response; } @@ -647,9 +650,9 @@ public async Task PostPremium(PremiumRequestModel model) var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); - var managedByOrganizationId = await GetManagedByOrganizationIdAsync(user); + var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); - var profile = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, managedByOrganizationId); + var profile = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); return new PaymentResponseModel { UserProfile = profile, @@ -937,14 +940,9 @@ public async Task VerifyOTP([FromBody] VerifyOTPRequestModel model) } } - private async Task GetManagedByOrganizationIdAsync(User user) + private async Task?> GetOrganizationIdsManagingUserAsync(Guid userId) { - if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) - { - return null; - } - - var organizationManagingUser = await _userService.GetOrganizationManagingUserAsync(user.Id); - return organizationManagingUser?.Id; + var organizationManagingUser = await _userService.GetOrganizationsManagingUserAsync(userId); + return organizationManagingUser?.Select(o => o.Id); } } diff --git a/src/Api/Billing/Controllers/OrganizationsController.cs b/src/Api/Billing/Controllers/OrganizationsController.cs index 5371186b1fa7..80bf86419c57 100644 --- a/src/Api/Billing/Controllers/OrganizationsController.cs +++ b/src/Api/Billing/Controllers/OrganizationsController.cs @@ -24,6 +24,8 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; +#nullable enable + namespace Bit.Api.Billing.Controllers; [Route("organizations")] @@ -200,8 +202,10 @@ await addSecretsManagerSubscriptionCommand.SignUpAsync(organization, model.Addit var organizationDetails = await organizationUserRepository.GetDetailsByUserAsync(userId, organization.Id, OrganizationUserStatusType.Confirmed); + var organizationManagingActiveUser = await userService.GetOrganizationsManagingUserAsync(userId); + var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id); - return new ProfileOrganizationResponseModel(organizationDetails); + return new ProfileOrganizationResponseModel(organizationDetails, organizationIdsManagingActiveUser); } [HttpPost("{id:guid}/seat")] diff --git a/src/Api/Models/Response/ProfileResponseModel.cs b/src/Api/Models/Response/ProfileResponseModel.cs index f5d0382e515e..ef7efb75af6e 100644 --- a/src/Api/Models/Response/ProfileResponseModel.cs +++ b/src/Api/Models/Response/ProfileResponseModel.cs @@ -5,6 +5,8 @@ using Bit.Core.Models.Api; using Bit.Core.Models.Data.Organizations.OrganizationUsers; +#nullable enable + namespace Bit.Api.Models.Response; public class ProfileResponseModel : ResponseModel @@ -15,7 +17,7 @@ public ProfileResponseModel(User user, IEnumerable providerUserOrganizationDetails, bool twoFactorEnabled, bool premiumFromOrganization, - Guid? managedByOrganizationId) : base("profile") + IEnumerable? organizationIdsManagingActiveUser) : base("profile") { if (user == null) { @@ -37,11 +39,10 @@ public ProfileResponseModel(User user, UsesKeyConnector = user.UsesKeyConnector; AvatarColor = user.AvatarColor; CreationDate = user.CreationDate; - Organizations = organizationsUserDetails?.Select(o => new ProfileOrganizationResponseModel(o)); + Organizations = organizationsUserDetails?.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); Providers = providerUserDetails?.Select(p => new ProfileProviderResponseModel(p)); ProviderOrganizations = providerUserOrganizationDetails?.Select(po => new ProfileProviderOrganizationResponseModel(po)); - ManagedByOrganizationId = managedByOrganizationId; } public ProfileResponseModel() : base("profile") @@ -63,7 +64,6 @@ public ProfileResponseModel() : base("profile") public bool UsesKeyConnector { get; set; } public string AvatarColor { get; set; } public DateTime CreationDate { get; set; } - public Guid? ManagedByOrganizationId { get; set; } public IEnumerable Organizations { get; set; } public IEnumerable Providers { get; set; } public IEnumerable ProviderOrganizations { get; set; } diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index d8d918a2fce5..06a4224e67b4 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -912,7 +912,7 @@ public async Task PostPurge([FromBody] SecretVerificationRequestModel model, str // 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)) + && await _userService.IsManagedByAnyOrganizationAsync(user.Id) == true) { throw new BadRequestException("Accounts managed by an organization cannot be purged."); } diff --git a/src/Api/Vault/Controllers/SyncController.cs b/src/Api/Vault/Controllers/SyncController.cs index 79c71bb87dfa..8222e045ec24 100644 --- a/src/Api/Vault/Controllers/SyncController.cs +++ b/src/Api/Vault/Controllers/SyncController.cs @@ -16,6 +16,8 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; +#nullable enable + namespace Bit.Api.Vault.Controllers; [Route("sync")] @@ -95,23 +97,32 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); - var managedByOrganizationId = await GetManagedByOrganizationIdAsync(user, organizationUserDetails); + var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user, organizationUserDetails); var response = new SyncResponseModel(_globalSettings, user, userTwoFactorEnabled, userHasPremiumFromOrganization, - managedByOrganizationId, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails, + organizationIdsManagingActiveUser, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails, folders, collections, ciphers, collectionCiphersGroupDict, excludeDomains, policies, sends); return response; } - private async Task GetManagedByOrganizationIdAsync(User user, IEnumerable organizationUserDetails) + /// + /// Gets the IDs of the organizations that manage a user. + /// + /// + /// Organizations are considered to manage a user if the user's email domain is verified by the organization and the user is a member of it. + /// The organization must be enabled and able to have verified domains. + /// + private async Task?> GetOrganizationIdsManagingUserAsync(User user, IEnumerable organizationUserDetails) { - if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) || - !organizationUserDetails.Any(o => o.Enabled && o.UseSso)) + // Account deprovisioning must be enabled and organizations must be enabled and able to have verified domains. + // TODO: Replace "UseSso" with a new organization ability like "UseOrganizationDomains" (PM-11622). + if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + || !organizationUserDetails.Any(o => o is { Enabled: true, UseSso: true })) { return null; } - var organizationManagingUser = await _userService.GetOrganizationManagingUserAsync(user.Id); - return organizationManagingUser?.Id; + var organizationsManagingUser = await _userService.GetOrganizationsManagingUserAsync(user.Id); + return organizationsManagingUser?.Select(o => o.Id); } } diff --git a/src/Api/Vault/Models/Response/SyncResponseModel.cs b/src/Api/Vault/Models/Response/SyncResponseModel.cs index 2170a5232218..c428bf5ab4b2 100644 --- a/src/Api/Vault/Models/Response/SyncResponseModel.cs +++ b/src/Api/Vault/Models/Response/SyncResponseModel.cs @@ -12,6 +12,8 @@ using Bit.Core.Vault.Entities; using Bit.Core.Vault.Models.Data; +#nullable enable + namespace Bit.Api.Vault.Models.Response; public class SyncResponseModel : ResponseModel @@ -21,7 +23,7 @@ public SyncResponseModel( User user, bool userTwoFactorEnabled, bool userHasPremiumFromOrganization, - Guid? managedByOrganizationId, + IEnumerable? organizationIdsManagingActiveUser, IEnumerable organizationUserDetails, IEnumerable providerUserDetails, IEnumerable providerUserOrganizationDetails, @@ -35,7 +37,7 @@ public SyncResponseModel( : base("sync") { Profile = new ProfileResponseModel(user, organizationUserDetails, providerUserDetails, - providerUserOrganizationDetails, userTwoFactorEnabled, userHasPremiumFromOrganization, managedByOrganizationId); + providerUserOrganizationDetails, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); Folders = folders.Select(f => new FolderResponseModel(f)); Ciphers = ciphers.Select(c => new CipherDetailsResponseModel(c, globalSettings, collectionCiphersDict)); Collections = collections?.Select( diff --git a/src/Core/AdminConsole/Repositories/IOrganizationRepository.cs b/src/Core/AdminConsole/Repositories/IOrganizationRepository.cs index 9c14c4fbdfca..5b274d3f88af 100644 --- a/src/Core/AdminConsole/Repositories/IOrganizationRepository.cs +++ b/src/Core/AdminConsole/Repositories/IOrganizationRepository.cs @@ -19,7 +19,7 @@ public interface IOrganizationRepository : IRepository Task> GetOwnerEmailAddressesById(Guid organizationId); /// - /// Gets the organization that has a claimed domain matching the user's email domain. + /// Gets the organizations that have a verified domain matching the user's email domain. /// - Task GetByClaimedUserDomainAsync(Guid userId); + Task> GetByVerifiedUserEmailDomainAsync(Guid userId); } diff --git a/src/Core/Services/IUserService.cs b/src/Core/Services/IUserService.cs index 0135b5f1b1bc..e4c823b7f37f 100644 --- a/src/Core/Services/IUserService.cs +++ b/src/Core/Services/IUserService.cs @@ -8,6 +8,8 @@ using Fido2NetLib; using Microsoft.AspNetCore.Identity; +#nullable enable + namespace Bit.Core.Services; public interface IUserService @@ -92,14 +94,17 @@ Task UpdatePasswordHash(User user, string newPassword, /// Indicates if the user is managed by any organization. /// /// - /// A managed user is a user whose email domain matches one of the Organization's verified domains. - /// The organization must be enabled and be on an Enterprise plan. + /// A user is considered managed by an organization if their email domain matches one of the verified domains of that organization, and the user is a member of it. + /// The organization must be enabled and able to have verified domains. /// - Task IsManagedByAnyOrganizationAsync(Guid userId); + /// + /// This returns a nullable object because if the Account Deprovisioning feature flag is disabled, we should return null. + /// + Task IsManagedByAnyOrganizationAsync(Guid userId); /// - /// Gets the organization that manages the user. + /// Gets the organizations that manage the user. /// /// - Task GetOrganizationManagingUserAsync(Guid userId); + Task?> GetOrganizationsManagingUserAsync(Guid userId); } diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index fe04efa22ff6..d37f6695f224 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -31,6 +31,8 @@ using File = System.IO.File; using JsonSerializer = System.Text.Json.JsonSerializer; +#nullable enable + namespace Bit.Core.Services; public class UserService : UserManager, IUserService, IDisposable @@ -1263,20 +1265,26 @@ public async Task IsLegacyUser(string userId) return IsLegacyUser(user); } - public async Task IsManagedByAnyOrganizationAsync(Guid userId) + public async Task IsManagedByAnyOrganizationAsync(Guid userId) { - var managingOrganization = await GetOrganizationManagingUserAsync(userId); - return managingOrganization != null; + var managingOrganizations = await GetOrganizationsManagingUserAsync(userId); + return managingOrganizations?.Any(); } - public async Task GetOrganizationManagingUserAsync(Guid userId) + public async Task?> GetOrganizationsManagingUserAsync(Guid userId) { - // Users can only be managed by an Organization that is enabled and can have organization domains - var organization = await _organizationRepository.GetByClaimedUserDomainAsync(userId); + if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) + { + return null; + } + + // Get all organizations that have verified the user's email domain. + var organizationsWithVerifiedUserEmailDomain = await _organizationRepository.GetByVerifiedUserEmailDomainAsync(userId); + // Organizations must be enabled and able to have verified domains. // TODO: Replace "UseSso" with a new organization ability like "UseOrganizationDomains" (PM-11622). // Verified domains were tied to SSO, so we currently check the "UseSso" organization ability. - return (organization is { Enabled: true, UseSso: true }) ? organization : null; + return organizationsWithVerifiedUserEmailDomain.Where(organization => organization is { Enabled: true, UseSso: true }).ToList(); } /// diff --git a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs index bdc2fb4cab1d..20fdf83155f1 100644 --- a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs +++ b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs @@ -168,7 +168,7 @@ public async Task> GetOwnerEmailAddressesById(Guid organizat commandType: CommandType.StoredProcedure); } - public async Task GetByClaimedUserDomainAsync(Guid userId) + public async Task> GetByVerifiedUserEmailDomainAsync(Guid userId) { using (var connection = new SqlConnection(ConnectionString)) { @@ -177,7 +177,7 @@ public async Task GetByClaimedUserDomainAsync(Guid userId) new { UserId = userId }, commandType: CommandType.StoredProcedure); - return result.SingleOrDefault(); + return result.ToList(); } } } diff --git a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs index 96c9a912e1ee..bb9090e0ae9a 100644 --- a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs +++ b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs @@ -276,7 +276,7 @@ into grouped return await query.ToListAsync(); } - public async Task GetByClaimedUserDomainAsync(Guid userId) + public async Task> GetByVerifiedUserEmailDomainAsync(Guid userId) { using (var scope = ServiceScopeFactory.CreateScope()) { @@ -291,7 +291,7 @@ join od in dbContext.OrganizationDomains on ou.OrganizationId equals od.Organiza && u.Email.ToLower().EndsWith("@" + od.DomainName.ToLower()) select o; - return await query.FirstOrDefaultAsync(); + return await query.ToArrayAsync(); } } diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 098d4d2796c6..29cec5e8877b 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -26,7 +26,6 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using NSubstitute; -using NSubstitute.ReceivedExtensions; using Xunit; namespace Bit.Core.Test.Services; @@ -280,45 +279,70 @@ await tokenProvider } [Theory, BitAutoData] - public async Task IsManagedByAnyOrganizationAsync_WithManagingEnabledOrganization_ReturnsTrue( + public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningDisabled_ReturnsNull( + SutProvider sutProvider, Guid userId) + { + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(false); + + var result = await sutProvider.Sut.IsManagedByAnyOrganizationAsync(userId); + Assert.Null(result); + } + + + [Theory, BitAutoData] + public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningEnabled_WithManagingEnabledOrganization_ReturnsTrue( SutProvider sutProvider, Guid userId, Organization organization) { organization.Enabled = true; organization.UseSso = true; + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(true); + sutProvider.GetDependency() - .GetByClaimedUserDomainAsync(userId) - .Returns(organization); + .GetByVerifiedUserEmailDomainAsync(userId) + .Returns(new[] { organization }); var result = await sutProvider.Sut.IsManagedByAnyOrganizationAsync(userId); Assert.True(result); } [Theory, BitAutoData] - public async Task IsManagedByAnyOrganizationAsync_WithManagingDisabledOrganization_ReturnsFalse( + public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningEnabled_WithManagingDisabledOrganization_ReturnsFalse( SutProvider sutProvider, Guid userId, Organization organization) { organization.Enabled = false; organization.UseSso = true; + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(true); + sutProvider.GetDependency() - .GetByClaimedUserDomainAsync(userId) - .Returns(organization); + .GetByVerifiedUserEmailDomainAsync(userId) + .Returns(new[] { organization }); var result = await sutProvider.Sut.IsManagedByAnyOrganizationAsync(userId); Assert.False(result); } [Theory, BitAutoData] - public async Task IsManagedByAnyOrganizationAsync_WithOrganizationUseSsoFalse_ReturnsFalse( + public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningEnabled_WithOrganizationUseSsoFalse_ReturnsFalse( SutProvider sutProvider, Guid userId, Organization organization) { organization.Enabled = true; organization.UseSso = false; + sutProvider.GetDependency() + .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) + .Returns(true); + sutProvider.GetDependency() - .GetByClaimedUserDomainAsync(userId) - .Returns(organization); + .GetByVerifiedUserEmailDomainAsync(userId) + .Returns(new[] { organization }); var result = await sutProvider.Sut.IsManagedByAnyOrganizationAsync(userId); Assert.False(result); diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationRepositoryTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationRepositoryTests.cs index eac71e9c24b5..f6dc4a989d8e 100644 --- a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationRepositoryTests.cs @@ -97,13 +97,160 @@ await organizationUserRepository.CreateAsync(new OrganizationUser ResetPasswordKey = "resetpasswordkey1", }); - var user1Response = await organizationRepository.GetByClaimedUserDomainAsync(user1.Id); - var user2Response = await organizationRepository.GetByClaimedUserDomainAsync(user2.Id); - var user3Response = await organizationRepository.GetByClaimedUserDomainAsync(user3.Id); - - Assert.NotNull(user1Response); - Assert.Equal(organization.Id, user1Response.Id); - Assert.Null(user2Response); - Assert.Null(user3Response); + var user1Response = await organizationRepository.GetByVerifiedUserEmailDomainAsync(user1.Id); + var user2Response = await organizationRepository.GetByVerifiedUserEmailDomainAsync(user2.Id); + var user3Response = await organizationRepository.GetByVerifiedUserEmailDomainAsync(user3.Id); + + Assert.NotEmpty(user1Response); + Assert.Equal(organization.Id, user1Response.First().Id); + Assert.Empty(user2Response); + Assert.Empty(user3Response); + } + + [DatabaseTheory, DatabaseData] + public async Task GetByVerifiedUserEmailDomainAsync_WithUnverifiedDomains_ReturnsEmpty( + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + IOrganizationDomainRepository organizationDomainRepository) + { + var id = Guid.NewGuid(); + var domainName = $"{id}.example.com"; + + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{id}@{domainName}", + ApiKey = "TEST", + SecurityStamp = "stamp", + Kdf = KdfType.PBKDF2_SHA256, + KdfIterations = 1, + KdfMemory = 2, + KdfParallelism = 3 + }); + + var organization = await organizationRepository.CreateAsync(new Organization + { + Name = $"Test Org {id}", + BillingEmail = user.Email, + Plan = "Test", + PrivateKey = "privatekey", + }); + + var organizationDomain = new OrganizationDomain + { + OrganizationId = organization.Id, + DomainName = domainName, + Txt = "btw+12345", + }; + organizationDomain.SetNextRunDate(12); + organizationDomain.SetJobRunCount(); + await organizationDomainRepository.CreateAsync(organizationDomain); + + await organizationUserRepository.CreateAsync(new OrganizationUser + { + OrganizationId = organization.Id, + UserId = user.Id, + Status = OrganizationUserStatusType.Confirmed, + ResetPasswordKey = "resetpasswordkey", + }); + + var result = await organizationRepository.GetByVerifiedUserEmailDomainAsync(user.Id); + + Assert.Empty(result); + } + + [DatabaseTheory, DatabaseData] + public async Task GetByVerifiedUserEmailDomainAsync_WithMultipleVerifiedDomains_ReturnsAllMatchingOrganizations( + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + IOrganizationDomainRepository organizationDomainRepository) + { + var id = Guid.NewGuid(); + var domainName = $"{id}.example.com"; + + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{id}@{domainName}", + ApiKey = "TEST", + SecurityStamp = "stamp", + Kdf = KdfType.PBKDF2_SHA256, + KdfIterations = 1, + KdfMemory = 2, + KdfParallelism = 3 + }); + + var organization1 = await organizationRepository.CreateAsync(new Organization + { + Name = $"Test Org 1 {id}", + BillingEmail = user.Email, + Plan = "Test", + PrivateKey = "privatekey1", + }); + + var organization2 = await organizationRepository.CreateAsync(new Organization + { + Name = $"Test Org 2 {id}", + BillingEmail = user.Email, + Plan = "Test", + PrivateKey = "privatekey2", + }); + + var organizationDomain1 = new OrganizationDomain + { + OrganizationId = organization1.Id, + DomainName = domainName, + Txt = "btw+12345", + }; + organizationDomain1.SetNextRunDate(12); + organizationDomain1.SetJobRunCount(); + organizationDomain1.SetVerifiedDate(); + await organizationDomainRepository.CreateAsync(organizationDomain1); + + var organizationDomain2 = new OrganizationDomain + { + OrganizationId = organization2.Id, + DomainName = domainName, + Txt = "btw+67890", + }; + organizationDomain2.SetNextRunDate(12); + organizationDomain2.SetJobRunCount(); + organizationDomain2.SetVerifiedDate(); + await organizationDomainRepository.CreateAsync(organizationDomain2); + + await organizationUserRepository.CreateAsync(new OrganizationUser + { + OrganizationId = organization1.Id, + UserId = user.Id, + Status = OrganizationUserStatusType.Confirmed, + ResetPasswordKey = "resetpasswordkey1", + }); + + await organizationUserRepository.CreateAsync(new OrganizationUser + { + OrganizationId = organization2.Id, + UserId = user.Id, + Status = OrganizationUserStatusType.Confirmed, + ResetPasswordKey = "resetpasswordkey2", + }); + + var result = await organizationRepository.GetByVerifiedUserEmailDomainAsync(user.Id); + + Assert.Equal(2, result.Count); + Assert.Contains(result, org => org.Id == organization1.Id); + Assert.Contains(result, org => org.Id == organization2.Id); + } + + [DatabaseTheory, DatabaseData] + public async Task GetByVerifiedUserEmailDomainAsync_WithNonExistentUser_ReturnsEmpty( + IOrganizationRepository organizationRepository) + { + var nonExistentUserId = Guid.NewGuid(); + + var result = await organizationRepository.GetByVerifiedUserEmailDomainAsync(nonExistentUserId); + + Assert.Empty(result); } } From 0c6107be29648033b3b2ce6cd1178759a67a49c3 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Wed, 9 Oct 2024 15:26:15 +0100 Subject: [PATCH 03/15] Rename the property ManagesActiveUser to UserIsManagedByOrganization --- .../Models/Response/ProfileOrganizationResponseModel.cs | 8 ++++---- src/Api/Models/Response/ProfileResponseModel.cs | 4 ++-- src/Api/Vault/Models/Response/SyncResponseModel.cs | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs index f2ba8d564d98..70dbed5fa79b 100644 --- a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs +++ b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs @@ -19,7 +19,7 @@ public ProfileOrganizationResponseModel(string str) : base(str) { } public ProfileOrganizationResponseModel( OrganizationUserOrganizationDetails organization, - IEnumerable? organizationIdsManagingActiveUser) + IEnumerable? organizationIdsManagingUser) : this("profileOrganization") { Id = organization.OrganizationId; @@ -69,7 +69,7 @@ public ProfileOrganizationResponseModel( AccessSecretsManager = organization.AccessSecretsManager; LimitCollectionCreationDeletion = organization.LimitCollectionCreationDeletion; AllowAdminAccessToAllCollectionItems = organization.AllowAdminAccessToAllCollectionItems; - ManagesActiveUser = organizationIdsManagingActiveUser?.Contains(organization.OrganizationId); + UserIsManagedByOrganization = organizationIdsManagingUser?.Contains(organization.OrganizationId); if (organization.SsoConfig != null) { @@ -129,12 +129,12 @@ public ProfileOrganizationResponseModel( public bool LimitCollectionCreationDeletion { get; set; } public bool AllowAdminAccessToAllCollectionItems { get; set; } /// - /// Indicates if the organization manages the active user. + /// Indicates if the organization manages the user. /// /// /// An organization manages a user if the user's email domain is verified by the organization and the user is a member of it. /// The organization must be enabled and able to have verified domains. /// This property is null if the Account Deprovisioning feature flag is disabled. /// - public bool? ManagesActiveUser { get; set; } + public bool? UserIsManagedByOrganization { get; set; } } diff --git a/src/Api/Models/Response/ProfileResponseModel.cs b/src/Api/Models/Response/ProfileResponseModel.cs index ef7efb75af6e..52063852d68f 100644 --- a/src/Api/Models/Response/ProfileResponseModel.cs +++ b/src/Api/Models/Response/ProfileResponseModel.cs @@ -17,7 +17,7 @@ public ProfileResponseModel(User user, IEnumerable providerUserOrganizationDetails, bool twoFactorEnabled, bool premiumFromOrganization, - IEnumerable? organizationIdsManagingActiveUser) : base("profile") + IEnumerable? organizationIdsManagingUser) : base("profile") { if (user == null) { @@ -39,7 +39,7 @@ public ProfileResponseModel(User user, UsesKeyConnector = user.UsesKeyConnector; AvatarColor = user.AvatarColor; CreationDate = user.CreationDate; - Organizations = organizationsUserDetails?.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); + Organizations = organizationsUserDetails?.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingUser)); Providers = providerUserDetails?.Select(p => new ProfileProviderResponseModel(p)); ProviderOrganizations = providerUserOrganizationDetails?.Select(po => new ProfileProviderOrganizationResponseModel(po)); diff --git a/src/Api/Vault/Models/Response/SyncResponseModel.cs b/src/Api/Vault/Models/Response/SyncResponseModel.cs index c428bf5ab4b2..fe7d87eef0d5 100644 --- a/src/Api/Vault/Models/Response/SyncResponseModel.cs +++ b/src/Api/Vault/Models/Response/SyncResponseModel.cs @@ -23,7 +23,7 @@ public SyncResponseModel( User user, bool userTwoFactorEnabled, bool userHasPremiumFromOrganization, - IEnumerable? organizationIdsManagingActiveUser, + IEnumerable? organizationIdsManagingUser, IEnumerable organizationUserDetails, IEnumerable providerUserDetails, IEnumerable providerUserOrganizationDetails, @@ -37,7 +37,7 @@ public SyncResponseModel( : base("sync") { Profile = new ProfileResponseModel(user, organizationUserDetails, providerUserDetails, - providerUserOrganizationDetails, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); + providerUserOrganizationDetails, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingUser); Folders = folders.Select(f => new FolderResponseModel(f)); Ciphers = ciphers.Select(c => new CipherDetailsResponseModel(c, globalSettings, collectionCiphersDict)); Collections = collections?.Select( From 6d1859f856564b15ccff20d4a6d3d02eb0d7f677 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Wed, 9 Oct 2024 16:24:24 +0100 Subject: [PATCH 04/15] Remove whole class #nullable enable and add it to specific places --- .../Controllers/OrganizationsController.cs | 6 ++++-- .../ProfileOrganizationResponseModel.cs | 7 ++++--- .../Auth/Controllers/AccountsController.cs | 20 +++++++++++++++++-- .../Controllers/OrganizationsController.cs | 5 +++-- .../Models/Response/ProfileResponseModel.cs | 7 ++++--- src/Api/Vault/Controllers/SyncController.cs | 7 +++++-- .../Models/Response/SyncResponseModel.cs | 4 ++-- src/Core/Services/IUserService.cs | 4 ++-- .../Services/Implementations/UserService.cs | 4 ++-- 9 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationsController.cs b/src/Api/AdminConsole/Controllers/OrganizationsController.cs index 03cc6a384216..202c19f5da7a 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationsController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationsController.cs @@ -31,8 +31,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; -#nullable enable - namespace Bit.Api.AdminConsole.Controllers; [Route("organizations")] @@ -122,8 +120,12 @@ public async Task> GetUser() var userId = _userService.GetProperUserId(User).Value; var organizations = await _organizationUserRepository.GetManyDetailsByUserAsync(userId, OrganizationUserStatusType.Confirmed); + +#nullable enable var organizationManagingActiveUser = await _userService.GetOrganizationsManagingUserAsync(userId); var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id); +#nullable disable + var responses = organizations.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); return new ListResponseModel(responses); } diff --git a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs index 70dbed5fa79b..4a9c9c8698af 100644 --- a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs +++ b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs @@ -9,8 +9,6 @@ using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Utilities; -#nullable enable - namespace Bit.Api.AdminConsole.Models.Response; public class ProfileOrganizationResponseModel : ResponseModel @@ -19,7 +17,10 @@ public ProfileOrganizationResponseModel(string str) : base(str) { } public ProfileOrganizationResponseModel( OrganizationUserOrganizationDetails organization, - IEnumerable? organizationIdsManagingUser) +#nullable enable + IEnumerable? organizationIdsManagingUser +#nullable disable + ) : this("profileOrganization") { Id = organization.OrganizationId; diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index b1180883dffa..bb57e1416102 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -40,8 +40,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; -#nullable enable - namespace Bit.Api.Auth.Controllers; [Route("accounts")] @@ -445,7 +443,10 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user); + +#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); +#nullable disable var response = new ProfileResponseModel(user, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails, twoFactorEnabled, @@ -459,7 +460,11 @@ public async Task> GetOrgani var userId = _userService.GetProperUserId(User); var organizationUserDetails = await _organizationUserRepository.GetManyDetailsByUserAsync(userId.Value, OrganizationUserStatusType.Confirmed); + +#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(userId.Value); +#nullable disable + var responseData = organizationUserDetails.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); return new ListResponseModel(responseData); } @@ -478,7 +483,10 @@ public async Task PutProfile([FromBody] UpdateProfileReque var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user); + +#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); +#nullable disable var response = new ProfileResponseModel(user, null, null, null, twoFactorEnabled, hasPremiumFromOrg, organizationIdsManagingActiveUser); return response; @@ -497,7 +505,10 @@ public async Task PutAvatar([FromBody] UpdateAvatarRequest var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); + +#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); +#nullable disable var response = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); return response; @@ -650,7 +661,10 @@ public async Task PostPremium(PremiumRequestModel model) var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); + +#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); +#nullable disable var profile = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); return new PaymentResponseModel @@ -940,9 +954,11 @@ public async Task VerifyOTP([FromBody] VerifyOTPRequestModel model) } } +#nullable enable private async Task?> GetOrganizationIdsManagingUserAsync(Guid userId) { var organizationManagingUser = await _userService.GetOrganizationsManagingUserAsync(userId); return organizationManagingUser?.Select(o => o.Id); } +#nullable disable } diff --git a/src/Api/Billing/Controllers/OrganizationsController.cs b/src/Api/Billing/Controllers/OrganizationsController.cs index 80bf86419c57..bab5fe33d1d6 100644 --- a/src/Api/Billing/Controllers/OrganizationsController.cs +++ b/src/Api/Billing/Controllers/OrganizationsController.cs @@ -24,8 +24,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; -#nullable enable - namespace Bit.Api.Billing.Controllers; [Route("organizations")] @@ -202,8 +200,11 @@ await addSecretsManagerSubscriptionCommand.SignUpAsync(organization, model.Addit var organizationDetails = await organizationUserRepository.GetDetailsByUserAsync(userId, organization.Id, OrganizationUserStatusType.Confirmed); + +#nullable enable var organizationManagingActiveUser = await userService.GetOrganizationsManagingUserAsync(userId); var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id); +#nullable disable return new ProfileOrganizationResponseModel(organizationDetails, organizationIdsManagingActiveUser); } diff --git a/src/Api/Models/Response/ProfileResponseModel.cs b/src/Api/Models/Response/ProfileResponseModel.cs index 52063852d68f..0df28961d629 100644 --- a/src/Api/Models/Response/ProfileResponseModel.cs +++ b/src/Api/Models/Response/ProfileResponseModel.cs @@ -5,8 +5,6 @@ using Bit.Core.Models.Api; using Bit.Core.Models.Data.Organizations.OrganizationUsers; -#nullable enable - namespace Bit.Api.Models.Response; public class ProfileResponseModel : ResponseModel @@ -17,7 +15,10 @@ public ProfileResponseModel(User user, IEnumerable providerUserOrganizationDetails, bool twoFactorEnabled, bool premiumFromOrganization, - IEnumerable? organizationIdsManagingUser) : base("profile") +#nullable enable + IEnumerable? organizationIdsManagingUser +#nullable disable + ) : base("profile") { if (user == null) { diff --git a/src/Api/Vault/Controllers/SyncController.cs b/src/Api/Vault/Controllers/SyncController.cs index 8222e045ec24..0f8fc1c28ca9 100644 --- a/src/Api/Vault/Controllers/SyncController.cs +++ b/src/Api/Vault/Controllers/SyncController.cs @@ -16,8 +16,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; -#nullable enable - namespace Bit.Api.Vault.Controllers; [Route("sync")] @@ -97,7 +95,10 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); + +#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user, organizationUserDetails); +#nullable disable var response = new SyncResponseModel(_globalSettings, user, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails, @@ -105,6 +106,7 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, return response; } +#nullable enable /// /// Gets the IDs of the organizations that manage a user. /// @@ -125,4 +127,5 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, var organizationsManagingUser = await _userService.GetOrganizationsManagingUserAsync(user.Id); return organizationsManagingUser?.Select(o => o.Id); } +#nullable disable } diff --git a/src/Api/Vault/Models/Response/SyncResponseModel.cs b/src/Api/Vault/Models/Response/SyncResponseModel.cs index fe7d87eef0d5..725e22fe583f 100644 --- a/src/Api/Vault/Models/Response/SyncResponseModel.cs +++ b/src/Api/Vault/Models/Response/SyncResponseModel.cs @@ -12,8 +12,6 @@ using Bit.Core.Vault.Entities; using Bit.Core.Vault.Models.Data; -#nullable enable - namespace Bit.Api.Vault.Models.Response; public class SyncResponseModel : ResponseModel @@ -23,7 +21,9 @@ public SyncResponseModel( User user, bool userTwoFactorEnabled, bool userHasPremiumFromOrganization, +#nullable enable IEnumerable? organizationIdsManagingUser, +#nullable disable IEnumerable organizationUserDetails, IEnumerable providerUserDetails, IEnumerable providerUserOrganizationDetails, diff --git a/src/Core/Services/IUserService.cs b/src/Core/Services/IUserService.cs index e4c823b7f37f..5945b2414d53 100644 --- a/src/Core/Services/IUserService.cs +++ b/src/Core/Services/IUserService.cs @@ -8,8 +8,6 @@ using Fido2NetLib; using Microsoft.AspNetCore.Identity; -#nullable enable - namespace Bit.Core.Services; public interface IUserService @@ -90,6 +88,7 @@ Task UpdatePasswordHash(User user, string newPassword, /// Task IsLegacyUser(string userId); +#nullable enable /// /// Indicates if the user is managed by any organization. /// @@ -107,4 +106,5 @@ Task UpdatePasswordHash(User user, string newPassword, /// /// Task?> GetOrganizationsManagingUserAsync(Guid userId); +#nullable disable } diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index d37f6695f224..163731dcb14d 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -31,8 +31,6 @@ using File = System.IO.File; using JsonSerializer = System.Text.Json.JsonSerializer; -#nullable enable - namespace Bit.Core.Services; public class UserService : UserManager, IUserService, IDisposable @@ -1265,6 +1263,7 @@ public async Task IsLegacyUser(string userId) return IsLegacyUser(user); } +#nullable enable public async Task IsManagedByAnyOrganizationAsync(Guid userId) { var managingOrganizations = await GetOrganizationsManagingUserAsync(userId); @@ -1286,6 +1285,7 @@ public async Task IsLegacyUser(string userId) // Verified domains were tied to SSO, so we currently check the "UseSso" organization ability. return organizationsWithVerifiedUserEmailDomain.Where(organization => organization is { Enabled: true, UseSso: true }).ToList(); } +#nullable disable /// public static bool IsLegacyUser(User user) From 6decb51268926e086e5e176892957a6e2453abd0 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Thu, 10 Oct 2024 13:42:36 +0100 Subject: [PATCH 05/15] [PM-11405] Account Deprovisioning: Prevent a verified user from changing their email address --- .../Auth/Controllers/AccountsController.cs | 14 +++++ .../Controllers/AccountsControllerTests.cs | 62 +++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index bb57e1416102..51bf8458bdee 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -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) == true) + { + throw new BadRequestException("Accounts managed by an organization cannot change email."); + } + await _userService.InitiateEmailChangeAsync(user, model.NewEmail); } @@ -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) == true) + { + throw new BadRequestException("Accounts managed by an organization cannot change email."); + } + var result = await _userService.ChangeEmailAsync(user, model.MasterPasswordHash, model.NewEmail, model.NewMasterPasswordHash, model.Token, model.Key); if (result.Succeeded) diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index a16a9cb55fe2..bddf7cf18715 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -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; @@ -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 = "example@user.com"; + + await _sut.PostEmailToken(new EmailTokenRequestModel { NewEmail = newEmail }); + + await _userService.Received(1).InitiateEmailChangeAsync(user, newEmail); + } + [Fact] public async Task PostEmailToken_WhenNotAuthorized_ShouldThrowUnauthorizedAccessException() { @@ -165,6 +181,22 @@ await Assert.ThrowsAsync( ); } + [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("Accounts managed by an organization cannot change email.", result.Message); + } + [Fact] public async Task PostEmail_ShouldChangeUserEmail() { @@ -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() { @@ -201,6 +248,21 @@ await Assert.ThrowsAsync( ); } + [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("Accounts managed by an organization cannot change email.", result.Message); + } + [Fact] public async Task PostVerifyEmail_ShouldSendEmailVerification() { From d6a3a10ffc1aa28248ddbce568c198175b815fbf Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Thu, 10 Oct 2024 16:26:05 +0100 Subject: [PATCH 06/15] Remove unnecessary .ToList() --- src/Core/Services/Implementations/UserService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index 163731dcb14d..3e80dfd1534a 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -1283,7 +1283,7 @@ public async Task IsLegacyUser(string userId) // Organizations must be enabled and able to have verified domains. // TODO: Replace "UseSso" with a new organization ability like "UseOrganizationDomains" (PM-11622). // Verified domains were tied to SSO, so we currently check the "UseSso" organization ability. - return organizationsWithVerifiedUserEmailDomain.Where(organization => organization is { Enabled: true, UseSso: true }).ToList(); + return organizationsWithVerifiedUserEmailDomain.Where(organization => organization is { Enabled: true, UseSso: true }); } #nullable disable From ee24a919ff1545d9f09675d9a1de6fa6dfd4271a Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Fri, 11 Oct 2024 10:59:25 +0100 Subject: [PATCH 07/15] Refactor IUserService methods GetOrganizationsManagingUserAsync and IsManagedByAnyOrganizationAsync to not return nullable objects. Update ProfileOrganizationResponseModel.UserIsManagedByOrganization to not be nullable --- .../Controllers/OrganizationsController.cs | 4 +-- .../ProfileOrganizationResponseModel.cs | 13 ++++---- .../Auth/Controllers/AccountsController.cs | 21 ++----------- .../Controllers/OrganizationsController.cs | 4 +-- .../Models/Response/ProfileResponseModel.cs | 5 +-- .../Vault/Controllers/CiphersController.cs | 2 +- src/Api/Vault/Controllers/SyncController.cs | 31 ++----------------- .../Models/Response/SyncResponseModel.cs | 4 +-- src/Core/Services/IUserService.cs | 11 ++++--- .../Services/Implementations/UserService.cs | 10 +++--- test/Core.Test/Services/UserServiceTests.cs | 5 ++- 11 files changed, 27 insertions(+), 83 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationsController.cs b/src/Api/AdminConsole/Controllers/OrganizationsController.cs index 202c19f5da7a..4aff3c3e0fac 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationsController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationsController.cs @@ -121,10 +121,8 @@ public async Task> GetUser() var organizations = await _organizationUserRepository.GetManyDetailsByUserAsync(userId, OrganizationUserStatusType.Confirmed); -#nullable enable var organizationManagingActiveUser = await _userService.GetOrganizationsManagingUserAsync(userId); - var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id); -#nullable disable + var organizationIdsManagingActiveUser = organizationManagingActiveUser.Select(o => o.Id); var responses = organizations.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); return new ListResponseModel(responses); diff --git a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs index 4a9c9c8698af..a573bfb8d167 100644 --- a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs +++ b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs @@ -17,10 +17,7 @@ public ProfileOrganizationResponseModel(string str) : base(str) { } public ProfileOrganizationResponseModel( OrganizationUserOrganizationDetails organization, -#nullable enable - IEnumerable? organizationIdsManagingUser -#nullable disable - ) + IEnumerable organizationIdsManagingUser) : this("profileOrganization") { Id = organization.OrganizationId; @@ -70,7 +67,7 @@ public ProfileOrganizationResponseModel( AccessSecretsManager = organization.AccessSecretsManager; LimitCollectionCreationDeletion = organization.LimitCollectionCreationDeletion; AllowAdminAccessToAllCollectionItems = organization.AllowAdminAccessToAllCollectionItems; - UserIsManagedByOrganization = organizationIdsManagingUser?.Contains(organization.OrganizationId); + UserIsManagedByOrganization = organizationIdsManagingUser.Contains(organization.OrganizationId); if (organization.SsoConfig != null) { @@ -135,7 +132,9 @@ public ProfileOrganizationResponseModel( /// /// An organization manages a user if the user's email domain is verified by the organization and the user is a member of it. /// The organization must be enabled and able to have verified domains. - /// This property is null if the Account Deprovisioning feature flag is disabled. /// - public bool? UserIsManagedByOrganization { get; set; } + /// + /// False if the Account Deprovisioning feature flag is disabled. + /// + public bool UserIsManagedByOrganization { get; set; } } diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index bb57e1416102..a0c01752a8ec 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -443,10 +443,7 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user); - -#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); -#nullable disable var response = new ProfileResponseModel(user, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails, twoFactorEnabled, @@ -460,10 +457,7 @@ public async Task> GetOrgani var userId = _userService.GetProperUserId(User); var organizationUserDetails = await _organizationUserRepository.GetManyDetailsByUserAsync(userId.Value, OrganizationUserStatusType.Confirmed); - -#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(userId.Value); -#nullable disable var responseData = organizationUserDetails.Select(o => new ProfileOrganizationResponseModel(o, organizationIdsManagingActiveUser)); return new ListResponseModel(responseData); @@ -483,10 +477,7 @@ public async Task PutProfile([FromBody] UpdateProfileReque var twoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var hasPremiumFromOrg = await _userService.HasPremiumFromOrganization(user); - -#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); -#nullable disable var response = new ProfileResponseModel(user, null, null, null, twoFactorEnabled, hasPremiumFromOrg, organizationIdsManagingActiveUser); return response; @@ -505,10 +496,7 @@ public async Task PutAvatar([FromBody] UpdateAvatarRequest var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); - -#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); -#nullable disable var response = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); return response; @@ -661,10 +649,7 @@ public async Task PostPremium(PremiumRequestModel model) var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); - -#nullable enable var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user.Id); -#nullable disable var profile = new ProfileResponseModel(user, null, null, null, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser); return new PaymentResponseModel @@ -954,11 +939,9 @@ public async Task VerifyOTP([FromBody] VerifyOTPRequestModel model) } } -#nullable enable - private async Task?> GetOrganizationIdsManagingUserAsync(Guid userId) + private async Task> GetOrganizationIdsManagingUserAsync(Guid userId) { var organizationManagingUser = await _userService.GetOrganizationsManagingUserAsync(userId); - return organizationManagingUser?.Select(o => o.Id); + return organizationManagingUser.Select(o => o.Id); } -#nullable disable } diff --git a/src/Api/Billing/Controllers/OrganizationsController.cs b/src/Api/Billing/Controllers/OrganizationsController.cs index bab5fe33d1d6..75ae2fb89c4a 100644 --- a/src/Api/Billing/Controllers/OrganizationsController.cs +++ b/src/Api/Billing/Controllers/OrganizationsController.cs @@ -201,10 +201,8 @@ await addSecretsManagerSubscriptionCommand.SignUpAsync(organization, model.Addit var organizationDetails = await organizationUserRepository.GetDetailsByUserAsync(userId, organization.Id, OrganizationUserStatusType.Confirmed); -#nullable enable var organizationManagingActiveUser = await userService.GetOrganizationsManagingUserAsync(userId); - var organizationIdsManagingActiveUser = organizationManagingActiveUser?.Select(o => o.Id); -#nullable disable + var organizationIdsManagingActiveUser = organizationManagingActiveUser.Select(o => o.Id); return new ProfileOrganizationResponseModel(organizationDetails, organizationIdsManagingActiveUser); } diff --git a/src/Api/Models/Response/ProfileResponseModel.cs b/src/Api/Models/Response/ProfileResponseModel.cs index 0df28961d629..a6ed4ebfa2e7 100644 --- a/src/Api/Models/Response/ProfileResponseModel.cs +++ b/src/Api/Models/Response/ProfileResponseModel.cs @@ -15,10 +15,7 @@ public ProfileResponseModel(User user, IEnumerable providerUserOrganizationDetails, bool twoFactorEnabled, bool premiumFromOrganization, -#nullable enable - IEnumerable? organizationIdsManagingUser -#nullable disable - ) : base("profile") + IEnumerable organizationIdsManagingUser) : base("profile") { if (user == null) { diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 06a4224e67b4..d8d918a2fce5 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -912,7 +912,7 @@ public async Task PostPurge([FromBody] SecretVerificationRequestModel model, str // 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) == true) + && await _userService.IsManagedByAnyOrganizationAsync(user.Id)) { throw new BadRequestException("Accounts managed by an organization cannot be purged."); } diff --git a/src/Api/Vault/Controllers/SyncController.cs b/src/Api/Vault/Controllers/SyncController.cs index 0f8fc1c28ca9..853320ec6812 100644 --- a/src/Api/Vault/Controllers/SyncController.cs +++ b/src/Api/Vault/Controllers/SyncController.cs @@ -1,5 +1,4 @@ using Bit.Api.Vault.Models.Response; -using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.AdminConsole.Repositories; @@ -7,7 +6,6 @@ using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.Models.Data; -using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Settings; @@ -95,37 +93,12 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id, var userTwoFactorEnabled = await _userService.TwoFactorIsEnabledAsync(user); var userHasPremiumFromOrganization = await _userService.HasPremiumFromOrganization(user); - -#nullable enable - var organizationIdsManagingActiveUser = await GetOrganizationIdsManagingUserAsync(user, organizationUserDetails); -#nullable disable + var organizationManagingActiveUser = await _userService.GetOrganizationsManagingUserAsync(user.Id); + var organizationIdsManagingActiveUser = organizationManagingActiveUser.Select(o => o.Id); var response = new SyncResponseModel(_globalSettings, user, userTwoFactorEnabled, userHasPremiumFromOrganization, organizationIdsManagingActiveUser, organizationUserDetails, providerUserDetails, providerUserOrganizationDetails, folders, collections, ciphers, collectionCiphersGroupDict, excludeDomains, policies, sends); return response; } - -#nullable enable - /// - /// Gets the IDs of the organizations that manage a user. - /// - /// - /// Organizations are considered to manage a user if the user's email domain is verified by the organization and the user is a member of it. - /// The organization must be enabled and able to have verified domains. - /// - private async Task?> GetOrganizationIdsManagingUserAsync(User user, IEnumerable organizationUserDetails) - { - // Account deprovisioning must be enabled and organizations must be enabled and able to have verified domains. - // TODO: Replace "UseSso" with a new organization ability like "UseOrganizationDomains" (PM-11622). - if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) - || !organizationUserDetails.Any(o => o is { Enabled: true, UseSso: true })) - { - return null; - } - - var organizationsManagingUser = await _userService.GetOrganizationsManagingUserAsync(user.Id); - return organizationsManagingUser?.Select(o => o.Id); - } -#nullable disable } diff --git a/src/Api/Vault/Models/Response/SyncResponseModel.cs b/src/Api/Vault/Models/Response/SyncResponseModel.cs index 725e22fe583f..ce5f4562d8d8 100644 --- a/src/Api/Vault/Models/Response/SyncResponseModel.cs +++ b/src/Api/Vault/Models/Response/SyncResponseModel.cs @@ -21,9 +21,7 @@ public SyncResponseModel( User user, bool userTwoFactorEnabled, bool userHasPremiumFromOrganization, -#nullable enable - IEnumerable? organizationIdsManagingUser, -#nullable disable + IEnumerable organizationIdsManagingUser, IEnumerable organizationUserDetails, IEnumerable providerUserDetails, IEnumerable providerUserOrganizationDetails, diff --git a/src/Core/Services/IUserService.cs b/src/Core/Services/IUserService.cs index 5945b2414d53..2d781cbe2067 100644 --- a/src/Core/Services/IUserService.cs +++ b/src/Core/Services/IUserService.cs @@ -88,7 +88,6 @@ Task UpdatePasswordHash(User user, string newPassword, /// Task IsLegacyUser(string userId); -#nullable enable /// /// Indicates if the user is managed by any organization. /// @@ -97,14 +96,16 @@ Task UpdatePasswordHash(User user, string newPassword, /// The organization must be enabled and able to have verified domains. /// /// - /// This returns a nullable object because if the Account Deprovisioning feature flag is disabled, we should return null. + /// False if the Account Deprovisioning feature flag is disabled. /// - Task IsManagedByAnyOrganizationAsync(Guid userId); + Task IsManagedByAnyOrganizationAsync(Guid userId); /// /// Gets the organizations that manage the user. /// + /// + /// An empty collection if the Account Deprovisioning feature flag is disabled. + /// /// - Task?> GetOrganizationsManagingUserAsync(Guid userId); -#nullable disable + Task> GetOrganizationsManagingUserAsync(Guid userId); } diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index 3e80dfd1534a..1289683ffbff 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -1263,18 +1263,17 @@ public async Task IsLegacyUser(string userId) return IsLegacyUser(user); } -#nullable enable - public async Task IsManagedByAnyOrganizationAsync(Guid userId) + public async Task IsManagedByAnyOrganizationAsync(Guid userId) { var managingOrganizations = await GetOrganizationsManagingUserAsync(userId); - return managingOrganizations?.Any(); + return managingOrganizations.Any(); } - public async Task?> GetOrganizationsManagingUserAsync(Guid userId) + public async Task> GetOrganizationsManagingUserAsync(Guid userId) { if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) { - return null; + return Enumerable.Empty(); } // Get all organizations that have verified the user's email domain. @@ -1285,7 +1284,6 @@ public async Task IsLegacyUser(string userId) // Verified domains were tied to SSO, so we currently check the "UseSso" organization ability. return organizationsWithVerifiedUserEmailDomain.Where(organization => organization is { Enabled: true, UseSso: true }); } -#nullable disable /// public static bool IsLegacyUser(User user) diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 29cec5e8877b..b2d3c51e9004 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -279,7 +279,7 @@ await tokenProvider } [Theory, BitAutoData] - public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningDisabled_ReturnsNull( + public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningDisabled_ReturnsFalse( SutProvider sutProvider, Guid userId) { sutProvider.GetDependency() @@ -287,10 +287,9 @@ public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningDisab .Returns(false); var result = await sutProvider.Sut.IsManagedByAnyOrganizationAsync(userId); - Assert.Null(result); + Assert.False(result); } - [Theory, BitAutoData] public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningEnabled_WithManagingEnabledOrganization_ReturnsTrue( SutProvider sutProvider, Guid userId, Organization organization) From cfec28ee4215e8ba05cc0c060a293db4ea054758 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 15 Oct 2024 11:59:50 +0100 Subject: [PATCH 08/15] Update error message when unable to purge vault for managed account --- src/Api/Vault/Controllers/CiphersController.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index d8d918a2fce5..09ade4d0d47d 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -914,7 +914,7 @@ public async Task PostPurge([FromBody] SecretVerificationRequestModel model, str if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && await _userService.IsManagedByAnyOrganizationAsync(user.Id)) { - throw new BadRequestException("Accounts managed by an organization cannot be purged."); + throw new BadRequestException("Cannot purge accounts owned by an organization. Contact your organization administrator for additional details."); } if (string.IsNullOrWhiteSpace(organizationId)) From 7a1cdaa209d316383168a5db1298ce277b6c5b4e Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 15 Oct 2024 12:15:36 +0100 Subject: [PATCH 09/15] Update error message when unable to change email for managed account --- src/Api/Auth/Controllers/AccountsController.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index fc5d4e479115..d9dfbafc7975 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -150,9 +150,9 @@ public async Task PostEmailToken([FromBody] EmailTokenRequestModel model) // 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) == true) + && await _userService.IsManagedByAnyOrganizationAsync(user.Id)) { - throw new BadRequestException("Accounts managed by an organization cannot change email."); + 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); @@ -174,9 +174,9 @@ public async Task PostEmail([FromBody] EmailRequestModel model) // 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) == true) + && await _userService.IsManagedByAnyOrganizationAsync(user.Id)) { - throw new BadRequestException("Accounts managed by an organization cannot change email."); + 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, From 6c9fe980252d0363795e947900c7da86da99bedb Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Fri, 18 Oct 2024 11:31:38 +0100 Subject: [PATCH 10/15] Update expected error messages on unit tests --- test/Api.Test/Auth/Controllers/AccountsControllerTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index bddf7cf18715..4127c92eedb6 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -194,7 +194,7 @@ public async Task PostEmailToken_WithAccountDeprovisioningEnabled_WhenUserIsMana () => _sut.PostEmailToken(new EmailTokenRequestModel()) ); - Assert.Equal("Accounts managed by an organization cannot change email.", result.Message); + Assert.Equal("Cannot change emails for accounts owned by an organization. Contact your organization administrator for additional details.", result.Message); } [Fact] @@ -260,7 +260,7 @@ public async Task PostEmail_WithAccountDeprovisioningEnabled_WhenUserIsManagedBy () => _sut.PostEmail(new EmailRequestModel()) ); - Assert.Equal("Accounts managed by an organization cannot change email.", result.Message); + Assert.Equal("Cannot change emails for accounts owned by an organization. Contact your organization administrator for additional details.", result.Message); } [Fact] From 28947af7bb8721989cec3040b459c075296a0758 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Wed, 23 Oct 2024 15:40:12 +0100 Subject: [PATCH 11/15] Add TestFeatureService to Api.IntegrationTest.Helpers and use it on ApiApplicationFactory to be able to enable specific features for each test --- .../Factories/ApiApplicationFactory.cs | 15 ++++++++++++++- .../Helpers/TestFeatureService.cs | 12 ++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 test/Api.IntegrationTest/Helpers/TestFeatureService.cs diff --git a/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs b/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs index 230f0bcf0812..bf644434b603 100644 --- a/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs +++ b/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs @@ -1,4 +1,6 @@ -using Bit.Identity.Models.Request.Accounts; +using Bit.Api.IntegrationTest.Helpers; +using Bit.Core.Services; +using Bit.Identity.Models.Request.Accounts; using Bit.IntegrationTestCommon.Factories; using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.AspNetCore.TestHost; @@ -12,6 +14,7 @@ public class ApiApplicationFactory : WebApplicationFactoryBase { private readonly IdentityApplicationFactory _identityApplicationFactory; private const string _connectionString = "DataSource=:memory:"; + private readonly Dictionary _features = new Dictionary(); public ApiApplicationFactory() { @@ -36,9 +39,19 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) { options.BackchannelHttpHandler = _identityApplicationFactory.Server.CreateHandler(); }); + + // Remove the default IFeatureService implementation + services.Remove(services.First(sd => sd.ServiceType == typeof(IFeatureService))); + services.AddSingleton(sp => new TestFeatureService(_features)); }); } + public ApiApplicationFactory WithFeature(string feature, object value) + { + _features[feature] = value; + return this; + } + /// /// Helper for registering and logging in to a new account /// diff --git a/test/Api.IntegrationTest/Helpers/TestFeatureService.cs b/test/Api.IntegrationTest/Helpers/TestFeatureService.cs new file mode 100644 index 000000000000..f40245c0c59f --- /dev/null +++ b/test/Api.IntegrationTest/Helpers/TestFeatureService.cs @@ -0,0 +1,12 @@ +using Bit.Core.Services; + +namespace Bit.Api.IntegrationTest.Helpers; + +public class TestFeatureService(Dictionary features) : IFeatureService +{ + public bool IsEnabled(string feature, bool defaultValue = false) => features.TryGetValue(feature, out var value) ? (bool)value : defaultValue; + public bool IsOnline() => true; + public int GetIntVariation(string feature, int defaultValue = 0) => features.TryGetValue(feature, out var value) ? (int)value : defaultValue; + public string GetStringVariation(string feature, string defaultValue = "") => features.TryGetValue(feature, out var value) ? (string)value : defaultValue; + public Dictionary GetAll() => features; +} From 4ae6abc349ac3db74ff0f650d01066c893d4bd53 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Wed, 23 Oct 2024 15:40:40 +0100 Subject: [PATCH 12/15] Add CreateVerifiedDomainAsync method to OrganizationTestHelpers --- .../Helpers/OrganizationTestHelpers.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/Api.IntegrationTest/Helpers/OrganizationTestHelpers.cs b/test/Api.IntegrationTest/Helpers/OrganizationTestHelpers.cs index 83b345e78443..64f719e82ecf 100644 --- a/test/Api.IntegrationTest/Helpers/OrganizationTestHelpers.cs +++ b/test/Api.IntegrationTest/Helpers/OrganizationTestHelpers.cs @@ -105,4 +105,22 @@ public static async Task CreateUserAsync( return (email, organizationUser); } + + /// + /// Creates a VerifiedDomain for the specified organization. + /// + public static async Task CreateVerifiedDomainAsync(ApiApplicationFactory factory, Guid organizationId, string domain) + { + var organizationDomainRepository = factory.GetService(); + + var verifiedDomain = new OrganizationDomain + { + OrganizationId = organizationId, + DomainName = domain, + Txt = "btw+test18383838383" + }; + verifiedDomain.SetVerifiedDate(); + + await organizationDomainRepository.CreateAsync(verifiedDomain); + } } From 0d4db113a110a03e566554fb729d04d3801dc45c Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Wed, 23 Oct 2024 15:41:21 +0100 Subject: [PATCH 13/15] Add tests to AccountsControllerTest to prevent changing email for managed accounts --- .../Controllers/AccountsControllerTest.cs | 84 ++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs index b6a0ccbedb34..ec18ebc826cb 100644 --- a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs +++ b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs @@ -1,6 +1,13 @@ -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 Xunit; namespace Bit.Api.IntegrationTest.Controllers; @@ -35,4 +42,79 @@ 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.WithFeature(FeatureFlagKeys.AccountDeprovisioning, true).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() + { + // 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; + } } From 8e2e79002252a295a7a58d8f975675d7f5807908 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Thu, 24 Oct 2024 10:49:48 +0100 Subject: [PATCH 14/15] Remove setting the feature flag value in ApiApplicationFactory and set it on AccountsControllerTest --- .../Controllers/AccountsControllerTest.cs | 7 ++++++- .../Factories/ApiApplicationFactory.cs | 15 +-------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs index ec18ebc826cb..6dd7f42c6336 100644 --- a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs +++ b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs @@ -8,6 +8,8 @@ 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; @@ -49,7 +51,7 @@ public async Task PostEmailToken_WhenAccountDeprovisioningEnabled_WithManagedAcc var email = await SetupOrganizationManagedAccount(); var tokens = await _factory.LoginAsync(email); - var client = _factory.WithFeature(FeatureFlagKeys.AccountDeprovisioning, true).CreateClient(); + var client = _factory.CreateClient(); var model = new EmailTokenRequestModel { @@ -100,6 +102,9 @@ public async Task PostEmail_WhenAccountDeprovisioningEnabled_WithManagedAccount_ private async Task SetupOrganizationManagedAccount() { + _factory.SubstituteService(featureService => + featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning).Returns(true)); + // Create the owner account var ownerEmail = $"{Guid.NewGuid()}@bitwarden.com"; await _factory.LoginWithNewAccount(ownerEmail); diff --git a/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs b/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs index bf644434b603..230f0bcf0812 100644 --- a/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs +++ b/test/Api.IntegrationTest/Factories/ApiApplicationFactory.cs @@ -1,6 +1,4 @@ -using Bit.Api.IntegrationTest.Helpers; -using Bit.Core.Services; -using Bit.Identity.Models.Request.Accounts; +using Bit.Identity.Models.Request.Accounts; using Bit.IntegrationTestCommon.Factories; using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.AspNetCore.TestHost; @@ -14,7 +12,6 @@ public class ApiApplicationFactory : WebApplicationFactoryBase { private readonly IdentityApplicationFactory _identityApplicationFactory; private const string _connectionString = "DataSource=:memory:"; - private readonly Dictionary _features = new Dictionary(); public ApiApplicationFactory() { @@ -39,19 +36,9 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) { options.BackchannelHttpHandler = _identityApplicationFactory.Server.CreateHandler(); }); - - // Remove the default IFeatureService implementation - services.Remove(services.First(sd => sd.ServiceType == typeof(IFeatureService))); - services.AddSingleton(sp => new TestFeatureService(_features)); }); } - public ApiApplicationFactory WithFeature(string feature, object value) - { - _features[feature] = value; - return this; - } - /// /// Helper for registering and logging in to a new account /// From e2c3b66be9052f94777f94c15637b04e71f5620d Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Fri, 25 Oct 2024 11:03:30 +0100 Subject: [PATCH 15/15] Remove TestFeatureService class from Api.IntegrationTest.Helpers --- .../Helpers/TestFeatureService.cs | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 test/Api.IntegrationTest/Helpers/TestFeatureService.cs diff --git a/test/Api.IntegrationTest/Helpers/TestFeatureService.cs b/test/Api.IntegrationTest/Helpers/TestFeatureService.cs deleted file mode 100644 index f40245c0c59f..000000000000 --- a/test/Api.IntegrationTest/Helpers/TestFeatureService.cs +++ /dev/null @@ -1,12 +0,0 @@ -using Bit.Core.Services; - -namespace Bit.Api.IntegrationTest.Helpers; - -public class TestFeatureService(Dictionary features) : IFeatureService -{ - public bool IsEnabled(string feature, bool defaultValue = false) => features.TryGetValue(feature, out var value) ? (bool)value : defaultValue; - public bool IsOnline() => true; - public int GetIntVariation(string feature, int defaultValue = 0) => features.TryGetValue(feature, out var value) ? (int)value : defaultValue; - public string GetStringVariation(string feature, string defaultValue = "") => features.TryGetValue(feature, out var value) ? (string)value : defaultValue; - public Dictionary GetAll() => features; -}