[PM-12475] Add GroupUserAuthorizationHandler and related context for group user …#7514
[PM-12475] Add GroupUserAuthorizationHandler and related context for group user …#7514
Conversation
…assignments - Introduced GroupUserAuthorizationHandler to manage user assignment permissions within groups. - Added GroupUserAssignmentContext to encapsulate the necessary data for authorization checks. - Updated GroupsController and OrganizationUsersController to utilize the new authorization logic for user assignments. - Removed unused dependencies and streamlined authorization checks for better clarity and performance.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR extracts the "admin can't self-assign to a group" check into a new Code Review Details
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public class GroupUserAuthorizationHandler( | ||
| ICurrentContext currentContext, | ||
| IApplicationCacheService applicationCacheService, | ||
| IOrganizationUserRepository organizationUserRepository, | ||
| IGroupRepository groupRepository) | ||
| : AuthorizationHandler<GroupUserOperationRequirement, GroupUserAssignmentContext> | ||
| { | ||
| protected override async Task HandleRequirementAsync( | ||
| AuthorizationHandlerContext context, | ||
| GroupUserOperationRequirement requirement, | ||
| GroupUserAssignmentContext resource) | ||
| { | ||
| var authorized = requirement.Name switch | ||
| { | ||
| nameof(GroupUserOperations.AssignUsers) => await CanAssignUsersAsync(resource), | ||
| _ => false | ||
| }; | ||
|
|
||
| if (authorized) | ||
| { | ||
| context.Succeed(requirement); | ||
| } | ||
| } | ||
|
|
||
| private async Task<bool> CanAssignUsersAsync(GroupUserAssignmentContext resource) | ||
| { | ||
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | ||
|
|
||
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | ||
| if (orgAbility.AllowAdminAccessToAllCollectionItems) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (currentContext.UserId is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| var organizationUser = await organizationUserRepository.GetByOrganizationAsync( | ||
| resource.OrganizationId, currentContext.UserId.Value); | ||
|
|
||
| // Providers are not org members and are exempt from this restriction. | ||
| if (organizationUser is null) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // If the caller's own OrganizationUser ID is not among the requested users, there is no self-assignment. | ||
| if (!resource.RequestedUserIds.Contains(organizationUser.Id)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // When a GroupId is provided, check whether the caller is already a member of the group. | ||
| // Keeping an existing membership is permitted; only newly adding oneself is blocked. | ||
| if (resource.GroupId.HasValue) | ||
| { | ||
| var currentGroupUsers = await groupRepository.GetManyUserIdsByIdAsync(resource.GroupId.Value); | ||
| if (currentGroupUsers.Contains(organizationUser.Id)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Details and fix
CONTRIBUTING / CLAUDE.md require unit tests for new feature development, and the existing controller-level coverage in GroupsControllerPutTests no longer exercises this logic end-to-end once the authorization check is delegated to IAuthorizationService. Please add a GroupUserAuthorizationHandlerTests class covering at least:
AllowAdminAccessToAllCollectionItems = true→ succeedscurrentContext.UserId is null→ fails- caller is a provider (org user not found) → succeeds
- caller's org user id is not in
RequestedUserIds→ succeeds - caller's org user id is in
RequestedUserIds,GroupIdnull → fails - caller's org user id is in
RequestedUserIds,GroupIdset, already a member → succeeds - caller's org user id is in
RequestedUserIds,GroupIdset, not a member → fails
This is also the natural place to move the scenarios currently in GroupsControllerPutTests that rely on mocked IApplicationCacheService / IGroupRepository / IOrganizationUserRepository — the controller tests can then just verify that it calls AuthorizeAsync with the right context and throws/short-circuits based on the result.
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | ||
|
|
||
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | ||
| if (orgAbility.AllowAdminAccessToAllCollectionItems) |
There was a problem hiding this comment.
orgAbility can be null and is dereferenced without a guard.
Details and fix
IApplicationCacheService.GetOrganizationAbilityAsync returns Task<OrganizationAbility?> and returns null when the org id is missing from the cached dictionary (see InMemoryApplicationCacheService.GetOrganizationAbilityAsync). In #nullable enable code, orgAbility.AllowAdminAccessToAllCollectionItems dereferences a possibly-null reference.
If the org ability is unexpectedly absent, the handler will throw NullReferenceException instead of making a deterministic authorization decision. Use the pattern already established in this codebase (e.g. GetCipherPermissionsForUserQuery, BulkCollectionAuthorizationHandler):
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | |
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | |
| if (orgAbility.AllowAdminAccessToAllCollectionItems) | |
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | |
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | |
| if (orgAbility is { AllowAdminAccessToAllCollectionItems: true }) | |
| { | |
| return true; | |
| } |
This also treats "ability not found" as the restrictive branch rather than an exception, which is the safer default for an authorization decision.
| // Authorization check: | ||
| // If admins are not allowed access to all collections, you cannot add yourself to a group. | ||
| // No error is thrown for this, we just don't update groups. | ||
| var orgAbility = await _applicationCacheService.GetOrganizationAbilityAsync(orgId); | ||
| if (!orgAbility.AllowAdminAccessToAllCollectionItems) | ||
| var groupUserAssignment = new GroupUserAssignmentContext(orgId, model.Users, GroupId: id); | ||
| if (!(await _authorizationService.AuthorizeAsync(User, groupUserAssignment, GroupUserOperations.AssignUsers)).Succeeded) | ||
| { | ||
| var userId = _userService.GetProperUserId(User).Value; | ||
| var organizationUser = await _organizationUserRepository.GetByOrganizationAsync(orgId, userId); | ||
| var currentGroupUsers = await _groupRepository.GetManyUserIdsByIdAsync(id); | ||
| // OrganizationUser may be null if the current user is a provider | ||
| if (organizationUser != null && !currentGroupUsers.Contains(organizationUser.Id) && model.Users.Contains(organizationUser.Id)) | ||
| { | ||
| throw new BadRequestException("You cannot add yourself to groups."); | ||
| } | ||
| throw new BadRequestException("You cannot add yourself to groups."); | ||
| } |
There was a problem hiding this comment.
Details and fix
test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs covers the four self-assignment scenarios (Put_UpdateMembers_NoAdminAccess_CannotAddSelfToGroup, ..._AlreadyInGroup_Success, ..._ProviderUser_Success, ..._WithAdminAccess_CanAddSelfToGroup) by mocking IApplicationCacheService, IGroupRepository, IUserService, and IOrganizationUserRepository directly on the controller.
Now that the controller calls _authorizationService.AuthorizeAsync(User, groupUserAssignment, GroupUserOperations.AssignUsers), those mocks are never read by this logic and IAuthorizationService returns a default Task<AuthorizationResult> whose result is null, causing .Succeeded to NRE. The same issue applies to OrganizationUserControllerPutTests.Put_NoAdminAccess_CannotAddSelfToGroups / Put_WithAdminAccess_CanAddSelfToGroups. This is likely why the "Run tests" CI job is failing.
Please either stub the new AuthorizeAsync call in each affected test or migrate the scenarios into a new GroupUserAuthorizationHandlerTests class (preferred — keeps the coverage where the logic now lives).
| public class GroupUserAuthorizationHandler( | ||
| ICurrentContext currentContext, | ||
| IApplicationCacheService applicationCacheService, | ||
| IOrganizationUserRepository organizationUserRepository, | ||
| IGroupRepository groupRepository) | ||
| : AuthorizationHandler<GroupUserOperationRequirement, GroupUserAssignmentContext> | ||
| { | ||
| protected override async Task HandleRequirementAsync( | ||
| AuthorizationHandlerContext context, | ||
| GroupUserOperationRequirement requirement, | ||
| GroupUserAssignmentContext resource) | ||
| { | ||
| var authorized = requirement.Name switch | ||
| { | ||
| nameof(GroupUserOperations.AssignUsers) => await CanAssignUsersAsync(resource), | ||
| _ => false | ||
| }; | ||
|
|
||
| if (authorized) | ||
| { | ||
| context.Succeed(requirement); | ||
| } | ||
| } | ||
|
|
||
| private async Task<bool> CanAssignUsersAsync(GroupUserAssignmentContext resource) | ||
| { | ||
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | ||
|
|
||
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | ||
| if (orgAbility.AllowAdminAccessToAllCollectionItems) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (currentContext.UserId is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| var organizationUser = await organizationUserRepository.GetByOrganizationAsync( | ||
| resource.OrganizationId, currentContext.UserId.Value); | ||
|
|
||
| // Providers are not org members and are exempt from this restriction. | ||
| if (organizationUser is null) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // If the caller's own OrganizationUser ID is not among the requested users, there is no self-assignment. | ||
| if (!resource.RequestedUserIds.Contains(organizationUser.Id)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // When a GroupId is provided, check whether the caller is already a member of the group. | ||
| // Keeping an existing membership is permitted; only newly adding oneself is blocked. | ||
| if (resource.GroupId.HasValue) | ||
| { | ||
| var currentGroupUsers = await groupRepository.GetManyUserIdsByIdAsync(resource.GroupId.Value); | ||
| if (currentGroupUsers.Contains(organizationUser.Id)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Details and fix
CONTRIBUTING / CLAUDE.md require unit tests for new feature development, and the existing controller-level coverage in GroupsControllerPutTests no longer exercises this logic end-to-end once the authorization check is delegated to IAuthorizationService. Please add a GroupUserAuthorizationHandlerTests class covering at least:
AllowAdminAccessToAllCollectionItems = true→ succeedscurrentContext.UserId is null→ fails- caller is a provider (org user not found) → succeeds
- caller's org user id is not in
RequestedUserIds→ succeeds - caller's org user id is in
RequestedUserIds,GroupIdnull → fails - caller's org user id is in
RequestedUserIds,GroupIdset, already a member → succeeds - caller's org user id is in
RequestedUserIds,GroupIdset, not a member → fails
This is also the natural place to move the scenarios currently in GroupsControllerPutTests that rely on mocked IApplicationCacheService / IGroupRepository / IOrganizationUserRepository — the controller tests can then just verify that it calls AuthorizeAsync with the right context and throws/short-circuits based on the result.
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | ||
|
|
||
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | ||
| if (orgAbility.AllowAdminAccessToAllCollectionItems) |
There was a problem hiding this comment.
orgAbility can be null and is dereferenced without a guard.
Details and fix
IApplicationCacheService.GetOrganizationAbilityAsync returns Task<OrganizationAbility?> and returns null when the org id is missing from the cached dictionary (see InMemoryApplicationCacheService.GetOrganizationAbilityAsync). In #nullable enable code, orgAbility.AllowAdminAccessToAllCollectionItems dereferences a possibly-null reference.
If the org ability is unexpectedly absent, the handler will throw NullReferenceException instead of making a deterministic authorization decision, and the previous call site in GroupsController.Put had the same latent issue. Use the pattern already established in this codebase (e.g. GetCipherPermissionsForUserQuery, BulkCollectionAuthorizationHandler):
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | |
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | |
| if (orgAbility.AllowAdminAccessToAllCollectionItems) | |
| var orgAbility = await applicationCacheService.GetOrganizationAbilityAsync(resource.OrganizationId); | |
| // When admins have unrestricted collection access, self-assignment to groups is permitted. | |
| if (orgAbility is { AllowAdminAccessToAllCollectionItems: true }) | |
| { | |
| return true; | |
| } |
This also treats "ability not found" as the restrictive branch rather than an exception, which is the safer default for an authorization decision.
|
Great job! No new security vulnerabilities introduced in this pull request |
…ning-users-to-groups
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12475
📔 Objective
Instead of having duplicated code in places for authorization for assigning users to group authorization, implement an authorization service to be used in both places to de-duplicate (is that a word?) code.