-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[PM-38927] - Extract Organziation User Role Validation #7876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| ο»Ώusing Bit.Core.AdminConsole.Utilities.v2; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; | ||
|
|
||
| /// <summary> | ||
| /// Returned when an acting user attempts to act on (or assign) an organization role that outranks their own. | ||
| /// </summary> | ||
| public record CannotManageHigherRoleError() | ||
| : BadRequestError("You cannot perform this action on a member with a higher organization role than your own."); | ||
|
|
||
| /// <summary> | ||
| /// Returned when the acting user has no authority to manage members for the action β for example a regular | ||
| /// User, or a Custom user without the required permission. | ||
| /// </summary> | ||
| public record MissingManagePermissionError() | ||
| : BadRequestError("You do not have permission to manage organization members."); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| ο»Ώusing Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; | ||
|
|
||
| /// <summary> | ||
| /// Whether an acting user may manage organization members at all, independent of any target. | ||
| /// <see cref="OrganizationUserActionRequest"/> adds a target role for the full role-escalation check. | ||
| /// </summary> | ||
| /// <param name="ActingUserRole">The acting user's role, or <c>null</c> if not a confirmed member (they may still be authorized via <paramref name="IsProviderUserForOrg"/>).</param> | ||
| /// <param name="ActingUserPermissions">The acting user's custom permissions. Only consulted for Custom users.</param> | ||
| /// <param name="PermissionPicker">Picks the permission that authorizes a Custom user for this action (e.g. <c>p => p.ManageUsers</c>). Only consulted for Custom users.</param> | ||
| /// <param name="IsProviderUserForOrg">Whether the acting user is a provider user for the org, which grants Owner-level authority. Invoked last because it hits the database.</param> | ||
| public record OrganizationUserManageMembersRequest( | ||
| OrganizationUserType? ActingUserRole, | ||
| Permissions? ActingUserPermissions, | ||
| Func<Permissions, bool> PermissionPicker, | ||
| Func<Task<bool>> IsProviderUserForOrg); | ||
|
|
||
| /// <summary> | ||
| /// Extends <see cref="OrganizationUserManageMembersRequest"/> (see it for the acting-user fields) with the | ||
| /// target role, to decide whether the acting user may manage (or assign) that role without escalating privileges. | ||
| /// </summary> | ||
| /// <param name="TargetUserRole">The role being acted upon β an existing member's current role, or the new role being assigned.</param> | ||
| public record OrganizationUserActionRequest( | ||
| OrganizationUserType? ActingUserRole, | ||
| Permissions? ActingUserPermissions, | ||
| Func<Permissions, bool> PermissionPicker, | ||
| Func<Task<bool>> IsProviderUserForOrg, | ||
| OrganizationUserType TargetUserRole) | ||
| : OrganizationUserManageMembersRequest(ActingUserRole, ActingUserPermissions, PermissionPicker, IsProviderUserForOrg); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| ο»Ώusing Bit.Core.AdminConsole.Utilities.v2.Validation; | ||
| using Bit.Core.Enums; | ||
| using static Bit.Core.AdminConsole.Utilities.v2.Validation.ValidationResultHelpers; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; | ||
|
|
||
| /// <summary> | ||
| /// Validates that an acting user is permitted to manage a target member without escalating privileges, | ||
| /// according to Bitwarden's organization role hierarchy: | ||
| /// <list type="bullet"> | ||
| /// <item>Owners (and provider users) can manage Owners, Admins, or Users.</item> | ||
| /// <item>Admins can manage Admins or Users.</item> | ||
| /// <item>Custom users can manage Users and other Custom users, but only when they hold the manage permission required for the action.</item> | ||
| /// <item>All other members (including regular Users) cannot manage any member.</item> | ||
| /// </list> | ||
| /// A role can only be raised to a level the acting user already holds (e.g. only an Owner can grant the | ||
| /// Owner role). | ||
| /// </summary> | ||
| public static class OrganizationUserActionValidator | ||
| { | ||
| /// <summary> | ||
| /// Validates whether the acting user can manage members at all, independent of any target. Use this where | ||
| /// authorization needs the gate without a specific target. Returns valid, or a <see cref="MissingManagePermissionError"/>. | ||
| /// </summary> | ||
| public static async Task<ValidationResult<OrganizationUserManageMembersRequest>> ValidateCanManageMembersAsync(OrganizationUserManageMembersRequest request) | ||
| { | ||
| var isProvider = !IsAuthorizedByRole(request) && await request.IsProviderUserForOrg(); | ||
|
|
||
| return IsAuthorizedByRole(request) || isProvider | ||
| ? Valid(request) | ||
| : Invalid(request, new MissingManagePermissionError()); | ||
| } | ||
|
Comment on lines
+21
to
+32
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can already do simple RBAC checks well at the controller layer via attributes. I don't think we need to duplicate it here. This would be limited to the escalation concern. |
||
|
|
||
| /// <summary> | ||
| /// Validates whether the acting user may manage (or assign) the target role: the management-authority gate | ||
| /// first, then role escalation. Returns valid, a <see cref="MissingManagePermissionError"/> if they cannot | ||
| /// manage members at all, or a <see cref="CannotManageHigherRoleError"/> if the target role outranks their authority. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Covers management authority and role escalation only. Callers remain responsible for other checks such as | ||
| /// self-actions, system/automated users, and confirmed-owner counts. | ||
| /// </remarks> | ||
| public static async Task<ValidationResult<OrganizationUserActionRequest>> ValidateAsync(OrganizationUserActionRequest request) | ||
| { | ||
| // First, the management-authority gate (shared with authorization). | ||
| var canManageResult = await ValidateCanManageMembersAsync(request); | ||
| if (canManageResult.IsError) | ||
| { | ||
| return Invalid(request, canManageResult.AsError); | ||
| } | ||
|
|
||
| // The acting user can manage members; confirm the target's role is within their authority. A provider | ||
| // user has Owner-level authority, so treat them as an Owner. The expensive provider lookup already ran | ||
| // inside the gate above, so we reuse the cheap synchronous role check here rather than repeating it. | ||
| var effectiveRole = IsAuthorizedByRole(request) ? request.ActingUserRole : OrganizationUserType.Owner; | ||
|
Comment on lines
+52
to
+55
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be simplified, and it's better to avoid implicit logic such as:
I would just check the role first, then check provider status if required. If we are doing multiple provider lookups there are better ways to handle that - e.g. push it out to the caller to pass a memoized Func or just a concrete boolean value. |
||
|
|
||
| return request.TargetUserRole switch | ||
| { | ||
| OrganizationUserType.Owner when effectiveRole is OrganizationUserType.Owner => Valid(request), | ||
| OrganizationUserType.Admin when effectiveRole is OrganizationUserType.Owner or OrganizationUserType.Admin => Valid(request), | ||
| OrganizationUserType.User or OrganizationUserType.Custom | ||
| when effectiveRole is OrganizationUserType.Owner or OrganizationUserType.Admin or OrganizationUserType.Custom => Valid(request), | ||
| _ => Invalid(request, new CannotManageHigherRoleError()) | ||
| }; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Whether the acting user can manage members by their organization role alone, without the provider lookup: | ||
| /// Owners and Admins always, and Custom users holding the action's permission. Cheap and synchronous, so it | ||
| /// is safe to evaluate more than once. | ||
| /// </summary> | ||
| private static bool IsAuthorizedByRole(OrganizationUserManageMembersRequest request) => | ||
| request.ActingUserRole switch | ||
| { | ||
| OrganizationUserType.Owner => true, | ||
| OrganizationUserType.Admin => true, | ||
| OrganizationUserType.Custom when request.ActingUserPermissions != null | ||
| && request.PermissionPicker(request.ActingUserPermissions) => true, | ||
| _ => false | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| ο»Ώusing Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
| using Xunit; | ||
|
|
||
| namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; | ||
|
|
||
| public class OrganizationUserActionValidatorTests | ||
| { | ||
| private static OrganizationUserActionRequest BuildRequest( | ||
| OrganizationUserType? actingRole, | ||
| OrganizationUserType targetRole, | ||
| bool hasManagePermission = false, | ||
| bool isProvider = false) | ||
| { | ||
| // A null acting role represents a non-member (e.g. a provider-only user). | ||
| var permissions = actingRole is null | ||
| ? null | ||
| : new Permissions { ManageUsers = hasManagePermission }; | ||
|
|
||
| return new OrganizationUserActionRequest( | ||
| actingRole, | ||
| permissions, | ||
| p => p.ManageUsers, | ||
| () => Task.FromResult(isProvider), | ||
| targetRole); | ||
| } | ||
|
|
||
| private static OrganizationUserManageMembersRequest BuildManageMembersRequest( | ||
| OrganizationUserType? actingRole, | ||
| bool hasManagePermission = false, | ||
| bool isProvider = false) | ||
| { | ||
| // A null acting role represents a non-member (e.g. a provider-only user). | ||
| var permissions = actingRole is null | ||
| ? null | ||
| : new Permissions { ManageUsers = hasManagePermission }; | ||
|
|
||
| return new OrganizationUserManageMembersRequest( | ||
| actingRole, | ||
| permissions, | ||
| p => p.ManageUsers, | ||
| () => Task.FromResult(isProvider)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(OrganizationUserType.Owner, false, OrganizationUserType.Owner)] | ||
| [InlineData(OrganizationUserType.Owner, false, OrganizationUserType.Admin)] | ||
| [InlineData(OrganizationUserType.Owner, false, OrganizationUserType.User)] | ||
| [InlineData(OrganizationUserType.Owner, false, OrganizationUserType.Custom)] | ||
| [InlineData(OrganizationUserType.Admin, false, OrganizationUserType.Admin)] | ||
| [InlineData(OrganizationUserType.Admin, false, OrganizationUserType.User)] | ||
| [InlineData(OrganizationUserType.Admin, false, OrganizationUserType.Custom)] | ||
| [InlineData(OrganizationUserType.Custom, true, OrganizationUserType.User)] | ||
| [InlineData(OrganizationUserType.Custom, true, OrganizationUserType.Custom)] | ||
| public async Task ValidateAsync_WhenActingUserCanManageTargetRole_ReturnsSuccess( | ||
| OrganizationUserType actingRole, | ||
| bool actingUserHasManagePermission, | ||
| OrganizationUserType targetRole) | ||
| { | ||
| // Note: Owners and Admins manage by role, so the manage permission is irrelevant for them. | ||
| var request = BuildRequest(actingRole, targetRole, hasManagePermission: actingUserHasManagePermission); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateAsync(request); | ||
|
|
||
| Assert.True(result.IsValid); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(OrganizationUserType.Admin, true, OrganizationUserType.Owner)] | ||
| [InlineData(OrganizationUserType.Custom, true, OrganizationUserType.Owner)] | ||
| [InlineData(OrganizationUserType.Custom, true, OrganizationUserType.Admin)] | ||
| public async Task ValidateAsync_WhenTargetRoleOutranksActingUser_ReturnsCannotManageHigherRoleError( | ||
| OrganizationUserType actingRole, | ||
| bool actingUserHasManagePermission, | ||
| OrganizationUserType targetRole) | ||
| { | ||
| var request = BuildRequest(actingRole, targetRole, hasManagePermission: actingUserHasManagePermission); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateAsync(request); | ||
|
|
||
| Assert.True(result.IsError); | ||
| Assert.IsType<CannotManageHigherRoleError>(result.AsError); | ||
| } | ||
|
|
||
| [Theory] | ||
| // A regular User cannot manage any member, regardless of target role. | ||
| [InlineData(OrganizationUserType.User, OrganizationUserType.Owner)] | ||
| [InlineData(OrganizationUserType.User, OrganizationUserType.Admin)] | ||
| [InlineData(OrganizationUserType.User, OrganizationUserType.User)] | ||
| [InlineData(OrganizationUserType.User, OrganizationUserType.Custom)] | ||
| public async Task ValidateAsync_WhenActingUserIsRegularUser_ReturnsMissingManagePermissionError( | ||
| OrganizationUserType actingRole, | ||
| OrganizationUserType targetRole) | ||
| { | ||
| var request = BuildRequest(actingRole, targetRole); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateAsync(request); | ||
|
|
||
| Assert.True(result.IsError); | ||
| Assert.IsType<MissingManagePermissionError>(result.AsError); | ||
| } | ||
|
|
||
| [Theory] | ||
| // A Custom user without the manage permission has no authority over any member. | ||
| [InlineData(OrganizationUserType.Owner)] | ||
| [InlineData(OrganizationUserType.Admin)] | ||
| [InlineData(OrganizationUserType.User)] | ||
| [InlineData(OrganizationUserType.Custom)] | ||
| public async Task ValidateAsync_WhenCustomUserLacksManagePermission_ReturnsMissingManagePermissionError( | ||
| OrganizationUserType targetRole) | ||
| { | ||
| var request = BuildRequest(OrganizationUserType.Custom, targetRole, hasManagePermission: false); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateAsync(request); | ||
|
|
||
| Assert.True(result.IsError); | ||
| Assert.IsType<MissingManagePermissionError>(result.AsError); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(OrganizationUserType.Owner)] | ||
| [InlineData(OrganizationUserType.Admin)] | ||
| [InlineData(OrganizationUserType.User)] | ||
| [InlineData(OrganizationUserType.Custom)] | ||
| public async Task ValidateAsync_WhenActingUserIsProvider_ReturnsSuccessForAnyTargetRole( | ||
| OrganizationUserType targetRole) | ||
| { | ||
| // A provider user has Owner-level authority. They are not a member of the organization, so their | ||
| // role/permissions are null and authority comes solely from the provider callback. | ||
| var request = BuildRequest(actingRole: null, targetRole, isProvider: true); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateAsync(request); | ||
|
|
||
| Assert.True(result.IsValid); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ValidateAsync_WhenAuthorizedByRole_DoesNotInvokeProviderCallback() | ||
| { | ||
| // The provider check hits the database, so it must not be invoked when the role/permission checks | ||
| // already authorize the action. | ||
| var providerCallbackInvoked = false; | ||
| var request = new OrganizationUserActionRequest( | ||
| OrganizationUserType.Owner, | ||
| new Permissions(), | ||
| p => p.ManageUsers, | ||
| () => | ||
| { | ||
| providerCallbackInvoked = true; | ||
| return Task.FromResult(false); | ||
| }, | ||
| OrganizationUserType.User); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateAsync(request); | ||
|
|
||
| Assert.True(result.IsValid); | ||
| Assert.False(providerCallbackInvoked); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(OrganizationUserType.Owner)] | ||
| [InlineData(OrganizationUserType.Admin)] | ||
| public async Task ValidateCanManageMembersAsync_WhenOwnerOrAdmin_ReturnsSuccess( | ||
| OrganizationUserType actingRole) | ||
| { | ||
| var request = BuildManageMembersRequest(actingRole); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateCanManageMembersAsync(request); | ||
|
|
||
| Assert.True(result.IsValid); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(true)] | ||
| [InlineData(false)] | ||
| public async Task ValidateCanManageMembersAsync_WhenCustomUser_DependsOnManagePermission( | ||
| bool hasManagePermission) | ||
| { | ||
| var request = BuildManageMembersRequest(OrganizationUserType.Custom, hasManagePermission); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateCanManageMembersAsync(request); | ||
|
|
||
| Assert.Equal(hasManagePermission, result.IsValid); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ValidateCanManageMembersAsync_WhenProvider_ReturnsSuccess() | ||
| { | ||
| var request = BuildManageMembersRequest(actingRole: null, isProvider: true); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateCanManageMembersAsync(request); | ||
|
|
||
| Assert.True(result.IsValid); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ValidateCanManageMembersAsync_WhenRegularUser_ReturnsMissingManagePermissionError() | ||
| { | ||
| var request = BuildManageMembersRequest(OrganizationUserType.User); | ||
|
|
||
| var result = await OrganizationUserActionValidator.ValidateCanManageMembersAsync(request); | ||
|
|
||
| Assert.True(result.IsError); | ||
| Assert.IsType<MissingManagePermissionError>(result.AsError); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really flexible but I think it degrades the interface. Simplified:
OrganizationUser? actingUserOrganizationUser targetUserbool actingUserIsProviderThen if this is a service method - they can easily be passed as arguments.