[PM-10319] - Revoke Non Complaint Users for 2FA and Single Org Policy Enablement #5037

jrmccannon · 2024-11-13T20:38:00Z

🎟️ Tracking

📔 Objective

This change revokes non-compliant users when 2FA or Single Org policies are enabled. This adds the a RevokeNonCompliantOrganizationUserCommand along with a bulk set status stored procedure. It also adds two new email templates to be sent when a user is revoked from an organization.

📸 Screenshots

Two FA Policy Revocation Email

Single Org Policy Email

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

…request.

# Conflicts: # src/Api/AdminConsole/Models/Response/Organizations/CommandResult.cs # src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs

github-actions · 2024-11-13T21:34:29Z

Checkmarx One – Scan Summary & Details – 12dcc08e-ac28-497d-9358-790cbcf0e21f

New Issues

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 240	Attack Vector
CSRF	/src/Api/Auth/Controllers/TwoFactorController.cs: 438	Attack Vector
Log_Forging	/src/Api/Billing/Controllers/OrganizationSponsorshipsController.cs: 97	Attack Vector
Log_Forging	/src/Api/Billing/Controllers/OrganizationSponsorshipsController.cs: 97	Attack Vector
Log_Forging	/src/Api/Billing/Controllers/OrganizationSponsorshipsController.cs: 97	Attack Vector
Unsafe_Use_Of_Target_blank	/src/Core/MailTemplates/Handlebars/AdminConsole/OrganizationUserRevokedForTwoFactorPolicy.html.hbs: 11	Attack Vector
Unsafe_Use_Of_Target_blank	/src/Admin/AdminConsole/Views/Providers/Edit.cshtml: 103	Attack Vector
Unsafe_Use_Of_Target_blank	/src/Admin/AdminConsole/Views/Providers/Edit.cshtml: 90	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 372
	Unsafe_Use_Of_Target_blank	/src/Core/MailTemplates/Handlebars/FamiliesForEnterprise/FamiliesForEnterpriseRemovedFromFamilyUser.html.hbs: 11

codecov · 2024-11-13T21:49:09Z

Codecov Report

Attention: Patch coverage is 56.55431% with 116 lines in your changes missing coverage. Please review.

Project coverage is 43.06%. Comparing base (8b1b078) to head (1d77345).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...icies/PolicyValidators/SingleOrgPolicyValidator.cs	21.42%	32 Missing and 1 partial ⚠️
.../Services/Implementations/HandlebarsMailService.cs	4.00%	24 Missing ⚠️
...lidators/TwoFactorAuthenticationPolicyValidator.cs	69.64%	10 Missing and 7 partials ⚠️
...Users/RevokeNonCompliantOrganizationUserCommand.cs	79.66%	7 Missing and 5 partials ⚠️
...Console/Repositories/OrganizationUserRepository.cs	0.00%	7 Missing ⚠️
...Console/Repositories/OrganizationUserRepository.cs	0.00%	7 Missing ⚠️
...nizationDomains/VerifyOrganizationDomainCommand.cs	87.50%	3 Missing and 1 partial ⚠️
...nConsole/Services/Implementations/PolicyService.cs	42.85%	4 Missing ⚠️
src/Core/AdminConsole/Models/Data/SystemUser.cs	57.14%	3 Missing ⚠️
...re/Services/NoopImplementations/NoopMailService.cs	0.00%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5037      +/-   ##
==========================================
+ Coverage   42.68%   43.06%   +0.37%     
==========================================
  Files        1406     1419      +13     
  Lines       65090    64981     -109     
  Branches     5969     5950      -19     
==========================================
+ Hits        27782    27982     +200     
+ Misses      36063    35752     -311     
- Partials     1245     1247       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

r-tome

Looks good, left some suggestions

src/Sql/dbo/Stored Procedures/OrganizationUser_SetStatusForUsersById.sql

src/Core/AdminConsole/OrganizationFeatures/Shared/CommandResult.cs

...nConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs

…evokeNonCompliantOrganizationUserCommand.cs Co-authored-by: Rui Tomé <[email protected]>

