feat(account): let a signed-in user manage their own account - #131
feat(account): let a signed-in user manage their own account#131MOHITKOURAV01 wants to merge 1 commit into
Conversation
Sixteen routes on /api/user and not one of them read or wrote the account: no /me, no /profile, no /change-password. The Settings dropdown held a theme toggle. The only way to set a new password was Log Out → Forgot password? → check email. That is not only a missing screen. The signed-in user has already proved something the server can check — they hold a valid token for this account. The reset flow makes them prove something it cannot, that they can read a mailbox, which is outside the application, which udaycodespace#95 had to harden against strangers, and which anyone with mailbox access can drive too. It also sets isVerified and discards any pending OTP, side effects a routine rotation has no business causing. And nothing was recorded: the activity log's action enum was login, logout, login_failed. - GET /api/user/account returns what is stored, through an allow-list rather than a list of fields to strip, so it cannot leak a field nobody has thought about yet. - PUT /api/user/account changes the display name and only that. Not email — that is the account's unique identity since udaycodespace#72 and moving it needs proof against the new mailbox. Not type — that is the field udaycodespace#55 closed. - POST /api/user/change-password requires the current password. It clears any reset code in flight and deliberately leaves isVerified and the OTP alone, because it proves nothing about the mailbox and must not claim to. The response says plainly that other sessions stay signed in: the token is stateless and there is nothing to revoke. - password_changed, password_change_failed and profile_updated added to the schema enum, the listing filter and the admin dropdown. A test keeps the storable set and the filterable set identical. - An Account panel for every role, with the rules mirrored in lib/account.js and asserted on both sides. Rate limited like the other credential endpoints, but with no failure throttle: that locks an email address, and locking someone out of the app for mistyping their current password twice is worse than what it prevents. The configured admin has no users row, so its id would cast and throw on findById. It reads as editable: false and both writes answer 403. backend 526 pass (505 before), frontend 230 pass (214 before). Closes udaycodespace#126
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic Password | 89d4edd | frontend/src/lib/account.test.js | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
One note for whoever merges this alongside #130. This PR defines the reserved-admin check locally, in const ADMIN_ID = "admin";
const isAdminId = (id) => String(id ?? "") === ADMIN_ID;It has to, to stand alone against #130 introduces |
1 participant
Closes #126.
The defect
Sixteen routes on
/api/user, and not one of them read or wrote the account. No/me, no/profile, no/change-password. The ⚙️ Settings dropdown held onecontrol, a theme toggle, and
grep -rn "profile" frontend/srcreturned nothing.The only way to set a new password was Log Out → Forgot password? → check email.
Why that is a security problem and not only a missing screen
Rotating a password required giving up the session and using a weaker path.
The signed-in user has already proved something the application can check: they
hold a valid token for this account. Sending them to
/forgot-passwordmakesthem prove something it cannot — that they can read a mailbox. That channel is
outside the application, #95 had to harden it precisely because it answers
uniformly to strangers, and anybody with mailbox access can drive it too.
The recovery path has side effects a rotation should not cause.
Marking the address verified is correct for a reset — holding the emailed code
is proof of the mailbox — but it is a state change a routine password change
has no business making, and it silently discards a pending verification code.
A password change left no trace. The
actionenum was["login", "logout", "login_failed"], so the log could not answer "was thisaccount's password changed, and from where" — the successor to the question #87
added
login_failedfor.Nothing else could be corrected. A name typed wrong at registration was
permanent, and it is read by the navbar, the certificate and every review
byline.
What is here
GET /api/user/account— what the application stores about you. There wasno way for anyone to see it.
toAccountViewis an explicit allow-list ratherthan a list of fields to strip:
adminControllerprojects with-password -otp -resetToken …, which is correct but has to be extended everytime a sensitive field is added. An allow-list cannot leak a field nobody
thought about. There is a test that hands it a field nobody has declared yet.
PUT /api/user/account— the display name, and only that. Deliberately notemail: the address is the account's identity, unique-indexed since #72, andmoving it has to be proved against the new mailbox first, which is a
verification flow of its own rather than a text input. Deliberately not
type:that is the field #55 closed. The updated account comes back so the client can
replace the stored session user instead of rendering a stale name until the next
sign-in.
POST /api/user/change-password— requires the current password, and thatrequirement is the point of the endpoint: possession of an unattended tab must
not be enough to lock the owner out of their own account.
It clears any reset code in flight, so one somebody else requested does not
still work afterwards. It deliberately does not touch
isVerifiedor theOTP fields — the reset flow sets them because holding the emailed code proves
the mailbox, and this route proves nothing about the mailbox, so it must not
claim to. The success message says plainly that sessions signed in elsewhere
stay signed in until their token expires; the token is stateless and there is
nothing to revoke, and anything else would be a promise the application cannot
keep.
Three new activity log actions —
password_changed,password_change_failed,profile_updated— in the schema enum, the listingfilter's allow-list and the admin dropdown and badge labels.
password_change_failedis the one that matters most: a run of them against asigned-in account is somebody working on a session they should not have. A test
asserts the storable set and the filterable set are identical, so an action the
log can write but the filter refuses cannot appear again.
The Account panel at
/dashboard?panel=account, listed for every role.lib/account.jsmirrors the server's rules — the same table asserted on bothsides, the pattern #114 established. The confirmation field exists only in the
browser: the server takes one new password and has nothing to compare a second
field against.
Rate limiting
/change-passwordgets the same per-client rate limiter every credentialendpoint has: it is behind a valid token, which is the bound that matters, but
it also takes a password and tells you whether the guess was right.
It gets no failure throttle. That mechanism locks an email address, and
locking the owner of a live session out of the whole application because they
mistyped their current password twice is worse than the thing it prevents. The
failures go to the activity log instead.
The configured admin
authMiddlewarerecognises the reserved id"admin"without a lookup — it is acredential pair in the environment, not a
usersrow. Every route here wouldotherwise hand
"admin"tofindById, which casts it to an ObjectId andthrows. It reads its account from what the middleware already knows, marked
editable: false, and both writes answer 403 saying where those credentialsactually live.
Tests
backend/tests/account-management.test.js— 21 tests with injected models, sono database. An update cannot reach
type,emailorisVerified; the wrongcurrent password changes nothing and is recorded; a change clears the reset
credential but leaves
isVerifiedand the OTP alone; and the admin branch isasserted with a
Userstub whosefindByIdthrows, so the test fails if thelookup is ever reached.
frontend/src/lib/account.test.js— 16 tests, including a seven-row table ofpassword cases the server asserts too.
frontend/src/lib/dashboardPanels.test.js— the existing expectations updatedfor the new panel, plus tests that every real role may open it and an unreadable
role falls back to home.
docs/issue-126-account-panel.mdhas the write-up.Checklist
cd backend && npm test— 526 pass (505 onmain, 21 added)cd frontend && npm test— 230 pass (214 onmain, 16 added)cd frontend && npm run buildnpm run lint— does not pass onmain(69 problems) and does not here.ActivityLogs.jsxandDashboard.jsxreport the same 1 each before andafter, a pre-existing unused
Reactimport.lib/account.jsandAccountPanel.jsxlint clean.Notes
styles/account.cssis its own file rather than another block appended totheme.css, for the reasontheme.css's own header gives: several brancheseach appending to one shared file conflicts on every pair of them.
The emailed reset flow is unchanged, for the case it was built for — somebody
who cannot sign in.