feat: implement email change flow and harden password reset#301
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an authenticated email-change flow (request + verify) and strengthens both email-change and password-reset links by introducing per-user version counters that invalidate stale links after any underlying credential mutation.
Changes:
- Implement
POST /1/account/emailto create an email-change request (requires current password) and send a verification link to the new address; apply change onPOST /1/account/verify-email. - Add
PasswordVersion/EmailVersiontousersand snapshot columns onuser_password_resets/user_email_changes, enforcingsnapshot == currentat verification/consumption time. - Add cleanup cron job for expired email-change requests and integration tests covering happy paths + invalidation behavior.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Cron/Jobs/ClearOldEmailChangesJob.cs | Daily cleanup job for unused/expired email change requests. |
| Common/OpenShockDb/UserPasswordReset.cs | Adds snapshot version column + improved doc comments for reset rows. |
| Common/OpenShockDb/UserEmailChange.cs | Adds snapshot version column + documents the email-change lifecycle. |
| Common/OpenShockDb/User.cs | Introduces PasswordVersion / EmailVersion counters on users. |
| Common/OpenShockDb/OpenShockContext.cs | Maps new version columns for users and snapshot columns for reset/change tables. |
| Common/Migrations/OpenShockContextModelSnapshot.cs | Updates EF model snapshot to include new columns and defaults. |
| Common/Migrations/20260520153340_AddPasswordAndEmailVersions.Designer.cs | Generated migration designer reflecting the updated model. |
| Common/Migrations/20260520153340_AddPasswordAndEmailVersions.cs | Migration adding the four new integer columns with default 0. |
| Common/Errors/AccountError.cs | Adds new problems for email-change flow outcomes (conflict/unchanged/too-many/not-found). |
| Common/Constants/Constants.cs | Adds EmailChangeRequestLifetime. |
| API/Services/Email/Smtp/SmtpEmailService.cs | Fixes template variable name for email verification link (VerifyLink). |
| API/Services/Account/IAccountService.cs | Adds CreateEmailChangeFlowAsync and related result marker structs. |
| API/Services/Account/AccountService.cs | Implements email change request + verification; adds version predicates and bumps versions on password/email mutation. |
| API/Models/Requests/ChangeEmailRequest.cs | Extends request model to include CurrentPassword. |
| API/Controller/Account/VerifyEmail.cs | Makes invalid/expired tokens return 400 Account.Email.VerifyNotFound instead of silently OK. |
| API/Controller/Account/Authenticated/ChangeEmail.cs | Implements the previously stubbed /1/account/email endpoint. |
| API.IntegrationTests/Tests/MailTests.cs | Adds integration coverage for email-change flow + sibling invalidation behavior. |
Files not reviewed (1)
- Common/Migrations/20260520153340_AddPasswordAndEmailVersions.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…throws TryVerifyEmailAsync now returns OneOf<Success, NotFound, EmailAlreadyInUse> instead of bool, so the 23505 race (another account claims the new address between request creation and verification) surfaces as 409 Conflict rather than 400 VerifyNotFound. Update VerifyEmail controller to match on the richer result type and add the ProducesResponseType annotation for 409. Replace throw new Exception with throw new UnreachableException in ChangeEmail and ChangePassword controllers.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- Common/Migrations/20260520153340_AddPasswordAndEmailVersions.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
API/Services/Account/AccountService.cs:541
Include(x => x.UserDeactivation)is ignored here because the query projects withSelect(...). As a result,data.User.UserDeactivationwill always be null (no lazy-loading configured), so deactivated accounts may still be able to initiate email changes. Consider projecting the deactivation predicate (e.g.,IsDeactivated = x.UserDeactivation != null) or querying the user entity with the navigation loaded before projecting/counting pending requests.
var data = await _db.Users
.Where(x => x.Id == userId)
.Include(x => x.UserDeactivation)
.Select(x => new
{
Rename to consistent resource paths:
POST /2/account/reset-password -> /2/account/password-reset
HEAD /1/account/recover/{id}/{secret} -> GET /1/account/password-reset/{id}/{secret}
POST /1/account/recover/{id}/{secret} -> POST /1/account/password-reset/{id}/{secret}/complete
POST /1/account/verify-email -> POST /1/account/email-change/verify
POST /1/account/email (authenticated) -> POST /1/account/email-change
Switch the reset-validity check from HEAD to GET so it can carry a body
and isn't subject to proxy quirks around HEAD.
Send an informational notice to the user's previous address when an
email change is initiated, alongside the verification email to the new
address. The notice contains no action link. Best-effort: a send failure
is logged but does not roll back the request.
Integration tests cover the new notice path, the unchanged-email
rejection, and a direct 404 on the renamed reset-check endpoint, plus
the existing flows now exercise the renamed routes end-to-end.
The old completion route stays as a working alias for clients that
already integrated against it. Both routes call the same handler; the
legacy one is marked [Obsolete] so it shows up as deprecated in the
generated OpenAPI spec.
New endpoint: POST /1/account/password-reset/{id}/{secret}/complete
Legacy alias: POST /1/account/recover/{id}/{secret} (deprecated)
Integration test asserts the legacy route still completes a reset.
Same treatment as the complete route: the legacy HEAD check stays as a
working alias delegating to the same handler, marked [Obsolete] so it
surfaces as deprecated in the OpenAPI spec.
New endpoint: GET /1/account/password-reset/{id}/{secret}
Legacy alias: HEAD /1/account/recover/{id}/{secret} (deprecated)
Integration test confirms the legacy HEAD route still returns 200 for a
valid pending reset.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Adds AccountNotActivated to the OneOf return of CreateEmailChangeFlowAsync and ChangePasswordAsync, so the service refuses to act on a user whose ActivatedAt is null. Both controllers map it to UnreachableException since the only callers are session-authenticated and login already blocks unactivated accounts; the guard is for paths that bypass the controller.
…nvr:OpenShock/API into feature/email-and-password-change-flow
- fix Include() being dropped under Select() projection in CreatePasswordResetFlowAsync and CreateEmailChangeFlowAsync; deactivation was never detected. Project IsDeactivated explicitly. - race-safe complete/verify via ExecuteUpdateAsync gated on the SecurityStamp predicate so two concurrent valid tokens cannot both win and clobber the user's password/email. - add deprecated aliases POST /reset-password and POST /verify-email delegating to the renamed routes. - ChangeEmail: return AccountOAuthOnly (401) for users without a password, instead of the misleading PasswordChangeInvalidPassword. - fix unresolved cref to OpenShock.Common.Constants.Duration in UserPasswordReset and UserEmailChange xmldocs. - clarify User.SecurityStamp doc to make explicit that re-hashing the same password is not a rotation. - update stale EmailVersion comment in MailTests.
hhvrc
changed the title
hhvrc
changed the title
…ness - Send verification email before committing the email-change row so a mailer failure no longer leaves orphan rows counting against the rate limit - Make deactivated path unreachable in ChangeEmail/ChangePassword to match notActivated/notFound — UserSessionAuthentication blocks all three - EmailUnchanged compare is now OrdinalIgnoreCase (defensive) - Note that the password-rehash path intentionally does not rotate SecurityStamp (credential value is unchanged) - Sibling-reset test no longer depends on Mailpit ordering
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
Implements the previously-stubbed authenticated email change flow and hardens the password reset flow against stale and concurrent links via a
User.SecurityStamprotation mechanism. Restructures account flow URLs with deprecated aliases for backwards compatibility.Routes
POST /2/account/password-resetPOST /2/account/reset-passwordGET /1/account/password-reset/{id}/{secret}HEAD /1/account/recover/{id}/{secret}POST /1/account/password-reset/{id}/{secret}/completePOST /1/account/recover/{id}/{secret}POST /1/account/email-change(auth)POST /1/account/email-change/verifyPOST /1/account/verify-emailHEADcheck becameGETso it can carry a body later.Email change
Auth + current password. OAuth-only accounts get
AccountOAuthOnly(401).Verification mail goes to the new address;
users.emailis only written on confirmation.An informational notice goes to the old address simultaneously so the legitimate owner sees the request if the session was compromised.
Cross-account email conflict at verify time is caught and returned as 409.
SecurityStamp
User.SecurityStampis a Guid rotated on any password value or email change.Pending reset / email-change rows snapshot it at creation; verify/complete queries gate on
SecurityStampAtCreate == User.SecurityStamp.Any change through any path silently kills every other outstanding link for that user.
No sibling-row UPDATE to forget. BCrypt re-hashes (same password) do not rotate.
Concurrency
CompletePasswordResetFlowAsyncandTryVerifyEmailAsyncuseExecuteUpdateAsyncwith the stamp predicate inline.Two concurrent valid tokens cannot both win; the loser sees zero rows updated and returns
NotFound.Migration
20260521205924_AddUserSecurityStampaddsusers.security_stamp(defaultgen_random_uuid()) andsecurity_stamp_at_createon both child tables.It also
DELETEs existing rows inuser_password_resetsanduser_email_changessince pre-existing pending rows have no valid stamp to snapshot.Other fixes
EmailVerification.liquidwas usingActivationLinkwhere the templateexpected
VerifyLink; verification links were not rendering.POST /1/account/verify-emailpreviously returned 200 unconditionally; nowreturns 400 / 409 as appropriate.
Includewas silently dropped under aSelectprojection in two flows;deactivated users could initiate resets / email changes. Fixed.
ChangeEmail/ChangePasswordservices refuse unactivated accounts.ClearOldEmailChangesJobmirroring the password reset cleanup.