From d6211327db01bf3d7108bbdacea2c1f467e43e89 Mon Sep 17 00:00:00 2001 From: d11n Date: Mon, 25 Nov 2024 01:38:44 +0100 Subject: [PATCH] Greenfield: Users API fixes (#6425) * Do not crash when creatuing users without password * More precise error message for user approval toggling --- BTCPayServer.Tests/GreenfieldAPITests.cs | 22 ++++++++++++++-- .../GreenField/GreenfieldUsersController.cs | 25 +++++++++++-------- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/BTCPayServer.Tests/GreenfieldAPITests.cs b/BTCPayServer.Tests/GreenfieldAPITests.cs index dd059b95e..bfebb99b7 100644 --- a/BTCPayServer.Tests/GreenfieldAPITests.cs +++ b/BTCPayServer.Tests/GreenfieldAPITests.cs @@ -999,7 +999,7 @@ namespace BTCPayServer.Tests Assert.Contains("ServerAdmin", admin.Roles); Assert.NotNull(admin.Created); Assert.True((DateTimeOffset.Now - admin.Created).Value.Seconds < 10); - + // Creating a new user without proper creds is now impossible (unauthorized) // Because if registration are locked and that an admin exists, we don't accept unauthenticated connection var ex = await AssertAPIError("unauthenticated", @@ -1051,7 +1051,14 @@ namespace BTCPayServer.Tests Password = "afewfoiewiou", IsAdministrator = true }); + + // Create user without password + await adminClient.CreateUser(new CreateApplicationUserRequest + { + Email = "nopassword@gmail.com" + }); + // Regular user var user1Acc = tester.NewAccount(); user1Acc.UserId = user1.Id; user1Acc.IsAdmin = false; @@ -4168,6 +4175,11 @@ namespace BTCPayServer.Tests Assert.True((await adminClient.GetUserByIdOrEmail(unapprovedUser.UserId)).Approved); Assert.True((await unapprovedUserApiKeyClient.GetCurrentUser()).Approved); Assert.True((await unapprovedUserBasicAuthClient.GetCurrentUser()).Approved); + var err = await AssertAPIError("invalid-state", async () => + { + await adminClient.ApproveUser(unapprovedUser.UserId, true, CancellationToken.None); + }); + Assert.Equal("User is already approved", err.APIError.Message); // un-approve Assert.True(await adminClient.ApproveUser(unapprovedUser.UserId, false, CancellationToken.None)); @@ -4180,6 +4192,11 @@ namespace BTCPayServer.Tests { await unapprovedUserBasicAuthClient.GetCurrentUser(); }); + err = await AssertAPIError("invalid-state", async () => + { + await adminClient.ApproveUser(unapprovedUser.UserId, false, CancellationToken.None); + }); + Assert.Equal("User is already unapproved", err.APIError.Message); // reset policies to not require approval await settings.UpdateSetting(new PoliciesSettings { LockSubscription = false, RequiresUserApproval = false }); @@ -4196,10 +4213,11 @@ namespace BTCPayServer.Tests Assert.Single(await adminClient.GetNotifications(false)); // try unapproving user which does not have the RequiresApproval flag - await AssertAPIError("invalid-state", async () => + err = await AssertAPIError("invalid-state", async () => { await adminClient.ApproveUser(newUser.UserId, false, CancellationToken.None); }); + Assert.Equal("Unapproving user failed: No approval required", err.APIError.Message); } [Fact(Timeout = 60 * 2 * 1000)] diff --git a/BTCPayServer/Controllers/GreenField/GreenfieldUsersController.cs b/BTCPayServer/Controllers/GreenField/GreenfieldUsersController.cs index fcaa5d911..6097d020b 100644 --- a/BTCPayServer/Controllers/GreenField/GreenfieldUsersController.cs +++ b/BTCPayServer/Controllers/GreenField/GreenfieldUsersController.cs @@ -112,14 +112,13 @@ namespace BTCPayServer.Controllers.Greenfield return this.UserNotFound(); } - var success = false; if (user.RequiresApproval) { - success = await _userService.SetUserApproval(user.Id, request.Approved, Request.GetAbsoluteRootUri()); + return await _userService.SetUserApproval(user.Id, request.Approved, Request.GetAbsoluteRootUri()) + ? Ok() + : this.CreateAPIError("invalid-state", $"User is already {(request.Approved ? "approved" : "unapproved")}"); } - - return success ? Ok() : this.CreateAPIError("invalid-state", - $"{(request.Approved ? "Approving" : "Unapproving")} user failed"); + return this.CreateAPIError("invalid-state", $"{(request.Approved ? "Approving" : "Unapproving")} user failed: No approval required"); } [Authorize(Policy = Policies.CanViewUsers, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] @@ -350,21 +349,25 @@ namespace BTCPayServer.Controllers.Greenfield blob.Name = request.Name; blob.ImageUrl = request.ImageUrl; user.SetBlob(blob); - var passwordValidation = await this._passwordValidator.ValidateAsync(_userManager, user, request.Password); - if (!passwordValidation.Succeeded) + var hasPassword = !string.IsNullOrEmpty(request.Password); + if (hasPassword) { - foreach (var error in passwordValidation.Errors) + var passwordValidation = await _passwordValidator.ValidateAsync(_userManager, user, request.Password); + if (!passwordValidation.Succeeded) { - ModelState.AddModelError(nameof(request.Password), error.Description); + foreach (var error in passwordValidation.Errors) + { + ModelState.AddModelError(nameof(request.Password), error.Description); + } + return this.CreateValidationError(ModelState); } - return this.CreateValidationError(ModelState); } if (!isAdmin) { if (!await _throttleService.Throttle(ZoneLimits.Register, this.HttpContext.Connection.RemoteIpAddress, cancellationToken)) return new TooManyRequestsResult(ZoneLimits.Register); } - var identityResult = await _userManager.CreateAsync(user, request.Password); + var identityResult = hasPassword ? await _userManager.CreateAsync(user, request.Password) : await _userManager.CreateAsync(user); if (!identityResult.Succeeded) { foreach (var error in identityResult.Errors)