From ff413ea5acfeafdee8c5a92fdabf9024f72a4d58 Mon Sep 17 00:00:00 2001 From: Jared McCannon Date: Thu, 2 Jul 2026 14:29:42 -0500 Subject: [PATCH 1/4] [PM-38923] feat: Add v2 UpdateOrganizationUser command with role-escalation validation Refactor the OrganizationUsersController.Put flow into a v2 command + validator (returning CommandResult / ValidationResult), gated behind the pm-28365-change-member-email-no-mp feature flag. The validator ports v1's request validation and adds the escalating-role guard by delegating to the shared IOrganizationUserValidationService.CanManage rules (PM-38927), while preserving v1's specific error messages. The controller supplies the acting user's membership (role + permissions) as a plain field so Core stays free of ICurrentContext. --- .../OrganizationUsersController.cs | 111 +++-- .../OrganizationUsers/UpdateUser/v2/Errors.cs | 16 + .../v2/IUpdateOrganizationUserCommand.cs | 8 + .../v2/IUpdateOrganizationUserValidator.cs | 13 + .../v2/UpdateOrganizationUserCommand.cs | 80 ++++ .../v2/UpdateOrganizationUserRequest.cs | 58 +++ .../v2/UpdateOrganizationUserValidator.cs | 213 ++++++++++ ...OrganizationServiceCollectionExtensions.cs | 4 + .../OrganizationUserControllerPutTests.cs | 15 +- .../OrganizationUsersControllerTests.cs | 84 ++++ .../v2/UpdateOrganizationUserCommandTests.cs | 191 +++++++++ .../UpdateOrganizationUserValidatorTests.cs | 397 ++++++++++++++++++ 12 files changed, 1151 insertions(+), 39 deletions(-) create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserCommand.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserValidator.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs create mode 100644 test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs create mode 100644 test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index d436b1a7ad3f..1c7591c32648 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -37,6 +37,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 +50,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; @@ -90,6 +92,8 @@ public class OrganizationUsersController : BaseAdminConsoleController private readonly ISelfRevokeOrganizationUserCommand _selfRevokeOrganizationUserCommand; private readonly IUpdateUserResetPasswordEnrollmentCommand _updateUserResetPasswordEnrollmentCommand; private readonly IAcceptOrganizationInviteLinkCommand _acceptOrganizationInviteLinkCommand; + private readonly IFeatureService _featureService; + private readonly V2_UpdateUserCommand.IUpdateOrganizationUserCommand _updateOrganizationUserCommandVNext; public OrganizationUsersController(IOrganizationRepository organizationRepository, IOrganizationUserRepository organizationUserRepository, @@ -124,7 +128,9 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor ISelfRevokeOrganizationUserCommand selfRevokeOrganizationUserCommand, IUpdateUserResetPasswordEnrollmentCommand updateUserResetPasswordEnrollmentCommand, IGetPendingAutoConfirmUsersQuery getPendingAutoConfirmUsersQuery, - IAcceptOrganizationInviteLinkCommand acceptOrganizationInviteLinkCommand) + IAcceptOrganizationInviteLinkCommand acceptOrganizationInviteLinkCommand, + IFeatureService featureService, + V2_UpdateUserCommand.IUpdateOrganizationUserCommand updateOrganizationUserCommandVNext) { _organizationRepository = organizationRepository; _organizationUserRepository = organizationUserRepository; @@ -160,6 +166,8 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor _selfRevokeOrganizationUserCommand = selfRevokeOrganizationUserCommand; _updateUserResetPasswordEnrollmentCommand = updateUserResetPasswordEnrollmentCommand; _acceptOrganizationInviteLinkCommand = acceptOrganizationInviteLinkCommand; + _featureService = featureService; + _updateOrganizationUserCommandVNext = updateOrganizationUserCommandVNext; } [HttpGet("{id}")] @@ -393,10 +401,13 @@ public async Task> Use [HttpPut("{id}")] [Authorize] - public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) + public async Task Put([BindOrganization] Organization organization, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) { var (organizationUser, currentAccess) = await _organizationUserRepository.GetByIdWithCollectionsAsync(id); - if (organizationUser == null || organizationUser.OrganizationId != orgId) + + var organizationAbility = await _organizationAbilityCacheService.GetOrganizationAbilityAsync(organization.Id); + + if (organizationUser == null || organizationUser.OrganizationId != organization.Id || organizationAbility == null) { throw new NotFoundException(); } @@ -407,15 +418,48 @@ 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 ? null : model.Groups; + var currentAccessIds = currentAccess.Select(c => c.Id).ToHashSet(); + var existingUserType = organizationUser.Type; + + if (_featureService.IsEnabled(FeatureFlagKeys.ChangeMemberEmailNoMp)) + { + // The self-add-to-collection check is handled by the v2 validator. + var collectionsToSave = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess); + + // The acting user's own membership drives the role-escalation check. Null when the caller is not + // an organization member (e.g. a provider), whose authority comes from the owner/provider flag. + var actingContext = _currentContext.GetOrganization(organization.Id); + OrganizationUser savingOrganizationUser = null; + if (actingContext != null) + { + savingOrganizationUser = new OrganizationUser { Type = actingContext.Type }; + savingOrganizationUser.SetPermissions(actingContext.Permissions); + } + + var request = new V2_UpdateUserCommand.UpdateOrganizationUserRequest( + organizationUser, + organization, + organizationAbility, + model.Type.Value, + model.Permissions, + model.AccessSecretsManager, + new StandardUser(userId, await _currentContext.OrganizationOwner(organization.Id)), + savingOrganizationUser, + collectionsToSave, + groupsToSave, + currentAccessIds); + + var result = await _updateOrganizationUserCommandVNext.UpdateUserAsync(request); + return Handle(result); + } + // 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 && model.Collections.Any(c => !currentAccessIds.Contains(c.Id))) @@ -423,56 +467,59 @@ public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequ 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); + + return TypedResults.Ok(); + } + + /// + /// Resolves the collection access to persist for an organization user update. Collections the saving user + /// cannot are validated and the user's current + /// access to them is preserved so it is not accidentally overwritten. + /// + private async Task> GetAuthorizedCollectionsToSaveAsync(OrganizationUserUpdateRequestModel model, ICollection 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. The bulk handler + // authorizes the whole set in a single call (it succeeds only if every collection is authorized). + 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(); + 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()); - var readonlyCollectionAccess = currentAccess - .Where(ca => readonlyCollectionIds.Contains(ca.Id)); - var collectionsToSave = editedCollectionAccess + var editedCollectionAccess = model.Collections.Select(c => c.ToSelectionReadOnly()); + var readonlyCollectionAccess = currentAccess.Where(ca => readonlyCollectionIds.Contains(ca.Id)); + return editedCollectionAccess .Concat(readonlyCollectionAccess) .ToList(); - - var existingUserType = organizationUser.Type; - - await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), existingUserType, userId, - collectionsToSave, groupsToSave); } [HttpPost("{id}")] [Obsolete("This endpoint is deprecated. Use PUT method instead")] [Authorize] - public async Task PostPut(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) + public async Task PostPut([BindOrganization] Organization organization, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) { - await Put(orgId, id, model); + return await Put(organization, id, model); } [HttpPut("{userId}/reset-password-enrollment")] diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs new file mode 100644 index 000000000000..bb49d75cef85 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs @@ -0,0 +1,16 @@ +using Bit.Core.AdminConsole.Utilities.v2; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +// Use generic "Resource not found." messages to avoid enumeration, matching the v1 NotFoundException behavior. +public record CollectionNotFound() : NotFoundError("Resource not found."); +public record GroupNotFound() : NotFoundError("Resource not found."); + +public record InviteUserFirst() : BadRequestError("Invite the user first."); +public record CannotBeAdminOfMultipleFreeOrgs() : BadRequestError("User can only be an admin of one free organization."); +public record CannotAddSelfToCollection() : BadRequestError("You cannot add yourself to a collection."); +public record MustHaveConfirmedOwner() : BadRequestError("Organization must have at least one confirmed owner."); +public record OnlyOwnersCanManageOwners() : BadRequestError("Only an Owner can manage another Owner's account."); +public record CustomUsersCannotManageAdminsOrOwners() : BadRequestError("Custom users can not manage Admins or Owners."); +public record ManageMutuallyExclusive() : BadRequestError("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); +public record CustomPermissionsNotEnabled() : BadRequestError("To enable custom permissions the organization must be on an Enterprise plan."); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserCommand.cs new file mode 100644 index 000000000000..fd5642e0136a --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserCommand.cs @@ -0,0 +1,8 @@ +using Bit.Core.AdminConsole.Utilities.v2.Results; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +public interface IUpdateOrganizationUserCommand +{ + Task UpdateUserAsync(UpdateOrganizationUserRequest request); +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserValidator.cs new file mode 100644 index 000000000000..bb16e3ad4a4f --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/IUpdateOrganizationUserValidator.cs @@ -0,0 +1,13 @@ +using Bit.Core.AdminConsole.Utilities.v2.Validation; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +public interface IUpdateOrganizationUserValidator +{ + /// + /// Validates an organization user update. On success, the returned request carries the + /// collection access list with default user collections filtered out, ready to persist. + /// + Task> ValidateAsync( + UpdateOrganizationUserValidationRequest request); +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs new file mode 100644 index 000000000000..7dcea8ce235c --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs @@ -0,0 +1,80 @@ +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.Utilities.v2.Results; +using Bit.Core.Billing.Pricing; +using Bit.Core.Enums; +using Bit.Core.Models.Business; +using Bit.Core.OrganizationFeatures.OrganizationSubscriptions.Interface; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Utilities; +using OneOf.Types; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +public class UpdateOrganizationUserCommand( + IOrganizationUserRepository organizationUserRepository, + IUpdateOrganizationUserValidator validator, + ICountNewSmSeatsRequiredQuery countNewSmSeatsRequiredQuery, + IUpdateSecretsManagerSubscriptionCommand updateSecretsManagerSubscriptionCommand, + IPricingClient pricingClient, + IEventService eventService, + TimeProvider timeProvider) + : IUpdateOrganizationUserCommand +{ + public async Task UpdateUserAsync(UpdateOrganizationUserRequest request) + { + var (organizationUser, organization, organizationAbility) = (request.OrganizationUser, request.Organization, request.OrganizationAbility); + + var validationResult = await validator.ValidateAsync(new UpdateOrganizationUserValidationRequest( + organizationUser, + request.Type, + request.PerformedBy, + request.PerformedByOrganizationUser, + request.CollectionsToSave?.ToList() ?? [], + request.GroupsToSave, + request.CurrentAccessIds, + organization, + organizationAbility)); + + if (validationResult.IsError) + { + return validationResult.AsError; + } + + // The validator returns the collection access with default user collections filtered out. + var collectionsToSave = validationResult.Request.CollectionsToSave; + var groupsToSave = validationResult.Request.GroupsToSave; + + var enablingSecretsManager = !organizationUser.AccessSecretsManager && request.AccessSecretsManager; + + organizationUser.Type = request.Type; + organizationUser.Permissions = CoreHelpers.ClassToJsonData(request.Permissions); + organizationUser.AccessSecretsManager = request.AccessSecretsManager; + + // Only autoscale (if required) after all validation has passed so that we know it's a valid request before + // updating Stripe. + if (enablingSecretsManager) + { + var additionalSmSeatsRequired = await countNewSmSeatsRequiredQuery.CountNewSmSeatsRequiredAsync(organizationUser.OrganizationId, 1); + if (additionalSmSeatsRequired > 0) + { + // TODO: https://bitwarden.atlassian.net/browse/PM-17012 + var plan = await pricingClient.GetPlanOrThrow(organization.PlanType); + var update = new SecretsManagerSubscriptionUpdate(organization, plan, true) + .AdjustSeats(additionalSmSeatsRequired); + await updateSecretsManagerSubscriptionCommand.UpdateSubscriptionAsync(update); + } + } + + await organizationUserRepository.ReplaceAsync(organizationUser, collectionsToSave); + + if (groupsToSave != null) + { + await organizationUserRepository.UpdateGroupsAsync(organizationUser.Id, groupsToSave, timeProvider.GetUtcNow().UtcDateTime); + } + + await eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Updated); + + return new None(); + } +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs new file mode 100644 index 000000000000..d975775d6c99 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs @@ -0,0 +1,58 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +/// +/// The request to update an organization user. Carries the loaded database copy (its current, unmodified +/// state) together with the requested field changes — the two are kept separate, and the command applies +/// the changes onto the copy. Collection and group access are trusted as already-authorized by the caller +/// (the API layer resolves collection resource authorization). +/// +/// The loaded organization user in its current (unmodified) state. +/// The loaded organization the user belongs to. +/// The loaded organization ability for the user's organization. +/// The requested member role. +/// The requested custom permissions (used when is Custom). +/// Whether the user should have Secrets Manager access. +/// The actor making the change. +/// The acting user's own organization membership (its role and +/// permissions), used to prevent granting a role higher than their own. Null when the actor is not an +/// organization member (e.g. a provider), whose authority is instead governed by +/// . +/// The user's updated collection access. Null removes all collection access. +/// The user's updated group access. Null means groups are not updated. +/// The collection ids the user currently has access to (used for the self-add check). +public record UpdateOrganizationUserRequest( + OrganizationUser OrganizationUser, + Organization Organization, + OrganizationAbility OrganizationAbility, + OrganizationUserType Type, + Permissions? Permissions, + bool AccessSecretsManager, + IActingUser PerformedBy, + OrganizationUser? PerformedByOrganizationUser, + List? CollectionsToSave, + IEnumerable? GroupsToSave, + HashSet CurrentAccessIds); + +/// +/// The input to the validator: the in its current (unmodified) database state, +/// the requested it would be changed to, and the loaded and +/// . The validator checks the current state against the requested change; the +/// command only applies the change after validation succeeds. +/// +public record UpdateOrganizationUserValidationRequest( + OrganizationUser OrganizationUser, + OrganizationUserType NewType, + IActingUser PerformedBy, + OrganizationUser? PerformedByOrganizationUser, + List CollectionsToSave, + IEnumerable? GroupsToSave, + HashSet CurrentAccessIds, + Organization Organization, + OrganizationAbility OrganizationAbility); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs new file mode 100644 index 000000000000..37dc169b9e40 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs @@ -0,0 +1,213 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.AdminConsole.Utilities.v2; +using Bit.Core.AdminConsole.Utilities.v2.Validation; +using Bit.Core.Billing.Enums; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Bit.Core.Repositories; +using static Bit.Core.AdminConsole.Utilities.v2.Validation.ValidationResultHelpers; +using Organization = Bit.Core.AdminConsole.Entities.Organization; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +public class UpdateOrganizationUserValidator( + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository, + IGroupRepository groupRepository, + IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery, + IOrganizationUserValidationService organizationUserValidationService) + : IUpdateOrganizationUserValidator +{ + public async Task> ValidateAsync( + UpdateOrganizationUserValidationRequest request) + { + var organizationUser = request.OrganizationUser; + + // The user must already be invited to the organization. + if (organizationUser.Id == Guid.Empty) + { + return Invalid(request, new InviteUserFirst()); + } + + // A user can only be an admin or owner of a single free organization. + if (!await IsValidFreeOrganizationAdminAsync(organizationUser, request.NewType, request.Organization)) + { + return Invalid(request, new CannotBeAdminOfMultipleFreeOrgs()); + } + + // When admins are not allowed access to all collections, a user editing themselves cannot add + // themselves to collections they don't already have access to. + if (IsAddingSelfToCollection(request)) + { + return Invalid(request, new CannotAddSelfToCollection()); + } + + // All posted collections must exist and belong to the organization. Default user collections are + // filtered out of the set that is ultimately persisted. + var collectionsToSave = request.CollectionsToSave; + if (collectionsToSave.Count != 0) + { + var collections = await collectionRepository.GetManyByManyIdsAsync(collectionsToSave.Select(c => c.Id)); + if (!CollectionsAreValid(collectionsToSave, collections, organizationUser.OrganizationId)) + { + return Invalid(request, new CollectionNotFound()); + } + + collectionsToSave = ExcludeDefaultUserCollections(collectionsToSave, collections); + } + + // All posted groups must exist and belong to the organization. + if (request.GroupsToSave?.Any() == true) + { + var groupAccess = request.GroupsToSave.ToList(); + var groups = await groupRepository.GetManyByManyIds(groupAccess); + if (!GroupsAreValid(groupAccess, groups, organizationUser.OrganizationId)) + { + return Invalid(request, new GroupNotFound()); + } + } + + // A caller cannot grant a role higher than their own. Mirrors v1's + // OrganizationService.ValidateOrganizationUserUpdatePermissions escalation guard. + var escalationError = ValidateNotEscalatingAboveOwnRole(request); + if (escalationError is not null) + { + return Invalid(request, escalationError); + } + + // Custom permissions require an Enterprise plan. + if (request.NewType == OrganizationUserType.Custom && !request.Organization.UseCustomPermissions) + { + return Invalid(request, new CustomPermissionsNotEnabled()); + } + + // The organization must retain at least one confirmed owner after the update. + if (request.NewType != OrganizationUserType.Owner && + !await hasConfirmedOwnersExceptQuery.HasConfirmedOwnersExceptAsync(organizationUser.OrganizationId, + [organizationUser.Id])) + { + return Invalid(request, new MustHaveConfirmedOwner()); + } + + // The Manage permission is mutually exclusive with ReadOnly and HidePasswords. + if (collectionsToSave.Count > 0 && collectionsToSave.Any(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords))) + { + return Invalid(request, new ManageMutuallyExclusive()); + } + + return Valid(request with { CollectionsToSave = collectionsToSave }); + } + + private async Task IsValidFreeOrganizationAdminAsync(OrganizationUser organizationUser, OrganizationUserType newType, Organization organization) + { + if (organization.PlanType != PlanType.Free) + { + return true; + } + + if (!organizationUser.UserId.HasValue) + { + return true; + } + + if (newType is not (OrganizationUserType.Admin or OrganizationUserType.Owner)) + { + return true; + } + + // Since free organizations only support a few users there is not much point in avoiding N+1 queries for this. + var adminCount = await organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(organizationUser.UserId!.Value); + var isCurrentAdminOrOwner = organizationUser.Type is OrganizationUserType.Admin or OrganizationUserType.Owner; + + if (isCurrentAdminOrOwner && adminCount <= 1) + { + return true; + } + + if (!isCurrentAdminOrOwner && adminCount == 0) + { + return true; + } + + return false; + } + + /// + /// Prevents a caller from granting a role higher than their own (privilege escalation). Delegates the + /// authority decision to the shared rules, + /// checking both the member's current role and the requested role so an existing higher-ranked member + /// cannot be modified from below either. System users act with full authority (v1 gated this on a real + /// acting user), so the check is skipped for them. The v1-specific error messages are preserved by + /// mapping the denial back to which role was out of reach. + /// + private Error? ValidateNotEscalatingAboveOwnRole(UpdateOrganizationUserValidationRequest request) + { + if (request.PerformedBy is SystemUser) + { + return null; + } + + var actingUser = request.PerformedByOrganizationUser; + var isProvider = request.PerformedBy.IsOrganizationOwnerOrProvider; + + var canManageCurrentRole = + organizationUserValidationService.CanManage(actingUser, request.OrganizationUser, isProvider) is null; + var canManageNewRole = + organizationUserValidationService.CanManage(actingUser, new OrganizationUser { Type = request.NewType }, isProvider) is null; + + if (canManageCurrentRole && canManageNewRole) + { + return null; + } + + // Map the denial to v1's specific message. An Owner (current or requested) can only be managed by an + // Owner; everything else that trips the check is a Custom user reaching above their authority. + if (request.OrganizationUser.Type == OrganizationUserType.Owner || request.NewType == OrganizationUserType.Owner) + { + return new OnlyOwnersCanManageOwners(); + } + + return new CustomUsersCannotManageAdminsOrOwners(); + } + + private static bool IsAddingSelfToCollection(UpdateOrganizationUserValidationRequest request) + { + var editingSelf = request.PerformedBy is not SystemUser + && request.OrganizationUser.UserId.HasValue + && request.OrganizationUser.UserId == request.PerformedBy.UserId; + + return editingSelf + && !request.OrganizationAbility.AllowAdminAccessToAllCollectionItems + && request.CollectionsToSave.Any(c => !request.CurrentAccessIds.Contains(c.Id)); + } + + private static bool CollectionsAreValid(List collectionAccess, + ICollection collections, Guid organizationId) + { + var collectionIds = collections.Select(c => c.Id); + + var missingCollection = collectionAccess.FirstOrDefault(cas => !collectionIds.Contains(cas.Id)); + + return missingCollection == null && collections.All(c => c.OrganizationId == organizationId); + } + + private static List ExcludeDefaultUserCollections( + List collectionAccess, ICollection collections) => + collectionAccess + .Where(cas => collections.Any(c => c.Id == cas.Id && c.Type != CollectionType.DefaultUserCollection)) + .ToList(); + + private static bool GroupsAreValid(ICollection groupAccess, ICollection groups, Guid organizationId) + { + var groupIds = groups.Select(g => g.Id); + + var missingGroupId = groupAccess.FirstOrDefault(gId => !groupIds.Contains(gId)); + + return missingGroupId == Guid.Empty && groups.All(g => g.OrganizationId == organizationId); + } +} diff --git a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs index 52757374c3ab..d60be1327451 100644 --- a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs +++ b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs @@ -55,6 +55,7 @@ using Microsoft.Extensions.Logging; using V1_RevokeUsersCommand = Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v1; using V2_RevokeUsersCommand = Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.RevokeUser.v2; +using V2_UpdateUserCommand = Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; namespace Bit.Core.OrganizationFeatures; @@ -154,6 +155,9 @@ private static void AddOrganizationUserCommands(this IServiceCollection services services.AddScoped(); services.AddScoped(); services.AddScoped(); + + services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index 1ce22f2cfc0b..f0ab610cab4a 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -4,6 +4,7 @@ using Bit.Api.AdminConsole.Models.Request.Organizations; using Bit.Api.Models.Request; using Bit.Core.AdminConsole.AbilitiesCache; +using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.Context; using Bit.Core.Entities; @@ -46,7 +47,7 @@ public async Task Put_Success(OrganizationUserUpdateRequestModel model, var existingUserType = organizationUser.Type; // Act - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + await sutProvider.Sut.Put(new Organization { Id = organizationAbility.Id }, organizationUser.Id, model); // Assert await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => @@ -73,7 +74,7 @@ public async Task Put_NoAdminAccess_CannotAddSelfToCollections(OrganizationUserU Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); - var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(new Organization { Id = organizationAbility.Id }, organizationUser.Id, model)); Assert.Contains("You cannot add yourself to a collection.", exception.Message); } [Theory] @@ -97,7 +98,7 @@ public async Task Put_NoAdminAccess_CannotAddSelfToGroups(OrganizationUserUpdate var existingUserType = organizationUser.Type; // Act - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + await sutProvider.Sut.Put(new Organization { Id = organizationAbility.Id }, organizationUser.Id, model); // Assert await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => @@ -134,7 +135,7 @@ public async Task Put_WithAdminAccess_CanAddSelfToGroups(OrganizationUserUpdateR var existingUserType = organizationUser.Type; // Act - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + await sutProvider.Sut.Put(new Organization { Id = organizationAbility.Id }, organizationUser.Id, model); // Assert await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => @@ -211,7 +212,7 @@ public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections( var existingUserType = organizationUser.Type; // Act - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + await sutProvider.Sut.Put(new Organization { Id = organizationAbility.Id }, organizationUser.Id, model); // Assert // Expect all collection access (modified and unmodified) to be saved @@ -248,7 +249,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); - await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(new Organization { Id = organizationAbility.Id }, organizationUser.Id, model)); } [Theory] @@ -267,7 +268,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(O Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); - await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(new Organization { Id = organizationAbility.Id }, organizationUser.Id, model)); } private void Put_Setup(SutProvider sutProvider, diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index 8f31d2f94cd2..34dfec390a78 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -39,6 +39,7 @@ using NSubstitute; using OneOf.Types; using Xunit; +using V2_UpdateUserCommand = Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; namespace Bit.Api.Test.AdminConsole.Controllers; @@ -760,4 +761,87 @@ await sutProvider.GetDependency() .Received(1) .BulkResendInvitesAsync(organizationId, userId, bulkRequestModel.Ids); } + + [Theory] + [BitAutoData] + public async Task Put_WhenFeatureFlagEnabled_RoutesToV2AndReturnsNoContentOnSuccess( + Organization organization, OrganizationUserUpdateRequestModel model, Guid userId, OrganizationAbility organizationAbility, + OrganizationUser organizationUser, SutProvider sutProvider) + { + PutSetup(sutProvider, organization, organizationUser, organizationAbility, userId, featureEnabled: true); + + sutProvider.GetDependency() + .UpdateUserAsync(Arg.Any()) + .Returns(new CommandResult(new None())); + + var result = await sutProvider.Sut.Put(organization, organizationUser.Id, model); + + Assert.IsType(result); + await sutProvider.GetDependency() + .Received(1) + .UpdateUserAsync(Arg.Any()); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .UpdateUserAsync(default, default, default, default, default); + } + + [Theory] + [BitAutoData] + public async Task Put_WhenFeatureFlagEnabledAndCommandFails_MapsErrorToStatus( + Organization organization, OrganizationUserUpdateRequestModel model, Guid userId, OrganizationAbility organizationAbility, + OrganizationUser organizationUser, SutProvider sutProvider) + { + PutSetup(sutProvider, organization, organizationUser, organizationAbility, userId, featureEnabled: true); + + sutProvider.GetDependency() + .UpdateUserAsync(Arg.Any()) + .Returns(new CommandResult(new V2_UpdateUserCommand.MustHaveConfirmedOwner())); + + var result = await sutProvider.Sut.Put(organization, organizationUser.Id, model); + + Assert.IsType>(result); + } + + [Theory] + [BitAutoData] + public async Task Put_WhenFeatureFlagDisabled_RoutesToV1( + Organization organization, OrganizationUserUpdateRequestModel model, Guid userId, OrganizationAbility organizationAbility, + OrganizationUser organizationUser, SutProvider sutProvider) + { + PutSetup(sutProvider, organization, organizationUser, organizationAbility, userId, featureEnabled: false); + + var result = await sutProvider.Sut.Put(organization, organizationUser.Id, model); + + Assert.IsType(result); + await sutProvider.GetDependency() + .Received(1) + .UpdateUserAsync(Arg.Any(), Arg.Any(), userId, + Arg.Any>(), Arg.Any>()); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .UpdateUserAsync(default); + } + + private static void PutSetup(SutProvider sutProvider, Organization organization, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, Guid userId, bool featureEnabled) + { + organizationUser.OrganizationId = organization.Id; + organizationAbility.Id = organization.Id; + organizationAbility.AllowAdminAccessToAllCollectionItems = true; + + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>( + organizationUser, new List())); + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(userId); + sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id) + .Returns(organizationAbility); + sutProvider.GetDependency().GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List()); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Any(), Arg.Any>()) + .Returns(AuthorizationResult.Success()); + sutProvider.GetDependency().IsEnabled(Bit.Core.FeatureFlagKeys.ChangeMemberEmailNoMp) + .Returns(featureEnabled); + } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs new file mode 100644 index 000000000000..7b851927a694 --- /dev/null +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs @@ -0,0 +1,191 @@ +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; +using Bit.Core.AdminConsole.Utilities.v2.Validation; +using Bit.Core.Billing.Enums; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.OrganizationFeatures.OrganizationSubscriptions.Interface; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Test.AutoFixture.OrganizationUserFixtures; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; +using Organization = Bit.Core.AdminConsole.Entities.Organization; + +namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +[SutProviderCustomize] +public class UpdateOrganizationUserCommandTests +{ + [Theory] + [BitAutoData] + public async Task UpdateUserAsync_WhenValidatorReturnsError_ReturnsErrorAndDoesNotPersist( + SutProvider sutProvider, + Organization organization, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser) + { + var request = Setup(sutProvider, organization, organizationUser, valid: false); + + var result = await sutProvider.Sut.UpdateUserAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default, default(IEnumerable)); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .UpdateGroupsAsync(default, default, default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .LogOrganizationUserEventAsync(default(OrganizationUser), default); + } + + [Theory] + [BitAutoData] + public async Task UpdateUserAsync_OnSuccessWithGroups_ReplacesUserUpdatesGroupsAndLogsEvent( + SutProvider sutProvider, + Organization organization, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser, + Guid groupId) + { + var request = Setup(sutProvider, organization, organizationUser, groups: [groupId]); + + var result = await sutProvider.Sut.UpdateUserAsync(request); + + Assert.True(result.IsSuccess); + + await sutProvider.GetDependency() + .Received(1) + .ReplaceAsync(organizationUser, Arg.Any>()); + await sutProvider.GetDependency() + .Received(1) + .UpdateGroupsAsync(organizationUser.Id, Arg.Any>(), Arg.Any()); + await sutProvider.GetDependency() + .Received(1) + .LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_Updated); + } + + [Theory] + [BitAutoData] + public async Task UpdateUserAsync_OnSuccessWithNullGroups_DoesNotUpdateGroups( + SutProvider sutProvider, + Organization organization, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser) + { + var request = Setup(sutProvider, organization, organizationUser, groups: null); + + var result = await sutProvider.Sut.UpdateUserAsync(request); + + Assert.True(result.IsSuccess); + + await sutProvider.GetDependency() + .Received(1) + .ReplaceAsync(organizationUser, Arg.Any>()); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .UpdateGroupsAsync(default, default, default); + } + + [Theory] + [BitAutoData] + public async Task UpdateUserAsync_AppliesRequestedChangesToTheDatabaseCopy( + SutProvider sutProvider, + Organization organization, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser) + { + organizationUser.AccessSecretsManager = false; + var request = Setup(sutProvider, organization, organizationUser, + type: OrganizationUserType.Admin, targetAccessSecretsManager: true); + + var result = await sutProvider.Sut.UpdateUserAsync(request); + + Assert.True(result.IsSuccess); + Assert.Equal(OrganizationUserType.Admin, organizationUser.Type); + Assert.True(organizationUser.AccessSecretsManager); + } + + [Theory] + [BitAutoData] + public async Task UpdateUserAsync_WhenEnablingSecretsManager_ChecksRequiredSeats( + SutProvider sutProvider, + Organization organization, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser) + { + organizationUser.AccessSecretsManager = false; + var request = Setup(sutProvider, organization, organizationUser, targetAccessSecretsManager: true); + + sutProvider.GetDependency() + .CountNewSmSeatsRequiredAsync(organization.Id, 1) + .Returns(0); + + var result = await sutProvider.Sut.UpdateUserAsync(request); + + Assert.True(result.IsSuccess); + + await sutProvider.GetDependency() + .Received(1) + .CountNewSmSeatsRequiredAsync(organization.Id, 1); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .UpdateSubscriptionAsync(default); + } + + [Theory] + [BitAutoData] + public async Task UpdateUserAsync_WhenSecretsManagerAccessUnchanged_DoesNotCheckSeats( + SutProvider sutProvider, + Organization organization, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser organizationUser) + { + organizationUser.AccessSecretsManager = false; + var request = Setup(sutProvider, organization, organizationUser, targetAccessSecretsManager: false); + + var result = await sutProvider.Sut.UpdateUserAsync(request); + + Assert.True(result.IsSuccess); + + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .CountNewSmSeatsRequiredAsync(default, default); + } + + private static UpdateOrganizationUserRequest Setup( + SutProvider sutProvider, + Organization organization, + OrganizationUser organizationUser, + OrganizationUserType type = OrganizationUserType.User, + List collections = null, + IEnumerable groups = null, + bool valid = true, + bool targetAccessSecretsManager = false) + { + organization.PlanType = PlanType.EnterpriseAnnually; + organizationUser.OrganizationId = organization.Id; + + sutProvider.GetDependency() + .ValidateAsync(Arg.Any()) + .Returns(ci => valid + ? ValidationResultHelpers.Valid(ci.Arg()) + : ValidationResultHelpers.Invalid(ci.Arg(), new MustHaveConfirmedOwner())); + + return new UpdateOrganizationUserRequest( + organizationUser, + organization, + new OrganizationAbility { Id = organization.Id }, + type, + null, + targetAccessSecretsManager, + new StandardUser(organizationUser.UserId ?? Guid.NewGuid(), true), + new OrganizationUser { Type = OrganizationUserType.Owner }, + collections, + groups, + []); + } +} diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs new file mode 100644 index 000000000000..6cb88aaa6bdd --- /dev/null +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs @@ -0,0 +1,397 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Billing.Enums; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.Repositories; +using Bit.Core.Test.AutoFixture.OrganizationUserFixtures; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; +using Collection = Bit.Core.Entities.Collection; +using Organization = Bit.Core.AdminConsole.Entities.Organization; + +namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; + +[SutProviderCustomize] +public class UpdateOrganizationUserValidatorTests +{ + [Theory] + [BitAutoData] + public async Task ValidateAsync_WithNoCollectionsOrGroups_ReturnsValid( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsValid); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WithEmptyId_ReturnsInviteUserFirst( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + orgUser.Id = Guid.Empty; + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenBecomingAdminOfSecondFreeOrg_ReturnsCannotBeAdminOfMultipleFreeOrgs( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + orgUser.UserId = Guid.NewGuid(); + var organization = CreateOrganization(orgUser.OrganizationId, PlanType.Free); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Admin, organization: organization); + + sutProvider.GetDependency() + .GetCountByFreeOrganizationAdminUserAsync(orgUser.UserId!.Value) + .Returns(1); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenEditingSelfAndAddingNewCollection_ReturnsCannotAddSelfToCollection( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser, + Guid newCollectionId) + { + orgUser.UserId = Guid.NewGuid(); + var performedBy = new StandardUser(orgUser.UserId!.Value, false); + var ability = CreateAbility(orgUser.OrganizationId, allowAdminAccessToAllCollectionItems: false); + + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User, + performedBy: performedBy, + ability: ability, + collections: [new CollectionAccessSelection { Id = newCollectionId }], + currentAccessIds: []); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenCollectionDoesNotExist_ReturnsCollectionNotFound( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser, + Guid missingCollectionId) + { + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User, + collections: [new CollectionAccessSelection { Id = missingCollectionId }]); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List()); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenGroupDoesNotExist_ReturnsGroupNotFound( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser, + Guid missingGroupId) + { + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User, + groups: [missingGroupId]); + + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(new List()); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenCustomTypeAndCustomPermissionsDisabled_ReturnsCustomPermissionsNotEnabled( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + var organization = CreateOrganization(orgUser.OrganizationId, PlanType.EnterpriseAnnually, useCustomPermissions: false); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Custom, organization: organization); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenRemovingLastConfirmedOwner_ReturnsMustHaveConfirmedOwner( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser orgUser) + { + // Demoting the org's last confirmed owner (Owner -> User). + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User); + + sutProvider.GetDependency() + .HasConfirmedOwnersExceptAsync(orgUser.OrganizationId, Arg.Any>()) + .Returns(false); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenManageCombinedWithReadOnly_ReturnsManageMutuallyExclusive( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser, + Guid collectionId) + { + var ability = CreateAbility(orgUser.OrganizationId, allowAdminAccessToAllCollectionItems: true); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User, + ability: ability, + collections: [new CollectionAccessSelection { Id = collectionId, Manage = true, ReadOnly = true }]); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List + { + new() { Id = collectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.SharedCollection } + }); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_FiltersDefaultUserCollectionsFromValidResult( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser, + Guid sharedCollectionId, + Guid defaultCollectionId) + { + var ability = CreateAbility(orgUser.OrganizationId, allowAdminAccessToAllCollectionItems: true); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User, + ability: ability, + collections: + [ + new CollectionAccessSelection { Id = sharedCollectionId }, + new CollectionAccessSelection { Id = defaultCollectionId } + ]); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List + { + new() { Id = sharedCollectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.SharedCollection }, + new() { Id = defaultCollectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.DefaultUserCollection } + }); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsValid); + Assert.Single(result.Request.CollectionsToSave); + Assert.Equal(sharedCollectionId, result.Request.CollectionsToSave.Single().Id); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenNonOwnerPromotesUserToOwner_ReturnsOnlyOwnersCanManageOwners( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + var performedBy = new StandardUser(Guid.NewGuid(), isOrganizationOwner: false); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Owner, + performedBy: performedBy, + performedByOrganizationUser: ActingOrganizationUser(OrganizationUserType.Admin)); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenNonOwnerModifiesExistingOwner_ReturnsOnlyOwnersCanManageOwners( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Owner)] OrganizationUser orgUser) + { + var performedBy = new StandardUser(Guid.NewGuid(), isOrganizationOwner: false); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Admin, + performedBy: performedBy, + performedByOrganizationUser: ActingOrganizationUser(OrganizationUserType.Admin)); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenCustomUserPromotesUserToAdmin_ReturnsCustomUsersCannotManageAdminsOrOwners( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + var performedBy = new StandardUser(Guid.NewGuid(), isOrganizationOwner: false); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Admin, + performedBy: performedBy, + performedByOrganizationUser: ActingOrganizationUser(OrganizationUserType.Custom)); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenCustomUserModifiesExistingAdmin_ReturnsCustomUsersCannotManageAdminsOrOwners( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.Admin)] OrganizationUser orgUser) + { + var performedBy = new StandardUser(Guid.NewGuid(), isOrganizationOwner: false); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User, + performedBy: performedBy, + performedByOrganizationUser: ActingOrganizationUser(OrganizationUserType.Custom)); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsError); + Assert.IsType(result.AsError); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenOwnerPromotesUserToOwner_ReturnsValid( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + var performedBy = new StandardUser(Guid.NewGuid(), isOrganizationOwner: true); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Owner, + performedBy: performedBy, + performedByOrganizationUser: ActingOrganizationUser(OrganizationUserType.Owner)); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsValid); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenAdminPromotesUserToAdmin_ReturnsValid( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + var performedBy = new StandardUser(Guid.NewGuid(), isOrganizationOwner: false); + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Admin, + performedBy: performedBy, + performedByOrganizationUser: ActingOrganizationUser(OrganizationUserType.Admin)); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsValid); + } + + [Theory] + [BitAutoData] + public async Task ValidateAsync_WhenSystemUserPromotesToOwner_SkipsEscalationCheck( + SutProvider sutProvider, + [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser) + { + var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Owner, + performedBy: new SystemUser(EventSystemUser.SCIM), + performedByOrganizationUser: null); + + var result = await sutProvider.Sut.ValidateAsync(request); + + Assert.True(result.IsValid); + } + + private static UpdateOrganizationUserValidationRequest CreateRequest( + SutProvider sutProvider, + OrganizationUser organizationUser, + OrganizationUserType newType, + IActingUser performedBy = null, + OrganizationUser performedByOrganizationUser = null, + Organization organization = null, + OrganizationAbility ability = null, + List collections = null, + IEnumerable groups = null, + HashSet currentAccessIds = null) + { + // Use the real role-validation service so the escalation check exercises its actual rules. The + // dependency is set under the constructor parameter name because BitAutoData has already stored an + // auto-mock under that name, which the SutProvider resolves in preference to a type-only override. + sutProvider.SetDependency( + new OrganizationUserValidationService(), "organizationUserValidationService"); + sutProvider.Create(); + + // Default to a state where validation passes unless a test overrides it. + sutProvider.GetDependency() + .HasConfirmedOwnersExceptAsync(organizationUser.OrganizationId, Arg.Any>()) + .Returns(true); + + return new UpdateOrganizationUserValidationRequest( + organizationUser, + newType, + performedBy ?? new StandardUser(Guid.NewGuid(), true), + performedByOrganizationUser, + collections ?? [], + groups, + currentAccessIds ?? [], + organization ?? CreateOrganization(organizationUser.OrganizationId, PlanType.EnterpriseAnnually), + ability ?? CreateAbility(organizationUser.OrganizationId, allowAdminAccessToAllCollectionItems: true)); + } + + // The acting user's own membership. Custom users are given ManageUsers by default, since that is the + // authority a Custom user needs to act on other members. + private static OrganizationUser ActingOrganizationUser(OrganizationUserType type, bool manageUsers = true) + { + var actingUser = new OrganizationUser { Type = type }; + if (type == OrganizationUserType.Custom) + { + actingUser.SetPermissions(new Permissions { ManageUsers = manageUsers }); + } + + return actingUser; + } + + private static Organization CreateOrganization(Guid id, PlanType planType, bool useCustomPermissions = true) => + new() { Id = id, PlanType = planType, UseCustomPermissions = useCustomPermissions }; + + private static OrganizationAbility CreateAbility(Guid id, bool allowAdminAccessToAllCollectionItems) => + new() { Id = id, AllowAdminAccessToAllCollectionItems = allowAdminAccessToAllCollectionItems }; +} From 02f2c5a375d71d22a7f2017fbc3f50726f81d7f5 Mon Sep 17 00:00:00 2001 From: Jared McCannon Date: Thu, 2 Jul 2026 14:35:21 -0500 Subject: [PATCH 2/4] Fix Put collection-auth mocks to match bulk ModifyUserAccess check --- .../OrganizationUserControllerPutTests.cs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index f0ab610cab4a..1b7f7820d945 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -35,7 +35,12 @@ public async Task Put_Success(OrganizationUserUpdateRequestModel model, // Arrange Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); - // Authorize all changes for basic happy path test + // Authorize all changes for basic happy path test. The controller authorizes the posted collections + // as a single bulk set, then re-checks each current collection individually for the readonly merge. + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Any>(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Success()); sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Any(), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) @@ -198,7 +203,13 @@ public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections( var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; - // Authorize the editedCollection + // Authorize the posted collections as a set (only the edited collection is posted) + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is>(colls => colls.All(c => c.Id == editedCollectionId)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Success()); + + // Authorize the editedCollection individually (readonly-merge check) sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) @@ -245,7 +256,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection // But the saving user does not have permission to update them sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + .AuthorizeAsync(Arg.Any(), Arg.Is>(colls => colls.All(c => postedCollectionIds.Contains(c.Id))), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); @@ -264,7 +275,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(O var postedCollectionIds = model.Collections.Select(c => c.Id).ToHashSet(); // But the saving user does not have permission to assign access to the collections sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + .AuthorizeAsync(Arg.Any(), Arg.Is>(colls => colls.All(c => postedCollectionIds.Contains(c.Id))), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); From 0b3cd14c5889aff793327bf5377d3a5ca35b9ad1 Mon Sep 17 00:00:00 2001 From: Jared McCannon Date: Thu, 2 Jul 2026 15:08:44 -0500 Subject: [PATCH 3/4] chore: Remove v1 command references from v2 UpdateOrganizationUser comments --- .../OrganizationUsers/UpdateUser/v2/Errors.cs | 2 +- .../UpdateUser/v2/UpdateOrganizationUserValidator.cs | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs index bb49d75cef85..9141ba9459e6 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs @@ -2,7 +2,7 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; -// Use generic "Resource not found." messages to avoid enumeration, matching the v1 NotFoundException behavior. +// Use generic "Resource not found." messages to avoid enumeration. public record CollectionNotFound() : NotFoundError("Resource not found."); public record GroupNotFound() : NotFoundError("Resource not found."); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs index 37dc169b9e40..19a6238d0956 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs @@ -72,8 +72,7 @@ public async Task> Val } } - // A caller cannot grant a role higher than their own. Mirrors v1's - // OrganizationService.ValidateOrganizationUserUpdatePermissions escalation guard. + // A caller cannot grant a role higher than their own. var escalationError = ValidateNotEscalatingAboveOwnRole(request); if (escalationError is not null) { @@ -141,9 +140,9 @@ private async Task IsValidFreeOrganizationAdminAsync(OrganizationUser orga /// Prevents a caller from granting a role higher than their own (privilege escalation). Delegates the /// authority decision to the shared rules, /// checking both the member's current role and the requested role so an existing higher-ranked member - /// cannot be modified from below either. System users act with full authority (v1 gated this on a real - /// acting user), so the check is skipped for them. The v1-specific error messages are preserved by - /// mapping the denial back to which role was out of reach. + /// cannot be modified from below either. System users act with full authority, so the check is skipped + /// for them. The specific error messages are preserved by mapping the denial back to which role was out + /// of reach. /// private Error? ValidateNotEscalatingAboveOwnRole(UpdateOrganizationUserValidationRequest request) { @@ -165,7 +164,7 @@ private async Task IsValidFreeOrganizationAdminAsync(OrganizationUser orga return null; } - // Map the denial to v1's specific message. An Owner (current or requested) can only be managed by an + // Map the denial to its specific message. An Owner (current or requested) can only be managed by an // Owner; everything else that trips the check is a Custom user reaching above their authority. if (request.OrganizationUser.Type == OrganizationUserType.Owner || request.NewType == OrganizationUserType.Owner) { From 41676f09443ea7ccfae7efd3bc8da9b846f37f1f Mon Sep 17 00:00:00 2001 From: Jared McCannon Date: Thu, 2 Jul 2026 19:51:58 -0500 Subject: [PATCH 4/4] refactor: Fetch collections once and reject default collections in v2 UpdateOrganizationUser Pass the API-loaded collections into the v2 validation request so the validator validates existence without re-querying. The validator now rejects default user collections with a new CannotAssignDefaultCollection error instead of silently filtering them, and the controller excludes current-access defaults from the preserved read-only set. --- .../OrganizationUsersController.cs | 32 +++++++++--- .../OrganizationUsers/UpdateUser/v2/Errors.cs | 1 + .../v2/UpdateOrganizationUserCommand.cs | 4 +- .../v2/UpdateOrganizationUserRequest.cs | 9 +++- .../v2/UpdateOrganizationUserValidator.cs | 25 +++++----- .../OrganizationUsersControllerTests.cs | 50 +++++++++++++++++++ .../v2/UpdateOrganizationUserCommandTests.cs | 1 + .../UpdateOrganizationUserValidatorTests.cs | 49 +++++++++--------- 8 files changed, 122 insertions(+), 49 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 1c7591c32648..69d60803a654 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -427,8 +427,7 @@ public async Task Put([BindOrganization] Organization organization, Gui if (_featureService.IsEnabled(FeatureFlagKeys.ChangeMemberEmailNoMp)) { - // The self-add-to-collection check is handled by the v2 validator. - var collectionsToSave = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess); + var (collectionsToSave, postedCollections) = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess); // The acting user's own membership drives the role-escalation check. Null when the caller is not // an organization member (e.g. a provider), whose authority comes from the owner/provider flag. @@ -451,7 +450,8 @@ public async Task Put([BindOrganization] Organization organization, Gui savingOrganizationUser, collectionsToSave, groupsToSave, - currentAccessIds); + currentAccessIds, + postedCollections); var result = await _updateOrganizationUserCommandVNext.UpdateUserAsync(request); return Handle(result); @@ -467,7 +467,7 @@ public async Task Put([BindOrganization] Organization organization, Gui throw new BadRequestException("You cannot add yourself to a collection."); } - var collectionsToSaveV1 = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess); + var (collectionsToSaveV1, _) = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess); await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), existingUserType, userId, collectionsToSaveV1, groupsToSave); @@ -480,7 +480,7 @@ await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(or /// cannot are validated and the user's current /// access to them is preserved so it is not accidentally overwritten. /// - private async Task> GetAuthorizedCollectionsToSaveAsync(OrganizationUserUpdateRequestModel model, ICollection currentAccess) + private async Task<(List CollectionsToSave, ICollection Collections)> GetAuthorizedCollectionsToSaveAsync(OrganizationUserUpdateRequestModel model, ICollection currentAccess) { // Authorization check: // You must have authorization to ModifyUserAccess for all collections being saved. The bulk handler @@ -507,11 +507,29 @@ private async Task> GetAuthorizedCollectionsToSa } } + // 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(); + var editedCollectionAccess = model.Collections.Select(c => c.ToSelectionReadOnly()); - var readonlyCollectionAccess = currentAccess.Where(ca => readonlyCollectionIds.Contains(ca.Id)); - return editedCollectionAccess + var readonlyCollectionAccess = currentAccess + .Where(ca => readonlyCollectionIds.Contains(ca.Id) && !defaultCollectionIds.Contains(ca.Id)); + var collectionsToSave = editedCollectionAccess .Concat(readonlyCollectionAccess) .ToList(); + + // The hydrated collections referenced by collectionsToSave (posted plus preserved read-only) so the + // validator can check existence and reject defaults without re-querying. + var collections = postedCollections + .Concat(currentCollections) + .DistinctBy(c => c.Id) + .ToList(); + + return (collectionsToSave, collections); } [HttpPost("{id}")] diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs index 9141ba9459e6..aec6d084e620 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/Errors.cs @@ -14,3 +14,4 @@ public record OnlyOwnersCanManageOwners() : BadRequestError("Only an Owner can m public record CustomUsersCannotManageAdminsOrOwners() : BadRequestError("Custom users can not manage Admins or Owners."); public record ManageMutuallyExclusive() : BadRequestError("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); public record CustomPermissionsNotEnabled() : BadRequestError("To enable custom permissions the organization must be on an Enterprise plan."); +public record CannotAssignDefaultCollection() : BadRequestError("Default collections cannot be assigned to a member."); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs index 7dcea8ce235c..9d0edcd08c30 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs @@ -34,14 +34,14 @@ public async Task UpdateUserAsync(UpdateOrganizationUserRequest r request.GroupsToSave, request.CurrentAccessIds, organization, - organizationAbility)); + organizationAbility, + request.PostedCollections)); if (validationResult.IsError) { return validationResult.AsError; } - // The validator returns the collection access with default user collections filtered out. var collectionsToSave = validationResult.Request.CollectionsToSave; var groupsToSave = validationResult.Request.GroupsToSave; diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs index d975775d6c99..6cf891996499 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserRequest.cs @@ -27,6 +27,9 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUse /// The user's updated collection access. Null removes all collection access. /// The user's updated group access. Null means groups are not updated. /// The collection ids the user currently has access to (used for the self-add check). +/// The hydrated collections referenced by , already +/// loaded and authorized by the API layer, so the validator can check existence and reject default collections without +/// re-querying. public record UpdateOrganizationUserRequest( OrganizationUser OrganizationUser, Organization Organization, @@ -38,7 +41,8 @@ public record UpdateOrganizationUserRequest( OrganizationUser? PerformedByOrganizationUser, List? CollectionsToSave, IEnumerable? GroupsToSave, - HashSet CurrentAccessIds); + HashSet CurrentAccessIds, + ICollection PostedCollections); /// /// The input to the validator: the in its current (unmodified) database state, @@ -55,4 +59,5 @@ public record UpdateOrganizationUserValidationRequest( IEnumerable? GroupsToSave, HashSet CurrentAccessIds, Organization Organization, - OrganizationAbility OrganizationAbility); + OrganizationAbility OrganizationAbility, + ICollection PostedCollections); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs index 19a6238d0956..814698f5c496 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs @@ -17,7 +17,6 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUse public class UpdateOrganizationUserValidator( IOrganizationUserRepository organizationUserRepository, - ICollectionRepository collectionRepository, IGroupRepository groupRepository, IHasConfirmedOwnersExceptQuery hasConfirmedOwnersExceptQuery, IOrganizationUserValidationService organizationUserValidationService) @@ -28,7 +27,6 @@ public async Task> Val { var organizationUser = request.OrganizationUser; - // The user must already be invited to the organization. if (organizationUser.Id == Guid.Empty) { return Invalid(request, new InviteUserFirst()); @@ -47,18 +45,22 @@ public async Task> Val return Invalid(request, new CannotAddSelfToCollection()); } - // All posted collections must exist and belong to the organization. Default user collections are - // filtered out of the set that is ultimately persisted. + // All posted collections must exist and belong to the organization. The API layer has already loaded + // and authorized these collections, so they are validated against the request rather than re-queried. var collectionsToSave = request.CollectionsToSave; if (collectionsToSave.Count != 0) { - var collections = await collectionRepository.GetManyByManyIdsAsync(collectionsToSave.Select(c => c.Id)); - if (!CollectionsAreValid(collectionsToSave, collections, organizationUser.OrganizationId)) + if (!CollectionsAreValid(collectionsToSave, request.PostedCollections, organizationUser.OrganizationId)) { return Invalid(request, new CollectionNotFound()); } - collectionsToSave = ExcludeDefaultUserCollections(collectionsToSave, collections); + // Default user collections ("My Items") cannot be assigned through member management; their presence + // here is invalid input (they are excluded from the current-access set upstream). + if (ContainsDefaultUserCollection(collectionsToSave, request.PostedCollections)) + { + return Invalid(request, new CannotAssignDefaultCollection()); + } } // All posted groups must exist and belong to the organization. @@ -80,7 +82,7 @@ public async Task> Val } // Custom permissions require an Enterprise plan. - if (request.NewType == OrganizationUserType.Custom && !request.Organization.UseCustomPermissions) + if (request is { NewType: OrganizationUserType.Custom, Organization.UseCustomPermissions: false }) { return Invalid(request, new CustomPermissionsNotEnabled()); } @@ -99,7 +101,7 @@ public async Task> Val return Invalid(request, new ManageMutuallyExclusive()); } - return Valid(request with { CollectionsToSave = collectionsToSave }); + return Valid(request); } private async Task IsValidFreeOrganizationAdminAsync(OrganizationUser organizationUser, OrganizationUserType newType, Organization organization) @@ -195,11 +197,10 @@ private static bool CollectionsAreValid(List collecti return missingCollection == null && collections.All(c => c.OrganizationId == organizationId); } - private static List ExcludeDefaultUserCollections( + private static bool ContainsDefaultUserCollection( List collectionAccess, ICollection collections) => collectionAccess - .Where(cas => collections.Any(c => c.Id == cas.Id && c.Type != CollectionType.DefaultUserCollection)) - .ToList(); + .Any(cas => collections.Any(c => c.Id == cas.Id && c.Type == CollectionType.DefaultUserCollection)); private static bool GroupsAreValid(ICollection groupAccess, ICollection groups, Guid organizationId) { diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index 34dfec390a78..65565c60a2b1 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -822,6 +822,56 @@ await sutProvider.GetDependency() .UpdateUserAsync(default); } + [Theory] + [BitAutoData] + public async Task Put_WhenFeatureFlagEnabled_ExcludesDefaultCollectionsFromPreservedAccess( + Organization organization, OrganizationUserUpdateRequestModel model, Guid userId, OrganizationAbility organizationAbility, + OrganizationUser organizationUser, Guid sharedCollectionId, Guid defaultCollectionId, + SutProvider sutProvider) + { + PutSetup(sutProvider, organization, organizationUser, organizationAbility, userId, featureEnabled: true); + + // The client posts no collections; the user currently has access to a shared and a default collection. + model.Collections = []; + var currentAccess = new List + { + new() { Id = sharedCollectionId }, + new() { Id = defaultCollectionId } + }; + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, currentAccess)); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List + { + new() { Id = sharedCollectionId, OrganizationId = organization.Id, Type = CollectionType.SharedCollection }, + new() { Id = defaultCollectionId, OrganizationId = organization.Id, Type = CollectionType.DefaultUserCollection } + }); + + // The saving user cannot ModifyUserAccess on the current collections, so they would be preserved as read-only. + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Any(), + Arg.Any>()) + .Returns(AuthorizationResult.Failed()); + + V2_UpdateUserCommand.UpdateOrganizationUserRequest captured = null; + sutProvider.GetDependency() + .UpdateUserAsync(Arg.Do(r => captured = r)) + .Returns(new CommandResult(new None())); + + var result = await sutProvider.Sut.Put(organization, organizationUser.Id, model); + + Assert.IsType(result); + Assert.NotNull(captured); + // The default collection is dropped from the preserved set; the shared one is kept. + Assert.Contains(captured.CollectionsToSave, c => c.Id == sharedCollectionId); + Assert.DoesNotContain(captured.CollectionsToSave, c => c.Id == defaultCollectionId); + // Both hydrated collections are still passed through so the validator can reject any posted default. + Assert.Contains(captured.PostedCollections, c => c.Id == defaultCollectionId); + } + private static void PutSetup(SutProvider sutProvider, Organization organization, OrganizationUser organizationUser, OrganizationAbility organizationAbility, Guid userId, bool featureEnabled) { diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs index 7b851927a694..fa132e0c5285 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommandTests.cs @@ -186,6 +186,7 @@ private static UpdateOrganizationUserRequest Setup( new OrganizationUser { Type = OrganizationUserType.Owner }, collections, groups, + [], []); } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs index 6cb88aaa6bdd..1afc6a565877 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs @@ -102,11 +102,8 @@ public async Task ValidateAsync_WhenCollectionDoesNotExist_ReturnsCollectionNotF Guid missingCollectionId) { var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.User, - collections: [new CollectionAccessSelection { Id = missingCollectionId }]); - - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List()); + collections: [new CollectionAccessSelection { Id = missingCollectionId }], + postedCollections: []); var result = await sutProvider.Sut.ValidateAsync(request); @@ -180,13 +177,6 @@ public async Task ValidateAsync_WhenManageCombinedWithReadOnly_ReturnsManageMutu ability: ability, collections: [new CollectionAccessSelection { Id = collectionId, Manage = true, ReadOnly = true }]); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List - { - new() { Id = collectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.SharedCollection } - }); - var result = await sutProvider.Sut.ValidateAsync(request); Assert.True(result.IsError); @@ -195,7 +185,7 @@ public async Task ValidateAsync_WhenManageCombinedWithReadOnly_ReturnsManageMutu [Theory] [BitAutoData] - public async Task ValidateAsync_FiltersDefaultUserCollectionsFromValidResult( + public async Task ValidateAsync_WhenAssigningDefaultUserCollection_ReturnsCannotAssignDefaultCollection( SutProvider sutProvider, [OrganizationUser(OrganizationUserStatusType.Confirmed, OrganizationUserType.User)] OrganizationUser orgUser, Guid sharedCollectionId, @@ -208,21 +198,17 @@ public async Task ValidateAsync_FiltersDefaultUserCollectionsFromValidResult( [ new CollectionAccessSelection { Id = sharedCollectionId }, new CollectionAccessSelection { Id = defaultCollectionId } + ], + postedCollections: + [ + new Collection { Id = sharedCollectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.SharedCollection }, + new Collection { Id = defaultCollectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.DefaultUserCollection } ]); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List - { - new() { Id = sharedCollectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.SharedCollection }, - new() { Id = defaultCollectionId, OrganizationId = orgUser.OrganizationId, Type = CollectionType.DefaultUserCollection } - }); - var result = await sutProvider.Sut.ValidateAsync(request); - Assert.True(result.IsValid); - Assert.Single(result.Request.CollectionsToSave); - Assert.Equal(sharedCollectionId, result.Request.CollectionsToSave.Single().Id); + Assert.True(result.IsError); + Assert.IsType(result.AsError); } [Theory] @@ -350,7 +336,8 @@ private static UpdateOrganizationUserValidationRequest CreateRequest( OrganizationAbility ability = null, List collections = null, IEnumerable groups = null, - HashSet currentAccessIds = null) + HashSet currentAccessIds = null, + ICollection postedCollections = null) { // Use the real role-validation service so the escalation check exercises its actual rules. The // dependency is set under the constructor parameter name because BitAutoData has already stored an @@ -373,7 +360,17 @@ private static UpdateOrganizationUserValidationRequest CreateRequest( groups, currentAccessIds ?? [], organization ?? CreateOrganization(organizationUser.OrganizationId, PlanType.EnterpriseAnnually), - ability ?? CreateAbility(organizationUser.OrganizationId, allowAdminAccessToAllCollectionItems: true)); + ability ?? CreateAbility(organizationUser.OrganizationId, allowAdminAccessToAllCollectionItems: true), + // By default, treat every posted collection as an existing shared collection in the org so + // validation passes; tests override this to exercise missing or default collections. + postedCollections ?? (collections ?? []) + .Select(c => new Collection + { + Id = c.Id, + OrganizationId = organizationUser.OrganizationId, + Type = CollectionType.SharedCollection + }) + .ToList()); } // The acting user's own membership. Custom users are given ManageUsers by default, since that is the