Fix and finish company sub-accounts (owner checks, Store save, admin owner rules) by KrzysztofPajak · Pull Request #991 · grandnode/grandnode2

KrzysztofPajak · 2026-10-24T10:14:13Z

Type: bugfix

Issue

Company accounts in GrandNode are the owner / sub-account pair (Customer.OwnerId): an owner creates sub-accounts for its staff on the storefront (My account → Subaccounts), sees their orders and merchandise returns, and an administrator can link an owner by e-mail on the customer form. Several defects make this unreliable for a business customer:

The "Hide Subaccounts tab" setting only hides the link. The add, edit and delete actions stay reachable by URL. A fresh install hides the tab by default, so the feature is half-off out of the box.
A sub-account can be locked out by its owner. The edit stores a changed e-mail exactly as typed, while sign-in looks e-mails up lowercased. John@Firm.com can no longer sign in. Saving a new password in the same edit fails, because the password change looks the account up by that e-mail.
Saving a sub-account in the Store panel detaches it from its owner. ApplyPostConstraints blanked Owner on every save, and the shared update path then cleared OwnerId.
The admin owner field only checks that the e-mail exists. An owner can itself be a sub-account (a chain), a customer that already has sub-accounts can be made a sub-account, an unregistered account can be an owner, and with per-store identity the lookup on save is global while the validator is store-scoped, so a different store's account can be linked.
Weaker spots: the edit command trusted the validator for ownership. The edit page rendered an empty form for someone else's id. The validator answered missing and foreign ids differently. The delete endpoint returned an empty error (it read ModelState, not the handler's result). Messages were hard-coded English, one of them a raw resource key ("Account is not registered").
The Store panel stripped more than the owner. Every customer save also blanked the sales employee, vendor and staff store, and reset the groups to Registered. A store manager correcting a name took away the customer's sales employee and price groups, and a colleague's Store manager role.
The Store panel reached beyond the store's customers. It is meant for a store's registered customers, but its scope took in every registered account of the store, staff accounts included (administrators, vendors, store managers, sales managers, accounts tied to a vendor or a staff store). These appeared in its customer list and opened in its customer form.
The same lowercase lockout on the customer's own account. In My account, a changed e-mail or username was saved as typed. A customer who typed Me@Shop.COM, or a username John, could no longer sign in.
No visibility in the back office: an owner's customer form does not show its sub-accounts, and lists of sub-accounts scan the whole customer collection (no index on OwnerId).

Solution

No new concept. The existing owner / sub-account model is fixed and finished.

Storefront (Grand.Web)

Every sub-account action goes through one check (SubAccountsDenied): an owner only, and the setting really switches the feature off.
SubAccountEditCommandHandler re-checks ownership and changes nothing for a foreign, deleted or missing account. It stores the e-mail lowercased, and without usernames the username follows the e-mail, the same as SubAccountAddCommandHandler creates it.
GetSubAccountHandler returns null instead of an empty model, and the controller redirects to the list.
SubAccountEditValidator checks ownership first and gives a missing id and someone else's id the same answer (no id enumeration, no state disclosure). It no longer throws on an empty e-mail.
The delete endpoint returns the handler's localized message.
Inactive sub-accounts carry an Inactive badge on the owner's list.
UpdateCustomerInfoCommandHandler (My account) stores a changed e-mail and username lowercased. A value differing only in case repairs a row stored as typed, and without usernames the username follows the e-mail, as registration sets it.

Admin and Store panels (Grand.Web.AdminShared, Grand.Web.Store)

CustomerValidator keeps sub-accounts one level deep. The owner must be a registered, live customer that is not itself a sub-account. A customer with sub-accounts cannot get an owner, and a customer cannot own itself. An owner that is already linked stays valid when posted back unchanged, so legacy data never blocks an unrelated save.
CustomerViewModelService.ResolveOwnerId uses the same store scope as the validator and keeps the linked owner when it is unchanged.
Store panel (ApplyPostConstraints): an existing customer keeps where it belongs as stored: owner, vendor, staff store, sales employee and groups. A new customer gets none of them and Registered only. Posted values are never used either way, so nothing can be smuggled in.
StoreCustomerDataScope: the Store panel covers the store's registered customers only. Staff accounts are left out of its customer list in the query (new optional filters on GetAllCustomers and GetCustomerQuery), so paging and totals stay right. They are also refused when opened by id, which covers every customer tab through LoadAuthorizedCustomer.
The customer form shows a read-only Subaccounts card on an owner and an Owner link on a sub-account, on both panels. The list is scoped in the query like the customer list (store of the Store panel, sales employee of a sales manager), and each row also passes the panel's own HasAccess.

