Store users: Ensure the last owner cannot be downgraded (#6654)

* Store users: Ensure the last owner cannot be downgraded

Changes the behaviour of the `AddOrUpdateStoreUser` method to throw errors for the failure cases, so that the UI and API can report the actual problem. A role change might fail if the user already has that role or if they are the last owner of the store.

* Cleanup code

---------

Co-authored-by: nicolas.dorier <nicolas.dorier@gmail.com>

BTCPayServer.Tests/GreenfieldAPITests.cs

@@ -4082,12 +4082,14 @@ namespace BTCPayServer.Tests
             await AssertPermissionError(Policies.CanModifyStoreSettings, async () => await managerClient.RemoveStoreUser(user.StoreId, user.UserId));
 
             // updates
-            await client.UpdateStoreUser(user.StoreId, employee.UserId, new StoreUserData { StoreRole = ownerRole.Id });
+            await client.UpdateStoreUser(user.StoreId, employee.UserId, new StoreUserData { StoreRole = managerRole.Id });
             await employeeClient.GetStore(user.StoreId);
+            await AssertAPIError("store-user-role-orphaned", async () => await client.UpdateStoreUser(user.StoreId, user.UserId, new StoreUserData { StoreRole = managerRole.Id }));
             
             // remove
             await client.RemoveStoreUser(user.StoreId, employee.UserId);
             await AssertHttpError(403, async () => await employeeClient.GetStore(user.StoreId));
+            await AssertAPIError("store-user-role-orphaned", async () => await client.RemoveStoreUser(user.StoreId, user.UserId));
 
             // test duplicate add
             await client.AddStoreUser(user.StoreId, new StoreUserData { StoreRole = ownerRole.Id, Id = employee.UserId });
@@ -4103,7 +4105,6 @@ namespace BTCPayServer.Tests
 
             await AssertAPIError("store-user-role-orphaned", async () => await employeeClient.RemoveStoreUser(user.StoreId, employee.UserId));
         }
-        
 
         [Fact(Timeout = TestTimeout)]
         [Trait("Integration", "Integration")]

BTCPayServer.Tests/SeleniumTests.cs

@@ -3936,6 +3936,14 @@ retry:
             Assert.Equal(3, options.Count);
             Assert.DoesNotContain(options, element => element.Text.Equals("store role", StringComparison.InvariantCultureIgnoreCase));
             
+            s.Driver.FindElement(By.Id("Email")).SendKeys(s.AsTestAccount().Email);
+            s.Driver.FindElement(By.Id("Role")).SendKeys("owner");
+            s.Driver.FindElement(By.Id("AddUser")).Click();
+            Assert.Contains("The user already has the role Owner.", s.Driver.FindElement(By.CssSelector(".validation-summary-errors")).Text);
+            s.Driver.FindElement(By.Id("Role")).SendKeys("manager");
+            s.Driver.FindElement(By.Id("AddUser")).Click();
+            Assert.Contains("The user is the last owner. Their role cannot be changed.", s.Driver.FindElement(By.CssSelector(".validation-summary-errors")).Text);
+            
             s.GoToStore(StoreNavPages.Roles);
             s.ClickPagePrimary();
             s.Driver.FindElement(By.Id("Role")).SendKeys("Malice");
@@ -4188,7 +4196,7 @@ retry:
             Assert.Equal(s.Driver.WaitForElement(By.Id("EditUserEmail")).Text, employee);
             // no change, no alert message
             s.Driver.FindElement(By.Id("EditContinue")).Click();
-            s.Driver.ElementDoesNotExist(By.CssSelector("#mainContent .alert"));
+            Assert.Contains("The user already has the role Manager.", s.FindAlertMessage(StatusMessageModel.StatusSeverity.Error).Text);
             
             // Should not change last owner
             userRows = s.Driver.FindElements(By.CssSelector("#StoreUsersList tr"));
@@ -4203,7 +4211,7 @@ retry:
             Assert.Equal(s.Driver.WaitForElement(By.Id("EditUserEmail")).Text, owner);
             new SelectElement(s.Driver.FindElement(By.Id("EditUserRole"))).SelectByValue("Employee");
             s.Driver.FindElement(By.Id("EditContinue")).Click();
