Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-15547] Revoke managed user on 2FA removal if enforced by organization policy #5124

Merged
merged 3 commits into from
Dec 13, 2024
Merged

[PM-15547] Revoke managed user on 2FA removal if enforced by organization policy #5124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99f02b3
Revoke managed user on 2FA removal if enforced by organization policy
r-tome Dec 6, 2024
82a360c
Rename TwoFactorDisabling to TwoFactorDisabled in EventSystemUser enum
r-tome Dec 11, 2024
0dbcb16
Merge branch 'main' into ac/pm-15547/disabling-two-factor-should-revo…
r-tome Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Core/AdminConsole/Enums/EventSystemUser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ public enum EventSystemUser : byte
SCIM = 1,
DomainVerification = 2,
PublicApi = 3,
TwoFactorDisabled = 4,
}
28 changes: 24 additions & 4 deletions src/Core/Services/Implementations/UserService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using System.Security.Claims;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums;
using Bit.Core.AdminConsole.Models.Data;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.AdminConsole.Services;
using Bit.Core.Auth.Enums;
Expand All @@ -14,6 +16,7 @@
using Bit.Core.Enums;
using Bit.Core.Exceptions;
using Bit.Core.Models.Business;
using Bit.Core.Models.Data.Organizations.OrganizationUsers;
using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.Repositories;
using Bit.Core.Settings;
Expand Down Expand Up @@ -67,6 +70,7 @@
private readonly IFeatureService _featureService;
private readonly IPremiumUserBillingService _premiumUserBillingService;
private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand;
private readonly IRevokeNonCompliantOrganizationUserCommand _revokeNonCompliantOrganizationUserCommand;

public UserService(
IUserRepository userRepository,
Expand Down Expand Up @@ -101,7 +105,8 @@
IDataProtectorTokenFactory<OrgUserInviteTokenable> orgUserInviteTokenDataFactory,
IFeatureService featureService,
IPremiumUserBillingService premiumUserBillingService,
IRemoveOrganizationUserCommand removeOrganizationUserCommand)
IRemoveOrganizationUserCommand removeOrganizationUserCommand,
IRevokeNonCompliantOrganizationUserCommand revokeNonCompliantOrganizationUserCommand)
: base(
store,
optionsAccessor,
Expand Down Expand Up @@ -142,6 +147,7 @@
_featureService = featureService;
_premiumUserBillingService = premiumUserBillingService;
_removeOrganizationUserCommand = removeOrganizationUserCommand;
_revokeNonCompliantOrganizationUserCommand = revokeNonCompliantOrganizationUserCommand;
}

public Guid? GetProperUserId(ClaimsPrincipal principal)
Expand All @@ -167,7 +173,7 @@
return null;
}

_currentContext.User = await _userRepository.GetByIdAsync(userIdGuid);

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

View workflow job for this annotation

GitHub Actions / Quality scan 

'_currentContext' is null on at least one execution path. (https://rules.sonarsource.com/csharp/RSPEC-2259)
return _currentContext.User;
}

Expand All @@ -178,7 +184,7 @@
return _currentContext.User;
}

_currentContext.User = await _userRepository.GetByIdAsync(userId);

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

View workflow job for this annotation

GitHub Actions / Quality scan 

'_currentContext' is null on at least one execution path. (https://rules.sonarsource.com/csharp/RSPEC-2259)
return _currentContext.User;
}

Expand Down Expand Up @@ -964,7 +970,7 @@
}
catch when (!_globalSettings.SelfHosted)
{
await paymentService.CancelAndRecoverChargesAsync(user);

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

View workflow job for this annotation

GitHub Actions / Quality scan 

'paymentService' is null on at least one execution path. (https://rules.sonarsource.com/csharp/RSPEC-2259)
throw;
}
return new Tuple<bool, string>(string.IsNullOrWhiteSpace(paymentIntentClientSecret),
Expand Down Expand Up @@ -1355,13 +1361,27 @@
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 =>
{
await _removeOrganizationUserCommand.RemoveUserAsync(p.OrganizationId, user.Id);
var organization = await _organizationRepository.GetByIdAsync(p.OrganizationId);
await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(
organization.DisplayName(), user.Email);
if (_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning) && organizationsManagingUser.Any(o => o.Id == p.OrganizationId))
JimmyVo16 marked this conversation as resolved.
Show resolved Hide resolved
{
await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync(
new RevokeOrganizationUsersRequest(
p.OrganizationId,
[new OrganizationUserUserDetails { UserId = user.Id, OrganizationId = p.OrganizationId }],
new SystemUser(EventSystemUser.TwoFactorDisabled)));
await _mailService.SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(organization.DisplayName(), user.Email);
}
else
{
await _removeOrganizationUserCommand.RemoveUserAsync(p.OrganizationId, user.Id);
await _mailService.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(
organization.DisplayName(), user.Email);
}

}).ToArray();

