Lock user: Improve return code and fix docs (#4377)

* Lock user: Improve return code and fix docs

The docs state that the `DELETE` method should be used, though the controller wants `POST`. The latter seems appropriate here, as the action can be used for locking and unlocking.

Also adapted the action to return a status code based on the actual outcome of the user toggle call.

Closes #4310.

* Update clients

BTCPayServer.Client/BTCPayServerClient.Users.cs

@@ -33,11 +33,12 @@ namespace BTCPayServer.Client
             return await HandleResponse<ApplicationUserData>(response);
         }
 
-        public virtual async Task LockUser(string idOrEmail, bool locked, CancellationToken token = default)
+        public virtual async Task<bool> LockUser(string idOrEmail, bool locked, CancellationToken token = default)
         {
             var response = await _httpClient.SendAsync(CreateHttpRequest($"api/v1/users/{idOrEmail}/lock", null,
-                new LockUserRequest() {Locked = locked}, HttpMethod.Post), token);
+                new LockUserRequest {Locked = locked}, HttpMethod.Post), token);
             await HandleResponse(response);
+            return response.IsSuccessStatusCode;
         }
 
         public virtual async Task<ApplicationUserData[]> GetUsers( CancellationToken token = default)

BTCPayServer.Tests/GreenfieldAPITests.cs

@@ -2920,8 +2920,7 @@ namespace BTCPayServer.Tests
             var newUserClient = await newUser.CreateClient(Policies.Unrestricted);
             Assert.False((await newUserClient.GetCurrentUser()).Disabled);
 
-            await adminClient.LockUser(newUser.UserId, true, CancellationToken.None);
+            Assert.True(await adminClient.LockUser(newUser.UserId, true, CancellationToken.None));
-            
             Assert.True((await adminClient.GetUserByIdOrEmail(newUser.UserId)).Disabled);
             await AssertAPIError("unauthenticated",async () =>
             {
@@ -2934,12 +2933,12 @@ namespace BTCPayServer.Tests
                 await newUserBasicClient.GetCurrentUser();
             });
 
-            await adminClient.LockUser(newUser.UserId, false, CancellationToken.None);
+            Assert.True(await adminClient.LockUser(newUser.UserId, false, CancellationToken.None));
             Assert.False((await adminClient.GetUserByIdOrEmail(newUser.UserId)).Disabled);
             await newUserClient.GetCurrentUser();
             await newUserBasicClient.GetCurrentUser();
             // Twice for good measure
-            await adminClient.LockUser(newUser.UserId, false, CancellationToken.None);
+            Assert.True(await adminClient.LockUser(newUser.UserId, false, CancellationToken.None));
             Assert.False((await adminClient.GetUserByIdOrEmail(newUser.UserId)).Disabled);
             await newUserClient.GetCurrentUser();
             await newUserBasicClient.GetCurrentUser();

BTCPayServer/Controllers/GreenField/GreenfieldUsersController.cs

@@ -76,18 +76,20 @@ namespace BTCPayServer.Controllers.Greenfield
             }
             return UserNotFound();
         }
+        
         [Authorize(Policy = Policies.CanModifyServerSettings, AuthenticationSchemes = AuthenticationSchemes.Greenfield)]
         [HttpPost("~/api/v1/users/{idOrEmail}/lock")]
-        public async Task<IActionResult> LockUser(string idOrEmail, LockUserRequest request )
+        public async Task<IActionResult> LockUser(string idOrEmail, LockUserRequest request)
         {
-            var user = (await _userManager.FindByIdAsync(idOrEmail) ) ?? await _userManager.FindByEmailAsync(idOrEmail);
+            var user = await _userManager.FindByIdAsync(idOrEmail) ?? await _userManager.FindByEmailAsync(idOrEmail);
             if (user is null)
             {
                 return UserNotFound();
             }
 
-            await _userService.ToggleUser(user.Id, request.Locked ? DateTimeOffset.MaxValue : null);
+            var success = await _userService.ToggleUser(user.Id, request.Locked ? DateTimeOffset.MaxValue : null);
-            return Ok();
+            return success.HasValue && success.Value ? Ok() : this.CreateAPIError("invalid-state",
+                $"{(request.Locked ? "Locking" : "Unlocking")} user failed");
         }
         
         [Authorize(Policy = Policies.CanViewUsers, AuthenticationSchemes = AuthenticationSchemes.Greenfield)]

BTCPayServer/Controllers/GreenField/LocalBTCPayServerClient.cs

@@ -1047,9 +1047,11 @@ namespace BTCPayServer.Controllers.Greenfield
             return GetFromActionResult<ApplicationUserData>(await GetController<GreenfieldUsersController>().GetUser(idOrEmail));
         }
 
-        public override async Task LockUser(string idOrEmail, bool disabled, CancellationToken token = default)
+        public override async Task<bool> LockUser(string idOrEmail, bool disabled, CancellationToken token = default)
         {
-            HandleActionResult(await GetController<GreenfieldUsersController>().LockUser(idOrEmail, new LockUserRequest() {Locked = disabled}));
+            return GetFromActionResult<bool>(
+                await GetController<GreenfieldUsersController>().LockUser(idOrEmail,
+                    new LockUserRequest { Locked = disabled }));
         }
 
         public override async Task<OnChainWalletTransactionData> PatchOnChainWalletTransaction(string storeId,

BTCPayServer/Services/UserService.cs

@@ -65,12 +65,12 @@ namespace BTCPayServer.Services
             return user.LockoutEnabled && user.LockoutEnd is not null &&
                    DateTimeOffset.UtcNow < user.LockoutEnd.Value.UtcDateTime;
         }
-        public async Task ToggleUser(string userId, DateTimeOffset? lockedOutDeadline)
+        public async Task<bool?> ToggleUser(string userId, DateTimeOffset? lockedOutDeadline)
         {
             var user = await _userManager.FindByIdAsync(userId);
             if (user is null)
             {
-                return;
+                return null;
             }
             if (lockedOutDeadline is not null)
             {
@@ -87,6 +87,7 @@ namespace BTCPayServer.Services
                 _logger.LogError($"Failed to set lockout for user {user.Id}");
             }
 
+            return res.Succeeded;
         }
 
         public async Task<bool> IsAdminUser(string userId)

BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json

@@ -249,7 +249,7 @@
             }
         },
         "/api/v1/users/{idOrEmail}/lock": {
-            "delete": {
+            "post": {
                 "operationId": "Users_ToggleUserLock",
                 "tags": [
                     "Users"