…ation. Adding unknown type to event system user. Adding optional parameter to SaveAsync in policy service in order to pass in event system user.

eliykat · 2024-11-18T10:49:54Z

...nConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommand.cs

Is there anything in here that is specific to users being revoked for non-compliance with an org policy?

Users can also be revoked for other reasons (including because you looked at an admin funny). If this command is a bit more general, it can be used in all cases and can replace the other (various, confusing) OrganizationService implementations.

I think it could, but I would want to do that change in a separate PR. I also don't want to change the name until then so we know its safe to be the de facto RevokeOrganizationUserCommand

That's cool, can you please make a follow-up tech debt ticket to look into this?

Created:
https://bitwarden.atlassian.net/browse/PM-15052

...Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs

eliykat · 2024-11-18T11:02:39Z

...Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs

+            if (commandResult.HasErrors)
            {
-                await _removeOrganizationUserCommand.RemoveUserAsync(organizationId, orgUser.Id,
-                    savingUserId);
+                throw new BadRequestException(string.Join(", ", commandResult.ErrorMessages));
+            }

-                await _mailService.SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(
-                    org.DisplayName(), orgUser.Email);
+            await Task.WhenAll(revocableUsers.Select(x =>
+                _mailService.SendOrganizationUserRevokedForPolicySingleOrgEmailAsync(org.DisplayName(), x.Email)));
+        }


The command does not allow a mixed success / fail state, right? I was initially concerned that if any member could not be revoked, then nobody gets their email. But looking at the command, it validates everyone upfront and then doesn't proceed if there are any errors, so I think that's OK.

That said, if an admin revokes multiple users from the UI, we do allow a success or failure state per user, so we may need to accommodate both behaviors if the command is expanded to replace the OrganizationService methods.

The complicated thing about this command is all the validation logic. What we could do is break out the validation into a validator class, then have two commands (one for the 2FA/Single Org policy enablement and one for a user explicitly revoking a group of users). Then each command can make the decision about enforcing the validation result.

Unless we just want the command to just only reject invalid responses. Then we would make a Response object to extend CommandResult. Maybe something like

public class RevokeOrganizationUserCommandResponse : CommandResult { ... public Guid[] SuccessfullyRevokedUsers { get; } public Guid[] FailedToRevokeUsers { get; } ... }

That way we should show the partial success of the action.

...ole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs

src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs

src/Core/Services/Implementations/HandlebarsMailService.cs

src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs

...AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs

...rganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs

eliykat · 2024-11-18T11:38:13Z

...ole/OrganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidator.cs

+            var result = await _revokeNonCompliantOrganizationUserCommand.RevokeNonCompliantOrganizationUsersAsync(
+                new RevokeOrganizationUsersRequest(organizationId, revocableOrgUsers,
+                    new StandardUser(savingUserId ?? Guid.Empty,
+                        await _currentContext.OrganizationOwner(organizationId))));


So here's a fun one. currentContext.OrganizationOwner implicitly includes providers. This is... somewhat known when dealing with currentContext. But I recommend avoiding this ambiguity in your new object - e.g. adding an xmldoc noting that IsOwner can include providers in this context.

I'll do you one better. I'll change it to IsOwnerOrProvider

r-tome · 2024-11-20T10:48:31Z

...ore/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs



    public async Task<OrganizationDomain> UserVerifyOrganizationDomainAsync(OrganizationDomain organizationDomain)
    {
-        var domainVerificationResult = await VerifyOrganizationDomainAsync(organizationDomain);
+        var actingUser = new StandardUser(currentContext.UserId ?? Guid.Empty, await currentContext.OrganizationOwner(organizationDomain.OrganizationId));


Why pass an empty Guid and not the nullable Guid in the first parameter?

I'd rather not have null. I could throw at this point instead of using Guid.Empty. I was just defaulting to Guid.Empty in the event that a caller hasn't initialized the CurrentContext.

Do we know which applications would call this method without the CurrentContext populated with a user?

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Requests/RevokeOrganizationUser.cs

r-tome · 2024-11-20T11:05:22Z

...AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs

@@ -126,4 +127,69 @@ await sutProvider.GetDependency<IMailService>()
            .SendOrganizationUserRemovedForPolicySingleOrgEmailAsync(organization.DisplayName(),
                "[email protected]");
    }
