From 99b086450c0a2bb669a7f28dae4c3a65e43c52f3 Mon Sep 17 00:00:00 2001 From: support Date: Sun, 1 Nov 2026 09:00:36 +0100 Subject: [PATCH 1/5] Keep sub-account edits and password changes on the right account The edit command relied on the validator for ownership. It now checks it too and changes nothing for a missing, deleted or someone else's account. The edit page redirects for such an id instead of showing an empty form. Hiding the Subaccounts tab only removed the link, so every sub-account action now goes through one check that also honours the setting. The storefront validators now check an e-mail or username in the form it is stored in, within the store of the account being edited. Both are stored trimmed and lowercased, which is what every lookup expects, and a sub-account password is changed only when its e-mail resolves to that same account. With per-store identity one e-mail can belong to several stores, so the customer's own password change now uses the store of the signed-in account. --- .../SubAccountEditCommandHandlerTests.cs | 182 ++++++++++++++++-- .../AccountControllerSubAccountsTests.cs | 126 ++++++++++++ .../CustomerValidatorsStoreScopingTests.cs | 131 ++++++++++++- .../SubAccountDeleteCommandHandler.cs | 2 +- .../Customers/SubAccountEditCommandHandler.cs | 26 ++- .../UpdateCustomerInfoCommandHandler.cs | 30 ++- .../Controllers/AccountController.cs | 40 ++-- .../Customers/GetSubAccountHandler.cs | 21 +- .../Customer/CustomerInfoValidator.cs | 28 +-- .../Customer/SubAccountEditValidator.cs | 46 +++-- 10 files changed, 526 insertions(+), 106 deletions(-) create mode 100644 src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs diff --git a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs index 39337c109b..aa4b8aa8b9 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,45 @@ 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); + _customerServiceMock.Setup(c => c.GetCustomerByEmail(It.IsAny(), "store-9")).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 +72,138 @@ 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); - - var command = new SubAccountEditCommand { - CurrentCustomer = new Customer { Id = "owner" }, - EditModel = new SubAccountEditModel { Id = "c1", Email = "sub@x.com", Active = true } - }; + SubAccount(); - 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_AccountOfAnotherOwner_ChangesNothing() + { + SubAccount(ownerId: "another-owner"); + + var result = await _handler.Handle(Command(email: "new@x.com", password: "newpass"), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_AccountWithoutOwner_ChangesNothing() + { + SubAccount(ownerId: ""); + + var result = await _handler.Handle(Command(password: "newpass"), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_DeletedAccount_ChangesNothing() + { + SubAccount().Deleted = true; + + var result = await _handler.Handle(Command(password: "newpass"), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_MissingAccount_ChangesNothing() + { + var result = await _handler.Handle(Command(password: "newpass"), default); + + Assert.IsFalse(result); + VerifyNothingWritten(); + } + + [TestMethod] + public async Task Handle_EmailLeadsToAnotherAccount_DoesNotChangePassword() + { + SubAccount(); + _customerServiceMock.Setup(c => c.GetCustomerByEmail("sub@x.com", "store-9")) + .ReturnsAsync(new Customer { Id = "other", Email = "sub@x.com", StoreId = "store-9" }); + + var result = await _handler.Handle(Command(password: "newpass"), default); + + Assert.IsFalse(result); + _customerManagerServiceMock.Verify(m => m.ChangePassword( + It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task Handle_MixedCaseEmail_IsStoredLowercasedAndTrimmed() + { + 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_EmailStoredInMixedCase_IsRepairedEvenWhenChangesAreNotAllowed() + { + _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() + { + _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..f26b8cd0eb --- /dev/null +++ b/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs @@ -0,0 +1,126 @@ +using Grand.Business.Core.Interfaces.Authentication; +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Common.Localization; +using Grand.Business.Core.Interfaces.Customers; +using Grand.Business.Core.Interfaces.Messages; +using Grand.Business.Core.Utilities.Customers; +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.Controllers; +using Grand.Web.Features.Models.Customers; +using Grand.Web.Models.Customer; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Tests.Controllers; + +[TestClass] +public class AccountControllerSubAccountsTests +{ + private readonly Customer _customer = new() { Id = "owner", Email = "owner@x.com", StoreId = "store-9" }; + private Mock _customerManagerServiceMock; + private CustomerSettings _customerSettings; + private Mock _groupServiceMock; + private Mock _mediatorMock; + private AccountController _controller; + + [TestInitialize] + public void Init() + { + _customerManagerServiceMock = new Mock(); + _customerSettings = new CustomerSettings { HideSubAccountsTab = false }; + _groupServiceMock = new Mock(); + _groupServiceMock.Setup(g => g.IsOwner(_customer)).ReturnsAsync(true); + _mediatorMock = new Mock(); + + var workContext = new Mock(); + workContext.Setup(w => w.CurrentCustomer).Returns(_customer); + var contextAccessor = new Mock(); + contextAccessor.Setup(c => c.WorkContext).Returns(workContext.Object); + + _controller = new AccountController( + new Mock().Object, + new Mock().Object, + contextAccessor.Object, + new Mock().Object, + _groupServiceMock.Object, + _customerManagerServiceMock.Object, + new Mock().Object, + _mediatorMock.Object, + new Mock().Object, + new CaptchaSettings(), + _customerSettings, + new CustomerConfig()) { + ControllerContext = new ControllerContext { HttpContext = HtmlRequest() } + }; + } + + private static DefaultHttpContext HtmlRequest() + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.Headers.Accept = "text/html"; + return httpContext; + } + + private void VerifyNoCommand() + { + _mediatorMock.Verify(m => m.Send(It.IsAny(), It.IsAny()), Times.Never); + _mediatorMock.Verify(m => m.Send(It.IsAny(), It.IsAny()), Times.Never); + _mediatorMock.Verify(m => m.Send(It.IsAny(), It.IsAny()), Times.Never); + } + + private static void AssertRedirectsTo(string route, IActionResult result) + { + Assert.IsInstanceOfType(result); + Assert.AreEqual(route, ((RedirectToRouteResult)result).RouteName); + } + + [TestMethod] + public async Task SubAccountActions_TabHidden_RedirectAndChangeNothing() + { + _customerSettings.HideSubAccountsTab = true; + + AssertRedirectsTo("CustomerInfo", await _controller.SubAccounts()); + AssertRedirectsTo("CustomerInfo", await _controller.SubAccountAdd()); + AssertRedirectsTo("CustomerInfo", await _controller.SubAccountAdd(new SubAccountCreateModel())); + AssertRedirectsTo("CustomerInfo", await _controller.SubAccountEdit("c1")); + AssertRedirectsTo("CustomerInfo", await _controller.SubAccountEdit(new SubAccountEditModel())); + AssertRedirectsTo("CustomerInfo", await _controller.SubAccountDelete("c1")); + VerifyNoCommand(); + } + + [TestMethod] + public async Task SubAccountActions_NotAnOwner_ChallengeAndChangeNothing() + { + _groupServiceMock.Setup(g => g.IsOwner(_customer)).ReturnsAsync(false); + + Assert.IsInstanceOfType(await _controller.SubAccountAdd(new SubAccountCreateModel())); + Assert.IsInstanceOfType(await _controller.SubAccountEdit(new SubAccountEditModel())); + Assert.IsInstanceOfType(await _controller.SubAccountDelete("c1")); + VerifyNoCommand(); + } + + [TestMethod] + public async Task SubAccountEdit_AccountNotAvailable_RedirectsToTheList() + { + _mediatorMock.Setup(m => m.Send(It.IsAny(), It.IsAny())) + .ReturnsAsync((SubAccountEditModel)null); + + AssertRedirectsTo("CustomerSubAccounts", await _controller.SubAccountEdit("c1")); + } + + [TestMethod] + public async Task ChangePassword_IsScopedToTheStoreOfTheCustomer() + { + await _controller.ChangePassword(new ChangePasswordModel { OldPassword = "old", NewPassword = "new" }); + + _customerManagerServiceMock.Verify(m => m.ChangePassword( + It.Is(r => r.Email == "owner@x.com"), "store-9"), Times.Once); + } +} diff --git a/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs b/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs index 943686c5ed..9370440ce5 100644 --- a/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs +++ b/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs @@ -27,6 +27,7 @@ namespace Grand.Web.Tests.Validators.Customers; public class CustomerValidatorsStoreScopingTests { private const string StoreId = "store-1"; + private const string OwnerId = "owner"; private static Mock Translation() { @@ -38,7 +39,9 @@ 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 = OwnerId, Email = currentCustomerEmail, StoreId = StoreId + }); var store = new Mock(); store.Setup(s => s.CurrentStore).Returns(new Grand.Domain.Stores.Store { Id = StoreId }); var ctx = new Mock(); @@ -170,23 +173,131 @@ await validator.ValidateAsync(new SubAccountCreateModel { } [TestMethod] - public async Task SubAccountEditValidator_ScopesEmailLookupToCurrentStore_WhenEmailChanged() + public async Task SubAccountEditValidator_ScopesEmailLookupToTheAccountStore_WhenEmailChanged() + { + var (validator, customerService) = SubAccountEdit(new Customer { + Id = "c1", Email = "old@x.com", StoreId = "store-2", OwnerId = OwnerId + }); + + await validator.ValidateAsync(SubAccountEditModel("new@x.com")); + + //the account's store, as the current store (store-1) is not where this address has to stay unique + customerService.Verify(c => c.GetCustomerByEmail("new@x.com", "store-2"), Times.Once); + } + + [TestMethod] + public async Task SubAccountEditValidator_ChecksTheAddressAsItWillBeStored() + { + var (validator, customerService) = SubAccountEdit(new Customer { + Id = "c1", Email = "old@x.com", StoreId = StoreId, OwnerId = OwnerId + }); + customerService.Setup(c => c.GetCustomerByEmail("taken@x.com", StoreId)) + .ReturnsAsync(new Customer { Id = "other" }); + + var result = await validator.ValidateAsync(SubAccountEditModel(" Taken@X.com ")); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.EmailAlreadyExists")); + } + + [TestMethod] + public async Task SubAccountEditValidator_RepairingCaseChecksTheAddress_WhenChangesAreNotAllowed() + { + var (validator, customerService) = SubAccountEdit(new Customer { + Id = "c1", Email = "Taken@X.com", StoreId = StoreId, OwnerId = OwnerId + }, new CustomerSettings { AllowUsersToChangeEmail = false }); + customerService.Setup(c => c.GetCustomerByEmail("taken@x.com", StoreId)) + .ReturnsAsync(new Customer { Id = "other" }); + + var result = await validator.ValidateAsync(SubAccountEditModel("Taken@X.com")); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.EmailAlreadyExists")); + } + + [TestMethod] + [DataRow("another-owner", false)] + [DataRow("", false)] + [DataRow(OwnerId, true)] + public async Task SubAccountEditValidator_RejectsDeletedOrNotOwnedAccount(string ownerId, bool deleted) + { + var (validator, _) = SubAccountEdit(new Customer { + Id = "c1", Email = "sub@x.com", StoreId = StoreId, OwnerId = ownerId, Deleted = deleted + }); + + var result = await validator.ValidateAsync(SubAccountEditModel("sub@x.com")); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "You are not owner of this account")); + } + + [TestMethod] + public async Task SubAccountEditValidator_MissingEmailIsReportedWithoutThrowing() + { + var (validator, _) = SubAccountEdit(new Customer { + Id = "c1", Email = "sub@x.com", StoreId = StoreId, OwnerId = OwnerId + }); + + var result = await validator.ValidateAsync(SubAccountEditModel(null)); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.Fields.Email.Required")); + } + + [TestMethod] + public async Task CustomerInfoValidator_ChecksTheAddressAsItWillBeStored() + { + var customerService = new Mock(); + customerService.Setup(c => c.GetCustomerByEmail("taken@x.com", StoreId)) + .ReturnsAsync(new Customer { Id = "other" }); + + var result = await CustomerInfo(customerService, new CustomerSettings { AllowUsersToChangeEmail = true }) + .ValidateAsync(new CustomerInfoModel { Email = " Taken@X.com " }); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.EmailAlreadyExists")); + } + + [TestMethod] + public async Task CustomerInfoValidator_ChecksTheUsernameAsItWillBeStored() + { + var customerService = new Mock(); + customerService.Setup(c => c.GetCustomerByUsername("taken", StoreId)) + .ReturnsAsync(new Customer { Id = "other" }); + + var result = await CustomerInfo(customerService, new CustomerSettings { + UsernamesEnabled = true, AllowUsersToChangeUsernames = true + }).ValidateAsync(new CustomerInfoModel { Email = "old@x.com", Username = " Taken " }); + + Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.UsernameAlreadyExists")); + } + + private static (SubAccountEditValidator validator, Mock customerService) SubAccountEdit( + Customer account, CustomerSettings settings = null) { var customerService = new Mock(); - customerService.Setup(c => c.GetCustomerById("c1")) - .ReturnsAsync(new Customer { Id = "c1", Email = "old@x.com" }); + customerService.Setup(c => c.GetCustomerById(account.Id)).ReturnsAsync(account); var groupService = new Mock(); groupService.Setup(g => g.IsRegistered(It.IsAny())).ReturnsAsync(true); - var validator = new SubAccountEditValidator( + return (new SubAccountEditValidator( new List>(), customerService.Object, groupService.Object, Translation().Object, - Context(), new CustomerSettings { AllowUsersToChangeEmail = true }, PerStoreOn().Config); + Context(), settings ?? new CustomerSettings { AllowUsersToChangeEmail = true }, PerStoreOn().Config), + customerService); + } - await validator.ValidateAsync(new SubAccountEditModel { - Id = "c1", Email = "new@x.com", FirstName = "A", LastName = "B" - }); + private static SubAccountEditModel SubAccountEditModel(string email) => new() { + Id = "c1", Email = email, FirstName = "A", LastName = "B" + }; - customerService.Verify(c => c.GetCustomerByEmail(It.IsAny(), StoreId), Times.AtLeastOnce); + private static CustomerInfoValidator CustomerInfo(Mock customerService, CustomerSettings settings) + { + var mediator = new Mock(); + mediator.Setup(m => m.Send(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + var attrParser = new Mock(); + attrParser.Setup(p => p.GetAttributeWarnings(It.IsAny>(), It.IsAny())) + .ReturnsAsync(new List()); + + return new CustomerInfoValidator( + new List>(), + Context("old@x.com"), customerService.Object, mediator.Object, attrParser.Object, + Translation().Object, new Mock().Object, settings, PerStoreOn().Config); } } diff --git a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs index 31026f5bfc..a1aed5d9f1 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountDeleteCommandHandler.cs @@ -19,7 +19,7 @@ public SubAccountDeleteCommandHandler(ICustomerService customerService) ArgumentNullException.ThrowIfNull(request.CurrentCustomer); var customer = await _customerService.GetCustomerById(request.CustomerId); - if (customer == null || customer.OwnerId != request.CurrentCustomer.Id) + if (customer == null || customer.Deleted || customer.OwnerId != request.CurrentCustomer.Id) return (false, "You are not owner of this account"); //delete 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..3fe898f27e 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs @@ -26,19 +26,37 @@ 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 lookups miss it. An address + //differing only in case is the same address, so it is repaired even when changing it is not allowed + var email = request.EditModel.Email.Trim().ToLowerInvariant(); + var sameAddress = string.Equals(customer.Email, email, StringComparison.OrdinalIgnoreCase); + if (customer.Email != email && (sameAddress || _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) if (!string.IsNullOrEmpty(request.EditModel.Password)) + { + //the password change finds its account by email, which has to lead back to this account + var emailHolder = await _customerService.GetCustomerByEmail(customer.Email, customer.StoreId); + if (emailHolder?.Id != customer.Id) + return false; + await _customerManagerService.ChangePassword( new ChangePasswordRequest(customer.Email, _customerSettings.DefaultPasswordFormat, request.EditModel.Password), customer.StoreId); + } //update active customer.Active = request.EditModel.Active; diff --git a/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs b/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs index 08b64e22c5..967088e3bf 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs @@ -38,26 +38,36 @@ public UpdateCustomerInfoCommandHandler( public async Task Handle(UpdateCustomerInfoCommand request, CancellationToken cancellationToken) { - //username + //username - stored lowercased like every other write, or the lowercased lookups miss it 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) + //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)) { //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) diff --git a/src/Web/Grand.Web/Controllers/AccountController.cs b/src/Web/Grand.Web/Controllers/AccountController.cs index 3317f5fcca..d6878668e1 100644 --- a/src/Web/Grand.Web/Controllers/AccountController.cs +++ b/src/Web/Grand.Web/Controllers/AccountController.cs @@ -927,7 +927,9 @@ public virtual async Task ChangePassword(ChangePasswordModel mode var changePasswordRequest = new ChangePasswordRequest(_contextAccessor.WorkContext.CurrentCustomer.Email, _customerSettings.DefaultPasswordFormat, model.NewPassword, model.OldPassword); - await _customerManagerService.ChangePassword(changePasswordRequest); + //scope the lookup to the customer's store - the same e-mail may exist in another store with per-store identity + await _customerManagerService.ChangePassword(changePasswordRequest, + _contextAccessor.WorkContext.CurrentCustomer.StoreId); var customer = await _customerService.GetCustomerById(_contextAccessor.WorkContext.CurrentCustomer.Id); //sign in @@ -1111,9 +1113,7 @@ await _customerService.UpdateUserField(_contextAccessor.WorkContext.Curr #region My account / Sub accounts - [HttpGet] - [CustomerGroupAuthorize(SystemCustomerGroupNames.Registered)] - public virtual async Task SubAccounts() + private async Task SubAccountsDenied() { if (!await _groupService.IsOwner(_contextAccessor.WorkContext.CurrentCustomer)) return Challenge(); @@ -1121,6 +1121,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); @@ -1130,8 +1140,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 @@ -1143,8 +1153,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); @@ -1161,11 +1171,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); } @@ -1174,8 +1186,8 @@ 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); @@ -1192,8 +1204,8 @@ 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, 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/CustomerInfoValidator.cs b/src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs index 528e915db5..63c3ad890d 100644 --- a/src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs @@ -26,10 +26,6 @@ public CustomerInfoValidator( CustomerConfig customerConfig) : base(validators) { - var storeId = customerConfig.RegisterCustomersPerStore - ? contextAccessor.StoreContext.CurrentStore.Id - : ""; - RuleFor(x => x.Email).NotEmpty() .WithMessage(translationService.GetResource("Account.Fields.Email.Required")); RuleFor(x => x.Email).EmailAddress().WithMessage(translationService.GetResource("Common.WrongEmail")); @@ -126,28 +122,36 @@ public CustomerInfoValidator( contextAccessor.StoreContext.CurrentStore.Id); foreach (var error in customerAttributeWarnings) context.AddFailure(error); - if (contextAccessor.WorkContext.CurrentCustomer.Email != x.Email.ToLower() && customerSettings.AllowUsersToChangeEmail) + //the same values the command stores, checked where the account lives + var currentCustomer = contextAccessor.WorkContext.CurrentCustomer; + var storeId = customerConfig.RegisterCustomersPerStore ? currentCustomer.StoreId : ""; + + var email = x.Email?.Trim().ToLowerInvariant(); + var sameAddress = string.Equals(currentCustomer.Email, email, StringComparison.OrdinalIgnoreCase); + if (!string.IsNullOrEmpty(email) && currentCustomer.Email != email && + (sameAddress || customerSettings.AllowUsersToChangeEmail)) { - if (!CommonHelper.IsValidEmail(x.Email)) + if (!CommonHelper.IsValidEmail(email)) context.AddFailure( translationService.GetResource("Account.EmailUsernameErrors.NewEmailIsNotValid")); - if (x.Email.Length > 100) + if (email.Length > 100) context.AddFailure(translationService.GetResource("Account.EmailUsernameErrors.EmailTooLong")); - var customer2 = await customerService.GetCustomerByEmail(x.Email, storeId); + var customer2 = await customerService.GetCustomerByEmail(email, storeId); if (customer2 != null) context.AddFailure( translationService.GetResource("Account.EmailUsernameErrors.EmailAlreadyExists")); } - if (customerSettings.UsernamesEnabled && customerSettings.AllowUsersToChangeUsernames && - contextAccessor.WorkContext.CurrentCustomer.Username != x.Username.ToLower()) + var username = x.Username?.Trim().ToLowerInvariant(); + if (!string.IsNullOrEmpty(username) && customerSettings.UsernamesEnabled && + customerSettings.AllowUsersToChangeUsernames && currentCustomer.Username != username) { - if (x.Username.ToLower().Length > 100) + if (username.Length > 100) context.AddFailure(translationService.GetResource("Account.EmailUsernameErrors.UsernameTooLong")); - var customer2 = await customerService.GetCustomerByUsername(x.Username.ToLower(), storeId); + var customer2 = await customerService.GetCustomerByUsername(username, storeId); if (customer2 != null) context.AddFailure( translationService.GetResource("Account.EmailUsernameErrors.UsernameAlreadyExists")); diff --git a/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs b/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs index c719ff68c2..b96c504530 100644 --- a/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs @@ -22,10 +22,6 @@ public SubAccountEditValidator( CustomerConfig customerConfig) : base(validators) { - var storeId = customerConfig.RegisterCustomersPerStore - ? contextAccessor.StoreContext.CurrentStore.Id - : ""; - RuleFor(x => x.Email).NotEmpty().WithMessage(translationService.GetResource("Account.Fields.Email.Required")); RuleFor(x => x.Email).EmailAddress().WithMessage(translationService.GetResource("Common.WrongEmail")); @@ -43,36 +39,38 @@ public SubAccountEditValidator( { var customer = await customerService.GetCustomerById(x.Id); - switch (customer) + //ownership first: a missing id, a deleted account and someone else's account get the same answer + if (customer == null || customer.Deleted || + customer.OwnerId != contextAccessor.WorkContext.CurrentCustomer.Id) { - case null: - context.AddFailure( - translationService.GetResource("Account.NoExists")); - break; - case { Deleted: true }: - context.AddFailure( - translationService.GetResource("Account.DeleteAccount")); - break; - case not null when !await groupService.IsRegistered(customer): - context.AddFailure( - translationService.GetResource( - "Account is not registered")); - break; + context.AddFailure("You are not owner of this account"); + return; } - if (customer != null && customer.OwnerId != contextAccessor.WorkContext.CurrentCustomer.Id) - context.AddFailure("You are not owner of this account"); + if (!await groupService.IsRegistered(customer)) + { + context.AddFailure(translationService.GetResource("Account is not registered")); + return; + } + + //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) + //the same address the command stores, checked where the account lives + var email = x.Email.Trim().ToLowerInvariant(); + var sameAddress = string.Equals(customer.Email, email, StringComparison.OrdinalIgnoreCase); + if (customer.Email != email && (sameAddress || customerSettings.AllowUsersToChangeEmail)) { - if (!CommonHelper.IsValidEmail(x.Email)) + if (!CommonHelper.IsValidEmail(email)) context.AddFailure( translationService.GetResource("Account.EmailUsernameErrors.NewEmailIsNotValid")); - if (x.Email.Length > 100) + if (email.Length > 100) context.AddFailure(translationService.GetResource("Account.EmailUsernameErrors.EmailTooLong")); - var customer2 = await customerService.GetCustomerByEmail(x.Email, storeId); + var storeId = customerConfig.RegisterCustomersPerStore ? customer.StoreId : ""; + var customer2 = await customerService.GetCustomerByEmail(email, storeId); if (customer2 != null && customer.Id != customer2.Id) context.AddFailure( translationService.GetResource("Account.EmailUsernameErrors.EmailAlreadyExists")); From 4e4760a01efc58765ec55a461faa81d8eac6be01 Mon Sep 17 00:00:00 2001 From: support Date: Sun, 1 Nov 2026 09:54:20 +0100 Subject: [PATCH 2/5] Change a password on the account itself, not through its e-mail These callers already hold the account, but the password change looked it up again by e-mail, a value that can change between the two steps and is unique only as far as the application keeps it so. A new overload takes the customer and the existing method now resolves the e-mail and calls it. The sub-account edit no longer needs its check that the e-mail leads back to the account, nor can it skip the remaining edits without telling the owner. --- .../Customers/ICustomerManagerService.cs | 8 +++ .../Services/CustomerManagerService.cs | 22 ++++++-- .../Services/CustomerManagerServiceTests.cs | 23 +++++++++ .../SubAccountEditCommandHandlerTests.cs | 50 +++++++++---------- .../AccountControllerSubAccountsTests.cs | 8 +-- .../Customers/SubAccountEditCommandHandler.cs | 15 ++---- .../Controllers/AccountController.cs | 8 +-- 7 files changed, 82 insertions(+), 52 deletions(-) diff --git a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs index 9536c9197c..6aedff58d8 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs @@ -39,4 +39,12 @@ public interface ICustomerManagerService /// Request /// Store identifier - used to resolve the customer when per-store customer identity is enabled Task ChangePassword(ChangePasswordRequest request, string storeId = ""); + + /// + /// Change password of this customer, without resolving the account by e-mail + /// + /// Customer + /// Password format + /// New password + Task ChangePassword(Customer customer, PasswordFormat passwordFormat, string newPassword); } \ No newline at end of file diff --git a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs index 7768df188f..c6386ddb4a 100644 --- a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs +++ b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs @@ -173,32 +173,44 @@ public virtual async Task ChangePassword(ChangePasswordRequest request, string s ArgumentNullException.ThrowIfNull(request); var customer = await _customerService.GetCustomerByEmail(request.Email, storeId); + + await ChangePassword(customer, request.PasswordFormat, request.NewPassword); + } + + /// + /// Change password of this customer, without resolving the account by e-mail + /// + /// Customer + /// Password format + /// New password + public virtual async Task ChangePassword(Customer customer, PasswordFormat passwordFormat, string newPassword) + { ArgumentNullException.ThrowIfNull(customer); - switch (request.PasswordFormat) + switch (passwordFormat) { case PasswordFormat.Clear: { - customer.Password = request.NewPassword; + customer.Password = newPassword; } break; case PasswordFormat.Encrypted: { customer.PasswordSalt = CommonHelper.GenerateRandomDigitCode(24); - customer.Password = _encryptionService.EncryptText(request.NewPassword, customer.PasswordSalt); + customer.Password = _encryptionService.EncryptText(newPassword, customer.PasswordSalt); } break; case PasswordFormat.Hashed: { //modern self-describing PBKDF2 hash - salt and parameters are embedded in the value itself customer.PasswordSalt = string.Empty; - customer.Password = _encryptionService.HashPassword(request.NewPassword); + customer.Password = _encryptionService.HashPassword(newPassword); } break; } customer.PasswordChangeDateUtc = DateTime.UtcNow; - customer.PasswordFormatId = request.PasswordFormat; + customer.PasswordFormatId = passwordFormat; await _customerService.UpdateCustomer(customer); //insert password history await _customerHistoryPasswordService.InsertCustomerPassword(customer); diff --git a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs index c9137a7e30..535e03b113 100644 --- a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs +++ b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs @@ -100,6 +100,29 @@ public async Task ChangePassword_PassesStoreIdToLookup() _customerServiceMock.Verify(c => c.GetCustomerByEmail("admin@admin.com", "store-1"), Times.Once); } + [TestMethod] + public async Task ChangePassword_ForCustomer_ChangesThatCustomerWithoutLookup() + { + //Arrange + var customer = new Customer { Id = "c1", Email = "a@x.com", PasswordFormatId = PasswordFormat.Clear, Password = "123456" }; + //Act + await _customerManagerService.ChangePassword(customer, PasswordFormat.Clear, "zxcvbn"); + //Assert + Assert.AreEqual("zxcvbn", customer.Password); + _customerServiceMock.Verify(c => c.UpdateCustomer(customer), Times.Once); + _customerServiceMock.Verify(c => c.GetCustomerByEmail(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task ChangePassword_UnknownEmail_Throws() + { + //Arrange + var changepassword = new ChangePasswordRequest("nobody@x.com", PasswordFormat.Clear, "zxcvbn"); + //Act + Assert + await Assert.ThrowsExactlyAsync(() => + _customerManagerService.ChangePassword(changepassword, "store-1")); + } + [TestMethod] public async Task ChangePasswordTest_Success() { diff --git a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs index aa4b8aa8b9..73becd1ced 100644 --- a/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs +++ b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs @@ -34,10 +34,18 @@ private Customer SubAccount(string ownerId = "owner") Id = "c1", Email = "sub@x.com", Username = "sub@x.com", StoreId = "store-9", OwnerId = ownerId }; _customerServiceMock.Setup(c => c.GetCustomerById("c1")).ReturnsAsync(customer); - _customerServiceMock.Setup(c => c.GetCustomerByEmail(It.IsAny(), "store-9")).ReturnsAsync(customer); return customer; } + private void VerifyPasswordChangedOn(Customer customer) + { + _customerManagerServiceMock.Verify(m => m.ChangePassword(customer, It.IsAny(), "newpass"), + Times.Once); + //never through a lookup by e-mail, which may lead to another account + _customerManagerServiceMock.Verify(m => m.ChangePassword(It.IsAny(), + It.IsAny()), Times.Never); + } + private static SubAccountEditCommand Command(string email = "sub@x.com", string password = null) => new() { CurrentCustomer = new Customer { Id = "owner" }, EditModel = new SubAccountEditModel { @@ -54,19 +62,22 @@ private void VerifyNothingWritten() It.IsAny(), It.IsAny()), Times.Never); _customerManagerServiceMock.Verify(m => m.ChangePassword(It.IsAny(), It.IsAny()), Times.Never); + _customerManagerServiceMock.Verify(m => m.ChangePassword(It.IsAny(), It.IsAny(), + It.IsAny()), Times.Never); } [TestMethod] - public async Task Handle_WithPassword_ChangesPasswordScopedToCustomerStore() + public async Task Handle_WithPassword_ChangesPasswordOfTheEditedAccount() { - SubAccount(); + var customer = SubAccount(); + //another account holding the same address must not matter + _customerServiceMock.Setup(c => c.GetCustomerByEmail("sub@x.com", "store-9")) + .ReturnsAsync(new Customer { Id = "other", Email = "sub@x.com", StoreId = "store-9" }); 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) - _customerManagerServiceMock.Verify(m => m.ChangePassword( - It.Is(r => r.Email == "sub@x.com"), "store-9"), Times.Once); + VerifyPasswordChangedOn(customer); } [TestMethod] @@ -79,6 +90,8 @@ public async Task Handle_WithoutPassword_DoesNotChangePassword() Assert.IsTrue(result); _customerManagerServiceMock.Verify(m => m.ChangePassword( It.IsAny(), It.IsAny()), Times.Never); + _customerManagerServiceMock.Verify(m => m.ChangePassword( + It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); } [TestMethod] @@ -123,20 +136,6 @@ public async Task Handle_MissingAccount_ChangesNothing() VerifyNothingWritten(); } - [TestMethod] - public async Task Handle_EmailLeadsToAnotherAccount_DoesNotChangePassword() - { - SubAccount(); - _customerServiceMock.Setup(c => c.GetCustomerByEmail("sub@x.com", "store-9")) - .ReturnsAsync(new Customer { Id = "other", Email = "sub@x.com", StoreId = "store-9" }); - - var result = await _handler.Handle(Command(password: "newpass"), default); - - Assert.IsFalse(result); - _customerManagerServiceMock.Verify(m => m.ChangePassword( - It.IsAny(), It.IsAny()), Times.Never); - } - [TestMethod] public async Task Handle_MixedCaseEmail_IsStoredLowercasedAndTrimmed() { @@ -150,14 +149,14 @@ public async Task Handle_MixedCaseEmail_IsStoredLowercasedAndTrimmed() } [TestMethod] - public async Task Handle_NewEmailAndPassword_ChangesPasswordOfTheNewEmail() + public async Task Handle_NewEmailAndPassword_ChangesPasswordOfTheEditedAccount() { - SubAccount(); + var customer = 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); + Assert.AreEqual("new@x.com", customer.Email); + VerifyPasswordChangedOn(customer); } [TestMethod] @@ -192,8 +191,7 @@ public async Task Handle_EmailStoredInMixedCase_IsRepairedEvenWhenChangesAreNotA 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); + VerifyPasswordChangedOn(customer); } [TestMethod] diff --git a/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs b/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs index f26b8cd0eb..47ff3156b5 100644 --- a/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs +++ b/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs @@ -116,11 +116,13 @@ public async Task SubAccountEdit_AccountNotAvailable_RedirectsToTheList() } [TestMethod] - public async Task ChangePassword_IsScopedToTheStoreOfTheCustomer() + public async Task ChangePassword_ChangesThePasswordOfTheSignedInAccount() { await _controller.ChangePassword(new ChangePasswordModel { OldPassword = "old", NewPassword = "new" }); - _customerManagerServiceMock.Verify(m => m.ChangePassword( - It.Is(r => r.Email == "owner@x.com"), "store-9"), Times.Once); + _customerManagerServiceMock.Verify(m => m.ChangePassword(_customer, It.IsAny(), "new"), + Times.Once); + _customerManagerServiceMock.Verify(m => m.ChangePassword(It.IsAny(), + It.IsAny()), Times.Never); } } diff --git a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs index 3fe898f27e..472a2996c8 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs @@ -1,5 +1,4 @@ using Grand.Business.Core.Interfaces.Customers; -using Grand.Business.Core.Utilities.Customers; using Grand.Domain.Customers; using Grand.Web.Commands.Models.Customers; using Grand.Mediator; @@ -45,18 +44,10 @@ public async Task Handle(SubAccountEditCommand request, CancellationToken } } - //update password (scope the lookup to the sub-account's store - safe with or without per-store identity) + //update password - of this account, the one the ownership check above is about if (!string.IsNullOrEmpty(request.EditModel.Password)) - { - //the password change finds its account by email, which has to lead back to this account - var emailHolder = await _customerService.GetCustomerByEmail(customer.Email, customer.StoreId); - if (emailHolder?.Id != customer.Id) - return false; - - await _customerManagerService.ChangePassword( - new ChangePasswordRequest(customer.Email, _customerSettings.DefaultPasswordFormat, - request.EditModel.Password), customer.StoreId); - } + await _customerManagerService.ChangePassword(customer, _customerSettings.DefaultPasswordFormat, + request.EditModel.Password); //update active customer.Active = request.EditModel.Active; diff --git a/src/Web/Grand.Web/Controllers/AccountController.cs b/src/Web/Grand.Web/Controllers/AccountController.cs index d6878668e1..442263ef91 100644 --- a/src/Web/Grand.Web/Controllers/AccountController.cs +++ b/src/Web/Grand.Web/Controllers/AccountController.cs @@ -924,12 +924,8 @@ public virtual async Task ChangePassword(ChangePasswordModel mode { if (!ModelState.IsValid) return View(model); - var changePasswordRequest = new ChangePasswordRequest(_contextAccessor.WorkContext.CurrentCustomer.Email, - _customerSettings.DefaultPasswordFormat, model.NewPassword, model.OldPassword); - - //scope the lookup to the customer's store - the same e-mail may exist in another store with per-store identity - await _customerManagerService.ChangePassword(changePasswordRequest, - _contextAccessor.WorkContext.CurrentCustomer.StoreId); + await _customerManagerService.ChangePassword(_contextAccessor.WorkContext.CurrentCustomer, + _customerSettings.DefaultPasswordFormat, model.NewPassword); var customer = await _customerService.GetCustomerById(_contextAccessor.WorkContext.CurrentCustomer.Id); //sign in From 82f2c8fa422d71d66a65233bda5901a7cf38a490 Mon Sep 17 00:00:00 2001 From: support Date: Sun, 1 Nov 2026 10:28:50 +0100 Subject: [PATCH 3/5] Write only the password fields when changing a password The password change saved the whole customer, including its e-mail, username, store and active flag. With a copy the caller loaded earlier in the request, that could undo an edit made in the meantime, such as a deactivation. A dedicated update now writes the four password fields in one operation, and still refuses a system account and raises the update event. --- .../Interfaces/Customers/ICustomerService.cs | 6 +++ .../Services/CustomerManagerService.cs | 3 +- .../Services/CustomerService.cs | 20 ++++++++++ .../Services/CustomerManagerServiceTests.cs | 4 +- .../Services/CustomerServiceTests.cs | 38 +++++++++++++++++++ 5 files changed, 69 insertions(+), 2 deletions(-) diff --git a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs index 685404e62b..a2e999fe7d 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs @@ -190,6 +190,12 @@ Task UpdateCustomerField(string customerId, /// Customer Task UpdateActive(Customer customer); + /// + /// Updates the password fields of the customer, and nothing else + /// + /// Customer + Task UpdateCustomerPassword(Customer customer); + /// /// Update the customer /// diff --git a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs index c6386ddb4a..738df793f7 100644 --- a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs +++ b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs @@ -211,7 +211,8 @@ public virtual async Task ChangePassword(Customer customer, PasswordFormat passw customer.PasswordChangeDateUtc = DateTime.UtcNow; customer.PasswordFormatId = passwordFormat; - await _customerService.UpdateCustomer(customer); + //only the password fields: the caller's copy may be older than the account + await _customerService.UpdateCustomerPassword(customer); //insert password history await _customerHistoryPasswordService.InsertCustomerPassword(customer); diff --git a/src/Business/Grand.Business.Customers/Services/CustomerService.cs b/src/Business/Grand.Business.Customers/Services/CustomerService.cs index 143fe78201..127eb46794 100644 --- a/src/Business/Grand.Business.Customers/Services/CustomerService.cs +++ b/src/Business/Grand.Business.Customers/Services/CustomerService.cs @@ -551,6 +551,26 @@ public virtual async Task UpdateCustomerInAdminPanel(Customer customer) } + public virtual async Task UpdateCustomerPassword(Customer customer) + { + ArgumentNullException.ThrowIfNull(customer); + + if (customer.IsSystemAccount) + throw new GrandException( + $"System customer account ({(string.IsNullOrEmpty(customer.SystemName) ? customer.Email : customer.SystemName)}) could not be updated"); + + var update = UpdateBuilder.Create() + .Set(x => x.Password, customer.Password) + .Set(x => x.PasswordSalt, customer.PasswordSalt) + .Set(x => x.PasswordFormatId, customer.PasswordFormatId) + .Set(x => x.PasswordChangeDateUtc, customer.PasswordChangeDateUtc); + + await _customerRepository.UpdateOneAsync(x => x.Id == customer.Id, update); + + //event notification + await _mediator.EntityUpdated(customer); + } + public virtual async Task UpdateActive(Customer customer) { ArgumentNullException.ThrowIfNull(customer); diff --git a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs index 535e03b113..bb2ce32673 100644 --- a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs +++ b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs @@ -109,7 +109,9 @@ public async Task ChangePassword_ForCustomer_ChangesThatCustomerWithoutLookup() await _customerManagerService.ChangePassword(customer, PasswordFormat.Clear, "zxcvbn"); //Assert Assert.AreEqual("zxcvbn", customer.Password); - _customerServiceMock.Verify(c => c.UpdateCustomer(customer), Times.Once); + _customerServiceMock.Verify(c => c.UpdateCustomerPassword(customer), Times.Once); + //the whole-customer update would write back everything else the caller's copy holds + _customerServiceMock.Verify(c => c.UpdateCustomer(It.IsAny()), Times.Never); _customerServiceMock.Verify(c => c.GetCustomerByEmail(It.IsAny(), It.IsAny()), Times.Never); } diff --git a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerServiceTests.cs b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerServiceTests.cs index ee12c930ae..0d755fe0b4 100644 --- a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerServiceTests.cs +++ b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerServiceTests.cs @@ -9,6 +9,7 @@ using Grand.Infrastructure.Configuration; using Grand.Infrastructure.Tests.Caching; using Grand.Mediator; +using Grand.SharedKernel; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; @@ -474,6 +475,43 @@ public async Task UpdateCustomerinAdminPanelTest() Assert.AreEqual("store-1", updated.StoreId); } + [TestMethod] + public async Task UpdateCustomerPassword_WritesOnlyThePasswordFields() + { + //Arrange + await _repository.InsertAsync(new Customer { + Id = "c1", Email = "current@x.com", Username = "current", StoreId = "store-1", Active = false, + Password = "old", PasswordSalt = "salt", PasswordFormatId = PasswordFormat.Clear + }); + //taken before the account changed, as a caller holds it during a request + var changedAt = new DateTime(2026, 11, 1, 12, 0, 0, DateTimeKind.Utc); + var snapshot = new Customer { + Id = "c1", Email = "stale@x.com", Username = "stale", StoreId = "store-2", Active = true, + Password = "new", PasswordSalt = "", PasswordFormatId = PasswordFormat.Hashed, + PasswordChangeDateUtc = changedAt + }; + //Act + await _customerService.UpdateCustomerPassword(snapshot); + //Assert + var stored = _repository.Table.First(x => x.Id == "c1"); + Assert.AreEqual("new", stored.Password); + Assert.AreEqual("", stored.PasswordSalt); + Assert.AreEqual(PasswordFormat.Hashed, stored.PasswordFormatId); + Assert.AreEqual(changedAt, stored.PasswordChangeDateUtc); + Assert.AreEqual("current@x.com", stored.Email); + Assert.AreEqual("current", stored.Username); + Assert.AreEqual("store-1", stored.StoreId); + Assert.IsFalse(stored.Active); + } + + [TestMethod] + public async Task UpdateCustomerPassword_SystemAccount_Throws() + { + var customer = new Customer { Id = "c1", IsSystemAccount = true, SystemName = "system" }; + + await Assert.ThrowsExactlyAsync(() => _customerService.UpdateCustomerPassword(customer)); + } + [TestMethod] public async Task UpdateActiveTest() { From 6c33de270a99f25d574e9f3690708eee339baaa0 Mon Sep 17 00:00:00 2001 From: support Date: Sun, 1 Nov 2026 10:38:27 +0100 Subject: [PATCH 4/5] Keep existing implementations loading without a rebuild 2.4.x is a patch line, so plugins built for 2.4.0 must keep working. The new interface members get default bodies that use the existing methods, so an implementation built for an earlier 2.4 release still loads and behaves as it did. The shipped services override them. --- .../Customers/ICustomerManagerService.cs | 4 +- .../Interfaces/Customers/ICustomerService.cs | 3 +- .../Services/CustomerManagerServiceTests.cs | 43 +++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs index 6aedff58d8..e3515b3fe2 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerManagerService.cs @@ -46,5 +46,7 @@ public interface ICustomerManagerService /// Customer /// Password format /// New password - Task ChangePassword(Customer customer, PasswordFormat passwordFormat, string newPassword); + /// The default body keeps implementations built for an earlier 2.4 release loading without a rebuild + Task ChangePassword(Customer customer, PasswordFormat passwordFormat, string newPassword) => + ChangePassword(new ChangePasswordRequest(customer.Email, passwordFormat, newPassword), customer.StoreId); } \ No newline at end of file diff --git a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs index a2e999fe7d..d1c2919f6c 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs @@ -194,7 +194,8 @@ Task UpdateCustomerField(string customerId, /// Updates the password fields of the customer, and nothing else /// /// Customer - Task UpdateCustomerPassword(Customer customer); + /// The default body keeps implementations built for an earlier 2.4 release loading without a rebuild + Task UpdateCustomerPassword(Customer customer) => UpdateCustomer(customer); /// /// Update the customer diff --git a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs index bb2ce32673..6dba653f5b 100644 --- a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs +++ b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs @@ -115,6 +115,49 @@ public async Task ChangePassword_ForCustomer_ChangesThatCustomerWithoutLookup() _customerServiceMock.Verify(c => c.GetCustomerByEmail(It.IsAny(), It.IsAny()), Times.Never); } + [TestMethod] + public async Task ChangePassword_ImplementationBuiltForAnEarlierRelease_UsesItsExistingMethod() + { + ICustomerManagerService manager = new EarlierReleaseManager(); + + await manager.ChangePassword(new Customer { Email = "a@x.com", StoreId = "store-1" }, PasswordFormat.Hashed, "new"); + + var implementation = (EarlierReleaseManager)manager; + Assert.AreEqual("a@x.com", implementation.Request.Email); + Assert.AreEqual(PasswordFormat.Hashed, implementation.Request.PasswordFormat); + Assert.AreEqual("new", implementation.Request.NewPassword); + Assert.AreEqual("store-1", implementation.StoreId); + } + + [TestMethod] + public void NewInterfaceMembers_HaveDefaultBodies_SoExistingImplementationsKeepLoading() + { + Assert.IsFalse(typeof(ICustomerService).GetMethod(nameof(ICustomerService.UpdateCustomerPassword))!.IsAbstract); + Assert.IsFalse(typeof(ICustomerManagerService).GetMethod(nameof(ICustomerManagerService.ChangePassword), + [typeof(Customer), typeof(PasswordFormat), typeof(string)])!.IsAbstract); + } + + private sealed class EarlierReleaseManager : ICustomerManagerService + { + public ChangePasswordRequest Request { get; private set; } + public string StoreId { get; private set; } + + public Task LoginCustomer(string usernameOrEmail, string password, string storeId = "") => + throw new NotImplementedException(); + + public Task RegisterCustomer(RegistrationRequest request) => throw new NotImplementedException(); + + public bool PasswordMatch(PasswordFormat passwordFormat, string oldPassword, string newPassword, + string passwordSalt) => throw new NotImplementedException(); + + public Task ChangePassword(ChangePasswordRequest request, string storeId = "") + { + Request = request; + StoreId = storeId; + return Task.CompletedTask; + } + } + [TestMethod] public async Task ChangePassword_UnknownEmail_Throws() { From 5f0c9e6b57b68e67d641091682f79a9d7c7d2ef0 Mon Sep 17 00:00:00 2001 From: support Date: Sun, 1 Nov 2026 17:29:23 +0100 Subject: [PATCH 5/5] Do not report a customer's own e-mail or username as taken A lookup can return the customer being edited, for example where it ignores case and the stored value differs only in case. Saving My account then failed with an "already exists" message about the customer's own address. --- .../CustomerValidatorsStoreScopingTests.cs | 23 +++++++++++++++++-- .../Customer/CustomerInfoValidator.cs | 4 ++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs b/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs index 9370440ce5..ef019d7d77 100644 --- a/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs +++ b/src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs @@ -286,7 +286,26 @@ private static (SubAccountEditValidator validator, Mock custom Id = "c1", Email = email, FirstName = "A", LastName = "B" }; - private static CustomerInfoValidator CustomerInfo(Mock customerService, CustomerSettings settings) + [TestMethod] + public async Task CustomerInfoValidator_LookupReturningTheAccountItself_IsNotAConflict() + { + //a lookup that ignores case finds the account whose address differs only in case + var customerService = new Mock(); + customerService.Setup(c => c.GetCustomerByEmail("old@x.com", StoreId)) + .ReturnsAsync(new Customer { Id = OwnerId }); + customerService.Setup(c => c.GetCustomerByUsername("taken", StoreId)) + .ReturnsAsync(new Customer { Id = OwnerId }); + + var result = await CustomerInfo(customerService, new CustomerSettings { + AllowUsersToChangeEmail = false, UsernamesEnabled = true, AllowUsersToChangeUsernames = true + }, "Old@X.com").ValidateAsync(new CustomerInfoModel { Email = "Old@X.com", Username = "Taken" }); + + Assert.IsFalse(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.EmailAlreadyExists")); + Assert.IsFalse(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.UsernameAlreadyExists")); + } + + private static CustomerInfoValidator CustomerInfo(Mock customerService, CustomerSettings settings, + string currentEmail = "old@x.com") { var mediator = new Mock(); mediator.Setup(m => m.Send(It.IsAny(), It.IsAny())) @@ -297,7 +316,7 @@ private static CustomerInfoValidator CustomerInfo(Mock custome return new CustomerInfoValidator( new List>(), - Context("old@x.com"), customerService.Object, mediator.Object, attrParser.Object, + Context(currentEmail), customerService.Object, mediator.Object, attrParser.Object, Translation().Object, new Mock().Object, settings, PerStoreOn().Config); } } diff --git a/src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs b/src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs index 63c3ad890d..ffb205b570 100644 --- a/src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs @@ -139,7 +139,7 @@ public CustomerInfoValidator( context.AddFailure(translationService.GetResource("Account.EmailUsernameErrors.EmailTooLong")); var customer2 = await customerService.GetCustomerByEmail(email, storeId); - if (customer2 != null) + if (customer2 != null && customer2.Id != currentCustomer.Id) context.AddFailure( translationService.GetResource("Account.EmailUsernameErrors.EmailAlreadyExists")); } @@ -152,7 +152,7 @@ public CustomerInfoValidator( context.AddFailure(translationService.GetResource("Account.EmailUsernameErrors.UsernameTooLong")); var customer2 = await customerService.GetCustomerByUsername(username, storeId); - if (customer2 != null) + if (customer2 != null && customer2.Id != currentCustomer.Id) context.AddFailure( translationService.GetResource("Account.EmailUsernameErrors.UsernameAlreadyExists")); }