From c4965350d19306e206bf99df3726a841d1d11407 Mon Sep 17 00:00:00 2001 From: Jared McCannon Date: Tue, 15 Jul 2025 07:52:47 -0500 Subject: [PATCH] [PM-12474] Move to authorization to attibutes/handlers/requirements (#6001) * Created ReadAllOrganizationUsersBasicInformationRequirement for use with Authorize attribute. * Removed unused req and Handler and tests. Moved to new auth attribute * Moved tests to integration tests with new response. * Removed tests that were migrated to integration tests. * Made string params Guids instead of parsing them manually in methods. * Admin and Owner added to requirement. * Added XML docs for basic get endpoint. Removed unused. Added another auth check. Inverted if check. * Removed unused endpoint * Added tests for requirement * Added checks for both User and Custom * Added org id check to validate the user being requested belongs to the org in the route. * typo --- .../ManageGroupsOrUsersRequirement.cs | 17 ++ .../OrganizationUsersController.cs | 190 +++++------------- ...UserUserMiniDetailsAuthorizationHandler.cs | 50 ----- ...OrganizationServiceCollectionExtensions.cs | 1 - .../OrganizationUserControllerTests.cs | 101 ++++++++++ .../ManageGroupsOrUsersRequirementTests.cs | 84 ++++++++ .../OrganizationUsersControllerTests.cs | 36 +--- ...serMiniDetailsAuthorizationHandlerTests.cs | 81 -------- 8 files changed, 253 insertions(+), 307 deletions(-) create mode 100644 src/Api/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirement.cs delete mode 100644 src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandler.cs create mode 100644 test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs create mode 100644 test/Api.Test/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirementTests.cs delete mode 100644 test/Core.Test/AdminConsole/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandlerTests.cs diff --git a/src/Api/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirement.cs b/src/Api/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirement.cs new file mode 100644 index 0000000000..55dfb766d6 --- /dev/null +++ b/src/Api/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirement.cs @@ -0,0 +1,17 @@ +using Bit.Core.Context; +using Bit.Core.Enums; + +namespace Bit.Api.AdminConsole.Authorization.Requirements; + +public class ManageGroupsOrUsersRequirement : IOrganizationRequirement +{ + public async Task AuthorizeAsync(CurrentContextOrganization organizationClaims, Func> isProviderUserForOrg) => + organizationClaims switch + { + { Type: OrganizationUserType.Owner } => true, + { Type: OrganizationUserType.Admin } => true, + { Permissions.ManageGroups: true } => true, + { Permissions.ManageUsers: true } => true, + _ => await isProviderUserForOrg() + }; +} diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 81c31355e3..5409adc825 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -1,6 +1,7 @@ // FIXME: Update this file to be null safe and then delete the line below #nullable disable +using Bit.Api.AdminConsole.Authorization; using Bit.Api.AdminConsole.Authorization.Requirements; using Bit.Api.AdminConsole.Models.Request.Organizations; using Bit.Api.AdminConsole.Models.Response.Organizations; @@ -126,10 +127,11 @@ public class OrganizationUsersController : Controller } [HttpGet("{id}")] - public async Task Get(Guid id, bool includeGroups = false) + [Authorize] + public async Task Get(Guid orgId, Guid id, bool includeGroups = false) { var (organizationUser, collections) = await _organizationUserRepository.GetDetailsByIdWithCollectionsAsync(id); - if (organizationUser == null || !await _currentContext.ManageUsers(organizationUser.OrganizationId)) + if (organizationUser == null || organizationUser.OrganizationId != orgId) { throw new NotFoundException(); } @@ -148,16 +150,17 @@ public class OrganizationUsersController : Controller return response; } + /// + /// Returns a set of basic information about all members of the organization. This is available to all members of + /// the organization to manage collections. For this reason, it contains as little information as possible and no + /// cryptographic keys or other sensitive data. + /// + /// Organization identifier + /// List of users for the organization. [HttpGet("mini-details")] + [Authorize] public async Task> GetMiniDetails(Guid orgId) { - var authorizationResult = await _authorizationService.AuthorizeAsync(User, new OrganizationScope(orgId), - OrganizationUserUserMiniDetailsOperations.ReadAll); - if (!authorizationResult.Succeeded) - { - throw new NotFoundException(); - } - var organizationUserUserDetails = await _organizationUserRepository.GetManyDetailsByOrganizationAsync(orgId); return new ListResponseModel( organizationUserUserDetails.Select(ou => new OrganizationUserUserMiniDetailsResponseModel(ou))); @@ -207,7 +210,7 @@ public class OrganizationUsersController : Controller { OrganizationId = orgId, IncludeGroups = includeGroups, - IncludeCollections = includeCollections, + IncludeCollections = includeCollections }; if ((await _authorizationService.AuthorizeAsync(User, new ManageUsersRequirement())).Succeeded) @@ -231,34 +234,12 @@ public class OrganizationUsersController : Controller .ToList()); } - - [HttpGet("{id}/groups")] - public async Task> GetGroups(string orgId, string id) - { - var organizationUser = await _organizationUserRepository.GetByIdAsync(new Guid(id)); - if (organizationUser == null || (!await _currentContext.ManageGroups(organizationUser.OrganizationId) && - !await _currentContext.ManageUsers(organizationUser.OrganizationId))) - { - throw new NotFoundException(); - } - - var groupIds = await _groupRepository.GetManyIdsByUserIdAsync(organizationUser.Id); - var responses = groupIds.Select(g => g.ToString()); - return responses; - } - [HttpGet("{id}/reset-password-details")] - public async Task GetResetPasswordDetails(string orgId, string id) + [Authorize] + public async Task GetResetPasswordDetails(Guid orgId, Guid id) { - // Make sure the calling user can reset passwords for this org - var orgGuidId = new Guid(orgId); - if (!await _currentContext.ManageResetPassword(orgGuidId)) - { - throw new NotFoundException(); - } - - var organizationUser = await _organizationUserRepository.GetByIdAsync(new Guid(id)); - if (organizationUser == null || !organizationUser.UserId.HasValue) + var organizationUser = await _organizationUserRepository.GetByIdAsync(id); + if (organizationUser is null || organizationUser.UserId is null) { throw new NotFoundException(); } @@ -272,7 +253,7 @@ public class OrganizationUsersController : Controller } // Retrieve Encrypted Private Key from organization - var org = await _organizationRepository.GetByIdAsync(orgGuidId); + var org = await _organizationRepository.GetByIdAsync(orgId); if (org == null) { throw new NotFoundException(); @@ -282,26 +263,17 @@ public class OrganizationUsersController : Controller } [HttpPost("account-recovery-details")] + [Authorize] public async Task> GetAccountRecoveryDetails(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - // Make sure the calling user can reset passwords for this org - if (!await _currentContext.ManageResetPassword(orgId)) - { - throw new NotFoundException(); - } - var responses = await _organizationUserRepository.GetManyAccountRecoveryDetailsByOrganizationUserAsync(orgId, model.Ids); return new ListResponseModel(responses.Select(r => new OrganizationUserResetPasswordDetailsResponseModel(r))); } [HttpPost("invite")] + [Authorize] public async Task Invite(Guid orgId, [FromBody] OrganizationUserInviteRequestModel model) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - // Check the user has permission to grant access to the collections for the new user if (model.Collections?.Any() == true) { @@ -317,35 +289,25 @@ public class OrganizationUsersController : Controller var userId = _userService.GetProperUserId(User); await _organizationService.InviteUsersAsync(orgId, userId.Value, systemUser: null, - new (OrganizationUserInvite, string)[] { (new OrganizationUserInvite(model.ToData()), null) }); + [(new OrganizationUserInvite(model.ToData()), null)]); } [HttpPost("reinvite")] - public async Task> BulkReinvite(string orgId, [FromBody] OrganizationUserBulkRequestModel model) + [Authorize] + public async Task> BulkReinvite(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - var orgGuidId = new Guid(orgId); - if (!await _currentContext.ManageUsers(orgGuidId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); - var result = await _organizationService.ResendInvitesAsync(orgGuidId, userId.Value, model.Ids); + var result = await _organizationService.ResendInvitesAsync(orgId, userId.Value, model.Ids); return new ListResponseModel( result.Select(t => new OrganizationUserBulkResponseModel(t.Item1.Id, t.Item2))); } [HttpPost("{id}/reinvite")] - public async Task Reinvite(string orgId, string id) + [Authorize] + public async Task Reinvite(Guid orgId, Guid id) { - var orgGuidId = new Guid(orgId); - if (!await _currentContext.ManageUsers(orgGuidId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); - await _organizationService.ResendInviteAsync(orgGuidId, userId.Value, new Guid(id)); + await _organizationService.ResendInviteAsync(orgId, userId.Value, id); } [HttpPost("{organizationUserId}/accept-init")] @@ -406,57 +368,39 @@ public class OrganizationUsersController : Controller } [HttpPost("{id}/confirm")] + [Authorize] public async Task Confirm(Guid orgId, Guid id, [FromBody] OrganizationUserConfirmRequestModel model) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); - var result = await _confirmOrganizationUserCommand.ConfirmUserAsync(orgId, id, model.Key, userId.Value, model.DefaultUserCollectionName); + _ = await _confirmOrganizationUserCommand.ConfirmUserAsync(orgId, id, model.Key, userId.Value, model.DefaultUserCollectionName); } [HttpPost("confirm")] - public async Task> BulkConfirm(string orgId, + [Authorize] + public async Task> BulkConfirm(Guid orgId, [FromBody] OrganizationUserBulkConfirmRequestModel model) { - var orgGuidId = new Guid(orgId); - if (!await _currentContext.ManageUsers(orgGuidId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); - var results = await _confirmOrganizationUserCommand.ConfirmUsersAsync(orgGuidId, model.ToDictionary(), userId.Value); + var results = await _confirmOrganizationUserCommand.ConfirmUsersAsync(orgId, model.ToDictionary(), userId.Value); return new ListResponseModel(results.Select(r => new OrganizationUserBulkResponseModel(r.Item1.Id, r.Item2))); } [HttpPost("public-keys")] - public async Task> UserPublicKeys(string orgId, [FromBody] OrganizationUserBulkRequestModel model) + [Authorize] + public async Task> UserPublicKeys(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - var orgGuidId = new Guid(orgId); - if (!await _currentContext.ManageUsers(orgGuidId)) - { - throw new NotFoundException(); - } - - var result = await _organizationUserRepository.GetManyPublicKeysByOrganizationUserAsync(orgGuidId, model.Ids); + var result = await _organizationUserRepository.GetManyPublicKeysByOrganizationUserAsync(orgId, model.Ids); var responses = result.Select(r => new OrganizationUserPublicKeyResponseModel(r.Id, r.UserId, r.PublicKey)).ToList(); return new ListResponseModel(responses); } [HttpPut("{id}")] [HttpPost("{id}")] + [Authorize] public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var (organizationUser, currentAccess) = await _organizationUserRepository.GetByIdWithCollectionsAsync(id); if (organizationUser == null || organizationUser.OrganizationId != orgId) { @@ -557,27 +501,19 @@ public class OrganizationUsersController : Controller } [HttpPut("{id}/reset-password")] - public async Task PutResetPassword(string orgId, string id, [FromBody] OrganizationUserResetPasswordRequestModel model) + [Authorize] + public async Task PutResetPassword(Guid orgId, Guid id, [FromBody] OrganizationUserResetPasswordRequestModel model) { - - var orgGuidId = new Guid(orgId); - - // Calling user must have Manage Reset Password permission - if (!await _currentContext.ManageResetPassword(orgGuidId)) - { - throw new NotFoundException(); - } - // Get the users role, since provider users aren't a member of the organization we use the owner check - var orgUserType = await _currentContext.OrganizationOwner(orgGuidId) + var orgUserType = await _currentContext.OrganizationOwner(orgId) ? OrganizationUserType.Owner - : _currentContext.Organizations?.FirstOrDefault(o => o.Id == orgGuidId)?.Type; + : _currentContext.Organizations?.FirstOrDefault(o => o.Id == orgId)?.Type; if (orgUserType == null) { throw new NotFoundException(); } - var result = await _userService.AdminResetPasswordAsync(orgUserType.Value, orgGuidId, new Guid(id), model.NewMasterPasswordHash, model.Key); + var result = await _userService.AdminResetPasswordAsync(orgUserType.Value, orgId, id, model.NewMasterPasswordHash, model.Key); if (result.Succeeded) { return; @@ -594,26 +530,18 @@ public class OrganizationUsersController : Controller [HttpDelete("{id}")] [HttpPost("{id}/remove")] + [Authorize] public async Task Remove(Guid orgId, Guid id) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); await _removeOrganizationUserCommand.RemoveUserAsync(orgId, id, userId.Value); } [HttpDelete("")] [HttpPost("remove")] + [Authorize] public async Task> BulkRemove(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); var result = await _removeOrganizationUserCommand.RemoveUsersAsync(orgId, model.Ids, userId.Value); return new ListResponseModel(result.Select(r => @@ -622,13 +550,9 @@ public class OrganizationUsersController : Controller [HttpDelete("{id}/delete-account")] [HttpPost("{id}/delete-account")] + [Authorize] public async Task DeleteAccount(Guid orgId, Guid id) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var currentUser = await _userService.GetUserByPrincipalAsync(User); if (currentUser == null) { @@ -640,13 +564,9 @@ public class OrganizationUsersController : Controller [HttpDelete("delete-account")] [HttpPost("delete-account")] + [Authorize] public async Task> BulkDeleteAccount(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var currentUser = await _userService.GetUserByPrincipalAsync(User); if (currentUser == null) { @@ -661,6 +581,7 @@ public class OrganizationUsersController : Controller [HttpPatch("{id}/revoke")] [HttpPut("{id}/revoke")] + [Authorize] public async Task RevokeAsync(Guid orgId, Guid id) { await RestoreOrRevokeUserAsync(orgId, id, _organizationService.RevokeUserAsync); @@ -668,6 +589,7 @@ public class OrganizationUsersController : Controller [HttpPatch("revoke")] [HttpPut("revoke")] + [Authorize] public async Task> BulkRevokeAsync(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { return await RestoreOrRevokeUsersAsync(orgId, model, _organizationService.RevokeUsersAsync); @@ -675,6 +597,7 @@ public class OrganizationUsersController : Controller [HttpPatch("{id}/restore")] [HttpPut("{id}/restore")] + [Authorize] public async Task RestoreAsync(Guid orgId, Guid id) { await RestoreOrRevokeUserAsync(orgId, id, (orgUser, userId) => _restoreOrganizationUserCommand.RestoreUserAsync(orgUser, userId)); @@ -682,6 +605,7 @@ public class OrganizationUsersController : Controller [HttpPatch("restore")] [HttpPut("restore")] + [Authorize] public async Task> BulkRestoreAsync(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { return await RestoreOrRevokeUsersAsync(orgId, model, (orgId, orgUserIds, restoringUserId) => _restoreOrganizationUserCommand.RestoreUsersAsync(orgId, orgUserIds, restoringUserId, _userService)); @@ -689,14 +613,10 @@ public class OrganizationUsersController : Controller [HttpPatch("enable-secrets-manager")] [HttpPut("enable-secrets-manager")] + [Authorize] public async Task BulkEnableSecretsManagerAsync(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var orgUsers = (await _organizationUserRepository.GetManyAsync(model.Ids)) .Where(ou => ou.OrganizationId == orgId && !ou.AccessSecretsManager).ToList(); if (orgUsers.Count == 0) @@ -729,11 +649,6 @@ public class OrganizationUsersController : Controller Guid id, Func statusAction) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); var orgUser = await _organizationUserRepository.GetByIdAsync(id); if (orgUser == null || orgUser.OrganizationId != orgId) @@ -749,11 +664,6 @@ public class OrganizationUsersController : Controller OrganizationUserBulkRequestModel model, Func, Guid?, Task>>> statusAction) { - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - var userId = _userService.GetProperUserId(User); var result = await statusAction(orgId, model.Ids, userId.Value); return new ListResponseModel(result.Select(r => diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandler.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandler.cs deleted file mode 100644 index e63b6bf096..0000000000 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandler.cs +++ /dev/null @@ -1,50 +0,0 @@ -// FIXME: Update this file to be null safe and then delete the line below -#nullable disable - -using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization; -using Bit.Core.Context; -using Microsoft.AspNetCore.Authorization; - -namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization; - -public class OrganizationUserUserMiniDetailsAuthorizationHandler : - AuthorizationHandler -{ - private readonly ICurrentContext _currentContext; - - public OrganizationUserUserMiniDetailsAuthorizationHandler(ICurrentContext currentContext) - { - _currentContext = currentContext; - } - - protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, - OrganizationUserUserMiniDetailsOperationRequirement requirement, OrganizationScope organizationScope) - { - var authorized = false; - - switch (requirement) - { - case not null when requirement.Name == nameof(OrganizationUserUserMiniDetailsOperations.ReadAll): - authorized = await CanReadAllAsync(organizationScope); - break; - } - - if (authorized) - { - context.Succeed(requirement); - } - } - - private async Task CanReadAllAsync(Guid organizationId) - { - // All organization users can access this data to manage collection access - var organization = _currentContext.GetOrganization(organizationId); - if (organization != null) - { - return true; - } - - // Providers can also access this to manage the organization generally - return await _currentContext.ProviderUserForOrgAsync(organizationId); - } -} diff --git a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs index ac1fe262c2..ae24017e48 100644 --- a/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs +++ b/src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs @@ -178,7 +178,6 @@ public static class OrganizationServiceCollectionExtensions services.AddScoped(); - services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs new file mode 100644 index 0000000000..ca13585017 --- /dev/null +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs @@ -0,0 +1,101 @@ +using System.Net; +using Bit.Api.AdminConsole.Models.Request.Organizations; +using Bit.Api.IntegrationTest.Factories; +using Bit.Api.IntegrationTest.Helpers; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Billing.Enums; +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Xunit; + +namespace Bit.Api.IntegrationTest.AdminConsole.Controllers; + +public class OrganizationUserControllerTests : IClassFixture, IAsyncLifetime +{ + public OrganizationUserControllerTests(ApiApplicationFactory apiFactory) + { + _factory = apiFactory; + _client = _factory.CreateClient(); + _loginHelper = new LoginHelper(_factory, _client); + } + + private readonly HttpClient _client; + private readonly ApiApplicationFactory _factory; + private readonly LoginHelper _loginHelper; + + private Organization _organization = null!; + private string _ownerEmail = null!; + + [Theory] + [InlineData(OrganizationUserType.User)] + [InlineData(OrganizationUserType.Custom)] + public async Task BulkDeleteAccount_WhenUserCannotManageUsers_ReturnsForbiddenResponse(OrganizationUserType organizationUserType) + { + var (userEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, organizationUserType, new Permissions { ManageUsers = false }); + + await _loginHelper.LoginAsync(userEmail); + + var request = new OrganizationUserBulkRequestModel + { + Ids = new List { Guid.NewGuid() } + }; + + var httpResponse = await _client.PostAsJsonAsync($"organizations/{_organization.Id}/users/remove", request); + + Assert.Equal(HttpStatusCode.Forbidden, httpResponse.StatusCode); + } + + [Theory] + [InlineData(OrganizationUserType.User)] + [InlineData(OrganizationUserType.Custom)] + public async Task DeleteAccount_WhenUserCannotManageUsers_ReturnsForbiddenResponse(OrganizationUserType organizationUserType) + { + var (userEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, organizationUserType, new Permissions { ManageUsers = false }); + + await _loginHelper.LoginAsync(userEmail); + + var userToRemove = Guid.NewGuid(); + + var httpResponse = await _client.DeleteAsync($"organizations/{_organization.Id}/users/{userToRemove}"); + + Assert.Equal(HttpStatusCode.Forbidden, httpResponse.StatusCode); + } + + [Theory] + [InlineData(OrganizationUserType.User)] + [InlineData(OrganizationUserType.Custom)] + public async Task GetAccountRecoveryDetails_WithoutManageResetPasswordPermission_ReturnsForbiddenResponse(OrganizationUserType organizationUserType) + { + var (userEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, organizationUserType, new Permissions { ManageUsers = false }); + + await _loginHelper.LoginAsync(userEmail); + + var request = new OrganizationUserBulkRequestModel + { + Ids = [] + }; + + var httpResponse = + await _client.PostAsJsonAsync($"organizations/{_organization.Id}/users/account-recovery-details", request); + + Assert.Equal(HttpStatusCode.Forbidden, httpResponse.StatusCode); + } + + public async Task InitializeAsync() + { + _ownerEmail = $"org-user-integration-test-{Guid.NewGuid()}@bitwarden.com"; + await _factory.LoginWithNewAccount(_ownerEmail); + + (_organization, _) = await OrganizationTestHelpers.SignUpAsync(_factory, plan: PlanType.EnterpriseAnnually2023, + ownerEmail: _ownerEmail, passwordManagerSeats: 5, paymentMethod: PaymentMethodType.Card); + } + + public Task DisposeAsync() + { + _client.Dispose(); + return Task.CompletedTask; + } +} diff --git a/test/Api.Test/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirementTests.cs b/test/Api.Test/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirementTests.cs new file mode 100644 index 0000000000..1d6270ba1f --- /dev/null +++ b/test/Api.Test/AdminConsole/Authorization/Requirements/ManageGroupsOrUsersRequirementTests.cs @@ -0,0 +1,84 @@ +using Bit.Api.AdminConsole.Authorization.Requirements; +using Bit.Core.Context; +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Bit.Core.Test.AdminConsole.AutoFixture; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Authorization.Requirements; + +[SutProviderCustomize] +public class ManageGroupsOrUsersRequirementTests +{ + [Theory] + [CurrentContextOrganizationCustomize] + [BitAutoData(OrganizationUserType.Admin)] + [BitAutoData(OrganizationUserType.Owner)] + public async Task AuthorizeAsync_WhenUserTypeCanManageUsers_ThenRequestShouldBeAuthorized( + OrganizationUserType type, + CurrentContextOrganization organization, + SutProvider sutProvider) + { + organization.Type = type; + + var actual = await sutProvider.Sut.AuthorizeAsync(organization, () => Task.FromResult(false)); + + Assert.True(actual); + } + + [Theory] + [CurrentContextOrganizationCustomize] + [BitAutoData(OrganizationUserType.Custom, true, false)] + [BitAutoData(OrganizationUserType.Custom, false, true)] + public async Task AuthorizeAsync_WhenCustomUserThatCanManageUsersOrGroups_ThenRequestShouldBeAuthorized( + OrganizationUserType type, + bool canManageUsers, + bool canManageGroups, + CurrentContextOrganization organization, + SutProvider sutProvider) + { + organization.Type = type; + organization.Permissions = new Permissions { ManageUsers = canManageUsers, ManageGroups = canManageGroups }; + + var actual = await sutProvider.Sut.AuthorizeAsync(organization, () => Task.FromResult(false)); + + Assert.True(actual); + } + + [Theory] + [CurrentContextOrganizationCustomize] + [BitAutoData] + public async Task AuthorizeAsync_WhenProviderUserForAnOrganization_ThenRequestShouldBeAuthorized( + CurrentContextOrganization organization, + SutProvider sutProvider) + { + var actual = await sutProvider.Sut.AuthorizeAsync(organization, IsProviderUserForOrg); + + Assert.True(actual); + return; + + Task IsProviderUserForOrg() => Task.FromResult(true); + } + + [Theory] + [CurrentContextOrganizationCustomize] + [BitAutoData(OrganizationUserType.User)] + [BitAutoData(OrganizationUserType.Custom)] + public async Task AuthorizeAsync_WhenUserCannotManageUsersOrGroupsAndIsNotAProviderUser_ThenRequestShouldBeDenied( + OrganizationUserType type, + CurrentContextOrganization organization, + SutProvider sutProvider) + { + organization.Type = type; + organization.Permissions = new Permissions { ManageUsers = false, ManageGroups = false }; // When Type is User, the canManage permissions don't matter + + var actual = await sutProvider.Sut.AuthorizeAsync(organization, IsNotProviderUserForOrg); + + Assert.False(actual); + return; + + Task IsNotProviderUserForOrg() => Task.FromResult(false); + } +} diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index de54a44bca..cc480d1dcb 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -257,7 +257,7 @@ public class OrganizationUsersControllerTests .GetUsersOrganizationClaimedStatusAsync(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.True(response.ManagedByOrganization); @@ -303,18 +303,6 @@ public class OrganizationUsersControllerTests ou.EncryptedPrivateKey == r.EncryptedPrivateKey))); } - [Theory] - [BitAutoData] - public async Task GetAccountRecoveryDetails_WithoutManageResetPasswordPermission_Throws( - Guid organizationId, - OrganizationUserBulkRequestModel bulkRequestModel, - SutProvider sutProvider) - { - sutProvider.GetDependency().ManageResetPassword(organizationId).Returns(false); - - await Assert.ThrowsAsync(async () => await sutProvider.Sut.GetAccountRecoveryDetails(organizationId, bulkRequestModel)); - } - [Theory] [BitAutoData] public async Task DeleteAccount_WhenUserCanManageUsers_Success( @@ -330,17 +318,6 @@ public class OrganizationUsersControllerTests .DeleteUserAsync(orgId, id, currentUser.Id); } - [Theory] - [BitAutoData] - public async Task DeleteAccount_WhenUserCannotManageUsers_ThrowsNotFoundException( - Guid orgId, Guid id, SutProvider sutProvider) - { - sutProvider.GetDependency().ManageUsers(orgId).Returns(false); - - await Assert.ThrowsAsync(() => - sutProvider.Sut.DeleteAccount(orgId, id)); - } - [Theory] [BitAutoData] public async Task DeleteAccount_WhenCurrentUserNotFound_ThrowsUnauthorizedAccessException( @@ -374,17 +351,6 @@ public class OrganizationUsersControllerTests .DeleteManyUsersAsync(orgId, model.Ids, currentUser.Id); } - [Theory] - [BitAutoData] - public async Task BulkDeleteAccount_WhenUserCannotManageUsers_ThrowsNotFoundException( - Guid orgId, OrganizationUserBulkRequestModel model, SutProvider sutProvider) - { - sutProvider.GetDependency().ManageUsers(orgId).Returns(false); - - await Assert.ThrowsAsync(() => - sutProvider.Sut.BulkDeleteAccount(orgId, model)); - } - [Theory] [BitAutoData] public async Task BulkDeleteAccount_WhenCurrentUserNotFound_ThrowsUnauthorizedAccessException( diff --git a/test/Core.Test/AdminConsole/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandlerTests.cs b/test/Core.Test/AdminConsole/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandlerTests.cs deleted file mode 100644 index 6732486a54..0000000000 --- a/test/Core.Test/AdminConsole/Authorization/OrganizationUserUserMiniDetailsAuthorizationHandlerTests.cs +++ /dev/null @@ -1,81 +0,0 @@ -using System.Security.Claims; -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization; -using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization; -using Bit.Core.Context; -using Bit.Core.Enums; -using Bit.Core.Test.AdminConsole.AutoFixture; -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 OrganizationUserUserMiniDetailsAuthorizationHandlerTests -{ - [Theory, CurrentContextOrganizationCustomize] - [BitAutoData(OrganizationUserType.Admin)] - [BitAutoData(OrganizationUserType.Owner)] - [BitAutoData(OrganizationUserType.Custom)] - [BitAutoData(OrganizationUserType.User)] - public async Task ReadAll_AnyOrganizationMember_Success( - OrganizationUserType userType, - CurrentContextOrganization organization, - SutProvider sutProvider) - { - organization.Type = userType; - sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); - - var context = new AuthorizationHandlerContext( - new[] { OrganizationUserUserMiniDetailsOperations.ReadAll }, - new ClaimsPrincipal(), - new OrganizationScope(organization.Id)); - - await sutProvider.Sut.HandleAsync(context); - - Assert.True(context.HasSucceeded); - } - - [Theory, BitAutoData, CurrentContextOrganizationCustomize] - public async Task ReadAll_ProviderUser_Success( - CurrentContextOrganization organization, - SutProvider sutProvider) - { - organization.Type = OrganizationUserType.User; - sutProvider.GetDependency() - .GetOrganization(organization.Id) - .Returns((CurrentContextOrganization)null); - sutProvider.GetDependency() - .ProviderUserForOrgAsync(organization.Id) - .Returns(true); - - var context = new AuthorizationHandlerContext( - new[] { OrganizationUserUserMiniDetailsOperations.ReadAll }, - new ClaimsPrincipal(), - new OrganizationScope(organization.Id)); - - await sutProvider.Sut.HandleAsync(context); - - Assert.True(context.HasSucceeded); - } - - [Theory, BitAutoData, CurrentContextOrganizationCustomize] - public async Task ReadAll_NotMember_NoSuccess( - CurrentContextOrganization organization, - SutProvider sutProvider) - { - var context = new AuthorizationHandlerContext( - new[] { OrganizationUserUserMiniDetailsOperations.ReadAll }, - new ClaimsPrincipal(), - new OrganizationScope(organization.Id) - ); - - sutProvider.GetDependency().GetOrganization(Arg.Any()).Returns((CurrentContextOrganization)null); - sutProvider.GetDependency().ProviderUserForOrgAsync(Arg.Any()).Returns(false); - - await sutProvider.Sut.HandleAsync(context); - Assert.False(context.HasSucceeded); - } -}