Add the Plan 6 implementation plan for account settings - #62
Merged
Merged
Conversation
Eight tasks against the signed-off design: the change-email token provider and email seam, three endpoints (change-password, change-email, confirm-email-change), a real-cookie suite, the Account screen, the confirm-email-change screen, and the PR write-up. All eight of the design's open items are resolved against dotnet/aspnetcore release/10.0. Three answers changed the plan: - ChangeEmailAsync distinguishes DuplicateEmail from InvalidToken, so the 409/400 split works off the error code and no pre-check moves into the confirm handler. - SetUserNameAsync rotates the security stamp a second time, so the same-instance requirement is real rather than precautionary. - RefreshSignInAsync returns no result and is a silent no-op under the Test auth scheme. Two of the design's required assertions would have passed vacuously where its task list put them, so they move to their own real-cookie suite - the eighth task the design does not have. Test totals pinned 257 -> 291. Two tasks end with a mutation check rather than a green run: removing SetUserNameAsync must fail exactly one test, likewise RefreshSignInAsync. Nine deviations recorded for the PR body, five of them introduced at plan time. Nothing in docs/backlog.md is owed, and the stolen-session takeover stays open as a Plan 8 launch gate.
HenryElendheim
approved these changes
Aug 31, 2026
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.
Docs only — no production code, no tests, nothing to run. This is the implementation plan for
Plan 6 (account settings), written against the tree at
e6bda61now that the auth-input fix andremember-me have both merged.
It follows the design merged in #48, and the reviewer's checklist that pairs with it is
docs/2026-08-13-wend-review-guide.md§ Plan 6 review checklist.What's in it
Eight tasks: the change-email token provider and the two new email-seam methods; three endpoints
(
/change-password,/change-email,/confirm-email-change); a real-cookie test suite; theAccount screen; the
/confirm-email-changescreen; and the PR write-up. Every step carries theactual code rather than a description of it.
The three findings worth your attention
The design closed by asking that its eight open items be verified against the .NET 10 source
before the plan was written rather than assumed. All eight are now answered against
dotnet/aspnetcorerelease/10.0. Three of the answers changed the plan:ChangeEmailAsyncdoes distinguishDuplicateEmailfromInvalidToken.ChangeEmailCoreAsyncreturns
InvalidTokenon a token failure and otherwise reachesUpdateUserAsync→UserValidator, whose duplicate branches useCode = nameof(DuplicateEmail). So the 409-vs-400split works off the error code, and the pre-check does not have to move into the confirm handler
as the design worried it might.
SetUserNameAsyncrotates the security stamp a second time. Harmless — every session isalready gone from the first rotation — but it means two writes, and it is why the second call
must use the same
userinstance rather than a reload.RefreshSignInAsyncreturns no result, and is a silent no-op under theTestauth scheme.It returns
Task, notTask<IdentityResult>, so "if it fails" can only mean an exception. Moreimportantly it calls
Context.AuthenticateAsync(ApplicationScheme), finds no cookie under thetest scheme, logs an error and returns.
SecurityStampValidatorhangs off the cookie too, so itnever runs there either.
This is the one that changed the shape of the plan. Two of the design's own required
assertions — the acting session surviving and other sessions dying — would have passed vacuously
where the design's task list put them. They move into their own real-cookie suite, which is the
eighth task the design's seven-task breakdown does not have.
Verification built into the plan
Test totals are pinned at 257 → 291, per task. Two tasks end with a mutation check rather than a
green run:
SetUserNameAsync→ exactly one test must fail(
Registering_the_old_address_afterwards_still_works);RefreshSignInAsync→ exactly one test must fail(
The_acting_session_survives_a_password_change_while_another_one_dies).If either passes with the call removed, the guard is decorative and the task is not done.
Deviations
Nine are recorded up front for the implementation PR's body — four inherited from the design, five
introduced at plan time. The one most worth a second opinion is #6: the change-email success
message says "if that address is free, we've sent it a confirmation link" rather than naming the
address it went to. The endpoint answers 204 for both a free address and one somebody else holds,
so a screen claiming a link was sent would leak exactly what that 204 exists to protect.
Not touched
docs/backlog.mdis unchanged. Its four Plan 6 entries were already extended on 2026-08-13, andthe stolen-session takeover stays open as a Plan 8 launch gate — the plan says so explicitly so
nobody closes it in passing.