diff --git a/src/Api/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandler.cs b/src/Api/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandler.cs index 239148ab2532..eddc2732a7a1 100644 --- a/src/Api/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandler.cs +++ b/src/Api/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandler.cs @@ -1,4 +1,6 @@ using System.Security.Claims; +using Bit.Core.AdminConsole.Entities.Provider; +using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; using Bit.Core.Entities; @@ -21,7 +23,7 @@ public class RecoverAccountAuthorizationRequirement : IAuthorizationRequirement; /// /// /// This prevents privilege escalation by ensuring that a user cannot recover the account of -/// another user with a higher role or with provider membership. +/// another user with a higher role, in either the organization or a provider. /// public class RecoverAccountAuthorizationHandler( IOrganizationContext organizationContext, @@ -30,7 +32,7 @@ public class RecoverAccountAuthorizationHandler( : AuthorizationHandler { public const string FailureReason = "You are not permitted to recover this user's account."; - public const string ProviderFailureReason = "You are not permitted to recover a Provider member's account."; + public const string ProviderFailureReason = "You are not permitted to recover this Provider member's account."; protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, RecoverAccountAuthorizationRequirement requirement, @@ -49,7 +51,8 @@ protected override async Task HandleRequirementAsync(AuthorizationHandlerContext } // Step 2: check that the User has permissions with respect to any provider the target user is a member of. - // This prevents an organization admin performing privilege escalation into an unrelated provider. + // This prevents an organization admin performing privilege escalation into an unrelated provider, + // and prevents a provider member escalating into a higher role within their own provider. var canRecoverProviderMember = await CanRecoverProviderAsync(targetOrganizationUser); if (!canRecoverProviderMember) { @@ -99,11 +102,24 @@ private async Task CanRecoverProviderAsync(OrganizationUser targetOrganiza var targetUserProviderUsers = await providerUserRepository.GetManyByUserAsync(targetOrganizationUser.UserId.Value); - // If the target user belongs to any provider that the current user is not a member of, - // deny the action to prevent privilege escalation from organization to provider. + // The current user must be able to recover the target user in every provider the target belongs to. // Note: we do not expect that a user is a member of more than 1 provider, but there is also no guarantee // against it; this returns a sequence, so we handle the possibility. - var authorized = targetUserProviderUsers.All(providerUser => currentContext.ProviderUser(providerUser.ProviderId)); + var authorized = targetUserProviderUsers.All(AuthorizeProviderMember); + return authorized; + } + + private bool AuthorizeProviderMember(ProviderUser targetProviderUser) + { + // Current user must be a member of the provider with equal or greater permissions than the user + // account being recovered. Membership alone is not enough: a Service User recovering a Provider admin + // would escalate into the higher role. + var authorized = targetProviderUser.Type switch + { + ProviderUserType.ProviderAdmin => currentContext.ProviderProviderAdmin(targetProviderUser.ProviderId), + _ => currentContext.ProviderUser(targetProviderUser.ProviderId) + }; + return authorized; } } diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 230d62b1ba30..1cd6c39b7a76 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -261,6 +261,11 @@ public async Task GetResetPas throw new NotFoundException(); } + if (!await CanRecoverAccountAsync(organizationUser)) + { + throw new NotFoundException(); + } + // Retrieve data necessary for response (KDF, KDF Iterations, ResetPasswordKey) // TODO Reset Password - Revisit this and create SPROC to reduce DB calls var user = await _userService.GetUserByIdAsync(organizationUser.UserId.Value); @@ -276,10 +281,29 @@ public async Task GetResetPas [Authorize] public async Task> GetAccountRecoveryDetails(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { - var responses = await _organizationUserRepository.GetManyAccountRecoveryDetailsByOrganizationUserAsync(orgId, model.Ids); + var organizationUsers = await _organizationUserRepository.GetManyAsync(model.Ids); + var authorizedIds = new List(); + foreach (var organizationUser in organizationUsers.Where(organizationUser => organizationUser.OrganizationId == orgId)) + { + if (await CanRecoverAccountAsync(organizationUser)) + { + authorizedIds.Add(organizationUser.Id); + } + } + + if (authorizedIds.Count == 0) + { + return new ListResponseModel([]); + } + + var responses = await _organizationUserRepository.GetManyAccountRecoveryDetailsByOrganizationUserAsync(orgId, authorizedIds); return new ListResponseModel(responses.Select(r => new OrganizationUserResetPasswordDetailsResponseModel(r))); } + private async Task CanRecoverAccountAsync(OrganizationUser organizationUser) => + (await _authorizationService.AuthorizeAsync( + User, organizationUser, new RecoverAccountAuthorizationRequirement())).Succeeded; + [HttpPost("invite")] [Authorize] public async Task Invite(Guid orgId, [FromBody] OrganizationUserInviteRequestModel model) diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index 308c0897b28f..34c5669bf7f0 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -1,6 +1,7 @@ using System.Net; using Bit.Api.AdminConsole.Authorization; using Bit.Api.AdminConsole.Models.Request.Organizations; +using Bit.Api.AdminConsole.Models.Response.Organizations; using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; using Bit.Core.AdminConsole.Entities; @@ -70,6 +71,22 @@ public Task DisposeAsync() return Task.CompletedTask; } + /// + /// Minimal shape for reading the account recovery details response. + /// is response-only and has no + /// constructor that System.Text.Json can bind to. + /// + private sealed class AccountRecoveryDetails + { + public Guid OrganizationUserId { get; set; } + public string? ResetPasswordKey { get; set; } + } + + private sealed class AccountRecoveryDetailsList + { + public List Data { get; set; } = []; + } + /// /// Helper method to set the ResetPasswordKey on an organization user, which is required for account recovery /// @@ -268,4 +285,190 @@ await providerUserRepository.CreateAsync(new ProviderUser var model = await response.Content.ReadFromJsonAsync(); Assert.Equal(RecoverAccountAuthorizationHandler.ProviderFailureReason, model.Message); } + + [Fact] + public async Task RecoverAccount_AsFellowProviderMember_CannotRecoverProviderAdmin() + { + // Arrange - the caller and the target belong to the same provider. Shared membership is not enough + // on its own: the caller must also hold an equal or greater role, otherwise a Service User could + // take over a Provider admin. + var provider = await ProviderTestHelpers.CreateProviderAndLinkToOrganizationAsync( + _factory, _organization.Id, ProviderType.Msp); + + var (callerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Owner); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, callerEmail, + ProviderUserType.ServiceUser); + + var (targetEmail, targetOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, targetEmail, + ProviderUserType.ProviderAdmin); + await SetResetPasswordKeyAsync(targetOrgUser); + + // Log in only once the provider membership exists, so that it is present in the caller's claims + await _loginHelper.LoginAsync(callerEmail); + + var resetPasswordRequest = new OrganizationUserResetPasswordRequestModel + { + ResetMasterPassword = true, + NewMasterPasswordHash = "new-master-password-hash", + Key = "encrypted-recovery-key" + }; + + // Act + var response = await _client.PutAsJsonAsync( + $"organizations/{_organization.Id}/users/{targetOrgUser.Id}/recover-account", + resetPasswordRequest); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + var model = await response.Content.ReadFromJsonAsync(); + Assert.Equal(RecoverAccountAuthorizationHandler.ProviderFailureReason, model.Message); + } + + [Fact] + public async Task GetResetPasswordDetails_AsLowerRole_DoesNotDiscloseHigherRoleKeyMaterial() + { + // Arrange + var (adminEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Admin); + await _loginHelper.LoginAsync(adminEmail); + + var (_, targetOwnerOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.Owner); + await SetResetPasswordKeyAsync(targetOwnerOrgUser); + + // Act + var response = await _client.GetAsync( + $"organizations/{_organization.Id}/users/{targetOwnerOrgUser.Id}/reset-password-details"); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task GetResetPasswordDetails_ForProviderMemberOutsideCallersProviders_DoesNotDiscloseKeyMaterial() + { + // Arrange - the caller is an organization Owner with no membership of the target's provider + var (ownerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Owner); + await _loginHelper.LoginAsync(ownerEmail); + + var (targetEmail, targetOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await SetResetPasswordKeyAsync(targetOrgUser); + + var provider = await ProviderTestHelpers.CreateProviderAndLinkToOrganizationAsync( + _factory, _organization.Id, ProviderType.Msp); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, targetEmail, + ProviderUserType.ProviderAdmin); + + // Act + var response = await _client.GetAsync( + $"organizations/{_organization.Id}/users/{targetOrgUser.Id}/reset-password-details"); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task GetResetPasswordDetails_AsFellowProviderMember_DoesNotDiscloseProviderAdminKeyMaterial() + { + // Arrange - a Service User must not be able to read the key material of a Provider admin in their + // own provider. Reading it is equivalent to the recovery itself, so it is denied on the same terms. + var provider = await ProviderTestHelpers.CreateProviderAndLinkToOrganizationAsync( + _factory, _organization.Id, ProviderType.Msp); + + var (callerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Owner); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, callerEmail, + ProviderUserType.ServiceUser); + + var (targetEmail, targetOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, targetEmail, + ProviderUserType.ProviderAdmin); + await SetResetPasswordKeyAsync(targetOrgUser); + + await _loginHelper.LoginAsync(callerEmail); + + // Act + var response = await _client.GetAsync( + $"organizations/{_organization.Id}/users/{targetOrgUser.Id}/reset-password-details"); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task GetAccountRecoveryDetails_AsFellowProviderMember_OmitsProviderAdmin() + { + // Arrange + var provider = await ProviderTestHelpers.CreateProviderAndLinkToOrganizationAsync( + _factory, _organization.Id, ProviderType.Msp); + + var (callerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Owner); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, callerEmail, + ProviderUserType.ServiceUser); + + var (providerAdminEmail, providerAdminOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await ProviderTestHelpers.CreateProviderUserAsync(_factory, provider.Id, providerAdminEmail, + ProviderUserType.ProviderAdmin); + await SetResetPasswordKeyAsync(providerAdminOrgUser); + + var (_, memberOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await SetResetPasswordKeyAsync(memberOrgUser); + + await _loginHelper.LoginAsync(callerEmail); + + var request = new OrganizationUserBulkRequestModel { Ids = [providerAdminOrgUser.Id, memberOrgUser.Id] }; + + // Act + var response = await _client.PostAsJsonAsync( + $"organizations/{_organization.Id}/users/account-recovery-details", request); + + // Assert - the Provider admin is dropped, the ordinary member is still returned + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var result = await response.Content + .ReadFromJsonAsync(); + var details = Assert.Single(result.Data); + Assert.Equal(memberOrgUser.Id, details.OrganizationUserId); + // The filtering is what withholds the key material, so confirm it is really being returned + // for the user who is allowed through + Assert.Equal("encrypted-reset-password-key", details.ResetPasswordKey); + } + + [Fact] + public async Task GetAccountRecoveryDetails_OmitsUsersTheCallerCannotRecover() + { + // Arrange + var (adminEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Admin); + await _loginHelper.LoginAsync(adminEmail); + + var (_, ownerOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.Owner); + await SetResetPasswordKeyAsync(ownerOrgUser); + + var (_, memberOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await SetResetPasswordKeyAsync(memberOrgUser); + + var request = new OrganizationUserBulkRequestModel { Ids = [ownerOrgUser.Id, memberOrgUser.Id] }; + + // Act + var response = await _client.PostAsJsonAsync( + $"organizations/{_organization.Id}/users/account-recovery-details", request); + + // Assert - an Admin may recover the member but not the Owner + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var result = await response.Content + .ReadFromJsonAsync(); + var details = Assert.Single(result.Data); + Assert.Equal(memberOrgUser.Id, details.OrganizationUserId); + } } diff --git a/test/Api.Test/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandlerTests.cs b/test/Api.Test/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandlerTests.cs index 92efb641f19b..34dfa69cd768 100644 --- a/test/Api.Test/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandlerTests.cs +++ b/test/Api.Test/AdminConsole/Authorization/Organizations/Handlers/RecoverAccountAuthorizationHandlerTests.cs @@ -1,6 +1,7 @@ using System.Security.Claims; using Bit.Api.AdminConsole.Authorization; using Bit.Core.AdminConsole.Entities.Provider; +using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; using Bit.Core.Entities; @@ -32,6 +33,7 @@ [new RecoverAccountAuthorizationRequirement()], MockOrganizationClaims(sutProvider, claimsPrincipal, targetOrganizationUser, null); MockCurrentUserIsProvider(sutProvider, claimsPrincipal, targetOrganizationUser); + MockTargetUserProviders(sutProvider, targetOrganizationUser, []); // Act await sutProvider.Sut.HandleAsync(context); @@ -94,6 +96,7 @@ [new RecoverAccountAuthorizationRequirement()], targetOrganizationUser); MockOrganizationClaims(sutProvider, claimsPrincipal, targetOrganizationUser, currentContextOrganization); + MockTargetUserProviders(sutProvider, targetOrganizationUser, []); // Act await sutProvider.Sut.HandleAsync(context); @@ -173,6 +176,83 @@ await sutProvider.GetDependency().DidNotReceiveWithAnyA .GetManyByUserAsync(Arg.Any()); } + // Pairing of the current user's provider role and the target user's provider role, in the same provider. + // Read this as: a ___ can recover the account for a ___ + public static IEnumerable AuthorizedProviderRoleCombinations => new object[][] + { + [ProviderUserType.ProviderAdmin, ProviderUserType.ProviderAdmin], + [ProviderUserType.ProviderAdmin, ProviderUserType.ServiceUser], + [ProviderUserType.ServiceUser, ProviderUserType.ServiceUser], + }; + + [Theory, BitMemberAutoData(nameof(AuthorizedProviderRoleCombinations))] + public async Task CanRecoverProviderAsync_RecoverEqualOrLesserProviderRoles_Authorized( + ProviderUserType currentUserProviderType, + ProviderUserType targetProviderUserType, + SutProvider sutProvider, + [OrganizationUser] OrganizationUser targetOrganizationUser, + ClaimsPrincipal claimsPrincipal, + Guid providerId) + { + // Arrange + var context = new AuthorizationHandlerContext( + [new RecoverAccountAuthorizationRequirement()], + claimsPrincipal, + targetOrganizationUser); + + MockCurrentUserIsProvider(sutProvider, claimsPrincipal, targetOrganizationUser); + MockCurrentUserProviderRole(sutProvider, providerId, currentUserProviderType); + MockTargetUserProviders(sutProvider, targetOrganizationUser, + [new ProviderUser { ProviderId = providerId, UserId = targetOrganizationUser.UserId, Type = targetProviderUserType }]); + + // Act + await sutProvider.Sut.HandleAsync(context); + + // Assert + Assert.True(context.HasSucceeded); + } + + // Pairing of the current user's provider role and the target user's provider role, in the same provider. + // Read this as: a ___ cannot recover the account for a ___ + // A null current user role means they are not a member of the target's provider at all. + public static IEnumerable UnauthorizedProviderRoleCombinations => new object[][] + { + // A Service User cannot escalate into the higher provider role + [ProviderUserType.ServiceUser, ProviderUserType.ProviderAdmin], + + // A non-member of the provider cannot recover any of its members + [null!, ProviderUserType.ProviderAdmin], + [null!, ProviderUserType.ServiceUser], + }; + + [Theory, BitMemberAutoData(nameof(UnauthorizedProviderRoleCombinations))] + public async Task CanRecoverProviderAsync_InvalidProviderRoles_Unauthorized( + ProviderUserType? currentUserProviderType, + ProviderUserType targetProviderUserType, + SutProvider sutProvider, + [OrganizationUser] OrganizationUser targetOrganizationUser, + ClaimsPrincipal claimsPrincipal, + Guid providerId) + { + // Arrange + var context = new AuthorizationHandlerContext( + [new RecoverAccountAuthorizationRequirement()], + claimsPrincipal, + targetOrganizationUser); + + // The current user clears step 1 as an organization Owner, so only the provider check can fail + MockCurrentUserIsOwner(sutProvider, claimsPrincipal, targetOrganizationUser); + MockCurrentUserProviderRole(sutProvider, providerId, currentUserProviderType); + MockTargetUserProviders(sutProvider, targetOrganizationUser, + [new ProviderUser { ProviderId = providerId, UserId = targetOrganizationUser.UserId, Type = targetProviderUserType }]); + + // Act + await sutProvider.Sut.HandleAsync(context); + + // Assert + AssertFailed(context, RecoverAccountAuthorizationHandler.ProviderFailureReason); + } + [Theory, BitAutoData] public async Task HandleRequirementAsync_CurrentUserIsMemberOfAllTargetUserProviders_DoesNotBlock( SutProvider sutProvider, @@ -182,30 +262,19 @@ public async Task HandleRequirementAsync_CurrentUserIsMemberOfAllTargetUserProvi Guid providerId2) { // Arrange - var targetUserProviders = new List - { - new() { ProviderId = providerId1, UserId = targetOrganizationUser.UserId }, - new() { ProviderId = providerId2, UserId = targetOrganizationUser.UserId } - }; - var context = new AuthorizationHandlerContext( [new RecoverAccountAuthorizationRequirement()], claimsPrincipal, targetOrganizationUser); MockCurrentUserIsProvider(sutProvider, claimsPrincipal, targetOrganizationUser); - - sutProvider.GetDependency() - .GetManyByUserAsync(targetOrganizationUser.UserId!.Value) - .Returns(targetUserProviders); - - sutProvider.GetDependency() - .ProviderUser(providerId1) - .Returns(true); - - sutProvider.GetDependency() - .ProviderUser(providerId2) - .Returns(true); + MockCurrentUserProviderRole(sutProvider, providerId1, ProviderUserType.ProviderAdmin); + MockCurrentUserProviderRole(sutProvider, providerId2, ProviderUserType.ProviderAdmin); + MockTargetUserProviders(sutProvider, targetOrganizationUser, + [ + new ProviderUser { ProviderId = providerId1, UserId = targetOrganizationUser.UserId, Type = ProviderUserType.ServiceUser }, + new ProviderUser { ProviderId = providerId2, UserId = targetOrganizationUser.UserId, Type = ProviderUserType.ProviderAdmin } + ]); // Act await sutProvider.Sut.HandleAsync(context); @@ -223,37 +292,72 @@ public async Task HandleRequirementAsync_CurrentUserMissingProviderMembership_Bl Guid providerId2) { // Arrange - var targetUserProviders = new List - { - new() { ProviderId = providerId1, UserId = targetOrganizationUser.UserId }, - new() { ProviderId = providerId2, UserId = targetOrganizationUser.UserId } - }; + var context = new AuthorizationHandlerContext( + [new RecoverAccountAuthorizationRequirement()], + claimsPrincipal, + targetOrganizationUser); + + MockCurrentUserIsOwner(sutProvider, claimsPrincipal, targetOrganizationUser); + MockCurrentUserProviderRole(sutProvider, providerId1, ProviderUserType.ProviderAdmin); + // Not a member of this provider + MockCurrentUserProviderRole(sutProvider, providerId2, null); + MockTargetUserProviders(sutProvider, targetOrganizationUser, + [ + new ProviderUser { ProviderId = providerId1, UserId = targetOrganizationUser.UserId, Type = ProviderUserType.ServiceUser }, + new ProviderUser { ProviderId = providerId2, UserId = targetOrganizationUser.UserId, Type = ProviderUserType.ServiceUser } + ]); + + // Act + await sutProvider.Sut.HandleAsync(context); + + // Assert + AssertFailed(context, RecoverAccountAuthorizationHandler.ProviderFailureReason); + } + [Theory, BitAutoData] + public async Task HandleRequirementAsync_TargetUserHasNoProviders_DoesNotBlock( + SutProvider sutProvider, + [OrganizationUser] OrganizationUser targetOrganizationUser, + ClaimsPrincipal claimsPrincipal) + { + // Arrange var context = new AuthorizationHandlerContext( [new RecoverAccountAuthorizationRequirement()], claimsPrincipal, targetOrganizationUser); MockCurrentUserIsOwner(sutProvider, claimsPrincipal, targetOrganizationUser); + MockTargetUserProviders(sutProvider, targetOrganizationUser, []); + // Act + await sutProvider.Sut.HandleAsync(context); + + // Assert + Assert.True(context.HasSucceeded); + } + + private static void MockTargetUserProviders(SutProvider sutProvider, + OrganizationUser targetOrganizationUser, List targetUserProviders) + { sutProvider.GetDependency() .GetManyByUserAsync(targetOrganizationUser.UserId!.Value) .Returns(targetUserProviders); + } + /// + /// Mocks the current user's role in the specified provider. A null + /// means they are not a member of that provider. + /// + private static void MockCurrentUserProviderRole(SutProvider sutProvider, + Guid providerId, ProviderUserType? providerUserType) + { sutProvider.GetDependency() - .ProviderUser(providerId1) - .Returns(true); + .ProviderUser(providerId) + .Returns(providerUserType is not null); - // Not a member of this provider sutProvider.GetDependency() - .ProviderUser(providerId2) - .Returns(false); - - // Act - await sutProvider.Sut.HandleAsync(context); - - // Assert - AssertFailed(context, RecoverAccountAuthorizationHandler.ProviderFailureReason); + .ProviderProviderAdmin(providerId) + .Returns(providerUserType is ProviderUserType.ProviderAdmin); } private static void MockOrganizationClaims(SutProvider sutProvider, diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index 95cc7f2077ec..bfcb7649eb38 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -326,9 +326,15 @@ public async Task GetAccountRecoveryDetails_ReturnsDetails( ICollection resetPasswordDetails, SutProvider sutProvider) { - sutProvider.GetDependency().ManageResetPassword(organizationId).Returns(true); + var organizationUsers = MockAccountRecoveryCandidates(sutProvider, organizationId, bulkRequestModel); + foreach (var organizationUser in organizationUsers) + { + MockCanRecoverAccount(sutProvider, organizationUser, true); + } + sutProvider.GetDependency() - .GetManyAccountRecoveryDetailsByOrganizationUserAsync(organizationId, bulkRequestModel.Ids) + .GetManyAccountRecoveryDetailsByOrganizationUserAsync(organizationId, + Arg.Is>(ids => ids.SequenceEqual(organizationUsers.Select(ou => ou.Id)))) .Returns(resetPasswordDetails); var response = await sutProvider.Sut.GetAccountRecoveryDetails(organizationId, bulkRequestModel); @@ -346,6 +352,76 @@ public async Task GetAccountRecoveryDetails_ReturnsDetails( ou.MasterPasswordSalt == r.MasterPasswordSalt))); } + [Theory] + [BitAutoData] + public async Task GetAccountRecoveryDetails_OmitsUsersTheCallerCannotRecover( + Guid organizationId, + OrganizationUserBulkRequestModel bulkRequestModel, + SutProvider sutProvider) + { + // Arrange: the caller may recover the first user but not the rest + var organizationUsers = MockAccountRecoveryCandidates(sutProvider, organizationId, bulkRequestModel); + foreach (var organizationUser in organizationUsers) + { + MockCanRecoverAccount(sutProvider, organizationUser, organizationUser == organizationUsers[0]); + } + + // Act + await sutProvider.Sut.GetAccountRecoveryDetails(organizationId, bulkRequestModel); + + // Assert: only the authorized user's id reaches the query that returns key material + await sutProvider.GetDependency().Received(1) + .GetManyAccountRecoveryDetailsByOrganizationUserAsync(organizationId, + Arg.Is>(ids => ids.SequenceEqual(new[] { organizationUsers[0].Id }))); + } + + [Theory] + [BitAutoData] + public async Task GetAccountRecoveryDetails_WhenNoUsersAuthorized_ReturnsEmptyWithoutQueryingDetails( + Guid organizationId, + OrganizationUserBulkRequestModel bulkRequestModel, + SutProvider sutProvider) + { + // Arrange + var organizationUsers = MockAccountRecoveryCandidates(sutProvider, organizationId, bulkRequestModel); + foreach (var organizationUser in organizationUsers) + { + MockCanRecoverAccount(sutProvider, organizationUser, false); + } + + // Act + var response = await sutProvider.Sut.GetAccountRecoveryDetails(organizationId, bulkRequestModel); + + // Assert + Assert.Empty(response.Data); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .GetManyAccountRecoveryDetailsByOrganizationUserAsync(default, default); + } + + [Theory] + [BitAutoData] + public async Task GetAccountRecoveryDetails_OmitsUsersFromAnotherOrganization( + Guid organizationId, + OrganizationUserBulkRequestModel bulkRequestModel, + SutProvider sutProvider) + { + // Arrange: every user is authorized, but they belong to a different organization than the route + var organizationUsers = MockAccountRecoveryCandidates(sutProvider, organizationId, bulkRequestModel); + foreach (var organizationUser in organizationUsers) + { + organizationUser.OrganizationId = Guid.NewGuid(); + MockCanRecoverAccount(sutProvider, organizationUser, true); + } + + // Act + var response = await sutProvider.Sut.GetAccountRecoveryDetails(organizationId, bulkRequestModel); + + // Assert + Assert.Empty(response.Data); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .AuthorizeAsync(default, default, default(IEnumerable)); + } + [Theory] [BitAutoData] public async Task GetResetPasswordDetails_WhenOrganizationUserNotFound_ThrowsNotFound( @@ -404,6 +480,7 @@ public async Task GetResetPasswordDetails_WhenValid_ReturnsDetails( organizationUser.UserId = user.Id; sutProvider.GetDependency().GetByIdAsync(orgUserId).Returns(organizationUser); sutProvider.GetDependency().GetUserByIdAsync(user.Id).Returns(user); + MockCanRecoverAccount(sutProvider, organizationUser, true); // Act — org is passed directly via [BindOrganization]; the repository is no longer called var response = await sutProvider.Sut.GetResetPasswordDetails(orgUserId, org); @@ -416,6 +493,54 @@ public async Task GetResetPasswordDetails_WhenValid_ReturnsDetails( Assert.Equal(user.MasterPasswordSalt, response.MasterPasswordSalt); } + [Theory] + [BitAutoData] + public async Task GetResetPasswordDetails_WhenCallerCannotRecoverTargetUser_ThrowsNotFound( + Guid orgId, Guid orgUserId, OrganizationUser organizationUser, User user, Organization org, + SutProvider sutProvider) + { + // Arrange: the caller passes the ManageAccountRecovery check but is not permitted to recover + // this particular user, e.g. an Admin targeting an Owner, or anyone targeting a Provider member. + org.Id = orgId; + organizationUser.OrganizationId = org.Id; + organizationUser.UserId = user.Id; + sutProvider.GetDependency().GetByIdAsync(orgUserId).Returns(organizationUser); + sutProvider.GetDependency().GetUserByIdAsync(user.Id).Returns(user); + MockCanRecoverAccount(sutProvider, organizationUser, false); + + // Act & Assert + await Assert.ThrowsAsync(() => sutProvider.Sut.GetResetPasswordDetails(orgUserId, org)); + + // The target user's key material must not be read at all + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetUserByIdAsync(default(Guid)); + } + + private static void MockCanRecoverAccount(SutProvider sutProvider, + OrganizationUser organizationUser, bool authorized) + { + sutProvider.GetDependency() + .AuthorizeAsync( + Arg.Any(), + organizationUser, + Arg.Is>(x => x.SingleOrDefault() is RecoverAccountAuthorizationRequirement)) + .Returns(authorized ? AuthorizationResult.Success() : AuthorizationResult.Failed()); + } + + private static List MockAccountRecoveryCandidates( + SutProvider sutProvider, Guid organizationId, + OrganizationUserBulkRequestModel bulkRequestModel) + { + var organizationUsers = bulkRequestModel.Ids + .Select(id => new OrganizationUser { Id = id, OrganizationId = organizationId, UserId = Guid.NewGuid() }) + .ToList(); + + sutProvider.GetDependency() + .GetManyAsync(bulkRequestModel.Ids) + .Returns(organizationUsers); + + return organizationUsers; + } + [Theory] [BitAutoData] public async Task DeleteAccount_WhenCurrentUserNotFound_ReturnsUnauthorizedResult(