Add ProductController characterization tests; dedup Vendor access checks - #785
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…ndleProductModel 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 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This was referenced Aug 16, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First step of a planned consolidation of the near-duplicate
ProductController/ProductViewModelServicecopies inGrand.Web.Admin/Grand.Web.Store/Grand.Web.Vendor(see.ai/workflows/refactor-safely.mdPhase 2/3/4).ProductController.Edit/Deleteacross all three areas, plus the vendor-specific parts of the forkedProductViewModelService.Grand.Web.Vendor.Testsis a new project - none existed before.Edit(GET)/Edit(POST)/Deletenow use the existing (previously unused by these three actions, but used by the other 27 actions in the file)CheckAccessToProducthelper instead of reimplementing the check inline. No behavior change - verified by the tests above.Grand.Web.Vendor.ProductViewModelService.InsertSimilarProductModel/InsertBundleProductModelcheckedHasAccessToProductagainst the product already being edited (always the vendor's own) instead of the candidate being linked in, so the check was a no-op - any vendor could attach another vendor's product to their own listing as "similar" or "bundle". Fixed to check the candidate, matching the siblingInsertRelatedProductModelwhich already had this right.Left alone (documented, not addressed here)
AccessToEntityByStoreinlined ~30 times inGrand.Web.Store/ProductController.cs) - no existing helper to consolidate onto, and only 2 of those sites are currently test-covered. Needs broader test coverage first.Testing
dotnet test src/Tests/Grand.Web.Vendor.Tests-> 12/12dotnet test src/Tests/Grand.Web.Store.Tests-> 26/26dotnet test src/Tests/Grand.Web.Admin.Tests-> 61/61🤖 Generated with Claude Code