Dedup Store ProductController access checks onto CanAccessProduct, with full test coverage - #786
Merged
Merged
Conversation
…oduct helper
Grand.Web.Store/ProductController.cs inlined
`product == null || !product.AccessToEntityByStore(StaffStoreId)` (or the
bare `!product.AccessToEntityByStore(...)` form once product was already
null-checked) at 88 call sites, unlike Vendor's ProductController which
already funnels the equivalent check through one CheckAccessToProduct
helper. Adds the same pattern here as CanAccessProduct(product), a
null-safe bool, and replaces all 88 sites with it. Callers keep deciding
independently how to respond to a denial - RedirectToAction (3 different
targets depending on action), ErrorForKendoGridJson, Content, Json(new
{errors=...}), Json(new DataSourceResult{Errors=...}), or `continue` in a
loop - only the "is this even allowed" condition is centralized.
Note the null-check duplication itself was already redundant before this
change: AccessToEntityByStore null-checks its receiver internally and
returns false for null, so `product == null || !x.AccessToEntityByStore`
and bare `!x.AccessToEntityByStore` were already behaviorally identical
for a null product. CanAccessProduct(product) := product != null &&
product.AccessToEntityByStore(...) is provably equivalent to both original
shapes for every value of product, not just tested ones - this refactor
is a mechanical, scripted substitution across all 88 sites, not a
hand-edit.
Safety net for this one: Grand.Web.Store.Tests directly exercises 6 of the
88 sites (Delete, Edit GET/POST, GoToSku) end to end; the remaining 82 are
covered by the equivalence proof above plus a clean build and a full diff
review confirming only the condition changed at each site, never the
response building around it. That is a lighter net than per-site tests -
flagging it rather than overstating coverage.
Grand.Web.Store.Tests: 26/26 green, unmodified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t sites
Covers 87 of the 88 CanAccessProduct call sites introduced in the
previous commit (all except ProductPictureAdd, which needs a non-empty
IFormFileCollection and a separate Pictures-permission check to reach -
disproportionate setup for the same one-line condition tested everywhere
else here).
Each test arranges a "foreign" product (belongs to a single other store)
and asserts the exact denial response the action already returns -
ErrorForKendoGridJson, Content, an anonymous Json{errors}, a direct
Json(DataSourceResult{Errors=...}), RedirectToAction("List","Product"),
or a thrown ArgumentException, depending on the action. Two tests cover
behavior beyond a single deny/allow branch:
- LoadProductFriendlyNames: denied products are skipped, not erroring
the whole request (and documents a pre-existing, unrelated separator
quirk found while writing the test - not fixed here).
- AssociatedProductAddPopup: a positive-form CanAccessProduct(selected)
check that filters SelectedProductIds down to the accessible ones
rather than denying the whole request.
- BulkEditUpdate/BulkEditDelete: both funnel through the same
FilterValidProductsForStore helper, verified via one test each.
Also adds an EditGet test for the strict single-foreign-store branch,
distinct from the existing permissive multi-store test.
Grand.Web.Store.Tests: 112/112 green (26 existing + 86 new).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
KrzysztofPajak
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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
Continuation of #785: consolidates
Grand.Web.Store/ProductController.cs's tenant-isolation checks the same way Vendor's already were - a singleCanAccessProduct(product)helper instead of the check inlined at every call site - and now backs all but one of those 88 sites with a characterization test.What changed
CanAccessProduct(Product product):product != null && product.AccessToEntityByStore(StaffStoreId).product == null || !product.AccessToEntityByStore(StaffStoreId)/!product.AccessToEntityByStore(StaffStoreId)with!CanAccessProduct(product)(and the one positive-form occurrence withCanAccessProduct(selected)).RedirectToActionto 3 different targets depending on the action,ErrorForKendoGridJson,Content,Json(new {errors=...}),Json(new DataSourceResult{Errors=...}), a thrownArgumentException, orcontinuein a loop).Grand.Web.Store.Tests: 112/112, up from 26. 86 new tests cover 87 of the 88 sites end to end (one test per action, asserting the exact denial response it already returns). The one gap isProductPictureAdd, which needs a non-emptyIFormFileCollectionplus a separate Pictures-permission check to reach itsCanAccessProductcall - disproportionate setup for the same one-line condition covered everywhere else.Why the mechanical part is safe regardless of test coverage
The two inline shapes being replaced are provably equivalent to the new helper for every value of
product, not just tested ones:AccessToEntityByStorealready null-checks its receiver and returnsfalsefornull, soproduct == null || !x.AccessToEntityByStore(...)and the bare!x.AccessToEntityByStore(...)were already behaviorally identical whenproductisnull- the explicit null-check on 19 of the 88 sites was redundant even before this change.CanAccessProduct(product) := product != null && product.AccessToEntityByStore(...)collapses to the same boolean as both original shapes by construction.The substitution itself was scripted (3 fixed shapes, matched and replaced programmatically), not 88 hand edits.
Findings along the way (not fixed here, flagged in test comments)
LoadProductFriendlyNamesappends a trailing", "separator based on loop position rather than whether a name was actually appended for the previous id - a pre-existing, unrelated cosmetic bug, preserved as-is.Testing
dotnet build src/Web/Grand.Web.Store→ cleandotnet test src/Tests/Grand.Web.Store.Tests→ 112/112, all new tests plus the 26 that predate this PR🤖 Generated with Claude Code