Repository navigation
Check customer e-mails in the account's store and change passwords on the account - #1009
Open
KrzysztofPajak wants to merge 3 commits into
Open
KrzysztofPajak wants to merge 3 commits into
KrzysztofPajak wants to merge 3 commits into
Conversation
added 2 commits
November 1, 2026 09:19
The storefront validators checked an e-mail or username as typed and in the current store, while the commands store it trimmed and lowercased on an account that belongs to its own store. They now check the stored form in the store of the account being edited, including when a case-only repair applies. A password change finds its account by e-mail, and with per-store identity one e-mail can belong to several stores. The customer's own change and the SCIM credentials reset now use the store of the account they act on, and a sub-account password is changed only when its e-mail resolves to that account.
These callers already hold the account, but the password change looked it up again by e-mail, a value that can change between the two steps and is unique only as far as the application keeps it so. A new overload takes the customer and the existing method now resolves the e-mail and calls it. The sub-account edit no longer needs its check that the e-mail leads back to the account, nor can it skip the remaining edits without telling the owner.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The password-change overload writes a stale customer snapshot, potentially overwriting concurrent account edits or reactivating a deactivated account, so credential updates must be persisted atomically by customer ID.
1 open issue
Review effort: Balanced
| /// <param name="customer">Customer</param> | ||
| /// <param name="passwordFormat">Password format</param> | ||
| /// <param name="newPassword">New password</param> | ||
| public virtual async Task ChangePassword(Customer customer, PasswordFormat passwordFormat, string newPassword) |
Member
Author
There was a problem hiding this comment.
Fixed in d8d1c71: a new ICustomerService.UpdateCustomerPassword writes only the four password fields in one operation by customer id, keeps the system-account check and the update event. Password history and token rotation are unchanged.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A concurrent e-mail change can cause the password update to reset a different account’s password, while guard failures silently discard edits without notifying the user.
2 open issues
What changed in this PR
This follow-up to #991 brings account-store validation and password targeting fixes to develop, alongside the related 2.4.x changes in #1008.
Changes:
- Validate normalized e-mails and usernames in the edited account’s store, including case-only e-mail repairs.
- Scope storefront and SCIM password changes to the account’s store, and check sub-account identity before changing its password.
- Add regression tests for normalization, store scoping, and mismatched password targets.
Review covered the supplied changes and relevant lookup and controller behavior. No builds or tests were run during this review.
| File | Description |
|---|---|
src/Web/Grand.Web/Validators/Customer/SubAccountEditValidator.cs |
Checks normalized e-mails in the sub-account’s store. |
src/Web/Grand.Web/Validators/Customer/CustomerInfoValidator.cs |
Uses normalized identity values and the customer’s store. |
src/Web/Grand.Web/Controllers/AccountController.cs |
Passes the customer’s store to password changes. |
src/Web/Grand.Web/Commands/Handler/Customers/SubAccountEditCommandHandler.cs |
Checks that the password lookup resolves to the edited account. |
src/Tests/Grand.Web.Tests/Validators/Customer/CustomerValidatorsStoreScopingTests.cs |
Tests normalization, store scoping, and case-repair conflicts. |
src/Tests/Grand.Web.Tests/Controllers/AccountControllerProviderOnlyTests.cs |
Verifies store-scoped password changes. |
src/Tests/Grand.Web.Tests/Commands/Customers/SubAccountEditCommandHandlerTests.cs |
Tests rejection of a mismatched password target. |
src/Tests/Grand.Business.Authentication.Tests/Services/Scim/ScimAccountLockTests.cs |
Tests account-store scoping for credential resets. |
src/Business/Grand.Business.Authentication/Services/Scim/ScimAccountLock.cs |
Passes the account’s store to credential resets. |
Review effort: Balanced
The password change saved the whole customer, including its e-mail, username, store and active flag. With a copy the caller loaded earlier in the request, that could undo an edit made in the meantime, such as a deactivation. A dedicated update now writes the four password fields in one operation, and still refuses a system account and raises the update event.
KrzysztofPajak
changed the title
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: bugfix
Issue
Follow-up to #991, and the
developside of #1008. Customer e-mails and passwords were not handled the same way in every step:Solution
SubAccountEditValidatorandCustomerInfoValidatorcheck the normalized value in the store of the edited account, also when a case-only repair applies.ICustomerManagerServicegetsChangePassword(Customer, PasswordFormat, string), which changes the password of that account without an e-mail lookup. The existing method resolves the e-mail and calls it. Both write only the password fields through a newICustomerService.UpdateCustomerPassword, so a copy the caller loaded earlier cannot undo other edits.Password recovery and the admin panel keep the existing method. The API
SetPasswordalso keeps it, because it receives only an e-mail.No new resources, settings or migrations.
Breaking changes
ICustomerManagerServiceandICustomerServiceeach have a new member, so a custom implementation of either must add it. Callers are not affected. An account whose stored e-mail differs only in case now has that address checked on save, even when changing e-mails is turned off.Testing
Unit tests pass:
Grand.Web.Tests(402),Grand.Business.Authentication.Tests(399),Grand.Business.Customers.Tests(142),Grand.Web.Admin.Tests(1839). The new tests failed before the callers were switched. The full solution and the steps below were not run.HideSubAccountsTabtofalse, sign in on the storefront, add a sub-account, change its password, and check it can sign in with the new one.AllowUsersToChangeEmailon, change its e-mail to one with capitals and surrounding spaces, together with a new password. The address is saved trimmed and lowercased, and the sub-account can sign in. An address that is already taken is rejected in any form.Customer:RegisterCustomersPerStoreon and the same e-mail registered in two stores, change the password in one store. The account in the other store keeps its password.