[PM-18238] Add RequireTwoFactorPolicyRequirement #5840
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Implemented RequireTwoFactorPolicyRequirement to enforce two-factor authentication policies. * Created RequireTwoFactorPolicyRequirementFactory to generate policy requirements based on user status. * Added unit tests for the factory to validate behavior with various user statuses and policy details.
…actor authentication validation
…ement # Conflicts: # src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs
|
Great job, no security vulnerabilities found in this Pull Request |
…quirement to check for 2FA requirement
…oFactorPolicyRequirement; update tests to reflect new logic for two-factor authentication policy handling.
…licy based on feature flag; update validation logic and tests accordingly.
…ication policy based on feature flag; refactor validation logic and update related tests for improved policy handling.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5840 +/- ##
==========================================
- Coverage 50.82% 47.59% -3.23%
==========================================
Files 1662 1663 +1
Lines 75231 75260 +29
Branches 6762 6764 +2
==========================================
- Hits 38235 35819 -2416
- Misses 35472 37976 +2504
+ Partials 1524 1465 -59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…o determine user restoration eligibility based on two-factor authentication status; add corresponding unit tests for various scenarios.
…for two-factor authentication policies checks
…rPolicyRequirement and corresponding unit tests for policy retrieval based on user status
…tor authentication policy checks
…rty in RequireTwoFactorPolicyRequirement to clarify its purpose and return value.
r-tome
commented
May 21, 2025
| @@ -1782,72 +1775,6 @@ public async Task<List<Tuple<OrganizationUser, string>>> RevokeUsersAsync(Guid o | |||
| return result; | |||
| } | |||
|
|
|||
| private async Task CheckPoliciesBeforeRestoreAsync(OrganizationUser orgUser, bool userHasTwoFactorEnabled) | |||
There was a problem hiding this comment.
Unused method, leftover when lifting and shifting into command
r-tome
commented
May 21, 2025
There was a problem hiding this comment.
vNext tests were redundant
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…Async method in ConfirmOrganizationUserCommand to clarify error handling for users without two-step login enabled.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the RequireTwoFactorPolicyRequirement to enforce two‐factor authentication policies consistently across various user management commands. Key changes include updating tests and services (UserService, RestoreOrganizationUserCommand, ConfirmOrganizationUserCommand, and AcceptOrgUserCommand) to leverage the new policy requirement for two‐factor authentication, and refactoring dependency injections to support policy queries.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/Core.Test/Services/UserServiceTests.cs | Added dependency on IPolicyRequirementQuery and updated tests to verify two‐factor policy revocations. |
| test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/RequireTwoFactorPolicyRequirementFactoryTests.cs | Introduced and validated the policy requirement creation and behavior. |
| test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RestoreUser/RestoreOrganizationUserCommandTests.cs | Updated tests to assert behavior changes with policy requirements for 2FA. |
| test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommandTests.cs | Adjusted tests to enforce policy checks during confirmation. |
| test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommandTests.cs | Modified tests to validate error and success paths with policy requirements. |
| src/Core/Services/Implementations/UserService.cs | Integrated IPolicyRequirementQuery into two‐factor removal checks. |
| src/Core/AdminConsole/Services/Implementations/OrganizationService.cs | Removed unused two‐factor query dependency as policies are now enforced via policy requirements. |
| src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/RequireTwoFactorPolicyRequirement.cs | Added new policy requirement class and its factory for two‐factor authentication. |
| src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/RestoreUser/v1/RestoreOrganizationUserCommand.cs | Updated restore logic to factor in the new two‐factor policy requirements. |
| src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs | Introduced enhanced policy validation for user confirmation. |
| src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs | Incorporated policy-based two‐factor authentication validation during acceptance of organization users. |
Comments suppressed due to low confidence (2)
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs:206
- [nitpick] Consider unifying the error message terminology to consistently refer to 'two-factor authentication' rather than 'two-step login', aligning with the rest of the codebase.
throw new BadRequestException("User does not have two-step login enabled.");
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs:261
- [nitpick] Consider standardizing error messages across commands by replacing 'two-step login' with 'two-factor authentication' for consistency.
throw new BadRequestException("You cannot join this organization until you enable two-step login on your user account.");
…mmand to clarify handling of two-step login and 2FA policy checks.
BTreston
previously approved these changes
May 22, 2025
...onsole/OrganizationFeatures/Policies/PolicyRequirements/RequireTwoFactorPolicyRequirement.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new RequireTwoFactorPolicyRequirement along with the matching factory to enforce two‐factor authentication policies across multiple user management commands. Key changes include:
- Implementation of the new 2FA policy requirement and its corresponding factory.
- Updates to user management commands (UserService, Restore, Confirm, and Accept commands) to incorporate the 2FA policy checks based on a feature flag.
- Addition of extensive unit tests for validating the new 2FA policy behaviors in various user scenarios.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Core.Test/Services/UserServiceTests.cs | Updated tests to validate behavior when revoking 2FA providers using the new 2FA policy requirement. |
| test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/RequireTwoFactorPolicyRequirementFactoryTests.cs | Added unit tests for the new RequireTwoFactorPolicyRequirement factory. |
| test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/* | Enhanced tests across Restore, Confirm, and Accept commands to check for correct 2FA policy enforcement. |
| src/Core/Services/Implementations/UserService.cs | Injected and used IPolicyRequirementQuery for enforcing 2FA policies during provider removal. |
| src/Core/AdminConsole/OrganizationFeatures/Policies/* | Introduced the new policy requirement implementation and factory for two-factor authentication. |
| src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/* | Adjusted restore, confirm, and accept commands to conditionally enforce the new 2FA requirement when the feature flag is enabled. |
Comments suppressed due to low confidence (1)
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs:129
- [nitpick] Consider ensuring consistency in naming the two-factor enablement flag across methods. In some areas it is referenced as 'twoFactorEnabled' while here it is renamed to 'userTwoFactorEnabled'.
var userTwoFactorEnabled = usersTwoFactorEnabled.FirstOrDefault(tuple => tuple.userId == user.Id).twoFactorIsEnabled;
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs
Outdated
Show resolved
Hide resolved
…izationId, OrganizationUserId) for active memberships requiring two-factor authentication. Update UserService and related tests to reflect this change.
eliykat
requested changes
May 26, 2025
...onsole/OrganizationFeatures/Policies/PolicyRequirements/RequireTwoFactorPolicyRequirement.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs
Outdated
Show resolved
Hide resolved
test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommandTests.cs
Show resolved
Hide resolved
…idateTwoFactorAuthenticationPolicyAsync method
…entication policy validation logic
…heck by removing intermediate variable
…the purpose of the IsTwoFactorRequiredForOrganization
…hentication checks and simplify test setup
…treamline two-factor authentication checks by removing redundant conditions and simplifying logic flow.
…tication compliance checks by consolidating logic into a new method, IsTwoFactorRequiredForOrganizationAsync.
…om AcceptOrgUserCommand
…to ensure correct validation of organization user policies.
BTreston
reviewed
May 27, 2025
|
|
||
| var revokeOrgUserTasks = requirement.OrganizationsRequiringTwoFactor.Select(async o => | ||
| { | ||
| var organization = await _organizationRepository.GetByIdAsync(o.OrganizationId); |
There was a problem hiding this comment.
I think we can use the bulk method for this instead and store in a dict keyed by organizationId
…izing organization retrieval and logging when no organizations require two-factor authentication.
eliykat
approved these changes
May 29, 2025
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18238
📔 Objective
Add a new
RequireTwoFactorPolicyRequirementto be used for checking if a user complies with the 2FA policy on various services (UserService,RestoreOrganizationUserCommand,ConfirmOrganizationUserCommand, andAcceptOrgUserCommand)⏰ Reminders before review
🦮 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