Data

Index OwnerId_1 on Customer: created for fresh installs in InstallationService, and for existing installations by MigrationCustomerOwnerIndex (2.5).
8 resources in DefaultLanguage.xml and Upgrade/en_250.xml.

Patterns followed: MigrationRecentlyViewedProductCustomerIndex for the migration, the order-details static tables for the admin card, and IAdminDataScope<Customer> injected into a shared service as in MerchandiseReturnViewModelService.

Breaking changes

Behaviour: with Hide Subaccounts tab switched on (the default on new installs), the sub-account pages and actions now respond as switched off instead of staying reachable by URL. A shop that relies on them has to switch the tab on in Settings → Customer.
Admin owner field: links that the new rules refuse can no longer be created. Existing links are kept, and an unchanged owner never blocks a save.
Store panel scope: staff accounts (administrators, vendors, store and sales managers, accounts tied to a vendor or a staff store) no longer appear in, or open from, the Store panel's customer screens. They are managed in the Admin panel.
ICustomerService.GetAllCustomers gains two optional parameters at the end. Source-compatible, but a plugin compiled against the old signature that calls it must be rebuilt.
Public constructor: CustomerViewModelService takes two more parameters (CustomerConfig, IAdminDataScope<Customer>). DI resolves them. Only code that constructs or subclasses it by hand needs updating.
CustomerModel gains display-only properties (OwnerCustomerId, OwnerCustomerEmail, SubAccounts). This is additive, nothing is removed or renamed.

Testing

Unit tests (all green on this branch):

Project	Result	New or changed for this PR
Grand.Web.Tests	259 / 259	`AccountControllerSubAccountsTests` (10), `SubAccountEditCommandHandlerTests` (12), `SubAccountDeleteCommandHandlerTests` (4), `GetSubAccountHandlerTests` (5), `SubAccountEditValidatorTests` (7), `UpdateCustomerInfoCommandHandlerTests` (+8)
Grand.Web.Admin.Tests	1720 / 1720	`CustomerValidatorOwnerTests` (13), `CustomerViewModelServiceTests` (+12), `StoreCustomerDataScopeTests` (+7)
Grand.Business.Customers.Tests	136 / 136	`GetCustomerQueryHandlerTests` (+2)
Grand.Web.Store.Tests	187 / 187	`CustomerControllerTests` (+3)
Grand.Modules.Tests	242 / 242	(migration project compiles and its suite passes)

The whole solution builds with 0 errors.

E2E (Playwright, fresh install with sample data, per-store identity on, 48/48 checks, repeated green runs). Run against the old Store panel behaviour, the owner, sales employee and price group checks fail, so they are not vacuous. To repeat by hand:

Install, sign in as admin, open Settings → Customer and check that Hide Subaccounts tab is on (the default). Register a customer and open /account/subaccounts: you are redirected. A POST to /account/subaccountadd with a valid token creates nothing.
Switch the tab on, and switch Allow users to change e-mail on. As the customer, add a sub-account, then edit it: set the e-mail to Sub.Two@Firm.COM and a new password in one save. The save succeeds, and the sub-account signs in as SUB.TWO@FIRM.COM with the new password.
Sign in as the sub-account: /account/subaccounts sends you to sign in, and a POST to /account/subaccountadd is refused.
Register a second customer. Open /account/subaccountedit?id=<first owner's sub-account>: you land on your own list and nothing of the account is shown. POST edit and delete for that id: nothing changes, and delete answers "This account is not one of your subaccounts." The same holds for the first owner's own id.
As the first owner, untick Active on the sub-account: the list shows Inactive and the sub-account cannot sign in.
Admin → the owner's customer form shows the Subaccounts card, and the sub-account's form links the Owner. Try setting the second customer's owner to the sub-account's e-mail (refused: is itself a subaccount), the first owner's owner to the second customer (refused: has subaccounts of its own), and an unknown e-mail (refused).
Make a store manager of that store. Give the sub-account a sales employee and a new customer group (for example Wholesale) in the admin. In /Store/Customer/Edit/<sub-account> the owner is shown read only. Change the last name and save: the name is stored, and the owner, sales employee and group are all kept.
Make the second customer a vendor (Vendors group and a vendor). As the store manager: neither that account nor your own appears in the Store customer list, and /Store/Customer/Edit/<id> of either redirects to the list. Plain customers are still listed and editable.
As the owner, change your own e-mail in My account to Owner.New@Firm.COM. It is stored lowercased, and you can sign in typing it in any case.
As the owner, delete the sub-account from the list.