-            Assert.Contains($"User {owner} is the last owner. Their role cannot be changed.", s.FindAlertMessage(StatusMessageModel.StatusSeverity.Error).Text);
+            Assert.Contains("The user is the last owner. Their role cannot be changed.", s.FindAlertMessage(StatusMessageModel.StatusSeverity.Error).Text);
         }
 
         private static void CanBrowseContent(SeleniumTester s)

BTCPayServer/Controllers/GreenField/GreenfieldStoreUsersController.cs

@@ -1,7 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.Data;
-using System.Reflection.Metadata;
 using System.Threading.Tasks;
 using BTCPayServer.Abstractions.Constants;
 using BTCPayServer.Abstractions.Extensions;
@@ -15,7 +13,7 @@ using Microsoft.AspNetCore.Cors;
 using Microsoft.AspNetCore.Identity;
 using Microsoft.AspNetCore.Mvc;
 using Newtonsoft.Json.Linq;
-using NicolasDorier.RateLimits;
+using static BTCPayServer.Services.Stores.StoreRepository;
 using StoreData = BTCPayServer.Data.StoreData;
 
 namespace BTCPayServer.Controllers.Greenfield
@@ -79,7 +77,6 @@ namespace BTCPayServer.Controllers.Greenfield
             // Deprecated properties
             request.StoreRole ??= request.AdditionalData.TryGetValue("role", out var role) ? role.ToString() : null;
             request.Id ??= request.AdditionalData.TryGetValue("userId", out var userId) ? userId.ToString() : null;
-            /////
 
             var user = await _userManager.FindByIdOrEmail(idOrEmail ?? request.Id);
             if (user == null)
@@ -96,12 +93,24 @@ namespace BTCPayServer.Controllers.Greenfield
             if (!ModelState.IsValid)
                 return this.CreateValidationError(ModelState);
 
-            var result = string.IsNullOrEmpty(idOrEmail)
+            AddOrUpdateStoreUserResult res;
-                ? await _storeRepository.AddStoreUser(storeId, user.Id, roleId)
+            if (string.IsNullOrEmpty(idOrEmail))
-                : await _storeRepository.AddOrUpdateStoreUser(storeId, user.Id, roleId);
+            {
-            return result
+                res = await _storeRepository.AddStoreUser(storeId, user.Id, roleId) ? new AddOrUpdateStoreUserResult.Success() : new AddOrUpdateStoreUserResult.DuplicateRole(roleId);
-                ? Ok()
+            }
-                : this.CreateAPIError(409, "duplicate-store-user-role", "The user is already added to the store");
+            else
+            {
+                res = await _storeRepository.AddOrUpdateStoreUser(storeId, user.Id, roleId);
+            }
+
+            return res switch
+            {
+                AddOrUpdateStoreUserResult.Success => Ok(),
+                AddOrUpdateStoreUserResult.DuplicateRole _ => this.CreateAPIError(409, "duplicate-store-user-role", "The user is already added to the store"),
+                // AddOrUpdateStoreUserResult.InvalidRole
+                // AddOrUpdateStoreUserResult.LastOwner
+                _ => this.CreateAPIError(409, "store-user-role-orphaned", "Removing this user would result in the store having no owner."),
+            };
         }
 
         private async Task<IEnumerable<StoreUserData>> ToAPI(StoreData store)

BTCPayServer/Controllers/GreenField/GreenfieldUsersController.cs

@@ -409,7 +409,6 @@ namespace BTCPayServer.Controllers.Greenfield
                         settings.FirstRun = false;
                         await _settingsRepository.UpdateSetting(settings);
                     }
-
                     await _settingsRepository.FirstAdminRegistered(policies, _options.UpdateUrl != null, _options.DisableRegistration, Logs);
                 }
             }

BTCPayServer/Controllers/UIStoresController.Users.cs

@@ -14,6 +14,7 @@ using BTCPayServer.Services.Mails;
 using BTCPayServer.Services.Stores;
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Mvc;
+using static BTCPayServer.Services.Stores.StoreRepository;
 
 namespace BTCPayServer.Controllers;
 
