Stop bots from creating a guest customer on every request

Directory.Packages.props

@@ -57,7 +57,6 @@
     <PackageVersion Include="Flurl.Http" Version="4.0.2" />
     <PackageVersion Include="Microsoft.AspNetCore.DataProtection.StackExchangeRedis" Version="10.0.12" />
     <PackageVersion Include="NetEscapades.AspNetCore.SecurityHeaders" Version="1.3.1" />
-    <PackageVersion Include="Wangkanai.Detection" Version="8.20.0" />
     <PackageVersion Include="Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation" Version="10.0.12" />
     <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="18.10.1" />
     <PackageVersion Include="Moq" Version="4.21.0" />

src/Business/Grand.Business.Customers/Services/CustomerService.cs

@@ -327,7 +327,8 @@ public virtual async Task<Customer> InsertGuestCustomer(Customer customer)
             { SystemName = SystemCustomerGroupNames.Guests });
         if (guestGroup == null)
             throw new GrandException("'Guests' group could not be loaded");
-        customer.Groups.Add(guestGroup.Id);
+        if (!customer.Groups.Contains(guestGroup.Id))
+            customer.Groups.Add(guestGroup.Id);
 
         await _customerRepository.InsertAsync(customer);
 

src/Modules/Grand.Module.Installer/Services/InstallationService.cs

@@ -482,7 +482,7 @@ await dbContext.CreateIndex(_customerRepository,
         await dbContext.CreateIndex(_customerRepository,
             OrderBuilder<Customer>.Create().Ascending(x => x.LastActivityDateUtc), "LastActivityDateUtc_1");
         await dbContext.CreateIndex(_customerRepository, OrderBuilder<Customer>.Create().Ascending(x => x.CustomerGuid),
-            "CustomerGuid_1");
+            "CustomerGuid_1", true);
         await dbContext.CreateIndex(_customerRepository, OrderBuilder<Customer>.Create().Ascending(x => x.Email),
             "Email_1");
         //compound lookup indexes supporting per-store customer identity (uniqueness of Email/Username + StoreId

src/Modules/Grand.Module.Migration/Migrations/2.5/MigrationCustomerGuidUniqueIndex.cs

@@ -0,0 +1,91 @@
+using Grand.Data;
+using Grand.Data.Mongo;
+using Grand.Domain.Customers;
+using Grand.Infrastructure.Migrations;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using MongoDB.Driver;
+
+namespace Grand.Module.Migration.Migrations._2._5;
+
+public class MigrationCustomerGuidUniqueIndex : IMigration
+{
+    private const string IndexName = "CustomerGuid_1";
+
+    //LiteDB names an index after its field and ignores the given name
+    private const string LiteDbIndexName = "CustomerGuid";
+
+    public int Priority => 0;
+    public DbVersion Version => new(2, 5);
+    public Guid Identity => new("f0c98624-441d-42b6-95ae-429bfb64d266");
+    public string Name => "Unique index on Customer.CustomerGuid";
+
+    public bool UpgradeProcess(IServiceProvider serviceProvider)
+    {
+        var repository = serviceProvider.GetRequiredService<IRepository<Customer>>();
+        var databaseContext = serviceProvider.GetRequiredService<IDatabaseContext>();
+        var logService = serviceProvider.GetRequiredService<ILogger<MigrationCustomerGuidUniqueIndex>>();
+
+        try
+        {
+            MakeUnique(repository, databaseContext, logService);
+        }
+        catch (Exception ex)
+        {
+            logService.LogError(ex, "UpgradeProcess - CustomerGuidUniqueIndex (2.5)");
+        }
+
+        return true;
+    }
+
+    /// <summary>
+    ///     Recreates the CustomerGuid index as unique. Returns false when it stays non-unique
+    /// </summary>
+    public static bool MakeUnique(IRepository<Customer> repository, IDatabaseContext databaseContext, ILogger logService)
+    {
+        var duplicates = repository.Table
+            .GroupBy(x => x.CustomerGuid)
+            .Where(x => x.Count() > 1)
+            .Select(x => x.Key)
+            .Take(20)
+            .ToList();
+
+        //duplicates must not stop the upgrade - keep the non-unique index for the lookups and report them
+        if (duplicates.Count != 0)
+        {
+            logService.LogError(
+                "The index {IndexName} stays non-unique - these customer GUIDs are used more than once: {Guids}. Remove the duplicates and recreate the index as unique",
+                IndexName, string.Join(", ", duplicates));
+            return false;
+        }
+
+        var mongoRepository = repository as MongoRepository<Customer>;
+        var keys = OrderBuilder<Customer>.Create().Ascending(x => x.CustomerGuid);
+
+        databaseContext.DeleteIndex(repository, mongoRepository != null ? IndexName : LiteDbIndexName)
+            .GetAwaiter().GetResult();
+        databaseContext.CreateIndex(repository, keys, IndexName, true).GetAwaiter().GetResult();
+
+        //when the unique index exists this is a rejected, swallowed no-op; when the unique create failed
+        //it restores the lookup index
+        databaseContext.CreateIndex(repository, keys, IndexName).GetAwaiter().GetResult();
+
+        //CreateIndex swallows its errors: a duplicate saved by a running instance after the check above
+        //would leave the index non-unique without a trace
+        if (mongoRepository != null && !IsUnique(mongoRepository))
+        {
+            logService.LogError(
+                "The index {IndexName} could not be created unique and stays non-unique. Remove duplicate customer GUIDs and recreate the index as unique",
+                IndexName);
+            return false;
+        }
+
+        return true;
+    }
+
+    private static bool IsUnique(MongoRepository<Customer> repository)
+    {
+        return repository.Collection.Indexes.List().ToList()
+            .Any(x => x["name"] == IndexName && x.Contains("unique") && x["unique"].ToBoolean());
+    }
+}