await Task.WhenAll(removeOrgUserTasks);
Expand Down
171 changes: 170 additions & 1 deletion test/Core.Test/Services/UserServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using System.Security.Claims;
using System.Text.Json;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.AdminConsole.Services;
using Bit.Core.Auth.Enums;
Expand All @@ -10,13 +12,16 @@
using Bit.Core.Billing.Services;
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Models.Business;
using Bit.Core.Models.Data.Organizations;
using Bit.Core.Models.Data.Organizations.OrganizationUsers;
using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Settings;
using Bit.Core.Tools.Services;
using Bit.Core.Utilities;
using Bit.Core.Vault.Repositories;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
Expand Down Expand Up @@ -268,7 +273,8 @@ public async Task VerifySecretAsync_Works(
new FakeDataProtectorTokenFactory<OrgUserInviteTokenable>(),
sutProvider.GetDependency<IFeatureService>(),
sutProvider.GetDependency<IPremiumUserBillingService>(),
sutProvider.GetDependency<IRemoveOrganizationUserCommand>()
sutProvider.GetDependency<IRemoveOrganizationUserCommand>(),
sutProvider.GetDependency<IRevokeNonCompliantOrganizationUserCommand>()
);

var actualIsVerified = await sut.VerifySecretAsync(user, secret);
Expand Down Expand Up @@ -353,6 +359,169 @@ public async Task IsManagedByAnyOrganizationAsync_WithAccountDeprovisioningEnabl
Assert.False(result);
}

[Theory, BitAutoData]
public async Task DisableTwoFactorProviderAsync_WhenOrganizationHas2FAPolicyEnabled_DisablingAllProviders_RemovesUserFromOrganizationAndSendsEmail(
SutProvider<UserService> sutProvider, User user, Organization organization)
{
// Arrange
user.SetTwoFactorProviders(new Dictionary<TwoFactorProviderType, TwoFactorProvider>
{
[TwoFactorProviderType.Email] = new() { Enabled = true }
});
sutProvider.GetDependency<IPolicyService>()
.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication)
.Returns(
[
new OrganizationUserPolicyDetails
{
OrganizationId = organization.Id,
PolicyType = PolicyType.TwoFactorAuthentication,
PolicyEnabled = true
}
]);
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization.Id)
.Returns(organization);
var expectedSavedProviders = JsonHelpers.LegacySerialize(new Dictionary<TwoFactorProviderType, TwoFactorProvider>(), JsonHelpers.LegacyEnumKeyResolver);

// Act
await sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email);

// Assert
await sutProvider.GetDependency<IUserRepository>()
.Received(1)
.ReplaceAsync(Arg.Is<User>(u => u.Id == user.Id && u.TwoFactorProviders == expectedSavedProviders));
await sutProvider.GetDependency<IEventService>()
.Received(1)
.LogUserEventAsync(user.Id, EventType.User_Disabled2fa);
await sutProvider.GetDependency<IRemoveOrganizationUserCommand>()
.Received(1)
.RemoveUserAsync(organization.Id, user.Id);
await sutProvider.GetDependency<IMailService>()
.Received(1)
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization.DisplayName(), user.Email);
}

[Theory, BitAutoData]
public async Task DisableTwoFactorProviderAsync_WhenOrganizationHas2FAPolicyEnabled_UserHasOneProviderEnabled_DoesNotRemoveUserFromOrganization(
SutProvider<UserService> sutProvider, User user, Organization organization)
{
// Arrange
user.SetTwoFactorProviders(new Dictionary<TwoFactorProviderType, TwoFactorProvider>
{
[TwoFactorProviderType.Email] = new() { Enabled = true },
[TwoFactorProviderType.Remember] = new() { Enabled = true }
});
sutProvider.GetDependency<IPolicyService>()
.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication)
.Returns(
[
new OrganizationUserPolicyDetails
{
OrganizationId = organization.Id,
PolicyType = PolicyType.TwoFactorAuthentication,
PolicyEnabled = true
}
]);
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization.Id)
.Returns(organization);
var expectedSavedProviders = JsonHelpers.LegacySerialize(new Dictionary<TwoFactorProviderType, TwoFactorProvider>
{
[TwoFactorProviderType.Remember] = new() { Enabled = true }
}, JsonHelpers.LegacyEnumKeyResolver);

