From 65b3029ece3cb45bf00e891f9184513f035ba332 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 4 Aug 2026 15:16:07 +0100 Subject: [PATCH 1/2] [PM-41448] feat: add unified collection PATCH endpoint with delta access for users and groups --- .../Controllers/BaseAdminConsoleController.cs | 33 +---- .../Endpoints/AdminConsoleEndpoints.cs | 29 ++++ ...AdminConsoleServiceCollectionExtensions.cs | 15 ++ .../Endpoints/CollectionEndpoints.cs | 25 ++++ ...inConsoleExceptionHandlerEndpointFilter.cs | 35 +++++ .../Handlers/UpdateCollectionHandler.cs | 136 ++++++++++++++++++ .../CollectionGroupAccessDeltaRequestModel.cs | 13 ++ .../CollectionUserAccessDeltaRequestModel.cs | 13 ++ .../UpdateCollectionWithDeltaRequestModel.cs | 22 +++ .../Utilities/CommandResultExtensions.cs | 52 +++++++ src/Api/Startup.cs | 8 +- src/Core/Constants.cs | 1 + 12 files changed, 351 insertions(+), 31 deletions(-) create mode 100644 src/Api/AdminConsole/Endpoints/AdminConsoleEndpoints.cs create mode 100644 src/Api/AdminConsole/Endpoints/AdminConsoleServiceCollectionExtensions.cs create mode 100644 src/Api/AdminConsole/Endpoints/CollectionEndpoints.cs create mode 100644 src/Api/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilter.cs create mode 100644 src/Api/AdminConsole/Endpoints/Handlers/UpdateCollectionHandler.cs create mode 100644 src/Api/AdminConsole/Models/Request/CollectionGroupAccessDeltaRequestModel.cs create mode 100644 src/Api/AdminConsole/Models/Request/CollectionUserAccessDeltaRequestModel.cs create mode 100644 src/Api/AdminConsole/Models/Request/UpdateCollectionWithDeltaRequestModel.cs create mode 100644 src/Api/AdminConsole/Utilities/CommandResultExtensions.cs diff --git a/src/Api/AdminConsole/Controllers/BaseAdminConsoleController.cs b/src/Api/AdminConsole/Controllers/BaseAdminConsoleController.cs index efe1f2645545..048ee1032793 100644 --- a/src/Api/AdminConsole/Controllers/BaseAdminConsoleController.cs +++ b/src/Api/AdminConsole/Controllers/BaseAdminConsoleController.cs @@ -1,10 +1,8 @@ -using Bit.Core.AdminConsole.Utilities.v2; +using Bit.Api.AdminConsole.Utilities; using Bit.Core.AdminConsole.Utilities.v2.Results; -using Bit.Core.AdminConsole.Utilities.v2.Validation; using Bit.Core.Models.Api; using Microsoft.AspNetCore.Http.HttpResults; using Microsoft.AspNetCore.Mvc; -using CommandError = Bit.Core.AdminConsole.Utilities.v2.Error; namespace Bit.Api.AdminConsole.Controllers; @@ -14,11 +12,7 @@ public abstract class BaseAdminConsoleController : Controller /// Maps a void to an HTTP response. /// Returns 204 No Content on success, or the appropriate error status code on failure. /// - protected static IResult Handle(CommandResult commandResult) => - commandResult.Match( - error => MapError(error), - _ => TypedResults.NoContent() - ); + protected static IResult Handle(CommandResult commandResult) => commandResult.ToHttpResult(); /// /// Maps a to an HTTP response. @@ -27,10 +21,7 @@ protected static IResult Handle(CommandResult commandResult) => /// On failure, returns the appropriate error status code. /// protected static IResult Handle(CommandResult commandResult, Func success) => - commandResult.Match( - error => MapError(error), - success - ); + commandResult.ToHttpResult(success); protected static class Error { @@ -49,22 +40,4 @@ public static JsonHttpResult InternalError( new ErrorResponseModel(message), statusCode: StatusCodes.Status500InternalServerError); } - - private static IResult MapError(CommandError error) => - error switch - { - IValidationError validationError => TypedResults.BitwardenValidationProblem(validationError), - BadRequestError badRequest => TypedResults.BadRequest(new ErrorResponseModel(badRequest.Message)), - NotFoundError notFound => TypedResults.NotFound(new ErrorResponseModel(notFound.Message)), - ConflictError conflict => TypedResults.Json( - new ErrorResponseModel(conflict.Message), - statusCode: StatusCodes.Status409Conflict), - InternalError internalError => TypedResults.Json( - new ErrorResponseModel(internalError.Message), - statusCode: StatusCodes.Status500InternalServerError), - _ => TypedResults.Json( - new ErrorResponseModel(error.Message), - statusCode: StatusCodes.Status500InternalServerError - ) - }; } diff --git a/src/Api/AdminConsole/Endpoints/AdminConsoleEndpoints.cs b/src/Api/AdminConsole/Endpoints/AdminConsoleEndpoints.cs new file mode 100644 index 000000000000..a07a1f3daa7d --- /dev/null +++ b/src/Api/AdminConsole/Endpoints/AdminConsoleEndpoints.cs @@ -0,0 +1,29 @@ +using Bit.Api.AdminConsole.Endpoints.Filters; +using Bit.Core.Auth.Identity; + +namespace Bit.Api.AdminConsole.Endpoints; + +/// +/// Maps the admin console Minimal API endpoint groups. Add new groups here rather than in Startup.cs. +/// +public static class AdminConsoleEndpoints +{ + public static void MapAdminConsoleEndpoints(this IEndpointRouteBuilder endpoints) + { + endpoints.MapCollectionEndpoints(); + } + + /// + /// Applies the shared admin console endpoint chain to a single Minimal API endpoint. Keeps + /// outermost so exceptions from downstream + /// filters and the handler are translated into the ErrorResponseModel contract, and hides + /// the endpoint from the public OpenAPI spec via the "internal" group name. + /// + public static RouteHandlerBuilder WithAdminConsoleDefaults(this RouteHandlerBuilder builder) + { + builder.RequireAuthorization(Policies.Application); + builder.AddEndpointFilter(); + builder.WithGroupName("internal"); + return builder; + } +} diff --git a/src/Api/AdminConsole/Endpoints/AdminConsoleServiceCollectionExtensions.cs b/src/Api/AdminConsole/Endpoints/AdminConsoleServiceCollectionExtensions.cs new file mode 100644 index 000000000000..64d613f5f96e --- /dev/null +++ b/src/Api/AdminConsole/Endpoints/AdminConsoleServiceCollectionExtensions.cs @@ -0,0 +1,15 @@ +using Bit.Api.AdminConsole.Endpoints.Handlers; +using Microsoft.Extensions.DependencyInjection.Extensions; + +namespace Bit.Api.AdminConsole.Endpoints; + +/// +/// Registers the handler classes behind admin console Minimal API endpoints. +/// +public static class AdminConsoleServiceCollectionExtensions +{ + public static void AddAdminConsoleEndpointHandlers(this IServiceCollection services) + { + services.TryAddScoped(); + } +} diff --git a/src/Api/AdminConsole/Endpoints/CollectionEndpoints.cs b/src/Api/AdminConsole/Endpoints/CollectionEndpoints.cs new file mode 100644 index 000000000000..bbd47a9c9b2a --- /dev/null +++ b/src/Api/AdminConsole/Endpoints/CollectionEndpoints.cs @@ -0,0 +1,25 @@ +using System.Security.Claims; +using Bit.Api.AdminConsole.Endpoints.Handlers; +using Bit.Api.AdminConsole.Models.Request; +using Bit.Core; + +namespace Bit.Api.AdminConsole.Endpoints; + +/// +/// Maps the unified collection routes handled via Minimal API. When +/// is off the flagged +/// endpoints return 404 and the existing MVC controller answers the request instead. +/// +public static class CollectionEndpoints +{ + public static void MapCollectionEndpoints(this IEndpointRouteBuilder endpoints) + { + endpoints.MapPatch("organizations/{orgId:guid}/collections/{id:guid}", + (Guid orgId, Guid id, UpdateCollectionWithDeltaRequestModel model, ClaimsPrincipal user, + UpdateCollectionHandler handler) => + handler.HandleAsync(orgId, id, model, user)) + .RequireFeature(FeatureFlagKeys.PM35160CollectionAuthorizationHandlers) + .WithAdminConsoleDefaults() + .WithName("PatchCollectionWithDelta"); + } +} diff --git a/src/Api/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilter.cs b/src/Api/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilter.cs new file mode 100644 index 000000000000..3617c5ee0e63 --- /dev/null +++ b/src/Api/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilter.cs @@ -0,0 +1,35 @@ +using Bit.Core.Exceptions; +using Bit.Core.Models.Api; + +namespace Bit.Api.AdminConsole.Endpoints.Filters; + +/// +/// Turns exceptions thrown by admin console Minimal API endpoints into the same ErrorResponseModel shape +/// every other Bitwarden endpoint already returns. +/// +public class AdminConsoleExceptionHandlerEndpointFilter : IEndpointFilter +{ + public async ValueTask InvokeAsync(EndpointFilterInvocationContext context, EndpointFilterDelegate next) + { + try + { + return await next(context); + } + catch (NotFoundException) + { + return TypedResults.NotFound(new ErrorResponseModel("Resource not found.")); + } + catch (BadRequestException badRequestException) + { + return TypedResults.BadRequest(new ErrorResponseModel(badRequestException.Message)); + } + catch (Exception exception) + { + var endpointName = context.HttpContext.GetEndpoint()?.DisplayName; + context.HttpContext.RequestServices.GetRequiredService>() + .LogError(exception, "Unhandled exception in {EndpointName}", endpointName); + return TypedResults.Json( + new ErrorResponseModel("An error has occurred."), statusCode: StatusCodes.Status500InternalServerError); + } + } +} diff --git a/src/Api/AdminConsole/Endpoints/Handlers/UpdateCollectionHandler.cs b/src/Api/AdminConsole/Endpoints/Handlers/UpdateCollectionHandler.cs new file mode 100644 index 000000000000..9957979c8e47 --- /dev/null +++ b/src/Api/AdminConsole/Endpoints/Handlers/UpdateCollectionHandler.cs @@ -0,0 +1,136 @@ +using System.Security.Claims; +using System.Transactions; +using Bit.Api.AdminConsole.Authorization.Collections; +using Bit.Api.AdminConsole.Models.Request; +using Bit.Api.AdminConsole.Utilities; +using Bit.Core.AdminConsole.AbilitiesCache; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections.ModifyGroupAccess; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections.ModifyUserAccess; +using Bit.Core.AdminConsole.Utilities.v2.Results; +using Bit.Core.Context; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; +using Bit.Core.Utilities; +using Microsoft.AspNetCore.Authorization; + +namespace Bit.Api.AdminConsole.Endpoints.Handlers; + +/// +/// Handles the unified PUT organizations/{orgId}/collections/{id} endpoint: updates a collection's +/// metadata alongside add/update/remove deltas for its user and group access. Each concern is authorized +/// against its own requirement, and the three writes commit as a single atomic transaction so a partial +/// failure never leaves a collection with metadata applied but access changes rejected (or vice versa). +/// +public class UpdateCollectionHandler( + ICollectionRepository collectionRepository, + IOrganizationUserRepository organizationUserRepository, + IOrganizationAbilityCacheService organizationAbilityCacheService, + IAuthorizationService authorizationService, + ICurrentContext currentContext, + IUpdateCollectionCommand updateCollectionCommand, + IModifyCollectionUserAccessCommand modifyCollectionUserAccessCommand, + IModifyCollectionGroupAccessCommand modifyCollectionGroupAccessCommand) +{ + public async Task HandleAsync( + Guid orgId, + Guid id, + UpdateCollectionWithDeltaRequestModel model, + ClaimsPrincipal user) + { + // Resolve the target collection once with its full access details; the same tuple feeds both + // the update itself and the two access-resource authorization checks that follow. + var (collection, accessDetails) = await collectionRepository.GetByIdWithAccessAsync(id); + if (collection is null || collection.OrganizationId != orgId) + { + throw new NotFoundException(); + } + + await authorizationService.AuthorizeOrThrowAsync(user, collection, BulkCollectionOperations.Update); + + var userResource = new CollectionUserAccessResource(collection, accessDetails); + await authorizationService.AuthorizeOrThrowAsync(user, userResource, CollectionUserOperations.Update); + + var groupResource = new CollectionGroupAccessResource(collection, accessDetails); + await authorizationService.AuthorizeOrThrowAsync(user, groupResource, CollectionGroupOperations.Update); + + var organizationAbility = await organizationAbilityCacheService.GetOrganizationAbilityAsync(orgId); + var allowAdminAccessToAllCollectionItems = + organizationAbility is { AllowAdminAccessToAllCollectionItems: true }; + + var callerOrganizationUser = currentContext.UserId.HasValue + ? await organizationUserRepository.GetByOrganizationAsync(orgId, currentContext.UserId.Value) + : null; + + var userTargets = new[] { new CollectionUserAccessTarget(collection, accessDetails) }; + var groupTargets = new[] { new CollectionGroupAccessTarget(collection, accessDetails) }; + + var userRequest = new ModifyCollectionUserAccessRequest( + userTargets, + model.Users.Add.Select(u => u.ToSelectionReadOnly()).ToList(), + model.Users.Update.Select(u => u.ToSelectionReadOnly()).ToList(), + model.Users.Remove.ToList(), + callerOrganizationUser?.Id, + allowAdminAccessToAllCollectionItems); + + var groupRequest = new ModifyCollectionGroupAccessRequest( + groupTargets, + model.Groups.Add.Select(g => g.ToSelectionReadOnly()).ToList(), + model.Groups.Update.Select(g => g.ToSelectionReadOnly()).ToList(), + model.Groups.Remove.ToList(), + callerOrganizationUser?.Id, + allowAdminAccessToAllCollectionItems); + + // Bail before opening a transaction if either access delta already knows it's invalid — the + // access commands emit typed CommandResult errors instead of throwing, so we surface those + // as the HTTP response and skip persisting the metadata update. + CommandResult? failedResult = null; + + using (var scope = new TransactionScope( + TransactionScopeOption.Required, + new TransactionOptions { IsolationLevel = IsolationLevel.ReadCommitted }, + TransactionScopeAsyncFlowOption.Enabled)) + { + // Apply Name/ExternalId first so subsequent access changes see the current revision. The + // update command itself throws BadRequestException on invalid input, which the endpoint + // filter translates into the shared ErrorResponseModel contract. + ApplyMetadataChanges(collection, model); + await updateCollectionCommand.UpdateAsync(collection); + + var userResult = await modifyCollectionUserAccessCommand.ModifyAsync(userRequest); + if (userResult.IsError) + { + failedResult = userResult; + } + else + { + var groupResult = await modifyCollectionGroupAccessCommand.ModifyAsync(groupRequest); + if (groupResult.IsError) + { + failedResult = groupResult; + } + } + + if (failedResult is null) + { + scope.Complete(); + } + } + + return failedResult is not null + ? failedResult.ToHttpResult() + : TypedResults.NoContent(); + } + + private static void ApplyMetadataChanges(Bit.Core.Entities.Collection collection, UpdateCollectionWithDeltaRequestModel model) + { + // Mirror the existing MVC UpdateCollectionRequestModel behaviour: leave a default user collection's + // name untouched, and always accept an ExternalId (including clearing it). + if (string.IsNullOrEmpty(collection.DefaultUserCollectionEmail) && !string.IsNullOrWhiteSpace(model.Name)) + { + collection.Name = model.Name; + } + + collection.ExternalId = model.ExternalId; + } +} diff --git a/src/Api/AdminConsole/Models/Request/CollectionGroupAccessDeltaRequestModel.cs b/src/Api/AdminConsole/Models/Request/CollectionGroupAccessDeltaRequestModel.cs new file mode 100644 index 000000000000..2166536d0ec7 --- /dev/null +++ b/src/Api/AdminConsole/Models/Request/CollectionGroupAccessDeltaRequestModel.cs @@ -0,0 +1,13 @@ +using Bit.Api.Models.Request; + +namespace Bit.Api.AdminConsole.Models.Request; + +/// +/// Explicit add/update/remove changes to a collection's group access, rather than the full desired list. +/// +public class CollectionGroupAccessDeltaRequestModel +{ + public IEnumerable Add { get; init; } = []; + public IEnumerable Update { get; init; } = []; + public IEnumerable Remove { get; init; } = []; +} diff --git a/src/Api/AdminConsole/Models/Request/CollectionUserAccessDeltaRequestModel.cs b/src/Api/AdminConsole/Models/Request/CollectionUserAccessDeltaRequestModel.cs new file mode 100644 index 000000000000..7dc22a9938e0 --- /dev/null +++ b/src/Api/AdminConsole/Models/Request/CollectionUserAccessDeltaRequestModel.cs @@ -0,0 +1,13 @@ +using Bit.Api.Models.Request; + +namespace Bit.Api.AdminConsole.Models.Request; + +/// +/// Explicit add/update/remove changes to a collection's user access, rather than the full desired list. +/// +public class CollectionUserAccessDeltaRequestModel +{ + public IEnumerable Add { get; init; } = []; + public IEnumerable Update { get; init; } = []; + public IEnumerable Remove { get; init; } = []; +} diff --git a/src/Api/AdminConsole/Models/Request/UpdateCollectionWithDeltaRequestModel.cs b/src/Api/AdminConsole/Models/Request/UpdateCollectionWithDeltaRequestModel.cs new file mode 100644 index 000000000000..c07cadca1f4c --- /dev/null +++ b/src/Api/AdminConsole/Models/Request/UpdateCollectionWithDeltaRequestModel.cs @@ -0,0 +1,22 @@ +using System.ComponentModel.DataAnnotations; +using Bit.Core.Utilities; + +namespace Bit.Api.AdminConsole.Models.Request; + +/// +/// Request body for the unified PUT organizations/{orgId}/collections/{id} endpoint: +/// updates a collection's metadata alongside add/update/remove deltas for its user and group access. +/// +public class UpdateCollectionWithDeltaRequestModel +{ + [EncryptedString] + [EncryptedStringLength(1000)] + public string? Name { get; init; } + + [StringLength(300)] + public string? ExternalId { get; init; } + + public CollectionUserAccessDeltaRequestModel Users { get; init; } = new(); + + public CollectionGroupAccessDeltaRequestModel Groups { get; init; } = new(); +} diff --git a/src/Api/AdminConsole/Utilities/CommandResultExtensions.cs b/src/Api/AdminConsole/Utilities/CommandResultExtensions.cs new file mode 100644 index 000000000000..0585c475a54b --- /dev/null +++ b/src/Api/AdminConsole/Utilities/CommandResultExtensions.cs @@ -0,0 +1,52 @@ +using Bit.Core.AdminConsole.Utilities.v2; +using Bit.Core.AdminConsole.Utilities.v2.Results; +using Bit.Core.AdminConsole.Utilities.v2.Validation; +using Bit.Core.Models.Api; +using Microsoft.AspNetCore.Http.HttpResults; +using CommandError = Bit.Core.AdminConsole.Utilities.v2.Error; + +namespace Bit.Api.AdminConsole.Utilities; + +/// +/// Maps a / to an HTTP response. +/// Shared by MVC controllers (via BaseAdminConsoleController.Handle) and Minimal API endpoint handlers. +/// +public static class CommandResultExtensions +{ + /// + /// Returns 204 No Content on success, or the mapped error status code on failure. + /// + public static IResult ToHttpResult(this CommandResult commandResult) => + commandResult.Match( + error => MapError(error), + _ => TypedResults.NoContent() + ); + + /// + /// Delegates to on success so the caller chooses the response shape, or returns + /// the mapped error status code on failure. + /// + public static IResult ToHttpResult(this CommandResult commandResult, Func success) => + commandResult.Match( + error => MapError(error), + success + ); + + private static IResult MapError(CommandError error) => + error switch + { + IValidationError validationError => TypedResults.BitwardenValidationProblem(validationError), + BadRequestError badRequest => TypedResults.BadRequest(new ErrorResponseModel(badRequest.Message)), + NotFoundError notFound => TypedResults.NotFound(new ErrorResponseModel(notFound.Message)), + ConflictError conflict => TypedResults.Json( + new ErrorResponseModel(conflict.Message), + statusCode: StatusCodes.Status409Conflict), + InternalError internalError => TypedResults.Json( + new ErrorResponseModel(internalError.Message), + statusCode: StatusCodes.Status500InternalServerError), + _ => TypedResults.Json( + new ErrorResponseModel(error.Message), + statusCode: StatusCodes.Status500InternalServerError + ) + }; +} diff --git a/src/Api/Startup.cs b/src/Api/Startup.cs index b755e286ac50..042694a096f1 100644 --- a/src/Api/Startup.cs +++ b/src/Api/Startup.cs @@ -1,4 +1,5 @@ -using Bit.Api.Utilities; +using Bit.Api.AdminConsole.Endpoints; +using Bit.Api.Utilities; using Bit.Core; using Bit.Core.Context; using Bit.Core.Settings; @@ -195,6 +196,9 @@ public void ConfigureServices(IServiceCollection services) // Authorization Handlers services.AddAuthorizationHandlers(); + // Admin Console Minimal API endpoint handlers + services.AddAdminConsoleEndpointHandlers(); + //health check if (!globalSettings.SelfHosted) { @@ -287,6 +291,8 @@ public void Configure( { endpoints.MapDefaultControllerRoute(); + endpoints.MapAdminConsoleEndpoints(); + #if !OSS // PAM is a commercial feature; its Minimal API endpoints are only mapped in non-OSS builds. endpoints.MapPamEndpoints(); diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index 73e1b23f579c..9f2ae935352e 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -144,6 +144,7 @@ public static class FeatureFlagKeys public const string PoliciesInAcceptedState = "pm-34145-policies-in-accepted-state"; public const string ChangeMemberEmailNoMp = "pm-28365-change-member-email-no-mp"; public const string PM34423StagedStatus = "pm-34423-staged-status"; + public const string PM35160CollectionAuthorizationHandlers = "pm-35160-collection-user-collection-group-authorization-handlers"; /* Architecture */ public const string DesktopMigrationMilestone1 = "desktop-ui-migration-milestone-1"; From e1a40222ea6fc1b14589b03ce296b8f991dbc8f8 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 4 Aug 2026 15:16:56 +0100 Subject: [PATCH 2/2] [PM-41448] test: add unit tests for unified collection PATCH endpoint handler and filter --- ...soleExceptionHandlerEndpointFilterTests.cs | 115 +++++ .../Handlers/UpdateCollectionHandlerTests.cs | 476 ++++++++++++++++++ 2 files changed, 591 insertions(+) create mode 100644 test/Api.Test/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilterTests.cs create mode 100644 test/Api.Test/AdminConsole/Endpoints/Handlers/UpdateCollectionHandlerTests.cs diff --git a/test/Api.Test/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilterTests.cs b/test/Api.Test/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilterTests.cs new file mode 100644 index 000000000000..f9681c89edee --- /dev/null +++ b/test/Api.Test/AdminConsole/Endpoints/Filters/AdminConsoleExceptionHandlerEndpointFilterTests.cs @@ -0,0 +1,115 @@ +using Bit.Api.AdminConsole.Endpoints.Filters; +using Bit.Core.Exceptions; +using Bit.Core.Models.Api; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.HttpResults; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Endpoints.Filters; + +public class AdminConsoleExceptionHandlerEndpointFilterTests +{ + private static EndpointFilterInvocationContext BuildContext(HttpContext? httpContext = null) + { + httpContext ??= BuildHttpContextWithLogger(); + var context = Substitute.For(); + context.HttpContext.Returns(httpContext); + return context; + } + + /// + /// The filter's fallback branch pulls an ILogger out of HttpContext.RequestServices; give it a real service + /// provider so that resolution works even when the test doesn't otherwise care about logging. + /// + private static DefaultHttpContext BuildHttpContextWithLogger() + { + var services = new ServiceCollection(); + services.AddLogging(); + return new DefaultHttpContext + { + RequestServices = services.BuildServiceProvider() + }; + } + + [Fact] + public async Task InvokeAsync_NotFoundException_ReturnsNotFoundResultWithErrorResponseModel() + { + var filter = new AdminConsoleExceptionHandlerEndpointFilter(); + var context = BuildContext(); + + EndpointFilterDelegate next = _ => throw new NotFoundException(); + + var result = await filter.InvokeAsync(context, next); + + var notFound = Assert.IsType>(result); + Assert.NotNull(notFound.Value); + Assert.Equal("Resource not found.", notFound.Value!.Message); + } + + [Fact] + public async Task InvokeAsync_BadRequestException_ReturnsBadRequestResultWithExceptionMessage() + { + var filter = new AdminConsoleExceptionHandlerEndpointFilter(); + var context = BuildContext(); + + EndpointFilterDelegate next = _ => throw new BadRequestException("Bad input."); + + var result = await filter.InvokeAsync(context, next); + + var badRequest = Assert.IsType>(result); + Assert.NotNull(badRequest.Value); + Assert.Equal("Bad input.", badRequest.Value!.Message); + } + + [Fact] + public async Task InvokeAsync_UnhandledException_Returns500WithGenericErrorAndLogs() + { + var filter = new AdminConsoleExceptionHandlerEndpointFilter(); + + // Wire a substitute logger so we can assert that unhandled exceptions get logged. + var logger = Substitute.For>(); + var services = new ServiceCollection(); + services.AddSingleton(logger); + services.AddLogging(); + var httpContext = new DefaultHttpContext + { + RequestServices = services.BuildServiceProvider() + }; + var context = BuildContext(httpContext); + + var thrown = new InvalidOperationException("boom"); + EndpointFilterDelegate next = _ => throw thrown; + + var result = await filter.InvokeAsync(context, next); + + var jsonResult = Assert.IsType>(result); + Assert.Equal(StatusCodes.Status500InternalServerError, jsonResult.StatusCode); + Assert.NotNull(jsonResult.Value); + Assert.Equal("An error has occurred.", jsonResult.Value!.Message); + + // The filter must log unhandled exceptions with the underlying exception attached. + logger.Received(1).Log( + LogLevel.Error, + Arg.Any(), + Arg.Any(), + thrown, + Arg.Any>()); + } + + [Fact] + public async Task InvokeAsync_NoException_PassesThroughNextResult() + { + var filter = new AdminConsoleExceptionHandlerEndpointFilter(); + var context = BuildContext(); + var expected = TypedResults.NoContent(); + + EndpointFilterDelegate next = _ => ValueTask.FromResult(expected); + + var result = await filter.InvokeAsync(context, next); + + Assert.Same(expected, result); + } +} diff --git a/test/Api.Test/AdminConsole/Endpoints/Handlers/UpdateCollectionHandlerTests.cs b/test/Api.Test/AdminConsole/Endpoints/Handlers/UpdateCollectionHandlerTests.cs new file mode 100644 index 000000000000..ec93491e3c7a --- /dev/null +++ b/test/Api.Test/AdminConsole/Endpoints/Handlers/UpdateCollectionHandlerTests.cs @@ -0,0 +1,476 @@ +using System.Security.Claims; +using Bit.Api.AdminConsole.Authorization.Collections; +using Bit.Api.AdminConsole.Endpoints.Handlers; +using Bit.Api.AdminConsole.Models.Request; +using Bit.Api.Models.Request; +using Bit.Core.AdminConsole.AbilitiesCache; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections.ModifyGroupAccess; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections.ModifyUserAccess; +using Bit.Core.AdminConsole.Utilities.v2; +using Bit.Core.AdminConsole.Utilities.v2.Results; +using Bit.Core.Context; +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Models.Api; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.Repositories; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http.HttpResults; +using NSubstitute; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Endpoints.Handlers; + +[SutProviderCustomize] +public class UpdateCollectionHandlerTests +{ + private const string EncryptedName = "2.abcdEFGH123|abcdEFGH123abcdEFGH123==|abcdEFGH123abcdEFGH123abcdEFGH123abcdEFGH123abcd="; + + private static Collection MakeCollection(Guid orgId, Guid id, string? defaultUserEmail = null) => + new() + { + Id = id, + OrganizationId = orgId, + Name = "original-name", + ExternalId = "original-external", + DefaultUserCollectionEmail = defaultUserEmail + }; + + private static CollectionAccessDetails MakeAccessDetails() => new() + { + Users = new List(), + Groups = new List() + }; + + private static UpdateCollectionWithDeltaRequestModel MakeModel(string? name = EncryptedName, string? externalId = "new-external") => + new() + { + Name = name, + ExternalId = externalId, + Users = new CollectionUserAccessDeltaRequestModel(), + Groups = new CollectionGroupAccessDeltaRequestModel() + }; + + /// + /// Configures the AuthorizationService so all three of the handler's checks succeed by default, + /// letting individual tests override just the check under test. + /// + private static void AllowAllAuthorizationChecks(SutProvider sutProvider) + { + sutProvider.GetDependency() + .AuthorizeAsync( + Arg.Any(), + Arg.Any(), + Arg.Any>()) + .Returns(AuthorizationResult.Success()); + } + + [Theory, BitAutoData] + public async Task HandleAsync_CollectionNotFound_ThrowsNotFoundException( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(null, MakeAccessDetails())); + + await Assert.ThrowsAsync(() => + sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal())); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .UpdateAsync(default!); + } + + [Theory, BitAutoData] + public async Task HandleAsync_CollectionBelongsToDifferentOrganization_ThrowsNotFoundException( + SutProvider sutProvider, + Guid orgId, + Guid otherOrgId, + Guid collectionId) + { + var collection = MakeCollection(otherOrgId, collectionId); + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, MakeAccessDetails())); + + await Assert.ThrowsAsync(() => + sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal())); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .UpdateAsync(default!); + } + + [Theory, BitAutoData] + public async Task HandleAsync_MetadataUpdateAuthorizationFails_ThrowsNotFoundException( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + + // The BulkCollectionOperations.Update check is evaluated against the collection itself. + sutProvider.GetDependency() + .AuthorizeAsync( + Arg.Any(), + collection, + Arg.Is>(reqs => + reqs.Contains(BulkCollectionOperations.Update))) + .Returns(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => + sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal())); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .UpdateAsync(default!); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ModifyAsync(default!); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ModifyAsync(default!); + } + + [Theory, BitAutoData] + public async Task HandleAsync_UserDeltaAuthorizationFails_ThrowsNotFoundException( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + + AllowAllAuthorizationChecks(sutProvider); + + // Fail the user-access-resource check specifically. + sutProvider.GetDependency() + .AuthorizeAsync( + Arg.Any(), + Arg.Any(), + Arg.Is>(reqs => + reqs.Contains(CollectionUserOperations.Update))) + .Returns(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => + sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal())); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .UpdateAsync(default!); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ModifyAsync(default!); + } + + [Theory, BitAutoData] + public async Task HandleAsync_GroupDeltaAuthorizationFails_ThrowsNotFoundException( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + + AllowAllAuthorizationChecks(sutProvider); + + // Fail the group-access-resource check specifically. + sutProvider.GetDependency() + .AuthorizeAsync( + Arg.Any(), + Arg.Any(), + Arg.Is>(reqs => + reqs.Contains(CollectionGroupOperations.Update))) + .Returns(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => + sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal())); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .UpdateAsync(default!); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ModifyAsync(default!); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ModifyAsync(default!); + } + + [Theory, BitAutoData] + public async Task HandleAsync_UserDeltaCommandReturnsError_ReturnsErrorResultAndSkipsGroupDelta( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + AllowAllAuthorizationChecks(sutProvider); + + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new DuplicateOrganizationUserId())); + + var result = await sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal()); + + // BadRequestError maps to a 400 with an ErrorResponseModel body — see CommandResultExtensions. + var badRequest = Assert.IsType>(result); + Assert.NotNull(badRequest.Value); + Assert.Equal(new DuplicateOrganizationUserId().Message, badRequest.Value!.Message); + + // The group delta must not run once the user delta fails — the transaction is being unwound. + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ModifyAsync(default!); + } + + [Theory, BitAutoData] + public async Task HandleAsync_GroupDeltaCommandReturnsError_ReturnsErrorResult( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + AllowAllAuthorizationChecks(sutProvider); + + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + + var groupError = new TestGroupBadRequestError(); + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(groupError)); + + var result = await sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal()); + + var badRequest = Assert.IsType>(result); + Assert.Equal(groupError.Message, badRequest.Value!.Message); + + // Metadata still ran (it's the first write inside the transaction), but the transaction + // is not committed. We can only observe the write attempt from the mock. + await sutProvider.GetDependency().Received(1) + .UpdateAsync(collection); + } + + [Theory, BitAutoData] + public async Task HandleAsync_AllThreeSucceed_ReturnsNoContent( + SutProvider sutProvider, + Guid orgId, + Guid collectionId, + Guid userId, + Guid callerOrgUserId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + AllowAllAuthorizationChecks(sutProvider); + + sutProvider.GetDependency().UserId.Returns(userId); + sutProvider.GetDependency() + .GetByOrganizationAsync(orgId, userId) + .Returns(new OrganizationUser { Id = callerOrgUserId, OrganizationId = orgId, UserId = userId }); + + sutProvider.GetDependency() + .GetOrganizationAbilityAsync(orgId) + .Returns(new OrganizationAbility { Id = orgId, AllowAdminAccessToAllCollectionItems = true }); + + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + + var result = await sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal()); + + Assert.IsType(result); + await sutProvider.GetDependency().Received(1).UpdateAsync(collection); + + // The caller's OrganizationUser.Id and the org-ability flag must be threaded through to both commands. + await sutProvider.GetDependency().Received(1) + .ModifyAsync(Arg.Is(r => + r.PerformingOrganizationUserId == callerOrgUserId + && r.AllowAdminAccessToAllCollectionItems == true)); + await sutProvider.GetDependency().Received(1) + .ModifyAsync(Arg.Is(r => + r.PerformingOrganizationUserId == callerOrgUserId + && r.AllowAdminAccessToAllCollectionItems == true)); + } + + [Theory, BitAutoData] + public async Task HandleAsync_UserNotSignedIn_PassesNullPerformingOrganizationUserId( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + AllowAllAuthorizationChecks(sutProvider); + + sutProvider.GetDependency().UserId.Returns((Guid?)null); + + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + + var result = await sutProvider.Sut.HandleAsync(orgId, collectionId, MakeModel(), new ClaimsPrincipal()); + + Assert.IsType(result); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .GetByOrganizationAsync(default, default); + await sutProvider.GetDependency().Received(1) + .ModifyAsync(Arg.Is(r => r.PerformingOrganizationUserId == null)); + } + + [Theory, BitAutoData] + public async Task HandleAsync_DefaultUserCollection_DoesNotOverwriteName( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + // A default user collection has a non-null DefaultUserCollectionEmail. Its Name must not + // be replaced by the caller's model — this mirrors the MVC UpdateCollectionRequestModel behaviour. + var collection = MakeCollection(orgId, collectionId, defaultUserEmail: "user@example.com"); + var originalName = collection.Name; + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + AllowAllAuthorizationChecks(sutProvider); + + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + + var result = await sutProvider.Sut.HandleAsync( + orgId, collectionId, MakeModel(name: EncryptedName, externalId: "new-external"), new ClaimsPrincipal()); + + Assert.IsType(result); + await sutProvider.GetDependency().Received(1) + .UpdateAsync(Arg.Is(c => c.Name == originalName && c.ExternalId == "new-external")); + } + + [Theory, BitAutoData] + public async Task HandleAsync_NonDefaultCollection_AppliesNameFromModel( + SutProvider sutProvider, + Guid orgId, + Guid collectionId) + { + var collection = MakeCollection(orgId, collectionId, defaultUserEmail: null); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + AllowAllAuthorizationChecks(sutProvider); + + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + + var result = await sutProvider.Sut.HandleAsync( + orgId, collectionId, MakeModel(name: EncryptedName, externalId: "new-external"), new ClaimsPrincipal()); + + Assert.IsType(result); + await sutProvider.GetDependency().Received(1) + .UpdateAsync(Arg.Is(c => c.Name == EncryptedName && c.ExternalId == "new-external")); + } + + [Theory, BitAutoData] + public async Task HandleAsync_MapsDeltaSelectionsThroughToCommands( + SutProvider sutProvider, + Guid orgId, + Guid collectionId, + Guid addUserId, + Guid updateUserId, + Guid removeUserId, + Guid addGroupId, + Guid removeGroupId) + { + var collection = MakeCollection(orgId, collectionId); + var accessDetails = MakeAccessDetails(); + + sutProvider.GetDependency() + .GetByIdWithAccessAsync(collectionId) + .Returns(new Tuple(collection, accessDetails)); + AllowAllAuthorizationChecks(sutProvider); + + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + sutProvider.GetDependency() + .ModifyAsync(Arg.Any()) + .Returns(new CommandResult(new OneOf.Types.None())); + + var model = new UpdateCollectionWithDeltaRequestModel + { + Name = EncryptedName, + ExternalId = "ext", + Users = new CollectionUserAccessDeltaRequestModel + { + Add = new[] { new SelectionReadOnlyRequestModel { Id = addUserId, Manage = true } }, + Update = new[] { new SelectionReadOnlyRequestModel { Id = updateUserId, ReadOnly = true } }, + Remove = new[] { removeUserId } + }, + Groups = new CollectionGroupAccessDeltaRequestModel + { + Add = new[] { new SelectionReadOnlyRequestModel { Id = addGroupId, Manage = true } }, + Remove = new[] { removeGroupId } + } + }; + + var result = await sutProvider.Sut.HandleAsync(orgId, collectionId, model, new ClaimsPrincipal()); + + Assert.IsType(result); + await sutProvider.GetDependency().Received(1) + .ModifyAsync(Arg.Is(r => + r.Add.Any(s => s.Id == addUserId && s.Manage) + && r.Update.Any(s => s.Id == updateUserId && s.ReadOnly) + && r.Remove.Contains(removeUserId))); + await sutProvider.GetDependency().Received(1) + .ModifyAsync(Arg.Is(r => + r.Add.Any(s => s.Id == addGroupId && s.Manage) + && r.Remove.Contains(removeGroupId))); + } + + /// + /// Local test-only error used to drive the group-command failure path without pulling in a + /// production error record whose message might change. + /// + private record TestGroupBadRequestError() : BadRequestError("Group delta rejected."); +}