**COLLECTING BRANCH** Account management: passkeys, password change, account deletion #68
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Reference
marvin8/yunjin!68
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-67-account-management"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Effort 3 of Phase 6 multi-user (final effort before the collecting branch lands on
main). Implements the approved account-management design: CLI-only account creation (yunjin create-user, no admin role), self-service password change (current-password gate; current session survives, others revoked), passkey enrollment + username-less login viawebauthn3.0.1 (additive to passwords, ~12 h passkey sessions vs 30-day password sessions), credential management on a new Account card, and self-service account deletion with data cleanup (FK cascades + orphan-feed GC; last-user guard in web, CLIdelete-userdeliberately unrestricted). One Alembic migration (20261004_webauthn_credentials). Passkey features gate on new env varsYUNJIN_RP_ID+YUNJIN_ORIGIN; documented indocs/config.md,docs/cli.md, newdocs/passkeys.md(HTTPS-on-stable-domain prerequisite).Also closes a latent setup-gate gap this branch made reachable:
POST /setupnow enforces the recorded per-request gate (redirect once any user is credentialed), and_ensure_default_userno longer resurrects a passwordlessdefaultwhile other users survive.Base:
feat/multiuser(collecting branch).Refs #25
Refs #67
Test suite: 622 passed (617 + fix-wave tests); full nox green on CI.
WuMing
Found 2 issue(s). See inline comments below.
@ -9,0 +10,4 @@[[allowlists]]description = "Canned WebAuthn test vectors copied verbatim from the public webauthn 3.0.1 sdist test suite; not secrets"paths = ['''(?:^|/)tests/webauthn_fixtures\.py$''',config [MEDIUM]
This allowlist suppresses ALL gitleaks rules for the entire tests/webauthn_fixtures.py file, not just the canned WebAuthn vectors. Any genuine credential later added to (or accidentally pasted into) that file will never be detected, silently widening the secret-scanning blind spot. Prefer scoping the exception to the known values instead of the whole path, e.g. use
regexes = [...]that match the specific fixture keys/vectors (optionally withregexTarget = "match"), or mark only those lines in the test file with#gitleaks:allow.@ -0,0 +53,4 @@" VALUES (1, X'00', X'00', 0, 'Passkey again')")raised = Falseexcept Exception:tests [MEDIUM]
This uniqueness test wraps the second INSERT in
try/except Exceptionand asserts a boolean flag, so it passes if the statement fails for ANY reason (missing column, FK problem, wrong table, etc.), not just the intendedcredential_idUNIQUE constraint. It also reports nothing about which constraint fired, making future regressions hard to diagnose. Replace the flag pattern with a specific expectation, e.g.with pytest.raises(sqlite3.IntegrityError): conn.execute(...), so only the expected violation satisfies the test..gitleaks.toml✅ Fixed in commit
bbc03b6— the allowlist no longer usespathsat all. It is now a single line-exactregexesentry (regexTarget = "line") matching only the one public sdist vector line that actually trips the scanner, so any other finding in that file is still detected. Verified empirically with the gitleaks 8.30.1 binary: fixture-only scan → no leaks; planted GitHub PAT in a copy of the same file → 1 finding (detected); same file with an empty config → 2 findings (both would trip without the allowlist). An inline#gitleaks:allowwas not possible: the flagged line lives inside a JSON string constant that tests compare byte-for-byte.tests/test_migration_webauthn.py✅ Fixed in commit
f05ef6a— the flag pattern is replaced withwith pytest.raises(sqlite3.IntegrityError):so only the intended UNIQUE-constraint violation satisfies the test.WuMing
Found 1 issue(s). See inline comments below.
@ -54,0 +86,4 @@```Answering anything but `y` or `yes` aborts with no changes. A deletion removesthe account's subscriptions, aggregates, read state, settings, and passkeys;docs [LOW]
Inconsistent terminology between the two docs describing the same deletion.
delete-usersays it removes the account's "subscriptions, aggregates, read state, settings, and passkeys", while the new docs/passkeys.md (line 91) describes the same web-UI deletion as removing "subscriptions, groups, read state, settings, and passkeys". If "aggregates" and "groups" are the same concept, pick one term and use it in both places; if they are different concepts, each list is incomplete and should be reconciled so readers know exactly what data is removed.docs/passkeys.mdline 91✅ Fixed in commit
f94e6ca— passkeys.md now says "aggregates", matching docs/cli.md. ("Groups" was stale wording; the domain concept is aggregates throughout.)WuMing
Found 4 issue(s). See inline comments below.
@ -9,0 +9,4 @@[[allowlists]]description = "Canned WebAuthn assertion vector copied verbatim from the public webauthn 3.0.1 sdist test suite; not a secret. Line-exact: any other finding in this file still trips."regexTarget = "line"config [MEDIUM]
This second allowlist entry has no
pathsrestriction and its regex is not anchored, so it is not "line-exact" as the description claims: gitleaks applies allowlistregexesas unanchored substring matches against the target, and an entry withoutpathsapplies repo-wide. A real secret that happens to sit on a line containing this substring (or a different file that ever embeds this vector) would be silently suppressed. Scope the exception to where the vendored test vector lives and anchor the pattern, e.g. addpaths = ['''tests/data/webauthn_assertion.json''']and use^\s*'''"authenticatorData": "SZYN...Tg",'''$so any other finding on that line still trips.@ -31,1 +31,4 @@keyboard shortcuts, next-unread navigation- **Passkeys** — optional WebAuthn sign-in with self-service enrollment andcredential management (passwords remain the fallback)- **Account management** — password change, self-service account deletion,docs [LOW]
This bullet advertises self-service password change, but no documentation in this diff explains it. The only page describing the Settings → Account card (docs/passkeys.md, lines 54–95) covers passkey enrollment and account deletion, and docs/cli.md only covers
create-user/delete-user; nowhere is it stated how to change a password, that the current password is required, or that the current session survives while other sessions are revoked. Add a short "Changing your password" section where the Account card is documented and link this bullet to it.@ -53,1 +53,4 @@### `create-user`Create a new account with a password.docs [LOW]
The section never states that this command is the only way to create an account. Since there is no web sign-up and no admin role, an administrator reading the docs may otherwise hunt for a registration page or an
add-user-style flow in the UI. Add one sentence, e.g. "Accounts can only be created from the command line; the web interface has no sign-up page and there is no admin role."@ -0,0 +85,4 @@deleting a passkey in Yunjin does not remove the credential from the device'spasskey provider; remove it there too if you no longer want it on the device.## Account deletiondocs [MEDIUM]
Account deletion is not a passkey feature, yet it is documented only on this page. The README advertises "self-service account deletion" as a headline capability (README line 34), and docs/cli.md documents the matching
delete-usercommand, so a reader looking for the web deletion flow will never think to look under "Passkeys". Move this section to a general account/settings page (e.g. a new docs/account.md, or the settings section of docs/reader.md), and cross-link it from the README bullet and from docs/cli.md'sdelete-usersection (which currently does not link back)..gitleaks.toml✅ Partially — commit
e1a8fb2anchors the pattern (^\s*...$), making it literally line-exact. The repo-wide claim, though, was already disproven empirically during wave 1 (comment 4271): a GitHub PAT planted in the same file is detected (1 finding) becauseregexTarget = "line"evaluates the allowlist regex per line — the same-file negative control and the suggestedtests/data/webauthn_assertion.jsonlayout (which does not exist in this repo) were re-checked before anchoring. Notepaths+regexesin one entry was the wave-1 bug: gitleaks ORs the conditions within a single[[allowlists]]block, so a path-scoped entry re-widens the exemption to the whole file.docs/passkeys.mdline 91🔴 Declined for now — restructuring the docs (new
docs/account.md, cross-links from README/cli.md) is an information-architecture decision for Marvin8, not a review-wave fix. The current placement is deliberate: the approved implementation plan pins the deletion documentation insidedocs/passkeys.md(Task 9's content list), alongside the passkey-management and password-change sections of the same Settings → Account card. Raised with Marvin8; happy to do the move if he wants it.README.md✅ Fixed in commit
e1a8fb2— passkeys.md (the page documenting the Settings → Account card) gains a "Changing your password" section: current password required, current session survives, other sessions revoked, andyunjin set-passwordas the lost-password path, cross-linked to the CLI docs.docs/cli.md✅ Fixed in commit
e1a8fb2— thecreate-usersection now states accounts can only be created from the command line: the web interface has no sign-up page and there is no admin role.docs/passkeys.mdline 91✅ Done in commit
ab3edcd(Marvin8 approved the restructure): new docs/account.md — Account management, holding password change and account deletion, cross-linked from the passkeys page intro and footer, nav (after Passkeys), README's Account-management bullet, anddocs/cli.md'sdelete-usersection.WuMing
Found 3 issue(s). See inline comments below.
@ -0,0 +1,24 @@# Account managementYour account is managed from **Settings → Account** in the web interface:that card holds password change, passkey enrollment and management (seedocs [LOW]
This sentence presents passkey enrollment and management as permanently available on the Account card, but per docs/passkeys.md the Passkeys card only appears when passkey support is enabled (both YUNJIN_RP_ID and YUNJIN_ORIGIN set). Add a qualifier, e.g. 'passkey enrollment and management (only when passkey support is enabled — see Passkeys)'.
@ -0,0 +10,4 @@Open **Settings → Account**, enter your current password and the new passwordtwice, and save. Your current session stays signed in; every other signed-indevice is signed out. If you have lost your password, an administrator on thedocs [LOW]
Line 6 of this same page states 'there is no admin role', so referring to 'an administrator on the server' here is inconsistent and may confuse readers looking for an admin UI. Reword to something role-free, e.g. 'anyone with access to the server's command line can reset it with
yunjin set-password'.@ -53,1 +53,4 @@### `create-user`Create a new account with a password. This is the only way to create andocs [LOW]
This claim conflicts with the rest of the page: the web interface auto-seeds a passwordless
defaultaccount on first start (see theset-passwordsection above and thedelete-usernote at line 96, which says a fresh start 're-seeds the passwordlessdefaultaccount'). Qualify it, e.g. 'This is the only way to explicitly create an account (the app also seeds a passwordlessdefaultaccount on first run)'.docs/account.md✅ Fixed in commit
f7fba96— the intro now reads "— when passkey support is enabled (see Passkeys) — passkey enrollment and management".docs/account.md✅ Fixed in commit
f7fba96— reworded to "whoever operates the server can reset it withyunjin set-password".docs/cli.md✅ Fixed in commit
f7fba96— the sentence now reads "This is the only way to explicitly create an account (the app also seeds a passwordlessdefaultaccount on first run)", keeping the no-sign-up/no-admin-role clause.WuMing
Found 1 issue(s). See inline comments below.
@ -0,0 +60,4 @@# Flask rejects any session cookie older than PERMANENT_SESSION_LIFETIME# (open_session max_age), so a literal 30-day offset needs a ceiling above# it; only the guard's cap mechanism is under test here.app.config["PERMANENT_SESSION_LIFETIME"] = timedelta(days=60)tests [LOW]
This test mutates the config of the
appfixture (PERMANENT_SESSION_LIFETIME) partway through and never restores it. If theappfixture is broader than function scope, the 60-day lifetime leaks into other tests in the module; even with a function-scoped fixture the test silently depends on that scope for correctness. Prefermonkeypatch.setitem(app.config, "PERMANENT_SESSION_LIFETIME", timedelta(days=60))(or snapshot/restore in a finally block) so the override is guaranteed to be undone.tests/test_session_lifetime.py✅ Fixed in commit
d631863— the override now usesmonkeypatch.setitem(app.config, "PERMANENT_SESSION_LIFETIME", timedelta(days=60)), so it is guaranteed to be restored regardless of fixture scope. (Theappfixture is function-scoped today, so nothing was leaking — this makes the test independent of that scope.)