Skip to content

[PM-33213] Remove FeatureFlag Around ResetPassword && PolicyRequirements#7188

Open
sven-bitwarden wants to merge 9 commits intomainfrom
ac/pm-33213/remove-resetpassword-feature-flag
Open

[PM-33213] Remove FeatureFlag Around ResetPassword && PolicyRequirements#7188
sven-bitwarden wants to merge 9 commits intomainfrom
ac/pm-33213/remove-resetpassword-feature-flag

Conversation

@sven-bitwarden
Copy link
Contributor

@sven-bitwarden sven-bitwarden commented Mar 9, 2026

🎟️ Tracking

PM-33213
PM-24528

📔 Objective

AC is moving on from the PolicyRequirements feature flag, and committing to its removal (and subsequent enablement).

This PR handles the code around the ResetPassword flag, and is now using PolicyRequirements where it can. This resulted in quite a bit of good cleanup.

There was also a bug (PM-24528) that will be fixed along the way.

Note

Due to the way SSO users are JIT-provisioned, we have left the PolicyQuery intact for verifying the automatic enrollment status, and removed the PolicyRequirementQuery around that area.

@sven-bitwarden sven-bitwarden added needs-qa ai-review Request a Claude code review labels Mar 9, 2026
@claude
Copy link
Contributor

claude bot commented Mar 9, 2026

Claude finished @sven-bitwarden's task in 2m 55s —— View job


Code Review: PM-33213 Remove FeatureFlag Around ResetPassword && PolicyRequirements

  • Gather PR diff and context
  • Read changed files
  • Analyze changes and post review

Reviewed commits: 9 commits on ac/pm-33213/remove-resetpassword-feature-flag
Scope: Removal of PolicyRequirements feature flag for the ResetPassword policy, replacing old IPolicyQuery-based code paths with PolicyRequirementQuery-based equivalents, plus cleanup of unused dependencies.

Summary

Clean feature flag removal PR. The ResetPasswordPolicyRequirement class is refactored from an init-only HashSet<Guid> to a constructor-based Dictionary<Guid, bool> that tracks both policy enablement and auto-enroll status per organization. The new IsEnabled(orgId) method replaces direct IPolicyQuery.RunAsync calls. Feature-flag-variant test pairs are consolidated into single tests.

The OrganizationsController.GetAutoEnrollStatus method still uses the legacy _policyQuery path — this is intentional as it was not wrapped by the feature flag being removed, so it remains out of scope for this PR.

Findings

No new issues found. The previous review's suggestions (unused _policyQuery, _policyService, _groupRepository in OrganizationService) have all been addressed in subsequent commits. ✅

Overall assessment: Clean PR, no remaining concerns.

#nullable disable

using System.Text.Json;
using Bit.Core.AdminConsole.Entities;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 SUGGESTED: _policyQuery is no longer used anywhere in this file after the UpdateUserResetPasswordEnrollmentAsync refactor. The field (line 50), constructor parameter (line 78), and assignment (line 104) should be removed -- matching the cleanup already done in OrganizationUsersController and OrganizationsController.

The removed import of Bit.Core.AdminConsole.Models.Data.Organizations.Policies (line 8 in old code) was the only ResetPasswordDataModel-related import cleaned up, but the IPolicyQuery references remain.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

Logo
Checkmarx One – Scan Summary & Details6f8fbaa9-6319-4e73-b42b-0514d0b35a20


New Issues (2) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1592
detailsMethod at line 1592 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
Attack Vector
2 MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1419
detailsMethod at line 1419 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
Attack Vector

Fixed Issues (2) Great job! The following issues were fixed in this Pull Request
Severity Issue Source File / Package
MEDIUM CSRF /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 105
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 293

@codecov
Copy link

codecov bot commented Mar 10, 2026

Codecov Report

❌ Patch coverage is 63.15789% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.61%. Comparing base (80e6f87) to head (0850159).

Files with missing lines Patch % Lines
...le/Services/Implementations/OrganizationService.cs 0.00% 6 Missing ⚠️
...licyRequirements/ResetPasswordPolicyRequirement.cs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7188      +/-   ##
==========================================
- Coverage   57.62%   57.61%   -0.01%     
==========================================
  Files        2033     2033              
  Lines       89554    89510      -44     
  Branches     7976     7969       -7     
==========================================
- Hits        51604    51573      -31     
+ Misses      36086    36074      -12     
+ Partials     1864     1863       -1     

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

@sven-bitwarden sven-bitwarden marked this pull request as ready for review March 17, 2026 18:57
@sven-bitwarden sven-bitwarden requested a review from a team as a code owner March 17, 2026 18:57
@sven-bitwarden sven-bitwarden requested a review from BTreston March 17, 2026 18:57
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review needs-qa

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant