[PM-26377] Correcting Auto Confirm Handler Provider Check

[jrmccannon]

🎟️ Tracking

PM-26377

📔 Objective

Users that were part of a provider were still allowed to be in an org when enabling Auto Confrim policy.

⏰ 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

[claude]

Claude finished @jrmccannon's task —— View job

---

Code Review Complete ✅

I've reviewed the PR changes for PM-26377 and have the following feedback:

---

Summary

This PR fixes a bug where users who are part of a provider were still allowed to remain in an organization when enabling the Auto Confirm policy. The fix correctly changes the provider validation logic to check users in the organization against provider memberships, rather than checking if the organization is managed by a provider.

---

Key Changes Analysis

✅ Core Bug Fix (AutomaticUserConfirmationPolicyEventHandler.cs)

What Changed:

• Previously: GetManyByOrganizationAsync(organizationId) - checked if the organization had provider users
• Now: GetManyByManyUsersAsync(userIds) - checks if the specific organization users are provider users

Why This Matters:
The old approach was checking if the organization was managed by a provider (provider-organization relationship), but the requirement is to check if individual users in the organization are also provider users (user-provider relationship). This is a significant logic correction.

Code Quality: ✅ Good

• Line 87-88: Proper null filtering with Where(x => x.UserId is not null)
• Line 90: Clean, readable LINQ logic
• Lines 57-71: Better code organization by fetching org users once and passing them to both validation methods

---

✅ New Repository Method (GetManyByManyUsersAsync)

Implementation Quality:

1. Interface Definition (IProviderUserRepository.cs:15) ✅

   • Properly added to interface with clear naming

2. Dapper Implementation (ProviderUserRepository.cs:64-74) ✅

   • Uses stored procedure correctly
   • Proper async/await pattern with await using
   • Converts to list properly

3. Entity Framework Implementation (ProviderUserRepository.cs:100-111) ✅

   • Null-safe check: pu.UserId != null && userIds.Contains(pu.UserId.Value)
   • Consistent with Dapper implementation
   • Proper async scope management

---

⚠️ SQL Stored Procedure Issues

Files:

• src/Sql/dbo/Stored Procedures/ProviderUser_ReadManyByManyUserIds.sql (lines 11-12)
• util/Migrator/DbScripts/2025-12-03_00_ProviderUserGetManyByUserIds.sql (lines 11-12)

Issue: SQL formatting does not follow Bitwarden code style guidelines

Current:

    FROM
        [dbo].[ProviderUserView] AS [pu]
    INNER JOIN
        @UserIds [u] ON [u].[Id] = [pu].[UserId]

Should be:

    FROM
        [dbo].[ProviderUserView] AS [pu]
    INNER JOIN
        @UserIds [u] ON [u].[Id] = [pu].[UserId]

Problem:

1. INNER JOIN should be aligned with FROM (same indentation level)
2. This was correctly identified by reviewer @mkincaid-bw

Reference: Bitwarden SQL Style Guide

---

✅ Test Updates

Test Quality: Excellent improvements

1. Removed Unnecessary Mocking ✅

   • Removed IPolicyRepository mocks since RequiredPolicies now handles single-org dependency
   • Tests are now cleaner and more focused

2. Updated Test Cases ✅

   • Line 119-158: ValidateAsync_EnablingPolicy_ProviderUsersExist_ReturnsError - properly updated to mock GetManyByManyUsersAsync
   • Line 306-347: Fixed "Revoked users" test - now correctly validates that revoked users ARE included in compliance checks (previous behavior was incorrect)

3. New Integration Tests ✅ (ProviderUserRepositoryTests.cs:93-370)

   • Comprehensive coverage of GetManyByManyUsersAsync:
     • Multiple users across multiple providers
     • Single user scenarios
     • Users with multiple provider memberships
     • Empty input handling
     • Non-existent user IDs
     • Mixed existent/non-existent IDs
     • All status types
     • All provider user types

---

✅ Simplified Architecture

What Was Removed:

• IOnPolicyPreUpdateEvent interface implementation
• ExecutePreUpsertSideEffectAsync method
• Side effect logic that set UseAutomaticUserConfirmation on the organization
• Dependency on IPolicyRepository, IOrganizationRepository, and TimeProvider

Impact: 👍 Positive

• Cleaner separation of concerns
• Reduced complexity
• The organization feature flag is likely being set elsewhere now (or no longer needed)

---

Security Review 🔒

✅ Zero-Knowledge Principles: Not compromised - no vault data accessed
✅ Input Validation: User IDs properly filtered for null values
✅ SQL Injection: Protected - using parameterized stored procedures
✅ No PII in Logs: No logging added
✅ Authorization: Existing org-level authorization applies

---

Issues Found

🔴 MUST FIX: SQL Formatting

Both SQL files need formatting corrections:

File 1: src/Sql/dbo/Stored Procedures/ProviderUser_ReadManyByManyUserIds.sql:11-12
File 2: util/Migrator/DbScripts/2025-12-03_00_ProviderUserGetManyByUserIds.sql:11-12

The INNER JOIN keyword must be aligned with the FROM keyword at the same indentation level per the SQL style guide.

---

Additional Observations

📝 Line Coverage Gap

Codecov reports 2 lines missing coverage in AutomaticUserConfirmationPolicyEventHandler.cs:

• Line 87-88: The Where clause filtering for non-null userIds
• This is likely an edge case where all organization users have null UserId (all invited users)

Recommendation: Consider adding a unit test for this edge case:

[Theory, BitAutoData]
public async Task ValidateAsync_EnablingPolicy_AllUsersAreInvited_ReturnsEmptyString(...)
{
    // All org users have UserId = null (invited status)
    // Should return empty string (no provider validation needed)
}

ℹ️ Documentation Update

Line 20-21: Documentation now correctly reflects the actual validations (removed outdated side effects section).

---

Testing Recommendations

1. ✅ Unit Tests: Comprehensive coverage with proper mocking
2. ✅ Integration Tests: Excellent coverage of the new repository method
3. ⚠️ Manual Testing Suggestion:
   • Create an org with users who are also provider users
   • Attempt to enable Auto Confirm policy
   • Verify the error message appears correctly
   • Remove provider users
   • Verify policy can now be enabled

---

Approval Blockers

❌ SQL formatting must be fixed before merge (reviewer @mkincaid-bw requested changes)

Once the SQL formatting is corrected, this PR will be ready to merge.

---

Summary Verdict

Overall: Excellent bug fix with one formatting issue to resolve

✅ Strengths:

• Correct logic fix addressing the root cause
• Excellent test coverage (unit + integration)
• Cleaner architecture with reduced dependencies
• Proper null safety
• Good performance (single query for all users)

⚠️ Must Fix:

• SQL formatting in both stored procedure files

💭 Nice to Have:

• Add unit test for edge case with all invited users (improves coverage)

---

[codecov]

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.26%. Comparing base (ded1c58) to head (07fd49e).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
...ors/AutomaticUserConfirmationPolicyEventHandler.cs	83.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6681      +/-   ##
==========================================
+ Coverage   53.35%   57.26%   +3.90%     
==========================================
  Files        1912     1917       +5     
  Lines       85138    85512     +374     
  Branches     7633     7673      +40     
==========================================
+ Hits        45423    48965    +3542     
+ Misses      37967    34703    -3264     
- Partials     1748     1844      +96     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Checkmarx One – Scan Summary & Details – 31bdd021-7ab1-4718-892c-6af52a0aa4ec

Fixed Issues (2)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 183
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 202

[mkincaid-bw]

Minor formatting issue.

[mkincaid-bw]

LGTM