From fbfabf2651276242b35e06706657d19a5a0450bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Fri, 10 Jan 2025 14:45:09 +0000 Subject: [PATCH] [PM-15547] Fix two-factor authentication revocation logic and update related tests (#5246) * Fix two-factor authentication revocation logic and update related tests * Refine test for RevokeNonCompliantOrganizationUserCommand to assert single user revocation --- .../Services/Implementations/UserService.cs | 5 +- ...onCompliantOrganizationUserCommandTests.cs | 2 +- test/Core.Test/Services/UserServiceTests.cs | 71 ++++++++++++++++--- 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index 4944dfe9e7..78da7b42e3 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -1372,17 +1372,16 @@ public class UserService : UserManager, IUserService, IDisposable private async Task CheckPoliciesOnTwoFactorRemovalAsync(User user) { var twoFactorPolicies = await _policyService.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication); - var organizationsManagingUser = await GetOrganizationsManagingUserAsync(user.Id); var removeOrgUserTasks = twoFactorPolicies.Select(async p => { var organization = await _organizationRepository.GetByIdAsync(p.OrganizationId); - if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && organizationsManagingUser.Any(o => o.Id == p.OrganizationId)) + if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) { await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync( new RevokeOrganizationUsersRequest( p.OrganizationId, - [new OrganizationUserUserDetails { UserId = user.Id, OrganizationId = p.OrganizationId }], + [new OrganizationUserUserDetails { Id = p.OrganizationUserId, OrganizationId = p.OrganizationId }], new SystemUser(EventSystemUser.TwoFactorDisabled))); await _mailService.SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(organization.DisplayName(), user.Email); } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index 3653cd27d7..0ccad9e5c7 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -162,7 +162,7 @@ public class RevokeNonCompliantOrganizationUserCommandTests await sutProvider.GetDependency() .Received(1) - .RevokeManyByIdAsync(Arg.Any>()); + .RevokeManyByIdAsync(Arg.Is>(x => x.Count() == 1 && x.Contains(userToRevoke.Id))); Assert.True(result.Success); diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index a07cc1907f..d8a0ade1fa 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -454,8 +454,10 @@ public class UserServiceTests } [Theory, BitAutoData] - public async Task DisableTwoFactorProviderAsync_WithAccountDeprovisioningEnabled_WhenOrganizationHas2FAPolicyEnabled_WhenUserIsManaged_DisablingAllProviders_RemovesOrRevokesUserAndSendsEmail( - SutProvider sutProvider, User user, Organization organization1, Organization organization2) + public async Task DisableTwoFactorProviderAsync_WithAccountDeprovisioningEnabled_WhenOrganizationHas2FAPolicyEnabled_DisablingAllProviders_RevokesUserAndSendsEmail( + SutProvider sutProvider, User user, + Organization organization1, Guid organizationUserId1, + Organization organization2, Guid organizationUserId2) { // Arrange user.SetTwoFactorProviders(new Dictionary @@ -464,6 +466,7 @@ public class UserServiceTests }); organization1.Enabled = organization2.Enabled = true; organization1.UseSso = organization2.UseSso = true; + sutProvider.GetDependency() .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) .Returns(true); @@ -474,12 +477,14 @@ public class UserServiceTests new OrganizationUserPolicyDetails { OrganizationId = organization1.Id, + OrganizationUserId = organizationUserId1, PolicyType = PolicyType.TwoFactorAuthentication, PolicyEnabled = true }, new OrganizationUserPolicyDetails { OrganizationId = organization2.Id, + OrganizationUserId = organizationUserId2, PolicyType = PolicyType.TwoFactorAuthentication, PolicyEnabled = true } @@ -490,9 +495,6 @@ public class UserServiceTests sutProvider.GetDependency() .GetByIdAsync(organization2.Id) .Returns(organization2); - sutProvider.GetDependency() - .GetByVerifiedUserEmailDomainAsync(user.Id) - .Returns(new[] { organization1 }); var expectedSavedProviders = JsonHelpers.LegacySerialize(new Dictionary(), JsonHelpers.LegacyEnumKeyResolver); // Act @@ -506,24 +508,71 @@ public class UserServiceTests .Received(1) .LogUserEventAsync(user.Id, EventType.User_Disabled2fa); - // Revoke the user from the first organization because they are managed by it + // Revoke the user from the first organization await sutProvider.GetDependency() .Received(1) .RevokeNonCompliantOrganizationUsersAsync( Arg.Is(r => r.OrganizationId == organization1.Id && - r.OrganizationUsers.First().UserId == user.Id && + r.OrganizationUsers.First().Id == organizationUserId1 && r.OrganizationUsers.First().OrganizationId == organization1.Id)); await sutProvider.GetDependency() .Received(1) .SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(organization1.DisplayName(), user.Email); - // Remove the user from the second organization because they are not managed by it - await sutProvider.GetDependency() + // Remove the user from the second organization + await sutProvider.GetDependency() .Received(1) - .RemoveUserAsync(organization2.Id, user.Id); + .RevokeNonCompliantOrganizationUsersAsync( + Arg.Is(r => r.OrganizationId == organization2.Id && + r.OrganizationUsers.First().Id == organizationUserId2 && + r.OrganizationUsers.First().OrganizationId == organization2.Id)); await sutProvider.GetDependency() .Received(1) - .SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization2.DisplayName(), user.Email); + .SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(organization2.DisplayName(), user.Email); + } + + [Theory, BitAutoData] + public async Task DisableTwoFactorProviderAsync_WithAccountDeprovisioningEnabled_UserHasOneProviderEnabled_DoesNotRemoveUserFromOrganization( + SutProvider sutProvider, User user, Organization organization) + { + // Arrange + user.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new() { Enabled = true }, + [TwoFactorProviderType.Remember] = new() { Enabled = true } + }); + sutProvider.GetDependency() + .GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication) + .Returns( + [ + new OrganizationUserPolicyDetails + { + OrganizationId = organization.Id, + PolicyType = PolicyType.TwoFactorAuthentication, + PolicyEnabled = true + } + ]); + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); + var expectedSavedProviders = JsonHelpers.LegacySerialize(new Dictionary + { + [TwoFactorProviderType.Remember] = new() { Enabled = true } + }, JsonHelpers.LegacyEnumKeyResolver); + + // Act + await sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email); + + // Assert + await sutProvider.GetDependency() + .Received(1) + .ReplaceAsync(Arg.Is(u => u.Id == user.Id && u.TwoFactorProviders == expectedSavedProviders)); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .RevokeNonCompliantOrganizationUsersAsync(default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(default, default); } [Theory, BitAutoData]