Implement the account settings page (task 97) #77

Merged

rob merged 4 commits from feat/profile-settings-page into main 2026-08-06 03:23:54 +00:00

Conversation 3 Commits 4 Files changed 17 +1023 -35

rob commented 2026-08-06 02:58:43 +00:00

Owner

Copy link

Builds task 97 on the existing /account page (task 56's placeholder named this ticket explicitly) rather than a floating panel — see ADR-0069 for the argument, since it departs from every panel-hosted UI ticket since ADR-0062.

Scope: view/edit display name and email, change password, link an external sign-in provider (OIDC).

The three consequences the task calls out are surfaced, not left to discover:

• A persistent hint on the email field says changing it changes the sign-in identifier, shown before saving.
• A bold notice above the password form says a password change signs out every device, including the current one, shown before the submit button. On success the page clears local session state via a new PlaceMarkAuthenticationStateProvider.SignOutLocallyAsync (no redundant POST /api/auth/logout, since ADR-0056 already revoked server-side) and sends the browser to /login?notice=password-changed, which renders an explanation rather than leaving the next request to fail with a bare 401.
• A duplicate email is told apart from a validation failure by ApiProblemException.StatusCode/Errors, never by matching Detail text (task 155).

OIDC linking is built, not deferred again — task 56's first deferral pointed at a ticket that was later closed unpicked. Account now has a "Link an external sign-in" button using BeginOidcLinkAsync and the existing challenge/redeem machinery; OidcComplete's link-mode branch (a 204 with no tokens) now returns the visitor to /account?linked=true instead of the placeholder "Nothing further to do." error.

Also: removed NavMenu's separate "Settings" placeholder, now redundant with "Your account" pointing at the same content.

Not verified in a browser (task 156): whether the notices read clearly, and layout on a narrow viewport. bUnit proves pre-population, warning-before-action ordering, the sign-out path, the 409/validation distinction, and every field's aria-describedby pairing.

Full CI sequence (build, test with Testcontainers, dotnet format --verify-no-changes) run locally and clean.

Builds task 97 on the existing `/account` page (task 56's placeholder named this ticket explicitly) rather than a floating panel — see ADR-0069 for the argument, since it departs from every panel-hosted UI ticket since ADR-0062. **Scope:** view/edit display name and email, change password, link an external sign-in provider (OIDC). **The three consequences the task calls out are surfaced, not left to discover:** - A persistent hint on the email field says changing it changes the sign-in identifier, shown before saving. - A bold notice above the password form says a password change signs out every device, including the current one, shown before the submit button. On success the page clears local session state via a new `PlaceMarkAuthenticationStateProvider.SignOutLocallyAsync` (no redundant `POST /api/auth/logout`, since ADR-0056 already revoked server-side) and sends the browser to `/login?notice=password-changed`, which renders an explanation rather than leaving the next request to fail with a bare 401. - A duplicate email is told apart from a validation failure by `ApiProblemException.StatusCode`/`Errors`, never by matching `Detail` text (task 155). **OIDC linking is built, not deferred again** — task 56's first deferral pointed at a ticket that was later closed unpicked. `Account` now has a "Link an external sign-in" button using `BeginOidcLinkAsync` and the existing challenge/redeem machinery; `OidcComplete`'s link-mode branch (a `204` with no tokens) now returns the visitor to `/account?linked=true` instead of the placeholder "Nothing further to do." error. Also: removed `NavMenu`'s separate "Settings" placeholder, now redundant with "Your account" pointing at the same content. **Not verified in a browser** (task 156): whether the notices read clearly, and layout on a narrow viewport. bUnit proves pre-population, warning-before-action ordering, the sign-out path, the 409/validation distinction, and every field's `aria-describedby` pairing. Full CI sequence (build, test with Testcontainers, `dotnet format --verify-no-changes`) run locally and clean.

rob added 1 commit 2026-08-06 02:58:43 +00:00

Build the account settings page: profile, password change and OIDC linking f1f27c5c40

rob reviewed 2026-08-06 03:05:05 +00:00

rob left a comment

Copy link

Verdict: changes needed

Build, dotnet test (WebUI + Architecture), and dotnet format --verify-no-changes all clean on the pinned 10.0.100 SDK. Mutated a validate-on-blur handler into Account.razor/.cs to confirm FieldErrorsValidatesOnSubmitOnlyTests actually reddens against this new consumer (it does), then reverted. ADR numbering is fine: PR #76 claims 0068, this claims 0069, no collision, README rows both append cleanly against main.

Traced the link redemption end to end (no browser available, task 156): StartOidcLinkAsync calls BeginOidcLinkAsync/RedeemOidcAsync through PlaceMarkApiClient, which attaches the caller's bearer token via BearerTokenHandler on every request, so step 4 is authenticated as the signed-in caller as ADR-0032 requires. OidcEndpoints.CompleteLinkAsync refuses unless callerUserId == payload.LinkUserId, and OidcEndpointsTests.Redeem_LinkHandleWasMintedForADifferentAccount_RefusesTheLink already covers the CSRF case server-side — nothing in this PR touches that. A failed, replayed or stale redemption throws ApiException inside OidcComplete.OnInitializedAsync and never reaches the ?linked=true redirect, so that path is sound.

One real problem: Account.razor's "linked" banner trusts the query parameter outright, not server state. Linked is [SupplyParameterFromQuery] and the markup is simply @if (Linked is not null) — anyone who types, bookmarks, or is sent /account?linked=true sees "This sign-in provider is now linked to your account" with zero connection to any actual link event. This isn't a hypothetical: AccountTests.Account_LinkedQueryParameterPresent_ShowsASuccessMessage proves it by navigating straight to ?linked=true with no OIDC flow involved at all. Compare OidcComplete, which strips its own code parameter via NavigateTo(..., replace: true) once consumed — Account never does the equivalent for linked, so the banner also persists across every reload of that URL, not just the one visit after a genuine link. There's no endpoint that exposes actual linked-provider state to re-derive this from, so the minimal fix is to stop it being replayable: strip linked from the URL immediately after reading it (same pattern OidcComplete already uses for code), and ideally have Account consume it once via a local flag rather than re-reading Linked on every render. Given the reviewing brief calls this out by name as a "misleading security signal," it should be fixed or the residual explicitly argued in ADR-0069 before merge, not left silent.

Everything else checks out: warnings for both email-changes-sign-in and password-revokes-everything sit ahead of their submit buttons (asserted by markup order, not visually); SignOutLocallyAsync skipping POST /api/auth/logout is sound because PutMyPassword awaits RevokeAllForUserAsync before returning 204, so any partial failure surfaces as an API error the WebUI's catch block handles without ever calling SignOutLocallyAsync or navigating away — confirmed by reading UserEndpoints.PutMyPassword directly. Login's notice parameter is a closed vocabulary lookup (_notices dictionary), not rendered free text, so it can't be used to inject arbitrary copy. The 409/validation distinction uses ApiProblemException.StatusCode, matching GroupForm's existing pattern. NavMenu still reaches settings via "Your account". ADR-0069's page-over-panel argument is substantive rather than asserted — it's grounded in "nothing here is about the map or a group" and the reversibility cost is stated, consistent with ADR-0062/0065/0067's own reasoning rather than contradicting it.

Cannot verify without a browser (task 156, as the PR itself says): whether either notice actually reads clearly, whether the linked/password-changed confirmations are noticed on return to their pages, and layout on a narrow viewport. Also not exercised: an actual live round trip through a real OIDC provider — verified instead by static tracing plus the existing (unchanged) API-level tests for the link CSRF case.

Verdict: changes needed Build, `dotnet test` (WebUI + Architecture), and `dotnet format --verify-no-changes` all clean on the pinned 10.0.100 SDK. Mutated a validate-on-blur handler into `Account.razor`/`.cs` to confirm `FieldErrorsValidatesOnSubmitOnlyTests` actually reddens against this new consumer (it does), then reverted. ADR numbering is fine: PR #76 claims 0068, this claims 0069, no collision, README rows both append cleanly against `main`. Traced the link redemption end to end (no browser available, task 156): `StartOidcLinkAsync` calls `BeginOidcLinkAsync`/`RedeemOidcAsync` through `PlaceMarkApiClient`, which attaches the caller's bearer token via `BearerTokenHandler` on every request, so step 4 is authenticated as the signed-in caller as ADR-0032 requires. `OidcEndpoints.CompleteLinkAsync` refuses unless `callerUserId == payload.LinkUserId`, and `OidcEndpointsTests.Redeem_LinkHandleWasMintedForADifferentAccount_RefusesTheLink` already covers the CSRF case server-side — nothing in this PR touches that. A failed, replayed or stale redemption throws `ApiException` inside `OidcComplete.OnInitializedAsync` and never reaches the `?linked=true` redirect, so that path is sound. **One real problem: `Account.razor`'s "linked" banner trusts the query parameter outright, not server state.** `Linked` is `[SupplyParameterFromQuery]` and the markup is simply `@if (Linked is not null)` — anyone who types, bookmarks, or is sent `/account?linked=true` sees "This sign-in provider is now linked to your account" with zero connection to any actual link event. This isn't a hypothetical: `AccountTests.Account_LinkedQueryParameterPresent_ShowsASuccessMessage` proves it by navigating straight to `?linked=true` with no OIDC flow involved at all. Compare `OidcComplete`, which strips its own `code` parameter via `NavigateTo(..., replace: true)` once consumed — `Account` never does the equivalent for `linked`, so the banner also persists across every reload of that URL, not just the one visit after a genuine link. There's no endpoint that exposes actual linked-provider state to re-derive this from, so the minimal fix is to stop it being replayable: strip `linked` from the URL immediately after reading it (same pattern `OidcComplete` already uses for `code`), and ideally have `Account` consume it once via a local flag rather than re-reading `Linked` on every render. Given the reviewing brief calls this out by name as a "misleading security signal," it should be fixed or the residual explicitly argued in ADR-0069 before merge, not left silent. Everything else checks out: warnings for both email-changes-sign-in and password-revokes-everything sit ahead of their submit buttons (asserted by markup order, not visually); `SignOutLocallyAsync` skipping `POST /api/auth/logout` is sound because `PutMyPassword` awaits `RevokeAllForUserAsync` before returning `204`, so any partial failure surfaces as an API error the WebUI's catch block handles without ever calling `SignOutLocallyAsync` or navigating away — confirmed by reading `UserEndpoints.PutMyPassword` directly. `Login`'s `notice` parameter is a closed vocabulary lookup (`_notices` dictionary), not rendered free text, so it can't be used to inject arbitrary copy. The 409/validation distinction uses `ApiProblemException.StatusCode`, matching `GroupForm`'s existing pattern. `NavMenu` still reaches settings via "Your account". ADR-0069's page-over-panel argument is substantive rather than asserted — it's grounded in "nothing here is about the map or a group" and the reversibility cost is stated, consistent with ADR-0062/0065/0067's own reasoning rather than contradicting it. **Cannot verify without a browser** (task 156, as the PR itself says): whether either notice actually reads clearly, whether the linked/password-changed confirmations are noticed on return to their pages, and layout on a narrow viewport. Also not exercised: an actual live round trip through a real OIDC provider — verified instead by static tracing plus the existing (unchanged) API-level tests for the link CSRF case.

rob added 1 commit 2026-08-06 03:11:59 +00:00

Verify a link confirmation before showing it, not just the query string 17094a8b23

rob reviewed 2026-08-06 03:16:24 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed the delta (f1f27c5→17094a8) in a fresh scratch worktree, pinned SDK (10.0.100). Full WebUI suite (356 passed), Architecture suite (27 passed), and dotnet format --verify-no-changes all clean.

Checked each point:

• Hand-typed /account?linked=true now shows nothing. Confirmed by Account_LinkedQueryParameterWithNoSessionStorageConfirmation_ShowsNoSuccessMessage (ran, passes) and by reading: _justLinked is only true when sessionStorage actually held OidcSessionStorageKeys.LinkConfirmed, which nothing but OidcComplete's own post-204 write ever sets.
• Single-use. RemoveItemAsync runs unconditionally, immediately after the read, before _justLinked is even assigned — so a second load of the page finds nothing regardless of the first outcome. The shipped test proves the banner appears once and the query string is gone; it doesn't separately render a second time to prove the marker is gone too, but that follows directly from the unconditional removal and needs no further test.
• Cannot be forged cross-origin, and nothing else in the tree reads LinkConfirmed — grepped; only OidcComplete writes it and only Account reads/clears it. It remains forgeable by same-origin script (anyone with devtools on their own session), which is fine since nothing keys on it but display — but the doc comment on OidcSessionStorageKeys.LinkConfirmed calls it "proof," which overstates that. Minor wording fix, not blocking: say it rules out a hand-typed/replayed URL specifically, not that it's authoritative.
• Clear on a throw. Not guarded — GetItemAsync/RemoveItemAsync have no try/catch, so a throw from the read would skip the removal and also skip the profile load beneath it in OnInitializedAsync. This is the same unguarded shape already used for Verifier/ReturnUrl throughout this file and OidcComplete, not a new gap introduced by this fix, so not blocking here — but it means a sessionStorage failure now takes the whole page down where before this change it didn't touch OnInitializedAsync at all. Worth a wrapping ticket if it's ever seen in practice, not this PR's problem to solve.
• Stripped on every path. The success path strips unconditionally, before knowing whether confirmed was set. A failed/replayed/stale redemption never sets the query parameter in the first place, so there's nothing to strip on that path. Uses GetUriWithQueryParameter("linked", null), which removes only that parameter — actually more correct than OidcComplete's own GetLeftPart(UriPartial.Path) for code, which wipes the whole query string; moot today since /account takes no other parameters, but worth knowing if one is ever added.
• Transient-cue judgement: legitimate scope limit, not a defect in this PR. Task 97's own acceptance criteria ask for linking to work, not for a persistent view of what's currently linked, and UserResponse has never carried that. Worth a follow-up ticket for "show current linked-provider state on account settings" — please file it.

Unrelated to this delta: the PR now reports mergeable: false against current main — a textual conflict in docs/adr/README.md's index table (adjacent rows for 0067/0068/0069), not a numbering collision; 0068 and 0069 are still distinct. Needs a rebase before merge, not a code change.

Verdict: mergeable Re-reviewed the delta (`f1f27c5`→`17094a8`) in a fresh scratch worktree, pinned SDK (`10.0.100`). Full WebUI suite (356 passed), Architecture suite (27 passed), and `dotnet format --verify-no-changes` all clean. Checked each point: - **Hand-typed `/account?linked=true` now shows nothing.** Confirmed by `Account_LinkedQueryParameterWithNoSessionStorageConfirmation_ShowsNoSuccessMessage` (ran, passes) and by reading: `_justLinked` is only true when `sessionStorage` actually held `OidcSessionStorageKeys.LinkConfirmed`, which nothing but `OidcComplete`'s own post-204 write ever sets. - **Single-use.** `RemoveItemAsync` runs unconditionally, immediately after the read, before `_justLinked` is even assigned — so a second load of the page finds nothing regardless of the first outcome. The shipped test proves the banner appears once and the query string is gone; it doesn't separately render a *second* time to prove the marker is gone too, but that follows directly from the unconditional removal and needs no further test. - **Cannot be forged cross-origin**, and nothing else in the tree reads `LinkConfirmed` — grepped; only `OidcComplete` writes it and only `Account` reads/clears it. It remains forgeable by same-origin script (anyone with devtools on their own session), which is fine since nothing keys on it but display — but the doc comment on `OidcSessionStorageKeys.LinkConfirmed` calls it "proof," which overstates that. Minor wording fix, not blocking: say it rules out a hand-typed/replayed URL specifically, not that it's authoritative. - **Clear on a throw.** Not guarded — `GetItemAsync`/`RemoveItemAsync` have no try/catch, so a throw from the read would skip the removal and also skip the profile load beneath it in `OnInitializedAsync`. This is the same unguarded shape already used for `Verifier`/`ReturnUrl` throughout this file and `OidcComplete`, not a new gap introduced by this fix, so not blocking here — but it means a `sessionStorage` failure now takes the whole page down where before this change it didn't touch `OnInitializedAsync` at all. Worth a wrapping ticket if it's ever seen in practice, not this PR's problem to solve. - **Stripped on every path.** The success path strips unconditionally, before knowing whether `confirmed` was set. A failed/replayed/stale redemption never sets the query parameter in the first place, so there's nothing to strip on that path. Uses `GetUriWithQueryParameter("linked", null)`, which removes only that parameter — actually more correct than `OidcComplete`'s own `GetLeftPart(UriPartial.Path)` for `code`, which wipes the whole query string; moot today since `/account` takes no other parameters, but worth knowing if one is ever added. - **Transient-cue judgement: legitimate scope limit, not a defect in this PR.** Task 97's own acceptance criteria ask for linking to work, not for a persistent view of what's currently linked, and `UserResponse` has never carried that. Worth a follow-up ticket for "show current linked-provider state on account settings" — please file it. Unrelated to this delta: the PR now reports `mergeable: false` against current `main` — a textual conflict in `docs/adr/README.md`'s index table (adjacent rows for 0067/0068/0069), not a numbering collision; 0068 and 0069 are still distinct. Needs a rebase before merge, not a code change.

rob added 2 commits 2026-08-06 03:21:14 +00:00

Soften the link-confirmation marker's doc comment 88a2a7f5d1

Merge remote-tracking branch 'origin/main' into feat/profile-settings-page ... 03f5c2f481

# Conflicts:
#	docs/adr/README.md

rob reviewed 2026-08-06 03:23:26 +00:00

rob left a comment

Copy link

Verdict: mergeable

Clean. Reviewed the delta only (88a2a7f softening commit plus the merge of main), pinned SDK, build/WebUI-tests(374 passed)/format all clean, mergeable: true confirmed via the PR API.

All four locations — OidcSessionStorageKeys.LinkConfirmed's doc comment, Account.Linked's doc comment, the write-site comment in OidcComplete, and ADR-0069's new paragraph — now agree: display cue, not a security control, forgeable by same-origin console, closes only the hand-typed/replayed-URL case. Grepped for "proof"/"evidence" across all four; the one hit left is OidcSessionStorageKeys.cs's own "not evidence that one did", which is the negation, correctly stated.

ADR-0069 states plainly this came out of review: "The query string alone is not trusted — caught in review, not written correctly the first time," matching the house style already used in ADR-0058/0062/0067 for the same kind of correction, not presented as original intent.

main merge touched only docs/adr/README.md's table: 0067 → 0068 → 0069 in order, PR #76's Accepted (partly superseded by 0068) annotation on 0067 intact, no conflict markers left anywhere. The merge commit's diff from its first parent matches PR #76's own file list exactly (18 files) — nothing else came along for the ride.

Verdict: mergeable Clean. Reviewed the delta only (`88a2a7f` softening commit plus the merge of `main`), pinned SDK, build/WebUI-tests(374 passed)/format all clean, `mergeable: true` confirmed via the PR API. All four locations — `OidcSessionStorageKeys.LinkConfirmed`'s doc comment, `Account.Linked`'s doc comment, the write-site comment in `OidcComplete`, and ADR-0069's new paragraph — now agree: display cue, not a security control, forgeable by same-origin console, closes only the hand-typed/replayed-URL case. Grepped for "proof"/"evidence" across all four; the one hit left is `OidcSessionStorageKeys.cs`'s own "not evidence that one did", which is the negation, correctly stated. ADR-0069 states plainly this came out of review: "The query string alone is not trusted — caught in review, not written correctly the first time," matching the house style already used in ADR-0058/0062/0067 for the same kind of correction, not presented as original intent. `main` merge touched only `docs/adr/README.md`'s table: 0067 → 0068 → 0069 in order, PR #76's `Accepted (partly superseded by 0068)` annotation on 0067 intact, no conflict markers left anywhere. The merge commit's diff from its first parent matches PR #76's own file list exactly (18 files) — nothing else came along for the ride.

rob merged commit 9b964bbf2c into main 2026-08-06 03:23:54 +00:00

rob deleted branch feat/profile-settings-page 2026-08-06 03:23:54 +00:00

rob referenced this pull request from a commit 2026-08-06 03:23:55 +00:00

Build the account settings page with OIDC linking (#77)

rob referenced this pull request 2026-08-20 06:37:17 +00:00

Give the image viewer the window, and both images a spinner #196

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!77

No description provided.