From 60672bbe4839837465a4678f114601c2071ad5e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Mon, 4 Nov 2024 14:48:13 +0000 Subject: [PATCH] [PM-10323] Remove user verification from organization user deletion methods (#4965) --- .../OrganizationUsersController.cs | 17 +---- .../SecureOrganizationUserBulkRequestModel.cs | 10 --- .../OrganizationUsersControllerTests.cs | 71 +++---------------- 3 files changed, 12 insertions(+), 86 deletions(-) delete mode 100644 src/Api/AdminConsole/Models/Request/Organizations/SecureOrganizationUserBulkRequestModel.cs diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 89a8627e9..b81cb068f 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -1,6 +1,5 @@ using Bit.Api.AdminConsole.Models.Request.Organizations; using Bit.Api.AdminConsole.Models.Response.Organizations; -using Bit.Api.Auth.Models.Request.Accounts; using Bit.Api.Models.Request.Organizations; using Bit.Api.Models.Response; using Bit.Api.Vault.AuthorizationHandlers.Collections; @@ -545,7 +544,7 @@ public class OrganizationUsersController : Controller [RequireFeature(FeatureFlagKeys.AccountDeprovisioning)] [HttpDelete("{id}/delete-account")] [HttpPost("{id}/delete-account")] - public async Task DeleteAccount(Guid orgId, Guid id, [FromBody] SecretVerificationRequestModel model) + public async Task DeleteAccount(Guid orgId, Guid id) { if (!await _currentContext.ManageUsers(orgId)) { @@ -558,19 +557,13 @@ public class OrganizationUsersController : Controller throw new UnauthorizedAccessException(); } - if (!await _userService.VerifySecretAsync(currentUser, model.Secret)) - { - await Task.Delay(2000); - throw new BadRequestException(string.Empty, "User verification failed."); - } - await _deleteManagedOrganizationUserAccountCommand.DeleteUserAsync(orgId, id, currentUser.Id); } [RequireFeature(FeatureFlagKeys.AccountDeprovisioning)] [HttpDelete("delete-account")] [HttpPost("delete-account")] - public async Task> BulkDeleteAccount(Guid orgId, [FromBody] SecureOrganizationUserBulkRequestModel model) + public async Task> BulkDeleteAccount(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model) { if (!await _currentContext.ManageUsers(orgId)) { @@ -583,12 +576,6 @@ public class OrganizationUsersController : Controller throw new UnauthorizedAccessException(); } - if (!await _userService.VerifySecretAsync(currentUser, model.Secret)) - { - await Task.Delay(2000); - throw new BadRequestException(string.Empty, "User verification failed."); - } - var results = await _deleteManagedOrganizationUserAccountCommand.DeleteManyUsersAsync(orgId, model.Ids, currentUser.Id); return new ListResponseModel(results.Select(r => diff --git a/src/Api/AdminConsole/Models/Request/Organizations/SecureOrganizationUserBulkRequestModel.cs b/src/Api/AdminConsole/Models/Request/Organizations/SecureOrganizationUserBulkRequestModel.cs deleted file mode 100644 index f8edb08ba..000000000 --- a/src/Api/AdminConsole/Models/Request/Organizations/SecureOrganizationUserBulkRequestModel.cs +++ /dev/null @@ -1,10 +0,0 @@ -using System.ComponentModel.DataAnnotations; -using Bit.Api.Auth.Models.Request.Accounts; - -namespace Bit.Api.AdminConsole.Models.Request.Organizations; - -public class SecureOrganizationUserBulkRequestModel : SecretVerificationRequestModel -{ - [Required] - public IEnumerable Ids { get; set; } -} diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index 2ff5c0cb4..0ba8a101d 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -1,7 +1,6 @@ using System.Security.Claims; using Bit.Api.AdminConsole.Controllers; using Bit.Api.AdminConsole.Models.Request.Organizations; -using Bit.Api.Auth.Models.Request.Accounts; using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core; using Bit.Core.AdminConsole.Entities; @@ -273,17 +272,12 @@ public class OrganizationUsersControllerTests [Theory] [BitAutoData] public async Task DeleteAccount_WhenUserCanManageUsers_Success( - Guid orgId, - Guid id, - SecretVerificationRequestModel model, - User currentUser, - SutProvider sutProvider) + Guid orgId, Guid id, User currentUser, SutProvider sutProvider) { sutProvider.GetDependency().ManageUsers(orgId).Returns(true); sutProvider.GetDependency().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser); - sutProvider.GetDependency().VerifySecretAsync(currentUser, model.Secret).Returns(true); - await sutProvider.Sut.DeleteAccount(orgId, id, model); + await sutProvider.Sut.DeleteAccount(orgId, id); await sutProvider.GetDependency() .Received(1) @@ -293,60 +287,34 @@ public class OrganizationUsersControllerTests [Theory] [BitAutoData] public async Task DeleteAccount_WhenUserCannotManageUsers_ThrowsNotFoundException( - Guid orgId, - Guid id, - SecretVerificationRequestModel model, - SutProvider sutProvider) + Guid orgId, Guid id, SutProvider sutProvider) { sutProvider.GetDependency().ManageUsers(orgId).Returns(false); await Assert.ThrowsAsync(() => - sutProvider.Sut.DeleteAccount(orgId, id, model)); + sutProvider.Sut.DeleteAccount(orgId, id)); } [Theory] [BitAutoData] public async Task DeleteAccount_WhenCurrentUserNotFound_ThrowsUnauthorizedAccessException( - Guid orgId, - Guid id, - SecretVerificationRequestModel model, - SutProvider sutProvider) + Guid orgId, Guid id, SutProvider sutProvider) { sutProvider.GetDependency().ManageUsers(orgId).Returns(true); sutProvider.GetDependency().GetUserByPrincipalAsync(default).ReturnsForAnyArgs((User)null); await Assert.ThrowsAsync(() => - sutProvider.Sut.DeleteAccount(orgId, id, model)); - } - - [Theory] - [BitAutoData] - public async Task DeleteAccount_WhenSecretVerificationFails_ThrowsBadRequestException( - Guid orgId, - Guid id, - SecretVerificationRequestModel model, - User currentUser, - SutProvider sutProvider) - { - sutProvider.GetDependency().ManageUsers(orgId).Returns(true); - sutProvider.GetDependency().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser); - sutProvider.GetDependency().VerifySecretAsync(currentUser, model.Secret).Returns(false); - - await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteAccount(orgId, id, model)); + sutProvider.Sut.DeleteAccount(orgId, id)); } [Theory] [BitAutoData] public async Task BulkDeleteAccount_WhenUserCanManageUsers_Success( - Guid orgId, - SecureOrganizationUserBulkRequestModel model, - User currentUser, - List<(Guid, string)> deleteResults, - SutProvider sutProvider) + Guid orgId, OrganizationUserBulkRequestModel model, User currentUser, + List<(Guid, string)> deleteResults, SutProvider sutProvider) { sutProvider.GetDependency().ManageUsers(orgId).Returns(true); sutProvider.GetDependency().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser); - sutProvider.GetDependency().VerifySecretAsync(currentUser, model.Secret).Returns(true); sutProvider.GetDependency() .DeleteManyUsersAsync(orgId, model.Ids, currentUser.Id) .Returns(deleteResults); @@ -363,9 +331,7 @@ public class OrganizationUsersControllerTests [Theory] [BitAutoData] public async Task BulkDeleteAccount_WhenUserCannotManageUsers_ThrowsNotFoundException( - Guid orgId, - SecureOrganizationUserBulkRequestModel model, - SutProvider sutProvider) + Guid orgId, OrganizationUserBulkRequestModel model, SutProvider sutProvider) { sutProvider.GetDependency().ManageUsers(orgId).Returns(false); @@ -376,9 +342,7 @@ public class OrganizationUsersControllerTests [Theory] [BitAutoData] public async Task BulkDeleteAccount_WhenCurrentUserNotFound_ThrowsUnauthorizedAccessException( - Guid orgId, - SecureOrganizationUserBulkRequestModel model, - SutProvider sutProvider) + Guid orgId, OrganizationUserBulkRequestModel model, SutProvider sutProvider) { sutProvider.GetDependency().ManageUsers(orgId).Returns(true); sutProvider.GetDependency().GetUserByPrincipalAsync(default).ReturnsForAnyArgs((User)null); @@ -387,21 +351,6 @@ public class OrganizationUsersControllerTests sutProvider.Sut.BulkDeleteAccount(orgId, model)); } - [Theory] - [BitAutoData] - public async Task BulkDeleteAccount_WhenSecretVerificationFails_ThrowsBadRequestException( - Guid orgId, - SecureOrganizationUserBulkRequestModel model, - User currentUser, - SutProvider sutProvider) - { - sutProvider.GetDependency().ManageUsers(orgId).Returns(true); - sutProvider.GetDependency().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser); - sutProvider.GetDependency().VerifySecretAsync(currentUser, model.Secret).Returns(false); - - await Assert.ThrowsAsync(() => sutProvider.Sut.BulkDeleteAccount(orgId, model)); - } - private void GetMany_Setup(OrganizationAbility organizationAbility, ICollection organizationUsers, SutProvider sutProvider)