From 589af12f8f4d83effc6c8cdccd9257ead4f74838 Mon Sep 17 00:00:00 2001 From: jrmccannon Date: Thu, 13 Mar 2025 11:50:46 -0500 Subject: [PATCH] Moved auth logic for GET /organizations/{orgId}/users/{id} to auth handler. Updated tests as well. --- .../OrganizationUsersController.cs | 15 ++++-- ...nizationUserDetailsAuthorizationHandler.cs | 30 ++++++++++++ .../OrganizationUserDetailsOperations.cs | 10 ++++ ...OrganizationServiceCollectionExtensions.cs | 2 + .../OrganizationUsersControllerTests.cs | 14 ++++-- ...ionUserDetailsAuthorizationHandlerTests.cs | 49 +++++++++++++++++++ 6 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsAuthorizationHandler.cs create mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsOperations.cs create mode 100644 test/Core.Test/AdminConsole/Authorization/OrganizationUserDetailsAuthorizationHandlerTests.cs diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 5a73e57204..deee408c23 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -7,6 +7,7 @@ using Bit.Core; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization.OrganizationUserDetails; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization; using Bit.Core.AdminConsole.Repositories; @@ -107,10 +108,18 @@ public class OrganizationUsersController : Controller } [HttpGet("{id}")] - public async Task Get(Guid id, bool includeGroups = false) + public async Task Get([FromRoute] Guid orgId, Guid id, bool includeGroups = false) { + var authorizationResult = await _authorizationService.AuthorizeAsync(User, new OrganizationScope(orgId), + OrganizationUserDetailsOperations.Read); + + if (authorizationResult.Succeeded is false) + { + throw new NotFoundException(); + } + var (organizationUser, collections) = await _organizationUserRepository.GetDetailsByIdWithCollectionsAsync(id); - if (organizationUser == null || !await _currentContext.ManageUsers(organizationUser.OrganizationId)) + if (organizationUser == null) { throw new NotFoundException(); } @@ -707,7 +716,7 @@ public class OrganizationUsersController : Controller { if (!_featureService.IsEnabled(FeatureFlagKeys.AccountDeprovisioning)) { - return userIds.ToDictionary(kvp => kvp, kvp => false); + return userIds.ToDictionary(kvp => kvp, _ => false); } var usersOrganizationManagementStatus = await _getOrganizationUsersManagementStatusQuery.GetUsersOrganizationManagementStatusAsync(orgId, userIds); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsAuthorizationHandler.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsAuthorizationHandler.cs new file mode 100644 index 0000000000..52399f4c32 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsAuthorizationHandler.cs @@ -0,0 +1,30 @@ +#nullable enable +using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization; +using Bit.Core.Context; +using Microsoft.AspNetCore.Authorization; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization.OrganizationUserDetails; + +public class OrganizationUserDetailsAuthorizationHandler(ICurrentContext currentContext) : + AuthorizationHandler +{ + protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, + OrganizationUserDetailsOperationRequirement requirement, + OrganizationScope organizationScope) + { + var authorized = requirement switch + { + not null when requirement.Name == nameof(OrganizationUserDetailsOperations.Read) => await currentContext + .ManageUsers(organizationScope), + _ => false + }; + + if (authorized) + { + context.Succeed(requirement!); + return; + } + + context.Fail(); + } +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsOperations.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsOperations.cs new file mode 100644 index 0000000000..ff6fbeb9d4 --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserDetails/OrganizationUserDetailsOperations.cs @@ -0,0 +1,10 @@ +using Microsoft.AspNetCore.Authorization.Infrastructure; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization.OrganizationUserDetails; + +public class OrganizationUserDetailsOperationRequirement : OperationAuthorizationRequirement; + +public static class OrganizationUserDetailsOperations +{ + public static readonly OrganizationUserDetailsOperationRequirement Read = new() { Name = nameof(Read) }; +} diff --git a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs index 232e04fbd0..74da607286 100644 --- a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs +++ b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs @@ -12,6 +12,7 @@ using Bit.Core.AdminConsole.OrganizationFeatures.Organizations; using Bit.Core.AdminConsole.OrganizationFeatures.Organizations.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization.OrganizationUserDetails; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.Models.Business.Tokenables; using Bit.Core.OrganizationFeatures.OrganizationCollections; @@ -169,6 +170,7 @@ public static class OrganizationServiceCollectionExtensions services.AddScoped(); services.AddScoped(); + services.AddScoped(); services.AddScoped(); } diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index e3071bd227..b8498da91e 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -6,7 +6,9 @@ using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization.OrganizationUserDetails; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Entities; using Bit.Core.Auth.Repositories; @@ -250,9 +252,13 @@ public class OrganizationUsersControllerTests .IsEnabled(FeatureFlagKeys.AccountDeprovisioning) .Returns(accountDeprovisioningEnabled); - sutProvider.GetDependency() - .ManageUsers(organizationUser.OrganizationId) - .Returns(true); + sutProvider.GetDependency() + .AuthorizeAsync( + user: Arg.Any(), + resource: Arg.Is(x => x == organizationUser.OrganizationId), + requirements: Arg.Is>(x => + x.Any(y => y == OrganizationUserDetailsOperations.Read))) + .Returns(AuthorizationResult.Success()); sutProvider.GetDependency() .GetDetailsByIdWithCollectionsAsync(organizationUser.Id) @@ -262,7 +268,7 @@ public class OrganizationUsersControllerTests .GetUsersOrganizationManagementStatusAsync(organizationUser.OrganizationId, Arg.Is>(ids => ids.Contains(organizationUser.Id))) .Returns(new Dictionary { { organizationUser.Id, true } }); - var response = await sutProvider.Sut.Get(organizationUser.Id, false); + var response = await sutProvider.Sut.Get(organizationUser.OrganizationId, organizationUser.Id, false); Assert.Equal(organizationUser.Id, response.Id); Assert.Equal(accountDeprovisioningEnabled, response.ManagedByOrganization); diff --git a/test/Core.Test/AdminConsole/Authorization/OrganizationUserDetailsAuthorizationHandlerTests.cs b/test/Core.Test/AdminConsole/Authorization/OrganizationUserDetailsAuthorizationHandlerTests.cs new file mode 100644 index 0000000000..ca100b9ba4 --- /dev/null +++ b/test/Core.Test/AdminConsole/Authorization/OrganizationUserDetailsAuthorizationHandlerTests.cs @@ -0,0 +1,49 @@ +using System.Security.Claims; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization.OrganizationUserDetails; +using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization; +using Bit.Core.Context; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using NSubstitute; +using Xunit; + +namespace Bit.Core.Test.AdminConsole.Authorization; + +[SutProviderCustomize] +public class OrganizationUserDetailsAuthorizationHandlerTests +{ + [Theory] + [BitAutoData] + public async Task Read_UserCanManageUsers_ShouldReturnSuccess(CurrentContextOrganization contextOrganization, + SutProvider sutProvider) + { + sutProvider.GetDependency().ManageUsers(contextOrganization.Id).Returns(true); + + var context = new AuthorizationHandlerContext( + [OrganizationUserDetailsOperations.Read], + new ClaimsPrincipal(), + new OrganizationScope(contextOrganization.Id)); + + await sutProvider.Sut.HandleAsync(context); + + Assert.True(context.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task Read_UserCannotManageUsers_ShouldReturnFailure(CurrentContextOrganization contextOrganization, + SutProvider sutProvider) + { + sutProvider.GetDependency().ManageUsers(contextOrganization.Id).Returns(false); + + var context = new AuthorizationHandlerContext( + [OrganizationUserDetailsOperations.Read], + new ClaimsPrincipal(), + new OrganizationScope(contextOrganization.Id)); + + await sutProvider.Sut.HandleAsync(context); + + Assert.True(context.HasFailed); + } +}