src/Tests/Grand.Business.Customers.Tests/Services/CustomerServiceTests.cs

@@ -333,6 +333,19 @@ public async Task InsertGuestCustomerTest()
         Assert.IsTrue(_repository.Table.Any(x => x.StoreId == "1"));
     }
 
+    [TestMethod]
+    public async Task InsertGuestCustomer_GroupAlreadyPresent_AddedOnce()
+    {
+        _mediatorMock.Setup(x => x.Send(It.IsAny<GetGroupBySystemNameQuery>(), default))
+            .Returns(Task.FromResult(new CustomerGroup { Id = "guests" }));
+        var customer = new Customer { CustomerGuid = Guid.NewGuid(), Active = true };
+        customer.Groups.Add("guests");
+
+        await _customerService.InsertGuestCustomer(customer);
+
+        CollectionAssert.AreEqual(new[] { "guests" }, customer.Groups.ToArray());
+    }
+
     [TestMethod]
     public async Task InsertCustomerTest()
     {

src/Tests/Grand.Modules.Tests/Services/Migrations/MigrationCustomerGuidUniqueIndexTests.cs

@@ -0,0 +1,55 @@
+using Grand.Data.LiteDb;
+using Grand.Data.Tests;
+using Grand.Domain.Customers;
+using Grand.Module.Migration.Migrations._2._5;
+using Grand.SharedKernel;
+using LiteDB;
+using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Grand.Modules.Tests.Services.Migrations;
+
+[TestClass]
+public class MigrationCustomerGuidUniqueIndexTests
+{
+    private LiteDatabase _database;
+    private LiteDBRepository<Customer> _repository;
+    private LiteDBContext _context;
+
+    [TestInitialize]
+    public void Setup()
+    {
+        _database = new LiteDatabase(new MemoryStream());
+        _repository = new LiteDBRepository<Customer>(_database, new AuditInfoProvider());
+        _context = new LiteDBContext(_database);
+        //an upgraded install: the lookup index exists, not unique
+        _database.GetCollection<Customer>(nameof(Customer)).EnsureIndex(x => x.CustomerGuid);
+    }
+
+    [TestCleanup]
+    public void Cleanup() => _database.Dispose();
+
+    [TestMethod]
+    public async Task LiteDb_RecreatesTheIndexUnique()
+    {
+        var guid = Guid.NewGuid();
+        await _repository.InsertAsync(new Customer { CustomerGuid = guid });
+
+        Assert.IsTrue(MigrationCustomerGuidUniqueIndex.MakeUnique(_repository, _context, NullLogger.Instance));
+
+        await Assert.ThrowsExactlyAsync<DuplicateKeyGrandException>(() => _repository.InsertAsync(new Customer { CustomerGuid = guid }));
+    }
+
+    [TestMethod]
+    public async Task LiteDb_Duplicates_KeepTheIndexNonUnique()
+    {
+        var guid = Guid.NewGuid();
+        await _repository.InsertAsync(new Customer { CustomerGuid = guid });
+        await _repository.InsertAsync(new Customer { CustomerGuid = guid });
+
+        Assert.IsFalse(MigrationCustomerGuidUniqueIndex.MakeUnique(_repository, _context, NullLogger.Instance));
+
+        await _repository.InsertAsync(new Customer { CustomerGuid = guid });
+        Assert.AreEqual(3, _repository.Table.Count(x => x.CustomerGuid == guid));
+    }
+}

src/Tests/Grand.Web.Common.Tests/Filters/CustomerActivityAttributeTests.cs

@@ -0,0 +1,89 @@
+using Grand.Business.Core.Interfaces.Customers;
+using Grand.Data;
+using Grand.Domain.Customers;
+using Grand.Infrastructure;
+using Grand.SharedKernel.Extensions;
+using Grand.Web.Common.Filters;
+using Grand.Web.Common.RequestCustomerResolvers;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Mvc.Abstractions;
+using Microsoft.AspNetCore.Mvc.Filters;
+using Microsoft.AspNetCore.Routing;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
+using System.Linq.Expressions;
+using System.Net;
+
+namespace Grand.Web.Common.Tests.Filters;
+
+[TestClass]
+public class CustomerActivityAttributeTests
+{
+    private Mock<ICustomerService> _customerService;
+    private Mock<IContextAccessor> _contextAccessor;
+    private Mock<IWorkContext> _workContext;
+    private Mock<IGuestCustomerPersistence> _persistence;
+
+    [TestInitialize]
+    public void Setup()
+    {
+        //the test App_Data/Settings.cfg has a connection string, so the filter sees an installed database
+        DataSettingsManager.Initialize(Path.Combine("", CommonPath.AppData, CommonPath.SettingsFile));
+        _customerService = new Mock<ICustomerService>();
+        _workContext = new Mock<IWorkContext>();
+        _persistence = new Mock<IGuestCustomerPersistence>();
+        _contextAccessor = new Mock<IContextAccessor>();
+        _contextAccessor.SetupGet(c => c.WorkContext).Returns(_workContext.Object);
+    }
+
+    private async Task RunGet(Customer current)
+    {
+        _workContext.SetupGet(w => w.CurrentCustomer).Returns(current);
+        var httpContext = new DefaultHttpContext();
+        httpContext.Request.Method = HttpMethods.Get;
+        httpContext.Request.Scheme = "https";
+        httpContext.Request.Host = new HostString("shop.test");
+        httpContext.Request.Path = "/product";
+        httpContext.Connection.RemoteIpAddress = IPAddress.Parse("10.0.0.1");
+        var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor());
+        var executing = new ActionExecutingContext(actionContext, new List<IFilterMetadata>(),
+            new Dictionary<string, object>(), null);
+        var filter = new CustomerActivityAttribute.CustomerActivityFilter(_customerService.Object,
+            _contextAccessor.Object, new CustomerSettings { StoreLastVisitedPage = true }, _persistence.Object);
+
+        await filter.OnActionExecutionAsync(executing,
+            () => Task.FromResult(new ActionExecutedContext(actionContext, new List<IFilterMetadata>(), null)));
+    }
+
+    [TestMethod]
+    public async Task SystemAccount_NothingWritten()
+    {
+        await RunGet(new Customer { Id = "searchengine", IsSystemAccount = true });
+
+        _customerService.Verify(c => c.UpdateCustomerField(It.IsAny<Customer>(),
+            It.IsAny<Expression<Func<Customer, DateTime>>>(), It.IsAny<DateTime>()), Times.Never);
+        _customerService.Verify(c => c.UpdateCustomerField(It.IsAny<Customer>(),
+            It.IsAny<Expression<Func<Customer, string>>>(), It.IsAny<string>()), Times.Never);
+    }
+
+    [TestMethod]
+    public async Task Guest_ActivityWritten()
+    {
+        await RunGet(new Customer { Id = "guest", Active = true });
+
+        _customerService.Verify(c => c.UpdateCustomerField(It.IsAny<Customer>(),
+            It.IsAny<Expression<Func<Customer, string>>>(), It.IsAny<string>()), Times.AtLeastOnce);
+    }
+
+    [TestMethod]
+    public async Task TransientGuest_NothingWritten()
+    {
+        _persistence.Setup(p => p.IsTransient("unsaved")).Returns(true);
+
+        await RunGet(new Customer { Id = "unsaved", Active = true });
+
+        _customerService.Verify(c => c.UpdateCustomerField(It.IsAny<Customer>(),
+            It.IsAny<Expression<Func<Customer, string>>>(), It.IsAny<string>()), Times.Never);
+    }
+}

