[PM-33216] Finalize RequireSsoPolicyRequirement#7173
Conversation
|
New Issues (3)Checkmarx found the following issues in this Pull Request
|
eliykat
force-pushed
the
ac/pm-33216/finalize-implementation-requiresso
branch
from
18a7c53 to
a4d874b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7173 +/- ##
==========================================
+ Coverage 57.53% 61.61% +4.08%
==========================================
Files 2032 2032
Lines 89486 89544 +58
Branches 7957 7960 +3
==========================================
+ Hits 51487 55174 +3687
+ Misses 36154 32442 -3712
- Partials 1845 1928 +83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @eliykat's task in 2m 0s —— View job Re-review: PR #7173 — Finalize RequireSsoPolicyRequirement
SummaryRe-reviewed after latest sync with main (merge commit All previous findings remain addressed:
No new issues found. The PR is clean and well-structured. Looks good to merge. | |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/Infrastructure.EntityFramework/AdminConsole/Repositories/PolicyRepository.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| /// <param name="userId">The user that you need to enforce the policy against.</param> | ||
| /// <typeparam name="T">The IPolicyRequirement that corresponds to the policy you want to enforce.</typeparam> | ||
| Task<T> GetAsyncVNext<T>(Guid userId) where T : IPolicyRequirement; |
There was a problem hiding this comment.
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):
-
CanUsePasskeyLoginlogic change (RequireSsoPolicyRequirement.cs:51-54): The oldAll(Revoked||Invited)→ new!Any(Accepted||Confirmed)change is logically equivalent sinceEnforcealready filters out Invited/Revoked. The new code is more robust by explicitly naming the disabling statuses. Good change. -
Dead mock setup in
BaseRequestValidatorTests:551,590: These still mockGetAsync<RequireSsoPolicyRequirement>(notGetAsyncVNext), but since_ssoRequestValidator.ValidateAsyncis also mocked, the mock is never hit. Pre-existing, not introduced by this PR. Cleanup candidate for follow-up. -
SQL sproc looks correct: CTE with
UNION ALLfor non-invited/invited users,@UserEmail IS NOT NULLguard,LEFT JOINfor providers, proper Enabled/UsePolicies filters. Migration matches sproc (onlyCREATEvsCREATE OR ALTER). -
EF implementation matches sproc logic: Handles email null-safety correctly (SQL
NULL = NULL→ false), usesHashSetfor provider lookup, consistent filter conditions. -
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. ✅
| AND @UserEmail IS NOT NULL | ||
| ), | ||
| Providers AS | ||
| ( |
There was a problem hiding this comment.
⛏️ This CTE is indented more than the rest
…implementation-requiresso
eliykat
dismissed stale reviews from sven-bitwarden and ike-kottlowski
via
818cdcc
|
Re-review after latest commits (818cdcc, 2e51bf2). The only changes since the last review are formatting fixes to the Providers CTE indentation in both the stored procedure and migration file, addressing mkincaid-bw's feedback. Formatting is now consistent across both files. No functional changes. Previous findings were addressed (null guard added to EF query in commit 21e6d54). No new issues. Looks good. |
|
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/jira/dashboards/10012
📔 Objective
Finalize the
RequireSsoPolicyRequirementimplementation. We are generally just removing this feature flag for other consumers, but thisPolicyRequirementis a bit different because it's used every time someone logs in. This means we need to: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
PolicyRequirementQueryover to the bulkPolicyDetails_ReadByUserIdsPolicyTypesproc 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 aPolicyDetails_ReadByUserIdPolicyTypesproc 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 userPolicyDetails_ReadByUserIdsPolicyType- for a many users (bulk)The new sproc is called by the new
IPolicyRequirement.GetAsyncVNextmethod which is only consumed by this location. As part of feature flag removal we will make this the standard method.The single
PolicyDetails_ReadByUserIdsproc 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