[PM-33043] Refactor PolicyService, CipherService, and TwoFactorAuthenticationValidator

[JimmyVo16]

🎟️ Tracking

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

📔 Objective

1. Refactor PolicyService, CipherService, and TwoFactorAuthenticationValidator to use the new Task<IDictionary<Guid, OrganizationAbility>> GetOrganizationAbilitiesAsync(IEnumerable<Guid> orgIds) instead of the deprecated GetOrganizationAbilitiesAsync().
2. Add unit tests

📸 Screenshots

I feel like the new unit test coverage is sufficient.

[claude]

Claude finished @JimmyVo16's task in 2m 43s —— View job

---

Code Review: PM-33043 Refactor Services for OrganizationAbility

• Gather context and read changed files
• Review production code changes
• Review test changes
• Post final review

Overall Assessment: APPROVE

This PR cleanly refactors three services to use the scoped GetOrganizationAbilitiesAsync(IEnumerable<Guid>) instead of the parameterless overload. The changes are behavior-preserving and test coverage is adequate.

Detailed Analysis

Security: No concerns. The refactoring reduces data exposure by fetching only the necessary organization abilities rather than the entire cache.

Correctness: The logic transformations preserve original behavior:

• PolicyService: The pre-filtering before the abilities fetch changes execution order but not results, since all original conditions were AND'd together. The UsePolicies check logic is logically equivalent.
• CipherService: .GroupBy() naturally produces distinct keys, so no .Distinct() is needed when extracting org IDs. The Guid? handling correctly filters for HasValue.
• TwoFactorAuthenticationValidator: Straightforward extraction of org IDs from the membership list.

Performance: Positive impact — fetches only the needed organization abilities from cache instead of all of them.

Minor Observations (non-blocking)

1. Misleading test name: DeleteManyAsync_WithOrgCipherNotFoundInCache_ThrowsNotFoundException in CipherServiceTests.cs asserts ThrowsAsync<Exception>, not NotFoundException. The test name implies a NotFoundException but the actual exception is a plain System.Exception. Cosmetic only.

2. No .Distinct() in TwoFactorAuthenticationValidator: The helper doesn't call .Distinct() on org IDs before passing to the cache service, while PolicyService does. In practice this is safe since OrganizationMembershipAsync returns unique memberships, but noting for consistency.

[claude]

Overall Assessment: REQUEST CHANGES

Reviewed the refactoring of PolicyService, CipherService, and TwoFactorAuthenticationValidator to use the scoped GetOrganizationAbilitiesAsync(IEnumerable Guid) overload instead of the obsolete parameterless version. The refactored logic in all three services is behaviorally equivalent to the original. However, existing tests still mock the parameterless overload, which will cause test failures.

Code Review Details

• ❌ CRITICAL: Tests mock the wrong GetOrganizationAbilitiesAsync overload causing NullReferenceException in CipherService tests and assertion failures in TwoFactorAuthenticationValidator tests
  • test/Core.Test/Vault/Services/CipherServiceTests.cs lines 1079, 1141, 1909, 1957, 2256, 2305
  • test/Identity.Test/IdentityServer/TwoFactorAuthenticationValidatorTests.cs line 138

[claude]

Inline Finding on src/Core/Vault/Services/Implementations/CipherService.cs line 1187:

❌ CRITICAL: Test mocks target the wrong overload and will cause failures

The code now calls the parameterized GetOrganizationAbilitiesAsync via this helper, but 7 existing tests still mock the parameterless overload. NSubstitute treats these as distinct overloads, so the parameterized call returns null.

CipherServiceTests (6 tests at lines 1079, 1141, 1909, 1957, 2256, 2305): The null dictionary causes a NullReferenceException at organizationAbilities.TryGetValue in FilterCiphersByDeletePermission.

TwoFactorAuthenticationValidatorTests (1 test at line 138): OrgUsing2fa guards against null, so no NRE, but the test expects result.Item1 to be true and result.Item2 to not be null, which both fail since the org abilities check is silently skipped.