@@ -83,7 +84,9 @@ public partial class UIStoresController
         var action = isExistingUser
             ? isExistingStoreUser ? "updated" : "added"
             : "invited";
-        if (await _storeRepo.AddOrUpdateStoreUser(CurrentStore.Id, user.Id, roleId))
+
+        var res = await _storeRepo.AddOrUpdateStoreUser(CurrentStore.Id, user.Id, roleId);
+        if (res is AddOrUpdateStoreUserResult.Success)
         {
             TempData.SetStatusMessageModel(new StatusMessageModel
             {
@@ -93,9 +96,11 @@ public partial class UIStoresController
             });
             return RedirectToAction(nameof(StoreUsers));
         }
-
+        else
-        ModelState.AddModelError(nameof(vm.Email), $"The user could not be {action}");
+        {
-        return View(vm);
+            ModelState.AddModelError(nameof(vm.Email), $"The user could not be {action}: {res.ToString()}");
+            return View(vm);
+        }
     }
 
     [HttpPost("{storeId}/users/{userId}")]
@@ -105,12 +110,16 @@ public partial class UIStoresController
         var roleId = await _storeRepo.ResolveStoreRoleId(storeId, vm.Role);
         var storeUsers = await _storeRepo.GetStoreUsers(storeId);
         var user = storeUsers.First(user => user.Id == userId);
-        var isOwner = user.StoreRole.Id == StoreRoleId.Owner.Id;
+
-        var isLastOwner = isOwner && storeUsers.Count(u => u.StoreRole.Id == StoreRoleId.Owner.Id) == 1;
+        var res = await _storeRepo.AddOrUpdateStoreUser(storeId, userId, roleId);
-        if (isLastOwner && roleId != StoreRoleId.Owner)
+        if (res is AddOrUpdateStoreUserResult.Success)
-            TempData[WellKnownTempData.ErrorMessage] = StringLocalizer["User {0} is the last owner. Their role cannot be changed.", user.Email].Value;
+        {
-        else if (await _storeRepo.AddOrUpdateStoreUser(storeId, userId, roleId))
             TempData[WellKnownTempData.SuccessMessage] = StringLocalizer["The role of {0} has been changed to {1}.", user.Email, vm.Role].Value;
+        }
+        else
+        {
+            TempData[WellKnownTempData.ErrorMessage] = StringLocalizer["Changing the role of user {0} failed: {1}", user.Email, res.ToString()].Value;
+        }
         return RedirectToAction(nameof(StoreUsers), new { storeId, userId });
     }
 

BTCPayServer/Services/Stores/StoreRepository.cs

@@ -7,6 +7,7 @@ using BTCPayServer.Abstractions.Contracts;
 using BTCPayServer.Client;
 using BTCPayServer.Data;
 using BTCPayServer.Events;
+using BTCPayServer.Migrations;
 using Dapper;
 using Microsoft.EntityFrameworkCore;
 using NBitcoin;
@@ -314,13 +315,34 @@ namespace BTCPayServer.Services.Stores
             }
         }
 
