PR 7 — Account settings: change password + notification prefs + delete account#11
PR 7 — Account settings: change password + notification prefs + delete account#11
Conversation
…(PR 7)
Wires the three new Account-screen cards into sysnode-info and extends
authService + VaultContext with the client-side crypto orchestration
the backend expects.
Change password
- authService: deriveChangePasswordKeys(oldPassword, newPassword, email)
separates key derivation from the API call so the card can orchestrate
new-master -> vault rewrap -> submit without authService having to
know about vault internals (same pattern as login).
- VaultContext.rewrapForPasswordChange(newMaster) wraps the existing
envelope under the newly-derived vaultKey and returns {blob,etag}
ready to POST; if no vault exists, returns null so the route treats
it as a pure password change.
- ChangePasswordCard ties it all together: derive new master, rewrap
vault, POST {oldAuthHash,newAuthHash,vault?}, surface 409
vault_rewrap_required as a hard error rather than papering over it.
Notification preferences
- authService.getPrefs() / updatePrefs(prefs).
- NotificationPreferencesCard gates its form behind a `hydrated` flag
so clicks cannot race with the initial /auth/me round-trip
(previously the source of flaky double-toggles).
Delete account (GDPR)
- authService.deleteAccount({oldAuthHash}).
- DeleteAccountCard is a two-step destructive UX: collapsed
"Delete account..." entry -> expanded warning list + email-match +
current-password re-proof. Email match is case-insensitive and
trim-tolerant so "alice@EXAMPLE.com " confirms correctly. On success
triggers useAuth().handleAuthLost() (which tears down VaultContext
via its isAuthenticated watcher) and history.replace('/').
Notes for tests
- New component tests reinstall deriveLoginKeys mock impl in beforeEach:
CRA's Jest config defaults to resetMocks: true which wipes
jest.fn(() => ...) implementations between tests. Without this the
tests would silently fall through to real 600k-iter PBKDF2 and blow
past testing-library's default 1s waitFor window. Same pattern
already used in ProposalVoteModal.test.js.
- DeleteAccountCard uses fireEvent.submit / fireEvent.change instead of
userEvent.click to avoid a React 17 + jsdom + userEvent@13 stale-state
race that bit NotificationPreferencesCard first; documented inline.
Made-with: Cursor
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d717a7d3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (stateRef.current.status !== UNLOCKED) return; | ||
| vaultKeyRef.current = newVaultKey; |
There was a problem hiding this comment.
The commit callback exits unless the vault is still UNLOCKED, which breaks a real race: if the user locks the vault while /auth/change-password is in flight, the server accepts the rewrapped blob but this callback becomes a no-op. That leaves state.blob/etag pointing to the old envelope, and later unlock attempts use the cached locked snapshot (via load() short-circuit) with the new password, causing repeated decrypt failures until a full page reload. The callback should still refresh blob/etag when locked, while avoiding key reinstallation.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| setErrCode((err && err.code) || 'http_error'); | ||
| return; |
There was a problem hiding this comment.
When deleteAccount returns unauthorized (for example, session expiry mid-submit), this branch only stores an error and returns. Because /auth/* responses do not trigger the global auth-loss interceptor, AuthContext remains authenticated and the user can stay on private account UI with stale session state. Handle this case by calling handleAuthLost() (and redirecting) so client auth state matches the server.
Useful? React with 👍 / 👎.
P1 (VaultContext.rewrapForPasswordChange)
If the user locks the vault between POST /auth/change-password and
rewrap.commit(), the server has already accepted the new envelope
but the commit used to no-op. state.{blob,etag} stayed pointing at
the old wrap; load()'s cache short-circuit then served that stale
snapshot for every subsequent unlock attempt, so the new password
would fail decrypt until a full page reload.
- Capture sessionGen at the start of rewrapForPasswordChange so the
commit can cleanly distinguish "same session, vault locked
mid-flight" (refresh blob/etag only) from "different session
entirely" (e.g. logout: hard no-op, as before).
- When UNLOCKED mid-commit: install new vaultKey + update blob/etag
(existing happy path).
- When LOCKED mid-commit: update blob/etag only — vault key is
rederived from the new master on the next unlock anyway.
- Test: lock mid-flight, commit, then unlockWithMaster(newMaster)
succeeds WITHOUT an extra load() round-trip — proving the cache
short-circuit now serves the fresh ciphertext.
P2 (DeleteAccountCard — unauthorized)
deleteAccount returning `unauthorized` meant the server considered
the client unauthenticated, but we only stored an errCode and sat
there — user stranded on private account UI while AuthContext
still thought they were signed in. The /auth/* apiClient path does
NOT drive the global auth-loss interceptor, so this branch has to
handle it itself: call handleAuthLost() and bounce to '/'. Test
added.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9f98a07bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (err) { | ||
| setErrCode((err && err.code) || 'http_error'); | ||
| return; |
There was a problem hiding this comment.
When /auth/change-password returns 401 unauthorized (e.g., session expired in another tab), this branch only sets errCode and returns, so AuthContext stays authenticated and the user remains on private Account UI with stale session state. Because /auth/* calls are excluded from the global auth-loss interceptor, this path needs to explicitly trigger auth loss (or force a refresh that can transition to anonymous), otherwise the user gets stuck in an inconsistent signed-in state.
Useful? React with 👍 / 👎.
| .catch((err) => { | ||
| if (cancelled || !mountedRef.current) return; | ||
| setErrCode((err && err.code) || 'http_error'); |
There was a problem hiding this comment.
If /auth/prefs responds with 401 unauthorized during hydration or save, this code path only records an inline error and continues rendering the authenticated account screen. Since /auth/* responses do not trigger the shared auth-loss interceptor, failing to clear auth state here leaves the app believing the user is signed in after the server has rejected the session.
Useful? React with 👍 / 👎.
…odex round 2 P2)
Apply the same /auth/* → handleAuthLost pattern that PR 7 already
lands for DeleteAccountCard to the two companion cards. /auth/*
responses bypass the global auth-loss interceptor, so each component
has to explicitly flip AuthContext to ANONYMOUS when the server
reports the session as expired — otherwise the user stays rendered
on private Account UI with a stale signed-in state until they
navigate manually.
ChangePasswordCard
- Wire up handleAuthLost from useAuth.
- On `unauthorized` from authService.changePassword, call
handleAuthLost() and bail. PrivateRoute redirects to /login on
the next render.
- Non-auth failures (invalid_credentials, precondition_failed, etc.)
still surface the existing inline alert — the fix is targeted.
- Added ChangePasswordCard.test.js covering both branches. Scope
note at the top explains why we mock useVault + swap the whole
authService rather than spinning up a real VaultProvider: the
crypto/rewrap paths are already covered in authService.test.js
and VaultContext.test.js, this file only guards the 401 branch.
NotificationPreferencesCard
- Same useAuth wiring.
- Both network paths (hydration GET /auth/prefs and save PUT
/auth/prefs) now route 401 through handleAuthLost. Hydration's
"show the form anyway so the user isn't behind an infinite
spinner" recovery is deliberately skipped for the 401 case —
the whole page is about to unmount.
- New tests use a sibling AuthProbe to observe the AuthContext
status transition, which is the user-visible effect.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c536b098
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [needsHydration, authService, handleAuthLost]); |
There was a problem hiding this comment.
The fallback /auth/prefs hydration effect is keyed only by needsHydration, so it does not re-run when user.id changes but both identities have notificationPrefs == null. In that account-switch case, the card can keep the previous user’s in-memory toggle and submit that stale value for the new user. Please key this effect (and hydration reset) on identity, e.g. user.id, not just the boolean gate.
Useful? React with 👍 / 👎.
| const hasVault = | ||
| vault.isLocked || vault.isUnlocked || vault.isError; | ||
| if (hasVault && !vault.isUnlocked) { |
There was a problem hiding this comment.
hasVault only checks isLocked/isUnlocked/isError, so during initial vault loading it evaluates false and allows submission to proceed into the expensive KDF path before failing at rewrapForPasswordChange with vault_not_unlocked. That creates a misleading error and unnecessary PBKDF2 work for a normal “page still hydrating” case. Treat unresolved vault states as not-ready and short-circuit before derivation.
Useful? React with 👍 / 👎.
…te prefs per identity (Codex round 2) Frontend Codex PR 7 round-2 P2 findings addressed: src/components/ChangePasswordCard.js:141 — submit now short-circuits when vault is isIdle or isLoading, surfacing a "vault still loading" message instead of running the full double-PBKDF2 derivation only to fail inside rewrapForPasswordChange with the misleading vault_not_unlocked copy. Added a regression test that asserts deriveChangePasswordKeys, rewrapForPasswordChange, and changePassword are all uncalled when the vault is in LOADING. src/components/NotificationPreferencesCard.js:156 — fallback GET /auth/prefs effect is now keyed on user.id, not just a boolean gate. Previously, if user A hydrated via the fallback path (notificationPrefs == null) and the app then swapped to user B with notificationPrefs also null, no dep changed and the effect would not re-run — user A's in-memory toggle would persist and a Save could PUT that stale value under user B's credentials. On identity change we now reset hydrated/errCode/success, refetch, and mark lastHydratedUserIdRef so the effect settles per-user without looping. Added a regression test that swaps /auth/me between two user ids via AuthContext.refresh() and asserts getPrefs is called once per identity with the form reflecting each user's stored value. Made-with: Cursor
|
@codex review Round-2 P2 findings addressed in 3878b80:
Note for schema reviewers on the companion backend PR (sysnode-backend#7): sysnode-backend is pre-production, so forward migrations are not relevant — in-place edits to |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Follow-up security audit finding from current Several non-login password flows derive the raw 32-byte PBKDF2 Concrete spots on current head:
This is a client-memory hygiene issue rather than a server-custody issue, but on a design that deliberately keeps voting keys client-side, shortening the lifetime of password-derived master material matters. I’d recommend wrapping these flows in
|
2 participants
Summary
Wires three new Account-screen cards into sysnode-info and extends
authService+VaultContextwith the client-side crypto orchestration the backend expects.Backend companion: syscoin/sysnode-backend#7
What's in it
Change password
authService.deriveChangePasswordKeys(oldPassword, newPassword, email)separates key derivation from the API call so the card can orchestrate new-master → vault rewrap → submit withoutauthServicehaving to know about vault internals (same pattern as login).VaultContext.rewrapForPasswordChange(newMaster)wraps the existing envelope under the newly-derived vaultKey and returns{blob,etag}ready to POST; if no vault exists, returnsnullso the route treats it as a pure password change.ChangePasswordCardties it together: derive new master, rewrap vault, POST{oldAuthHash,newAuthHash,vault?}, surface409 vault_rewrap_requiredas a hard error rather than papering over it.Notification preferences
authService.getPrefs()/updatePrefs(prefs).NotificationPreferencesCardgates its form behind ahydratedflag so clicks cannot race with the initial/auth/meround-trip (previously the source of flaky double-toggles).Delete account (GDPR)
authService.deleteAccount({oldAuthHash}).DeleteAccountCardis a two-step destructive UX: collapsed "Delete account…" entry → expanded warning list + email-match + current-password re-proof. Email match is case-insensitive and trim-tolerant so "alice@EXAMPLE.com " confirms correctly.useAuth().handleAuthLost()(which tears downVaultContextvia itsisAuthenticatedwatcher) andhistory.replace('/').Test plan
DeleteAccountCard(collapsed/expanded states, email mismatch guard, case-insensitive + trim tolerance, empty password guard,invalid_credentialssurfaces without destroying local state, success navigates to/),NotificationPreferencesCard(hydration gating, toggle + submit),ChangePasswordCardorchestration,VaultContext.rewrapForPasswordChange,authService.deleteAccount./auth/prefs./, logged out, and re-registering the same email works.Notes for review
resetMocks: true(CRA default) wipesjest.fn(() => ...)implementations between tests, so the new component tests install thederiveLoginKeysstub inbeforeEachrather than inline in thejest.mockfactory — same pattern already used inProposalVoteModal.test.js. Without this the component tests silently fall through to real PBKDF2 (600k SHA-512 iterations) and blow pastwaitFor's default 1s window. Cut affected test runtime from ~10s each to ~30ms.DeleteAccountCarddeliberately usesfireEvent.submiton the form (andfireEvent.changeon inputs) instead ofuserEvent.click/userEvent.type. Under React 17 + jsdom,userEvent@13's synthetic pointer sequence can commit a click before a pending controlled-input state update has flushed, running the handler on stale state (this bitNotificationPreferencesCardfirst and is documented inline in both test files).Made with Cursor