Fix: Update all affected test mocks to use the parameterized overload instead of the parameterless .GetOrganizationAbilitiesAsync().

Affected test files:

• test/Core.Test/Vault/Services/CipherServiceTests.cs (lines 1079, 1141, 1909, 1957, 2256, 2305)
• test/Identity.Test/IdentityServer/TwoFactorAuthenticationValidatorTests.cs (line 138)

[github-actions]

Checkmarx One – Scan Summary & Details – 7c1463b8-6063-4e52-baf0-54a048f77bf8

Great job! No new security vulnerabilities introduced in this pull request

[claude]

Overall Assessment: REQUEST CHANGES

Re-reviewed after commit e159f45 which fixed the TwoFactorAuthenticationValidatorTests mock and added a new test verifying scoped org ability fetching. The TwoFactor test fix is correct and the new test is well-structured. However, the CipherServiceTests issue from the previous review remains unaddressed.

Code Review Details

• ❌ CRITICAL: CipherServiceTests still mocks the parameterless GetOrganizationAbilitiesAsync() overload at 6 call sites, while CipherService now calls the parameterized overload via the new private helper at line 1187. NSubstitute treats these as distinct overloads so the parameterized call returns null, causing NullReferenceException in FilterCiphersByDeletePermission. This affects RestoreManyAsync, DeleteManyAsync, and SoftDeleteManyAsync tests.
  • test/Core.Test/Vault/Services/CipherServiceTests.cs lines 1079, 1141, 1909, 1957, 2256, 2305
  • Fix: Update all 6 mock setups to use the parameterized overload, matching the pattern applied in the TwoFactorAuthenticationValidatorTests fix at line 138.

[claude]

Overall Assessment: REQUEST CHANGES

Re-reviewed after commit 8c9e65b which adds a PolicyServiceTests test verifying that GetPoliciesApplicableToUserAsync only fetches organization abilities for filtered (non-provider) org IDs. The new test is correct and well-structured. The previous CRITICAL finding about CipherServiceTests remains unaddressed.

Code Review Details

• ❌ CRITICAL: CipherServiceTests still mocks the parameterless GetOrganizationAbilitiesAsync() overload at 6 call sites, while CipherService now calls the parameterized GetOrganizationAbilitiesAsync(IEnumerable of Guid) overload via the new private helper at line 1187. NSubstitute treats these as distinct overloads, so the parameterized call returns null, causing NullReferenceException at organizationAbilities.TryGetValue in FilterCiphersByDeletePermission.
  • test/Core.Test/Vault/Services/CipherServiceTests.cs lines 1079, 1141, 1909, 1957, 2256, 2305
  • Fix: Update all 6 mock setups to use the parameterized overload (Arg.Any of IEnumerable of Guid), matching the pattern already applied in TwoFactorAuthenticationValidatorTests at line 138 and the new PolicyServiceTests at line 175.

[claude]

Overall Assessment: APPROVE

Re-reviewed after commit 63d833c which fixes all 6 CipherServiceTests mock setups to target the parameterized GetOrganizationAbilitiesAsync(IEnumerable of Guid) overload and adds a new test verifying the behavior when a cipher organization is not found in the abilities cache. The previous CRITICAL finding is fully resolved. No new issues found.

[codecov]

Codecov Report

❌ Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.54%. Comparing base (e5fec6a) to head (d8c7cdc).

Files with missing lines	Patch %	Lines
...nConsole/Services/Implementations/PolicyService.cs	87.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7214      +/-   ##
==========================================
- Coverage   61.58%   57.54%   -4.05%     
==========================================
  Files        2032     2032              
  Lines       89476    89509      +33     
  Branches     7957     7958       +1     
==========================================
- Hits        55108    51508    -3600     
- Misses      32441    36155    +3714     
+ Partials     1927     1846      -81     

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

[sonarqubecloud]

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