[PM-38269] refactor: remove redundant permissions round-trip in OrganizationUserUserDetailsQuery

[r-tome]

🎟️ Tracking

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

📔 Objective

For each Custom-role user, OrganizationUserUserDetailsQuery was deserializing the Permissions JSON string from the database and immediately re-serializing it back to a string — a no-op round-trip before the string reaches the API response model, which deserializes it once anyway.

This PR removes the three identical if (Type == Custom) blocks from GetOrganizationUserUserDetails, Get, and GetAccountRecoveryEnrolledUsers, and drops the now-unused using Bit.Core.Utilities import.

An API integration test is added to OrganizationUsersControllerGetTests to verify that Custom-user permissions survive the full pipeline (DB → query → response model → JSON) correctly.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the removal of the Permissions deserialize/re-serialize round-trip from OrganizationUserUserDetailsQuery, the deletion of the now-dead GetOrganizationUserUserDetails method and its interface member, and the accompanying test cleanup. The refactor is behavior-preserving for the only consumer, OrganizationUsersController.GetAll: OrganizationUserUserDetailsResponseModel already deserializes the raw Permissions string via CoreHelpers.LoadClassFromJsonData<Permissions> with the same options the removed ClassToJsonData used. Confirmed GetOrganizationUserUserDetails has zero remaining references across src/, bitwarden_license/, and test/, that GetPermissions() remains used by other call sites (Admin portal, public API, UserDecryptionOptionsBuilder), and that the two removed usings in the controller test are not needed elsewhere in that file.

Code Review Details

• ♻️ : GetMany_Setup no longer stubs anything GetAll reads, so GetMany_ReturnsUsers asserts over an empty result set and passes vacuously (pre-existing, but adjacent to this cleanup)
  • test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs:548

The only behavioral delta is a Custom user whose Permissions column is NULL/empty: the response permissions changes from null to an all-false object, which is semantically equivalent and already the shape returned for every other user — not flagged.

PR Metadata Assessment

• ❓ QUESTION: The description still claims an API integration test was added to OrganizationUsersControllerGetTests, but no such file exists in the diff or the repo — worth updating to match the final scope.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.64%. Comparing base (fc4c606) to head (d060b68).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8119      +/-   ##
==========================================
- Coverage   68.12%   63.64%   -4.49%     
==========================================
  Files        2373     2373              
  Lines      102846   102816      -30     
  Branches     9328     9325       -3     
==========================================
- Hits        70068    65437    -4631     
- Misses      30454    35150    +4696     
+ Partials     2324     2229      -95     

☔ View full report in Codecov by Harness.
📢 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.

[jrmccannon]

No notes.

[claude]

♻️ DEBT: With the stub removed, GetMany_Setup no longer arranges anything GetAll reads, so GetMany_ReturnsUsers passes vacuously.

Details

OrganizationUsersController.GetAll resolves members through _organizationUserUserDetailsQuery.Get(request) (or GetAccountRecoveryEnrolledUsers), never IOrganizationUserRepository.GetManyDetailsByOrganizationAsync. That repository stub therefore has no effect, and the auto-mocked IOrganizationUserUserDetailsQuery returns an empty sequence, so response.Data is empty and Assert.True(response.Data.All(r => organizationUsers.Any(ou => ou.Id == r.Id))) is true regardless of the injected organizationUsers.

Since this PR is a dead-code cleanup, consider finishing the job in the setup: stub the query instead, e.g.

sutProvider.GetDependency<IOrganizationUserUserDetailsQuery>()
    .Get(Arg.Any<OrganizationUserUserDetailsQueryRequest>())
    .Returns(organizationUsers.Select(ou => (ou, false, false)));

(or delete GetMany_ReturnsUsers if the coverage is not worth restoring). Note the vacuity predates this PR — the removed stub was on a method GetAll never called — so this is optional.