[PM-38136] centralize role escalation validation - #8218
Conversation
…permissions checks - Introduced IOrganizationUserValidationService to centralize permission checks for managing organization users. - Updated RemoveOrganizationUserCommand and RevokeOrganizationUserCommand to utilize the new validation service for role management. - Refactored validation logic in DeleteClaimedOrganizationUserAccountValidator and RevokeOrganizationUsersValidator to streamline error handling. - Removed deprecated error messages and replaced them with more accurate ones reflecting the new validation logic. - Improved null safety and organization context handling across user management commands.
…and error handling - Removed deprecated error messages related to user restoration permissions. - Introduced IOrganizationUserValidationService for centralized validation of user management actions. - Refactored RestoreOrganizationUserCommand to utilize the new validation service, enhancing clarity and maintainability. - Improved error handling for user restoration scenarios, ensuring accurate feedback based on user roles and permissions.
…logic - Introduced IOrganizationUserValidationService to streamline permission checks for recovering organization user accounts. - Refactored RecoverAccountAuthorizationHandler to utilize the new validation service, improving clarity and maintainability. - Updated authorization logic to align with the Owner > Admin > Custom > User hierarchy, specifically for ManageResetPassword permissions. - Removed deprecated methods and improved error handling for account recovery scenarios, ensuring accurate feedback based on user roles and permissions.
JaredScar
added
ai-review
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR centralizes the "can the acting user manage this target member" role-escalation rules into Code Review Details
Also worth a look, but not blocking: several user-facing error messages change as a side effect of consolidation (e.g. "Your account does not have permission to manage users." now surfaces as "Custom users can not manage Admins or Owners." for a non-Custom actor). The updated tests show this is intentional, but the new message is misleading in that case and clients may match on the old strings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var actingUser = standardUser.OrganizationUserType.HasValue | ||
| ? new OrganizationUserRole(standardUser.OrganizationUserType.Value, request.OrganizationId, standardUser.Permissions) | ||
| : null; |
There was a problem hiding this comment.
❌ CRITICAL: Every real caller reaches here with OrganizationUserType == null, so all non-compliant revocations are now denied.
Details and fix
The only StandardUser paths into this command are SingleOrgPolicyEventHandler / TwoFactorAuthenticationPolicyEventHandler, and both read policyUpdate.PerformedBy, not the new ?? new StandardUser(..., actingOrganization?.Type, ...) fallback added in this PR. PolicyUpdate.PerformedBy is populated in PolicyRequestModel.ToPolicyUpdateAsync (line 19) and VerifyOrganizationDomainCommand (line 43) as:
new StandardUser(currentContext.UserId!.Value, await currentContext.OrganizationOwner(organizationId))— i.e. orgUserType/permissions are always null, so the fallback in the handlers is dead code on every request path.
Result: actingUser is null → IsAuthorizedByRole(null, ...) is false → IsProviderAsync is false for a normal Owner/Admin → CannotManageError for every target. commandResult.HasErrors is then true and the handler throws BadRequestException. Concretely, an Owner enabling the Two-Step Login or Single Organization policy (or verifying an organization domain, which auto-enables SingleOrg) in an org with any non-compliant member now fails with "Custom users can not manage Admins or Owners." repeated per member. Previously only Owner-typed targets were restricted, and those are already filtered out of the target list by the handlers.
Two fixes are needed:
- Populate the acting user's role in
PolicyRequestModel.ToPolicyUpdateAsync,SavePolicyRequest.ToSavePolicyModelAsync, andVerifyOrganizationDomainCommand(as was done for the controller atOrganizationUsersController.cs:709). - Keep a safe fallback here so a
StandardUserwith no role claim isn't silently denied — e.g. honourstandardUser.IsOrganizationOwnerOrProviderwhenOrganizationUserTypeis null.
The existing tests don't catch this: CreateActingUser in RevokeNonCompliantOrganizationUserCommandTests always supplies an explicit OrganizationUserType, so the null-type path is untested.
| var targetsById = requests | ||
| .Where(r => r.OrganizationUser is not null) | ||
| .ToDictionary(r => r.OrganizationUserId, r => (IOrganizationUserRole)r.OrganizationUser!); |
There was a problem hiding this comment.
ToDictionary throws on duplicate ids in a bulk delete request, turning a previously-tolerated payload into a 500.
Details and fix
DeleteClaimedOrganizationUserAccountCommand.CreateInternalRequests yields one request per entry of the caller-supplied orgUserIds without de-duplicating, and OrganizationUserBulkRequestModel.Ids is an unvalidated IEnumerable<Guid> from the request body. So DELETE /organizations/{orgId}/users/delete-account with {"ids": ["<same-guid>", "<same-guid>"]} reaches this line with two requests sharing the same OrganizationUserId and throws ArgumentException: An item with the same key has already been added. Before this PR each duplicate was simply validated independently.
Suggested fix:
var targetsById = requests
.Where(r => r.OrganizationUser is not null)
.GroupBy(r => r.OrganizationUserId)
.ToDictionary(g => g.Key, g => (IOrganizationUserRole)g.First().OrganizationUser!);(Or de-duplicate orgUserIds in the command.) The same pattern in RemoveOrganizationUserCommand / RestoreOrganizationUserCommand is safe because those lists come from a repository query.
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
I had a quick look over this, at a glance the service logic is already straining trying to accommodate these different flows with an easy-to-use interface. @jrmccannon can you please take a close look at this while I'm OOO? tyvm
| /// <summary> | ||
| /// Swaps the auto-mocked <see cref="IOrganizationUserValidationService"/> for a real instance backed by | ||
| /// mocked repositories, so the handler's role-hierarchy logic (Owner/Admin/Custom/User + provider override) | ||
| /// actually runs instead of just returning a NSubstitute default. | ||
| /// </summary> |
There was a problem hiding this comment.
We shouldn't do this in unit tests; this is what integration tests are for.
|
|
||
| if (!canRecoverOrganizationMember) | ||
| var manageError = await organizationUserValidationService.CanManageAsync( | ||
| currentContext.UserId ?? Guid.Empty, actingUser, targetOrganizationUser, |
There was a problem hiding this comment.
Do not pass Guid.Empty to comply with a non-nullable parameter. This interface expects a real userId; if the caller can't provide one, the caller needs to handle that itself.
| var actingUser = actingOrganization is null | ||
| ? null | ||
| : new OrganizationUserRole(actingOrganization.Type, targetOrganizationUser.OrganizationId, actingOrganization.Permissions); |
There was a problem hiding this comment.
Could CurrentContextOrganization just implement this interface directly?
| Task<Error?> CanManageAsync(Guid actingUserId, IOrganizationUserRole? actingUser, IOrganizationUserRole targetUser, | ||
| Func<Permissions, bool>? customPermissionGate = null); |
There was a problem hiding this comment.
customPermissionGate is not very intuitive and is poor encapsulation - it's easy for the caller to misunderstand this or pass the wrong callback, and it's tightly coupled with the logic of the method. Is there a way to redraw the interface to avoid this?
| /// <param name="targetUsersById">The members being managed, keyed by their <c>OrganizationUserId</c>.</param> | ||
| /// <param name="customPermissionGate">See <see cref="CanManageAsync(Guid, IOrganizationUserRole?, IOrganizationUserRole, Func{Permissions, bool}?)"/>.</param> | ||
| /// <returns>A per-target result, keyed by <c>OrganizationUserId</c>; <c>null</c> means allowed.</returns> | ||
| Task<IReadOnlyDictionary<Guid, Error?>> CanManageAsync(Guid actingUserId, IOrganizationUserRole? actingUser, |
There was a problem hiding this comment.
IReadOnlyDictionary<Guid, Error?> seems like it's recreating something like BulkCommandResult, or IEnumerable<ValidationResult>. It's neither a command nor a validator, so presumably it shouldn't use those, but we should have something more strongly typed than this.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38136
📔 Objective
Will centralize the role checking into a single service rather than having it across 7+ different places