-        public async Task<bool> AddOrUpdateStoreUser(string storeId, string userId, StoreRoleId? roleId = null)
+        public record AddOrUpdateStoreUserResult
+        {
+            public record Success : AddOrUpdateStoreUserResult;
+            public record InvalidRole : AddOrUpdateStoreUserResult
+            {
+                public override string ToString() => "The roleId doesn't exist";
+            }
+            public record LastOwner : AddOrUpdateStoreUserResult
+            {
+                public override string ToString() => "The user is the last owner. Their role cannot be changed.";
+            }
+            public record DuplicateRole(StoreRoleId RoleId) : AddOrUpdateStoreUserResult
+            {
+                public override string ToString() => $"The user already has the role {RoleId}.";
+            }
+        }
+        public async Task<AddOrUpdateStoreUserResult> AddOrUpdateStoreUser(string storeId, string userId, StoreRoleId? roleId = null)
         {
             ArgumentNullException.ThrowIfNull(storeId);
             AssertStoreRoleIfNeeded(storeId, roleId);
             roleId ??= await GetDefaultRole();
+            var storeRole = await GetStoreRole(roleId);
+            if (storeRole is null)
+                return new AddOrUpdateStoreUserResult.InvalidRole();
+
             await using var ctx = _ContextFactory.CreateContext();
-            var userStore = await ctx.UserStore.FindAsync(userId, storeId);
+            var userStore = await ctx.UserStore.Include(store => store.StoreRole)
+                .FirstOrDefaultAsync(u => u.ApplicationUserId == userId && u.StoreDataId == storeId);
             var added = false;
             if (userStore is null)
             {
@@ -328,9 +350,15 @@ namespace BTCPayServer.Services.Stores
                 ctx.UserStore.Add(userStore);
                 added = true;
             }
+            // ensure the last owner doesn't get downgraded
+            else if (userStore.StoreRole.Permissions.Contains(Policies.CanModifyStoreSettings))
+            {
+                if (storeRole.Permissions.Contains(Policies.CanModifyStoreSettings) is false && !await EnsureRemainingOwner(ctx.UserStore, storeId, userId))
+                    return new AddOrUpdateStoreUserResult.LastOwner();
+            }
 
             if (userStore.StoreRoleId == roleId.Id)
-                return false;
+                return new AddOrUpdateStoreUserResult.DuplicateRole(roleId);
             
             userStore.StoreRoleId = roleId.Id;
             try
@@ -340,11 +368,11 @@ namespace BTCPayServer.Services.Stores
                     ? new StoreUserEvent.Added(storeId, userId, userStore.StoreRoleId)
                     : new StoreUserEvent.Updated(storeId, userId, userStore.StoreRoleId);
                 _eventAggregator.Publish(evt);
-                return true;
+                return new AddOrUpdateStoreUserResult.Success();
             }
             catch (DbUpdateException)
             {
-                return false;
+                return new AddOrUpdateStoreUserResult.DuplicateRole(roleId);
             }
         }
 
@@ -357,11 +385,9 @@ namespace BTCPayServer.Services.Stores
         public async Task<bool> RemoveStoreUser(string storeId, string userId)
         {
             await using var ctx = _ContextFactory.CreateContext();
-            if (!await ctx.UserStore.Include(store => store.StoreRole).AnyAsync(store =>
+            if (!await EnsureRemainingOwner(ctx.UserStore, storeId, userId))
-                    store.StoreDataId == storeId && store.StoreRole.Permissions.Contains(Policies.CanModifyStoreSettings) &&
-                    userId != store.ApplicationUserId))
                 return false;
-            var userStore = new UserStore() { StoreDataId = storeId, ApplicationUserId = userId };
+            var userStore = new UserStore { StoreDataId = storeId, ApplicationUserId = userId };
             ctx.UserStore.Add(userStore);
             ctx.Entry(userStore).State = EntityState.Deleted;
             await ctx.SaveChangesAsync();
@@ -369,6 +395,13 @@ namespace BTCPayServer.Services.Stores
             return true;
         }
 
+        private async Task<bool> EnsureRemainingOwner(DbSet<UserStore> userStore, string storeId, string userId)
+        {
+            return await userStore.Include(store => store.StoreRole).AnyAsync(store =>
+                store.StoreDataId == storeId && store.StoreRole.Permissions.Contains(Policies.CanModifyStoreSettings) &&
+                store.ApplicationUserId != userId);
+        }
+
         private async Task DeleteStoreIfOrphan(string storeId)
         {
             await using var ctx = _ContextFactory.CreateContext();

BTCPayServer/Views/UIStores/StoreUsers.cshtml

@@ -27,7 +27,7 @@
             
         @if (!ViewContext.ModelState.IsValid)
         {
-            <div asp-validation-summary="All"></div>
+            <div asp-validation-summary="All" class="@(ViewContext.ModelState.ErrorCount.Equals(1) ? "no-marker" : "")"></div>
         }
 
         <form method="post" class="d-flex flex-wrap align-items-center gap-3" permission="@Policies.CanModifyStoreSettings">