[PM-27131] Auto confirm policy requirement #6649
Conversation
…provider attempts to take organization.
…nterface into separate file.
|
New Issues (2)Checkmarx found the following issues in this Pull Request
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6649 +/- ##
==========================================
+ Coverage 53.65% 53.66% +0.01%
==========================================
Files 1926 1929 +3
Lines 85720 85875 +155
Branches 7687 7705 +18
==========================================
+ Hits 45989 46089 +100
- Misses 37958 38008 +50
- Partials 1773 1778 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @jrmccannon's task —— View job Code Review CompleteI've completed a comprehensive review of PR #6649. Here are my findings: Tasks
Critical Issues ❌1. Null Reference Exception in RestoreOrganizationUserCommandFile: The code retrieves a user at line 285 but doesn't validate it's non-null before accessing var user = await userRepository.GetByIdAsync(userId);
if (!singleOrgCompliant && !twoFactorCompliant)
{
throw new BadRequestException(user.Email + // NULL REFERENCE POSSIBLE
" is not compliant with the single organization and two-step login policy");
}Impact: Runtime NullReferenceException if a revoked organization user's associated user account was deleted. Fix: Add null check immediately after line 285: var user = await userRepository.GetByIdAsync(userId);
if (user == null)
{
throw new BadRequestException("User not found.");
}Important Issues
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
...atures/Policies/Enforcement/AutoConfirm/AutomaticUserConfirmationPolicyEnforcementRequest.cs
Outdated
Show resolved
Hide resolved
...Features/Policies/Enforcement/AutoConfirm/AutomaticUserConfirmationPolicyEnforcementQuery.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs
Show resolved
Hide resolved
...Features/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUsersValidator.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/PolicyService.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs
Show resolved
Hide resolved
bitwarden_license/src/Commercial.Core/AdminConsole/Services/ProviderService.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/PolicyService.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs
Outdated
Show resolved
Hide resolved
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
This is a good approach to a difficult requirement. I think the tricky thing here is making sure we've covered all flows that can land a user/org in a prohibited state.
bitwarden_license/src/Commercial.Core/AdminConsole/Services/ProviderService.cs
Outdated
Show resolved
Hide resolved
| private async Task<Error?> OrganizationUserConformsToSingleOrgPolicyAsync( | ||
| AutomaticallyConfirmOrganizationUserValidationRequest request) | ||
| private async Task<Error?> OrganizationUserConformsToAutomaticUserConfirmationPolicyAsync(AutomaticallyConfirmOrganizationUserValidationRequest request) |
There was a problem hiding this comment.
Check my thinking - and if you agree, please add a comment...
For this organization: we know that autoconfirm is enabled, because we check it above; otherwise we wouldn't be in the autoconfirm flow. So we don't need to check the (regular) Single Org Policy, only autoconfirm (which is a stricter set of requirements).
For other organizations: we don't need to enforce their policies (whether Single Org or Autoconfirm), because if there are any other organizations, the prior check will always fail.
Is that right? I am just making sure that removing the current single org check is OK.
There was a problem hiding this comment.
I agree. I've now isolated single org from auto confirm as much as possible.
There was a problem hiding this comment.
The logic here looks good to me, but please add a comment explaining why we're not checking single org. (basically my explanation above.) I ask because this is a difference between the autoconfirm flow and the regular flow, it may otherwise look like an oversight.
...Features/Policies/Enforcement/AutoConfirm/AutomaticUserConfirmationPolicyEnforcementQuery.cs
Outdated
Show resolved
Hide resolved
...atures/Policies/Enforcement/AutoConfirm/AutomaticUserConfirmationPolicyEnforcementRequest.cs
Outdated
Show resolved
Hide resolved
...ganizationFeatures/Policies/PolicyRequirements/AutomaticUserConfirmationPolicyRequirement.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Are these changes necessary if we're now calling the new AutoConfirmPolicyRequirement in all these flows? This is effectively modifying Single Org behaviour which isn't the approach in the rest of this PR. I may be missing something though - are there places where this policyService method is called and the new policyRequirement/enforce query is not?
There was a problem hiding this comment.
They are not, but I have a question about what happens when someone later tries to call PolicyService with Auto Confirm policy type?
Should we throw exceptions or call the requirement here or here?
Anyone who calls those methods with the Auto Confirm policy type would not get the correct answer. Hopefully we would be involved if anyone is checking for auto confirm, but should we throw and inform them of the requirement or enforcement classes or just call the requirement?
I don't really think we have the context to answer the question they're attempting to solve for in the future so throwing might be a safe bet.
There was a problem hiding this comment.
That's a fair point. Given how we are treating it here - as a separate policy that should be checked separately - I think the answer is that the future caller should know to check autoconfirm as well, and this method goes unchanged.
That is maybe fragile. But the idea is that we now have more policies that affect these flows, Autoconfirm becomes just another policy that should be checked when a user tries to do an action affected by the policy. The caller needs to know to check it, but that's the case with all policies. (I think this is an argument for having another layer of abstraction for policy enforcement like we've discussed, but out of scope here.)
Also note that I am going to return to the PolicyRequirements migration in the next few sprints so we can remove this old code - which should help with PolicyService confusion: https://bitwarden.atlassian.net/browse/PM-28958
…nnon/ac/pm-27131-auto-confirm-req
…call within policy service.
# Conflicts: # src/Sql/dbo/Stored Procedures/ProviderUser_ReadManyByManyUserIds.sql # util/Migrator/DbScripts/2025-12-03_00_ProviderUserGetManyByUserIds.sql
bitwarden_license/src/Commercial.Core/AdminConsole/Services/ProviderService.cs
Show resolved
Hide resolved
| private async Task<Error?> OrganizationUserConformsToSingleOrgPolicyAsync( | ||
| AutomaticallyConfirmOrganizationUserValidationRequest request) | ||
| private async Task<Error?> OrganizationUserConformsToAutomaticUserConfirmationPolicyAsync(AutomaticallyConfirmOrganizationUserValidationRequest request) |
There was a problem hiding this comment.
The logic here looks good to me, but please add a comment explaining why we're not checking single org. (basically my explanation above.) I ask because this is a difference between the autoconfirm flow and the regular flow, it may otherwise look like an oversight.
| var allOrganizationUsersForUser = await organizationUserRepository | ||
| .GetManyByUserAsync(request.OrganizationUser!.UserId!.Value); | ||
|
|
||
| if (allOrganizationUsersForUser.Count == 1) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This early return could be moved inside the automaticUserConfirmationPolicyEnforcementValidator to better encapsulate it.
| public bool IsEnabled(Guid organizationId) => policyDetails.Any(p => p.OrganizationId == organizationId); | ||
| public bool CanBeGrantedEmergencyAccess() => policyDetails.Any(); | ||
|
|
||
| public bool UserBelongsToOrganizationWithAutomaticUserConfirmationEnabled() => policyDetails.Any(); |
There was a problem hiding this comment.
IPolicyRequirements work best when they express what the policy requires in business logic terms. This isn't always possible, but here this could be (for example) CanJoinProvider(). Then that's the interface that the ProviderService calls.
| { | ||
| var requirement = await policyRequirementQuery.GetAsync<AutomaticUserConfirmationPolicyRequirement>(ownerId); | ||
|
|
||
| if (requirement.UserBelongsToOrganizationWithAutomaticUserConfirmationEnabled()) |
There was a problem hiding this comment.
Similarly, here this interface could be CanCreateNewOrganization().
The idea being that this command doesn't need to know about org membership and policies. Only that this requirement doesn't block what the user is trying to do.
src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
That's a fair point. Given how we are treating it here - as a separate policy that should be checked separately - I think the answer is that the future caller should know to check autoconfirm as well, and this method goes unchanged.
That is maybe fragile. But the idea is that we now have more policies that affect these flows, Autoconfirm becomes just another policy that should be checked when a user tries to do an action affected by the policy. The caller needs to know to check it, but that's the case with all policies. (I think this is an argument for having another layer of abstraction for policy enforcement like we've discussed, but out of scope here.)
Also note that I am going to return to the PolicyRequirements migration in the next few sprints so we can remove this old code - which should help with PolicyService confusion: https://bitwarden.atlassian.net/browse/PM-28958
🎟️ Tracking
PM-27131
PM-28187
📔 Objective
This will add the auto confirm policy requirement and enforcement query for enforcing the auto confirm policy for a user and organization.
This mainly is a different version of the Single Organzation Policy Requirement, except that it enforces that the user may not be a member of other organizations regardless of their status or type (Single org exempts admin, owners and providers and users in the status of invited and revoked.)
This also blocks providers from joining organizations with the auto confirm policy on and enforces its own single org policy when users attempt to join the organization.
⏰ 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