[PM-37486] Remove IPolicyService and associated dead code by r-tome · Pull Request #7672 · bitwarden/server

r-tome · 2026-05-19T09:28:09Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-37486

📔 Objective

Migrate remaining IPolicyService callers to use IPolicyRequirementQuery and subsequently delete `IPolicyService.

…pendency and replace with IPolicyRequirementQuery for policy checks. Update related tests to reflect changes in policy validation logic.

…Service with IPolicyRequirementQuery for policy checks. Update tests accordingly to reflect changes in policy validation logic.

…updating PolicyServiceCollectionExtensions and deleting associated tests. This change streamlines policy management by relying on IPolicyRequirementQuery for policy checks.

…tailsAsync method and associated tests.

…icyDetails and PolicyDetails_ReadByUserId, as they are no longer called in the codebase.

github-actions · 2026-05-19T09:28:57Z

Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR completes the migration from the legacy IPolicyService to IPolicyRequirementQuery, deletes the orphaned service, its OrganizationUserPolicyDetails model, the GetByUserIdWithPolicyDetailsAsync repository method on both Dapper and EF implementations, and the associated AutoFixture types and integration tests. The five remaining call sites in AccountsController, InitPendingOrganizationValidator, and the Identity request validators (BaseRequestValidator, CustomTokenRequestValidator, ResourceOwnerPasswordValidator, SsoRequestValidator, WebAuthnGrantValidator) have been switched over with semantically equivalent replacements — SingleOrganizationPolicyRequirement.CanCreateOrganization() for the SingleOrg check (filters [Accepted, Confirmed], matching the prior minStatus = Accepted behavior) and MasterPasswordPolicyRequirement.EnforcedOptions for the master-password policy (same role exemptions as the deleted PolicyService path).

Prior reviewer concerns are addressed: the orphaned EfPolicyApplicableToUser/EfPolicyApplicableToUserInlineAutoDataAttribute fixtures are removed from PolicyFixtures.cs, and the stored-procedure DROP migration that risked a rollback-incompatible deployment has been reverted, leaving OrganizationUser_ReadByUserIdWithPolicyDetails and PolicyDetails_ReadByUserId in place for a safer cleanup PR once this deployment is hardened.

Code Review Details

No new findings.

codecov · 2026-05-19T09:36:23Z

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.82%. Comparing base (7180015) to head (7e8ab19).

Files with missing lines	Patch %	Lines
src/Api/Auth/Controllers/AccountsController.cs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7672      +/-   ##
==========================================
+ Coverage   60.43%   64.82%   +4.38%     
==========================================
  Files        2140     2138       -2     
  Lines       94622    94507     -115     
  Branches     8443     8440       -3     
==========================================
+ Hits        57188    61262    +4074     
+ Misses      35429    31151    -4278     
- Partials     2005     2094      +89

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

…es, as they are no longer needed in the codebase.

…licyRequirementQuery for SSO validation checks. Update related test logic to ensure accurate policy validation outcomes. Clean up unused test fixtures in PolicyFixtures.cs to streamline the codebase.

…ability by storing policy requirement results in local variables before returning values. This change enhances code clarity while maintaining existing functionality.

…of the policy requirement query in a local variable before returning the enforced options. This change enhances code readability while preserving existing functionality.

mkincaid-bw · 2026-05-19T17:27:10Z

+-- OrganizationUser_ReadByUserIdWithPolicyDetails was used by PolicyService (now deleted).
+-- PolicyDetails_ReadByUserId was never called anywhere.
+
+DROP PROCEDURE IF EXISTS [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails];


It looks like the C# code that calls this proc is being removed in this PR. If the deployment requires a server code rollback, this DROP would cause issues since DB changes are not reverted. This DROP should be moved to another PR to be done at a later date once this server deployment is hardened.

Thanks for calling this out! I have reverted the SQL changes in this branch. I will create a separate task to drop the stored procedures in a future release.

…dWithPolicyDetails and PolicyDetails_ReadByUserId, as they are no longer called in the codebase." This reverts commit 0f4fdca.

eliykat

🧹

…-sprocs

…-sprocs # Conflicts: # src/Identity/IdentityServer/RequestValidators/SsoRequestValidator.cs

sonarqubecloud · 2026-05-21T15:46:39Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

JaredSnider-Bitwarden

These changes LGTM!

The expected behavior is intended to be the same but since this isn't feature flagged, I would like to request the following flows be QA tested from an auth perspective:

Login master-password policy — log in as a member of an org with the Master Password policy configured; confirm the client picks up the correct rules on the direct login response. Repeat for a user in two orgs with conflicting settings — strictest values should win.
/accounts/verify-password reprompt - As the same user, perform any master-password reprompt action (Export vault is easiest); confirm the client receives the same policy block. Also verify a user in no org can successfully hit this endpoint and gets a default/empty policy block (no 500).
Accept org invite under Single Org policy

User already Confirmed in Org A (Single Org policy ON) must be blocked from accepting Org B's invite.
User only Invited (not yet Accepted) in Org A must still be able to accept Org B's invite.
User Revoked in Org A must be able to accept Org B's invite.
Provider-exempt user must be able to accept.

eliykat · 2026-05-22T01:18:41Z

@JaredSnider-Bitwarden

Login master-password policy — log in as a member of an org with the Master Password policy configured; confirm the client picks up the correct rules on the direct login response. Repeat for a user in two orgs with conflicting settings — strictest values should win.

I take it this is the change in BaseRequestValidator. It was calling PolicyService.GetMasterPasswordPolicyForUserAsync, but that already used PolicyRequirementQuery.GetAsyncVNext<MasterPasswordPolicyRequirement> under the hood. This PR is just removing the indirection; no change in the actual implementation code.

/accounts/verify-password reprompt - As the same user, perform any master-password reprompt action (Export vault is easiest); confirm the client receives the same policy block. Also verify a user in no org can successfully hit this endpoint and gets a default/empty policy block (no 500).

Same as above.

Accept org invite under Single Org policy

This has not been changed here. Maybe you're looking at the InitPendingOrganizationValidator? That is responsible for initializing a reseller organization created via the Bitwarden Portal. In this case, we are already using the SingleOrganizationPolicyRequirement in all the other accept/confirm flows, so we're comfortable with it here.

Let me know if I've missed or misunderstood anything.

JaredSnider-Bitwarden · 2026-05-22T02:33:11Z

@JaredSnider-Bitwarden

Login master-password policy — log in as a member of an org with the Master Password policy configured; confirm the client picks up the correct rules on the direct login response. Repeat for a user in two orgs with conflicting settings — strictest values should win.

I take it this is the change in BaseRequestValidator. It was calling PolicyService.GetMasterPasswordPolicyForUserAsync, but that already used PolicyRequirementQuery.GetAsyncVNext<MasterPasswordPolicyRequirement> under the hood. This PR is just removing the indirection; no change in the actual implementation code.

/accounts/verify-password reprompt - As the same user, perform any master-password reprompt action (Export vault is easiest); confirm the client receives the same policy block. Also verify a user in no org can successfully hit this endpoint and gets a default/empty policy block (no 500).

Same as above.

Accept org invite under Single Org policy

This has not been changed here. Maybe you're looking at the InitPendingOrganizationValidator? That is responsible for initializing a reseller organization created via the Bitwarden Portal. In this case, we are already using the SingleOrganizationPolicyRequirement in all the other accept/confirm flows, so we're comfortable with it here.

Let me know if I've missed or misunderstood anything.

@eliykat
Oh, ok, I'm sorry that I missed that it was already using that under the hood. Request for smoke tests retracted. Also, the last one was my mistake with vetting claude output - I apologize.

r-tome added 5 commits May 19, 2026 10:16

Refactor InitPendingOrganizationValidator to remove IPolicyService de…

c352b8d

…pendency and replace with IPolicyRequirementQuery for policy checks. Update related tests to reflect changes in policy validation logic.

Refactor AccountsController and related validators to replace IPolicy…

9ceffd6

…Service with IPolicyRequirementQuery for policy checks. Update tests accordingly to reflect changes in policy validation logic.

Remove IPolicyService and related implementations from the codebase, …

19e0d29

…updating PolicyServiceCollectionExtensions and deleting associated tests. This change streamlines policy management by relying on IPolicyRequirementQuery for policy checks.

Refactor OrganizationUserRepository to remove GetByUserIdWithPolicyDe…

9afce42

…tailsAsync method and associated tests.

Remove unused stored procedures: OrganizationUser_ReadByUserIdWithPol…

0f4fdca

…icyDetails and PolicyDetails_ReadByUserId, as they are no longer called in the codebase.

r-tome added needs-qa ai-review Request a Claude code review labels May 19, 2026

claude Bot reviewed May 19, 2026

View reviewed changes

Comment thread ...frastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs

r-tome added 4 commits May 19, 2026 11:09

Remove OrganizationUserPolicyDetails class and associated test fixtur…

9f24250

…es, as they are no longer needed in the codebase.

Refactor BaseRequestValidatorTests to replace IPolicyService with IPo…

09906e1

…licyRequirementQuery for SSO validation checks. Update related test logic to ensure accurate policy validation outcomes. Clean up unused test fixtures in PolicyFixtures.cs to streamline the codebase.

Refactor BaseRequestValidator and SsoRequestValidator to improve read…

55d296b

…ability by storing policy requirement results in local variables before returning values. This change enhances code clarity while maintaining existing functionality.

Refactor AccountsController to improve clarity by storing the result …

034e1ad

…of the policy requirement query in a local variable before returning the enforced options. This change enhances code readability while preserving existing functionality.

r-tome marked this pull request as ready for review May 19, 2026 15:10

r-tome requested review from a team as code owners May 19, 2026 15:10

r-tome requested review from eliykat and rr-bw May 19, 2026 15:10

rr-bw requested review from JaredSnider-Bitwarden and removed request for rr-bw May 19, 2026 16:04

mkincaid-bw reviewed May 19, 2026

View reviewed changes

Revert "Remove unused stored procedures: OrganizationUser_ReadByUserI…

99035ae

…dWithPolicyDetails and PolicyDetails_ReadByUserId, as they are no longer called in the codebase." This reverts commit 0f4fdca.

eliykat previously approved these changes May 20, 2026

View reviewed changes

Merge branch 'main' into ac/pm-37486/remove-unused-policy-methods-and…

4fa7a96

…-sprocs

r-tome dismissed eliykat’s stale review via 4fa7a96 May 21, 2026 08:39

Merge branch 'main' into ac/pm-37486/remove-unused-policy-methods-and…

7e8ab19

…-sprocs # Conflicts: # src/Identity/IdentityServer/RequestValidators/SsoRequestValidator.cs

JaredSnider-Bitwarden approved these changes May 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-37486] Remove IPolicyService and associated dead code#7672

[PM-37486] Remove IPolicyService and associated dead code#7672
r-tome wants to merge 12 commits into
mainfrom
ac/pm-37486/remove-unused-policy-methods-and-sprocs

r-tome commented May 19, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 19, 2026 •

edited

Loading

Uh oh!

Uh oh!

codecov Bot commented May 19, 2026 •

edited

Loading

Uh oh!

mkincaid-bw May 19, 2026

Uh oh!

r-tome May 19, 2026

Uh oh!

eliykat left a comment

Uh oh!

sonarqubecloud Bot commented May 21, 2026

Uh oh!

JaredSnider-Bitwarden left a comment

Uh oh!

eliykat commented May 22, 2026 •

edited

Loading

Uh oh!

JaredSnider-Bitwarden commented May 22, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

r-tome commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Uh oh!

github-actions Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitwarden Claude Code Review

Uh oh!

Uh oh!

codecov Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mkincaid-bw May 19, 2026

Choose a reason for hiding this comment

Uh oh!

r-tome May 19, 2026

Choose a reason for hiding this comment

Uh oh!

eliykat left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 21, 2026

Quality Gate passed

Uh oh!

JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

Uh oh!

eliykat commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaredSnider-Bitwarden commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

r-tome commented May 19, 2026 •

edited

Loading

github-actions Bot commented May 19, 2026 •

edited

Loading

codecov Bot commented May 19, 2026 •

edited

Loading

eliykat commented May 22, 2026 •

edited

Loading

JaredSnider-Bitwarden commented May 22, 2026 •

edited

Loading