From e50e3f662d0cfe00de0339e21e7fedd556133d81 Mon Sep 17 00:00:00 2001 From: "nicolas.dorier" Date: Wed, 18 Mar 2020 23:10:15 +0900 Subject: [PATCH] Can create user without authentication if there is no admin --- BTCPayServer.Client/BTCPayServerClient.cs | 10 +- .../Models/ApplicationUserData.cs | 4 - BTCPayServer.Tests/GreenfieldAPITests.cs | 102 +++++++++++++++--- BTCPayServer.Tests/ServerTester.cs | 12 ++- BTCPayServer/Controllers/AccountController.cs | 1 + .../RestApi/Users/UsersController.cs | 70 ++++++++++-- BTCPayServer/Hosting/BTCPayServerServices.cs | 2 + BTCPayServer/ZoneLimits.cs | 1 + .../wwwroot/swagger/v1/swagger.template.json | 33 +++--- 9 files changed, 189 insertions(+), 46 deletions(-) diff --git a/BTCPayServer.Client/BTCPayServerClient.cs b/BTCPayServer.Client/BTCPayServerClient.cs index a855e1028..60c81c400 100644 --- a/BTCPayServer.Client/BTCPayServerClient.cs +++ b/BTCPayServer.Client/BTCPayServerClient.cs @@ -23,6 +23,13 @@ namespace BTCPayServer.Client PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + public BTCPayServerClient(Uri btcpayHost, HttpClient httpClient = null) + { + if (btcpayHost == null) + throw new ArgumentNullException(nameof(btcpayHost)); + _btcpayHost = btcpayHost; + _httpClient = httpClient ?? new HttpClient(); + } public BTCPayServerClient(Uri btcpayHost, string APIKey, HttpClient httpClient = null) { _apiKey = APIKey; @@ -52,7 +59,8 @@ namespace BTCPayServer.Client } var httpRequest = new HttpRequestMessage(method ?? HttpMethod.Get, uriBuilder.Uri); - httpRequest.Headers.Authorization = new AuthenticationHeaderValue("token", _apiKey); + if (_apiKey != null) + httpRequest.Headers.Authorization = new AuthenticationHeaderValue("token", _apiKey); return httpRequest; diff --git a/BTCPayServer.Client/Models/ApplicationUserData.cs b/BTCPayServer.Client/Models/ApplicationUserData.cs index 6a88944b4..7c7dd142c 100644 --- a/BTCPayServer.Client/Models/ApplicationUserData.cs +++ b/BTCPayServer.Client/Models/ApplicationUserData.cs @@ -34,9 +34,5 @@ namespace BTCPayServer.Client.Models /// Whether this user is an administrator. If left null and there are no admins in the system, the user will be created as an admin. /// public bool? IsAdministrator { get; set; } - /// - /// If the server requires email confirmation, this allows you to set the account as confirmed from the start - /// - public bool? EmailConfirmed { get; set; } } } diff --git a/BTCPayServer.Tests/GreenfieldAPITests.cs b/BTCPayServer.Tests/GreenfieldAPITests.cs index 511178ca8..1e7403fd9 100644 --- a/BTCPayServer.Tests/GreenfieldAPITests.cs +++ b/BTCPayServer.Tests/GreenfieldAPITests.cs @@ -6,6 +6,7 @@ using BTCPayServer.Client; using BTCPayServer.Client.Models; using BTCPayServer.Controllers; using BTCPayServer.Controllers.RestApi.Users; +using BTCPayServer.Services; using BTCPayServer.Tests.Logging; using Microsoft.AspNet.SignalR.Client; using Microsoft.AspNetCore.Mvc; @@ -23,7 +24,7 @@ namespace BTCPayServer.Tests public GreenfieldAPITests(ITestOutputHelper helper) { - Logs.Tester = new XUnitLog(helper) {Name = "Tests"}; + Logs.Tester = new XUnitLog(helper) { Name = "Tests" }; Logs.LogProvider = new XUnitLogProvider(helper); } @@ -44,7 +45,7 @@ namespace BTCPayServer.Tests Assert.Equal(client.APIKey, apiKeyData.ApiKey); Assert.Equal(user.UserId, apiKeyData.UserId); Assert.Equal(2, apiKeyData.Permissions.Length); - + //revoke current api key await client.RevokeCurrentAPIKeyInfo(); await Assert.ThrowsAsync(async () => @@ -53,7 +54,81 @@ namespace BTCPayServer.Tests }); } } - + + [Fact(Timeout = TestTimeout)] + [Trait("Integration", "Integration")] + public async Task CanCreateUsersViaAPI() + { + using (var tester = ServerTester.Create(newDb: true)) + { + await tester.StartAsync(); + var unauthClient = new BTCPayServerClient(tester.PayTester.ServerUri); + await AssertHttpError(400, async () => await unauthClient.CreateUser(new CreateApplicationUserRequest())); + await AssertHttpError(400, async () => await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "test@gmail.com" })); + // Pass too simple + await AssertHttpError(400, async () => await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "test3@gmail.com", Password = "a" })); + + // We have no admin, so it should work + var user1 = await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "test@gmail.com", Password = "abceudhqw" }); + // We have no admin, so it should work + var user2 = await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "test2@gmail.com", Password = "abceudhqw" }); + + // Duplicate email + await AssertHttpError(400, async () => await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "test2@gmail.com", Password = "abceudhqw" })); + + // Let's make an admin + var admin = await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "admin@gmail.com", Password = "abceudhqw", IsAdministrator = true }); + + // 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 + await AssertHttpError(401, async () => await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "test3@gmail.com", Password = "afewfoiewiou" })); + + + // But should be ok with subscriptions unlocked + var settings = tester.PayTester.GetService(); + await settings.UpdateSetting(new PoliciesSettings() { LockSubscription = false }); + await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "test3@gmail.com", Password = "afewfoiewiou" }); + + // But it should be forbidden to create an admin without being authenticated + await AssertHttpError(403, async () => await unauthClient.CreateUser(new CreateApplicationUserRequest() { Email = "admin2@gmail.com", Password = "afewfoiewiou", IsAdministrator = true })); + await settings.UpdateSetting(new PoliciesSettings() { LockSubscription = true }); + + var adminAcc = tester.NewAccount(); + adminAcc.UserId = admin.Id; + adminAcc.IsAdmin = true; + var adminClient = await adminAcc.CreateClient(Permissions.ProfileManagement); + + // We should be forbidden to create a new user without proper admin permissions + await AssertHttpError(403, async () => await adminClient.CreateUser(new CreateApplicationUserRequest() { Email = "test4@gmail.com", Password = "afewfoiewiou" })); + await AssertHttpError(403, async () => await adminClient.CreateUser(new CreateApplicationUserRequest() { Email = "test4@gmail.com", Password = "afewfoiewiou", IsAdministrator = true })); + + // However, should be ok with the server management permissions + adminClient = await adminAcc.CreateClient(Permissions.ServerManagement); + await adminClient.CreateUser(new CreateApplicationUserRequest() { Email = "test4@gmail.com", Password = "afewfoiewiou" }); + // Even creating new admin should be ok + await adminClient.CreateUser(new CreateApplicationUserRequest() { Email = "admin4@gmail.com", Password = "afewfoiewiou", IsAdministrator = true }); + + var user1Acc = tester.NewAccount(); + user1Acc.UserId = user1.Id; + user1Acc.IsAdmin = false; + var user1Client = await user1Acc.CreateClient(Permissions.ServerManagement); + // User1 trying to get server management would still fail to create user + await AssertHttpError(403, async () => await user1Client.CreateUser(new CreateApplicationUserRequest() { Email = "test8@gmail.com", Password = "afewfoiewiou" })); + + // User1 should be able to create user if subscription unlocked + await settings.UpdateSetting(new PoliciesSettings() { LockSubscription = false }); + await user1Client.CreateUser(new CreateApplicationUserRequest() { Email = "test8@gmail.com", Password = "afewfoiewiou" }); + // But not an admin + await AssertHttpError(403, async () => await user1Client.CreateUser(new CreateApplicationUserRequest() { Email = "admin8@gmail.com", Password = "afewfoiewiou", IsAdministrator = true })); + } + } + + private async Task AssertHttpError(int code, Func act) + { + var ex = await Assert.ThrowsAsync(act); + Assert.Contains(code.ToString(), ex.Message); + } + [Fact(Timeout = TestTimeout)] [Trait("Integration", "Integration")] public async Task UsersControllerTests() @@ -76,35 +151,36 @@ namespace BTCPayServer.Tests await Assert.ThrowsAsync(async () => await clientInsufficient.GetCurrentUser()); await clientServer.GetCurrentUser(); - - + + await Assert.ThrowsAsync(async () => await clientInsufficient.CreateUser(new CreateApplicationUserRequest() { Email = $"{Guid.NewGuid()}@g.com", Password = Guid.NewGuid().ToString() - }) ); + })); var newUser = await clientServer.CreateUser(new CreateApplicationUserRequest() { - Email = $"{Guid.NewGuid()}@g.com", Password = Guid.NewGuid().ToString() + Email = $"{Guid.NewGuid()}@g.com", + Password = Guid.NewGuid().ToString() }); Assert.NotNull(newUser); - + await Assert.ThrowsAsync(async () => await clientServer.CreateUser(new CreateApplicationUserRequest() { Email = $"{Guid.NewGuid()}", Password = Guid.NewGuid().ToString() - }) ); - + })); + await Assert.ThrowsAsync(async () => await clientServer.CreateUser(new CreateApplicationUserRequest() { Email = $"{Guid.NewGuid()}@g.com", - }) ); - + })); + await Assert.ThrowsAsync(async () => await clientServer.CreateUser(new CreateApplicationUserRequest() { Password = Guid.NewGuid().ToString() - }) ); + })); } } diff --git a/BTCPayServer.Tests/ServerTester.cs b/BTCPayServer.Tests/ServerTester.cs index a5321e1ed..714ff14d5 100644 --- a/BTCPayServer.Tests/ServerTester.cs +++ b/BTCPayServer.Tests/ServerTester.cs @@ -29,13 +29,13 @@ namespace BTCPayServer.Tests { public class ServerTester : IDisposable { - public static ServerTester Create([CallerMemberNameAttribute]string scope = null) + public static ServerTester Create([CallerMemberNameAttribute]string scope = null, bool newDb = false) { - return new ServerTester(scope); + return new ServerTester(scope, newDb); } string _Directory; - public ServerTester(string scope) + public ServerTester(string scope, bool newDb) { _Directory = scope; if (Directory.Exists(_Directory)) @@ -59,6 +59,12 @@ namespace BTCPayServer.Tests Postgres = GetEnvironment("TESTS_POSTGRES", "User ID=postgres;Host=127.0.0.1;Port=39372;Database=btcpayserver"), MySQL = GetEnvironment("TESTS_MYSQL", "User ID=root;Host=127.0.0.1;Port=33036;Database=btcpayserver") }; + if (newDb) + { + var r = RandomUtils.GetUInt32(); + PayTester.Postgres = PayTester.Postgres.Replace("btcpayserver", $"btcpayserver{r}"); + PayTester.MySQL = PayTester.MySQL.Replace("btcpayserver", $"btcpayserver{r}"); + } PayTester.Port = int.Parse(GetEnvironment("TESTS_PORT", Utils.FreeTcpPort().ToString(CultureInfo.InvariantCulture)), CultureInfo.InvariantCulture); PayTester.HostName = GetEnvironment("TESTS_HOSTNAME", "127.0.0.1"); PayTester.InContainer = bool.Parse(GetEnvironment("TESTS_INCONTAINER", "false")); diff --git a/BTCPayServer/Controllers/AccountController.cs b/BTCPayServer/Controllers/AccountController.cs index d4c0fb68a..74b345832 100644 --- a/BTCPayServer/Controllers/AccountController.cs +++ b/BTCPayServer/Controllers/AccountController.cs @@ -404,6 +404,7 @@ namespace BTCPayServer.Controllers [HttpGet] [AllowAnonymous] + [RateLimitsFilter(ZoneLimits.Register, Scope = RateLimitsScope.RemoteAddress)] public async Task Register(string returnUrl = null, bool logon = true, bool useBasicLayout = false) { if (!CanLoginOrRegister()) diff --git a/BTCPayServer/Controllers/RestApi/Users/UsersController.cs b/BTCPayServer/Controllers/RestApi/Users/UsersController.cs index f1980358e..35493547d 100644 --- a/BTCPayServer/Controllers/RestApi/Users/UsersController.cs +++ b/BTCPayServer/Controllers/RestApi/Users/UsersController.cs @@ -1,18 +1,21 @@ using System; using System.ComponentModel.DataAnnotations; using System.Linq; +using System.Threading; using System.Threading.Tasks; using BTCPayServer.Client.Models; using BTCPayServer.Configuration; using BTCPayServer.Data; using BTCPayServer.Events; using BTCPayServer.Security; +using BTCPayServer.Security.APIKeys; using BTCPayServer.Services; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.ModelBinding; +using NicolasDorier.RateLimits; namespace BTCPayServer.Controllers.RestApi.Users { @@ -26,11 +29,15 @@ namespace BTCPayServer.Controllers.RestApi.Users private readonly SettingsRepository _settingsRepository; private readonly EventAggregator _eventAggregator; private readonly IPasswordValidator _passwordValidator; + private readonly RateLimitService _throttleService; + private readonly IAuthorizationService _authorizationService; public UsersController(UserManager userManager, BTCPayServerOptions btcPayServerOptions, RoleManager roleManager, SettingsRepository settingsRepository, EventAggregator eventAggregator, - IPasswordValidator passwordValidator) + IPasswordValidator passwordValidator, + NicolasDorier.RateLimits.RateLimitService throttleService, + IAuthorizationService authorizationService) { _userManager = userManager; _btcPayServerOptions = btcPayServerOptions; @@ -38,6 +45,8 @@ namespace BTCPayServer.Controllers.RestApi.Users _settingsRepository = settingsRepository; _eventAggregator = eventAggregator; _passwordValidator = passwordValidator; + _throttleService = throttleService; + _authorizationService = authorizationService; } [Authorize(Policy = Policies.CanModifyProfile.Key, AuthenticationSchemes = AuthenticationSchemes.ApiKey)] @@ -48,9 +57,9 @@ namespace BTCPayServer.Controllers.RestApi.Users return FromModel(user); } - [Authorize(Policy = Policies.CanCreateUser.Key, AuthenticationSchemes = AuthenticationSchemes.ApiKey)] + [AllowAnonymous] [HttpPost("~/api/v1/users")] - public async Task> CreateUser(CreateApplicationUserRequest request) + public async Task> CreateUser(CreateApplicationUserRequest request, CancellationToken cancellationToken = default) { if (request?.Email is null) return BadRequest(CreateValidationProblem(nameof(request.Email), "Email is missing")); @@ -60,15 +69,39 @@ namespace BTCPayServer.Controllers.RestApi.Users } if (request?.Password is null) return BadRequest(CreateValidationProblem(nameof(request.Password), "Password is missing")); - var policies = await _settingsRepository.GetSettingAsync() ?? new PoliciesSettings(); var anyAdmin = (await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Any(); - var admin = request.IsAdministrator.GetValueOrDefault(!anyAdmin); + var policies = await _settingsRepository.GetSettingAsync() ?? new PoliciesSettings(); + var isAuth = User.Identity.AuthenticationType == APIKeyConstants.AuthenticationType; + + // If registration are locked and that an admin exists, don't accept unauthenticated connection + if (anyAdmin && policies.LockSubscription && !isAuth) + return Unauthorized(); + + // Even if subscription are unlocked, it is forbidden to create admin unauthenticated + if (anyAdmin && request.IsAdministrator is true && !isAuth) + return Forbid(AuthenticationSchemes.ApiKey); + // You are de-facto admin if there is no other admin, else you need to be auth and pass policy requirements + bool isAdmin = anyAdmin ? (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.CanModifyServerSettings.Key))).Succeeded + && isAuth + : true; + // You need to be admin to create an admin + if (request.IsAdministrator is true && !isAdmin) + { + return Forbid(AuthenticationSchemes.ApiKey); + } + + if (!isAdmin && policies.LockSubscription) + { + // If we are not admin and subscriptions are locked, we need to check the Policies.CanCreateUser.Key permission + if (!isAuth || !(await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.CanCreateUser.Key))).Succeeded) + return Forbid(AuthenticationSchemes.ApiKey); + } + var user = new ApplicationUser { UserName = request.Email, Email = request.Email, - RequiresEmailConfirmation = policies.RequiresConfirmedEmail, - EmailConfirmed = request.EmailConfirmed.GetValueOrDefault(false) + RequiresEmailConfirmation = policies.RequiresConfirmedEmail }; var passwordValidation = await this._passwordValidator.ValidateAsync(_userManager, user, request.Password); if (!passwordValidation.Succeeded) @@ -79,6 +112,11 @@ namespace BTCPayServer.Controllers.RestApi.Users } return BadRequest(new ValidationProblemDetails(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); if (!identityResult.Succeeded) { @@ -88,13 +126,23 @@ namespace BTCPayServer.Controllers.RestApi.Users } return BadRequest(new ValidationProblemDetails(ModelState)); } - else if (admin) + + if (request.IsAdministrator is true) { - await _roleManager.CreateAsync(new IdentityRole(Roles.ServerAdmin)); + if (!anyAdmin) + { + await _roleManager.CreateAsync(new IdentityRole(Roles.ServerAdmin)); + } await _userManager.AddToRoleAsync(user, Roles.ServerAdmin); + if (!anyAdmin) + { + // automatically lock subscriptions now that we have our first admin + policies.LockSubscription = true; + await _settingsRepository.UpdateSetting(policies); + } } - _eventAggregator.Publish(new UserRegisteredEvent() {Request = Request, User = user, Admin = admin}); - return CreatedAtAction("", user); + _eventAggregator.Publish(new UserRegisteredEvent() {Request = Request, User = user, Admin = request.IsAdministrator is true }); + return CreatedAtAction(string.Empty, user); } private ValidationProblemDetails CreateValidationProblem(string propertyName, string errorMessage) diff --git a/BTCPayServer/Hosting/BTCPayServerServices.cs b/BTCPayServer/Hosting/BTCPayServerServices.cs index ba845c9ea..07d1282e7 100644 --- a/BTCPayServer/Hosting/BTCPayServerServices.cs +++ b/BTCPayServer/Hosting/BTCPayServerServices.cs @@ -256,10 +256,12 @@ namespace BTCPayServer.Hosting if (btcPayEnv.IsDevelopping) { rateLimits.SetZone($"zone={ZoneLimits.Login} rate=1000r/min burst=100 nodelay"); + rateLimits.SetZone($"zone={ZoneLimits.Register} rate=1000r/min burst=100 nodelay"); } else { rateLimits.SetZone($"zone={ZoneLimits.Login} rate=5r/min burst=3 nodelay"); + rateLimits.SetZone($"zone={ZoneLimits.Register} rate=2r/min burst=2 nodelay"); } return rateLimits; }); diff --git a/BTCPayServer/ZoneLimits.cs b/BTCPayServer/ZoneLimits.cs index 8214330a5..08e722cec 100644 --- a/BTCPayServer/ZoneLimits.cs +++ b/BTCPayServer/ZoneLimits.cs @@ -8,5 +8,6 @@ namespace BTCPayServer public class ZoneLimits { public const string Login = "btcpaylogin"; + public const string Register = "btcpayregister"; } } diff --git a/BTCPayServer/wwwroot/swagger/v1/swagger.template.json b/BTCPayServer/wwwroot/swagger/v1/swagger.template.json index c421e63a8..db8b1ce93 100644 --- a/BTCPayServer/wwwroot/swagger/v1/swagger.template.json +++ b/BTCPayServer/wwwroot/swagger/v1/swagger.template.json @@ -24,7 +24,7 @@ "parameters": [ { "name": "permissions", - "description": "The permissions to request. Current permissions available: ServerManagement, StoreManagement", + "description": "The permissions to request. Current permissions available: ServerManagement, StoreManagement, ProfileManagement", "in": "query", "style": "form", "explode": true, @@ -111,7 +111,7 @@ "security": [ { "APIKey": [ - "btcpay.store.canmodifyprofile" + "ProfileManagement" ] } ] @@ -123,8 +123,7 @@ "Users" ], "summary": "Create user", - "description": "Create a new user", - "operationId": "Users_CreateUser", + "description": "Create a new user. This operation can be called without authentication if there is not any administrator yet on the server.", "requestBody": { "x-name": "request", "content": { @@ -135,19 +134,15 @@ "properties": { "email": { "type": "string", - "nullable": true + "nullable": false }, "password": { - "type": "string", - "nullable": true + "type": "string" }, "isAdministrator": { "type": "boolean", - "nullable": true - }, - "emailConfirmed": { - "type": "boolean", - "nullable": true + "nullable": true, + "default": false } } } @@ -176,13 +171,23 @@ } } } + }, + "401": { + "description": "If you need to authenticate for this endpoint (ie. the server settings policies lock subscriptions and that an admin already exists)" + }, + "403": { + "description": "If you are authenticated but forbidden to create a new user (ie. you don't have the ServerManagement permission)" + }, + "429": { + "description": "DDoS protection if you are creating more than 2 accounts every minutes (non-admin only)" } }, "security": [ { "APIKey": [ - "btcpay.store.cancreateuser" - ] + "ServerManagement" + ], + "Anonymous": [ ] } ] }