ARCH-001 Phase 19: MessageTemplate controller/service consolidation (Admin/Store) - #814
Merged
KrzysztofPajak merged 11 commits intoSep 5, 2026
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…opy), delete MessageTemplateStoreModel Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
… attribute regression tests Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
…rUpdate views to AdminShared, add widget zones, fix Store Edit.cshtml Copy-button visibility Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
…sageTemplateModel IsReadOnly/CanCopy properties Task 5 added IsReadOnly and CanCopy to MessageTemplateModel but did not regenerate the AdminShared SystemMappingTests Verify snapshot, so MessageTemplate_ToModel failed on a clean run. Confirmed the received vs verified diff was exactly these two new properties (both false in the fixture), nothing else, before accepting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
CopyTemplate's Store-only guard checked scope.HasAccess(messageTemplate)
("deny only if the caller already exclusively owns it") instead of
messageTemplate.LimitedToStores ("deny any store-limited template,
regardless of owner" - Store's original, pre-consolidation behavior).
The two are not equivalent: a template exclusively owned by ANOTHER
store also has HasAccess == false, so it passed the old guard.
Found live: with a synthetic Store2-exclusive test template seeded via
MongoDB.Driver, store1 successfully copied its Name/Subject/Body into
store1's own store via a crafted CopyTemplate POST - a real cross-tenant
data leak. Store1's crafted Edit/Delete attempts against the same
template were correctly denied throughout, isolating the bug to this
one guard.
Fixed by checking LimitedToStores directly. Added
CopyTemplate_Store_OtherStoreExclusiveTemplate_Denied regression test;
existing CopyTemplate_Store_OwnTemplate_Denied's now-unused HasAccess
stub removed since the guard no longer reads it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
Same wrong-predicate shape as the CopyTemplate fix, found by that fix's own scoped re-review: Edit(GET) computed model.CanCopy from scope.HasAccess(messageTemplate) instead of !messageTemplate.LimitedToStores. Not exploitable server-side (CopyTemplate's own gate was already fixed), but for a multi-store- shared or other-store-exclusive template it showed the Copy button on a screen where clicking it silently redirects to List with no error - a UX dead end. Fixed to mirror CopyTemplate's real guard exactly. Added EditGet_OtherStoreSharedTemplate_Store_IsReadOnlyTrue_CanCopyFalse regression test for the case that was wrong; the three existing CanCopy tests already had LimitedToStores values that happen to agree with the corrected predicate, so none needed changing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
…ift, restore Admin's DefaultEmailAccountId fallback) - MessageTemplateModel.CanCopy XML doc: replace the pre-4aa1d2bf4 !HasAccess description with the actual, correct predicate (!LimitedToStores), and explain why the ownership-based formula was exploitable. - Design spec (docs/superpowers/specs/...-design.md, gitignored, updated on disk): amend the CopyTemplate/CanCopy sections to describe the shipped LimitedToStores-based guards instead of the exploited HasAccess-based design, with an explanation of the false equivalence. - BaseMessageTemplateController: restore Admin's original Edit(GET) DefaultEmailAccountId fallback (dropped silently during consolidation), gated to the unscoped/Admin case only so Store's original behavior (no fallback) is unchanged. Threads EmailAccountSettings back through both thin subclasses' constructors. - Add/extend BaseMessageTemplateControllerTests: fallback for Admin, no-fallback regression guard for Store.
…ssagetemplate-consolidation # Conflicts: # src/Web/Grand.Web.AdminShared/Startup/StartupApplication.cs
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new StoreMessageTemplateDataScope XML docs contain an internal “memory” reference and misleading statements about scope behavior, and the shared Create view renders an unconditional “ - Name” suffix even when Name is empty.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Consolidates Admin and Store MessageTemplate CRUD logic into a shared base controller and shared views, introducing a host-routed data-scope to preserve tenant isolation and aligning Store UI with the AdminShared tabbed editor while adding Store widget-zone extensibility.
Changes:
- Added
BaseMessageTemplateControllerinGrand.Web.AdminSharedand reduced Admin/Store controllers to thin, host-specific list/grid subclasses. - Introduced
StoreMessageTemplateDataScope+RoutedMessageTemplateDataScopeand registered them in AdminShared DI to ensure correct per-area scoping. - Migrated Create/Edit shared views into
Grand.Web.AdminShared, removed Store-only model/profile, added widget-zone partial indirection and new tests.
File summaries
| File | Description |
|---|---|
| src/Web/Grand.Web.Store/Models/Messages/MessageTemplateStoreModel.cs | Removed Store-only model now that shared model carries Store flags. |
| src/Web/Grand.Web.Store/Mapper/MessageTemplateStoreProfile.cs | Removed unused AutoMapper profile for the deleted Store-only model. |
| src/Web/Grand.Web.Store/Controllers/MessageTemplateController.cs | Converted to thin subclass of shared base; retains Store-specific list endpoints. |
| src/Web/Grand.Web.Store/Areas/Store/Views/MessageTemplate/Partials/WidgetZone.DetailsTop.cshtml | Added Store widget-zone hook (details top). |
| src/Web/Grand.Web.Store/Areas/Store/Views/MessageTemplate/Partials/WidgetZone.DetailsTabs.cshtml | Added Store widget-zone hook (details tabs). |
| src/Web/Grand.Web.Store/Areas/Store/Views/MessageTemplate/Partials/WidgetZone.DetailsButtons.cshtml | Added Store widget-zone hook (details buttons). |
| src/Web/Grand.Web.Store/Areas/Store/Views/MessageTemplate/Partials/WidgetZone.DetailsBottom.cshtml | Added Store widget-zone hook (details bottom). |
| src/Web/Grand.Web.Store/Areas/Store/Views/MessageTemplate/Partials/CreateOrUpdate.cshtml | Removed Store-local editor partial after migrating to AdminShared views. |
| src/Web/Grand.Web.Store/Areas/Store/Views/MessageTemplate/List.cshtml | Added Store widget-zone hook for list-page buttons. |
| src/Web/Grand.Web.Store/Areas/Store/Views/_ViewImports.cshtml | Removed Store Messages model import (now using shared model). |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/MessageTemplate/Partials/CreateOrUpdate.TabInfo.cshtml | Made Stores editor Admin-only via area routing. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/MessageTemplate/Partials/CreateOrUpdate.cshtml | Switched widget zones to partial indirection for host-specific widget components. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/MessageTemplate/Edit.cshtml | Updated shared Edit view to use shared model + IsReadOnly/CanCopy gating + widget zones. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/MessageTemplate/Create.cshtml | Updated shared Create view to use route area and widget-zone partial. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/_ViewImports.cshtml | Added Messages namespace for shared views. |
| src/Web/Grand.Web.AdminShared/Startup/StartupApplication.cs | Registered routed + store/global message-template scopes in AdminShared DI. |
| src/Web/Grand.Web.AdminShared/Services/StoreMessageTemplateDataScope.cs | Added Store-specific data scope implementing strict ownership + looser view rules. |
| src/Web/Grand.Web.AdminShared/Services/RoutedMessageTemplateDataScope.cs | Added per-area resolver for IAdminDataScope<MessageTemplate> to avoid DI “last registration wins”. |
| src/Web/Grand.Web.AdminShared/Models/Messages/MessageTemplateModel.cs | Added IsReadOnly and CanCopy flags to support correct Store UI gating. |
| src/Web/Grand.Web.AdminShared/Controllers/BaseMessageTemplateController.cs | New shared implementation of Create/Edit/Delete/CopyTemplate with scope-based authorization. |
| src/Web/Grand.Web.Admin/Controllers/MessageTemplateController.cs | Converted to thin subclass of shared base; keeps Admin list and store-filtered grid. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/MessageTemplate/Partials/WidgetZone.DetailsTop.cshtml | Added Admin widget-zone partial used by shared views. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/MessageTemplate/Partials/WidgetZone.DetailsTabs.cshtml | Added Admin widget-zone partial used by shared views. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/MessageTemplate/Partials/WidgetZone.DetailsButtons.cshtml | Added Admin widget-zone partial used by shared views. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/MessageTemplate/Partials/WidgetZone.DetailsBottom.cshtml | Added Admin widget-zone partial used by shared views. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/MessageTemplate/Edit.cshtml | Removed Admin-local Edit view after migrating to shared view. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/MessageTemplate/Create.cshtml | Removed Admin-local Create view after migrating to shared view. |
| src/Tests/Grand.Web.Store.Tests/Controllers/MessageTemplateControllerAttributeTests.cs | Added Store controller attribute/inheritance regression tests. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/StoreMessageTemplateDataScopeTests.cs | Added unit tests for Store scope ownership/view rules. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/RoutedMessageTemplateDataScopeTests.cs | Added unit tests for per-area scope resolution and fail-closed behavior. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/MessageTemplateControllerAttributeTests.cs | Added Admin controller attribute/inheritance regression tests. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/BaseMessageTemplateControllerTests.cs | Added unit tests for shared controller access rules and CopyTemplate security regression. |
| src/Tests/Grand.Mapping.Tests/AdminShared/SystemMappingTests.MessageTemplate_ToModel.verified.txt | Updated mapping verification for new model properties. |
Review details
Suppressed comments (1)
src/Web/Grand.Web.AdminShared/Views/AdminShared/MessageTemplate/Create.cshtml:14
- The Create page caption always appends " - @Model.Name", but Name is null/blank on Create GET, so the UI renders a trailing dash. Consider only appending the name when it’s non-empty.
- Files reviewed: 33/33 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| var result = await CreateController().Edit("missing") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
| var result = await CreateController().Edit("mt-1") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
| var model = result?.Model as MessageTemplateModel; | ||
|
|
||
| Assert.IsNotNull(model); | ||
| Assert.IsTrue(model.IsReadOnly); |
| var model = result?.Model as MessageTemplateModel; | ||
|
|
||
| Assert.IsNotNull(model); | ||
| Assert.IsFalse(model.IsReadOnly); |
| var model = result?.Model as MessageTemplateModel; | ||
|
|
||
| Assert.IsNotNull(model); | ||
| Assert.IsTrue(model.IsReadOnly); |
| var model = result?.Model as MessageTemplateModel; | ||
|
|
||
| Assert.IsNotNull(model); | ||
| Assert.IsFalse(model.IsReadOnly); |
| var model = result?.Model as MessageTemplateModel; | ||
|
|
||
| Assert.IsNotNull(model); | ||
| Assert.AreEqual(1, model.Locales.Count); |
| var model = result?.Model as MessageTemplateModel; | ||
|
|
||
| Assert.IsNotNull(model); | ||
| Assert.AreEqual(1, model.Locales.Count); |
| var result = await CreateController().Edit(new MessageTemplateModel { Id = "mt-1" }, false) as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
| var result = await CreateController().Delete("mt-1") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
KrzysztofPajak
deleted the
arch001/phase19-messagetemplate-consolidation
branch
September 5, 2026 06:36
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.
Type: feature
Issue
ARCH-001 (Architecture/Maintainability):
Grand.Web.AdminandGrand.Web.Storeeach carried theirown
MessageTemplateController(298 / 311 lines) instead of sharing one, the same duplicationpattern already fixed for ~18 other entities in this initiative (Product, Category, Collection,
Order, Shipment, PaymentTransaction, MerchandiseReturn, Reports, VendorReview, the attribute family,
Discount, Brand, Blog, Page, News, GiftVoucher, ProductReview). No Vendor screen exists for this
entity.
MessageTemplate : BaseEntity, ITranslationEntity, IStoreLinkEntity. Ownership uses thealready-established
AccessToEntityByStoreexact-single-store idiom shared with the attributefamily (CheckoutAttribute/ContactAttribute/CustomerAttribute/AddressAttribute), with a looser
Stores.Contains-basedCanViewfor shared/global templates — same loose/strict split asGiftVoucher's
StoreGiftVoucherDataScope.Solution
StoreMessageTemplateDataScope/RoutedMessageTemplateDataScope(bespoke, not the genericStoreAdminDataScope<TEntity>, for theAccessToEntityByStorereason above).BaseMessageTemplateControllerinGrand.Web.AdminSharedunifyingCreate/Edit/Delete/CopyTemplate.Listand its grid-data actions (List(POST)on Admin,ListGlobal/ListStoreon Store) stay host-specific on the concrete subclasses — Admin's single filterable grid and
Store's two-tab split are different UI decisions, not a scope difference (same reasoning as
Page's kept
List.cshtml).MessageTemplateModel(already shared) gainedIsReadOnly/CanCopy; the Store-onlyMessageTemplateStoreModeland its deadMessageTemplateStoreProfileAutoMapper profile(discovered mid-implementation, not in the original plan — confirmed genuinely unused, deleted)
were removed.
Create.cshtml/Edit.cshtml/Partials/CreateOrUpdate*.cshtmlmigrated intoGrand.Web.AdminShared,unified onto Admin's tabstrip shape (Store's original was a flatter, non-tabbed layout). 5 widget
zones added via the established
<partial name="Partials/WidgetZone.*">indirection — Store'soriginals had zero
<vc:>calls at all, so all 5 zones are new capability on that host, notconversions of dead code.
Headline bug fix: Store's original
Edit.cshtmlshowed the "Copy template" button exactly whenclicking it would be denied (an exclusively-owned store template) and hid it exactly when it would
succeed (a global template) — inverted. Fixed via two separate, deliberately-not-conflated model
flags:
IsReadOnlygates Save/SaveContinue/Delete,CanCopygates Copy.Critical finding, found via a live cross-tenant smoke test the controller ran directly (not
simulated) against a real dev MongoDB with real browser sessions as admin/store1/store2:
CopyTemplate's Store-only guard originally checkedscope.HasAccess(messageTemplate)("deny onlyif I already own it") instead of
messageTemplate.LimitedToStores("deny any store-limitedtemplate, regardless of owner" — Store's actual original, pre-consolidation behavior). The two are
not equivalent: a template exclusively owned by a different store also has
HasAccess == false,so it passed the guard. Confirmed exploitable live:
store1@store.comsuccessfully copiedstore2@store.com's exclusive template's Name/Subject/Body into store1's own store via a craftedPOST. Fixed by checking
LimitedToStoresdirectly; re-verified live after the fix (denied, zeroleak). A second, non-exploitable instance of the identical wrong-predicate shape was found by the
fix's own scoped re-review (
Edit(GET)'sCanCopycomputation) and fixed the same way.The final whole-branch review (dispatched separately, independently re-derived every access-control
gate in the file rather than trusting the prior reviews) found 2 further Important, non-security
findings, both fixed in one round: the
CanCopymodel property's doc comment and the design specitself still described the old, exploited predicate as the intended design (both updated to reflect
what's actually shipped, with a note on why the original design was wrong); and Admin's
Edit(GET)had silently lost its original fallback to the system default email account when a locale's
EmailAccountIdtranslation is empty (Task 3 unified onto Store's simpler version withoutdisclosing the Admin-side behavior change) — restored, gated so Store's own behavior (which never
had this fallback) is unaffected.
Breaking changes
None for end users. For custom Vendor./Admin. localization overrides: none — this entity has no
Vendor screen and Store's Loc keys were already
Admin.*-prefixed pre-consolidation.Testing
/Admin/MessageTemplate/List) — confirm the store-filter dropdown and grid stillwork exactly as before.
/Store/MessageTemplate/List) — confirm the Global/Store 2-tabsplit still works.
Edit/Delete should show, Copy template should NOT show.
Edit/Delete should NOT show (read-only), Copy template SHOULD show. Click it — confirm a new
store-exclusive copy is created for your store and the original global template is untouched.
URL — confirm you're redirected back to the list with no data shown.
account dropdown pre-selects the system default when no per-language override exists (this was
the restored fallback — previously it showed blank on this branch before the final-review fix).
🤖 Generated with Claude Code