-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[PM-38923] feat: Add v2 UpdateOrganizationUser command with role-escalation validation #7919
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
Changes from all commits
1499299
c65cf58
be9f0c7
ffbdd80
b874aef
b72ffe3
71e1b13
ad9f9ff
b361f7d
6842cb9
76e15a4
0cd8623
22b0edd
32225c4
710a99b
3ab47a6
03016b0
0107db6
cea4547
5a0cd43
6f31321
1a07035
dde4b4a
08e0866
34d3ab4
a90bc0a
7a40b38
dc28076
c16dfe0
c1a6bfa
5a0fd82
3ddea95
81de970
788165f
c4f8472
0aa2c2e
a078464
f60221e
d561aac
d2860ce
09dea82
cc62bef
170b035
b5ca2ab
ad23593
d5e4b8c
793af00
ae887c4
cd2c54a
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 |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ | |
| using Bit.Api.AdminConsole.Models.Response.Organizations; | ||
| using Bit.Api.Models.Response; | ||
| using Bit.Core; | ||
| using Bit.Core.AdminConsole.AbilitiesCache; | ||
| using Bit.Core.AdminConsole.Entities; | ||
| using Bit.Core.AdminConsole.Models.Data; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.AccountRecovery; | ||
|
|
@@ -37,6 +36,7 @@ | |
| using Bit.Core.Exceptions; | ||
| using Bit.Core.Models.Api; | ||
| using Bit.Core.Models.Business; | ||
| using Bit.Core.Models.Data; | ||
| using Bit.Core.Models.Data.Organizations.OrganizationUsers; | ||
| using Bit.Core.OrganizationFeatures.OrganizationSubscriptions.Interface; | ||
| using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces; | ||
|
|
@@ -49,6 +49,7 @@ | |
| using Microsoft.AspNetCore.Mvc; | ||
| using V1_RevokeOrganizationUserCommand = Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v1.IRevokeOrganizationUserCommand; | ||
| using V2_RevokeOrganizationUserCommand = Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; | ||
| using V2_UpdateUserCommand = Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; | ||
|
|
||
| namespace Bit.Api.AdminConsole.Controllers; | ||
|
|
||
|
|
@@ -68,7 +69,6 @@ public class OrganizationUsersController : BaseAdminConsoleController | |
| private readonly IUpdateOrganizationUserCommand _updateOrganizationUserCommand; | ||
| private readonly IAcceptOrgUserCommand _acceptOrgUserCommand; | ||
| private readonly IAuthorizationService _authorizationService; | ||
| private readonly IOrganizationAbilityCacheService _organizationAbilityCacheService; | ||
| private readonly ISsoConfigRepository _ssoConfigRepository; | ||
| private readonly IOrganizationUserUserDetailsQuery _organizationUserUserDetailsQuery; | ||
| private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand; | ||
|
|
@@ -91,6 +91,8 @@ public class OrganizationUsersController : BaseAdminConsoleController | |
| private readonly IUpdateUserResetPasswordEnrollmentCommand _updateUserResetPasswordEnrollmentCommand; | ||
| private readonly IAcceptOrganizationInviteLinkCommand _acceptOrganizationInviteLinkCommand; | ||
| private readonly IConfirmOrganizationInviteLinkCommand _confirmOrganizationInviteLinkCommand; | ||
| private readonly Bitwarden.Server.Sdk.Features.IFeatureService _featureService; | ||
| private readonly V2_UpdateUserCommand.IUpdateOrganizationUserCommand _updateOrganizationUserCommandVNext; | ||
|
|
||
| public OrganizationUsersController(IOrganizationRepository organizationRepository, | ||
| IOrganizationUserRepository organizationUserRepository, | ||
|
|
@@ -104,7 +106,6 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor | |
| IUpdateOrganizationUserCommand updateOrganizationUserCommand, | ||
| IAcceptOrgUserCommand acceptOrgUserCommand, | ||
| IAuthorizationService authorizationService, | ||
| IOrganizationAbilityCacheService organizationAbilityCacheService, | ||
| ISsoConfigRepository ssoConfigRepository, | ||
| IOrganizationUserUserDetailsQuery organizationUserUserDetailsQuery, | ||
| IRemoveOrganizationUserCommand removeOrganizationUserCommand, | ||
|
|
@@ -126,7 +127,9 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor | |
| IUpdateUserResetPasswordEnrollmentCommand updateUserResetPasswordEnrollmentCommand, | ||
| IGetPendingAutoConfirmUsersQuery getPendingAutoConfirmUsersQuery, | ||
| IAcceptOrganizationInviteLinkCommand acceptOrganizationInviteLinkCommand, | ||
| IConfirmOrganizationInviteLinkCommand confirmOrganizationInviteLinkCommand) | ||
| IConfirmOrganizationInviteLinkCommand confirmOrganizationInviteLinkCommand, | ||
| Bitwarden.Server.Sdk.Features.IFeatureService featureService, | ||
| V2_UpdateUserCommand.IUpdateOrganizationUserCommand updateOrganizationUserCommandVNext) | ||
| { | ||
| _organizationRepository = organizationRepository; | ||
| _organizationUserRepository = organizationUserRepository; | ||
|
|
@@ -140,7 +143,6 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor | |
| _updateOrganizationUserCommand = updateOrganizationUserCommand; | ||
| _acceptOrgUserCommand = acceptOrgUserCommand; | ||
| _authorizationService = authorizationService; | ||
| _organizationAbilityCacheService = organizationAbilityCacheService; | ||
| _ssoConfigRepository = ssoConfigRepository; | ||
| _organizationUserUserDetailsQuery = organizationUserUserDetailsQuery; | ||
| _removeOrganizationUserCommand = removeOrganizationUserCommand; | ||
|
|
@@ -163,6 +165,8 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor | |
| _updateUserResetPasswordEnrollmentCommand = updateUserResetPasswordEnrollmentCommand; | ||
| _acceptOrganizationInviteLinkCommand = acceptOrganizationInviteLinkCommand; | ||
| _confirmOrganizationInviteLinkCommand = confirmOrganizationInviteLinkCommand; | ||
| _featureService = featureService; | ||
| _updateOrganizationUserCommandVNext = updateOrganizationUserCommandVNext; | ||
| } | ||
|
|
||
| [HttpGet("{id}")] | ||
|
|
@@ -398,10 +402,11 @@ public async Task<ListResponseModel<OrganizationUserPublicKeyResponseModel>> Use | |
|
|
||
| [HttpPut("{id}")] | ||
| [Authorize<ManageUsersRequirement>] | ||
| public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) | ||
| public async Task<IResult> Put([BindOrganization] Organization organization, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) | ||
| { | ||
| var (organizationUser, currentAccess) = await _organizationUserRepository.GetByIdWithCollectionsAsync(id); | ||
| if (organizationUser == null || organizationUser.OrganizationId != orgId) | ||
|
|
||
| if (organizationUser == null || organizationUser.OrganizationId != organization.Id) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
@@ -412,72 +417,110 @@ public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequ | |
| // 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 organizationAbility = await _organizationAbilityCacheService.GetOrganizationAbilityAsync(orgId); | ||
| var groupsToSave = editingSelf && !organizationAbility.AllowAdminAccessToAllCollectionItems | ||
| var groupsToSave = editingSelf && !organization.AllowAdminAccessToAllCollectionItems | ||
| ? null | ||
| : model.Groups; | ||
|
|
||
| var currentAccessIds = currentAccess.Select(c => c.Id).ToHashSet(); | ||
| var existingUserType = organizationUser.Type; | ||
|
|
||
| if (_featureService.IsEnabled(FeatureFlagKeys.ChangeMemberEmailNoMp)) | ||
| { | ||
| var (collectionsToSave, postedCollections) = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess); | ||
|
|
||
| var actingContext = _currentContext.GetOrganization(organization.Id); | ||
|
|
||
| var request = new V2_UpdateUserCommand.UpdateOrganizationUserRequest( | ||
| organizationUser, | ||
| organization, | ||
| currentAccessIds, | ||
| postedCollections, | ||
| model.Type.Value, | ||
|
jrmccannon marked this conversation as resolved.
|
||
| model.Permissions, | ||
| model.AccessSecretsManager, | ||
| collectionsToSave, | ||
| groupsToSave, | ||
| new StandardUser( | ||
| userId, | ||
| await _currentContext.OrganizationOwner(organization.Id), | ||
| actingContext?.Type, | ||
| actingContext?.Permissions), | ||
| model.DefaultUserCollectionName); | ||
|
|
||
| var result = await _updateOrganizationUserCommandVNext.UpdateUserAsync(request); | ||
| return Handle(result); | ||
|
jrmccannon marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Authorization check: | ||
| // If admins are not allowed access to all collections, you cannot add yourself to collections. | ||
| // This is not caught by the requirement below that you can ModifyUserAccess and must be checked separately | ||
| var currentAccessIds = currentAccess.Select(c => c.Id).ToHashSet(); | ||
| if (editingSelf && | ||
| !organizationAbility.AllowAdminAccessToAllCollectionItems && | ||
| !organization.AllowAdminAccessToAllCollectionItems && | ||
| model.Collections.Any(c => !currentAccessIds.Contains(c.Id))) | ||
| { | ||
| throw new BadRequestException("You cannot add yourself to a collection."); | ||
| } | ||
|
|
||
| var (collectionsToSaveV1, _) = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess); | ||
|
|
||
| await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), existingUserType, userId, | ||
| collectionsToSaveV1, groupsToSave, model.DefaultUserCollectionName); | ||
|
|
||
| return TypedResults.Ok(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Resolves the collection access to persist for an organization user update. Collections the saving user | ||
| /// cannot <see cref="BulkCollectionOperations.ModifyUserAccess"/> are validated and the user's current | ||
| /// access to them is preserved so it is not accidentally overwritten. | ||
| /// </summary> | ||
| private async Task<(List<CollectionAccessSelection> CollectionsToSave, ICollection<Collection> Collections)> GetAuthorizedCollectionsToSaveAsync(OrganizationUserUpdateRequestModel model, ICollection<CollectionAccessSelection> currentAccess) | ||
| { | ||
| // Authorization check: | ||
| // You must have authorization to ModifyUserAccess for all collections being saved | ||
| var postedCollections = await _collectionRepository | ||
| .GetManyByManyIdsAsync(model.Collections.Select(c => c.Id)); | ||
| foreach (var collection in postedCollections) | ||
| { | ||
| if (!(await _authorizationService.AuthorizeAsync(User, collection, | ||
| BulkCollectionOperations.ModifyUserAccess)) | ||
| .Succeeded) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
| // You must have authorization to ModifyUserAccess for all collections being saved. | ||
| var postedCollections = await _collectionRepository.GetManyByManyIdsAsync(model.Collections.Select(c => c.Id)); | ||
| if (postedCollections.Count != 0 && | ||
| !(await _authorizationService.AuthorizeAsync(User, postedCollections, BulkCollectionOperations.ModifyUserAccess)).Succeeded) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| // The client only sends collections that the saving user has permissions to edit. | ||
| // We need to combine these with collections that the user doesn't have permissions for, so that we don't | ||
| // accidentally overwrite those | ||
| var currentCollections = await _collectionRepository | ||
| .GetManyByManyIdsAsync(currentAccess.Select(cas => cas.Id)); | ||
| var currentCollections = await _collectionRepository.GetManyByManyIdsAsync(currentAccess.Select(cas => cas.Id)); | ||
|
|
||
| var readonlyCollectionIds = new HashSet<Guid>(); | ||
|
|
||
| foreach (var collection in currentCollections) | ||
| { | ||
| if (!(await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyUserAccess)) | ||
| .Succeeded) | ||
| if (!(await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ModifyUserAccess)).Succeeded) | ||
| { | ||
| readonlyCollectionIds.Add(collection.Id); | ||
| } | ||
| } | ||
|
|
||
| var editedCollectionAccess = model.Collections | ||
| .Select(c => c.ToSelectionReadOnly()); | ||
| // Default user collections ("My Items") are never managed through this flow. Exclude any current-access | ||
| // defaults from the preserved set so they are not re-saved; posted defaults are rejected downstream. | ||
| var defaultCollectionIds = postedCollections | ||
| .Concat(currentCollections) | ||
| .Where(c => c.Type == CollectionType.DefaultUserCollection) | ||
| .Select(c => c.Id) | ||
| .ToHashSet(); | ||
|
Comment on lines
+503
to
+509
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 have very low confidence in this existing code so I don't want to modify it here if we can help it. I think this will break, because an admin never has write access to a My Items collection, yet it'll be excluded from Moving it into a private method makes sense (for re-use between v1 and v2 flows) but the guts of it shouldn't change. |
||
|
|
||
| var editedCollectionAccess = model.Collections.Select(c => c.ToSelectionReadOnly()); | ||
| var readonlyCollectionAccess = currentAccess | ||
| .Where(ca => readonlyCollectionIds.Contains(ca.Id)); | ||
| .Where(ca => readonlyCollectionIds.Contains(ca.Id) && !defaultCollectionIds.Contains(ca.Id)); | ||
| var collectionsToSave = editedCollectionAccess | ||
| .Concat(readonlyCollectionAccess) | ||
| .ToList(); | ||
|
|
||
| var existingUserType = organizationUser.Type; | ||
|
|
||
| await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), existingUserType, userId, | ||
| collectionsToSave, groupsToSave, model.DefaultUserCollectionName); | ||
| } | ||
| var collections = postedCollections | ||
| .Concat(currentCollections) | ||
| .DistinctBy(c => c.Id) | ||
| .ToList(); | ||
|
|
||
| [HttpPost("{id}")] | ||
| [Obsolete("This endpoint is deprecated. Use PUT method instead")] | ||
| [Authorize<ManageUsersRequirement>] | ||
| public async Task PostPut(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) | ||
| { | ||
| await Put(orgId, id, model); | ||
| return (collectionsToSave, collections); | ||
| } | ||
|
|
||
| [HttpPut("{userId}/reset-password-enrollment")] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| ο»Ώusing Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
|
|
||
| namespace Bit.Core.AdminConsole.Models.Data; | ||
|
|
||
| /// <summary> | ||
| /// The minimal role information needed to authorize whether one member can manage another. | ||
| /// </summary> | ||
| public interface IOrganizationUserRole | ||
| { | ||
| OrganizationUserType Type { get; } | ||
| Guid OrganizationId { get; } | ||
| Permissions? GetPermissions(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| ο»Ώusing Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
|
|
||
| namespace Bit.Core.AdminConsole.Models.Data; | ||
|
|
||
| /// <summary> | ||
| /// A lightweight <see cref="IOrganizationUserRole"/> for when a full OrganizationUser isn't available. | ||
| /// </summary> | ||
| public sealed record OrganizationUserRole( | ||
| OrganizationUserType Type, | ||
| Guid OrganizationId, | ||
| Permissions? Permissions = null) : IOrganizationUserRole | ||
| { | ||
| public Permissions? GetPermissions() => Permissions; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,15 @@ | ||
| ο»Ώusing Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
|
|
||
| namespace Bit.Core.AdminConsole.Models.Data; | ||
|
|
||
| public class StandardUser : IActingUser | ||
| public class StandardUser(Guid userId, bool isOrganizationOwner, OrganizationUserType? orgUserType = null, | ||
| Permissions? permissions = null) : IActingUser | ||
| { | ||
| public StandardUser(Guid userId, bool isOrganizationOwner) | ||
| { | ||
| UserId = userId; | ||
| IsOrganizationOwnerOrProvider = isOrganizationOwner; | ||
| } | ||
|
|
||
| public Guid? UserId { get; } | ||
| public bool IsOrganizationOwnerOrProvider { get; } | ||
| public Guid? UserId { get; } = userId; | ||
| public bool IsOrganizationOwnerOrProvider { get; } = isOrganizationOwner; | ||
| public OrganizationUserType? OrganizationUserType { get; } = orgUserType; | ||
| public Permissions? Permissions { get; } = permissions; | ||
| public EventSystemUser? SystemUserType => throw new Exception($"{nameof(StandardUser)} does not have a {nameof(SystemUserType)}"); | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.