diff --git a/src/Api/AdminConsole/Authorization/GetActingUserForOrganizationQuery.cs b/src/Api/AdminConsole/Authorization/GetActingUserForOrganizationQuery.cs new file mode 100644 index 000000000000..bb84b9c6b110 --- /dev/null +++ b/src/Api/AdminConsole/Authorization/GetActingUserForOrganizationQuery.cs @@ -0,0 +1,15 @@ +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.Context; + +namespace Bit.Api.AdminConsole.Authorization; + +public class GetActingUserForOrganizationQuery(ICurrentContext currentContext) : IGetActingUserForOrganizationQuery +{ + public async Task GetActingUserAsync(Guid userId, Guid organizationId) + { + var membership = currentContext.GetOrganization(organizationId); + var isProvider = await currentContext.ProviderUserForOrgAsync(organizationId); + + return new StandardUser(userId, isProvider, membership?.Type, membership?.Permissions); + } +} diff --git a/src/Api/AdminConsole/Authorization/IGetActingUserForOrganizationQuery.cs b/src/Api/AdminConsole/Authorization/IGetActingUserForOrganizationQuery.cs new file mode 100644 index 000000000000..5cb01e8f2e69 --- /dev/null +++ b/src/Api/AdminConsole/Authorization/IGetActingUserForOrganizationQuery.cs @@ -0,0 +1,16 @@ +using Bit.Core.AdminConsole.Models.Data; + +namespace Bit.Api.AdminConsole.Authorization; + +public interface IGetActingUserForOrganizationQuery +{ + /// + /// Resolves the caller into the that represents their role in a given organization. + /// When they are a member, their role and permissions are read from the current context. When they manage the + /// organization through a linked provider, a flagged as a provider is returned. + /// + /// + /// Thrown when the caller is neither a member of the organization nor manages it through a linked provider. + /// + Task GetActingUserAsync(Guid userId, Guid organizationId); +} diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 3393060b1b91..2f7a660115dd 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -95,6 +95,7 @@ public class OrganizationUsersController : BaseAdminConsoleController private readonly IConfirmOrganizationInviteLinkCommand _confirmOrganizationInviteLinkCommand; private readonly IGetOrganizationInviteCommand _getOrganizationInviteCommand; private readonly V2_UpdateUserCommand.IUpdateOrganizationUserCommand _updateOrganizationUserCommandVNext; + private readonly IGetActingUserForOrganizationQuery _getActingUserForOrganizationQuery; private readonly IGlobalSettings _globalSettings; public OrganizationUsersController(IOrganizationRepository organizationRepository, @@ -132,6 +133,7 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor IConfirmOrganizationInviteLinkCommand confirmOrganizationInviteLinkCommand, IGetOrganizationInviteCommand getOrganizationInviteCommand, V2_UpdateUserCommand.IUpdateOrganizationUserCommand updateOrganizationUserCommandVNext, + IGetActingUserForOrganizationQuery getActingUserForOrganizationQuery, IGlobalSettings globalSettings) { _organizationRepository = organizationRepository; @@ -169,6 +171,7 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor _confirmOrganizationInviteLinkCommand = confirmOrganizationInviteLinkCommand; _getOrganizationInviteCommand = getOrganizationInviteCommand; _updateOrganizationUserCommandVNext = updateOrganizationUserCommandVNext; + _getActingUserForOrganizationQuery = getActingUserForOrganizationQuery; _globalSettings = globalSettings; } @@ -450,8 +453,6 @@ public async Task Put([BindOrganization] Organization organization, Gui var collectionAccessToSave = await GetAuthorizedCollectionsToSaveAsync(model, currentAccess, editingSelf, organization); - var actingContext = _currentContext.GetOrganization(organization.Id); - var request = new V2_UpdateUserCommand.UpdateOrganizationUserRequest( organizationUser, organization, @@ -464,11 +465,7 @@ public async Task Put([BindOrganization] Organization organization, Gui model.Email, model.Name, model.DefaultUserCollectionName, - new StandardUser( - userId, - await _currentContext.OrganizationOwner(organization.Id), - actingContext?.Type, - actingContext?.Permissions)); + await _getActingUserForOrganizationQuery.GetActingUserAsync(userId, organization.Id)); var result = await _updateOrganizationUserCommandVNext.UpdateUserAsync(request); return Handle(result); diff --git a/src/Api/Utilities/ServiceCollectionExtensions.cs b/src/Api/Utilities/ServiceCollectionExtensions.cs index 278a04da2a10..43664e43b870 100644 --- a/src/Api/Utilities/ServiceCollectionExtensions.cs +++ b/src/Api/Utilities/ServiceCollectionExtensions.cs @@ -7,6 +7,7 @@ using Bit.SharedWeb.Swagger; using Bit.SharedWeb.Utilities; using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.OpenApi; namespace Bit.Api.Utilities; @@ -117,5 +118,8 @@ public static void AddAuthorizationHandlers(this IServiceCollection services) // Admin Console authorization handlers services.AddAdminConsoleAuthorizationHandlers(); + + // Admin Console ActingUserQuery + services.TryAddScoped(); } } diff --git a/src/Core/AdminConsole/Models/Data/IActingUser.cs b/src/Core/AdminConsole/Models/Data/IActingUser.cs index f97235f34cc4..cc16d3d41ef0 100644 --- a/src/Core/AdminConsole/Models/Data/IActingUser.cs +++ b/src/Core/AdminConsole/Models/Data/IActingUser.cs @@ -5,6 +5,7 @@ namespace Bit.Core.AdminConsole.Models.Data; public interface IActingUser { Guid? UserId { get; } + [Obsolete("This property is obsolete. Use isProvider or the OrganizationUserType where available instead.")] bool IsOrganizationOwnerOrProvider { get; } EventSystemUser? SystemUserType { get; } } diff --git a/src/Core/AdminConsole/Models/Data/StandardUser.cs b/src/Core/AdminConsole/Models/Data/StandardUser.cs index 0f2aa146f0a1..2a087fea6e62 100644 --- a/src/Core/AdminConsole/Models/Data/StandardUser.cs +++ b/src/Core/AdminConsole/Models/Data/StandardUser.cs @@ -3,13 +3,13 @@ namespace Bit.Core.AdminConsole.Models.Data; -public class StandardUser(Guid userId, bool isOrganizationOwner, OrganizationUserType? orgUserType = null, - Permissions? permissions = null) : IActingUser +public class StandardUser(Guid userId, bool isProvider, OrganizationUserType? orgUserType = null, Permissions? permissions = null) : IActingUser { public Guid? UserId { get; } = userId; - public bool IsOrganizationOwnerOrProvider { get; } = isOrganizationOwner; + [Obsolete("This property is obsolete. Use isProvider or the OrganizationUserType.")] + public bool IsOrganizationOwnerOrProvider => OrganizationUserType is Core.Enums.OrganizationUserType.Owner || IsProvider; public OrganizationUserType? OrganizationUserType { get; } = orgUserType; public Permissions? Permissions { get; } = permissions; public EventSystemUser? SystemUserType => throw new Exception($"{nameof(StandardUser)} does not have a {nameof(SystemUserType)}"); - + public bool IsProvider { get; } = isProvider; } diff --git a/src/Core/AdminConsole/Models/Data/SystemUser.cs b/src/Core/AdminConsole/Models/Data/SystemUser.cs index ab371ec024af..fef4ef0660d8 100644 --- a/src/Core/AdminConsole/Models/Data/SystemUser.cs +++ b/src/Core/AdminConsole/Models/Data/SystemUser.cs @@ -5,7 +5,7 @@ namespace Bit.Core.AdminConsole.Models.Data; public class SystemUser(EventSystemUser systemUser) : IActingUser { public Guid? UserId => throw new Exception($"{nameof(SystemUserType)} does not have a {nameof(UserId)}."); - + [Obsolete("This property is obsolete.")] public bool IsOrganizationOwnerOrProvider => false; public EventSystemUser? SystemUserType { get; } = systemUser; } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/Errors.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/Errors.cs index 06726aa9d176..342ac849b7b3 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/Errors.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/Errors.cs @@ -9,3 +9,5 @@ public record CustomUsersCannotManageAdminsOrOwners() : BadRequestError("Custom public record CustomUsersCanOnlyGrantOwnPermissions() : BadRequestError("Custom users can only grant the same custom permissions that they have."); public record CannotBeAdminOfMultipleFreeOrganizations() : BadRequestError("User can only be an admin of 1 free organization vault."); + +public record ActingUserMustBeMemberOrProvider() : BadRequestError("StandardUser must be organization member or managing provider member."); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/IOrganizationUserValidationService.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/IOrganizationUserValidationService.cs index 3f3c564add08..6246c444050b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/IOrganizationUserValidationService.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/IOrganizationUserValidationService.cs @@ -13,34 +13,41 @@ public interface IOrganizationUserValidationService /// /// Checks whether the acting user can manage the target user without escalating privileges: /// - /// Owners and provider users can manage anyone. + /// Owners can manage anyone. /// Admins can manage anyone except Owners. /// Custom users with ManageUsers can manage Users and other Custom users. /// Everyone else has no authority. /// - /// Pair with an AuthorizeAttribute for the standard RBAC check on the endpoint. + /// Pair with an AuthorizeAttribute for the standard RBAC check on the endpoint. Provider users hold + /// authority above the organization role hierarchy and are not evaluated here. /// - /// - /// Owners are allowed to manage provider users. If your operation affects the provider user as a provider user - /// (e.g. password reset, where account takeover would enable escalation) you may not want to allow this. - /// - /// The acting user's id, used to resolve provider authority. /// The acting user's role, or null if not a confirmed member. /// The member being managed. /// null when allowed, otherwise the error explaining why. - Task CanManageAsync(Guid actingUserId, IOrganizationUserRole? actingUser, IOrganizationUserRole targetUser); + Error? CanManage(IOrganizationUserRole? actingUser, IOrganizationUserRole targetUser); /// /// Checks whether the acting user can change the target member's role without escalating privileges. The acting /// user must be able to manage both the target's current and requested role, and a Custom user may only grant /// custom permissions they hold themselves. /// - /// The acting user's id, used to resolve provider authority. - /// The acting user's role. + /// The acting user. /// The member being managed, with their current role. /// The updated member being managed (desired role and permissions). - /// null when allowed, otherwise the error describing the denial. - Task CanManageRoleChangeAsync(Guid actingUserId, IOrganizationUserRole actingUser, IOrganizationUserRole targetUser, + /// null when allowed, otherwise the error describing why. + Error? CanManageRoleChange(IOrganizationUserRole actingUser, IOrganizationUserRole targetUser, + IOrganizationUserRole newTargetUser); + + /// + /// Checks whether the acting user can change the target member's role without escalating privileges. The acting + /// user must be able to manage both the target's current and requested role, and a Custom user may only grant + /// custom permissions they hold themselves. + /// + /// The caller acting on the member. + /// The member being managed, with their current role. + /// The updated member being managed (desired role and permissions). + /// null when allowed, otherwise the error describing why. + Error? CanManageRoleChange(IActingUser performedBy, IOrganizationUserRole targetUser, IOrganizationUserRole newTargetUser); /// diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationService.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationService.cs index 4fc56495d943..6ed6743fd287 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationService.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationService.cs @@ -1,7 +1,6 @@ -using Bit.Core.AdminConsole.Enums.Provider; -using Bit.Core.AdminConsole.Models.Data; -using Bit.Core.AdminConsole.Repositories; +using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.Utilities.v2; +using Bit.Core.AdminConsole.Utilities.v2.Results; using Bit.Core.Billing.Enums; using Bit.Core.Enums; using Bit.Core.Models.Data; @@ -11,34 +10,45 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Organizat /// public class OrganizationUserValidationService( - IProviderUserRepository providerUserRepository, IOrganizationUserRepository organizationUserRepository) : IOrganizationUserValidationService { - public async Task CanManageAsync(Guid actingUserId, IOrganizationUserRole? actingUser, IOrganizationUserRole targetUser) - { - if (IsAuthorizedByRole(actingUser, targetUser.Type) || await IsProviderAsync(actingUserId, targetUser.OrganizationId)) - { - return null; - } + public Error? CanManage(IOrganizationUserRole? actingUser, IOrganizationUserRole targetUser) => + IsAuthorizedByRole(actingUser, targetUser.Type) ? null : CannotManageError(targetUser.Type); - return CannotManageError(targetUser.Type); - } - - public async Task CanManageRoleChangeAsync(Guid actingUserId, IOrganizationUserRole actingUser, - IOrganizationUserRole targetUser, IOrganizationUserRole newTargetUser) + public Error? CanManageRoleChange(IOrganizationUserRole actingUser, IOrganizationUserRole targetUser, IOrganizationUserRole newTargetUser) { // Must be able to manage both the current and requested role. var authorizedByRole = IsAuthorizedByRole(actingUser, targetUser.Type) && IsAuthorizedByRole(actingUser, newTargetUser.Type); - if (!authorizedByRole && !await IsProviderAsync(actingUserId, targetUser.OrganizationId)) + return authorizedByRole + ? ValidateCustomPermissionsGrant(actingUser, newTargetUser) + : CannotManageError(targetUser.Type, newTargetUser.Type); + } + + public Error? CanManageRoleChange(IActingUser performedBy, IOrganizationUserRole targetUser, IOrganizationUserRole newTargetUser) + { + // SystemUsers exist outside the organization hierarchy. + if (performedBy is not StandardUser standardUser) { - return CannotManageError(targetUser.Type, newTargetUser.Type); + return null; } - return ValidateCustomPermissionsGrant(actingUser, newTargetUser); + return GetActingUser(standardUser, targetUser.OrganizationId) + .Match( + error => error, + role => CanManageRoleChange(role, targetUser, newTargetUser)); } + private static CommandResult GetActingUser(StandardUser standardUser, Guid organizationId) => + standardUser switch + { + // Providers can act as owners when managing organization members + { IsProvider: true } => new OrganizationUserRole(OrganizationUserType.Owner, organizationId), + { OrganizationUserType: not null } => new OrganizationUserRole(standardUser.OrganizationUserType.Value, organizationId, standardUser.Permissions), + _ => new ActingUserMustBeMemberOrProvider() + }; + public async Task ValidateFreeOrgAdminLimitAsync(Guid? userId, PlanType planType, OrganizationUserType currentUserType, OrganizationUserType newUserType) { @@ -93,9 +103,4 @@ private static bool IsAuthorizedByRole(IOrganizationUserRole? actingUser, Organi targetType is OrganizationUserType.User or OrganizationUserType.Custom, _ => false }; - - // Provider users aren't org members but hold Owner-level authority. - private async Task IsProviderAsync(Guid actingUserId, Guid organizationId) => - (await providerUserRepository.GetManyOrganizationDetailsByUserAsync(actingUserId, ProviderUserStatusType.Confirmed)) - .Any(po => po.OrganizationId == organizationId); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs index b9e2fef104e7..dbf6311b7d4c 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidator.cs @@ -69,7 +69,14 @@ public async Task> ValidateAsync } } - var roleChangeError = await ValidateRoleChangeAsync(request); + var roleChangeError = organizationUserValidationService.CanManageRoleChange( + request.PerformedBy, + request.OrganizationUserToUpdate, + new OrganizationUserRole( + request.NewType, + request.Organization.Id, + request.NewPermissions)); + if (roleChangeError is not null) { return Invalid(request, roleChangeError); @@ -178,34 +185,6 @@ public async Task> ValidateAsync : new EmailTakenOutsideOrganizationError(); } - /// - /// Delegates the role-change authority decision to - /// . System users skip the check. - /// - private async Task ValidateRoleChangeAsync(UpdateOrganizationUserRequest request) - { - if (request.PerformedBy is not StandardUser standardUser) - { - return null; - } - - var actingUser = new OrganizationUserRole( - standardUser.OrganizationUserType!.Value, - request.OrganizationUserToUpdate.OrganizationId, - standardUser.Permissions); - - var newTargetUser = new OrganizationUserRole( - request.NewType, - request.OrganizationUserToUpdate.OrganizationId, - request.NewPermissions); - - return await organizationUserValidationService.CanManageRoleChangeAsync( - standardUser.UserId!.Value, - actingUser, - request.OrganizationUserToUpdate, - newTargetUser); - } - private static bool CollectionsAreValid(List collectionAccessToSave, ICollection collectionsToSave, Guid organizationId) { diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index 307c18acde14..7d7072f8a7e6 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -5,6 +5,7 @@ using Bit.Api.IntegrationTest.Helpers; using Bit.Api.Models.Request; using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.UpdateUser.v2; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Utilities.v2.Validation; @@ -577,6 +578,40 @@ public async Task Put_WhenChangingEmailToAddressTakenOutsideOrganization_Returns await AssertValidationProblemAsync(response, new EmailTakenOutsideOrganizationError()); } + [Fact] + public async Task Put_AsProviderUserForOrganization_PersistsChanges() + { + // A provider user with no organization membership is authorized to manage the org's users + // because their provider is linked to the organization (ManageUsersRequirement falls through + // to the provider check). + var provider = await ProviderTestHelpers.CreateProviderAndLinkToOrganizationAsync( + _factory, _organization.Id, ProviderType.Msp); + + var providerUserEmail = $"provider-user-{Guid.NewGuid()}@bitwarden.com"; + await _factory.LoginWithNewAccount(providerUserEmail); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, providerUserEmail, + ProviderUserType.ProviderAdmin); + + var (_, member) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, _organization.Id, + OrganizationUserType.User); + + await _loginHelper.LoginAsync(providerUserEmail); + + var request = new OrganizationUserUpdateRequestModel + { + Type = OrganizationUserType.Admin, + Permissions = new Permissions(), + Collections = [], + Groups = [] + }; + var response = await _client.PutAsJsonAsync($"organizations/{_organization.Id}/users/{member.Id}", request); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + var updatedOrgUser = await _factory.GetService().GetByIdAsync(member.Id); + Assert.NotNull(updatedOrgUser); + Assert.Equal(OrganizationUserType.Admin, updatedOrgUser.Type); + } + private async Task SetAllowAdminAccessToAllCollectionItemsAsync(bool value) { _organization.AllowAdminAccessToAllCollectionItems = value; diff --git a/test/Api.Test/AdminConsole/Authorization/GetActingUserForOrganizationQueryTests.cs b/test/Api.Test/AdminConsole/Authorization/GetActingUserForOrganizationQueryTests.cs new file mode 100644 index 000000000000..d6fec60d0c20 --- /dev/null +++ b/test/Api.Test/AdminConsole/Authorization/GetActingUserForOrganizationQueryTests.cs @@ -0,0 +1,109 @@ +using Bit.Api.AdminConsole.Authorization; +using Bit.Core.AdminConsole.Models.Data; +using Bit.Core.Context; +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Authorization; + +public class GetActingUserForOrganizationQueryTests +{ + [Theory] + [BitAutoData] + public async Task GetActingUserAsync_OrganizationOwner_ReturnsStandardUserAsOwner(Guid userId, Guid organizationId) + { + var currentContext = Substitute.For(); + var sut = new GetActingUserForOrganizationQuery(currentContext); + currentContext.GetOrganization(organizationId).Returns(new CurrentContextOrganization + { + Id = organizationId, + Type = OrganizationUserType.Owner, + }); + + var result = await sut.GetActingUserAsync(userId, organizationId); + + var standardUser = Assert.IsType(result); + Assert.Equal(userId, standardUser.UserId); + Assert.Equal(OrganizationUserType.Owner, standardUser.OrganizationUserType); + Assert.True(standardUser.IsOrganizationOwnerOrProvider); + } + + [Theory] + [BitAutoData] + public async Task GetActingUserAsync_OrganizationAdmin_ReturnsStandardUserNotOwner(Guid userId, Guid organizationId) + { + var currentContext = Substitute.For(); + var sut = new GetActingUserForOrganizationQuery(currentContext); + currentContext.GetOrganization(organizationId).Returns(new CurrentContextOrganization + { + Id = organizationId, + Type = OrganizationUserType.Admin, + }); + + var result = await sut.GetActingUserAsync(userId, organizationId); + + var standardUser = Assert.IsType(result); + Assert.Equal(OrganizationUserType.Admin, standardUser.OrganizationUserType); + Assert.False(standardUser.IsOrganizationOwnerOrProvider); + } + + [Theory] + [BitAutoData] + public async Task GetActingUserAsync_OrganizationCustom_ReturnsStandardUserWithPermissions( + Guid userId, Guid organizationId, Permissions permissions) + { + var currentContext = Substitute.For(); + var sut = new GetActingUserForOrganizationQuery(currentContext); + currentContext.GetOrganization(organizationId).Returns(new CurrentContextOrganization + { + Id = organizationId, + Type = OrganizationUserType.Custom, + Permissions = permissions, + }); + + var result = await sut.GetActingUserAsync(userId, organizationId); + + var standardUser = Assert.IsType(result); + Assert.Equal(OrganizationUserType.Custom, standardUser.OrganizationUserType); + Assert.Same(permissions, standardUser.Permissions); + } + + [Theory] + [BitAutoData] + public async Task GetActingUserAsync_ManagingProvider_ReturnsStandardUserFlaggedAsProvider( + Guid userId, Guid organizationId) + { + var currentContext = Substitute.For(); + var sut = new GetActingUserForOrganizationQuery(currentContext); + currentContext.GetOrganization(organizationId).Returns((CurrentContextOrganization?)null); + currentContext.ProviderUserForOrgAsync(organizationId).Returns(true); + + var result = await sut.GetActingUserAsync(userId, organizationId); + + var standardUser = Assert.IsType(result); + Assert.Equal(userId, standardUser.UserId); + Assert.True(standardUser.IsProvider); + Assert.Null(standardUser.OrganizationUserType); + } + + [Theory] + [BitAutoData] + public async Task GetActingUserAsync_NeitherMemberNorProvider_ReturnsStandardUserWithNoAuthority( + Guid userId, Guid organizationId) + { + var currentContext = Substitute.For(); + var sut = new GetActingUserForOrganizationQuery(currentContext); + currentContext.GetOrganization(organizationId).Returns((CurrentContextOrganization?)null); + currentContext.ProviderUserForOrgAsync(organizationId).Returns(false); + + var result = await sut.GetActingUserAsync(userId, organizationId); + + var standardUser = Assert.IsType(result); + Assert.Equal(userId, standardUser.UserId); + Assert.False(standardUser.IsProvider); + Assert.Null(standardUser.OrganizationUserType); + } +} diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationServiceTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationServiceTests.cs index c175fed69380..89456ee511e1 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationServiceTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/OrganizationUserAction/OrganizationUserValidationServiceTests.cs @@ -1,8 +1,5 @@ -using Bit.Core.AdminConsole.Enums.Provider; -using Bit.Core.AdminConsole.Models.Data; -using Bit.Core.AdminConsole.Models.Data.Provider; +using Bit.Core.AdminConsole.Models.Data; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationUserAction; -using Bit.Core.AdminConsole.Repositories; using Bit.Core.Billing.Enums; using Bit.Core.Entities; using Bit.Core.Enums; @@ -15,20 +12,18 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.OrganizationUsers.Orga public class OrganizationUserValidationServiceTests { - private static readonly Guid _actingUserId = Guid.NewGuid(); private static readonly Guid _organizationId = Guid.NewGuid(); - private readonly IProviderUserRepository _providerUserRepository = Substitute.For(); private readonly IOrganizationUserRepository _organizationUserRepository = Substitute.For(); private readonly OrganizationUserValidationService _sut; public OrganizationUserValidationServiceTests() { - _sut = new OrganizationUserValidationService(_providerUserRepository, _organizationUserRepository); + _sut = new OrganizationUserValidationService(_organizationUserRepository); } - // NOTE: A null `actingUser` represents a non-member (provider-only user). Custom users are granted the - // ManageUsers permission by default, since that is the authority a Custom user needs to act on members. + // NOTE: A null `performedBy` represents a non-member. Custom users are granted the ManageUsers permission by + // default, since that is the authority a Custom user needs to act on members. private static OrganizationUser? ActingUser(OrganizationUserType? role, bool manageUsers = true) { if (role is null) @@ -61,11 +56,11 @@ private static OrganizationUserRole NewRole(OrganizationUserType type, Permissio [InlineData(OrganizationUserType.Admin, OrganizationUserType.Custom)] [InlineData(OrganizationUserType.Custom, OrganizationUserType.User)] [InlineData(OrganizationUserType.Custom, OrganizationUserType.Custom)] - public async Task CanManageAsync_WhenTargetRoleWithinAuthority_ReturnsNull( + public void CanManage_WhenTargetRoleWithinAuthority_ReturnsNull( OrganizationUserType actingRole, OrganizationUserType targetRole) { - var result = await _sut.CanManageAsync(_actingUserId, ActingUser(actingRole), TargetUser(targetRole)); + var result = _sut.CanManage(ActingUser(actingRole), TargetUser(targetRole)); Assert.Null(result); } @@ -74,16 +69,12 @@ public async Task CanManageAsync_WhenTargetRoleWithinAuthority_ReturnsNull( [InlineData(OrganizationUserType.Admin, OrganizationUserType.Owner, typeof(OnlyOwnersCanManageOwners))] [InlineData(OrganizationUserType.Custom, OrganizationUserType.Owner, typeof(OnlyOwnersCanManageOwners))] [InlineData(OrganizationUserType.Custom, OrganizationUserType.Admin, typeof(CustomUsersCannotManageAdminsOrOwners))] - public async Task CanManageAsync_WhenTargetRoleOutranksActingUser_ReturnsGranularError( + public void CanManage_WhenTargetRoleOutranksActingUser_ReturnsGranularError( OrganizationUserType actingRole, OrganizationUserType targetRole, Type expectedError) { - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([]); - - var result = await _sut.CanManageAsync(_actingUserId, ActingUser(actingRole), TargetUser(targetRole)); + var result = _sut.CanManage(ActingUser(actingRole), TargetUser(targetRole)); Assert.IsType(expectedError, result); } @@ -94,36 +85,28 @@ public async Task CanManageAsync_WhenTargetRoleOutranksActingUser_ReturnsGranula [InlineData(OrganizationUserType.Admin, typeof(CustomUsersCannotManageAdminsOrOwners))] [InlineData(OrganizationUserType.User, typeof(CustomUsersCannotManageAdminsOrOwners))] [InlineData(OrganizationUserType.Custom, typeof(CustomUsersCannotManageAdminsOrOwners))] - public async Task CanManageAsync_WhenActingUserIsRegularUser_ReturnsGranularError( + public void CanManage_WhenActingUserIsRegularUser_ReturnsGranularError( OrganizationUserType targetRole, Type expectedError) { - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([]); - - var result = await _sut.CanManageAsync(_actingUserId, ActingUser(OrganizationUserType.User), TargetUser(targetRole)); + var result = _sut.CanManage(ActingUser(OrganizationUserType.User), TargetUser(targetRole)); Assert.IsType(expectedError, result); } [Theory] - // A provider user has Owner-level authority and is not an organization member, so their membership is null and - // authority comes solely from provider status. - [InlineData(OrganizationUserType.Owner)] - [InlineData(OrganizationUserType.Admin)] - [InlineData(OrganizationUserType.User)] - [InlineData(OrganizationUserType.Custom)] - public async Task CanManageAsync_WhenActingUserIsProvider_ReturnsNullForAnyTargetRole( - OrganizationUserType targetRole) + // A non-member (null role) has no authority; provider authority is resolved upstream, not here. + [InlineData(OrganizationUserType.Owner, typeof(OnlyOwnersCanManageOwners))] + [InlineData(OrganizationUserType.Admin, typeof(CustomUsersCannotManageAdminsOrOwners))] + [InlineData(OrganizationUserType.User, typeof(CustomUsersCannotManageAdminsOrOwners))] + [InlineData(OrganizationUserType.Custom, typeof(CustomUsersCannotManageAdminsOrOwners))] + public void CanManage_WhenActingUserIsNonMember_ReturnsGranularError( + OrganizationUserType targetRole, + Type expectedError) { - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([new ProviderUserOrganizationDetails { OrganizationId = _organizationId }]); - - var result = await _sut.CanManageAsync(_actingUserId, actingUser: null, TargetUser(targetRole)); + var result = _sut.CanManage(actingUser: null, TargetUser(targetRole)); - Assert.Null(result); + Assert.IsType(expectedError, result); } [Theory] @@ -131,122 +114,163 @@ public async Task CanManageAsync_WhenActingUserIsProvider_ReturnsNullForAnyTarge // otherwise act on by rank. [InlineData(OrganizationUserType.User)] [InlineData(OrganizationUserType.Custom)] - public async Task CanManageAsync_WhenCustomUserLacksManageUsers_ReturnsCustomUsersCannotManageAdminsOrOwners( + public void CanManage_WhenCustomUserLacksManageUsers_ReturnsCustomUsersCannotManageAdminsOrOwners( OrganizationUserType targetRole) { - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([]); var actingUser = ActingUser(OrganizationUserType.Custom, manageUsers: false); - var result = await _sut.CanManageAsync(_actingUserId, actingUser, TargetUser(targetRole)); + var result = _sut.CanManage(actingUser, TargetUser(targetRole)); Assert.IsType(result); } [Fact] - public async Task CanManageAsync_WhenDemotingOwner_RejectsViaCurrentRole() + public void CanManage_WhenDemotingOwner_RejectsViaCurrentRole() { // An Admin demoting an Owner to User must be rejected. A member carrying the new role (User) is within // the Admin's authority, so escalation is only caught when the caller also checks the *current* member. - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([]); var admin = ActingUser(OrganizationUserType.Admin); - var currentRoleResult = await _sut.CanManageAsync(_actingUserId, admin, TargetUser(OrganizationUserType.Owner)); - var newRoleResult = await _sut.CanManageAsync(_actingUserId, admin, TargetUser(OrganizationUserType.User)); + var currentRoleResult = _sut.CanManage(admin, TargetUser(OrganizationUserType.Owner)); + var newRoleResult = _sut.CanManage(admin, TargetUser(OrganizationUserType.User)); Assert.IsType(currentRoleResult); Assert.Null(newRoleResult); } [Fact] - public async Task CanManageRoleChangeAsync_WhenActingUserCanManageBothRoles_ReturnsNull() + public void CanManageRoleChange_WhenActingUserCanManageBothRoles_ReturnsNull() { // An Admin promoting a User to Custom can manage both the current and new role. - var result = await _sut.CanManageRoleChangeAsync(_actingUserId, ActingUser(OrganizationUserType.Admin)!, + var result = _sut.CanManageRoleChange(ActingUser(OrganizationUserType.Admin)!, TargetUser(OrganizationUserType.User), NewRole(OrganizationUserType.Custom)); Assert.Null(result); } [Fact] - public async Task CanManageRoleChangeAsync_WhenDeniedAndNoOwnerInvolved_ReturnsCustomUsersCannotManageAdminsOrOwners() + public void CanManageRoleChange_WhenDeniedAndNoOwnerInvolved_ReturnsCustomUsersCannotManageAdminsOrOwners() { // A Custom user can't promote a User to Admin. Neither role is Owner, so the denial maps to the custom-user error. - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([]); - - var result = await _sut.CanManageRoleChangeAsync(_actingUserId, ActingUser(OrganizationUserType.Custom)!, + var result = _sut.CanManageRoleChange(ActingUser(OrganizationUserType.Custom)!, TargetUser(OrganizationUserType.User), NewRole(OrganizationUserType.Admin)); Assert.IsType(result); } [Fact] - public async Task CanManageRoleChangeAsync_WhenTargetIsOwner_ReturnsOnlyOwnersCanManageOwners() + public void CanManageRoleChange_WhenTargetIsOwner_ReturnsOnlyOwnersCanManageOwners() { // An Admin can't manage an Owner, so demoting one is rejected with the owner-specific error. - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([]); - - var result = await _sut.CanManageRoleChangeAsync(_actingUserId, ActingUser(OrganizationUserType.Admin)!, + var result = _sut.CanManageRoleChange(ActingUser(OrganizationUserType.Admin)!, TargetUser(OrganizationUserType.Owner), NewRole(OrganizationUserType.User)); Assert.IsType(result); } [Fact] - public async Task CanManageRoleChangeAsync_WhenPromotingToOwner_ReturnsOnlyOwnersCanManageOwners() + public void CanManageRoleChange_WhenPromotingToOwner_ReturnsOnlyOwnersCanManageOwners() { // An Admin can manage a User but can't promote them to Owner, so the new role maps to the owner-specific error. - _providerUserRepository - .GetManyOrganizationDetailsByUserAsync(_actingUserId, ProviderUserStatusType.Confirmed) - .Returns([]); - - var result = await _sut.CanManageRoleChangeAsync(_actingUserId, ActingUser(OrganizationUserType.Admin)!, + var result = _sut.CanManageRoleChange(ActingUser(OrganizationUserType.Admin)!, TargetUser(OrganizationUserType.User), NewRole(OrganizationUserType.Owner)); Assert.IsType(result); } [Fact] - public async Task CanManageRoleChangeAsync_WhenCustomActorGrantsPermissionTheyDoNotHold_ReturnsCustomUsersCanOnlyGrantOwnPermissions() + public void CanManageRoleChange_WhenCustomActorGrantsPermissionTheyDoNotHold_ReturnsCustomUsersCanOnlyGrantOwnPermissions() { // A Custom actor holding only ManageUsers can't grant ManageSso. var actingUser = CustomUser(new Permissions { ManageUsers = true }); - var result = await _sut.CanManageRoleChangeAsync(_actingUserId, actingUser, TargetUser(OrganizationUserType.Custom), + var result = _sut.CanManageRoleChange(actingUser, TargetUser(OrganizationUserType.Custom), NewRole(OrganizationUserType.Custom, new Permissions { ManageSso = true })); Assert.IsType(result); } [Fact] - public async Task CanManageRoleChangeAsync_WhenCustomActorGrantsPermissionsTheyHold_ReturnsNull() + public void CanManageRoleChange_WhenCustomActorGrantsPermissionsTheyHold_ReturnsNull() { var actingUser = CustomUser(new Permissions { ManageUsers = true }); - var result = await _sut.CanManageRoleChangeAsync(_actingUserId, actingUser, TargetUser(OrganizationUserType.Custom), + var result = _sut.CanManageRoleChange(actingUser, TargetUser(OrganizationUserType.Custom), NewRole(OrganizationUserType.Custom, new Permissions { ManageUsers = true })); Assert.Null(result); } [Fact] - public async Task CanManageRoleChangeAsync_WhenOwnerGrantsAnyPermission_ReturnsNull() + public void CanManageRoleChange_WhenOwnerGrantsAnyPermission_ReturnsNull() { // Owners are exempt from the grant-subset check. - var result = await _sut.CanManageRoleChangeAsync(_actingUserId, ActingUser(OrganizationUserType.Owner)!, + var result = _sut.CanManageRoleChange(ActingUser(OrganizationUserType.Owner)!, TargetUser(OrganizationUserType.Custom), NewRole(OrganizationUserType.Custom, new Permissions { ManageScim = true, ManageSso = true })); Assert.Null(result); } + [Fact] + public void CanManageRoleChange_ByActingUser_WhenPerformedBySystemUser_ReturnsNull() + { + // System users act outside the organization role hierarchy and skip the check. + var performedBy = new SystemUser(EventSystemUser.SCIM); + + var result = _sut.CanManageRoleChange(performedBy, TargetUser(OrganizationUserType.Owner), + NewRole(OrganizationUserType.User)); + + Assert.Null(result); + } + + [Fact] + public void CanManageRoleChange_ByActingUser_WhenPerformedByProvider_ActsWithOwnerAuthority() + { + // A managing provider member holds Owner-level authority, so it can promote a User to Owner. + var performedBy = new StandardUser(Guid.NewGuid(), isProvider: true); + + var result = _sut.CanManageRoleChange(performedBy, TargetUser(OrganizationUserType.User), + NewRole(OrganizationUserType.Owner)); + + Assert.Null(result); + } + + [Fact] + public void CanManageRoleChange_ByActingUser_WhenPerformedByProviderWhoIsAlsoMember_ActsWithOwnerAuthority() + { + // Provider authority takes precedence over the caller's own membership role. + var performedBy = new StandardUser(Guid.NewGuid(), isProvider: true, OrganizationUserType.Admin); + + var result = _sut.CanManageRoleChange(performedBy, TargetUser(OrganizationUserType.User), + NewRole(OrganizationUserType.Owner)); + + Assert.Null(result); + } + + [Fact] + public void CanManageRoleChange_ByActingUser_WhenPerformedByMember_UsesTheirRole() + { + // An Admin member can't promote a User to Owner. + var performedBy = new StandardUser(Guid.NewGuid(), isProvider: false, OrganizationUserType.Admin); + + var result = _sut.CanManageRoleChange(performedBy, TargetUser(OrganizationUserType.User), + NewRole(OrganizationUserType.Owner)); + + Assert.IsType(result); + } + + [Fact] + public void CanManageRoleChange_ByActingUser_WhenPerformedByNeitherMemberNorProvider_ReturnsActingUserMustBeMemberOrProvider() + { + var performedBy = new StandardUser(Guid.NewGuid(), isProvider: false); + + var result = _sut.CanManageRoleChange(performedBy, TargetUser(OrganizationUserType.User), + NewRole(OrganizationUserType.Admin)); + + Assert.IsType(result); + } + [Theory] [InlineData(PlanType.EnterpriseAnnually, OrganizationUserType.User, OrganizationUserType.Admin)] [InlineData(PlanType.Free, OrganizationUserType.User, OrganizationUserType.User)] diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs index 071d98aa4d7e..a7312e896247 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/RevokeNonCompliantOrganizationUserCommandTests.cs @@ -3,7 +3,6 @@ using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Requests; using Bit.Core.Enums; -using Bit.Core.Models.Data; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Core.Services; @@ -182,7 +181,5 @@ public class InvalidUser : IActingUser public Guid? UserId => Guid.Empty; public bool IsOrganizationOwnerOrProvider => false; public EventSystemUser? SystemUserType => null; - public Permissions? Permissions => null; - public OrganizationUserType? OrganizationUserType => null; } } 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 1b8fb46fd118..646e6670bc8b 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserValidatorTests.cs @@ -302,10 +302,10 @@ public async Task ValidateAsync_WhenRoleChangeIsDenied_ReturnsTheServiceError( // The escalation decision (and which error to return) lives in the validation service; the validator // just forwards whatever it returns. The mapping itself is covered by the service's own unit tests. var request = CreateRequest(sutProvider, orgUser, OrganizationUserType.Admin, - performedBy: new StandardUser(Guid.NewGuid(), isOrganizationOwner: false, OrganizationUserType.Custom)); + performedBy: new StandardUser(Guid.NewGuid(), isProvider: false, OrganizationUserType.Custom)); sutProvider.GetDependency() - .CanManageRoleChangeAsync(Arg.Any(), Arg.Any(), Arg.Any(), + .CanManageRoleChange(Arg.Any(), Arg.Any(), Arg.Any()) .Returns(new CustomUsersCannotManageAdminsOrOwners());