From d3fbe1332833bb54a15d10878769eeb854852c60 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 16 Aug 2026 12:04:22 +0200 Subject: [PATCH 1/3] Add characterization tests for ProductController access checks Ahead of consolidating the duplicated ProductController/ProductViewModelService copies in Grand.Web.Admin/Store/Vendor, add a safety net covering the tenant-isolation behavior each area implements differently: - New Grand.Web.Vendor.Tests project (none existed before) with tests for ProductController.Delete/Edit access denial, and for the vendor-specific parts of ProductViewModelService (VendorId assignment, vendor-scoped search, HasAccessToProduct filtering). - Grand.Web.Store.Tests: Delete/Edit(POST) store-scoping checks, including the counter-intuitive AccessToEntityByStore semantics (a product not limited to any store is denied, not allowed), and Edit(GET)'s separate permissive "show with warning" path that must not be folded into the same check as Delete/Edit(POST). - Grand.Web.Admin.Tests: baseline confirming Admin performs no ownership check at all. Also documents, without fixing, a suspected authorization bug in Grand.Web.Vendor ProductViewModelService.InsertSimilarProductModel / InsertBundleProductModel: the HasAccessToProduct check runs against the product already being edited (always the vendor's own) instead of the candidate product being linked in, so it never actually excludes another vendor's product. Co-Authored-By: Claude Sonnet 5 --- GrandNode.sln | 15 ++ .../Controllers/ProductControllerTests.cs | 83 +++++++ .../Controllers/ProductControllerTests.cs | 204 ++++++++++++++++++ .../Controllers/ProductControllerTests.cs | 138 ++++++++++++ .../Grand.Web.Vendor.Tests.csproj | 25 +++ .../Services/ProductViewModelServiceTests.cs | 193 +++++++++++++++++ 6 files changed, 658 insertions(+) create mode 100644 src/Tests/Grand.Web.Admin.Tests/Controllers/ProductControllerTests.cs create mode 100644 src/Tests/Grand.Web.Store.Tests/Controllers/ProductControllerTests.cs create mode 100644 src/Tests/Grand.Web.Vendor.Tests/Controllers/ProductControllerTests.cs create mode 100644 src/Tests/Grand.Web.Vendor.Tests/Grand.Web.Vendor.Tests.csproj create mode 100644 src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs diff --git a/GrandNode.sln b/GrandNode.sln index 1d44f0a7b2..74c6cc227d 100644 --- a/GrandNode.sln +++ b/GrandNode.sln @@ -167,6 +167,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Grand.Module.Api.Tests", "s EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Grand.Web.Tests", "src\Tests\Grand.Web.Tests\Grand.Web.Tests.csproj", "{920E338E-0E4A-4632-8D88-C89F340EB8A2}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Grand.Web.Vendor.Tests", "src\Tests\Grand.Web.Vendor.Tests\Grand.Web.Vendor.Tests.csproj", "{FD325D98-F10F-4912-BB2B-8287D413E430}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -969,6 +971,18 @@ Global {920E338E-0E4A-4632-8D88-C89F340EB8A2}.Release|x64.Build.0 = Release|Any CPU {920E338E-0E4A-4632-8D88-C89F340EB8A2}.Release|x86.ActiveCfg = Release|Any CPU {920E338E-0E4A-4632-8D88-C89F340EB8A2}.Release|x86.Build.0 = Release|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Debug|Any CPU.Build.0 = Debug|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Debug|x64.ActiveCfg = Debug|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Debug|x64.Build.0 = Debug|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Debug|x86.ActiveCfg = Debug|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Debug|x86.Build.0 = Debug|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Release|Any CPU.ActiveCfg = Release|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Release|Any CPU.Build.0 = Release|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Release|x64.ActiveCfg = Release|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Release|x64.Build.0 = Release|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Release|x86.ActiveCfg = Release|Any CPU + {FD325D98-F10F-4912-BB2B-8287D413E430}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -1041,6 +1055,7 @@ Global {8D626EA8-CB54-BC41-363A-217881BEBA6E} = {827E0CD3-B72D-47B6-A68D-7590B98EB39B} {03997797-E7F5-0643-168D-B8EA7178C2FE} = {827E0CD3-B72D-47B6-A68D-7590B98EB39B} {FE75CD3C-8329-C22D-A70E-B797DC30326D} = {6360202A-F931-4BBD-ADBD-C9A628EE59F8} + {FD325D98-F10F-4912-BB2B-8287D413E430} = {CEA09484-30F6-4D44-02F6-822E06DBC57C} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {88B478F4-FD3B-4C24-9E84-4FAAF0254397} diff --git a/src/Tests/Grand.Web.Admin.Tests/Controllers/ProductControllerTests.cs b/src/Tests/Grand.Web.Admin.Tests/Controllers/ProductControllerTests.cs new file mode 100644 index 0000000000..89e99b7f71 --- /dev/null +++ b/src/Tests/Grand.Web.Admin.Tests/Controllers/ProductControllerTests.cs @@ -0,0 +1,83 @@ +using Grand.Business.Core.Interfaces.Catalog.Products; +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Common.Localization; +using Grand.Business.Core.Interfaces.Common.Security; +using Grand.Business.Core.Interfaces.Storage; +using Grand.Domain.Catalog; +using Grand.Web.Admin.Controllers; +using Grand.Web.AdminShared.Interfaces; +using Grand.Web.Common.Localization; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ViewFeatures; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Admin.Tests.Controllers; + +// Characterization tests locking down the baseline for the planned ProductController consolidation: +// unlike Store/Vendor, Admin performs no ownership/scope check at all - any product can be deleted by +// any admin. If a shared base class is introduced later, this must stay true for Admin. +[TestClass] +public class ProductControllerTests +{ + private ProductController _controller; + private Mock _productServiceMock; + private Mock _productViewModelServiceMock; + private Mock _translationServiceMock; + + [TestInitialize] + public void Setup() + { + _productServiceMock = new Mock(); + _productViewModelServiceMock = new Mock(); + _translationServiceMock = new Mock(); + _translationServiceMock.Setup(t => t.GetResource(It.IsAny())).Returns("resource"); + + _controller = new ProductController( + _productViewModelServiceMock.Object, + _productServiceMock.Object, + new Mock().Object, + new Mock().Object, + _translationServiceMock.Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object); + + var httpContext = new DefaultHttpContext(); + _controller.ControllerContext = new ControllerContext { HttpContext = httpContext }; + _controller.TempData = new TempDataDictionary(httpContext, new Mock().Object); + } + + [TestMethod] + public async Task Delete_ProductNotFound_RedirectsToList() + { + _productServiceMock.Setup(p => p.GetProductById("missing", true)).ReturnsAsync((Product)null); + + var result = await _controller.Delete("missing"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task Delete_AnyExistingProduct_DeletesWithoutOwnershipCheck() + { + // No IContextAccessor is even injected here - unlike Store/Vendor, Admin has no notion of + // "not your product". A product limited to a store it has no relation to must still delete. + var product = new Product { Id = "p1", LimitedToStores = true }; + product.Stores.Add("some-other-store"); + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Delete("p1"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(product), Times.Once); + } +} diff --git a/src/Tests/Grand.Web.Store.Tests/Controllers/ProductControllerTests.cs b/src/Tests/Grand.Web.Store.Tests/Controllers/ProductControllerTests.cs new file mode 100644 index 0000000000..16845a4f6f --- /dev/null +++ b/src/Tests/Grand.Web.Store.Tests/Controllers/ProductControllerTests.cs @@ -0,0 +1,204 @@ +using Grand.Business.Core.Interfaces.Catalog.Products; +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Common.Localization; +using Grand.Business.Core.Interfaces.Common.Security; +using Grand.Business.Core.Interfaces.Storage; +using Grand.Domain.Catalog; +using Grand.Domain.Customers; +using Grand.Domain.Localization; +using Grand.Infrastructure; +using Grand.Web.AdminShared.Interfaces; +using Grand.Web.AdminShared.Models.Catalog; +using Grand.Web.Common.Localization; +using Grand.Web.Store.Controllers; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ViewFeatures; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Store.Tests.Controllers; + +// Characterization tests for the store-scoping checks in ProductController, ahead of the planned +// consolidation of the near-duplicate ProductController copies in Grand.Web.Admin / Grand.Web.Store / +// Grand.Web.Vendor. Note the current redirect target on denial differs from Vendor's equivalent +// (Edit id= here vs List there) - that asymmetry must survive any refactor, or be called out as an +// intentional behavior change. +[TestClass] +public class ProductControllerTests +{ + private const string StaffStoreId = "store-1"; + private const string OtherStoreId = "store-2"; + + private ProductController _controller; + private Mock _productServiceMock; + private Mock _productViewModelServiceMock; + private Mock _translationServiceMock; + + [TestInitialize] + public void Setup() + { + _productServiceMock = new Mock(); + _productViewModelServiceMock = new Mock(); + _translationServiceMock = new Mock(); + _translationServiceMock.Setup(t => t.GetResource(It.IsAny())).Returns("resource"); + + var workContextMock = new Mock(); + workContextMock.Setup(w => w.CurrentCustomer).Returns(new Customer { StaffStoreId = StaffStoreId }); + var contextAccessorMock = new Mock(); + contextAccessorMock.Setup(c => c.WorkContext).Returns(workContextMock.Object); + + var languageServiceMock = new Mock(); + languageServiceMock.Setup(l => l.GetAllLanguages(true, It.IsAny())).ReturnsAsync(new List()); + + _controller = new ProductController( + _productViewModelServiceMock.Object, + _productServiceMock.Object, + new Mock().Object, + contextAccessorMock.Object, + languageServiceMock.Object, + _translationServiceMock.Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object); + + var httpContext = new DefaultHttpContext(); + _controller.ControllerContext = new ControllerContext { HttpContext = httpContext }; + _controller.TempData = new TempDataDictionary(httpContext, new Mock().Object); + } + + [TestMethod] + public async Task Delete_ProductNotFound_RedirectsToList() + { + _productServiceMock.Setup(p => p.GetProductById("missing", true)).ReturnsAsync((Product)null); + + var result = await _controller.Delete("missing"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task Delete_ProductOutsideStaffStore_RedirectsToEditWithoutDeleting() + { + var product = new Product { Id = "p1", LimitedToStores = true }; + product.Stores.Add(OtherStoreId); + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Delete("p1"); + + // Unlike Vendor, denial here redirects back to Edit rather than List. + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("Edit", redirect.ActionName); + Assert.AreEqual("p1", redirect.RouteValues["id"]); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(product), Times.Never); + } + + [TestMethod] + public async Task Delete_ProductInStaffStore_DeletesAndRedirectsToList() + { + var product = new Product { Id = "p1", LimitedToStores = true }; + product.Stores.Add(StaffStoreId); + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Delete("p1"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(product), Times.Once); + } + + [TestMethod] + public async Task Delete_ProductNotLimitedToAnyStore_IsDenied() + { + // Counter-intuitive but current behavior: AccessToEntityByStore only grants access when + // LimitedToStores is true AND the product belongs to exactly one store (this one). A + // "global" (LimitedToStores=false) product is therefore NOT deletable by store staff - + // see AclMappingExtension.AccessToEntityByStore. A refactor must not silently "fix" this. + var product = new Product { Id = "p1", LimitedToStores = false }; + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Delete("p1"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("Edit", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(product), Times.Never); + } + + [TestMethod] + public async Task Delete_ProductInMultipleStoresIncludingStaffStore_IsDenied() + { + // Same source: Stores.Count == 1 is required, so a product shared across stores is denied + // even to a staff member of one of those stores. + var product = new Product { Id = "p1", LimitedToStores = true }; + product.Stores.Add(StaffStoreId); + product.Stores.Add(OtherStoreId); + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Delete("p1"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("Edit", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(product), Times.Never); + } + + [TestMethod] + public async Task EditPost_ProductNotFound_RedirectsToList() + { + _productServiceMock.Setup(p => p.GetProductById("missing", true)).ReturnsAsync((Product)null); + + var result = await _controller.Edit(new ProductModel { Id = "missing" }, continueEditing: false); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify( + s => s.UpdateProductModel(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task EditPost_ProductOutsideStaffStore_RedirectsToEditWithoutUpdating() + { + // Same check, same "Edit" redirect target as Delete - but note this is a *different* check + // from Edit(GET), which additionally allows a multi-store product through with a warning. + // Do not fold this into a helper shared with Edit(GET). + var product = new Product { Id = "p1", LimitedToStores = true }; + product.Stores.Add(OtherStoreId); + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Edit(new ProductModel { Id = "p1" }, continueEditing: false); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("Edit", redirect.ActionName); + Assert.AreEqual("p1", redirect.RouteValues["id"]); + _productViewModelServiceMock.Verify( + s => s.UpdateProductModel(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task EditGet_ProductSharedAcrossMultipleStoresIncludingStaffStore_ShowsFormWithWarning() + { + // Edit(GET)'s permissive branch: a product limited to more than one store, one of which is + // this staff member's store, is NOT denied here - it is shown with a warning instead. This is + // the one path that must stay outside any shared "authorize or redirect" helper. + var product = new Product { Id = "p1", LimitedToStores = true }; + product.Stores.Add(StaffStoreId); + product.Stores.Add(OtherStoreId); + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Edit("p1"); + + Assert.IsInstanceOfType(result); + _productViewModelServiceMock.Verify( + s => s.PrepareProductModel(It.IsAny(), product, false, false), Times.Once); + } +} diff --git a/src/Tests/Grand.Web.Vendor.Tests/Controllers/ProductControllerTests.cs b/src/Tests/Grand.Web.Vendor.Tests/Controllers/ProductControllerTests.cs new file mode 100644 index 0000000000..ea727905a1 --- /dev/null +++ b/src/Tests/Grand.Web.Vendor.Tests/Controllers/ProductControllerTests.cs @@ -0,0 +1,138 @@ +using Grand.Business.Core.Interfaces.Catalog.Products; +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Common.Localization; +using Grand.Business.Core.Interfaces.Common.Security; +using Grand.Business.Core.Interfaces.Storage; +using Grand.Domain.Catalog; +using Grand.Domain.Vendors; +using Grand.Infrastructure; +using Grand.Web.Common.Localization; +using Grand.Web.Vendor.Controllers; +using Grand.Web.Vendor.Interfaces; +using Grand.Web.Vendor.Models.Catalog; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ViewFeatures; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Web.Vendor.Tests.Controllers; + +// Characterization tests for the tenant-isolation checks in ProductController, ahead of the planned +// consolidation of the near-duplicate ProductController/ProductViewModelService copies in +// Grand.Web.Admin / Grand.Web.Store / Grand.Web.Vendor. These lock down the *current* behavior +// (including the redirect target chosen on access denial) so the refactor has something to fail against. +[TestClass] +public class ProductControllerTests +{ + private const string OwnVendorId = "vendor-1"; + private const string OtherVendorId = "vendor-2"; + + private ProductController _controller; + private Mock _productServiceMock; + private Mock _productViewModelServiceMock; + private Mock _translationServiceMock; + + [TestInitialize] + public void Setup() + { + _productServiceMock = new Mock(); + _productViewModelServiceMock = new Mock(); + _translationServiceMock = new Mock(); + _translationServiceMock.Setup(t => t.GetResource(It.IsAny())).Returns("resource"); + + var workContextMock = new Mock(); + workContextMock.Setup(w => w.CurrentVendor).Returns(new Domain.Vendors.Vendor { Id = OwnVendorId }); + var contextAccessorMock = new Mock(); + contextAccessorMock.Setup(c => c.WorkContext).Returns(workContextMock.Object); + + _controller = new ProductController( + _productViewModelServiceMock.Object, + _productServiceMock.Object, + new Mock().Object, + contextAccessorMock.Object, + new Mock().Object, + _translationServiceMock.Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object); + + var httpContext = new DefaultHttpContext(); + _controller.ControllerContext = new ControllerContext { HttpContext = httpContext }; + _controller.TempData = new TempDataDictionary(httpContext, new Mock().Object); + } + + [TestMethod] + public async Task Delete_ProductNotFound_RedirectsToList() + { + _productServiceMock.Setup(p => p.GetProductById("missing", true)).ReturnsAsync((Product)null); + + var result = await _controller.Delete("missing"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task Delete_ProductOwnedByAnotherVendor_RedirectsToListWithoutDeleting() + { + var product = new Product { Id = "p1", VendorId = OtherVendorId }; + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Delete("p1"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(product), Times.Never); + } + + [TestMethod] + public async Task Delete_ProductOwnedByCurrentVendor_DeletesAndRedirectsToList() + { + var product = new Product { Id = "p1", VendorId = OwnVendorId }; + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Delete("p1"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify(s => s.DeleteProduct(product), Times.Once); + } + + [TestMethod] + public async Task EditGet_ProductOwnedByAnotherVendor_RedirectsToListWithoutPreparingModel() + { + var product = new Product { Id = "p1", VendorId = OtherVendorId }; + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Edit("p1"); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify( + s => s.PrepareProductModel(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [TestMethod] + public async Task EditPost_ProductOwnedByAnotherVendor_RedirectsToListWithoutUpdating() + { + var product = new Product { Id = "p1", VendorId = OtherVendorId }; + _productServiceMock.Setup(p => p.GetProductById("p1", true)).ReturnsAsync(product); + + var result = await _controller.Edit(new ProductModel { Id = "p1" }, continueEditing: false); + + var redirect = result as RedirectToActionResult; + Assert.IsNotNull(redirect); + Assert.AreEqual("List", redirect.ActionName); + _productViewModelServiceMock.Verify( + s => s.UpdateProductModel(It.IsAny(), It.IsAny()), Times.Never); + } +} diff --git a/src/Tests/Grand.Web.Vendor.Tests/Grand.Web.Vendor.Tests.csproj b/src/Tests/Grand.Web.Vendor.Tests/Grand.Web.Vendor.Tests.csproj new file mode 100644 index 0000000000..b50ad060d5 --- /dev/null +++ b/src/Tests/Grand.Web.Vendor.Tests/Grand.Web.Vendor.Tests.csproj @@ -0,0 +1,25 @@ + + + + + false + + + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + + diff --git a/src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs b/src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs new file mode 100644 index 0000000000..51fbd93015 --- /dev/null +++ b/src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs @@ -0,0 +1,193 @@ +using Grand.Business.Core.Interfaces.Catalog.Categories; +using Grand.Business.Core.Interfaces.Catalog.Collections; +using Grand.Business.Core.Interfaces.Catalog.Directory; +using Grand.Business.Core.Interfaces.Catalog.Prices; +using Grand.Business.Core.Interfaces.Catalog.Products; +using Grand.Business.Core.Interfaces.Catalog.Tax; +using Grand.Business.Core.Interfaces.Checkout.Shipping; +using Grand.Business.Core.Interfaces.Common.Directory; +using Grand.Business.Core.Interfaces.Common.Localization; +using Grand.Business.Core.Interfaces.Common.Seo; +using Grand.Business.Core.Interfaces.Common.Stores; +using Grand.Business.Core.Interfaces.Customers; +using Grand.Business.Core.Interfaces.Storage; +using Grand.Domain; +using Grand.Domain.Catalog; +using Grand.Domain.Directory; +using Grand.Domain.Tax; +using Grand.Domain.Vendors; +using Grand.Infrastructure; +using Grand.Infrastructure.Mapper; +using Grand.Mapping; +using Grand.Web.Common.Localization; +using Grand.Web.Vendor.Mapper; +using Grand.Web.Vendor.Models.Catalog; +using Grand.Web.Vendor.Services; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using Assert = Microsoft.VisualStudio.TestTools.UnitTesting.Assert; + +namespace Grand.Web.Vendor.Tests.Services; + +// Characterization tests for the vendor-specific behavior of this forked ProductViewModelService, ahead +// of the planned consolidation back into Grand.Web.AdminShared.Services.ProductViewModelService (the two +// classes are ~85% identical; the remainder is the tenant-isolation logic covered here). These tests must +// keep passing (or have their expectation deliberately revised in the same change) once the fork is +// removed and the AdminShared implementation is parameterized/extended for the Vendor area instead. +[TestClass] +public class ProductViewModelServiceTests +{ + private const string CurrentVendorId = "vendor-1"; + private const string OtherVendorId = "vendor-2"; + + private Mock _productServiceMock; + private Mock _seNameServiceMock; + private ProductViewModelService _service; + + [TestInitialize] + public void Setup() + { + var mapperConfig = new MapperConfiguration(cfg => { cfg.AddProfile(); }); + AutoMapperConfig.Init(mapperConfig); + + _productServiceMock = new Mock(); + _seNameServiceMock = new Mock(); + _seNameServiceMock + .Setup(s => s.TranslationSeNameProperties(It.IsAny>(), + It.IsAny(), It.IsAny>>())) + .ReturnsAsync(new List()); + _seNameServiceMock + .Setup(s => s.ValidateSeName(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync("se-name"); + + var workContextMock = new Mock(); + workContextMock.Setup(w => w.CurrentVendor).Returns(new Grand.Domain.Vendors.Vendor { Id = CurrentVendorId }); + var contextAccessorMock = new Mock(); + contextAccessorMock.Setup(c => c.WorkContext).Returns(workContextMock.Object); + + var translationServiceMock = new Mock(); + translationServiceMock.Setup(t => t.GetResource(It.IsAny())).Returns("resource"); + + _service = new ProductViewModelService( + _productServiceMock.Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + translationServiceMock.Object, + new Mock().Object, + new Mock().Object, + contextAccessorMock.Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new Mock().Object, + new CurrencySettings(), + new MeasureSettings(), + new TaxSettings(), + _seNameServiceMock.Object, + new Mock().Object); + } + + [TestMethod] + public async Task InsertProductModel_SetsVendorIdFromCurrentVendor() + { + var model = new ProductModel { Name = "New product" }; + + var product = await _service.InsertProductModel(model); + + Assert.AreEqual(CurrentVendorId, product.VendorId); + _productServiceMock.Verify(p => p.InsertProduct(It.Is(x => x.VendorId == CurrentVendorId)), + Times.Once); + } + + [TestMethod] + public async Task PrepareProducts_FiltersSearchByCurrentVendor() + { + IPagedList paged = new PagedList { new() { Id = "p1", VendorId = CurrentVendorId } }; + _productServiceMock.Setup(p => p.SearchProducts( + false, 0, int.MaxValue, It.IsAny>(), It.IsAny(), It.IsAny(), + It.IsAny(), CurrentVendorId, It.IsAny(), It.IsAny(), false, false, + It.IsAny(), It.IsAny(), null, null, "", It.IsAny(), false, true, false, "", + null, null, ProductSortingEnum.Position, true, It.IsAny())) + .ReturnsAsync((paged, (IList)null)); + + var products = await _service.PrepareProducts(new ProductListModel()); + + Assert.AreEqual(1, products.Count); + _productServiceMock.Verify(p => p.SearchProducts( + false, 0, int.MaxValue, It.IsAny>(), It.IsAny(), It.IsAny(), + It.IsAny(), CurrentVendorId, It.IsAny(), It.IsAny(), false, false, + It.IsAny(), It.IsAny(), null, null, "", It.IsAny(), false, true, false, "", + null, null, ProductSortingEnum.Position, true, It.IsAny()), Times.Once); + } + + [TestMethod] + public async Task DeleteSelected_SkipsProductsNotOwnedByCurrentVendor() + { + var own = new Product { Id = "own", VendorId = CurrentVendorId }; + var other = new Product { Id = "other", VendorId = OtherVendorId }; + _productServiceMock.Setup(p => p.GetProductsByIds(new[] { "own", "other" }, true)) + .ReturnsAsync(new List { own, other }); + + await _service.DeleteSelected(new[] { "own", "other" }); + + _productServiceMock.Verify(p => p.DeleteProduct(own), Times.Once); + _productServiceMock.Verify(p => p.DeleteProduct(other), Times.Never); + } + + [TestMethod] + public async Task InsertRelatedProductModel_SkipsCandidateNotOwnedByCurrentVendor() + { + var source = new Product { Id = "source", VendorId = CurrentVendorId }; + var other = new Product { Id = "other", VendorId = OtherVendorId }; + _productServiceMock.Setup(p => p.GetProductById("source", true)).ReturnsAsync(source); + _productServiceMock.Setup(p => p.GetProductById("other", false)).ReturnsAsync(other); + + await _service.InsertRelatedProductModel(new ProductModel.AddRelatedProductModel { + ProductId = "source", + SelectedProductIds = ["other"] + }); + + Assert.IsFalse(source.RelatedProducts.Any(x => x.ProductId2 == "other")); + _productServiceMock.Verify(p => p.InsertRelatedProduct(It.IsAny(), "source"), Times.Never); + } + + [TestMethod] + public async Task InsertSimilarProductModel_ChecksAccessOnTheSourceProductNotTheCandidate() + { + // SUSPECTED BUG (found while writing this characterization test, not fixed here): unlike + // InsertRelatedProductModel/InsertBundleProductModel's sibling checks, this one evaluates + // HasAccessToProduct(productId1) - the product already being edited, which the vendor is + // guaranteed to own - instead of HasAccessToProduct(product), the candidate being linked in + // via `id`. The access check is therefore always true and never actually excludes another + // vendor's product from being linked as "similar". This test locks down that current + // (likely wrong) behavior so it isn't silently changed by the consolidation refactor; the fix, + // if wanted, is a separate change. + var source = new Product { Id = "source", VendorId = CurrentVendorId }; + var other = new Product { Id = "other", VendorId = OtherVendorId }; + _productServiceMock.Setup(p => p.GetProductById("source", true)).ReturnsAsync(source); + _productServiceMock.Setup(p => p.GetProductById("other", false)).ReturnsAsync(other); + + await _service.InsertSimilarProductModel(new ProductModel.AddSimilarProductModel { + ProductId = "source", + SelectedProductIds = ["other"] + }); + + Assert.IsTrue(source.SimilarProducts.Any(x => x.ProductId2 == "other"), + "current code links the candidate regardless of its owner - see comment above"); + } +} From e3292f6b717db15735f4dce40aa500f1cfa3fd9f Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 16 Aug 2026 12:04:31 +0200 Subject: [PATCH 2/3] Dedup Vendor ProductController access checks onto CheckAccessToProduct Edit(GET), Edit(POST) and Delete reimplemented the tenant-isolation check inline (product == null || !HasAccessToProduct(product)) instead of using the existing CheckAccessToProduct helper already used consistently by the other 27 actions in this controller. No behavior change: both denial reasons ("product not found" / "not your product") still redirect to List, as verified by Grand.Web.Vendor.Tests (10/10 green, unmodified). Covers only the sites already backed by a characterization test. Store's equivalent duplication (AccessToEntityByStore inlined ~30 times) is left alone - it has no existing helper to consolidate onto, and only 2 of those sites are test-covered, so a partial extraction there would trade one inconsistency for another. Co-Authored-By: Claude Sonnet 5 --- .../Controllers/ProductController.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/Web/Grand.Web.Vendor/Controllers/ProductController.cs b/src/Web/Grand.Web.Vendor/Controllers/ProductController.cs index 75a5109f8b..17eb92f430 100644 --- a/src/Web/Grand.Web.Vendor/Controllers/ProductController.cs +++ b/src/Web/Grand.Web.Vendor/Controllers/ProductController.cs @@ -163,8 +163,9 @@ public async Task Create(ProductModel model, bool continueEditing public async Task Edit(string id) { var product = await _productService.GetProductById(id, true); - if (product == null || !_contextAccessor.WorkContext.HasAccessToProduct(product)) - //No product found with the specified id + var permission = await CheckAccessToProduct(product); + if (!permission.allow) + //No product found with the specified id, or it's not this vendor's product return RedirectToAction("List"); var model = product.ToModel(_dateTimeService); @@ -191,8 +192,9 @@ await AddLocales(_languageService, model.Locales, (locale, languageId) => public async Task Edit(ProductModel model, bool continueEditing) { var product = await _productService.GetProductById(model.Id, true); - if (product == null || !_contextAccessor.WorkContext.HasAccessToProduct(product)) - //No product found with the specified id + var permission = await CheckAccessToProduct(product); + if (!permission.allow) + //No product found with the specified id, or it's not this vendor's product return RedirectToAction("List"); if (model.Ticks != product.Ticks) @@ -228,8 +230,9 @@ public async Task Edit(ProductModel model, bool continueEditing) public async Task Delete(string id) { var product = await _productService.GetProductById(id, true); - if (product == null || !_contextAccessor.WorkContext.HasAccessToProduct(product)) - //No product found with the specified id + var permission = await CheckAccessToProduct(product); + if (!permission.allow) + //No product found with the specified id, or it's not this vendor's product return RedirectToAction("List"); if (ModelState.IsValid) From 28b7dfde30c2afa67d6c6c8cc8991bb8572b65d0 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 16 Aug 2026 12:07:44 +0200 Subject: [PATCH 3/3] Fix vendor authorization bypass in InsertSimilarProductModel/InsertBundleProductModel Both methods checked HasAccessToProduct(productId1) - the product already being edited, which the vendor is guaranteed to own - instead of HasAccessToProduct(product), the candidate being linked in via the loop variable `id`. The check was therefore always true and never actually excluded another vendor's product from being linked as a similar or bundle product, letting a vendor attach a competitor's product to their own listing. Now checks the candidate, matching the sibling InsertRelatedProductModel which already had this right. Grand.Web.Vendor.Tests: 12/12 green (2 new regression tests + 1 existing test flipped from documenting the bug to asserting the fix). Co-Authored-By: Claude Sonnet 5 --- .../Services/ProductViewModelServiceTests.cs | 52 +++++++++++++++---- .../Services/ProductViewModelService.cs | 4 +- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs b/src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs index 51fbd93015..a6306550db 100644 --- a/src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs +++ b/src/Tests/Grand.Web.Vendor.Tests/Services/ProductViewModelServiceTests.cs @@ -167,16 +167,14 @@ await _service.InsertRelatedProductModel(new ProductModel.AddRelatedProductModel } [TestMethod] - public async Task InsertSimilarProductModel_ChecksAccessOnTheSourceProductNotTheCandidate() + public async Task InsertSimilarProductModel_SkipsCandidateNotOwnedByCurrentVendor() { - // SUSPECTED BUG (found while writing this characterization test, not fixed here): unlike - // InsertRelatedProductModel/InsertBundleProductModel's sibling checks, this one evaluates + // Regression test for a fixed authorization bug: this used to check // HasAccessToProduct(productId1) - the product already being edited, which the vendor is // guaranteed to own - instead of HasAccessToProduct(product), the candidate being linked in - // via `id`. The access check is therefore always true and never actually excludes another - // vendor's product from being linked as "similar". This test locks down that current - // (likely wrong) behavior so it isn't silently changed by the consolidation refactor; the fix, - // if wanted, is a separate change. + // via `id`. That made the check a no-op: any vendor could link any other vendor's product as + // "similar". Now it checks the candidate, matching InsertRelatedProductModel/ + // InsertBundleProductModel. var source = new Product { Id = "source", VendorId = CurrentVendorId }; var other = new Product { Id = "other", VendorId = OtherVendorId }; _productServiceMock.Setup(p => p.GetProductById("source", true)).ReturnsAsync(source); @@ -187,7 +185,43 @@ await _service.InsertSimilarProductModel(new ProductModel.AddSimilarProductModel SelectedProductIds = ["other"] }); - Assert.IsTrue(source.SimilarProducts.Any(x => x.ProductId2 == "other"), - "current code links the candidate regardless of its owner - see comment above"); + Assert.IsFalse(source.SimilarProducts.Any(x => x.ProductId2 == "other")); + _productServiceMock.Verify(p => p.InsertSimilarProduct(It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task InsertSimilarProductModel_LinksCandidateOwnedByCurrentVendor() + { + var source = new Product { Id = "source", VendorId = CurrentVendorId }; + var own = new Product { Id = "own", VendorId = CurrentVendorId }; + _productServiceMock.Setup(p => p.GetProductById("source", true)).ReturnsAsync(source); + _productServiceMock.Setup(p => p.GetProductById("own", false)).ReturnsAsync(own); + + await _service.InsertSimilarProductModel(new ProductModel.AddSimilarProductModel { + ProductId = "source", + SelectedProductIds = ["own"] + }); + + Assert.IsTrue(source.SimilarProducts.Any(x => x.ProductId2 == "own")); + _productServiceMock.Verify(p => p.InsertSimilarProduct(It.IsAny()), Times.Once); + } + + [TestMethod] + public async Task InsertBundleProductModel_SkipsCandidateNotOwnedByCurrentVendor() + { + // Same fixed bug as InsertSimilarProductModel, same fix. + var source = new Product { Id = "source", VendorId = CurrentVendorId }; + var other = new Product { Id = "other", VendorId = OtherVendorId }; + _productServiceMock.Setup(p => p.GetProductById("source", true)).ReturnsAsync(source); + _productServiceMock.Setup(p => p.GetProductById("other", false)).ReturnsAsync(other); + + await _service.InsertBundleProductModel(new ProductModel.AddBundleProductModel { + ProductId = "source", + SelectedProductIds = ["other"] + }); + + Assert.IsFalse(source.BundleProducts.Any(x => x.ProductId == "other")); + _productServiceMock.Verify(p => p.InsertBundleProduct(It.IsAny(), It.IsAny()), + Times.Never); } } diff --git a/src/Web/Grand.Web.Vendor/Services/ProductViewModelService.cs b/src/Web/Grand.Web.Vendor/Services/ProductViewModelService.cs index a190db9a4e..34001ada81 100644 --- a/src/Web/Grand.Web.Vendor/Services/ProductViewModelService.cs +++ b/src/Web/Grand.Web.Vendor/Services/ProductViewModelService.cs @@ -943,7 +943,7 @@ public virtual async Task InsertSimilarProductModel(ProductModel.AddSimilarProdu foreach (var id in model.SelectedProductIds) { var product = await _productService.GetProductById(id); - if (product != null && _contextAccessor.WorkContext.HasAccessToProduct(productId1)) + if (product != null && _contextAccessor.WorkContext.HasAccessToProduct(product)) { var existingSimilarProducts = productId1.SimilarProducts; if (model.ProductId != id) @@ -991,7 +991,7 @@ public virtual async Task InsertBundleProductModel(ProductModel.AddBundleProduct foreach (var id in model.SelectedProductIds) { var product = await _productService.GetProductById(id); - if (product != null && _contextAccessor.WorkContext.HasAccessToProduct(productId1)) + if (product != null && _contextAccessor.WorkContext.HasAccessToProduct(product)) { var existingBundleProducts = productId1.BundleProducts; if (model.ProductId != id)