From fe107cd23f09baeecd32648bc06c63975ef2b05a Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sun, 7 Mar 2021 16:50:55 -0800 Subject: [PATCH 01/20] Add DELETE action for api/v1/users endpoint --- .../Controllers/GreenField/UsersController.cs | 84 ++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index faab54b11..3122b80d2 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -9,10 +10,11 @@ using BTCPayServer.Configuration; using BTCPayServer.Data; using BTCPayServer.Events; using BTCPayServer.HostedServices; -using BTCPayServer.Logging; using BTCPayServer.Security; using BTCPayServer.Security.GreenField; using BTCPayServer.Services; +using BTCPayServer.Storage.Services; +using BTCPayServer.Services.Stores; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Cors; using Microsoft.AspNetCore.Identity; @@ -35,6 +37,9 @@ namespace BTCPayServer.Controllers.GreenField private readonly BTCPayServerOptions _options; private readonly IAuthorizationService _authorizationService; private readonly CssThemeManager _themeManager; + private readonly FileService _fileService; + private readonly StoredFileRepository _storedFileRepository; + private readonly StoreRepository _storeRepository; public UsersController(UserManager userManager, RoleManager roleManager, @@ -44,7 +49,10 @@ namespace BTCPayServer.Controllers.GreenField RateLimitService throttleService, BTCPayServerOptions options, IAuthorizationService authorizationService, - CssThemeManager themeManager) + CssThemeManager themeManager, + FileService fileService, + StoredFileRepository storedFileRepository, + StoreRepository storeRepository) { _userManager = userManager; _roleManager = roleManager; @@ -55,6 +63,9 @@ namespace BTCPayServer.Controllers.GreenField _options = options; _authorizationService = authorizationService; _themeManager = themeManager; + _fileService = fileService; + _storedFileRepository = storedFileRepository; + _storeRepository = storeRepository; } [Authorize(Policy = Policies.CanViewProfile, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] @@ -165,6 +176,75 @@ namespace BTCPayServer.Controllers.GreenField return CreatedAtAction(string.Empty, model); } + [HttpDelete("~/api/v1/users/{userId}")] + [Authorize(Policy = Policies.CanCreateUser, AuthenticationSchemes = AuthenticationSchemes.GreenfieldAPIKeys)] + public async Task> DeleteUser(string userId) + { + var isAdmin = await IsAdmin(); + // Only admins should be allowed to delete users + if (!isAdmin) + { + return Forbid(AuthenticationSchemes.GreenfieldBasic); + } + + var user = userId == null ? null : await _userManager.FindByIdAsync(userId); + if (user == null) + { + return NotFound(); + } + + var roles = await _userManager.GetRolesAsync(user); + // We can safely delete the user if it's not an admin user + if (!IsAdmin(roles)) + { + await DeleteUserAndAssociatedData(userId, user); + + return Ok(); + } + + var admins = await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin); + // User shouldn't be deleted if it's the only admin + if (admins.Count == 1) + { + return Forbid(AuthenticationSchemes.GreenfieldBasic); + } + + // Ok, this user is an admin but there are other admins as well so safe to delete + await DeleteUserAndAssociatedData(userId, user); + + return Ok(); + } + + private async Task DeleteUserAndAssociatedData(string userId, ApplicationUser user) + { + var files = await _storedFileRepository.GetFiles(new StoredFileRepository.FilesQuery() + { + UserIds = new[] { userId }, + }); + + await Task.WhenAll(files.Select(file => _fileService.RemoveFile(file.Id, userId))); + + await _userManager.DeleteAsync(user); + await _storeRepository.CleanUnreachableStores(); + } + + private async Task IsAdmin() + { + var anyAdmin = (await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Any(); + var isAuth = User.Identity.AuthenticationType == GreenFieldConstants.AuthenticationType; + var isAdmin = anyAdmin ? (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.CanModifyServerSettings))).Succeeded + && (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.Unrestricted))).Succeeded + && isAuth + : true; + + return isAdmin; + } + + private static bool IsAdmin(IList roles) + { + return roles.Contains(Roles.ServerAdmin, StringComparer.Ordinal); + } + private async Task FromModel(ApplicationUser data) { var roles = (await _userManager.GetRolesAsync(data)).ToArray(); From 37f7c4e0f9b1884dd92889053c353b306d036431 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Mon, 8 Mar 2021 18:25:11 -0800 Subject: [PATCH 02/20] Add DELETE user Swagger docs --- .../swagger/v1/swagger.template.users.json | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json b/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json index 57764207d..517d4c5b3 100644 --- a/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json +++ b/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json @@ -107,6 +107,48 @@ } ] } + }, + "/api/v1/users/{userId}": { + "delete": { + "tags": [ + "Users" + ], + "summary": "Delete user", + "description": "Delete a user.\n\nMust be an admin to perform this operation.\n\nAttempting to delete the only admin user will not succeed.\n\nAll data associated with the user will be deleted as well if the operation succeeds.", + "parameters": [ + { + "name": "userId", + "in": "path", + "required": true, + "description": "The ID of the user to be deleted", + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "User has been successfully deleted" + }, + "401": { + "description": "Missing authorization for deleting the user" + }, + "403": { + "description": "Authorized but forbidden to delete the user. Can happen if you attempt to delete the only admin user." + }, + "404": { + "description": "User with provided ID was not found" + } + }, + "security": [ + { + "API Key": [ + "btcpay.server.cancreateuser" + ], + "Basic": [] + } + ] + } } }, "components": { From 907ae760e0ad049e224f0872eb79100bce13988b Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Tue, 9 Mar 2021 18:21:33 -0800 Subject: [PATCH 03/20] Add CanDeleteUser policy --- BTCPayServer.Client/Permissions.cs | 2 ++ BTCPayServer/Controllers/GreenField/UsersController.cs | 2 +- BTCPayServer/Controllers/ManageController.APIKeys.cs | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/BTCPayServer.Client/Permissions.cs b/BTCPayServer.Client/Permissions.cs index 08dfeda61..3e6ad1b46 100644 --- a/BTCPayServer.Client/Permissions.cs +++ b/BTCPayServer.Client/Permissions.cs @@ -24,6 +24,7 @@ namespace BTCPayServer.Client public const string CanManageNotificationsForUser = "btcpay.user.canmanagenotificationsforuser"; public const string CanViewNotificationsForUser = "btcpay.user.canviewnotificationsforuser"; public const string CanCreateUser = "btcpay.server.cancreateuser"; + public const string CanDeleteUser = "btcpay.server.candeleteuser"; public const string CanManagePullPayments = "btcpay.store.canmanagepullpayments"; public const string Unrestricted = "unrestricted"; public static IEnumerable AllPolicies @@ -41,6 +42,7 @@ namespace BTCPayServer.Client yield return CanModifyProfile; yield return CanViewProfile; yield return CanCreateUser; + yield return CanDeleteUser; yield return CanManageNotificationsForUser; yield return CanViewNotificationsForUser; yield return Unrestricted; diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index 3122b80d2..f1e3d5b99 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -177,7 +177,7 @@ namespace BTCPayServer.Controllers.GreenField } [HttpDelete("~/api/v1/users/{userId}")] - [Authorize(Policy = Policies.CanCreateUser, AuthenticationSchemes = AuthenticationSchemes.GreenfieldAPIKeys)] + [Authorize(Policy = Policies.CanDeleteUser, AuthenticationSchemes = AuthenticationSchemes.GreenfieldAPIKeys)] public async Task> DeleteUser(string userId) { var isAdmin = await IsAdmin(); diff --git a/BTCPayServer/Controllers/ManageController.APIKeys.cs b/BTCPayServer/Controllers/ManageController.APIKeys.cs index dace755eb..d17552663 100644 --- a/BTCPayServer/Controllers/ManageController.APIKeys.cs +++ b/BTCPayServer/Controllers/ManageController.APIKeys.cs @@ -470,6 +470,7 @@ namespace BTCPayServer.Controllers { {BTCPayServer.Client.Policies.Unrestricted, ("Unrestricted access", "The app will have unrestricted access to your account.")}, {BTCPayServer.Client.Policies.CanCreateUser, ("Create new users", "The app will be able to create new users on this server.")}, + {BTCPayServer.Client.Policies.CanDeleteUser, ("Delete users", "The app will be able to delete users on this server.")}, {BTCPayServer.Client.Policies.CanModifyStoreSettings, ("Modify your stores", "The app will be able to view, modify, delete and create new invoices on all your stores.")}, {$"{BTCPayServer.Client.Policies.CanModifyStoreSettings}:", ("Manage selected stores", "The app will be able to view, modify, delete and create new invoices on the selected stores.")}, {BTCPayServer.Client.Policies.CanModifyStoreWebhooks, ("Modify stores webhooks", "The app will modify the webhooks of all your stores.")}, From e5a196918f9bbf5d648d1201aa4072c946aeff9a Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sun, 14 Mar 2021 12:24:32 -0700 Subject: [PATCH 04/20] Add user service --- .../Controllers/GreenField/UsersController.cs | 37 +++++++--------- .../Controllers/ServerController.Users.cs | 9 +--- BTCPayServer/Controllers/ServerController.cs | 6 ++- BTCPayServer/Hosting/BTCPayServerServices.cs | 1 + BTCPayServer/Services/UserService.cs | 44 +++++++++++++++++++ 5 files changed, 67 insertions(+), 30 deletions(-) create mode 100644 BTCPayServer/Services/UserService.cs diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index f1e3d5b99..675482855 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -40,6 +40,7 @@ namespace BTCPayServer.Controllers.GreenField private readonly FileService _fileService; private readonly StoredFileRepository _storedFileRepository; private readonly StoreRepository _storeRepository; + private readonly UserService _userService; public UsersController(UserManager userManager, RoleManager roleManager, @@ -52,7 +53,8 @@ namespace BTCPayServer.Controllers.GreenField CssThemeManager themeManager, FileService fileService, StoredFileRepository storedFileRepository, - StoreRepository storeRepository) + StoreRepository storeRepository, + UserService userService) { _userManager = userManager; _roleManager = roleManager; @@ -66,6 +68,7 @@ namespace BTCPayServer.Controllers.GreenField _fileService = fileService; _storedFileRepository = storedFileRepository; _storeRepository = storeRepository; + _userService = userService; } [Authorize(Policy = Policies.CanViewProfile, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] @@ -197,7 +200,7 @@ namespace BTCPayServer.Controllers.GreenField // We can safely delete the user if it's not an admin user if (!IsAdmin(roles)) { - await DeleteUserAndAssociatedData(userId, user); + await _userService.DeleteUserAndAssociatedData(user); return Ok(); } @@ -210,34 +213,26 @@ namespace BTCPayServer.Controllers.GreenField } // Ok, this user is an admin but there are other admins as well so safe to delete - await DeleteUserAndAssociatedData(userId, user); + await _userService.DeleteUserAndAssociatedData(user); return Ok(); } - private async Task DeleteUserAndAssociatedData(string userId, ApplicationUser user) - { - var files = await _storedFileRepository.GetFiles(new StoredFileRepository.FilesQuery() - { - UserIds = new[] { userId }, - }); - - await Task.WhenAll(files.Select(file => _fileService.RemoveFile(file.Id, userId))); - - await _userManager.DeleteAsync(user); - await _storeRepository.CleanUnreachableStores(); - } + private async Task IsAdmin() { var anyAdmin = (await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Any(); - var isAuth = User.Identity.AuthenticationType == GreenFieldConstants.AuthenticationType; - var isAdmin = anyAdmin ? (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.CanModifyServerSettings))).Succeeded - && (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.Unrestricted))).Succeeded - && isAuth - : true; + // You are an admin if there are no other admins + if (!anyAdmin) + { + return true; + } - return isAdmin; + var isAuth = User.Identity.AuthenticationType == GreenFieldConstants.AuthenticationType; + return (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.CanModifyServerSettings))).Succeeded + && (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.Unrestricted))).Succeeded + && isAuth; } private static bool IsAdmin(IList roles) diff --git a/BTCPayServer/Controllers/ServerController.Users.cs b/BTCPayServer/Controllers/ServerController.Users.cs index a3bc89f71..16a2ce375 100644 --- a/BTCPayServer/Controllers/ServerController.Users.cs +++ b/BTCPayServer/Controllers/ServerController.Users.cs @@ -236,15 +236,8 @@ namespace BTCPayServer.Controllers if (user == null) return NotFound(); - var files = await _StoredFileRepository.GetFiles(new StoredFileRepository.FilesQuery() - { - UserIds = new[] { userId }, - }); + await _userService.DeleteUserAndAssociatedData(user); - await Task.WhenAll(files.Select(file => _FileService.RemoveFile(file.Id, userId))); - - await _UserManager.DeleteAsync(user); - await _StoreRepository.CleanUnreachableStores(); TempData[WellKnownTempData.SuccessMessage] = "User deleted"; return RedirectToAction(nameof(ListUsers)); } diff --git a/BTCPayServer/Controllers/ServerController.cs b/BTCPayServer/Controllers/ServerController.cs index 221d0e465..247184325 100644 --- a/BTCPayServer/Controllers/ServerController.cs +++ b/BTCPayServer/Controllers/ServerController.cs @@ -46,6 +46,7 @@ namespace BTCPayServer.Controllers public partial class ServerController : Controller { private readonly UserManager _UserManager; + private readonly UserService _userService; readonly SettingsRepository _SettingsRepository; private readonly NBXplorerDashboard _dashBoard; private readonly StoreRepository _StoreRepository; @@ -61,7 +62,9 @@ namespace BTCPayServer.Controllers private readonly FileService _FileService; private readonly IEnumerable _StorageProviderServices; - public ServerController(UserManager userManager, + public ServerController( + UserManager userManager, + UserService userService, StoredFileRepository storedFileRepository, FileService fileService, IEnumerable storageProviderServices, @@ -83,6 +86,7 @@ namespace BTCPayServer.Controllers _FileService = fileService; _StorageProviderServices = storageProviderServices; _UserManager = userManager; + _userService = userService; _SettingsRepository = settingsRepository; _dashBoard = dashBoard; HttpClientFactory = httpClientFactory; diff --git a/BTCPayServer/Hosting/BTCPayServerServices.cs b/BTCPayServer/Hosting/BTCPayServerServices.cs index 084b39f52..b9db7bdd9 100644 --- a/BTCPayServer/Hosting/BTCPayServerServices.cs +++ b/BTCPayServer/Hosting/BTCPayServerServices.cs @@ -112,6 +112,7 @@ namespace BTCPayServer.Hosting services.TryAddSingleton(); services.TryAddSingleton(); services.TryAddSingleton(); + services.TryAddSingleton(); services.AddSingleton(); services.AddOptions().Configure( (options) => diff --git a/BTCPayServer/Services/UserService.cs b/BTCPayServer/Services/UserService.cs new file mode 100644 index 000000000..d828bed20 --- /dev/null +++ b/BTCPayServer/Services/UserService.cs @@ -0,0 +1,44 @@ +using System.Linq; +using System.Threading.Tasks; +using BTCPayServer.Data; +using BTCPayServer.Storage.Services; +using BTCPayServer.Services.Stores; +using Microsoft.AspNetCore.Identity; + +namespace BTCPayServer.Services +{ + public class UserService + { + private readonly UserManager _userManager; + private readonly StoredFileRepository _storedFileRepository; + private readonly FileService _fileService; + private readonly StoreRepository _storeRepository; + + public UserService( + UserManager userManager, + StoredFileRepository storedFileRepository, + FileService fileService, + StoreRepository storeRepository + ) + { + _userManager = userManager; + _storedFileRepository = storedFileRepository; + _fileService = fileService; + _storeRepository = storeRepository; + } + + public async Task DeleteUserAndAssociatedData(ApplicationUser user) + { + var userId = user.Id; + var files = await _storedFileRepository.GetFiles(new StoredFileRepository.FilesQuery() + { + UserIds = new[] { userId }, + }); + + await Task.WhenAll(files.Select(file => _fileService.RemoveFile(file.Id, userId))); + + await _userManager.DeleteAsync(user); + await _storeRepository.CleanUnreachableStores(); + } + } +} From 2ed83414035cee4d2bcec5c719a6c11fa43c47dc Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sun, 14 Mar 2021 13:02:43 -0700 Subject: [PATCH 05/20] Add IsRoleAdmin to user service --- .../Controllers/GreenField/UsersController.cs | 9 +-------- BTCPayServer/Controllers/ServerController.Users.cs | 11 +++-------- BTCPayServer/Services/UserService.cs | 13 ++++++++++++- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index 675482855..ad05825b5 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -198,7 +198,7 @@ namespace BTCPayServer.Controllers.GreenField var roles = await _userManager.GetRolesAsync(user); // We can safely delete the user if it's not an admin user - if (!IsAdmin(roles)) + if (!_userService.IsRoleAdmin(roles)) { await _userService.DeleteUserAndAssociatedData(user); @@ -218,8 +218,6 @@ namespace BTCPayServer.Controllers.GreenField return Ok(); } - - private async Task IsAdmin() { var anyAdmin = (await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Any(); @@ -235,11 +233,6 @@ namespace BTCPayServer.Controllers.GreenField && isAuth; } - private static bool IsAdmin(IList roles) - { - return roles.Contains(Roles.ServerAdmin, StringComparer.Ordinal); - } - private async Task FromModel(ApplicationUser data) { var roles = (await _userManager.GetRolesAsync(data)).ToArray(); diff --git a/BTCPayServer/Controllers/ServerController.Users.cs b/BTCPayServer/Controllers/ServerController.Users.cs index 16a2ce375..7a42734b7 100644 --- a/BTCPayServer/Controllers/ServerController.Users.cs +++ b/BTCPayServer/Controllers/ServerController.Users.cs @@ -79,16 +79,11 @@ namespace BTCPayServer.Controllers Id = user.Id, Email = user.Email, Verified = user.EmailConfirmed || !user.RequiresEmailConfirmation, - IsAdmin = IsAdmin(roles) + IsAdmin = _userService.IsRoleAdmin(roles) }; return View(userVM); } - private static bool IsAdmin(IList roles) - { - return roles.Contains(Roles.ServerAdmin, StringComparer.Ordinal); - } - [Route("server/users/{userId}")] [HttpPost] public new async Task User(string userId, UsersViewModel.UserViewModel viewModel) @@ -99,7 +94,7 @@ namespace BTCPayServer.Controllers var admins = await _UserManager.GetUsersInRoleAsync(Roles.ServerAdmin); var roles = await _UserManager.GetRolesAsync(user); - var wasAdmin = IsAdmin(roles); + var wasAdmin = _userService.IsRoleAdmin(roles); if (!viewModel.IsAdmin && admins.Count == 1 && wasAdmin) { TempData[WellKnownTempData.ErrorMessage] = "This is the only Admin, so their role can't be removed until another Admin is added."; @@ -206,7 +201,7 @@ namespace BTCPayServer.Controllers return NotFound(); var roles = await _UserManager.GetRolesAsync(user); - if (IsAdmin(roles)) + if (_userService.IsRoleAdmin(roles)) { var admins = await _UserManager.GetUsersInRoleAsync(Roles.ServerAdmin); if (admins.Count == 1) diff --git a/BTCPayServer/Services/UserService.cs b/BTCPayServer/Services/UserService.cs index d828bed20..ee1b455d0 100644 --- a/BTCPayServer/Services/UserService.cs +++ b/BTCPayServer/Services/UserService.cs @@ -1,27 +1,33 @@ +using System; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Identity; using BTCPayServer.Data; using BTCPayServer.Storage.Services; using BTCPayServer.Services.Stores; -using Microsoft.AspNetCore.Identity; namespace BTCPayServer.Services { public class UserService { private readonly UserManager _userManager; + private readonly IAuthorizationService _authorizationService; private readonly StoredFileRepository _storedFileRepository; private readonly FileService _fileService; private readonly StoreRepository _storeRepository; public UserService( UserManager userManager, + IAuthorizationService authorizationService, StoredFileRepository storedFileRepository, FileService fileService, StoreRepository storeRepository ) { _userManager = userManager; + _authorizationService = authorizationService; _storedFileRepository = storedFileRepository; _fileService = fileService; _storeRepository = storeRepository; @@ -40,5 +46,10 @@ namespace BTCPayServer.Services await _userManager.DeleteAsync(user); await _storeRepository.CleanUnreachableStores(); } + + public bool IsRoleAdmin(IList roles) + { + return roles.Contains(Roles.ServerAdmin, StringComparer.Ordinal); + } } } From 104092702a6632fd4280932111a83bd31d2f66ab Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sun, 4 Apr 2021 16:36:19 -0700 Subject: [PATCH 06/20] Simplify DeleteUser method --- BTCPayServer/Controllers/GreenField/UsersController.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index ad05825b5..e22f1ae6d 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -183,9 +183,8 @@ namespace BTCPayServer.Controllers.GreenField [Authorize(Policy = Policies.CanDeleteUser, AuthenticationSchemes = AuthenticationSchemes.GreenfieldAPIKeys)] public async Task> DeleteUser(string userId) { - var isAdmin = await IsAdmin(); // Only admins should be allowed to delete users - if (!isAdmin) + if (!User.IsInRole(Roles.ServerAdmin)) { return Forbid(AuthenticationSchemes.GreenfieldBasic); } @@ -196,18 +195,16 @@ namespace BTCPayServer.Controllers.GreenField return NotFound(); } - var roles = await _userManager.GetRolesAsync(user); // We can safely delete the user if it's not an admin user - if (!_userService.IsRoleAdmin(roles)) + if (!_userService.IsRoleAdmin(await _userManager.GetRolesAsync(user))) { await _userService.DeleteUserAndAssociatedData(user); return Ok(); } - var admins = await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin); // User shouldn't be deleted if it's the only admin - if (admins.Count == 1) + if ((await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Count == 1) { return Forbid(AuthenticationSchemes.GreenfieldBasic); } From 49f168b7b398c9f030223302f849cb779ad5af06 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sun, 4 Apr 2021 16:43:09 -0700 Subject: [PATCH 07/20] Update delete user swagger description --- BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json b/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json index 517d4c5b3..d7af9c565 100644 --- a/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json +++ b/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json @@ -143,7 +143,7 @@ "security": [ { "API Key": [ - "btcpay.server.cancreateuser" + "btcpay.server.candeleteuser" ], "Basic": [] } From 7c117369923e39ef48b74af2bfc83e0b6acacaf3 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sun, 4 Apr 2021 20:56:48 -0700 Subject: [PATCH 08/20] Enable nullable reference checking in UserService --- BTCPayServer/Services/UserService.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/BTCPayServer/Services/UserService.cs b/BTCPayServer/Services/UserService.cs index ee1b455d0..17586896f 100644 --- a/BTCPayServer/Services/UserService.cs +++ b/BTCPayServer/Services/UserService.cs @@ -1,3 +1,4 @@ +#nullable enable using System; using System.Collections.Generic; using System.Linq; From 53c81918a56a432d020a0e686d58f3bf67bb9426 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Tue, 6 Apr 2021 18:00:07 -0700 Subject: [PATCH 09/20] Enable nullable reference checking in UsersController --- .../Controllers/GreenField/UsersController.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index e22f1ae6d..0b9f28757 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -1,5 +1,5 @@ +#nullable enable using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -83,13 +83,13 @@ namespace BTCPayServer.Controllers.GreenField [HttpPost("~/api/v1/users")] public async Task CreateUser(CreateApplicationUserRequest request, CancellationToken cancellationToken = default) { - if (request?.Email is null) + if (request.Email is null) ModelState.AddModelError(nameof(request.Email), "Email is missing"); - if (!string.IsNullOrEmpty(request?.Email) && !Validation.EmailValidator.IsEmail(request.Email)) + if (!string.IsNullOrEmpty(request.Email) && !Validation.EmailValidator.IsEmail(request.Email)) { ModelState.AddModelError(nameof(request.Email), "Invalid email"); } - if (request?.Password is null) + if (request.Password is null) ModelState.AddModelError(nameof(request.Password), "Password is missing"); if (!ModelState.IsValid) @@ -168,8 +168,10 @@ namespace BTCPayServer.Controllers.GreenField if (!anyAdmin) { var settings = await _settingsRepository.GetSettingAsync(); - settings.FirstRun = false; - await _settingsRepository.UpdateSetting(settings); + if (settings != null) { + settings.FirstRun = false; + await _settingsRepository.UpdateSetting(settings); + } await _settingsRepository.FirstAdminRegistered(policies, _options.UpdateUrl != null, _options.DisableRegistration); } From b4076b53e82787d9207606a0e1faadf1ccecaf4a Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Tue, 6 Apr 2021 18:19:31 -0700 Subject: [PATCH 10/20] Add IsAdminUser method to UserService --- BTCPayServer/Controllers/GreenField/UsersController.cs | 2 +- BTCPayServer/Services/UserService.cs | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index 0b9f28757..e3fa08df7 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -198,7 +198,7 @@ namespace BTCPayServer.Controllers.GreenField } // We can safely delete the user if it's not an admin user - if (!_userService.IsRoleAdmin(await _userManager.GetRolesAsync(user))) + if (!(await _userService.IsAdminUser(user))) { await _userService.DeleteUserAndAssociatedData(user); diff --git a/BTCPayServer/Services/UserService.cs b/BTCPayServer/Services/UserService.cs index 17586896f..bfd8ddf16 100644 --- a/BTCPayServer/Services/UserService.cs +++ b/BTCPayServer/Services/UserService.cs @@ -34,6 +34,11 @@ namespace BTCPayServer.Services _storeRepository = storeRepository; } + public async Task IsAdminUser(ApplicationUser user) + { + return IsRoleAdmin(await _userManager.GetRolesAsync(user)); + } + public async Task DeleteUserAndAssociatedData(ApplicationUser user) { var userId = user.Id; From d9935ada9dd83578978ac51c4d7550b491085a69 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Wed, 7 Apr 2021 20:40:57 -0700 Subject: [PATCH 11/20] Add "/api/v1/users/me" endpoint --- BTCPayServer.Client/Permissions.cs | 2 +- .../Controllers/GreenField/UsersController.cs | 32 ++++++++++++++++++- .../Controllers/ManageController.APIKeys.cs | 2 +- .../GreenFieldAuthorizationHandler.cs | 1 + .../swagger/v1/swagger.template.users.json | 30 ++++++++++++----- 5 files changed, 56 insertions(+), 11 deletions(-) diff --git a/BTCPayServer.Client/Permissions.cs b/BTCPayServer.Client/Permissions.cs index 3e6ad1b46..adc1480f5 100644 --- a/BTCPayServer.Client/Permissions.cs +++ b/BTCPayServer.Client/Permissions.cs @@ -24,7 +24,7 @@ namespace BTCPayServer.Client public const string CanManageNotificationsForUser = "btcpay.user.canmanagenotificationsforuser"; public const string CanViewNotificationsForUser = "btcpay.user.canviewnotificationsforuser"; public const string CanCreateUser = "btcpay.server.cancreateuser"; - public const string CanDeleteUser = "btcpay.server.candeleteuser"; + public const string CanDeleteUser = "btcpay.user.candeleteuser"; public const string CanManagePullPayments = "btcpay.store.canmanagepullpayments"; public const string Unrestricted = "unrestricted"; public static IEnumerable AllPolicies diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index e3fa08df7..51fccca01 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -79,6 +79,21 @@ namespace BTCPayServer.Controllers.GreenField return await FromModel(user); } + [Authorize(Policy = Policies.CanDeleteUser, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] + [HttpDelete("~/api/v1/users/me")] + public async Task> DeleteCurrentUser() + { + // Don't want to allow the user to delete themselves if they are the only admin + if (await IsUserTheOnlyOneAdmin()) { + return Forbid(AuthenticationSchemes.GreenfieldBasic); + } + + var user = await _userManager.GetUserAsync(User); + await _userService.DeleteUserAndAssociatedData(user); + + return Ok(); + } + [AllowAnonymous] [HttpPost("~/api/v1/users")] public async Task CreateUser(CreateApplicationUserRequest request, CancellationToken cancellationToken = default) @@ -206,7 +221,7 @@ namespace BTCPayServer.Controllers.GreenField } // User shouldn't be deleted if it's the only admin - if ((await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Count == 1) + if (await IsUserTheOnlyOneAdmin(user)) { return Forbid(AuthenticationSchemes.GreenfieldBasic); } @@ -245,5 +260,20 @@ namespace BTCPayServer.Controllers.GreenField Created = data.Created }; } + + private async Task IsUserTheOnlyOneAdmin() + { + return await IsUserTheOnlyOneAdmin(await _userManager.GetUserAsync(User)); + } + + private async Task IsUserTheOnlyOneAdmin(ApplicationUser user) + { + var isUserAdmin = await _userService.IsAdminUser(user); + if (!isUserAdmin) { + return false; + } + + return (await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Count == 1; + } } } diff --git a/BTCPayServer/Controllers/ManageController.APIKeys.cs b/BTCPayServer/Controllers/ManageController.APIKeys.cs index d17552663..945ff12b4 100644 --- a/BTCPayServer/Controllers/ManageController.APIKeys.cs +++ b/BTCPayServer/Controllers/ManageController.APIKeys.cs @@ -470,7 +470,7 @@ namespace BTCPayServer.Controllers { {BTCPayServer.Client.Policies.Unrestricted, ("Unrestricted access", "The app will have unrestricted access to your account.")}, {BTCPayServer.Client.Policies.CanCreateUser, ("Create new users", "The app will be able to create new users on this server.")}, - {BTCPayServer.Client.Policies.CanDeleteUser, ("Delete users", "The app will be able to delete users on this server.")}, + {BTCPayServer.Client.Policies.CanDeleteUser, ("Delete user", "The app will be able to delete the user to whom it is assigned. Admin users can delete any user without this permission.")}, {BTCPayServer.Client.Policies.CanModifyStoreSettings, ("Modify your stores", "The app will be able to view, modify, delete and create new invoices on all your stores.")}, {$"{BTCPayServer.Client.Policies.CanModifyStoreSettings}:", ("Manage selected stores", "The app will be able to view, modify, delete and create new invoices on the selected stores.")}, {BTCPayServer.Client.Policies.CanModifyStoreWebhooks, ("Modify stores webhooks", "The app will modify the webhooks of all your stores.")}, diff --git a/BTCPayServer/Security/GreenField/GreenFieldAuthorizationHandler.cs b/BTCPayServer/Security/GreenField/GreenFieldAuthorizationHandler.cs index ffb65688f..c545f72dc 100644 --- a/BTCPayServer/Security/GreenField/GreenFieldAuthorizationHandler.cs +++ b/BTCPayServer/Security/GreenField/GreenFieldAuthorizationHandler.cs @@ -93,6 +93,7 @@ namespace BTCPayServer.Security.GreenField case Policies.CanViewNotificationsForUser: case Policies.CanModifyProfile: case Policies.CanViewProfile: + case Policies.CanDeleteUser: case Policies.Unrestricted: success = context.HasPermission(Permission.Create(policy), requiredUnscoped); break; diff --git a/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json b/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json index d7af9c565..1ecd2586e 100644 --- a/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json +++ b/BTCPayServer/wwwroot/swagger/v1/swagger.template.users.json @@ -28,6 +28,27 @@ "Basic": [] } ] + }, + "delete": { + "tags": [ + "Users" + ], + "summary": "Deletes user profile", + "description": "Deletes user profile and associated user data for user making the request", + "operationId": "Users_DeleteCurrentUser", + "responses": { + "200": { + "description": "User and associated data deleted successfully" + } + }, + "security": [ + { + "API Key": [ + "btcpay.user.candeleteuser" + ], + "Basic": [] + } + ] } }, "/api/v1/users": { @@ -140,14 +161,7 @@ "description": "User with provided ID was not found" } }, - "security": [ - { - "API Key": [ - "btcpay.server.candeleteuser" - ], - "Basic": [] - } - ] + "security": [] } } }, From 9fc2d2b76bcfe443ddf0b300505d340fe18736b0 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Wed, 7 Apr 2021 20:48:38 -0700 Subject: [PATCH 12/20] Remove CanDeleteUser constraint from "/api/v1/users/{userId}" endpoint --- BTCPayServer/Controllers/GreenField/UsersController.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index 51fccca01..8ee74b3af 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -197,7 +197,7 @@ namespace BTCPayServer.Controllers.GreenField } [HttpDelete("~/api/v1/users/{userId}")] - [Authorize(Policy = Policies.CanDeleteUser, AuthenticationSchemes = AuthenticationSchemes.GreenfieldAPIKeys)] + [Authorize(AuthenticationSchemes = AuthenticationSchemes.GreenfieldAPIKeys)] public async Task> DeleteUser(string userId) { // Only admins should be allowed to delete users From 949d6bf5840a5eabeeb2bbfdd77f648c6e5ae646 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sat, 10 Apr 2021 19:33:37 -0700 Subject: [PATCH 13/20] Replace admin check with CanModifyServerSettings authorization policy --- BTCPayServer/Controllers/GreenField/UsersController.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index 8ee74b3af..ae196f32e 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -197,15 +197,9 @@ namespace BTCPayServer.Controllers.GreenField } [HttpDelete("~/api/v1/users/{userId}")] - [Authorize(AuthenticationSchemes = AuthenticationSchemes.GreenfieldAPIKeys)] + [Authorize(Policy = Policies.CanModifyServerSettings, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] public async Task> DeleteUser(string userId) { - // Only admins should be allowed to delete users - if (!User.IsInRole(Roles.ServerAdmin)) - { - return Forbid(AuthenticationSchemes.GreenfieldBasic); - } - var user = userId == null ? null : await _userManager.FindByIdAsync(userId); if (user == null) { From 9f8f677125ab5bd4b43fb9feadbe90393434a687 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sat, 10 Apr 2021 19:34:10 -0700 Subject: [PATCH 14/20] Remove unused IsAdmin method --- .../Controllers/GreenField/UsersController.cs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index ae196f32e..e4a6ce503 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -226,21 +226,6 @@ namespace BTCPayServer.Controllers.GreenField return Ok(); } - private async Task IsAdmin() - { - var anyAdmin = (await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Any(); - // You are an admin if there are no other admins - if (!anyAdmin) - { - return true; - } - - var isAuth = User.Identity.AuthenticationType == GreenFieldConstants.AuthenticationType; - return (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.CanModifyServerSettings))).Succeeded - && (await _authorizationService.AuthorizeAsync(User, null, new PolicyRequirement(Policies.Unrestricted))).Succeeded - && isAuth; - } - private async Task FromModel(ApplicationUser data) { var roles = (await _userManager.GetRolesAsync(data)).ToArray(); From 0b4d94faf2b14d587807f38682f4f6cd2b6ff923 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sat, 10 Apr 2021 21:15:18 -0700 Subject: [PATCH 15/20] Remove unnecessary null check --- BTCPayServer/Controllers/GreenField/UsersController.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index e4a6ce503..4eaf56c61 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -200,7 +200,7 @@ namespace BTCPayServer.Controllers.GreenField [Authorize(Policy = Policies.CanModifyServerSettings, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] public async Task> DeleteUser(string userId) { - var user = userId == null ? null : await _userManager.FindByIdAsync(userId); + var user = await _userManager.FindByIdAsync(userId); if (user == null) { return NotFound(); From be05b6aa90ff443ef6b1e46931b578f829185bb2 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sat, 10 Apr 2021 22:27:17 -0700 Subject: [PATCH 16/20] Specify correct return type on delete user methods --- BTCPayServer/Controllers/GreenField/UsersController.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index 4eaf56c61..a15541940 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -81,7 +81,7 @@ namespace BTCPayServer.Controllers.GreenField [Authorize(Policy = Policies.CanDeleteUser, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] [HttpDelete("~/api/v1/users/me")] - public async Task> DeleteCurrentUser() + public async Task DeleteCurrentUser() { // Don't want to allow the user to delete themselves if they are the only admin if (await IsUserTheOnlyOneAdmin()) { @@ -198,7 +198,7 @@ namespace BTCPayServer.Controllers.GreenField [HttpDelete("~/api/v1/users/{userId}")] [Authorize(Policy = Policies.CanModifyServerSettings, AuthenticationSchemes = AuthenticationSchemes.Greenfield)] - public async Task> DeleteUser(string userId) + public async Task DeleteUser(string userId) { var user = await _userManager.FindByIdAsync(userId); if (user == null) From e943c55a918cca96926f3b63077be8e4affbcac0 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sat, 10 Apr 2021 22:36:34 -0700 Subject: [PATCH 17/20] Add basic tests for delete user endpoint --- .../BTCPayServerClient.Users.cs | 8 +++++++ BTCPayServer.Tests/GreenfieldAPITests.cs | 22 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/BTCPayServer.Client/BTCPayServerClient.Users.cs b/BTCPayServer.Client/BTCPayServerClient.Users.cs index 0879bd7ce..d3d3e50df 100644 --- a/BTCPayServer.Client/BTCPayServerClient.Users.cs +++ b/BTCPayServer.Client/BTCPayServerClient.Users.cs @@ -1,3 +1,4 @@ +#nullable enable using System.Net.Http; using System.Threading; using System.Threading.Tasks; @@ -19,5 +20,12 @@ namespace BTCPayServer.Client var response = await _httpClient.SendAsync(CreateHttpRequest("api/v1/users", null, request, HttpMethod.Post), token); return await HandleResponse(response); } + + public virtual async Task DeleteUser(string userId, CancellationToken token = default) + { + var response = await _httpClient.SendAsync(CreateHttpRequest($"api/v1/users/{userId}", null, HttpMethod.Delete), token); + await HandleResponse(response); + return response; + } } } diff --git a/BTCPayServer.Tests/GreenfieldAPITests.cs b/BTCPayServer.Tests/GreenfieldAPITests.cs index 558c0c6a3..5be327869 100644 --- a/BTCPayServer.Tests/GreenfieldAPITests.cs +++ b/BTCPayServer.Tests/GreenfieldAPITests.cs @@ -127,6 +127,28 @@ namespace BTCPayServer.Tests } } + [Fact(Timeout = TestTimeout)] + [Trait("Integration", "Integration")] + public async Task CanDeleteUsersViaApi() + { + using (var tester = ServerTester.Create(newDb: true)) + { + await tester.StartAsync(); + var unauthClient = new BTCPayServerClient(tester.PayTester.ServerUri); + // Should not be authorized to perform this action + await AssertHttpError(401, + async () => await unauthClient.DeleteUser("lol user id")); + + var user = tester.NewAccount(); + user.GrantAccess(); + await user.MakeAdmin(); + var adminClient = await user.CreateClient(Policies.Unrestricted); + // Should 404 if user doesn't exist + await AssertHttpError(404, + async () => await adminClient.DeleteUser("lol user id")); + } + } + [Fact(Timeout = TestTimeout)] [Trait("Integration", "Integration")] public async Task CanCreateUsersViaAPI() From 87310557860c7bc12f736b815842d7962906bca2 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Sun, 16 May 2021 17:48:25 -0700 Subject: [PATCH 18/20] Use UserNotFound instead of NotFound --- BTCPayServer/Controllers/GreenField/UsersController.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index a15541940..c4d2d526d 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -203,7 +203,7 @@ namespace BTCPayServer.Controllers.GreenField var user = await _userManager.FindByIdAsync(userId); if (user == null) { - return NotFound(); + return UserNotFound(); } // We can safely delete the user if it's not an admin user @@ -254,5 +254,10 @@ namespace BTCPayServer.Controllers.GreenField return (await _userManager.GetUsersInRoleAsync(Roles.ServerAdmin)).Count == 1; } + + private IActionResult UserNotFound() + { + return this.CreateAPIError(404, "user-not-found", "The user was not found"); + } } } From ca3f97c42f36e97b313dda4dcda26361fa12aaa6 Mon Sep 17 00:00:00 2001 From: Umar Bolatov Date: Wed, 2 Jun 2021 20:12:53 -0700 Subject: [PATCH 19/20] Add missing API key type description --- BTCPayServer/wwwroot/swagger/v1/swagger.template.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BTCPayServer/wwwroot/swagger/v1/swagger.template.json b/BTCPayServer/wwwroot/swagger/v1/swagger.template.json index 5753d73e7..5000aafe7 100644 --- a/BTCPayServer/wwwroot/swagger/v1/swagger.template.json +++ b/BTCPayServer/wwwroot/swagger/v1/swagger.template.json @@ -76,7 +76,7 @@ "securitySchemes": { "API Key": { "type": "apiKey", - "description": "BTCPay Server supports authenticating and authorizing users through an API Key that is generated by them. Send the API Key as a header value to Authorization with the format: `token {token}`. For a smoother experience, you can generate a url that redirects users to an API key creation screen.\n\n The following permissions are available to the context of the user creating the API Key:\n\n* `unrestricted`: Unrestricted access\n* `btcpay.user.canviewprofile`: View your profile\n* `btcpay.user.canmodifyprofile`: Manage your profile\n* `btcpay.user.canmanagenotificationsforuser`: Manage your notifications\n* `btcpay.user.canviewnotificationsforuser`: View your notifications\n\nThe following permissions are available if the user is an administrator:\n\n* `btcpay.server.cancreateuser`: Create new users\n* `btcpay.server.canmodifyserversettings`: Manage your server\n* `btcpay.server.canuseinternallightningnode`: Use the internal lightning node\n* `btcpay.server.cancreatelightninginvoiceinternalnode`: Create invoices with internal lightning node\n\nThe following permissions applies to all stores of the user, you can limit to a specific store with the following format: `btcpay.store.cancreateinvoice:6HSHAEU4iYWtjxtyRs9KyPjM9GAQp8kw2T9VWbGG1FnZ`:\n\n* `btcpay.store.canmodifystoresettings`: Modify your stores\n* `btcpay.store.webhooks.canmodifywebhooks`: Modify stores webhooks\n* `btcpay.store.canviewstoresettings`: View your stores\n* `btcpay.store.cancreateinvoice`: Create an invoice\n* `btcpay.store.canviewinvoices`: View invoices\n* `btcpay.store.canmodifypaymentrequests`: Modify your payment requests\n* `btcpay.store.canviewpaymentrequests`: View your payment requests\n* `btcpay.store.canuselightningnode`: Use the lightning nodes associated with your stores\n* `btcpay.store.cancreatelightninginvoice`: Create invoices the lightning nodes associated with your stores\n\nNote that API Keys only limits permission of a user and can never expand it. If an API Key has the permission `btcpay.server.canmodifyserversettings` but that the user account creating this API Key is not administrator, the API Key will not be able to modify the server settings.\n", + "description": "BTCPay Server supports authenticating and authorizing users through an API Key that is generated by them. Send the API Key as a header value to Authorization with the format: `token {token}`. For a smoother experience, you can generate a url that redirects users to an API key creation screen.\n\n The following permissions are available to the context of the user creating the API Key:\n\n* `unrestricted`: Unrestricted access\n* `btcpay.user.candeleteuser`: Delete user\n* `btcpay.user.canviewprofile`: View your profile\n* `btcpay.user.canmodifyprofile`: Manage your profile\n* `btcpay.user.canmanagenotificationsforuser`: Manage your notifications\n* `btcpay.user.canviewnotificationsforuser`: View your notifications\n\nThe following permissions are available if the user is an administrator:\n\n* `btcpay.server.cancreateuser`: Create new users\n* `btcpay.server.canmodifyserversettings`: Manage your server\n* `btcpay.server.canuseinternallightningnode`: Use the internal lightning node\n* `btcpay.server.cancreatelightninginvoiceinternalnode`: Create invoices with internal lightning node\n\nThe following permissions applies to all stores of the user, you can limit to a specific store with the following format: `btcpay.store.cancreateinvoice:6HSHAEU4iYWtjxtyRs9KyPjM9GAQp8kw2T9VWbGG1FnZ`:\n\n* `btcpay.store.canmodifystoresettings`: Modify your stores\n* `btcpay.store.webhooks.canmodifywebhooks`: Modify stores webhooks\n* `btcpay.store.canviewstoresettings`: View your stores\n* `btcpay.store.cancreateinvoice`: Create an invoice\n* `btcpay.store.canviewinvoices`: View invoices\n* `btcpay.store.canmodifypaymentrequests`: Modify your payment requests\n* `btcpay.store.canviewpaymentrequests`: View your payment requests\n* `btcpay.store.canuselightningnode`: Use the lightning nodes associated with your stores\n* `btcpay.store.cancreatelightninginvoice`: Create invoices the lightning nodes associated with your stores\n\nNote that API Keys only limits permission of a user and can never expand it. If an API Key has the permission `btcpay.server.canmodifyserversettings` but that the user account creating this API Key is not administrator, the API Key will not be able to modify the server settings.\n", "name": "Authorization", "in": "header", "scheme": "token" From 28da78fc7881fdc41a2045dcf154ce83363f6b57 Mon Sep 17 00:00:00 2001 From: Kukks Date: Fri, 4 Jun 2021 12:20:45 +0200 Subject: [PATCH 20/20] Improve tests and small refactoring --- .../BTCPayServerClient.Users.cs | 8 +++- BTCPayServer.Tests/GreenfieldAPITests.cs | 47 +++++++++++++------ .../Controllers/GreenField/UsersController.cs | 19 +------- 3 files changed, 39 insertions(+), 35 deletions(-) diff --git a/BTCPayServer.Client/BTCPayServerClient.Users.cs b/BTCPayServer.Client/BTCPayServerClient.Users.cs index d3d3e50df..08063878b 100644 --- a/BTCPayServer.Client/BTCPayServerClient.Users.cs +++ b/BTCPayServer.Client/BTCPayServerClient.Users.cs @@ -21,11 +21,15 @@ namespace BTCPayServer.Client return await HandleResponse(response); } - public virtual async Task DeleteUser(string userId, CancellationToken token = default) + public virtual async Task DeleteUser(string userId, CancellationToken token = default) { var response = await _httpClient.SendAsync(CreateHttpRequest($"api/v1/users/{userId}", null, HttpMethod.Delete), token); await HandleResponse(response); - return response; + } + + public virtual async Task DeleteCurrentUser(CancellationToken token = default) + { + await DeleteUser("me", token); } } } diff --git a/BTCPayServer.Tests/GreenfieldAPITests.cs b/BTCPayServer.Tests/GreenfieldAPITests.cs index 5be327869..acd0d84d2 100644 --- a/BTCPayServer.Tests/GreenfieldAPITests.cs +++ b/BTCPayServer.Tests/GreenfieldAPITests.cs @@ -131,22 +131,39 @@ namespace BTCPayServer.Tests [Trait("Integration", "Integration")] public async Task CanDeleteUsersViaApi() { - using (var tester = ServerTester.Create(newDb: true)) - { - await tester.StartAsync(); - var unauthClient = new BTCPayServerClient(tester.PayTester.ServerUri); - // Should not be authorized to perform this action - await AssertHttpError(401, - async () => await unauthClient.DeleteUser("lol user id")); + using var tester = ServerTester.Create(newDb: true); + await tester.StartAsync(); + var unauthClient = new BTCPayServerClient(tester.PayTester.ServerUri); + // Should not be authorized to perform this action + await AssertHttpError(401, + async () => await unauthClient.DeleteUser("lol user id")); - var user = tester.NewAccount(); - user.GrantAccess(); - await user.MakeAdmin(); - var adminClient = await user.CreateClient(Policies.Unrestricted); - // Should 404 if user doesn't exist - await AssertHttpError(404, - async () => await adminClient.DeleteUser("lol user id")); - } + var user = tester.NewAccount(); + await user.GrantAccessAsync(); + await user.MakeAdmin(); + var adminClient = await user.CreateClient(Policies.Unrestricted); + + //can't delete if the only admin + await AssertHttpError(403, + async () => await adminClient.DeleteCurrentUser()); + + // Should 404 if user doesn't exist + await AssertHttpError(404, + async () => await adminClient.DeleteUser("lol user id")); + + user = tester.NewAccount(); + await user.GrantAccessAsync(); + var badClient = await user.CreateClient(Policies.CanCreateInvoice); + + await AssertHttpError(403, + async () => await badClient.DeleteCurrentUser()); + + var goodClient = await user.CreateClient(Policies.CanDeleteUser, Policies.CanViewProfile); + await goodClient.DeleteCurrentUser(); + await AssertHttpError(404, + async () => await adminClient.DeleteUser(user.UserId)); + + tester.Stores.Remove(user.StoreId); } [Fact(Timeout = TestTimeout)] diff --git a/BTCPayServer/Controllers/GreenField/UsersController.cs b/BTCPayServer/Controllers/GreenField/UsersController.cs index c4d2d526d..fe5c2478a 100644 --- a/BTCPayServer/Controllers/GreenField/UsersController.cs +++ b/BTCPayServer/Controllers/GreenField/UsersController.cs @@ -37,9 +37,6 @@ namespace BTCPayServer.Controllers.GreenField private readonly BTCPayServerOptions _options; private readonly IAuthorizationService _authorizationService; private readonly CssThemeManager _themeManager; - private readonly FileService _fileService; - private readonly StoredFileRepository _storedFileRepository; - private readonly StoreRepository _storeRepository; private readonly UserService _userService; public UsersController(UserManager userManager, @@ -51,9 +48,6 @@ namespace BTCPayServer.Controllers.GreenField BTCPayServerOptions options, IAuthorizationService authorizationService, CssThemeManager themeManager, - FileService fileService, - StoredFileRepository storedFileRepository, - StoreRepository storeRepository, UserService userService) { _userManager = userManager; @@ -65,9 +59,6 @@ namespace BTCPayServer.Controllers.GreenField _options = options; _authorizationService = authorizationService; _themeManager = themeManager; - _fileService = fileService; - _storedFileRepository = storedFileRepository; - _storeRepository = storeRepository; _userService = userService; } @@ -83,15 +74,7 @@ namespace BTCPayServer.Controllers.GreenField [HttpDelete("~/api/v1/users/me")] public async Task DeleteCurrentUser() { - // Don't want to allow the user to delete themselves if they are the only admin - if (await IsUserTheOnlyOneAdmin()) { - return Forbid(AuthenticationSchemes.GreenfieldBasic); - } - - var user = await _userManager.GetUserAsync(User); - await _userService.DeleteUserAndAssociatedData(user); - - return Ok(); + return await DeleteUser(_userManager.GetUserId(User)); } [AllowAnonymous]