Upgrade path (verified on the E2E database): with the OwnerId_1 index and the 8 new resources removed and the version stamped back to 2.4, the first start ran Index on Customer owner 2.5 together with the other 2.5 migrations. Afterwards the index exists, all 8 resources are imported from en_250.xml, and the stamp is 2.5 again. Rollback: revert the code. The index is harmless if left behind, or can be dropped with db.Customer.dropIndex("OwnerId_1").

Mutation check: putting back three of the old behaviours one at a time (the hidden tab not blocking, the edit command without its own ownership check, the Store panel blanking the owner) makes 8 tests fail. Restored, all are green.

Review

The PR was reviewed after it was opened (9 findings). Commit 31ff502 fixes four of them:

The Store panel's validator checked the posted owner, which the panel replaces anyway. The answers disclosed the state of other stores' customers. It is no longer checked there.
An unchanged owner was accepted even when the customer moved to another store (per-store identity). It is now looked up in the new store.
Sub-accounts already stored with a mixed-case e-mail by the old code stayed locked out, and a password change failed on them when e-mail changes are disabled. An address differing only in case is now repaired on save.
An own deleted sub-account got the message "Delete account" (a button label). It now gets the same answer as a missing or foreign account.

Not changed, with reasons:

ResolveOwnerId keeps the existing owner when the e-mail resolves to nobody. The validator refuses that case on the Admin panel, and on the Store panel keeping the link is the point. Detaching on a lookup miss is the bug this PR removes.
SubAccountDelete answers a switched-off feature with a redirect, not JSON. This is unchanged from before for non-owners, and the list (with its delete buttons) is not rendered when the feature is off.
Lowercasing e-mails inside ICustomerService for every caller: it would change a shared service used by every customer flow. The two storefront callers that actually saved the case as typed (sub-account edit and My account) are fixed directly in this PR instead. The admin path already lowercases through UpdateCustomerInAdminPanel.
The unbounded sub-account card with a per-row HasAccess, and two reads by id in the Store panel's save: the query is already scoped (store, sales employee). Owners have a handful of sub-accounts, and the storefront list is unbounded the same way. Worth paging if real data shows large companies.

Self-assessment

What I am confident about. Every defect above has a unit test, three of the fixes were mutation-checked, and there is an E2E check on a real install (35/35, run again after the review fixes). Security-relevant paths (foreign ids, a sub-account acting as an owner, a switched-off feature, crafted POSTs with a valid antiforgery token, cross-store disclosure in the Store panel) are covered. Scoping follows the panels' existing IAdminDataScope, and nothing new is trusted from the posted model.

What is weaker.

The "used to" statements come from reading the old code and from the mutation check. The E2E did not run A/B against develop.

Deliberately left out (not universal enough to do without a decision, or a separate change):

Blocking sub-account sign-in when the owner is deactivated. Doing it only at the login form would be false security, because external sign-in and live sessions would still pass. It needs one place in the session resolution.
Sub-accounts inheriting the owner's customer groups (company pricing). Copying groups can copy staff roles, so it needs an explicit design.
Score I'd give it: 8.5/10. The fixes are solid and covered on four levels (unit, mutation, E2E, upgrade). Points off for the review's four valid findings, which a more careful first pass should have caught.

