From 1acbf208fe1a51622e52432faca369a0d35f7f86 Mon Sep 17 00:00:00 2001 From: support Date: Sat, 24 Oct 2026 12:12:07 +0200 Subject: [PATCH 1/7] Keep storefront sub-account management behind its setting and owner Hiding the Subaccounts tab only removed the link: the add, edit and delete actions stayed reachable by URL, and a fresh install hides the tab by default. Every sub-account action now goes through one check, so the setting switches the feature off. The edit command trusted the validator for ownership; it now re-checks it and changes nothing for someone else's, a deleted or a missing account. The edit page answers such an id with a redirect instead of an empty form, the validator gives a missing id and a foreign one the same answer, and the delete endpoint returns the handler's message instead of an empty one. A changed e-mail was stored as typed while sign-in looks it up lowercased, so a sub-account whose owner typed capitals could no longer sign in, and a new password saved together with it failed. The e-mail is stored lowercased and, without usernames, the username follows it. Inactive sub-accounts are marked on the owner's list. Hard-coded messages became resources (also the admin ones used by the next commit). --- .../SubAccountDeleteCommandHandlerTests.cs | 70 +++++++ .../SubAccountEditCommandHandlerTests.cs | 155 +++++++++++++-- .../AccountControllerSubAccountsTests.cs | 188 ++++++++++++++++++ .../Customers/GetSubAccountHandlerTests.cs | 58 ++++++ .../CustomerValidatorsStoreScopingTests.cs | 4 +- .../Customer/SubAccountEditValidatorTests.cs | 121 +++++++++++ .../App_Data/Resources/DefaultLanguage.xml | Bin 1590302 -> 1592738 bytes .../App_Data/Resources/Upgrade/en_250.xml | 24 +++ .../SubAccountDeleteCommandHandler.cs | 10 +- .../Customers/SubAccountEditCommandHandler.cs | 17 +- .../Controllers/AccountController.cs | 53 ++--- .../Customers/GetSubAccountHandler.cs | 21 +- .../Customer/SubAccountEditValidator.cs | 26 ++- .../Views/Account/SubAccounts.cshtml | 4 + 14 files changed, 677 insertions(+), 74 deletions(-) create mode 100644 src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountDeleteCommandHandlerTests.cs create mode 100644 src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs create mode 100644 src/Tests/Grand.Web.Tests/Features/Customers/GetSubAccountHandlerTests.cs create mode 100644 src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs diff --git a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountDeleteCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountDeleteCommandHandlerTests.cs new file mode 100644 index 0000000000..393b0d8876 --- /dev/null +++ b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountDeleteCommandHandlerTests.cs @@ -0,0 +1,70 @@ +using Grand.Business.Core.Interfaces.Customers; +using Grand.Domain.Customers; +using Grand.Web.Commands.Handler.Customers; +using Grand.Web.Commands.Models.Customers; +using Grand.Web.Common.Localization; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Tests.Commands.Customers; + +[TestClass] +public class SubAccountDeleteCommandHandlerTests +{ + private Mock _customerServiceMock; + private SubAccountDeleteCommandHandler _handler; + + [TestInitialize] + public void Init() + { + _customerServiceMock = new Mock(); + var translation = new Mock(); + translation.Setup(t => t.GetResource(It.IsAny())).Returns(key => key); + _handler = new SubAccountDeleteCommandHandler(_customerServiceMock.Object, translation.Object); + } + + private static SubAccountDeleteCommand Command(string id) => + new() { CurrentCustomer = new Customer { Id = "owner" }, CustomerId = id }; + + [TestMethod] + public async Task Handle_OwnSubAccount_DeletesIt() + { + var subAccount = new Customer { Id = "sub", OwnerId = "owner" }; + _customerServiceMock.Setup(c => c.GetCustomerById("sub")).ReturnsAsync(subAccount); + + var (success, error) = await _handler.Handle(Command("sub"), default); + + Assert.IsTrue(success); + Assert.AreEqual(string.Empty, error); + _customerServiceMock.Verify(c => c.DeleteCustomer(subAccount, false), Times.Once); + } + + [TestMethod] + [DataRow("another-owner")] + [DataRow("")] + public async Task Handle_SomeoneElsesAccount_DeletesNothing(string ownerId) + { + _customerServiceMock.Setup(c => c.GetCustomerById("victim")) + .ReturnsAsync(new Customer { Id = "victim", OwnerId = ownerId }); + + var (success, error) = await _handler.Handle(Command("victim"), default); + + Assert.IsFalse(success); + Assert.AreEqual("Account.SubAccounts.NotOwner", error); + _customerServiceMock.Verify(c => c.DeleteCustomer(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task Handle_MissingOrDeletedAccount_GivesTheSameAnswer() + { + _customerServiceMock.Setup(c => c.GetCustomerById("gone")) + .ReturnsAsync(new Customer { Id = "gone", OwnerId = "owner", Deleted = true }); + + var deleted = await _handler.Handle(Command("gone"), default); + var missing = await _handler.Handle(Command("missing"), default); + + Assert.AreEqual(missing, deleted); + Assert.IsFalse(deleted.success); + _customerServiceMock.Verify(c => c.DeleteCustomer(It.IsAny(), It.IsAny()), Times.Never); + } +} diff --git a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs index 39337c109b..a27f7fea6d 100644 --- a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs +++ b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs @@ -6,6 +6,7 @@ using Grand.Web.Models.Customer; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; +using System.Linq.Expressions; namespace Grand.Web.Tests.Commands.Customers; @@ -14,6 +15,7 @@ public class SubAccountEditCommandHandlerTests { private Mock _customerServiceMock; private Mock _customerManagerServiceMock; + private CustomerSettings _customerSettings; private SubAccountEditCommandHandler _handler; [TestInitialize] @@ -21,25 +23,44 @@ public void Init() { _customerServiceMock = new Mock(); _customerManagerServiceMock = new Mock(); + _customerSettings = new CustomerSettings { AllowUsersToChangeEmail = true }; _handler = new SubAccountEditCommandHandler(_customerServiceMock.Object, - _customerManagerServiceMock.Object, new CustomerSettings { AllowUsersToChangeEmail = true }); + _customerManagerServiceMock.Object, _customerSettings); } - [TestMethod] - public async Task Handle_WithPassword_ChangesPasswordScopedToCustomerStore() + private Customer SubAccount(string ownerId = "owner") { - var customer = new Customer { Id = "c1", Email = "sub@x.com", StoreId = "store-9" }; + var customer = new Customer { + Id = "c1", Email = "sub@x.com", Username = "sub@x.com", StoreId = "store-9", OwnerId = ownerId + }; _customerServiceMock.Setup(c => c.GetCustomerById("c1")).ReturnsAsync(customer); + return customer; + } - var command = new SubAccountEditCommand { - CurrentCustomer = new Customer { Id = "owner" }, - EditModel = new SubAccountEditModel { - Id = "c1", Email = "sub@x.com", Password = "newpass", Active = true, - FirstName = "John", LastName = "Doe" - } - }; + private static SubAccountEditCommand Command(string email = "sub@x.com", string password = null) => new() { + CurrentCustomer = new Customer { Id = "owner" }, + EditModel = new SubAccountEditModel { + Id = "c1", Email = email, Password = password, Active = true, FirstName = "John", LastName = "Doe" + } + }; + + private void VerifyNothingWritten() + { + _customerServiceMock.Verify(c => c.UpdateCustomerField(It.IsAny(), + It.IsAny>>(), It.IsAny()), Times.Never); + _customerServiceMock.Verify(c => c.UpdateActive(It.IsAny()), Times.Never); + _customerServiceMock.Verify(c => c.UpdateUserField(It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny()), Times.Never); + _customerManagerServiceMock.Verify(m => m.ChangePassword(It.IsAny(), + It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task Handle_WithPassword_ChangesPasswordScopedToCustomerStore() + { + SubAccount(); - var result = await _handler.Handle(command, default); + var result = await _handler.Handle(Command(password: "newpass"), default); Assert.IsTrue(result); //the sub-account's store must scope the password change (safe with or without per-store identity) @@ -50,18 +71,112 @@ public async Task Handle_WithPassword_ChangesPasswordScopedToCustomerStore() [TestMethod] public async Task Handle_WithoutPassword_DoesNotChangePassword() { - var customer = new Customer { Id = "c1", Email = "sub@x.com", StoreId = "store-9" }; - _customerServiceMock.Setup(c => c.GetCustomerById("c1")).ReturnsAsync(customer); + SubAccount(); - var command = new SubAccountEditCommand { - CurrentCustomer = new Customer { Id = "owner" }, - EditModel = new SubAccountEditModel { Id = "c1", Email = "sub@x.com", Active = true } - }; - - var result = await _handler.Handle(command, default); + var result = await _handler.Handle(Command(), default); Assert.IsTrue(result); _customerManagerServiceMock.Verify(m => m.ChangePassword( It.IsAny(), It.IsAny()), Times.Never); } + + [TestMethod] + public async Task Handle_SomeoneElsesAccount_ChangesNothing() + { + SubAccount(ownerId: "another-owner"); + + var result = await _handler.Handle(Command(email: "taken@x.com", password: "newpass"), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_AccountWithoutOwner_ChangesNothing() + { + //any main account's id, the current owner's own included: it has no owner, so it is nobody's sub-account + SubAccount(ownerId: ""); + + var result = await _handler.Handle(Command(), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_DeletedAccount_ChangesNothing() + { + SubAccount().Deleted = true; + + var result = await _handler.Handle(Command(), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_MissingAccount_ChangesNothing() + { + var result = await _handler.Handle(Command(), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_MixedCaseEmail_IsStoredLowercased() + { + //stored as typed, the lowercased login lookup missed it and the sub-account could no longer sign in + var customer = SubAccount(); + + await _handler.Handle(Command(email: " New.Sub@X.com "), default); + + Assert.AreEqual("new.sub@x.com", customer.Email); + _customerServiceMock.Verify(c => c.UpdateCustomerField(customer, + It.IsAny>>(), "new.sub@x.com"), Times.AtLeastOnce); + } + + [TestMethod] + public async Task Handle_NewEmailAndPassword_ChangesPasswordOfTheNewEmail() + { + SubAccount(); + + await _handler.Handle(Command(email: "New@X.com", password: "newpass"), default); + + _customerManagerServiceMock.Verify(m => m.ChangePassword( + It.Is(r => r.Email == "new@x.com"), "store-9"), Times.Once); + } + + [TestMethod] + public async Task Handle_NewEmail_UsernamesDisabled_KeepsUsernameEqualToEmail() + { + var customer = SubAccount(); + + await _handler.Handle(Command(email: "new@x.com"), default); + + Assert.AreEqual("new@x.com", customer.Username); + } + + [TestMethod] + public async Task Handle_NewEmail_UsernamesEnabled_LeavesUsername() + { + _customerSettings.UsernamesEnabled = true; + var customer = SubAccount(); + + await _handler.Handle(Command(email: "new@x.com"), default); + + Assert.AreEqual("new@x.com", customer.Email); + Assert.AreEqual("sub@x.com", customer.Username); + } + + [TestMethod] + public async Task Handle_EmailChangeNotAllowed_KeepsEmail() + { + _customerSettings.AllowUsersToChangeEmail = false; + var customer = SubAccount(); + + await _handler.Handle(Command(email: "new@x.com"), default); + + Assert.AreEqual("sub@x.com", customer.Email); + } } diff --git a/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs b/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs new file mode 100644 index 0000000000..b96c8bfde4 --- /dev/null +++ b/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs @@ -0,0 +1,188 @@ +using Grand.Business.Core.Interfaces.Authentication; +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Customers; +using Grand.Business.Core.Interfaces.Messages; +using Grand.Domain.Common; +using Grand.Domain.Customers; +using Grand.Infrastructure; +using Grand.Infrastructure.Configuration; +using Grand.Mediator; +using Grand.Web.Commands.Models.Customers; +using Grand.Web.Common.Localization; +using Grand.Web.Common.Security.Authentication; +using Grand.Web.Controllers; +using Grand.Web.Features.Models.Customers; +using Grand.Web.Models.Customer; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Routing; +using Microsoft.AspNetCore.Mvc.ViewFeatures; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Tests.Controllers; + +[TestClass] +public class AccountControllerSubAccountsTests +{ + private Customer _currentCustomer; + private CustomerSettings _customerSettings; + private Mock _mediator; + private AccountController _controller; + + [TestInitialize] + public void Init() + { + _currentCustomer = new Customer { Id = "owner" }; + var work = new Mock(); + work.Setup(w => w.CurrentCustomer).Returns(() => _currentCustomer); + var store = new Mock(); + store.Setup(s => s.CurrentStore).Returns(new Grand.Domain.Stores.Store { Id = "s1" }); + var context = new Mock(); + context.Setup(c => c.WorkContext).Returns(work.Object); + context.Setup(c => c.StoreContext).Returns(store.Object); + + //the real rule: an account without an owner is an owner + var groups = new Mock(); + groups.Setup(g => g.IsOwner(It.IsAny())) + .ReturnsAsync((Customer c) => string.IsNullOrEmpty(c.OwnerId)); + + _mediator = new Mock(); + _customerSettings = new CustomerSettings(); + _controller = new AccountController(new Mock().Object, + new Mock().Object, context.Object, new Mock().Object, + groups.Object, new Mock().Object, new Mock().Object, + _mediator.Object, new Mock().Object, new CaptchaSettings(), + _customerSettings, new CustomerConfig()); + _controller.ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() }; + _controller.TempData = new TempDataDictionary(_controller.HttpContext, new Mock().Object); + var url = new Mock(); + url.Setup(x => x.RouteUrl(It.IsAny())).Returns("/subaccounts"); + _controller.Url = url.Object; + } + + private static void AssertRedirectTo(IActionResult result, string routeName) + { + Assert.IsInstanceOfType(result, typeof(RedirectToRouteResult)); + Assert.AreEqual(routeName, ((RedirectToRouteResult)result).RouteName); + } + + private static object JsonValue(IActionResult result, string property) + { + var value = ((JsonResult)result).Value!; + return value.GetType().GetProperty(property)!.GetValue(value); + } + + //hiding the tab switches the feature off - the actions behind it used to stay reachable by URL + + [TestMethod] + public async Task SubAccountAddPost_TabHidden_CreatesNothing() + { + _customerSettings.HideSubAccountsTab = true; + + var result = await _controller.SubAccountAdd(new SubAccountCreateModel { Email = "a@b.com" }); + + AssertRedirectTo(result, "CustomerInfo"); + _mediator.Verify(m => m.Send(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task SubAccountEditPost_TabHidden_ChangesNothing() + { + _customerSettings.HideSubAccountsTab = true; + + var result = await _controller.SubAccountEdit(new SubAccountEditModel { Id = "sub" }); + + AssertRedirectTo(result, "CustomerInfo"); + _mediator.Verify(m => m.Send(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task SubAccountDelete_TabHidden_DeletesNothing() + { + _customerSettings.HideSubAccountsTab = true; + + var result = await _controller.SubAccountDelete("sub"); + + AssertRedirectTo(result, "CustomerInfo"); + _mediator.Verify(m => m.Send(It.IsAny(), It.IsAny()), + Times.Never); + } + + [TestMethod] + public async Task SubAccountGetActions_TabHidden_RedirectToCustomerInfo() + { + _customerSettings.HideSubAccountsTab = true; + + AssertRedirectTo(await _controller.SubAccounts(), "CustomerInfo"); + AssertRedirectTo(await _controller.SubAccountAdd(), "CustomerInfo"); + AssertRedirectTo(await _controller.SubAccountEdit("sub"), "CustomerInfo"); + } + + [TestMethod] + public async Task SubAccountAddPost_CalledBySubAccount_IsChallenged() + { + _currentCustomer.OwnerId = "someone"; + + var result = await _controller.SubAccountAdd(new SubAccountCreateModel { Email = "a@b.com" }); + + Assert.IsInstanceOfType(result, typeof(ChallengeResult)); + _mediator.Verify(m => m.Send(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task SubAccountDelete_CalledBySubAccount_IsChallenged() + { + _currentCustomer.OwnerId = "someone"; + + var result = await _controller.SubAccountDelete("sub"); + + Assert.IsInstanceOfType(result, typeof(ChallengeResult)); + } + + [TestMethod] + public async Task SubAccountEditGet_SomeoneElsesAccount_RedirectsToList() + { + _mediator.Setup(m => m.Send(It.IsAny(), It.IsAny())) + .ReturnsAsync((SubAccountEditModel)null); + + var result = await _controller.SubAccountEdit("foreign"); + + AssertRedirectTo(result, "CustomerSubAccounts"); + } + + [TestMethod] + public async Task SubAccountEditGet_OwnAccount_ShowsIt() + { + var model = new SubAccountEditModel { Id = "sub" }; + _mediator.Setup(m => m.Send(It.IsAny(), It.IsAny())).ReturnsAsync(model); + + var result = await _controller.SubAccountEdit("sub"); + + Assert.AreSame(model, ((ViewResult)result).Model); + } + + [TestMethod] + public async Task SubAccountDelete_Refused_ReturnsTheHandlersError() + { + _mediator.Setup(m => m.Send(It.IsAny(), It.IsAny())) + .ReturnsAsync((false, "not yours")); + + var result = await _controller.SubAccountDelete("foreign"); + + Assert.AreEqual(false, JsonValue(result, "success")); + Assert.AreEqual("not yours", JsonValue(result, "error")); + } + + [TestMethod] + public async Task SubAccountDelete_Done_ReportsSuccess() + { + _mediator.Setup(m => m.Send(It.IsAny(), It.IsAny())) + .ReturnsAsync((true, string.Empty)); + + var result = await _controller.SubAccountDelete("sub"); + + Assert.AreEqual(true, JsonValue(result, "success")); + Assert.AreEqual("/subaccounts", JsonValue(result, "redirect")); + } +} diff --git a/src/Tests/Grand.Web.Tests/Features/Customers/GetSubAccountHandlerTests.cs b/src/Tests/Grand.Web.Tests/Features/Customers/GetSubAccountHandlerTests.cs new file mode 100644 index 0000000000..658558cd9f --- /dev/null +++ b/src/Tests/Grand.Web.Tests/Features/Customers/GetSubAccountHandlerTests.cs @@ -0,0 +1,58 @@ +using Grand.Business.Core.Interfaces.Customers; +using Grand.Domain.Customers; +using Grand.Web.Features.Handlers.Customers; +using Grand.Web.Features.Models.Customers; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Tests.Features.Customers; + +[TestClass] +public class GetSubAccountHandlerTests +{ + private Mock _customerServiceMock; + private GetSubAccountHandler _handler; + + [TestInitialize] + public void Init() + { + _customerServiceMock = new Mock(); + _handler = new GetSubAccountHandler(_customerServiceMock.Object); + } + + private static GetSubAccount Query(string id) => + new() { CurrentCustomer = new Customer { Id = "owner" }, CustomerId = id }; + + [TestMethod] + public async Task Handle_OwnSubAccount_ReturnsIt() + { + _customerServiceMock.Setup(c => c.GetCustomerById("sub")) + .ReturnsAsync(new Customer { Id = "sub", OwnerId = "owner", Email = "sub@x.com", Active = true }); + + var model = await _handler.Handle(Query("sub"), default); + + Assert.IsNotNull(model); + Assert.AreEqual("sub", model.Id); + Assert.AreEqual("sub@x.com", model.Email); + Assert.IsTrue(model.Active); + } + + [TestMethod] + [DataRow("another-owner", false)] + [DataRow("", false)] + [DataRow("owner", true)] + public async Task Handle_NotAnOwnLiveSubAccount_ReturnsNull(string ownerId, bool deleted) + { + //an empty model used to be returned, which rendered as a blank edit form + _customerServiceMock.Setup(c => c.GetCustomerById("x")) + .ReturnsAsync(new Customer { Id = "x", OwnerId = ownerId, Deleted = deleted, Email = "x@x.com" }); + + Assert.IsNull(await _handler.Handle(Query("x"), default)); + } + + [TestMethod] + public async Task Handle_MissingAccount_ReturnsNull() + { + Assert.IsNull(await _handler.Handle(Query("missing"), default)); + } +} diff --git a/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs b/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs index 2bdfc98ec0..bf2ec099db 100644 --- a/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs +++ b/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs @@ -38,7 +38,7 @@ private static Mock Translation() private static IContextAccessor Context(string currentCustomerEmail = "owner@x.com") { var work = new Mock(); - work.Setup(w => w.CurrentCustomer).Returns(new Customer { Email = currentCustomerEmail }); + work.Setup(w => w.CurrentCustomer).Returns(new Customer { Id = "owner", Email = currentCustomerEmail }); var store = new Mock(); store.Setup(s => s.CurrentStore).Returns(new Grand.Domain.Stores.Store { Id = StoreId }); var ctx = new Mock(); @@ -176,7 +176,7 @@ public async Task SubAccountEditValidator_ScopesEmailLookupToCurrentStore_WhenEm { var customerService = new Mock(); customerService.Setup(c => c.GetCustomerById("c1")) - .ReturnsAsync(new Customer { Id = "c1", Email = "old@x.com" }); + .ReturnsAsync(new Customer { Id = "c1", Email = "old@x.com", OwnerId = "owner" }); var groupService = new Mock(); groupService.Setup(g => g.IsRegistered(It.IsAny())).ReturnsAsync(true); diff --git a/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs b/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs new file mode 100644 index 0000000000..cb9f3d862d --- /dev/null +++ b/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs @@ -0,0 +1,121 @@ +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Customers; +using Grand.Domain.Customers; +using Grand.Infrastructure; +using Grand.Infrastructure.Configuration; +using Grand.Infrastructure.Validators; +using Grand.Web.Common.Localization; +using Grand.Web.Models.Customer; +using Grand.Web.Validators.Customer; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Tests.Validators.Customers; + +[TestClass] +public class SubAccountEditValidatorTests +{ + private Mock _customerService; + private Mock _groupService; + private SubAccountEditValidator _validator; + + [TestInitialize] + public void Init() + { + _customerService = new Mock(); + _groupService = new Mock(); + _groupService.Setup(g => g.IsRegistered(It.IsAny())).ReturnsAsync(true); + var translation = new Mock(); + translation.Setup(t => t.GetResource(It.IsAny())).Returns(k => k); + + var work = new Mock(); + work.Setup(w => w.CurrentCustomer).Returns(new Customer { Id = "owner" }); + var store = new Mock(); + store.Setup(s => s.CurrentStore).Returns(new Grand.Domain.Stores.Store { Id = "s1" }); + var context = new Mock(); + context.Setup(c => c.WorkContext).Returns(work.Object); + context.Setup(c => c.StoreContext).Returns(store.Object); + + _validator = new SubAccountEditValidator(new List>(), + _customerService.Object, _groupService.Object, translation.Object, context.Object, + new CustomerSettings { AllowUsersToChangeEmail = true }, new CustomerConfig()); + } + + private static SubAccountEditModel Model(string email = "new@x.com") => + new() { Id = "c1", Email = email, FirstName = "A", LastName = "B" }; + + [TestMethod] + public async Task SomeoneElsesAccount_GetsOnlyTheNotOwnerAnswer() + { + //an unregistered account of another owner: its state must not show through + var victim = new Customer { Id = "c1", OwnerId = "another-owner", Email = "victim@x.com" }; + _customerService.Setup(c => c.GetCustomerById("c1")).ReturnsAsync(victim); + _groupService.Setup(g => g.IsRegistered(victim)).ReturnsAsync(false); + + var result = await _validator.ValidateAsync(Model()); + + CollectionAssert.AreEqual(new[] { "Account.SubAccounts.NotOwner" }, + result.Errors.Select(e => e.ErrorMessage).ToArray()); + //and no probing of whether the new email is taken + _customerService.Verify(c => c.GetCustomerByEmail(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task MissingAccount_GetsTheSameAnswerAsSomeoneElses() + { + //otherwise the answer tells which customer ids exist + var result = await _validator.ValidateAsync(Model()); + + CollectionAssert.AreEqual(new[] { "Account.SubAccounts.NotOwner" }, + result.Errors.Select(e => e.ErrorMessage).ToArray()); + } + + [TestMethod] + public async Task NotRegisteredSubAccount_UsesTheResourceNotARawKey() + { + var sub = new Customer { Id = "c1", OwnerId = "owner", Email = "sub@x.com" }; + _customerService.Setup(c => c.GetCustomerById("c1")).ReturnsAsync(sub); + _groupService.Setup(g => g.IsRegistered(sub)).ReturnsAsync(false); + + var result = await _validator.ValidateAsync(Model()); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.Login.WrongCredentials.NotRegistered")); + } + + [TestMethod] + public async Task EmptyEmail_FailsValidationWithoutThrowing() + { + _customerService.Setup(c => c.GetCustomerById("c1")) + .ReturnsAsync(new Customer { Id = "c1", OwnerId = "owner", Email = "sub@x.com" }); + + var result = await _validator.ValidateAsync(Model(email: null)); + + Assert.IsFalse(result.IsValid); + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.Fields.Email.Required")); + } + + [TestMethod] + public async Task SameEmailInOtherCase_IsNotAnEmailChange() + { + _customerService.Setup(c => c.GetCustomerById("c1")) + .ReturnsAsync(new Customer { Id = "c1", OwnerId = "owner", Email = "sub@x.com" }); + + var result = await _validator.ValidateAsync(Model(email: "Sub@X.com")); + + Assert.IsTrue(result.IsValid); + _customerService.Verify(c => c.GetCustomerByEmail(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task NewEmailTakenByAnotherCustomer_Fails() + { + _customerService.Setup(c => c.GetCustomerById("c1")) + .ReturnsAsync(new Customer { Id = "c1", OwnerId = "owner", Email = "sub@x.com" }); + _customerService.Setup(c => c.GetCustomerByEmail("new@x.com", It.IsAny())) + .ReturnsAsync(new Customer { Id = "other" }); + + var result = await _validator.ValidateAsync(Model()); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.EmailAlreadyExists")); + } +} diff --git a/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml b/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml index 9c81057e3ee07061b3d77f9b3edd891a08be68e1..69a851d0656681db760992a5b6150ea1e3798c86 100644 GIT binary patch delta 610 zcmY+=PiWIn90%~1Hc8)0(>808u5+$tV;gl$S+^b(hSUjq-C+#zWac(jaqYTRs_4N# zhwi96=+7ZY86v_Idh0=u2qF_N6+FxIq&rQJre4HfvU%9ylZ3qFeZTMbY{TAqYByiH z!8|GOO#gY9Cr9fGMN%M3#3DtqjMv+Eze(2lFSieN-pA&7?oS#n51Cm=O*8qS)2%sO0R>PHiAX{+ z3bmXrojVn@Ntp{B6QU3FI9NME8g?HW6!xZ=R=*Ub{I{Dh|L5gNg_QmDDk(teoqD2v zMH0i;u*Rxi%f>3bTx-sQ)z5M;Ho}yaHp*@Z{-bo7*SPJ$ z_Dkf3f4IZFeu{kYuK{;6$tW~NSt3#)D}L50u}Plaf2AMIgN2%O5uC{|PHb9>Lwlbp zaBf&p`18HvA&XS;w=#UbBNyuf*uksFx4w*fM;~WJT=oXM4kb=?!G>=``bk)R9lHwc z5jGPl;~KVu%)P)_c)L&IyrLSBqhooq(PP8z0Gk1AJ0#X8Y10d8t)jR*q@XZTQ3OR%3~8tb#gUE-WTHfiZuTa&{{aG^$@TyM delta 167 zcmZ29J8@n|;)WI9Czt&=(j4%sJ>VB35HkTWGZ3=?F)I+W0WtgbfL|Odbhp=Oa+WYo ze_+h9W%>j*PLpX*1h}UEImWqRdI1-w5|F1eUEwgZ+VlzU1WKkKP~>#j{=tZITV8w5 tB2FOY0%C3;<^f_}Am#&NejpYAVnHAl0%Bnx76D>WAQs!+vq)TQH2|zvOM3tS diff --git a/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml b/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml index c3a246c9ab..5262947a25 100644 --- a/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml +++ b/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml @@ -198,4 +198,28 @@ The request failed. Try again. + + Inactive + + + This account is not one of your subaccounts. + + + This customer has subaccounts of its own, so it cannot become a subaccount. + + + This customer is itself a subaccount. Enter the e-mail address of the main account. + + + No customer with this e-mail address was found. + + + The owner must be a registered customer. + + + A customer cannot be its own owner. + + + Subaccounts + diff --git a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs index 31026f5bfc..32d6ae227e 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs @@ -1,5 +1,6 @@ using Grand.Business.Core.Interfaces.Customers; using Grand.Web.Commands.Models.Customers; +using Grand.Web.Common.Localization; using Grand.Mediator; namespace Grand.Web.Commands.Handler.Customers; @@ -7,10 +8,13 @@ namespace Grand.Web.Commands.Handler.Customers; public class SubAccountDeleteCommandHandler : IRequestHandler { private readonly ICustomerService _customerService; + private readonly IRequestTranslationService _translationService; - public SubAccountDeleteCommandHandler(ICustomerService customerService) + public SubAccountDeleteCommandHandler(ICustomerService customerService, + IRequestTranslationService translationService) { _customerService = customerService; + _translationService = translationService; } public async Task<(bool success, string error)> Handle(SubAccountDeleteCommand request, @@ -19,8 +23,8 @@ public SubAccountDeleteCommandHandler(ICustomerService customerService) ArgumentNullException.ThrowIfNull(request.CurrentCustomer); var customer = await _customerService.GetCustomerById(request.CustomerId); - if (customer == null || customer.OwnerId != request.CurrentCustomer.Id) - return (false, "You are not owner of this account"); + if (customer == null || customer.Deleted || customer.OwnerId != request.CurrentCustomer.Id) + return (false, _translationService.GetResource("Account.SubAccounts.NotOwner")); //delete customer await _customerService.DeleteCustomer(customer); diff --git a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs index 32555ba9c5..b0a2b721a9 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs @@ -26,12 +26,21 @@ public async Task Handle(SubAccountEditCommand request, CancellationToken { ArgumentNullException.ThrowIfNull(request.CurrentCustomer); var customer = await _customerService.GetCustomerById(request.EditModel.Id); + if (customer == null || customer.Deleted || customer.OwnerId != request.CurrentCustomer.Id) + return false; - //update email - if (customer.Email != request.EditModel.Email.ToLower() && _customerSettings.AllowUsersToChangeEmail) + //update email - stored lowercased like every other write, or the lowercased login lookup misses it + var email = request.EditModel.Email.Trim().ToLowerInvariant(); + if (customer.Email != email && _customerSettings.AllowUsersToChangeEmail) { - customer.Email = request.EditModel.Email; - await _customerService.UpdateCustomerField(customer, x => x.Email, request.EditModel.Email); + customer.Email = email; + await _customerService.UpdateCustomerField(customer, x => x.Email, email); + //without usernames the username is the email (see SubAccountAddCommandHandler) + if (!_customerSettings.UsernamesEnabled) + { + customer.Username = email; + await _customerService.UpdateCustomerField(customer, x => x.Username, email); + } } //update password (scope the lookup to the sub-account's store - safe with or without per-store identity) diff --git a/src/Web/Grand.Web/Controllers/AccountController.cs b/src/Web/Grand.Web/Controllers/AccountController.cs index 9061918f11..8c2755ebb0 100644 --- a/src/Web/Grand.Web/Controllers/AccountController.cs +++ b/src/Web/Grand.Web/Controllers/AccountController.cs @@ -1310,9 +1310,11 @@ await _customerService.UpdateUserField(_contextAccessor.WorkContext.Curr #region My account / Sub accounts - [HttpGet] - [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] - public virtual async Task SubAccounts() + /// + /// Null when the current customer may manage sub-accounts, otherwise the result to return. Every + /// sub-account action goes through it, so hiding the tab switches the feature off and not only the link. + /// + private async Task SubAccountsDenied() { if (!await _groupService.IsOwner(_contextAccessor.WorkContext.CurrentCustomer)) return Challenge(); @@ -1320,6 +1322,16 @@ public virtual async Task SubAccounts() if (_customerSettings.HideSubAccountsTab) return RedirectToRoute("CustomerInfo"); + return null; + } + + [HttpGet] + [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] + public virtual async Task SubAccounts() + { + var denied = await SubAccountsDenied(); + if (denied != null) return denied; + var model = await _mediator.Send(new GetSubAccounts { Customer = _contextAccessor.WorkContext.CurrentCustomer }); return View(model); @@ -1329,8 +1341,8 @@ public virtual async Task SubAccounts() [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] public virtual async Task SubAccountAdd() { - if (!await _groupService.IsOwner(_contextAccessor.WorkContext.CurrentCustomer)) - return Challenge(); + var denied = await SubAccountsDenied(); + if (denied != null) return denied; var model = new SubAccountCreateModel { Active = true @@ -1342,8 +1354,8 @@ public virtual async Task SubAccountAdd() [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] public virtual async Task SubAccountAdd(SubAccountCreateModel model) { - if (!await _groupService.IsOwner(_contextAccessor.WorkContext.CurrentCustomer)) - return Challenge(); + var denied = await SubAccountsDenied(); + if (denied != null) return denied; if (!ModelState.IsValid) return View(model); @@ -1360,11 +1372,13 @@ await _mediator.Send(new SubAccountAddCommand { [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] public virtual async Task SubAccountEdit(string id) { - if (!await _groupService.IsOwner(_contextAccessor.WorkContext.CurrentCustomer)) - return Challenge(); + var denied = await SubAccountsDenied(); + if (denied != null) return denied; var model = await _mediator.Send(new GetSubAccount { CustomerId = id, CurrentCustomer = _contextAccessor.WorkContext.CurrentCustomer }); + if (model == null) + return RedirectToRoute("CustomerSubAccounts"); return View(model); } @@ -1373,11 +1387,12 @@ public virtual async Task SubAccountEdit(string id) [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] public virtual async Task SubAccountEdit(SubAccountEditModel model) { - if (!await _groupService.IsOwner(_contextAccessor.WorkContext.CurrentCustomer)) - return Challenge(); + var denied = await SubAccountsDenied(); + if (denied != null) return denied; if (!ModelState.IsValid) return View(model); + //the handler re-checks the ownership the validator already checked and changes nothing when it fails _ = await _mediator.Send(new SubAccountEditCommand { CurrentCustomer = _contextAccessor.WorkContext.CurrentCustomer, EditModel = model, @@ -1391,26 +1406,18 @@ public virtual async Task SubAccountEdit(SubAccountEditModel mode [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] public virtual async Task SubAccountDelete(string id) { - if (!await _groupService.IsOwner(_contextAccessor.WorkContext.CurrentCustomer)) - return Challenge(); + var denied = await SubAccountsDenied(); + if (denied != null) return denied; var result = await _mediator.Send(new SubAccountDeleteCommand { CurrentCustomer = _contextAccessor.WorkContext.CurrentCustomer, CustomerId = id }); - if (result.success) - return Json(new { - redirect = Url.RouteUrl("CustomerSubAccounts"), - success = true - }); - //errors return Json(new { redirect = Url.RouteUrl("CustomerSubAccounts"), - success = false, - error = string.Join("; ", ModelState.Values - .SelectMany(x => x.Errors) - .Select(x => x.ErrorMessage)) + success = result.success, + error = result.error }); } diff --git a/src/Web/Grand.Web/Features/Handlers/Customers/GetSubAccountHandler.cs b/src/Web/Grand.Web/Features/Handlers/Customers/GetSubAccountHandler.cs index 0be4202e6a..45e11bf93f 100644 --- a/src/Web/Grand.Web/Features/Handlers/Customers/GetSubAccountHandler.cs +++ b/src/Web/Grand.Web/Features/Handlers/Customers/GetSubAccountHandler.cs @@ -20,18 +20,17 @@ public async Task Handle(GetSubAccount request, Cancellatio { ArgumentNullException.ThrowIfNull(request.CurrentCustomer); - var model = new SubAccountEditModel(); - + //null for a missing, deleted or someone else's account, so the caller can tell it from an empty form var subaccount = await _customerService.GetCustomerById(request.CustomerId); - if (subaccount != null && subaccount.OwnerId == request.CurrentCustomer.Id) - model = new SubAccountEditModel { - Id = subaccount.Id, - Email = subaccount.Email, - Active = subaccount.Active, - FirstName = subaccount.GetUserFieldFromEntity(SystemCustomerFieldNames.FirstName), - LastName = subaccount.GetUserFieldFromEntity(SystemCustomerFieldNames.LastName) - }; + if (subaccount == null || subaccount.Deleted || subaccount.OwnerId != request.CurrentCustomer.Id) + return null; - return model; + return new SubAccountEditModel { + Id = subaccount.Id, + Email = subaccount.Email, + Active = subaccount.Active, + FirstName = subaccount.GetUserFieldFromEntity(SystemCustomerFieldNames.FirstName), + LastName = subaccount.GetUserFieldFromEntity(SystemCustomerFieldNames.LastName) + }; } } \ No newline at end of file diff --git a/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs b/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs index 3e66f36527..ccc87d4757 100644 --- a/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs @@ -43,27 +43,31 @@ public SubAccountEditValidator( { var customer = await customerService.GetCustomerById(x.Id); + //ownership first: a missing id and someone else's account get the same answer whatever its state, + //so the answer reveals neither which ids exist nor what state they are in + if (customer == null || customer.OwnerId != contextAccessor.WorkContext.CurrentCustomer.Id) + { + context.AddFailure(translationService.GetResource("Account.SubAccounts.NotOwner")); + return; + } + switch (customer) { - case null: - context.AddFailure( - translationService.GetResource("Account.NoExists")); - break; case { Deleted: true }: context.AddFailure( translationService.GetResource("Account.DeleteAccount")); - break; + return; case not null when !await groupService.IsRegistered(customer): context.AddFailure( - translationService.GetResource( - "Account is not registered")); - break; + translationService.GetResource("Account.Login.WrongCredentials.NotRegistered")); + return; } - if (customer != null && customer.OwnerId != contextAccessor.WorkContext.CurrentCustomer.Id) - context.AddFailure("You are not owner of this account"); + //an empty email is already reported by the NotEmpty rule + if (string.IsNullOrWhiteSpace(x.Email)) + return; - if (customer != null && customer.Email != x.Email.ToLower() && customerSettings.AllowUsersToChangeEmail) + if (customer.Email != x.Email.Trim().ToLowerInvariant() && customerSettings.AllowUsersToChangeEmail) { if (!CommonHelper.IsValidEmail(x.Email)) context.AddFailure( diff --git a/src/Web/Grand.Web/Views/Account/SubAccounts.cshtml b/src/Web/Grand.Web/Views/Account/SubAccounts.cshtml index 0a61af4bc6..59bbd3f0ef 100644 --- a/src/Web/Grand.Web/Views/Account/SubAccounts.cshtml +++ b/src/Web/Grand.Web/Views/Account/SubAccounts.cshtml @@ -31,6 +31,10 @@
@account.FirstName @account.LastName + @if (!account.Active) + { + @Loc["Account.SubAccounts.Inactive"] + }
From bf0c3b49071da2e029e46f4e021fc6db4c97b995 Mon Sep 17 00:00:00 2001 From: support Date: Sat, 24 Oct 2026 12:12:10 +0200 Subject: [PATCH 2/7] Validate sub-account owners in the admin and keep them on Store saves The Store panel blanked the owner on every customer save, which detached each sub-account a store manager touched. An existing customer now keeps the owner it has; a new one still gets none, and the posted value is never trusted. The admin owner field only checked that the e-mail existed. Sub-accounts stay one level deep now: the owner must be a registered, live customer that is not itself a sub-account, a customer with sub-accounts cannot get an owner, and a customer cannot own itself. The lookup on save uses the same store scope as the validator (it was global, so with per-store identity it could link another store's account), and an owner already linked is kept when posted back unchanged, so legacy data never blocks an unrelated save. The customer form lists an owner's sub-accounts and links a sub-account's owner, read only, on both panels and within each panel's data scope. --- .../Services/CustomerViewModelServiceTests.cs | 166 ++++++++++++- .../Validators/CustomerValidatorOwnerTests.cs | 221 ++++++++++++++++++ .../Controllers/CustomerControllerTests.cs | 29 +++ .../Models/Customers/CustomerModel.cs | 14 ++ .../Services/CustomerViewModelService.cs | 90 +++++-- .../Validators/Customers/CustomerValidator.cs | 67 ++++-- .../Partials/CreateOrUpdate.TabInfo.cshtml | 33 +++ .../Controllers/CustomerController.cs | 8 +- 8 files changed, 593 insertions(+), 35 deletions(-) create mode 100644 src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs diff --git a/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs b/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs index 17ce621e1e..9c642de595 100644 --- a/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs +++ b/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs @@ -19,6 +19,8 @@ using Grand.Domain.Vendors; using Grand.Domain.Messages; using Grand.Infrastructure; +using Grand.Infrastructure.Configuration; +using Grand.Web.AdminShared.Interfaces; using Grand.Web.AdminShared.Models.Customers; using Grand.Web.AdminShared.Services; using Grand.Web.Common.Localization; @@ -46,6 +48,8 @@ public class CustomerViewModelServiceTests private Mock _newsLetterSubscriptionServiceMock; private Mock _translationServiceMock; private Customer _currentCustomer; + private CustomerConfig _customerConfig; + private Mock> _scopeMock; private CustomerViewModelService _customerViewModelService; [TestInitialize] @@ -114,6 +118,13 @@ public void Setup() _groupServiceMock.Setup(g => g.GetAllByIds(It.IsAny())) .ReturnsAsync(new List()); + SetupGetAllCustomers(new PagedList()); + _customerConfig = new CustomerConfig(); + //Admin panel: no default store, every customer in reach + _scopeMock = new Mock>(); + _scopeMock.Setup(s => s.GetDefaultStoreIdAsync()).ReturnsAsync((string)null); + _scopeMock.Setup(s => s.HasAccess(It.IsAny())).ReturnsAsync(true); + _customerViewModelService = new CustomerViewModelService( _customerServiceMock.Object, _groupServiceMock.Object, @@ -133,7 +144,9 @@ public void Setup() new TaxSettings(), new LoyaltyPointsSettings(), new CommonSettings(), - _enumTranslationServiceMock.Object); + _enumTranslationServiceMock.Object, + _customerConfig, + _scopeMock.Object); } [TestMethod] @@ -432,6 +445,157 @@ public async Task PrepareCustomerList_NoCustomers_ReturnsEmpty() Assert.AreEqual(0, list.Count()); } + #region Owner and sub-accounts + + [TestMethod] + public async Task UpdateCustomerModel_OwnerEmailUnchanged_KeepsTheLinkedOwner() + { + //posted back as shown, the owner is kept even where an email lookup would find someone else + var owner = new Customer { Id = "owner-1", Email = "boss@firm.com" }; + _customerServiceMock.Setup(c => c.GetCustomerById("owner-1")).ReturnsAsync(owner); + _customerServiceMock.Setup(c => c.GetCustomerByEmail("boss@firm.com", It.IsAny())) + .ReturnsAsync(new Customer { Id = "same-email-other-store" }); + var customer = new Customer { Id = "c1", OwnerId = "owner-1" }; + + await _customerViewModelService.UpdateCustomerModel(customer, + new CustomerModel { Email = "c1@firm.com", Owner = "Boss@Firm.com " }); + + Assert.AreEqual("owner-1", customer.OwnerId); + } + + [TestMethod] + public async Task UpdateCustomerModel_NewOwner_PerStore_IsLookedUpInTheCustomersStore() + { + _customerConfig.RegisterCustomersPerStore = true; + _customerServiceMock.Setup(c => c.GetCustomerByEmail("boss@firm.com", "store-2")) + .ReturnsAsync(new Customer { Id = "owner-store-2" }); + var customer = new Customer { Id = "c1" }; + + await _customerViewModelService.UpdateCustomerModel(customer, + new CustomerModel { Email = "c1@firm.com", StoreId = "store-2", Owner = "boss@firm.com" }); + + //the same scope the validator checks in - the global lookup used to pick any store's account + Assert.AreEqual("owner-store-2", customer.OwnerId); + _customerServiceMock.Verify(c => c.GetCustomerByEmail("boss@firm.com", ""), Times.Never); + } + + [TestMethod] + public async Task UpdateCustomerModel_OwnerCleared_UnlinksTheOwner() + { + var customer = new Customer { Id = "c1", OwnerId = "owner-1" }; + + await _customerViewModelService.UpdateCustomerModel(customer, + new CustomerModel { Email = "c1@firm.com", Owner = "" }); + + Assert.AreEqual(string.Empty, customer.OwnerId); + } + + [TestMethod] + public async Task InsertCustomerModel_WithOwner_LinksIt() + { + _customerServiceMock.Setup(c => c.GetCustomerByEmail("boss@firm.com", "")) + .ReturnsAsync(new Customer { Id = "owner-1" }); + + var customer = await _customerViewModelService.InsertCustomerModel( + new CustomerModel { Email = "new@firm.com", Owner = "boss@firm.com" }); + + Assert.AreEqual("owner-1", customer.OwnerId); + } + + [TestMethod] + public async Task PrepareCustomerModel_Owner_ListsItsSubAccounts() + { + var owner = new Customer { Id = "owner-1", Email = "boss@firm.com" }; + _customerServiceMock + .Setup(c => c.GetAllCustomers(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), "owner-1", It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>>())) + .ReturnsAsync(new PagedList { + new() { Id = "sub-1", Email = "a@firm.com", Active = true, OwnerId = "owner-1" }, + new() { Id = "sub-2", Email = "b@firm.com", Active = false, OwnerId = "owner-1" } + }); + + var model = new CustomerModel(); + await _customerViewModelService.PrepareCustomerModel(model, owner, false); + + CollectionAssert.AreEqual(new[] { "sub-1", "sub-2" }, model.SubAccounts.Select(x => x.Id).ToArray()); + Assert.IsFalse(model.SubAccounts[1].Active); + Assert.IsTrue(string.IsNullOrEmpty(model.OwnerCustomerId)); + } + + [TestMethod] + public async Task PrepareCustomerModel_StorePanel_QueriesOnlyItsStoreAndSkipsOutOfScopeRows() + { + _scopeMock.Setup(s => s.GetDefaultStoreIdAsync()).ReturnsAsync("store-7"); + var foreign = new Customer { Id = "sub-x", Email = "x@firm.com", OwnerId = "owner-1" }; + _scopeMock.Setup(s => s.HasAccess(foreign)).ReturnsAsync(false); + SetupGetAllCustomers(new PagedList { + new() { Id = "sub-1", Email = "a@firm.com", OwnerId = "owner-1" }, foreign + }); + + var model = new CustomerModel(); + await _customerViewModelService.PrepareCustomerModel(model, new Customer { Id = "owner-1" }, false); + + CollectionAssert.AreEqual(new[] { "sub-1" }, model.SubAccounts.Select(x => x.Id).ToArray()); + _customerServiceMock.Verify(c => c.GetAllCustomers(It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), "store-7", "owner-1", It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>>()), Times.Once); + } + + [TestMethod] + public async Task PrepareCustomerModel_SalesManager_ListsOnlyTheirSubAccounts() + { + _currentCustomer.SeId = "se-1"; + + await _customerViewModelService.PrepareCustomerModel(new CustomerModel(), new Customer { Id = "owner-1" }, + false); + + _customerServiceMock.Verify(c => c.GetAllCustomers(It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), "owner-1", "se-1", It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>>()), Times.Once); + } + + [TestMethod] + public async Task PrepareCustomerModel_SubAccount_LinksTheOwnerInReach() + { + var owner = new Customer { Id = "owner-1", Email = "boss@firm.com" }; + _customerServiceMock.Setup(c => c.GetCustomerById("owner-1")).ReturnsAsync(owner); + + var model = new CustomerModel(); + await _customerViewModelService.PrepareCustomerModel(model, + new Customer { Id = "sub-1", OwnerId = "owner-1" }, false); + + Assert.AreEqual("owner-1", model.OwnerCustomerId); + Assert.AreEqual("boss@firm.com", model.OwnerCustomerEmail); + Assert.AreEqual(0, model.SubAccounts.Count); + } + + [TestMethod] + public async Task PrepareCustomerModel_SubAccount_OwnerOutOfReach_IsNotShown() + { + var owner = new Customer { Id = "owner-1", Email = "boss@other-store.com" }; + _customerServiceMock.Setup(c => c.GetCustomerById("owner-1")).ReturnsAsync(owner); + _scopeMock.Setup(s => s.HasAccess(owner)).ReturnsAsync(false); + + var model = new CustomerModel(); + await _customerViewModelService.PrepareCustomerModel(model, + new Customer { Id = "sub-1", OwnerId = "owner-1" }, false); + + Assert.IsTrue(string.IsNullOrEmpty(model.OwnerCustomerId)); + Assert.IsTrue(string.IsNullOrEmpty(model.OwnerCustomerEmail)); + } + + #endregion + private void SetupGetAllCustomers(IPagedList result) { _customerServiceMock diff --git a/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs new file mode 100644 index 0000000000..2cef96d98f --- /dev/null +++ b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs @@ -0,0 +1,221 @@ +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Customers; +using Grand.Domain; +using Grand.Domain.Customers; +using Grand.Domain.Orders; +using Grand.Domain.Stores; +using Grand.Infrastructure; +using Grand.Infrastructure.Configuration; +using Grand.Infrastructure.Validators; +using Grand.Web.AdminShared.Models.Customers; +using Grand.Web.AdminShared.Validators.Customers; +using Grand.Web.Common.Localization; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using System.Linq.Expressions; + +namespace Grand.Web.Admin.Tests.Validators; + +/// +/// The owner field of the admin customer form: sub-accounts stay one level deep, and the owner already +/// linked never blocks an unrelated save. +/// +[TestClass] +public class CustomerValidatorOwnerTests +{ + private const string NotFound = "Admin.Customers.Customers.Fields.Owner.NotFound"; + private const string IsSubAccount = "Admin.Customers.Customers.Fields.Owner.IsSubAccount"; + private const string HasSubAccounts = "Admin.Customers.Customers.Fields.Owner.HasSubAccounts"; + private const string NotRegistered = "Admin.Customers.Customers.Fields.Owner.NotRegistered"; + private const string Self = "Admin.Customers.Customers.Fields.Owner.Self"; + + private Mock _customerService; + private Mock _groupService; + private CustomerConfig _customerConfig; + + [TestInitialize] + public void Setup() + { + _groupService = new Mock(); + _groupService + .Setup(g => g.GetAllCustomerGroups(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(new PagedList()); + _groupService.Setup(g => g.IsAdmin(It.IsAny())).ReturnsAsync(true); + _groupService.Setup(g => g.IsRegistered(It.IsAny())).ReturnsAsync(true); + + //nobody has sub-accounts unless a test says so + _customerService = new Mock(); + _customerService + .Setup(c => c.GetAllCustomers(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>>())) + .ReturnsAsync(new PagedList()); + _customerConfig = new CustomerConfig(); + } + + private CustomerValidator Validator() + { + var translation = new Mock(); + translation.Setup(t => t.GetResource(It.IsAny())).Returns(k => k); + var work = new Mock(); + work.Setup(w => w.CurrentCustomer).Returns(new Customer { Id = "admin" }); + var store = new Mock(); + store.Setup(s => s.CurrentStore).Returns(new Store { Id = "s1" }); + var context = new Mock(); + context.Setup(c => c.WorkContext).Returns(work.Object); + context.Setup(c => c.StoreContext).Returns(store.Object); + + return new CustomerValidator(new List>(), translation.Object, + new Mock().Object, context.Object, _customerService.Object, _groupService.Object, + new CustomerSettings(), _customerConfig); + } + + private void SubAccountsOf(string ownerId, int count) + { + var list = new PagedList(); + for (var i = 0; i < count; i++) list.Add(new Customer { OwnerId = ownerId }); + _customerService + .Setup(c => c.GetAllCustomers(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), ownerId, It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>>())) + .ReturnsAsync(list); + } + + private Customer Existing(string id = "c1", string ownerId = "") + { + var customer = new Customer { Id = id, Email = $"{id}@firm.com", OwnerId = ownerId }; + _customerService.Setup(c => c.GetCustomerById(id)).ReturnsAsync(customer); + return customer; + } + + private Customer OwnerAccount(string email = "boss@firm.com", string id = "owner-1", string ownerId = "") + { + var owner = new Customer { Id = id, Email = email, OwnerId = ownerId }; + _customerService.Setup(c => c.GetCustomerByEmail(email, It.IsAny())).ReturnsAsync(owner); + _customerService.Setup(c => c.GetCustomerById(id)).ReturnsAsync(owner); + return owner; + } + + private async Task OwnerErrors(CustomerModel model) => + (await Validator().ValidateAsync(model)).Errors + .Where(e => e.PropertyName == nameof(CustomerModel.Owner)).Select(e => e.ErrorMessage).ToArray(); + + private static CustomerModel Edit(string owner, string id = "c1") => + new() { Id = id, Email = $"{id}@firm.com", StoreId = "s1", Owner = owner }; + + private static CustomerModel Create(string owner) => + new() { Email = "new@firm.com", StoreId = "s1", Owner = owner }; + + [TestMethod] + public async Task ValidOwner_Passes() + { + Existing(); + OwnerAccount(); + + CollectionAssert.AreEqual(Array.Empty(), await OwnerErrors(Edit("boss@firm.com"))); + CollectionAssert.AreEqual(Array.Empty(), await OwnerErrors(Create("boss@firm.com"))); + } + + [TestMethod] + public async Task NoOwner_Passes() + { + Existing(); + + CollectionAssert.AreEqual(Array.Empty(), await OwnerErrors(Edit(""))); + } + + [TestMethod] + public async Task UnknownOwner_Fails() + { + Existing(); + + CollectionAssert.AreEqual(new[] { NotFound }, await OwnerErrors(Edit("nobody@firm.com"))); + CollectionAssert.AreEqual(new[] { NotFound }, await OwnerErrors(Create("nobody@firm.com"))); + } + + [TestMethod] + public async Task DeletedOwner_Fails() + { + Existing(); + OwnerAccount().Deleted = true; + + CollectionAssert.AreEqual(new[] { NotFound }, await OwnerErrors(Edit("boss@firm.com"))); + } + + [TestMethod] + public async Task OwnerThatIsASubAccount_Fails() + { + //a chain would let the top account reach orders two levels down + Existing(); + OwnerAccount(ownerId: "top-owner"); + + CollectionAssert.AreEqual(new[] { IsSubAccount }, await OwnerErrors(Edit("boss@firm.com"))); + } + + [TestMethod] + public async Task CustomerWithSubAccounts_CannotGetAnOwner() + { + Existing(); + OwnerAccount(); + SubAccountsOf("c1", 2); + + CollectionAssert.AreEqual(new[] { HasSubAccounts }, await OwnerErrors(Edit("boss@firm.com"))); + } + + [TestMethod] + public async Task UnregisteredOwner_Fails() + { + Existing(); + var owner = OwnerAccount(); + _groupService.Setup(g => g.IsRegistered(owner)).ReturnsAsync(false); + + CollectionAssert.AreEqual(new[] { NotRegistered }, await OwnerErrors(Edit("boss@firm.com"))); + } + + [TestMethod] + public async Task OwnEmail_Fails() + { + Existing(); + + CollectionAssert.AreEqual(new[] { Self }, await OwnerErrors(Edit("C1@Firm.com"))); + } + + [TestMethod] + public async Task OwnAccountUnderAnotherEmail_Fails() + { + //the email lookup resolves to the customer being edited (e.g. its email is being changed at the same time) + Existing(); + _customerService.Setup(c => c.GetCustomerByEmail("old@firm.com", It.IsAny())) + .ReturnsAsync(new Customer { Id = "c1" }); + + CollectionAssert.AreEqual(new[] { Self }, await OwnerErrors(Edit("old@firm.com"))); + } + + [TestMethod] + public async Task OwnerAlreadyLinked_NeverBlocksTheSave() + { + //legacy data the rules would now refuse (the owner became a sub-account) must not lock the form + Existing(ownerId: "owner-1"); + OwnerAccount(ownerId: "top-owner"); + + CollectionAssert.AreEqual(Array.Empty(), await OwnerErrors(Edit("Boss@Firm.com"))); + } + + [TestMethod] + public async Task PerStoreIdentity_LooksTheOwnerUpInTheCustomersStore() + { + _customerConfig.RegisterCustomersPerStore = true; + Existing(); + OwnerAccount(); + + await OwnerErrors(Edit("boss@firm.com")); + + _customerService.Verify(c => c.GetCustomerByEmail("boss@firm.com", "s1"), Times.Once); + } +} diff --git a/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs b/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs index 3b57d73699..ae9d36aafe 100644 --- a/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs +++ b/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs @@ -209,6 +209,35 @@ public async Task ApplyPostConstraints_CraftedPost_CannotSmuggleOwnershipFields( CollectionAssert.AreEqual(new[] { "registered-id" }, craftedModel.CustomerGroups.ToArray()); } + [TestMethod] + public async Task ApplyPostConstraints_EditedSubAccount_KeepsItsRealOwner() + { + //blanking the owner here detached every sub-account a store manager saved; a crafted value must not + //move it to another owner either + _customerServiceMock.Setup(c => c.GetCustomerById("sub-1")) + .ReturnsAsync(new Customer { Id = "sub-1", OwnerId = "owner-1" }); + _customerServiceMock.Setup(c => c.GetCustomerById("owner-1")) + .ReturnsAsync(new Customer { Id = "owner-1", Email = "boss@firm.com" }); + var controller = BuildStoreController(); + + var model = new CustomerModel { Id = "sub-1", Owner = "attacker@evil.com" }; + await controller.ApplyPostConstraintsPublic(model); + + Assert.AreEqual("boss@firm.com", model.Owner); + } + + [TestMethod] + public async Task ApplyPostConstraints_EditedCustomerWithoutOwner_GetsNone() + { + _customerServiceMock.Setup(c => c.GetCustomerById("c1")).ReturnsAsync(new Customer { Id = "c1" }); + var controller = BuildStoreController(); + + var model = new CustomerModel { Id = "c1", Owner = "attacker@evil.com" }; + await controller.ApplyPostConstraintsPublic(model); + + Assert.AreEqual("", model.Owner); + } + [TestMethod] public async Task ApplyPostConstraints_NoRegisteredGroupFound_ClearsCustomerGroups() { diff --git a/src/Web/Grand.Web.AdminShared/Models/Customers/CustomerModel.cs b/src/Web/Grand.Web.AdminShared/Models/Customers/CustomerModel.cs index 1c794ca16d..dc43991213 100644 --- a/src/Web/Grand.Web.AdminShared/Models/Customers/CustomerModel.cs +++ b/src/Web/Grand.Web.AdminShared/Models/Customers/CustomerModel.cs @@ -218,6 +218,13 @@ public class CustomerModel : BaseEntityModel public IList AssociatedExternalAuthRecords { get; set; } = new List(); + //sub-accounts - display only, rebuilt from the entity on every render and never read back on save + public string OwnerCustomerId { get; set; } + + public string OwnerCustomerEmail { get; set; } + + public IList SubAccounts { get; set; } = new List(); + //customer notes [GrandResourceDisplayName("Admin.Customers.CustomerNotes.Fields.DisplayToCustomer")] public bool AddCustomerNoteDisplayToCustomer { get; set; } @@ -248,6 +255,13 @@ public class AssociatedExternalAuthModel : BaseEntityModel public string AuthMethodName { get; set; } } + public class SubAccountModel : BaseEntityModel + { + public string Email { get; set; } + public string FullName { get; set; } + public bool Active { get; set; } + } + public class LoyaltyPointsHistoryModel : BaseEntityModel { [GrandResourceDisplayName("Admin.Customers.Customers.LoyaltyPoints.Fields.Store")] diff --git a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs index fcad7fe8d4..dbc79f91c1 100644 --- a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs +++ b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs @@ -14,6 +14,7 @@ using Grand.Domain.Orders; using Grand.Domain.Tax; using Grand.Infrastructure; +using Grand.Infrastructure.Configuration; using Grand.SharedKernel; using Grand.Web.AdminShared.Interfaces; using Grand.Web.AdminShared.Models.Customers; @@ -49,7 +50,9 @@ public class CustomerViewModelService : ICustomerViewModelService private readonly IRequestTranslationService _translationService; private readonly IContextAccessor _contextAccessor; private readonly IEnumTranslationService _enumTranslationService; - + private readonly CustomerConfig _customerConfig; + private readonly IAdminDataScope _scope; + public CustomerViewModelService( ICustomerService customerService, IGroupService groupService, @@ -68,9 +71,13 @@ public CustomerViewModelService( CustomerSettings customerSettings, TaxSettings taxSettings, LoyaltyPointsSettings loyaltyPointsSettings, - CommonSettings commonSettings, - IEnumTranslationService enumTranslationService) + CommonSettings commonSettings, + IEnumTranslationService enumTranslationService, + CustomerConfig customerConfig, + IAdminDataScope scope) { + _customerConfig = customerConfig; + _scope = scope; _customerService = customerService; _groupService = groupService; _newsLetterSubscriptionService = newsLetterSubscriptionService; @@ -146,6 +153,7 @@ public virtual async Task PrepareCustomerModel(CustomerModel model, Customer cus { model.Id = customer.Id; model.ShowMessageContactForm = _commonSettings.StoreInDatabaseContactUsForm; + await PrepareSubAccounts(model, customer); if (!excludeProperties) { model.Email = customer.Email; @@ -332,15 +340,71 @@ public virtual async Task PrepareCustomerModel(CustomerModel model, Customer cus !customer.Active; } - public virtual async Task InsertCustomerModel(CustomerModel model) + /// + /// The owner id the posted owner email stands for. The owner already linked is kept when its email is + /// posted back unchanged; otherwise the email is looked up in the same store scope the validator used. + /// + private async Task ResolveOwnerId(Customer customer, CustomerModel model) + { + if (string.IsNullOrWhiteSpace(model.Owner)) + return string.Empty; + + var ownerEmail = model.Owner.Trim(); + if (!string.IsNullOrEmpty(customer?.OwnerId)) + { + var currentOwner = await _customerService.GetCustomerById(customer.OwnerId); + if (currentOwner != null && + string.Equals(currentOwner.Email, ownerEmail, StringComparison.OrdinalIgnoreCase)) + return customer.OwnerId; + } + + var storeId = _customerConfig.RegisterCustomersPerStore ? model.StoreId : ""; + var owner = await _customerService.GetCustomerByEmail(ownerEmail, storeId); + return owner?.Id ?? customer?.OwnerId ?? string.Empty; + } + + private async Task PrepareSubAccounts(CustomerModel model, Customer customer) { - var ownerId = string.Empty; - if (!string.IsNullOrEmpty(model.Owner)) + model.OwnerCustomerId = string.Empty; + model.OwnerCustomerEmail = string.Empty; + model.SubAccounts.Clear(); + //sub-accounts are one level deep, so a sub-account never has its own - only an owner, linked when the + //panel may open it + if (!string.IsNullOrEmpty(customer.OwnerId)) { - var customerOwner = await _customerService.GetCustomerByEmail(model.Owner); - if (customerOwner != null) ownerId = customerOwner.Id; + var owner = await _customerService.GetCustomerById(customer.OwnerId); + if (owner != null && await _scope.HasAccess(owner)) + { + model.OwnerCustomerId = owner.Id; + model.OwnerCustomerEmail = owner.Email; + } + + return; } + //scoped in the query like the customer list (store of the Store panel, sales employee of a sales + //manager); the panel's own access check then guards each row the same way it guards opening it + var storeId = await _scope.GetDefaultStoreIdAsync() ?? ""; + var subAccounts = await _customerService.GetAllCustomers(ownerId: customer.Id, storeId: storeId, + salesEmployeeId: _contextAccessor.WorkContext.CurrentCustomer.SeId); + foreach (var item in subAccounts) + { + if (!await _scope.HasAccess(item)) + continue; + + model.SubAccounts.Add(new CustomerModel.SubAccountModel { + Id = item.Id, + Email = item.Email, + FullName = item.GetFullName(), + Active = item.Active + }); + } + } + + public virtual async Task InsertCustomerModel(CustomerModel model) + { + var ownerId = await ResolveOwnerId(null, model); + var customer = new Customer { CustomerGuid = Guid.NewGuid(), Email = model.Email, @@ -470,15 +534,7 @@ await _customerService.UpdateUserField(customer, SystemCustomerFieldNames.TwoFac _ => customer.Username }; - if (!string.IsNullOrEmpty(model.Owner)) - { - var customerOwner = await _customerService.GetCustomerByEmail(model.Owner); - if (customerOwner != null) customer.OwnerId = customerOwner.Id; - } - else - { - customer.OwnerId = string.Empty; - } + customer.OwnerId = await ResolveOwnerId(customer, model); //VAT number if (_taxSettings.EuVatEnabled) diff --git a/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs b/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs index e56cfb508f..4aa06140c1 100644 --- a/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs +++ b/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs @@ -173,6 +173,54 @@ async Task ValidateCustomerGroups(IList customerGroups, C return ""; } + //sub-accounts are one level deep: an owner is never a sub-account itself and a customer with + //sub-accounts never becomes one, so the owner's access to their orders cannot chain further + async Task ValidateOwner(CustomerModel x, Customer customer, ValidationContext context) + { + if (string.IsNullOrWhiteSpace(x.Owner)) + return; + + var ownerEmail = x.Owner.Trim(); + + //the owner already linked stays valid, so an unrelated edit of an existing account is never blocked + if (!string.IsNullOrEmpty(customer?.OwnerId)) + { + var currentOwner = await customerService.GetCustomerById(customer.OwnerId); + if (currentOwner != null && + string.Equals(currentOwner.Email, ownerEmail, StringComparison.OrdinalIgnoreCase)) + return; + } + + if (string.Equals(ownerEmail, x.Email?.Trim(), StringComparison.OrdinalIgnoreCase)) + { + context.AddFailure(nameof(x.Owner), + translationService.GetResource("Admin.Customers.Customers.Fields.Owner.Self")); + return; + } + + var owner = await customerService.GetCustomerByEmail(ownerEmail, StoreScope(x)); + if (owner == null || owner.Deleted) + { + context.AddFailure(nameof(x.Owner), + translationService.GetResource("Admin.Customers.Customers.Fields.Owner.NotFound")); + return; + } + + if (owner.Id == customer?.Id) + context.AddFailure(nameof(x.Owner), + translationService.GetResource("Admin.Customers.Customers.Fields.Owner.Self")); + else if (!string.IsNullOrEmpty(owner.OwnerId)) + context.AddFailure(nameof(x.Owner), + translationService.GetResource("Admin.Customers.Customers.Fields.Owner.IsSubAccount")); + else if (!await groupService.IsRegistered(owner)) + context.AddFailure(nameof(x.Owner), + translationService.GetResource("Admin.Customers.Customers.Fields.Owner.NotRegistered")); + else if (customer != null && + (await customerService.GetAllCustomers(ownerId: customer.Id, pageSize: 1)).Count > 0) + context.AddFailure(nameof(x.Owner), + translationService.GetResource("Admin.Customers.Customers.Fields.Owner.HasSubAccounts")); + } + void CustomerCreateValidator() { When(x => string.IsNullOrEmpty(x.Id), () => @@ -194,12 +242,7 @@ void CustomerCreateValidator() context.AddFailure("Email is already registered"); } - if (!string.IsNullOrWhiteSpace(x.Owner)) - { - var customerOwner = await customerService.GetCustomerByEmail(x.Owner, StoreScope(x)); - if (customerOwner == null) - context.AddFailure("Owner email is not exists"); - } + await ValidateOwner(x, null, context); if (!string.IsNullOrWhiteSpace(x.Username) && customerSettings.UsernamesEnabled) { @@ -235,17 +278,9 @@ void CustomerEditValidator() var customerGroupsError = await ValidateCustomerGroups(newCustomerGroups, x); if (!string.IsNullOrEmpty(customerGroupsError)) context.AddFailure(customerGroupsError); - if (!string.IsNullOrWhiteSpace(x.Owner)) - { - var customerByOwner = await customerService.GetCustomerByEmail(x.Owner, StoreScope(x)); - if (customerByOwner == null) - context.AddFailure("Owner email is not exists"); - - if (string.Equals(x.Owner, x.Email, StringComparison.CurrentCultureIgnoreCase)) - context.AddFailure("You can't assign own email"); - } - var customer = await customerService.GetCustomerById(x.Id); + await ValidateOwner(x, customer, context); + if (await groupService.IsSalesManager(contextAccessor.WorkContext.CurrentCustomer) && customer?.Id == contextAccessor.WorkContext.CurrentCustomer.Id) context.AddFailure("You can't edit own data from admin panel"); diff --git a/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml b/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml index 513a1d317b..b1b8aa7948 100644 --- a/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml +++ b/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml @@ -105,6 +105,39 @@
} +@* read only on both panels: an owner lists its sub-accounts, a sub-account links its owner *@ +@if (Model.SubAccounts.Count > 0) +{ + + + + + + + + + + + @foreach (var subAccount in Model.SubAccounts) + { + + + + + + } + +
@Loc["Admin.Customers.Customers.Fields.Email"]@Loc["Admin.Customers.Customers.Fields.FullName"]@Loc["Admin.Customers.Customers.Fields.Active"]
@subAccount.Email@subAccount.FullName@(subAccount.Active ? Loc["Admin.Common.Yes"] : Loc["Admin.Common.No"])
+ +} +else if (!string.IsNullOrEmpty(Model.OwnerCustomerId)) +{ + +
+ @Model.OwnerCustomerEmail +
+ +}
@if (Model.GenderEnabled) diff --git a/src/Web/Grand.Web.Store/Controllers/CustomerController.cs b/src/Web/Grand.Web.Store/Controllers/CustomerController.cs index 730314c433..368e325a23 100644 --- a/src/Web/Grand.Web.Store/Controllers/CustomerController.cs +++ b/src/Web/Grand.Web.Store/Controllers/CustomerController.cs @@ -50,7 +50,13 @@ public class CustomerController( protected override async Task ApplyPostConstraints(CustomerModel model) { model.StoreId = Scope.DefaultStoreId; - model.Owner = ""; + //the owner link is not the store manager's to change: a new customer gets none and an existing one + //keeps its own (blanking it here used to detach every sub-account the store manager saved) + var existing = string.IsNullOrEmpty(model.Id) ? null : await CustomerService.GetCustomerById(model.Id); + var owner = string.IsNullOrEmpty(existing?.OwnerId) + ? null + : await CustomerService.GetCustomerById(existing.OwnerId); + model.Owner = owner?.Email ?? ""; model.VendorId = ""; model.StaffStoreId = ""; model.SeId = ""; From 009675c92d5986a8b349833cb8c906c800ad254c Mon Sep 17 00:00:00 2001 From: support Date: Sat, 24 Oct 2026 12:12:11 +0200 Subject: [PATCH 3/7] Index customers by owner Sub-accounts are listed on the storefront and the admin customer form, and the admin checks for them before linking an owner. Without an index each of these scanned the whole customer collection. Fresh installs create it with the other customer indexes, existing ones through a 2.5 migration. --- .../Services/InstallationService.cs | 3 ++ .../2.5/MigrationCustomerOwnerIndex.cs | 39 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 src/Modules/Grand.Module.Migration/Migrations/2.5/MigrationCustomerOwnerIndex.cs diff --git a/src/Modules/Grand.Module.Installer/Services/InstallationService.cs b/src/Modules/Grand.Module.Installer/Services/InstallationService.cs index a39d505e4d..3a45b80638 100644 --- a/src/Modules/Grand.Module.Installer/Services/InstallationService.cs +++ b/src/Modules/Grand.Module.Installer/Services/InstallationService.cs @@ -492,6 +492,9 @@ await dbContext.CreateIndex(_customerRepository, OrderBuilder.Create().Ascending(x => x.Email).Ascending(x => x.StoreId), "Email_StoreId"); await dbContext.CreateIndex(_customerRepository, OrderBuilder.Create().Ascending(x => x.Username).Ascending(x => x.StoreId), "Username_StoreId"); + //an owner's sub-accounts + await dbContext.CreateIndex(_customerRepository, + OrderBuilder.Create().Ascending(x => x.OwnerId), "OwnerId_1"); await dbContext.CreateIndex(_customerRepository, OrderBuilder.Create().Descending(x => x.CreatedOnUtc), "CreatedOnUtc"); diff --git a/src/Modules/Grand.Module.Migration/Migrations/2.5/MigrationCustomerOwnerIndex.cs b/src/Modules/Grand.Module.Migration/Migrations/2.5/MigrationCustomerOwnerIndex.cs new file mode 100644 index 0000000000..e78f7f75a1 --- /dev/null +++ b/src/Modules/Grand.Module.Migration/Migrations/2.5/MigrationCustomerOwnerIndex.cs @@ -0,0 +1,39 @@ +using Grand.Data; +using Grand.Domain.Customers; +using Grand.Infrastructure.Migrations; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; + +namespace Grand.Module.Migration.Migrations._2._5; + +public class MigrationCustomerOwnerIndex : IMigration +{ + private const string IndexName = "OwnerId_1"; + + public int Priority => 1; + public DbVersion Version => new(2, 5); + public Guid Identity => new("6f0d4b9e-3c1a-4e27-9a85-2b7e4c1d9f63"); + public string Name => "Index on Customer owner 2.5"; + + public async Task UpgradeProcess(IServiceProvider serviceProvider) + { + var repository = serviceProvider.GetRequiredService>(); + var databaseContext = serviceProvider.GetRequiredService(); + var logService = serviceProvider.GetRequiredService>(); + + try + { + //an owner's sub-accounts are listed on the storefront and in the admin customer form, and the admin + //checks for them before linking an owner - each a scan of the whole customer collection without it + await databaseContext.CreateIndex(repository, + OrderBuilder.Create().Ascending(x => x.OwnerId), IndexName); + } + catch (Exception ex) + { + logService.LogError(ex, "UpgradeProcess - CustomerOwnerIndex (2.5)"); + return false; + } + + return true; + } +} From 31ff5029d114807fccf5e7e8cec2464c86b612d7 Mon Sep 17 00:00:00 2001 From: support Date: Sat, 24 Oct 2026 19:51:31 +0200 Subject: [PATCH 4/7] Close the review findings on sub-account owners - The Store panel replaces the posted owner before saving, so its validator no longer checks the posted value: the answers told a store manager whether another store's e-mail existed, was a sub-account, was registered or had sub-accounts. - An unchanged owner is kept only while the customer stays in its store; a customer moved to another store has its owner looked up there. - A sub-account e-mail differing only in case is repaired on save even when e-mail changes are not allowed. Older edits stored it as typed, and those rows still could not sign in and failed on a password change. - An owner's own deleted sub-account gets the same answer as a missing or foreign one, as the edit page and the command already give. --- .../Services/CustomerViewModelServiceTests.cs | 16 +++++++++ .../Validators/CustomerValidatorOwnerTests.cs | 35 ++++++++++++++++++- .../Validators/CustomerValidatorTests.cs | 4 ++- .../SubAccountEditCommandHandlerTests.cs | 15 ++++++++ .../Customer/SubAccountEditValidatorTests.cs | 13 +++++++ .../Services/CustomerViewModelService.cs | 3 +- .../Validators/Customers/CustomerValidator.cs | 15 ++++++-- .../Customers/SubAccountEditCommandHandler.cs | 7 ++-- .../Customer/SubAccountEditValidator.cs | 11 +++--- 9 files changed, 104 insertions(+), 15 deletions(-) diff --git a/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs b/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs index 9c642de595..363f4f39fd 100644 --- a/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs +++ b/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs @@ -479,6 +479,22 @@ await _customerViewModelService.UpdateCustomerModel(customer, _customerServiceMock.Verify(c => c.GetCustomerByEmail("boss@firm.com", ""), Times.Never); } + [TestMethod] + public async Task UpdateCustomerModel_PerStore_CustomerMovedToAnotherStore_LooksTheOwnerUpAgain() + { + _customerConfig.RegisterCustomersPerStore = true; + _customerServiceMock.Setup(c => c.GetCustomerById("owner-a")) + .ReturnsAsync(new Customer { Id = "owner-a", Email = "boss@firm.com" }); + _customerServiceMock.Setup(c => c.GetCustomerByEmail("boss@firm.com", "store-b")) + .ReturnsAsync(new Customer { Id = "owner-b" }); + var customer = new Customer { Id = "c1", OwnerId = "owner-a", StoreId = "store-a" }; + + await _customerViewModelService.UpdateCustomerModel(customer, + new CustomerModel { Email = "c1@firm.com", StoreId = "store-b", Owner = "boss@firm.com" }); + + Assert.AreEqual("owner-b", customer.OwnerId); + } + [TestMethod] public async Task UpdateCustomerModel_OwnerCleared_UnlinksTheOwner() { diff --git a/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs index 2cef96d98f..3c8aa39112 100644 --- a/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs +++ b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs @@ -7,6 +7,7 @@ using Grand.Infrastructure; using Grand.Infrastructure.Configuration; using Grand.Infrastructure.Validators; +using Grand.Web.AdminShared.Interfaces; using Grand.Web.AdminShared.Models.Customers; using Grand.Web.AdminShared.Validators.Customers; using Grand.Web.Common.Localization; @@ -32,10 +33,15 @@ public class CustomerValidatorOwnerTests private Mock _customerService; private Mock _groupService; private CustomerConfig _customerConfig; + private Mock> _scope; [TestInitialize] public void Setup() { + //Admin panel: no default store, the owner field is editable + _scope = new Mock>(); + _scope.Setup(s => s.GetDefaultStoreIdAsync()).ReturnsAsync((string)null); + _groupService = new Mock(); _groupService .Setup(g => g.GetAllCustomerGroups(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -70,7 +76,7 @@ private CustomerValidator Validator() return new CustomerValidator(new List>(), translation.Object, new Mock().Object, context.Object, _customerService.Object, _groupService.Object, - new CustomerSettings(), _customerConfig); + new CustomerSettings(), _customerConfig, _scope.Object); } private void SubAccountsOf(string ownerId, int count) @@ -207,6 +213,33 @@ public async Task OwnerAlreadyLinked_NeverBlocksTheSave() CollectionAssert.AreEqual(Array.Empty(), await OwnerErrors(Edit("Boss@Firm.com"))); } + [TestMethod] + public async Task StorePanel_DoesNotCheckThePostedOwner() + { + //the Store panel replaces the posted owner before saving; checking it would answer a store manager + //whether another store's e-mail exists, is a sub-account, is registered or has sub-accounts + _scope.Setup(s => s.GetDefaultStoreIdAsync()).ReturnsAsync("s1"); + Existing(); + OwnerAccount(ownerId: "top-owner"); + + CollectionAssert.AreEqual(Array.Empty(), await OwnerErrors(Edit("boss@firm.com"))); + _customerService.Verify(c => c.GetCustomerByEmail("boss@firm.com", It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task OwnerAlreadyLinked_CustomerMovedToAnotherStore_MustHoldInTheNewStore() + { + _customerConfig.RegisterCustomersPerStore = true; + var customer = Existing(ownerId: "owner-1"); + customer.StoreId = "store-a"; + var owner = new Customer { Id = "owner-1", Email = "boss@firm.com" }; + _customerService.Setup(c => c.GetCustomerById("owner-1")).ReturnsAsync(owner); + _customerService.Setup(c => c.GetCustomerByEmail("boss@firm.com", "store-a")).ReturnsAsync(owner); + + //Edit() posts StoreId "s1": the owner lives in store-a only, so the link would cross stores + CollectionAssert.AreEqual(new[] { NotFound }, await OwnerErrors(Edit("boss@firm.com"))); + } + [TestMethod] public async Task PerStoreIdentity_LooksTheOwnerUpInTheCustomersStore() { diff --git a/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorTests.cs b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorTests.cs index c3666870fb..818d1bdf00 100644 --- a/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorTests.cs +++ b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorTests.cs @@ -6,6 +6,7 @@ using Grand.Infrastructure; using Grand.Infrastructure.Configuration; using Grand.Infrastructure.Validators; +using Grand.Web.AdminShared.Interfaces; using Grand.Web.AdminShared.Models.Customers; using Grand.Web.AdminShared.Validators.Customers; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -52,7 +53,8 @@ public void Setup() new Mock().Object, _groupServiceMock.Object, new CustomerSettings(), - new CustomerConfig()); + new CustomerConfig(), + new Mock>().Object); } private static CustomerModel BuildModel(string storeId) => diff --git a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs index a27f7fea6d..aa09ae098c 100644 --- a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs +++ b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs @@ -169,6 +169,21 @@ public async Task Handle_NewEmail_UsernamesEnabled_LeavesUsername() Assert.AreEqual("sub@x.com", customer.Username); } + [TestMethod] + public async Task Handle_EmailStoredInMixedCaseByOlderEdits_IsRepairedEvenWhenChangesAreNotAllowed() + { + //such a row cannot sign in, and its password change looked it up by that e-mail and threw + _customerSettings.AllowUsersToChangeEmail = false; + var customer = SubAccount(); + customer.Email = "Sub@X.com"; + + await _handler.Handle(Command(email: "Sub@X.com", password: "newpass"), default); + + Assert.AreEqual("sub@x.com", customer.Email); + _customerManagerServiceMock.Verify(m => m.ChangePassword( + It.Is(r => r.Email == "sub@x.com"), "store-9"), Times.Once); + } + [TestMethod] public async Task Handle_EmailChangeNotAllowed_KeepsEmail() { diff --git a/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs b/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs index cb9f3d862d..bae32c9672 100644 --- a/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs +++ b/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs @@ -70,6 +70,19 @@ public async Task MissingAccount_GetsTheSameAnswerAsSomeoneElses() result.Errors.Select(e => e.ErrorMessage).ToArray()); } + [TestMethod] + public async Task OwnDeletedSubAccount_GetsTheSameAnswer() + { + //the edit page and the command treat it as not yours; "Delete account" was a button label, not an error + _customerService.Setup(c => c.GetCustomerById("c1")) + .ReturnsAsync(new Customer { Id = "c1", OwnerId = "owner", Deleted = true, Email = "d@x.com" }); + + var result = await _validator.ValidateAsync(Model()); + + CollectionAssert.AreEqual(new[] { "Account.SubAccounts.NotOwner" }, + result.Errors.Select(e => e.ErrorMessage).ToArray()); + } + [TestMethod] public async Task NotRegisteredSubAccount_UsesTheResourceNotARawKey() { diff --git a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs index dbc79f91c1..e4fd2f596b 100644 --- a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs +++ b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs @@ -350,7 +350,8 @@ private async Task ResolveOwnerId(Customer customer, CustomerModel model return string.Empty; var ownerEmail = model.Owner.Trim(); - if (!string.IsNullOrEmpty(customer?.OwnerId)) + if (!string.IsNullOrEmpty(customer?.OwnerId) && + (!_customerConfig.RegisterCustomersPerStore || customer.StoreId == model.StoreId)) { var currentOwner = await _customerService.GetCustomerById(customer.OwnerId); if (currentOwner != null && diff --git a/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs b/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs index 4aa06140c1..5b5a0435de 100644 --- a/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs +++ b/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs @@ -6,6 +6,7 @@ using Grand.Infrastructure.Configuration; using Grand.Infrastructure.Validators; using Grand.SharedKernel.Extensions; +using Grand.Web.AdminShared.Interfaces; using Grand.Web.AdminShared.Models.Customers; using System.Text.RegularExpressions; using Grand.Web.Common.Localization; @@ -22,7 +23,8 @@ public CustomerValidator( ICustomerService customerService, IGroupService groupService, CustomerSettings customerSettings, - CustomerConfig customerConfig) + CustomerConfig customerConfig, + IAdminDataScope scope) : base(validators) { //when per-store customer identity is enabled, uniqueness is scoped to the customer's store @@ -180,10 +182,17 @@ async Task ValidateOwner(CustomerModel x, Customer customer, ValidationContext Handle(SubAccountEditCommand request, CancellationToken if (customer == null || customer.Deleted || customer.OwnerId != request.CurrentCustomer.Id) return false; - //update email - stored lowercased like every other write, or the lowercased login lookup misses it + //update email - stored lowercased like every other write, or the lowercased login lookup misses it. An + //address differing only in case is the same address, so it is repaired even when changing it is not + //allowed: older edits stored it as typed, which locked the account out and broke its password change var email = request.EditModel.Email.Trim().ToLowerInvariant(); - if (customer.Email != email && _customerSettings.AllowUsersToChangeEmail) + var sameAddress = string.Equals(customer.Email, email, StringComparison.OrdinalIgnoreCase); + if (customer.Email != email && (sameAddress || _customerSettings.AllowUsersToChangeEmail)) { customer.Email = email; await _customerService.UpdateCustomerField(customer, x => x.Email, email); diff --git a/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs b/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs index ccc87d4757..6cce1b35db 100644 --- a/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs @@ -43,9 +43,10 @@ public SubAccountEditValidator( { var customer = await customerService.GetCustomerById(x.Id); - //ownership first: a missing id and someone else's account get the same answer whatever its state, - //so the answer reveals neither which ids exist nor what state they are in - if (customer == null || customer.OwnerId != contextAccessor.WorkContext.CurrentCustomer.Id) + //ownership first: a missing id, a deleted account and someone else's account get the same answer + //(the edit page and the command treat them alike), so it reveals neither ids nor their state + if (customer == null || customer.Deleted || + customer.OwnerId != contextAccessor.WorkContext.CurrentCustomer.Id) { context.AddFailure(translationService.GetResource("Account.SubAccounts.NotOwner")); return; @@ -53,10 +54,6 @@ public SubAccountEditValidator( switch (customer) { - case { Deleted: true }: - context.AddFailure( - translationService.GetResource("Account.DeleteAccount")); - return; case not null when !await groupService.IsRegistered(customer): context.AddFailure( translationService.GetResource("Account.Login.WrongCredentials.NotRegistered")); From 7fb9a9d84fb0ec7343c31fbb48634a93cd8cfaed Mon Sep 17 00:00:00 2001 From: support Date: Sat, 24 Oct 2026 20:28:17 +0200 Subject: [PATCH 5/7] Keep a customer's sales employee, vendor, staff store and groups on Store saves The Store panel blanked the sales employee, vendor and staff store and reset the groups to Registered on every customer save. A store manager fixing a typo in a name took away the customer's sales employee and price groups, and a colleague's Store manager role. An existing customer now keeps all of these as stored; a new one still gets none of them and Registered only. Posted values are never used either way. --- .../Controllers/CustomerControllerTests.cs | 25 ++++++++++++++++++ .../Controllers/CustomerController.cs | 26 ++++++++++++++----- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs b/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs index ae9d36aafe..aff78fec18 100644 --- a/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs +++ b/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs @@ -226,6 +226,31 @@ public async Task ApplyPostConstraints_EditedSubAccount_KeepsItsRealOwner() Assert.AreEqual("boss@firm.com", model.Owner); } + [TestMethod] + public async Task ApplyPostConstraints_EditedCustomer_KeepsWhereItBelongs_IgnoringCraftedValues() + { + //blanking used to strip these on every save: the sales employee, the price groups, a colleague's + //Store manager role + _customerServiceMock.Setup(c => c.GetCustomerById("c1")).ReturnsAsync(new Customer { + Id = "c1", SeId = "se-1", VendorId = "vendor-1", StaffStoreId = StoreId, + Groups = { "registered-id", "wholesale-id", "storemanager-id" } + }); + var controller = BuildStoreController(); + + var model = new CustomerModel { + Id = "c1", SeId = "attacker-se", VendorId = "attacker-vendor", StaffStoreId = "attacker-store", + CustomerGroups = new[] { "attacker-admin-group-id" } + }; + await controller.ApplyPostConstraintsPublic(model); + + Assert.AreEqual(StoreId, model.StoreId); + Assert.AreEqual("se-1", model.SeId); + Assert.AreEqual("vendor-1", model.VendorId); + Assert.AreEqual(StoreId, model.StaffStoreId); + CollectionAssert.AreEqual(new[] { "registered-id", "wholesale-id", "storemanager-id" }, + model.CustomerGroups.ToArray()); + } + [TestMethod] public async Task ApplyPostConstraints_EditedCustomerWithoutOwner_GetsNone() { diff --git a/src/Web/Grand.Web.Store/Controllers/CustomerController.cs b/src/Web/Grand.Web.Store/Controllers/CustomerController.cs index 368e325a23..fcc3cbe08e 100644 --- a/src/Web/Grand.Web.Store/Controllers/CustomerController.cs +++ b/src/Web/Grand.Web.Store/Controllers/CustomerController.cs @@ -50,13 +50,27 @@ public class CustomerController( protected override async Task ApplyPostConstraints(CustomerModel model) { model.StoreId = Scope.DefaultStoreId; - //the owner link is not the store manager's to change: a new customer gets none and an existing one - //keeps its own (blanking it here used to detach every sub-account the store manager saved) + + //where a customer belongs - owner, vendor, staff store, sales employee, groups - is not the store + //manager's to change. An existing customer keeps what it has (blanking these used to strip them on every + //save: a sub-account lost its owner, a customer its sales employee and its price groups, a colleague + //the Store manager role); the posted values are never used either way var existing = string.IsNullOrEmpty(model.Id) ? null : await CustomerService.GetCustomerById(model.Id); - var owner = string.IsNullOrEmpty(existing?.OwnerId) - ? null - : await CustomerService.GetCustomerById(existing.OwnerId); - model.Owner = owner?.Email ?? ""; + if (existing != null) + { + var owner = string.IsNullOrEmpty(existing.OwnerId) + ? null + : await CustomerService.GetCustomerById(existing.OwnerId); + model.Owner = owner?.Email ?? ""; + model.VendorId = existing.VendorId ?? ""; + model.StaffStoreId = existing.StaffStoreId ?? ""; + model.SeId = existing.SeId ?? ""; + model.CustomerGroups = existing.Groups.ToArray(); + return; + } + + //a new customer belongs nowhere and is registered only + model.Owner = ""; model.VendorId = ""; model.StaffStoreId = ""; model.SeId = ""; From 04903652414dc212da5ec515b5d1d3390737f7b3 Mon Sep 17 00:00:00 2001 From: support Date: Sat, 24 Oct 2026 20:28:18 +0200 Subject: [PATCH 6/7] Store a customer's changed e-mail and username lowercased My account saved a changed e-mail or username as typed, while sign-in looks both up lowercased, so a customer who typed capitals could no longer sign in. Both are stored lowercased now, a value differing only in case repairs a row older edits stored as typed, and without usernames the username follows the e-mail as registration sets it. --- .../UpdateCustomerInfoCommandHandlerTests.cs | 84 ++++++++++++++++++- .../UpdateCustomerInfoCommandHandler.cs | 32 ++++--- 2 files changed, 102 insertions(+), 14 deletions(-) diff --git a/src/Tests/Grand.Web.Tests/Commands/Customers/UpdateCustomerInfoCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/UpdateCustomerInfoCommandHandlerTests.cs index b5b033ed89..3970c8b70c 100644 --- a/src/Tests/Grand.Web.Tests/Commands/Customers/UpdateCustomerInfoCommandHandlerTests.cs +++ b/src/Tests/Grand.Web.Tests/Commands/Customers/UpdateCustomerInfoCommandHandlerTests.cs @@ -10,6 +10,7 @@ using Grand.Web.Models.Customer; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; +using System.Linq.Expressions; namespace Grand.Web.Tests.Commands.Customers; @@ -18,17 +19,19 @@ public class UpdateCustomerInfoCommandHandlerTests { private Mock _customerService; - private async Task Update(OidcLoginConfig config) + private async Task Update(OidcLoginConfig config, CustomerSettings settings = null, + Customer customer = null, string email = "new@x.com", string username = null) { _customerService = new Mock(); var handler = new UpdateCustomerInfoCommandHandler(new Mock().Object, new Mock().Object, new Mock().Object, - _customerService.Object, new CustomerSettings { AllowUsersToChangeEmail = true }, new TaxSettings(), config); - var customer = new Customer { Id = "c1", Email = "old@x.com" }; + _customerService.Object, settings ?? new CustomerSettings { AllowUsersToChangeEmail = true }, + new TaxSettings(), config); + customer ??= new Customer { Id = "c1", Email = "old@x.com", Username = "old@x.com" }; await handler.Handle(new UpdateCustomerInfoCommand { Customer = customer, Store = new Domain.Stores.Store { Id = "s1" }, - Model = new CustomerInfoModel { Email = "new@x.com", FirstName = "A", LastName = "B" }, + Model = new CustomerInfoModel { Email = email, Username = username, FirstName = "A", LastName = "B" }, CustomerAttributes = [] }, CancellationToken.None); return customer; @@ -47,4 +50,77 @@ public async Task UpdateInfo_Mixed_EmailChanged() var customer = await Update(new OidcLoginConfig()); Assert.AreEqual("new@x.com", customer.Email); } + + [TestMethod] + public async Task UpdateInfo_MixedCaseEmail_IsStoredLowercased() + { + //stored as typed, the lowercased sign-in lookup missed it and the customer could no longer sign in + var customer = await Update(new OidcLoginConfig(), email: " New.Me@X.COM "); + + Assert.AreEqual("new.me@x.com", customer.Email); + _customerService.Verify(c => c.UpdateCustomerField(customer, + It.IsAny>>(), "new.me@x.com"), Times.AtLeastOnce); + } + + [TestMethod] + public async Task UpdateInfo_NewEmail_UsernamesDisabled_UsernameFollows() + { + var customer = await Update(new OidcLoginConfig()); + + Assert.AreEqual("new@x.com", customer.Username); + } + + [TestMethod] + public async Task UpdateInfo_SameEmailInOtherCase_ChangesNothing() + { + var customer = await Update(new OidcLoginConfig(), email: "OLD@X.com"); + + Assert.AreEqual("old@x.com", customer.Email); + _customerService.Verify(c => c.UpdateCustomerField(It.IsAny(), + It.IsAny>>(), It.IsAny()), Times.Never); + } + + [TestMethod] + [DataRow(false, OidcSignInMode.Mixed)] + [DataRow(true, OidcSignInMode.ProviderOnly)] + public async Task UpdateInfo_EmailStoredInMixedCaseByOlderEdits_IsRepaired(bool allowChange, OidcSignInMode mode) + { + //the same address, so repaired even where changing the e-mail is not allowed + var customer = await Update(new OidcLoginConfig { SignInMode = mode }, + new CustomerSettings { AllowUsersToChangeEmail = allowChange }, + new Customer { Id = "c1", Email = "Old@X.com", Username = "Old@X.com" }, email: "Old@X.com"); + + Assert.AreEqual("old@x.com", customer.Email); + } + + [TestMethod] + public async Task UpdateInfo_EmailChangeNotAllowed_OtherAddressIsNotTaken() + { + var customer = await Update(new OidcLoginConfig(), new CustomerSettings { AllowUsersToChangeEmail = false }); + + Assert.AreEqual("old@x.com", customer.Email); + } + + [TestMethod] + public async Task UpdateInfo_MixedCaseUsername_IsStoredLowercased() + { + var settings = new CustomerSettings { UsernamesEnabled = true, AllowUsersToChangeUsernames = true }; + var customer = await Update(new OidcLoginConfig(), settings, + new Customer { Id = "c1", Email = "old@x.com", Username = "old" }, email: "old@x.com", username: " John "); + + Assert.AreEqual("john", customer.Username); + //with usernames on, the e-mail does not drive the username + Assert.AreEqual("old@x.com", customer.Email); + } + + [TestMethod] + public async Task UpdateInfo_SameUsernameInOtherCase_ChangesNothing() + { + var settings = new CustomerSettings { UsernamesEnabled = true, AllowUsersToChangeUsernames = true }; + await Update(new OidcLoginConfig(), settings, + new Customer { Id = "c1", Email = "old@x.com", Username = "john" }, email: "old@x.com", username: "John"); + + _customerService.Verify(c => c.UpdateCustomerField(It.IsAny(), + It.IsAny>>(), It.IsAny()), Times.Never); + } } diff --git a/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs b/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs index 892b9acd5a..41a2d7e6c8 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs @@ -42,26 +42,38 @@ public UpdateCustomerInfoCommandHandler( public async Task Handle(UpdateCustomerInfoCommand request, CancellationToken cancellationToken) { - //username + //username - stored lowercased like every other write, or the lowercased sign-in lookup misses it + //(a value differing only in case is the stored one, so this repairs rows older edits stored as typed) if (_customerSettings.UsernamesEnabled && _customerSettings.AllowUsersToChangeUsernames) - if (!request.Customer.Username.Equals(request.Model.Username.Trim(), StringComparison.OrdinalIgnoreCase)) + { + var username = request.Model.Username.Trim().ToLowerInvariant(); + if (request.Customer.Username != username) { //change username - request.Customer.Username = request.Model.Username.Trim(); - await _customerService.UpdateCustomerField(request.Customer, x => x.Username, - request.Model.Username.Trim()); + request.Customer.Username = username; + await _customerService.UpdateCustomerField(request.Customer, x => x.Username, username); //re-authenticate if (request.OriginalCustomerIfImpersonated == null) await _authenticationService.SignIn(request.Customer, true); } + } - //email - if (!request.Customer.Email.Equals(request.Model.Email.Trim(), StringComparison.OrdinalIgnoreCase) && - _customerSettings.AllowUsersToChangeEmail && !_loginConfig.ProviderOnly) + //email - stored lowercased for the same reason. An address differing only in case is the same address, + //so it is repaired even where changing it is not allowed + var email = request.Model.Email.Trim().ToLowerInvariant(); + var sameAddress = string.Equals(request.Customer.Email, email, StringComparison.OrdinalIgnoreCase); + if (request.Customer.Email != email && + (sameAddress || (_customerSettings.AllowUsersToChangeEmail && !_loginConfig.ProviderOnly))) { //change email - request.Customer.Email = request.Model.Email.Trim(); - await _customerService.UpdateCustomerField(request.Customer, x => x.Email, request.Model.Email.Trim()); + request.Customer.Email = email; + await _customerService.UpdateCustomerField(request.Customer, x => x.Email, email); + //without usernames the username is the email (as registration sets it) + if (!_customerSettings.UsernamesEnabled) + { + request.Customer.Username = email; + await _customerService.UpdateCustomerField(request.Customer, x => x.Username, email); + } //re-authenticate (if usernames are disabled) //do not authenticate users in impersonation mode if (request.OriginalCustomerIfImpersonated == null) From 50485a6d674b0cd4c8016705f6544aa67c243405 Mon Sep 17 00:00:00 2001 From: support Date: Sun, 25 Oct 2026 00:09:11 +0200 Subject: [PATCH 7/7] Keep the Store panel to the store's customers, without staff accounts The Store panel is for a store's registered customers. Its scope also took in any registered account of the store, staff included: administrators, vendors, store managers, sales managers and accounts tied to a vendor or a staff store. These are now outside the panel - left out of the customer list in the query, so paging and totals stay right, and refused when opened by id, which covers every customer tab. GetAllCustomers gains two optional filters for this (excluded groups, and accounts tied to a vendor or a staff store); existing callers are unchanged. --- .../Interfaces/Customers/ICustomerService.cs | 5 +- .../Queries/Customers/GetCustomerQuery.cs | 7 +++ .../Handlers/GetCustomerQueryHandler.cs | 5 ++ .../Services/CustomerService.cs | 9 +++- .../Handlers/GetCustomerQueryHandlerTests.cs | 37 ++++++++++++++ .../StoreCustomerDataScopeTests.cs | 48 ++++++++++++++++++- .../Services/CustomerViewModelServiceTests.cs | 40 +++++++++++++++- .../Services/CustomerViewModelService.cs | 13 +++++ .../Services/StoreCustomerDataScope.cs | 15 ++++++ 9 files changed, 174 insertions(+), 5 deletions(-) diff --git a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs index 7f64d4c29c..2106a6890c 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs @@ -43,6 +43,8 @@ public interface ICustomerService /// Page index /// Page size /// + /// Customers in any of these groups are left out + /// Leaves out accounts tied to a vendor or a staff store /// Customers Task> GetAllCustomers(DateTime? createdFromUtc = null, DateTime? createdToUtc = null, string affiliateId = "", string vendorId = "", string storeId = "", @@ -52,7 +54,8 @@ Task> GetAllCustomers(DateTime? createdFromUtc = null, string firstName = null, string lastName = null, string company = null, string phone = null, string zipPostalCode = null, bool loadOnlyWithShoppingCart = false, ShoppingCartType? sct = null, - int pageIndex = 0, int pageSize = int.MaxValue, Expression> orderBySelector = null); + int pageIndex = 0, int pageSize = int.MaxValue, Expression> orderBySelector = null, + string[] excludedCustomerGroupIds = null, bool excludeVendorAndStaffAccounts = false); /// diff --git a/src/Business/Grand.Business.Core/Queries/Customers/GetCustomerQuery.cs b/src/Business/Grand.Business.Core/Queries/Customers/GetCustomerQuery.cs index 0d4cbd1046..086ae33aa3 100644 --- a/src/Business/Grand.Business.Core/Queries/Customers/GetCustomerQuery.cs +++ b/src/Business/Grand.Business.Core/Queries/Customers/GetCustomerQuery.cs @@ -18,6 +18,13 @@ public class GetCustomerQuery : IRequest> public string SalesEmployeeId { get; set; } = ""; public string[] CustomerGroupIds { get; set; } = null; public string[] CustomerTagIds { get; set; } = null; + + /// Customers in any of these groups are left out + public string[] ExcludedCustomerGroupIds { get; set; } = null; + + /// Leaves out accounts tied to a vendor or a staff store + public bool ExcludeVendorAndStaffAccounts { get; set; } = false; + public string Email { get; set; } = null; public string Username { get; set; } = null; public string FirstName { get; set; } = null; diff --git a/src/Business/Grand.Business.Customers/Queries/Handlers/GetCustomerQueryHandler.cs b/src/Business/Grand.Business.Customers/Queries/Handlers/GetCustomerQueryHandler.cs index c59cabdc86..6502b610ca 100644 --- a/src/Business/Grand.Business.Customers/Queries/Handlers/GetCustomerQueryHandler.cs +++ b/src/Business/Grand.Business.Customers/Queries/Handlers/GetCustomerQueryHandler.cs @@ -39,6 +39,11 @@ public async Task> Handle(GetCustomerQuery request, Cancell query = query.Where(c => !c.Deleted); if (request.CustomerGroupIds is { Length: > 0 }) query = query.Where(c => c.Groups.Any(x => request.CustomerGroupIds.Contains(x))); + if (request.ExcludedCustomerGroupIds is { Length: > 0 }) + query = query.Where(c => !c.Groups.Any(x => request.ExcludedCustomerGroupIds.Contains(x))); + if (request.ExcludeVendorAndStaffAccounts) + query = query.Where(c => (c.VendorId == null || c.VendorId == "") && + (c.StaffStoreId == null || c.StaffStoreId == "")); if (request.CustomerTagIds is { Length: > 0 }) foreach (var item in request.CustomerTagIds) query = query.Where(c => c.CustomerTags.Contains(item)); diff --git a/src/Business/Grand.Business.Customers/Services/CustomerService.cs b/src/Business/Grand.Business.Customers/Services/CustomerService.cs index da23c7bd06..6fe3747ebd 100644 --- a/src/Business/Grand.Business.Customers/Services/CustomerService.cs +++ b/src/Business/Grand.Business.Customers/Services/CustomerService.cs @@ -79,6 +79,8 @@ public CustomerService( /// Page index /// Page size /// order by selector + /// Customers in any of these groups are left out + /// Leaves out accounts tied to a vendor or a staff store /// Customers public virtual async Task> GetAllCustomers(DateTime? createdFromUtc = null, DateTime? createdToUtc = null, string affiliateId = "", string vendorId = "", string storeId = "", @@ -88,7 +90,8 @@ public virtual async Task> GetAllCustomers(DateTime? create string firstName = null, string lastName = null, string company = null, string phone = null, string zipPostalCode = null, bool loadOnlyWithShoppingCart = false, ShoppingCartType? sct = null, - int pageIndex = 0, int pageSize = 2147483647, Expression> orderBySelector = null) + int pageIndex = 0, int pageSize = 2147483647, Expression> orderBySelector = null, + string[] excludedCustomerGroupIds = null, bool excludeVendorAndStaffAccounts = false) { var queryModel = new GetCustomerQuery { CreatedFromUtc = createdFromUtc, @@ -111,7 +114,9 @@ public virtual async Task> GetAllCustomers(DateTime? create Sct = sct, PageIndex = pageIndex, PageSize = pageSize, - OrderBySelector = orderBySelector + OrderBySelector = orderBySelector, + ExcludedCustomerGroupIds = excludedCustomerGroupIds, + ExcludeVendorAndStaffAccounts = excludeVendorAndStaffAccounts }; return await _mediator.Send(queryModel); } diff --git a/src/Tests/Grand.Business.Customers.Tests/Queries/Handlers/GetCustomerQueryHandlerTests.cs b/src/Tests/Grand.Business.Customers.Tests/Queries/Handlers/GetCustomerQueryHandlerTests.cs index dd8b1af5b6..cfdfd07cf8 100644 --- a/src/Tests/Grand.Business.Customers.Tests/Queries/Handlers/GetCustomerQueryHandlerTests.cs +++ b/src/Tests/Grand.Business.Customers.Tests/Queries/Handlers/GetCustomerQueryHandlerTests.cs @@ -46,4 +46,41 @@ public async Task Handle_PageSizeTwo_ReturnsFirstPageAndTotal() Assert.AreEqual(4, result.TotalCount); Assert.AreEqual(2, result.TotalPages); } + + [TestMethod] + public async Task Handle_ExcludedGroupsAndStaffAccounts_LeftOutInTheQuery() + { + var repository = new MongoDBRepositoryTest(); + var customer = new Customer { Email = "customer@x.com", Groups = { "registered" } }; + var wholesale = new Customer { Email = "wholesale@x.com", Groups = { "registered", "wholesale" } }; + repository.Insert(customer); + repository.Insert(wholesale); + repository.Insert(new Customer { Email = "vendor-group@x.com", Groups = { "registered", "vendors" } }); + repository.Insert(new Customer { Email = "manager-group@x.com", Groups = { "registered", "storemanager" } }); + repository.Insert(new Customer { Email = "vendor-link@x.com", Groups = { "registered" }, VendorId = "v1" }); + repository.Insert(new Customer { Email = "staff-link@x.com", Groups = { "registered" }, StaffStoreId = "s1" }); + var queryHandler = new GetCustomerQueryHandler(repository); + + var result = await queryHandler.Handle(new GetCustomerQuery { + ExcludedCustomerGroupIds = ["vendors", "storemanager"], + ExcludeVendorAndStaffAccounts = true + }, CancellationToken.None); + + CollectionAssert.AreEquivalent(new[] { "customer@x.com", "wholesale@x.com" }, + result.Select(c => c.Email).ToArray()); + Assert.AreEqual(2, result.TotalCount); + } + + [TestMethod] + public async Task Handle_NoExclusions_KeepsEveryone() + { + var repository = new MongoDBRepositoryTest(); + repository.Insert(new Customer { Groups = { "vendors" }, VendorId = "v1" }); + repository.Insert(new Customer { StaffStoreId = "s1" }); + + var result = await new GetCustomerQueryHandler(repository).Handle(new GetCustomerQuery(), + CancellationToken.None); + + Assert.AreEqual(2, result.TotalCount); + } } \ No newline at end of file diff --git a/src/Tests/Grand.Web.Admin.Tests/Controllers/StoreCustomerDataScopeTests.cs b/src/Tests/Grand.Web.Admin.Tests/Controllers/StoreCustomerDataScopeTests.cs index 95a0335b29..0c4c603054 100644 --- a/src/Tests/Grand.Web.Admin.Tests/Controllers/StoreCustomerDataScopeTests.cs +++ b/src/Tests/Grand.Web.Admin.Tests/Controllers/StoreCustomerDataScopeTests.cs @@ -10,7 +10,8 @@ namespace Grand.Web.Admin.Tests.Controllers; [TestClass] public class StoreCustomerDataScopeTests { - private static StoreCustomerDataScope Build(string staffStoreId, bool isRegistered = true) + private static StoreCustomerDataScope Build(string staffStoreId, bool isRegistered = true, + Action> groups = null) { var customer = new Customer { StaffStoreId = staffStoreId }; var workContextMock = new Mock(); @@ -20,10 +21,55 @@ private static StoreCustomerDataScope Build(string staffStoreId, bool isRegister var groupServiceMock = new Mock(); groupServiceMock.Setup(g => g.IsRegistered(It.IsAny())).ReturnsAsync(isRegistered); + groups?.Invoke(groupServiceMock); return new StoreCustomerDataScope(contextAccessorMock.Object, groupServiceMock.Object); } + //a store manager manages registered customers only: a staff account in reach could be reset and taken over + + [TestMethod] + [DataRow("admin")] + [DataRow("vendor")] + [DataRow("storemanager")] + [DataRow("salesmanager")] + public async Task HasAccess_StaffGroupMember_False(string role) + { + var scope = Build("store-1", groups: g => { + g.Setup(x => x.IsAdmin(It.IsAny())).ReturnsAsync(role == "admin"); + g.Setup(x => x.IsVendor(It.IsAny())).ReturnsAsync(role == "vendor"); + g.Setup(x => x.IsStoreManager(It.IsAny())).ReturnsAsync(role == "storemanager"); + g.Setup(x => x.IsSalesManager(It.IsAny())).ReturnsAsync(role == "salesmanager"); + }); + + Assert.IsFalse(await scope.HasAccess(new Customer { StoreId = "store-1" })); + } + + [TestMethod] + public async Task HasAccess_TiedToAVendor_False() + { + var scope = Build("store-1"); + + Assert.IsFalse(await scope.HasAccess(new Customer { StoreId = "store-1", VendorId = "vendor-1" })); + } + + [TestMethod] + public async Task HasAccess_TiedToAStaffStore_False() + { + var scope = Build("store-1"); + + Assert.IsFalse(await scope.HasAccess(new Customer { StoreId = "store-1", StaffStoreId = "store-1" })); + } + + [TestMethod] + public void StaffGroupSystemNames_CoverEveryStaffRole() + { + CollectionAssert.AreEquivalent(new[] { + SystemCustomerGroupNames.Administrators, SystemCustomerGroupNames.Vendors, + SystemCustomerGroupNames.StoreManager, SystemCustomerGroupNames.SalesManager + }, StoreCustomerDataScope.StaffGroupSystemNames); + } + [TestMethod] public async Task HasAccess_MatchingStoreId_Registered_NotDeleted_True() { diff --git a/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs b/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs index 363f4f39fd..9cf0c5282b 100644 --- a/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs +++ b/src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs @@ -445,6 +445,43 @@ public async Task PrepareCustomerList_NoCustomers_ReturnsEmpty() Assert.AreEqual(0, list.Count()); } + #region Store panel list + + private void VerifyListQuery(string[] excludedGroupIds, bool excludeVendorAndStaff) => + _customerServiceMock.Verify(c => c.GetAllCustomers(It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>>(), + It.Is(ids => excludedGroupIds == null + ? ids == null + : ids != null && ids.OrderBy(x => x).SequenceEqual(excludedGroupIds.OrderBy(x => x))), + excludeVendorAndStaff), Times.Once); + + [TestMethod] + public async Task PrepareCustomerList_StorePanel_LeavesTheStaffOutInTheQuery() + { + _scopeMock.Setup(s => s.GetDefaultStoreIdAsync()).ReturnsAsync("store-7"); + foreach (var name in StoreCustomerDataScope.StaffGroupSystemNames) + _groupServiceMock.Setup(g => g.GetCustomerGroupBySystemName(name)) + .ReturnsAsync(new CustomerGroup { Id = "id-" + name, SystemName = name }); + + await _customerViewModelService.PrepareCustomerList(new CustomerListModel(), null, null, 1, 10, "store-7"); + + VerifyListQuery(StoreCustomerDataScope.StaffGroupSystemNames.Select(n => "id-" + n).ToArray(), true); + } + + [TestMethod] + public async Task PrepareCustomerList_AdminPanel_ExcludesNobody() + { + await _customerViewModelService.PrepareCustomerList(new CustomerListModel(), null, null, 1, 10); + + VerifyListQuery(null, false); + } + + #endregion + #region Owner and sub-accounts [TestMethod] @@ -621,7 +658,8 @@ private void SetupGetAllCustomers(IPagedList result) It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), - It.IsAny>>())) + It.IsAny>>(), + It.IsAny(), It.IsAny())) .ReturnsAsync(result); } } diff --git a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs index e4fd2f596b..f9a4a61e30 100644 --- a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs +++ b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs @@ -124,7 +124,20 @@ public virtual async Task PrepareCustomerListModel() { var salesEmployeeId = _contextAccessor.WorkContext.CurrentCustomer.SeId; + //the Store panel lists its store's customers, never the staff (see StoreCustomerDataScope); left out in the + //query, so paging and the total stay right + var staffExcluded = await _scope.GetDefaultStoreIdAsync() is not null; + var excludedGroupIds = new List(); + if (staffExcluded) + foreach (var systemName in StoreCustomerDataScope.StaffGroupSystemNames) + { + var group = await _groupService.GetCustomerGroupBySystemName(systemName); + if (group != null) excludedGroupIds.Add(group.Id); + } + var customers = await _customerService.GetAllCustomers( + excludedCustomerGroupIds: staffExcluded ? excludedGroupIds.ToArray() : null, + excludeVendorAndStaffAccounts: staffExcluded, storeId: storeId, customerGroupIds: searchCustomerGroupIds, customerTagIds: searchCustomerTagIds, diff --git a/src/Web/Grand.Web.AdminShared/Services/StoreCustomerDataScope.cs b/src/Web/Grand.Web.AdminShared/Services/StoreCustomerDataScope.cs index bca8cd55e1..f1345bf105 100644 --- a/src/Web/Grand.Web.AdminShared/Services/StoreCustomerDataScope.cs +++ b/src/Web/Grand.Web.AdminShared/Services/StoreCustomerDataScope.cs @@ -6,13 +6,28 @@ using Grand.Web.AdminShared.Interfaces; namespace Grand.Web.AdminShared.Services; +/// +/// The Store panel manages its store's registered customers and nothing more: never the staff - administrators, +/// vendors, store managers, sales managers, or any account tied to a vendor or a staff store - whose accounts +/// a store manager could otherwise reset and take over. +/// public class StoreCustomerDataScope(IContextAccessor contextAccessor, IGroupService groupService) : IAdminDataScope { + /// Groups whose members are staff, left out of the Store panel's customer list + public static readonly string[] StaffGroupSystemNames = [ + SystemCustomerGroupNames.Administrators, SystemCustomerGroupNames.Vendors, + SystemCustomerGroupNames.StoreManager, SystemCustomerGroupNames.SalesManager + ]; + public async Task HasAccess(Customer entity) { if (entity is null || entity.Deleted) return false; if (entity.StoreId != contextAccessor.WorkContext.CurrentCustomer.StaffStoreId) return false; + if (!string.IsNullOrEmpty(entity.VendorId) || !string.IsNullOrEmpty(entity.StaffStoreId)) return false; + if (await groupService.IsAdmin(entity) || await groupService.IsVendor(entity) || + await groupService.IsStoreManager(entity) || await groupService.IsSalesManager(entity)) + return false; return await groupService.IsRegistered(entity); }