+
+    [Theory, BitAutoData]
+    public async Task OnSaveSideEffectsAsync_GivenUpdatedSingleOrgPolicy_WhenAccountDeprovisioningIsDisabled_Then(


I would either rename this to something clearer like OnSaveSideEffectsAsync_WhenAccountDeprovisioningDisabled_DoesNotRevokeUsers or I would update the test above OnSaveSideEffectsAsync_RemovesNonCompliantUsers to assert that it does not revoke users

r-tome · 2024-11-20T11:06:40Z

...AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidatorTests.cs

Its missing testing when AccountDeprovisioning is enabled and users are revoked

r-tome · 2024-11-20T11:09:14Z

...rganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs

We're missing a test for the happy path when account deprovisioning is enabled and users have master passwords (should call RevokeNonCompliantOrganizationUsersAsync)

r-tome · 2024-11-20T11:10:39Z

...rganizationFeatures/Policies/PolicyValidators/TwoFactorAuthenticationPolicyValidatorTests.cs


        await sutProvider.GetDependency<IRemoveOrganizationUserCommand>().DidNotReceiveWithAnyArgs()
            .RemoveUserAsync(organizationId: default, organizationUserId: default, deletingUserId: default);
    }
+
+    [Theory, BitAutoData]
+    public async Task OnSaveSideEffectsAsync_GivenUpdateTo2faPolicy_WhenAccountProvisioningIsDisabled_ThenRevokeUserCommandShouldNotBeCalled(


OnSaveSideEffectsAsync_RemovesNonCompliantUsers could verify both the removal of users AND that revoke isn't called and then this test isn't required

r-tome · 2024-11-20T11:26:07Z

src/Core/AdminConsole/Services/Implementations/PolicyService.cs

    {
        if (_featureService.IsEnabled(FeatureFlagKeys.Pm13322AddPolicyDefinitions))
        {
            // Transitional mapping - this will be moved to callers once the feature flag is removed
+            // TODO make sure to populate with SystemUser if not an actual user


TODO no longer needed?

r-tome · 2024-11-20T11:28:55Z

src/Core/AdminConsole/Repositories/IOrganizationUserRepository.cs

@@ -58,4 +58,6 @@ UpdateEncryptedDataForKeyRotation UpdateForKeyRotation(Guid userId,
    /// Returns a list of OrganizationUsers with email domains that match one of the Organization's claimed domains.
    /// </summary>
    Task<ICollection<OrganizationUser>> GetManyByOrganizationWithClaimedDomainsAsync(Guid organizationId);
+
+    Task RevokeOrganizationUserAsync(IEnumerable<Guid> userIds);


Please add xmldoc here. Also, I would suggest renaming this to fit in with our usual naming scheme:

Suggested change

Task RevokeOrganizationUserAsync(IEnumerable<Guid> userIds);

Task RevokeManyByUserIdAsync(IEnumerable<Guid> userIds);

r-tome · 2024-11-20T11:54:27Z

...ole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs

+            .Received(1)
+            .RevokeOrganizationUserAsync(Arg.Any<IEnumerable<Guid>>());
+
+        Assert.True(result.Success);


Verify event logging for the revoked user

jrmccannon and others added 17 commits November 7, 2024 15:50

Revoking users when enabling single org and 2fa policies. Fixing tests.

0a4c88a

Added migration.

ff61b83

Wrote tests and fixed bugs found.

49620c7

Patch build process

e7a9a4f

Fixing tests.

8b534b0

Added unit test around disabling the feature flag.

6a36767

Updated error message to be public and added test for validating the …

f2f2a62

…request.

formatting

f009db0

Added some tests for single org policy validator.

595e4b9

Merge branch 'main' into ac/jmccannon/pm-10319-revoke-nc-users

d662fff

# Conflicts: # src/Api/AdminConsole/Models/Response/Organizations/CommandResult.cs # src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/SingleOrgPolicyValidator.cs

Fix issues from merge.

d64032c

Added sending emails to revoked non-compliant users.

abbd4f5

Fixing name. Adding two factor policy email.

3917114

Send email when user has been revoked.

da02c89

Correcting migration name.

1c3e4d8

Fixing templates and logic issue in Revoke command.

35643c1

Moving interface into its own file.

d3172c0

jrmccannon requested review from a team as code owners November 13, 2024 20:38

jrmccannon requested a review from eliykat November 13, 2024 20:38

Correcting namespaces for email templates.

fe5af90

jrmccannon changed the title ~~[PM-10319]~~ [PM-10319] - Revoke Non Complaint Users for 2FA and Single Org Policy Enablement Nov 13, 2024

correcting logic that would not allow normal users to revoke non owners.

95bb1f4

jrmccannon added 3 commits November 13, 2024 16:18

Actually correcting the test and logic.

eb693a8

dotnet format. Added exec to bottom of bulk sproc

58702d2

Merge branch 'main' into ac/jmccannon/pm-10319-revoke-nc-users

47cc545

eliykat requested a review from r-tome November 15, 2024 07:22

r-tome reviewed Nov 15, 2024

View reviewed changes

jrmccannon and others added 7 commits November 15, 2024 09:45

Update src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/R…

9427b9f

…evokeNonCompliantOrganizationUserCommand.cs Co-authored-by: Rui Tomé <[email protected]>

Updated OrgIds to be a json string

99f98f2

Fixing errors.

78cd03d

Updating test

436bebd

Moving command result.

8b89ad6

Formatting and request rename

0014d5c

Realized this would throw a null error from the system domain verific…

82b65e0

…ation. Adding unknown type to event system user. Adding optional parameter to SaveAsync in policy service in order to pass in event system user.

jrmccannon requested a review from r-tome November 15, 2024 16:56

eliykat requested changes Nov 18, 2024

View reviewed changes

Code review changes

51d0b12

jrmccannon requested a review from eliykat November 19, 2024 20:22

jrmccannon added 2 commits November 19, 2024 14:24

Removing todos

d4bd369

Corrected test name.

1d77345

r-tome requested changes Nov 20, 2024

View reviewed changes

Syncing filename to record name.

37ca7ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-10319] - Revoke Non Complaint Users for 2FA and Single Org Policy Enablement #5037

[PM-10319] - Revoke Non Complaint Users for 2FA and Single Org Policy Enablement #5037

jrmccannon commented Nov 13, 2024 •

edited by jira bot

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

r-tome left a comment

eliykat Nov 18, 2024

jrmccannon Nov 18, 2024

eliykat Nov 19, 2024

jrmccannon Nov 19, 2024

eliykat Nov 18, 2024

jrmccannon Nov 20, 2024

eliykat Nov 18, 2024

jrmccannon Nov 19, 2024

r-tome Nov 20, 2024

jrmccannon Nov 20, 2024

r-tome Nov 20, 2024

r-tome Nov 20, 2024

r-tome Nov 20, 2024

r-tome Nov 20, 2024

r-tome Nov 20, 2024

r-tome Nov 20, 2024

r-tome Nov 20, 2024

	Task RevokeOrganizationUserAsync(IEnumerable<Guid> userIds);
	Task RevokeManyByUserIdAsync(IEnumerable<Guid> userIds);

[PM-10319] - Revoke Non Complaint Users for 2FA and Single Org Policy Enablement #5037

Are you sure you want to change the base?

[PM-10319] - Revoke Non Complaint Users for 2FA and Single Org Policy Enablement #5037

Conversation

jrmccannon commented Nov 13, 2024 • edited by jira bot Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Nov 13, 2024 • edited Loading

New Issues

Fixed Issues

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

r-tome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrmccannon commented Nov 13, 2024 •

edited by jira bot

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading