[PM-33216] Finalize RequireSsoPolicyRequirement

[eliykat]

🎟️ Tracking

https://bitwarden.atlassian.net/jira/dashboards/10012

📔 Objective

Finalize the RequireSsoPolicyRequirement implementation. We are generally just removing this feature flag for other consumers, but this PolicyRequirement is a bit different because it's used every time someone logs in. This means we need to:

1. make sure the query is performant - something we haven't really been pressed on until now
2. keep the feature flag so we have a killswitch if it breaks login functionality

Context for Auth:

This is a minor update to your code as part of finalizing (and removing) the PolicyRequirements feature flag. Your code is now using a vNext endpoint which contains some optimizations specifically for the "hot" login code path. The changes are still feature flagged.

Claude also noticed some unnecessary tests and test mocks, which I have removed while I'm here.

Context for AC Team and dbops

For performance - @sven-bitwarden had switched PolicyRequirementQuery over to the bulk PolicyDetails_ReadByUserIdsPolicyType sproc for both single and bulk queries. I generally support this for simple queries, but here the dbops advice was that this produces a slightly slower result and a significantly more complex query plan. This PR adds a PolicyDetails_ReadByUserIdPolicyType sproc for the single user use case. This closely matches the optimizations we already made to the legacy code path, and it should perform equally as well. So after the EDD cycle, we will be left with:

• PolicyDetails_ReadByUserIdPolicyType - for a single user
• PolicyDetails_ReadByUserIdsPolicyType - for a many users (bulk)

The new sproc is called by the new IPolicyRequirement.GetAsyncVNext method which is only consumed by this location. As part of feature flag removal we will make this the standard method.

The single PolicyDetails_ReadByUserId sproc we were previously using actually performs far worse than anything else mentioned and will be dropped as part of feature flag cleanup as well.

📸 Screenshots

[github-actions]

Checkmarx One – Scan Summary & Details – 1470746c-0b67-454f-9a72-0cd8ecb33e29

---

New Issues (2)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		Path_Traversal	/src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 56	details Method at line 56 of /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs gets dynamic data from the model element. This ... Attack Vector
2		CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 452	details Method at line 452 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow... Attack Vector

[codecov]

Codecov Report

❌ Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.47%. Comparing base (b35976b) to head (22f87fe).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...Policies/Implementations/PolicyRequirementQuery.cs	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7173      +/-   ##
==========================================
+ Coverage   57.38%   61.47%   +4.08%     
==========================================
  Files        2031     2031              
  Lines       89283    89341      +58     
  Branches     7936     7939       +3     
==========================================
+ Hits        51237    54923    +3686     
+ Misses      36205    32493    -3712     
- Partials     1841     1925      +84     

☔ 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.

[claude]

Claude finished @eliykat's task in 3m 54s —— View job

---

Code Review: PR #7173 — Finalize RequireSsoPolicyRequirement

Summary: This PR adds an optimized single-user stored procedure (PolicyDetails_ReadByUserIdPolicyType) for policy detail queries, wires it through a new GetAsyncVNext method on IPolicyRequirementQuery, and switches the two RequireSsoPolicyRequirement consumers (WebAuthnController, SsoRequestValidator) to use it. Dead test mocks are cleaned up. The approach is sound: keeping the feature flag as a killswitch while optimizing the hot login path.

Verdict: Looks good. No issues found.

Review details:

• Security: The SSO policy enforcement on the login path is correctly implemented. The feature flag (PolicyRequirements) provides a safe rollback mechanism. No data exposure risks.
• SQL/EF parity: The stored procedure and EF implementation match in logic: CTE with UNION ALL for non-invited/invited users, @UserEmail IS NOT NULL guard, LEFT JOIN for providers (EF uses a HashSet instead), and identical filter conditions (Policy.Enabled, Organization.Enabled, Organization.UsePolicies). The EF null-email guard (userEmail != null) was correctly added for parity.
• Migration: CREATE OR ALTER in migration script, CREATE in sproc definition file -- correct pattern.
• Logic change: CanUsePasskeyLogin refactored from All(Revoked || Invited) to !Any(Accepted || Confirmed). These are logically equivalent because the Enforce filter in BasePolicyRequirementFactory already excludes Invited and Revoked statuses (via ExemptStatuses), so by the time Create is called, only Accepted/Confirmed entries remain. Both expressions evaluate to true on empty input.
• Test coverage: Unit tests for GetAsyncVNext (enforce callback, missing factory, empty policies), updated WebAuthn and SSO validator tests, removed dead BaseRequestValidatorTests that mocked GetAsync instead of GetAsyncVNext, and thorough integration tests covering confirmed/accepted/invited users, multiple orgs, policy type filtering, disabled policies/orgs, UsePolicies=false, and provider flag.
• Cleanup: Unused using Bit.Core import and stale mock setups for _policyService.AnyPoliciesApplicableToUserAsync in BaseRequestValidatorTests correctly removed.

[claude]

Code Review: PR #7173 — Finalize RequireSsoPolicyRequirement

Summary: This PR adds an optimized single-user stored procedure for policy detail queries, wires it through a GetAsyncVNext method, and switches the two RequireSsoPolicyRequirement consumers to use it. The approach is sound — keeping the feature flag as a killswitch while optimizing the hot login path. Overall this is a clean, well-structured PR.

Findings (all non-blocking):

1. CanUsePasskeyLogin logic change (RequireSsoPolicyRequirement.cs:51-54): The old All(Revoked||Invited) → new !Any(Accepted||Confirmed) change is logically equivalent since Enforce already filters out Invited/Revoked. The new code is more robust by explicitly naming the disabling statuses. Good change.

2. Dead mock setup in BaseRequestValidatorTests:551,590: These still mock GetAsync<RequireSsoPolicyRequirement> (not GetAsyncVNext), but since _ssoRequestValidator.ValidateAsync is also mocked, the mock is never hit. Pre-existing, not introduced by this PR. Cleanup candidate for follow-up.

3. SQL sproc looks correct: CTE with UNION ALL for non-invited/invited users, @UserEmail IS NOT NULL guard, LEFT JOIN for providers, proper Enabled/UsePolicies filters. Migration matches sproc (only CREATE vs CREATE OR ALTER).

4. EF implementation matches sproc logic: Handles email null-safety correctly (SQL NULL = NULL → false), uses HashSet for provider lookup, consistent filter conditions.

5. Integration tests are thorough: Covers confirmed/accepted/invited users, multiple orgs, policy type filtering, disabled policies/orgs, UsePolicies=false, and provider flag.

Verdict: Looks good. ✅

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
18.2% Duplication on New Code

See analysis details on SonarQube Cloud