// Act
await sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email);

// Assert
await sutProvider.GetDependency<IUserRepository>()
.Received(1)
.ReplaceAsync(Arg.Is<User>(u => u.Id == user.Id && u.TwoFactorProviders == expectedSavedProviders));
await sutProvider.GetDependency<IEventService>()
.Received(1)
.LogUserEventAsync(user.Id, EventType.User_Disabled2fa);
await sutProvider.GetDependency<IRemoveOrganizationUserCommand>()
.DidNotReceiveWithAnyArgs()
.RemoveUserAsync(default, default);
await sutProvider.GetDependency<IMailService>()
.DidNotReceiveWithAnyArgs()
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(default, default);
}

[Theory, BitAutoData]
public async Task DisableTwoFactorProviderAsync_WithAccountDeprovisioningEnabled_WhenOrganizationHas2FAPolicyEnabled_WhenUserIsManaged_DisablingAllProviders_RemovesOrRevokesUserAndSendsEmail(
SutProvider<UserService> sutProvider, User user, Organization organization1, Organization organization2)
{
// Arrange
user.SetTwoFactorProviders(new Dictionary<TwoFactorProviderType, TwoFactorProvider>
{
[TwoFactorProviderType.Email] = new() { Enabled = true }
});
organization1.Enabled = organization2.Enabled = true;
organization1.UseSso = organization2.UseSso = true;
sutProvider.GetDependency<IFeatureService>()
.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)
.Returns(true);
sutProvider.GetDependency<IPolicyService>()
.GetPoliciesApplicableToUserAsync(user.Id, PolicyType.TwoFactorAuthentication)
.Returns(
[
new OrganizationUserPolicyDetails
{
OrganizationId = organization1.Id,
PolicyType = PolicyType.TwoFactorAuthentication,
PolicyEnabled = true
},
new OrganizationUserPolicyDetails
{
OrganizationId = organization2.Id,
PolicyType = PolicyType.TwoFactorAuthentication,
PolicyEnabled = true
}
]);
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization1.Id)
.Returns(organization1);
sutProvider.GetDependency<IOrganizationRepository>()
.GetByIdAsync(organization2.Id)
.Returns(organization2);
sutProvider.GetDependency<IOrganizationRepository>()
.GetByVerifiedUserEmailDomainAsync(user.Id)
.Returns(new[] { organization1 });
var expectedSavedProviders = JsonHelpers.LegacySerialize(new Dictionary<TwoFactorProviderType, TwoFactorProvider>(), JsonHelpers.LegacyEnumKeyResolver);

// Act
await sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email);

// Assert
await sutProvider.GetDependency<IUserRepository>()
.Received(1)
.ReplaceAsync(Arg.Is<User>(u => u.Id == user.Id && u.TwoFactorProviders == expectedSavedProviders));
await sutProvider.GetDependency<IEventService>()
.Received(1)
.LogUserEventAsync(user.Id, EventType.User_Disabled2fa);

// Revoke the user from the first organization because they are managed by it
await sutProvider.GetDependency<IRevokeNonCompliantOrganizationUserCommand>()
.Received(1)
.RevokeNonCompliantOrganizationUsersAsync(
Arg.Is<RevokeOrganizationUsersRequest>(r => r.OrganizationId == organization1.Id &&
r.OrganizationUsers.First().UserId == user.Id &&
r.OrganizationUsers.First().OrganizationId == organization1.Id));
await sutProvider.GetDependency<IMailService>()
.Received(1)
.SendOrganizationUserRevokedForTwoFactoryPolicyEmailAsync(organization1.DisplayName(), user.Email);

// Remove the user from the second organization because they are not managed by it
await sutProvider.GetDependency<IRemoveOrganizationUserCommand>()
.Received(1)
.RemoveUserAsync(organization2.Id, user.Id);
await sutProvider.GetDependency<IMailService>()
.Received(1)
.SendOrganizationUserRemovedForPolicyTwoStepEmailAsync(organization2.DisplayName(), user.Email);
}

private static void SetupUserAndDevice(User user,
bool shouldHavePassword)
{
Expand Down
Loading