src/Tests/Grand.Web.Common.Tests/Filters/PersistGuestCustomerAttributeTests.cs

@@ -0,0 +1,102 @@
+using Grand.Business.Core.Interfaces.Customers;
+using Grand.Data;
+using Grand.Domain.Affiliates;
+using Grand.Domain.Customers;
+using Grand.Infrastructure;
+using Grand.SharedKernel.Extensions;
+using Grand.Web.Common.Filters;
+using Grand.Web.Common.RequestCustomerResolvers;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Mvc.Abstractions;
+using Microsoft.AspNetCore.Mvc.Filters;
+using Microsoft.AspNetCore.Routing;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
+using System.Linq.Expressions;
+
+namespace Grand.Web.Common.Tests.Filters;
+
+[TestClass]
+public class PersistGuestCustomerAttributeTests
+{
+    private readonly List<string> _order = [];
+    private Mock<IGuestCustomerPersistence> _persistence;
+
+    [TestInitialize]
+    public void Setup()
+    {
+        //the test App_Data/Settings.cfg has a connection string, so the filters see an installed database
+        DataSettingsManager.Initialize(Path.Combine("", CommonPath.AppData, CommonPath.SettingsFile));
+        _persistence = new Mock<IGuestCustomerPersistence>();
+        _persistence.Setup(p => p.EnsurePersisted()).Callback(() => _order.Add("persist"))
+            .Returns(Task.CompletedTask);
+    }
+
+    private static (ActionExecutingContext, ActionExecutionDelegate) Context(string query, Action onAction)
+    {
+        var httpContext = new DefaultHttpContext();
+        httpContext.Request.Method = HttpMethods.Get;
+        httpContext.Request.QueryString = new QueryString(query);
+        var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor());
+        var executing = new ActionExecutingContext(actionContext, new List<IFilterMetadata>(),
+            new Dictionary<string, object>(), null);
+        return (executing, () =>
+        {
+            onAction();
+            return Task.FromResult(new ActionExecutedContext(actionContext, new List<IFilterMetadata>(), null));
+        });
+    }
+
+    [TestMethod]
+    public async Task PersistsTheGuestBeforeTheAction()
+    {
+        var (executing, next) = Context("", () => _order.Add("action"));
+        var filter = new PersistGuestCustomerAttribute.PersistGuestCustomerFilter(_persistence.Object);
+
+        await filter.OnActionExecutionAsync(executing, next);
+
+        CollectionAssert.AreEqual(new[] { "persist", "action" }, _order);
+    }
+
+    [TestMethod]
+    public async Task Affiliate_LandingWithAffiliateId_PersistsThenWrites()
+    {
+        var customer = new Customer { Id = "g1" };
+        var (filter, customerService) = AffiliateFilter(customer,
+            new Affiliate { Id = "aff", Active = true });
+        customerService.Setup(c => c.UpdateCustomerField("g1", It.IsAny<Expression<Func<Customer, string>>>(), "aff"))
+            .Callback(() => _order.Add("write")).Returns(Task.CompletedTask);
+        var (executing, next) = Context("?affiliateid=aff", () => { });
+
+        await filter.OnActionExecutionAsync(executing, next);
+
+        CollectionAssert.AreEqual(new[] { "persist", "write" }, _order);
+    }
+
+    [TestMethod]
+    public async Task Affiliate_InactiveAffiliate_NothingPersisted()
+    {
+        var (filter, _) = AffiliateFilter(new Customer { Id = "g1" }, new Affiliate { Id = "aff", Active = false });
+        var (executing, next) = Context("?affiliateid=aff", () => { });
+
+        await filter.OnActionExecutionAsync(executing, next);
+
+        _persistence.Verify(p => p.EnsurePersisted(), Times.Never);
+    }
+
+    private (AffiliateAttribute.AffiliateFilter, Mock<ICustomerService>) AffiliateFilter(Customer current,
+        Affiliate affiliate)
+    {
+        var workContext = new Mock<IWorkContext>();
+        workContext.SetupGet(w => w.CurrentCustomer).Returns(current);
+        var contextAccessor = new Mock<IContextAccessor>();
+        contextAccessor.SetupGet(c => c.WorkContext).Returns(workContext.Object);
+        var affiliateService = new Mock<IAffiliateService>();
+        affiliateService.Setup(a => a.GetAffiliateById(affiliate.Id)).ReturnsAsync(affiliate);
+        var customerService = new Mock<ICustomerService>();
+
+        return (new AffiliateAttribute.AffiliateFilter(affiliateService.Object, customerService.Object,
+            contextAccessor.Object, _persistence.Object), customerService);
+    }
+}