[PM-42240] perms still returned for members who were previously custom - #8244
[PM-42240] perms still returned for members who were previously custom#8244JaredScar wants to merge 2 commits into
Conversation
- Updated logic to clear permissions for non-Custom roles to prevent stale data. - Adjusted response models to ensure permissions are only returned for Custom roles. - Added unit tests to verify that permissions are correctly managed during role changes. - Ensured consistency across models and response handling for organization users.
…ests - Added comments for clarity on the optional Permissions property for backward compatibility. - Updated unit tests in OrganizationUserControllerPutTests to ensure permissions are cleared when converting from Custom roles. - Adjusted setup methods to include model parameters for consistency in tests.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR stops custom permissions from being persisted or returned for members who are not in the Code Review Details
Minor, not blocking: the public API change in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (organizationUserType != OrganizationUserType.Custom) | ||
| { | ||
| // Custom permissions only apply to the Custom role. Clear them so a member demoted from Custom doesn't | ||
| // keep a stale permissions blob. | ||
| Permissions = null; | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: Was a one-time cleanup of existing stale Permissions rows considered?
Why this may still matter after this PR
This PR stops new stale blobs from being written and hides them in OrganizationUserResponseModel / ProfileOrganizationResponseModel, but rows already in the database keep their blob until the member is edited again. A couple of server-side consumers read GetPermissions() without a Custom role check and are not covered by the response-model gating:
src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs:177—organizationUser.GetPermissions() is { ManageResetPassword: true }is OR'd with the Admin/Owner check, so a member demoted from Custom with a staleManageResetPassword: trueblob would still getHasManageResetPasswordPermission = truein the TDE decryption options (which drives the post-SSO "must set a master password" flow).src/Core/AdminConsole/Services/Implementations/OrganizationDomainService.cs:116—o.GetPermissions()?.ManageSso == truekeeps those members on the domain-verification notification list.
Authorization itself looks safe — CoreHelpers.BuildIdentityClaims only emits permission claims for the Custom branch, so CurrentContext.ManageUsers/ManageResetPassword etc. are unaffected.
If a migration is out of scope here, a follow-up ticket (or adding a Type is Custom guard at those two call sites) would close the loop.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8244 +/- ##
==========================================
+ Coverage 63.29% 68.57% +5.28%
==========================================
Files 2401 2401
Lines 104043 104055 +12
Branches 9426 9432 +6
==========================================
+ Hits 65857 71360 +5503
+ Misses 35930 30341 -5589
- Partials 2256 2354 +98 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-42240
📔 Objective
Fixes bug found in QA where previously custom users would still send back the permissions map to the frontend even though it is now irrelevant if they are no longer a custom user.