Remove user verification in DeleteAccount endpoints

bitwarden · Oct 25, 2024 · 9abc95b · 9abc95b
1 parent 53ad9df
commit 9abc95b
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 73 deletions.
diff --git 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 async Task<ListResponseModel<OrganizationUserBulkResponseModel>> BulkRemo
     [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 async Task DeleteAccount(Guid orgId, Guid id, [FromBody] SecretVerificati
             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<ListResponseModel<OrganizationUserBulkResponseModel>> BulkDeleteAccount(Guid orgId, [FromBody] SecureOrganizationUserBulkRequestModel model)
+    public async Task<ListResponseModel<OrganizationUserBulkResponseModel>> BulkDeleteAccount(Guid orgId, [FromBody] OrganizationUserBulkRequestModel model)
     {
         if (!await _currentContext.ManageUsers(orgId))
         {
@@ -583,12 +576,6 @@ public async Task<ListResponseModel<OrganizationUserBulkResponseModel>> BulkDele
             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<OrganizationUserBulkResponseModel>(results.Select(r =>

diff --git a/src/Api/AdminConsole/Models/Request/Organizations/SecureOrganizationUserBulkRequestModel.cs b/src/Api/AdminConsole/Models/Request/Organizations/SecureOrganizationUserBulkRequestModel.cs
diff --git 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 async Task GetAccountRecoveryDetails_WithoutManageResetPasswordPermission
     [Theory]
     [BitAutoData]
     public async Task DeleteAccount_WhenUserCanManageUsers_Success(
-        Guid orgId,
-        Guid id,
-        SecretVerificationRequestModel model,
-        User currentUser,
-        SutProvider<OrganizationUsersController> sutProvider)
+        Guid orgId, Guid id, User currentUser, SutProvider<OrganizationUsersController> sutProvider)
     {
         sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(true);
         sutProvider.GetDependency<IUserService>().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser);
-        sutProvider.GetDependency<IUserService>().VerifySecretAsync(currentUser, model.Secret).Returns(true);
 
-        await sutProvider.Sut.DeleteAccount(orgId, id, model);
+        await sutProvider.Sut.DeleteAccount(orgId, id);
 
         await sutProvider.GetDependency<IDeleteManagedOrganizationUserAccountCommand>()
             .Received(1)
@@ -293,60 +287,35 @@ await sutProvider.GetDependency<IDeleteManagedOrganizationUserAccountCommand>()
     [Theory]
     [BitAutoData]
     public async Task DeleteAccount_WhenUserCannotManageUsers_ThrowsNotFoundException(
-        Guid orgId,
-        Guid id,
-        SecretVerificationRequestModel model,
-        SutProvider<OrganizationUsersController> sutProvider)
+        Guid orgId, Guid id, SutProvider<OrganizationUsersController> sutProvider)
     {
         sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(false);
 
-        await Assert.ThrowsAsync<NotFoundException>(() =>
-            sutProvider.Sut.DeleteAccount(orgId, id, model));
+        await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.DeleteAccount(orgId, id));
     }
 
     [Theory]
     [BitAutoData]
     public async Task DeleteAccount_WhenCurrentUserNotFound_ThrowsUnauthorizedAccessException(
-        Guid orgId,
-        Guid id,
-        SecretVerificationRequestModel model,
-        SutProvider<OrganizationUsersController> sutProvider)
+        Guid orgId, Guid id, SutProvider<OrganizationUsersController> sutProvider)
     {
         sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(true);
         sutProvider.GetDependency<IUserService>().GetUserByPrincipalAsync(default).ReturnsForAnyArgs((User)null);
 
-        await Assert.ThrowsAsync<UnauthorizedAccessException>(() =>
-            sutProvider.Sut.DeleteAccount(orgId, id, model));
-    }
-
-    [Theory]
-    [BitAutoData]
-    public async Task DeleteAccount_WhenSecretVerificationFails_ThrowsBadRequestException(
-        Guid orgId,
-        Guid id,
-        SecretVerificationRequestModel model,
-        User currentUser,
-        SutProvider<OrganizationUsersController> sutProvider)
-    {
-        sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(true);
-        sutProvider.GetDependency<IUserService>().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser);
-        sutProvider.GetDependency<IUserService>().VerifySecretAsync(currentUser, model.Secret).Returns(false);
-
-        await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.DeleteAccount(orgId, id, model));
+        await Assert.ThrowsAsync<UnauthorizedAccessException>(() => sutProvider.Sut.DeleteAccount(orgId, id));
     }
 
     [Theory]
     [BitAutoData]
     public async Task BulkDeleteAccount_WhenUserCanManageUsers_Success(
         Guid orgId,
-        SecureOrganizationUserBulkRequestModel model,
+        OrganizationUserBulkRequestModel model,
         User currentUser,
         List<(Guid, string)> deleteResults,
         SutProvider<OrganizationUsersController> sutProvider)
     {
         sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(true);
         sutProvider.GetDependency<IUserService>().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser);
-        sutProvider.GetDependency<IUserService>().VerifySecretAsync(currentUser, model.Secret).Returns(true);
         sutProvider.GetDependency<IDeleteManagedOrganizationUserAccountCommand>()
             .DeleteManyUsersAsync(orgId, model.Ids, currentUser.Id)
             .Returns(deleteResults);
@@ -364,21 +333,18 @@ await sutProvider.GetDependency<IDeleteManagedOrganizationUserAccountCommand>()
     [BitAutoData]
     public async Task BulkDeleteAccount_WhenUserCannotManageUsers_ThrowsNotFoundException(
         Guid orgId,
-        SecureOrganizationUserBulkRequestModel model,
+        OrganizationUserBulkRequestModel model,
         SutProvider<OrganizationUsersController> sutProvider)
     {
         sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(false);
 
-        await Assert.ThrowsAsync<NotFoundException>(() =>
-            sutProvider.Sut.BulkDeleteAccount(orgId, model));
+        await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.BulkDeleteAccount(orgId, model));
     }
 
     [Theory]
     [BitAutoData]
     public async Task BulkDeleteAccount_WhenCurrentUserNotFound_ThrowsUnauthorizedAccessException(
-        Guid orgId,
-        SecureOrganizationUserBulkRequestModel model,
-        SutProvider<OrganizationUsersController> sutProvider)
+        Guid orgId, OrganizationUserBulkRequestModel model, SutProvider<OrganizationUsersController> sutProvider)
     {
         sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(true);
         sutProvider.GetDependency<IUserService>().GetUserByPrincipalAsync(default).ReturnsForAnyArgs((User)null);
@@ -390,14 +356,11 @@ await Assert.ThrowsAsync<UnauthorizedAccessException>(() =>
     [Theory]
     [BitAutoData]
     public async Task BulkDeleteAccount_WhenSecretVerificationFails_ThrowsBadRequestException(
-        Guid orgId,
-        SecureOrganizationUserBulkRequestModel model,
-        User currentUser,
+        Guid orgId, OrganizationUserBulkRequestModel model, User currentUser,
         SutProvider<OrganizationUsersController> sutProvider)
     {
         sutProvider.GetDependency<ICurrentContext>().ManageUsers(orgId).Returns(true);
         sutProvider.GetDependency<IUserService>().GetUserByPrincipalAsync(default).ReturnsForAnyArgs(currentUser);
-        sutProvider.GetDependency<IUserService>().VerifySecretAsync(currentUser, model.Secret).Returns(false);
 
         await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.BulkDeleteAccount(orgId, model));
     }