diff --git a/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs b/src/Business/Grand.Business.Core/Interfaces/Customers/ICustomerService.cs index 7f64d4c29..2106a6890 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 0d4cbd104..086ae33aa 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 c59cabdc8..6502b610c 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 da23c7bd0..6fe3747eb 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/Modules/Grand.Module.Installer/Services/InstallationService.cs b/src/Modules/Grand.Module.Installer/Services/InstallationService.cs index a39d505e4..3a45b8063 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 000000000..e78f7f75a --- /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; + } +} 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 dd8b1af5b..cfdfd07cf 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 95a0335b2..0c4c60305 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 17ce621e1..9cf0c5282 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,210 @@ 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] + 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_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() + { + 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 @@ -441,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/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs new file mode 100644 index 000000000..3c8aa3911 --- /dev/null +++ b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorOwnerTests.cs @@ -0,0 +1,254 @@ +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.Interfaces; +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; + 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())) + .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, _scope.Object); + } + + 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 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() + { + _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.Admin.Tests/Validators/CustomerValidatorTests.cs b/src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorTests.cs index c3666870f..818d1bdf0 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.Store.Tests/Controllers/CustomerControllerTests.cs b/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs index 3b57d7369..aff78fec1 100644 --- a/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs +++ b/src/Tests/Grand.Web.Store.Tests/Controllers/CustomerControllerTests.cs @@ -209,6 +209,60 @@ 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_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() + { + _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/Tests/Grand.Web.Tests/Commands/Customers/SubAccountDeleteCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountDeleteCommandHandlerTests.cs new file mode 100644 index 000000000..393b0d887 --- /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 39337c109..aa09ae098 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,127 @@ 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_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_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() + { + _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/Commands/Customers/UpdateCustomerInfoCommandHandlerTests.cs b/src/Tests/Grand.Web.Tests/Commands/Customers/UpdateCustomerInfoCommandHandlerTests.cs index b5b033ed8..3970c8b70 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/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs b/src/Tests/Grand.Web.Tests/Controllers/AccountControllerSubAccountsTests.cs new file mode 100644 index 000000000..b96c8bfde --- /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 000000000..658558cd9 --- /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 2bdfc98ec..bf2ec099d 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 000000000..bae32c967 --- /dev/null +++ b/src/Tests/Grand.Web.Tests/Validators/Customer/SubAccountEditValidatorTests.cs @@ -0,0 +1,134 @@ +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 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() + { + 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.AdminShared/Models/Customers/CustomerModel.cs b/src/Web/Grand.Web.AdminShared/Models/Customers/CustomerModel.cs index 1c794ca16..dc4399121 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 fcad7fe8d..f9a4a61e3 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; @@ -117,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, @@ -146,6 +166,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,14 +353,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) { - var ownerId = string.Empty; - if (!string.IsNullOrEmpty(model.Owner)) + if (string.IsNullOrWhiteSpace(model.Owner)) + return string.Empty; + + var ownerEmail = model.Owner.Trim(); + if (!string.IsNullOrEmpty(customer?.OwnerId) && + (!_customerConfig.RegisterCustomersPerStore || customer.StoreId == model.StoreId)) + { + 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) + { + 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 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) { - var customerOwner = await _customerService.GetCustomerByEmail(model.Owner); - if (customerOwner != null) ownerId = customerOwner.Id; + 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(), @@ -470,15 +548,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/Services/StoreCustomerDataScope.cs b/src/Web/Grand.Web.AdminShared/Services/StoreCustomerDataScope.cs index bca8cd55e..f1345bf10 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); } diff --git a/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs b/src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs index e56cfb508..5b5a0435d 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 @@ -173,6 +175,61 @@ 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; + + //a panel with a default store (Store) cannot change the owner - the posted value is replaced before + //the save - and checking it would tell a store manager about customers of other stores + if (await scope.GetDefaultStoreIdAsync() is not null) + return; + + var ownerEmail = x.Owner.Trim(); + + //the owner already linked stays valid, so an unrelated edit of an existing account is never blocked - + //unless the customer moves to another store, where the link has to hold in the new store's scope + if (!string.IsNullOrEmpty(customer?.OwnerId) && + (!customerConfig.RegisterCustomersPerStore || customer.StoreId == x.StoreId)) + { + 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 +251,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 +287,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 513a1d317..b1b8aa794 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 730314c43..fcc3cbe08 100644 --- a/src/Web/Grand.Web.Store/Controllers/CustomerController.cs +++ b/src/Web/Grand.Web.Store/Controllers/CustomerController.cs @@ -50,6 +50,26 @@ public class CustomerController( protected override async Task ApplyPostConstraints(CustomerModel model) { model.StoreId = Scope.DefaultStoreId; + + //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); + 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 = ""; diff --git a/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml b/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml index fbf940308..36c651d6f 100644 Binary files a/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml and b/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml differ 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 17ce4f394..e7c3ca8f7 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 @@ -279,4 +279,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 31026f5bf..32d6ae227 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 32555ba9c..a9a756463 100644 --- a/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs +++ b/src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs @@ -26,12 +26,24 @@ 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. 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(); + 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) diff --git a/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs b/src/Web/Grand.Web/Commands/Handler/Customers/UpdateCustomerInfoCommandHandler.cs index 892b9acd5..41a2d7e6c 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) diff --git a/src/Web/Grand.Web/Controllers/AccountController.cs b/src/Web/Grand.Web/Controllers/AccountController.cs index 9061918f1..8c2755ebb 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 0be4202e6..45e11bf93 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 3e66f3652..6cce1b35d 100644 --- a/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs @@ -43,27 +43,28 @@ public SubAccountEditValidator( { var customer = await customerService.GetCustomerById(x.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; + } + switch (customer) { - 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; + 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 0a61af4bc..59bbd3f0e 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"] + }