Hiding the Subaccounts tab only removed the link: the add, edit and delete actions stayed reachable by URL, and a fresh install hides the tab by default. Every sub-account action now goes through one check, so the setting switches the feature off. The edit command trusted the validator for ownership; it now re-checks it and changes nothing for someone else's, a deleted or a missing account. The edit page answers such an id with a redirect instead of an empty form, the validator gives a missing id and a foreign one the same answer, and the delete endpoint returns the handler's message instead of an empty one. A changed e-mail was stored as typed while sign-in looks it up lowercased, so a sub-account whose owner typed capitals could no longer sign in, and a new password saved together with it failed. The e-mail is stored lowercased and, without usernames, the username follows it. Inactive sub-accounts are marked on the owner's list. Hard-coded messages became resources (also the admin ones used by the next commit).

The Store panel blanked the owner on every customer save, which detached each sub-account a store manager touched. An existing customer now keeps the owner it has; a new one still gets none, and the posted value is never trusted. The admin owner field only checked that the e-mail existed. Sub-accounts stay one level deep now: the owner must be a registered, live customer that is not itself a sub-account, a customer with sub-accounts cannot get an owner, and a customer cannot own itself. The lookup on save uses the same store scope as the validator (it was global, so with per-store identity it could link another store's account), and an owner already linked is kept when posted back unchanged, so legacy data never blocks an unrelated save. The customer form lists an owner's sub-accounts and links a sub-account's owner, read only, on both panels and within each panel's data scope.

Sub-accounts are listed on the storefront and the admin customer form, and the admin checks for them before linking an owner. Without an index each of these scanned the whole customer collection. Fresh installs create it with the other customer indexes, existing ones through a 2.5 migration.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

+        catch (Exception ex)
+        {
+            logService.LogError(ex, "UpgradeProcess - CustomerOwnerIndex (2.5)");
+            return false;
+        }


+        foreach (var item in subAccounts)
+        {
+            if (!await _scope.HasAccess(item))
+                continue;
+
+            model.SubAccounts.Add(new CustomerModel.SubAccountModel {
+                Id = item.Id,
+                Email = item.Email,
+                FullName = item.GetFullName(),
+                Active = item.Active
+            });
+        }


- The Store panel replaces the posted owner before saving, so its validator no longer checks the posted value: the answers told a store manager whether another store's e-mail existed, was a sub-account, was registered or had sub-accounts. - An unchanged owner is kept only while the customer stays in its store; a customer moved to another store has its owner looked up there. - A sub-account e-mail differing only in case is repaired on save even when e-mail changes are not allowed. Older edits stored it as typed, and those rows still could not sign in and failed on a password change. - An owner's own deleted sub-account gets the same answer as a missing or foreign one, as the edit page and the command already give.

…tore saves The Store panel blanked the sales employee, vendor and staff store and reset the groups to Registered on every customer save. A store manager fixing a typo in a name took away the customer's sales employee and price groups, and a colleague's Store manager role. An existing customer now keeps all of these as stored; a new one still gets none of them and Registered only. Posted values are never used either way.

My account saved a changed e-mail or username as typed, while sign-in looks both up lowercased, so a customer who typed capitals could no longer sign in. Both are stored lowercased now, a value differing only in case repairs a row older edits stored as typed, and without usernames the username follows the e-mail as registration sets it.

The Store panel is for a store's registered customers. Its scope also took in any registered account of the store, staff included: administrators, vendors, store managers, sales managers and accounts tied to a vendor or a staff store. These are now outside the panel - left out of the customer list in the query, so paging and totals stay right, and refused when opened by id, which covers every customer tab. GetAllCustomers gains two optional filters for this (excluded groups, and accounts tied to a vendor or a staff store); existing callers are unchanged.

support added 3 commits October 24, 2026 12:12

Copilot AI balanced review requested due to automatic review settings October 24, 2026 10:14

Copilot AI reviewed Oct 24, 2026

github-code-quality Bot found potential problems Oct 24, 2026

View reviewed changes

support added 4 commits October 24, 2026 19:51

Navigation Menu

Repository navigation

Fix and finish company sub-accounts (owner checks, Store save, admin owner rules) - #991

Fix and finish company sub-accounts (owner checks, Store save, admin owner rules)#991
KrzysztofPajak wants to merge 7 commits into
developfrom
feature/company-subaccounts

KrzysztofPajak commented Oct 24, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

KrzysztofPajak commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Breaking changes

Testing

Review

Self-assessment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KrzysztofPajak commented Oct 24, 2026 •

edited

Loading