mirror of
https://github.com/bretzel-app/crumbs.git
synced 2026-10-12 12:06:57 +02:00
main
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Frederic Cabassut
|
e4606cfc70 |
feat(auth): enable password login on OAuth-only accounts, with disable control
Admins can now reset the password of an OAuth-only user, which creates their credentials and enables password login; the new 'Disable password login' row action removes those credentials again. Resetting a password revokes all existing sessions. - add users.password_login_enabled column (migration 0005) - admin reset: set password, enable login, revoke sessions - disable action: clear password, disable login (only on OAuth-linked rows) - account endpoint: refuse password updates on OAuth-linked users - e2e: wipe the test DB before the webServer starts (Playwright starts the server before globalSetup, so wiping there orphaned the DB file) - row-actions strip test: five square actions, wrap on phone widths |
||
|
Frederic CabassutandClaude Opus 5
|
afce024cac |
refactor(wu-003b): one uniform icon per row action, and guard the reveal
Follow-up to d5c7ab6 after visual review. The 44px Reset button sat
beside ~24px siblings, and four text labels wrapped — Delete dropped to
its own line at 900px, three lines at 360px.
All four row actions are now identical 44x44 icon buttons on one line:
shield-check/shield-off, key-round, log-out, trash-2. Each carries a
tooltip for a mouse and an aria-label, since an icon alone has no
accessible name. In-flight swaps to a spinning loader-circle and an
"Resetting…" accessible name, so the e2e assertion reads the accessible
name rather than visible text that no longer exists.
`flex-wrap` on the strip is what keeps the squares square: flex
compresses items only on a line that overflows, so breaking the line
means no button is ever asked to shrink. Without it the four flatten to
40x44 at 320px — under the minimum, on a row whose Delete is
irreversible. They hold one line down to 336px, exactly 4x44 plus the
gaps, and take a second line below that. Correctly sized targets over
two lines beat cramped ones on one.
A first attempt also put `shrink-0` on each button and credited it in a
comment. Measurement disproved that: strip the token and the geometry is
byte-identical at every width from 280 to 900, because `flex-wrap` has
already spared the buttons from ever shrinking. The dead tokens are gone
and the comment now names the real mechanism, while noting that the
container's identical-looking `shrink-0` DOES carry weight — from 640px
the row turns sm:flex-row, where the truncating name block would
otherwise squeeze the strip.
Two things stay deliberately as they were. Copy and Dismiss remain text
buttons: Dismiss irreversibly destroys a credential the server may
already hold, and an icon-only cross beside Copy is the one place
ambiguity could cost a password. The blocked-reset reason remains visible
in-row text rather than moving into the tooltip, which needs hover the
reporter's Android device cannot give.
Also closes a gap in the reveal's regression cover. The rule that an
unconfirmed outcome REVEALS the generated password protects the only copy
of a credential the server may already hold, but only the network-abort
trigger was tested. Two more scenarios now pin the lines that decide it:
a 5xx carrying a parseable message (guarding `res.status < 500`) and a
4xx with an unreadable body (guarding `.json().catch()`), the latter
being what a reverse proxy returns as an HTML error page.
DoD items verified:
- [x] 1-3. four identical >=44px icon buttons, no wrap at 360px+,
per-icon subpath imports at 16-20px
- [x] 4-5. Copy/Dismiss still text; reason still visible in-row text
- [x] 6. spinner plus "Resetting…" accessible name, aria-disabled
- [x] 7-9. no disabled attribute, focusable, no request when
unavailable; Dismiss focus return; all 10 data-testids kept
- [x] 10. CSS variables only, no hover-only visibility
- [x] 11-12. 5xx-with-reason and unparseable-body both reveal
- [x] 13. distinct victim emails, no new failed logins (suite: 3 of 5)
- [x] 14. no regression in the 22 properties of d5c7ab6
Verified independently by the orchestrator: pnpm check 0 errors /
14 pre-existing warnings; pnpm test:unit 306 passed / 26 files;
admin-users.spec.ts 31 passed; full e2e suite 129 passed. Removing
flex-wrap fails the new scenario with "Expected: >= 44, Received: 40",
reproduced here, so the guard covers the token that actually matters.
Reviewed-by: adversarial-review (PASS on the icon work; a second
reviewer failed the first attempt for crediting shrink-0 with an effect
mutation testing showed it does not have)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RbqJcFofZSbhbGwUwZP3wU
|
||
|
Frederic CabassutandClaude Opus 5
|
ffde151b75 |
feat(wu-003): add admin reset-password control to Settings > Users
Closes the UI gap in issue #77. The endpoint, resetPassword() and the notification email have all existed since the feature was built; only the control was missing, which is why the docs promised something the screen did not offer. The control renders on EVERY row, the admin's own included. It is enabled only for accounts that sign in with a password and that are not the signed-in admin; otherwise it is presented as unavailable WITH A VISIBLE REASON. A control that simply vanishes is the confusion this replaces, so silence was not an option. No new control uses the `disabled` attribute. Unavailable is expressed with aria-disabled, the element stays focusable, and the handler refuses activation itself — a disabled button leaves the tab order, so a keyboard or TalkBack user loses the element they just activated, and a title tooltip explains nothing on a touch screen. The reporter is on Android. Two rules protect the credential, and both are enforced by the shape of the state rather than by convention: - An unconfirmed outcome REVEALS the password. If the request left the device and no answer came back, the server may well have stored it, making the revealed string the only copy in existence. Discarding it could leave an account holding a credential nobody knows. - `password` and `message` are independent fields, so an error has nowhere to write that would replace a reveal. Revealed rows are also marked unavailable, so a second reset cannot be issued until Dismiss. Offline is a definite state, not a lost response: the service worker caches only GET /api/*, so a PATCH never leaves the browser. It is checked before anything is generated, and reported as the certainty it is. The live region is rendered empty on first paint and only its text is replaced, because a region inserted together with its content is not reliably announced. It announces that a password appeared and where — never the secret itself, which a polite region would speak aloud once, unrepeatably, to whoever is in earshot. DoD items verified (22): - [x] 1-2. control on every row; enabled iff eligible and not self, else aria-disabled with a visible, aria-describedby reason - [x] 3-4. PATCHes newPassword; "Resetting…" and no double-submit - [x] 5-6. revealed as selectable text, announced; stays unavailable - [x] 7. Copy confirms success; failure offers a manual-copy hint - [x] 8-10. 4xx reveals nothing; offline definite; unknown REVEALS - [x] 11-12. errors never replace a reveal; both announced - [x] 13-14. 404 drops the row and its plaintext; rows isolated - [x] 15-17. Dismiss clears and restores focus; 44px targets; activating an aria-disabled control fires no request - [x] 18-19. victim login from a separate context, old password 401 last; plaintext in no storage, no IndexedDB, no console - [x] 20-22. CSS variables only, no hover-only; untruncated at 360px; declarative Gherkin with distinct victim emails Verified independently by the orchestrator: pnpm check 0 errors / 14 pre-existing warnings; pnpm test:unit 306 passed / 26 files; admin-users.spec.ts 28 passed; full e2e suite 126 passed. The reviewer additionally mutation-tested the security core, each mutation rebuilt and caught with clean attribution: revealing null on an unconfirmed outcome, forcing the offline pre-flight false (which falls into the unknown path and would reveal a password that never left the browser), dropping the state merge in patchReset, swapping aria-disabled for disabled (which breaks focus return), and deleting the activation guard — the last proving the force-click assertions test the component refusing rather than Playwright declining to click. Reviewed-by: adversarial-review (PASS) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbqJcFofZSbhbGwUwZP3wU |
||
|
Frederic CabassutandClaude Opus 5
|
a6f86ec1f0 |
fix(wu-002): refuse password resets that could never work
Second slice of issue #77. PATCH /api/admin/users/[id] accepted { newPassword } for any account and returned 200, but resetPassword() writes only passwordHash while verifyPassword() matches only rows whose authProvider is 'password'. So for an OAuth-only account (the "OAuth-only" checkbox stores 'none') or anyone who has signed in via OAuth (authProvider becomes the provider name), the endpoint wrote a hash that could never be used to sign in, returned success, and fired a "your password was reset" email. It lied. The endpoint already guards both other self-affecting operations server-side — "Cannot change your own role" and "Cannot delete your own account" — but a newPassword targeting the requesting admin succeeded. Two early returns in the newPassword branch, ordered length -> self -> eligibility so the existing length 400 keeps precedence. Both are placed before the resetPassword() call and before the email block: that ordering is what guarantees no unusable hash is written and no misleading email is sent, and it is a property of control flow rather than of an assertion, because neither is observable from a test. passwordHash never leaves the server (toUser omits it) and isEmailConfigured() is false under e2e, so an assertion either way would pass for a broken implementation too. Eligibility comes from canResetPassword() rather than an inline comparison, so the endpoint and the settings UI cannot drift; the 400 message comes from passwordResetBlockedReason() so the API reason and the UI explanation are the same string. DoD items verified: - [x] 1. eligible user other than self still 200s, new password works - [x] 2. authProvider 'none' returns 400 naming the reason - [x] 3. positive control: an eligible user CAN log in after a reset - [x] 4. targeting own id returns 400 naming the reason - [x] 5. sub-8-char password still 400s for length, beating eligibility - [x] 6. both guards precede the write and the email (diff-verified) - [x] 7. role, revokeSessions and DELETE byte-unchanged (15+/0-) - [x] 8. eligibility imported from wu-001, not re-implemented - [x] 9. declarative Given/When/Then, distinct victim emails throughout Verified independently by the orchestrator: pnpm check 0 errors / 14 pre-existing warnings; pnpm test:unit 306 passed / 26 files; pnpm build && pnpm test:e2e admin-users.spec.ts 14 passed (7 auth-setup + 3 pre-existing + 4 new). The reviewer additionally reverted the endpoint to its pre-change form and confirmed both guard scenarios fail with "Expected: 400, Received: 200", so they are genuine regression tests rather than assertions of behaviour that already held. Reviewed-by: adversarial-review (PASS) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbqJcFofZSbhbGwUwZP3wU |
||
|
Claude
|
ea403dcbee |
test: upload a real attachment in the user-deletion coverage test
The "notes, tags, and attachments" scenario previously only created a note and an API key — no attachment. Upload a PNG via the browser context so Origin is set correctly for SvelteKit's CSRF check, giving the atomic-delete path genuine attachment rows to clean up. https://claude.ai/code/session_015T2HE6VzB64ZmSSyy1Wz4R |
||
|
Claude
|
5719fc3459 |
fix: make admin user deletion atomic and surface errors in UI
deleteUser() ran each child-table cleanup as a separate statement, so a foreign-key violation on any step (e.g. sync_log rows from other users referencing the deleted user's notes) would abort mid-way and leave the database in a partial state. The API swallowed the error and the admin page silently ignored the non-ok response, so clicking Delete and confirming appeared to do nothing. - Wrap the full cascade in a single sqlite transaction so the deletion is atomic. - Also remove rows in sync_log, note_user_state, note_collaborators, note_versions, and attachments that belong to other users but reference the deleted user's notes, so the final DELETE FROM notes cannot trip a foreign-key check. - Surface delete failures in the users settings UI instead of silently swallowing them. - Add data-testids on the user row and delete button to support tests. - Add e2e coverage for the admin user deletion flow (happy path, cancelled confirmation, and a user with notes / tags / attachments / preferences / api keys). https://claude.ai/code/session_015T2HE6VzB64ZmSSyy1Wz4R |