Repository navigation
Conversation
On the default storage the backend belongs to the profile (users/<id>/storage.db), and every credential change rebinds the process to the next profile's database. Two paths wrote to the wrong one: - set_credential stored the app-session profile before the rebind, so the session landed in the pre-login database (users/local) and read back as absent from the user's own: get_session_token returned null and auth.get_state reported signed out right after a sign-in. - A session-only sign-out then found no session in the user's database (removedSession false), skipped the carry-forward, and rebound to users/local, where the API key was not: the key stopped working (tinyhumansai#8366), also after a restart. Rebind before storing the session. On a sign-out, take the API key out of its source while that database is still bound, then carry it into the signed-out profile after the teardown, putting it back if the teardown or the carry fails. This replaces the shared-store special case, which assumed one record for every profile. The key stays with the device's signed-out state, as tinyhumansai#6318 kept it; another account signing in does not get it. Regression tests on the default per-user layout (the existing storage tests install one backend with no URL, which is never rebound).
Tiny Sweeper reviewReviewed head: FindingsNo active actionable findings. Advisory notes
Verdict: Ready for maintainer review · Priority: none Diagnostics and supporting detailsFull summaryPull request purpose (title): fix(credentials): keep sessions and the API key on per-user storage. Fixes credential writes that landed in the wrong per-user storage database on the default SQLite backend. In `set_credential`, the storage rebind now runs before the session write (with the old rollback removed) so the session lands in the user's own `users/<id>/storage.db`; a failed rebind stores nothing, preserving the retry invariant. In `clear_credential`, the source API key is cleared while the source store is still installed, the teardown rebinds, the key is carried into the signed-out profile, and a new `restore_api_key` helper puts the key back if teardown or carry fails; the carry no longer depends on `removed_session` and the `shares_store` special case is dropped. A new storage-sqlite-gated test module covers the sign-in read-back, session-only sign-out persistence across a store reopen, and the multi-user isolation path. The tests lane notes the new failure-recovery branches are not exercised by any test. Review snapshot
Completeness: Complete What changedCredential operations on per-user storage are reordered: `set_credential` rebinds storage to the target profile before storing the app session, and `clear_credential` clears the source API key before teardown, carries it into the signed-out profile after teardown regardless of whether a session profile was removed, and restores the key on teardown/carry failure via a new `restore_api_key` helper. The prior `shares_store` special case and the set_credential rollback are removed. Tests: a new `ops_credential_per_user_storage_tests.rs` module (wired behind the `storage-sqlite` feature in `ops_tests.rs`) installs the default backend from its URL so rebinds really switch databases, exercising sign-in storage placement, session-only sign-out key survival across a reopen, and second-user isolation. Features
Tests
Before mergeNone. Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Credential installation and clearing crates/openhuman-core/src/security/credentials/ops/credential.rs |
set_credential rebinds process globals before storing credentials. clear_credential clears the source API key before teardown, reloads the signed-out configuration, and attempts key restoration if teardown or carrying fails. |
Per-user storage tests crates/openhuman-core/src/security/credentials/ops_credential_per_user_storage_tests.rs, crates/openhuman-core/src/security/credentials/ops_tests.rs |
SQLite-feature tests cover per-user session storage and API-key behavior after sign-out, storage reopening, and sign-in as the same or a different user. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related PRs
- tinyhumansai/openhuman#8366: Changes the session-only sign-out path for preserving an API key, which overlaps with this PR’s changes to
clear_credential.
Suggested reviewers: senamakel, tinysweeper
Merge Risk: 
71138
-
Credential installation is generally mergeable, but a persistence failure can leave the wrong user workspace active until the scope is corrected.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preserving sessions and API keys in per-user storage. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 12 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the keys at night,
And watches profiles settle right.
The session rests, the keys remain,
Then tests sign in and out again.
Small paws applaud the SQLite trail.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
Follow-up to tinyhumansai#8428. clear_credential takes the API key out of its source before the session teardown, and puts it back if the teardown or the carry into the signed-out workspace fails. A reload_config_or failure between the teardown and the carry returned with the key removed. All three steps now restore it the same way (inspect_err). clear_credential delegates to clear_credential_with, which takes the post-teardown reload as a closure (production passes reload_config_or), so tests can fail it: a failed reload and a failed carry each return the error with the key back at its source. restore_api_key is also tested directly.
Summary
users/<id>/storage.db, a sign-in stored the session in the pre-login database. Right after signing in,auth.get_session_tokenread null andauth.get_statereported signed out.auth.get_statereported signed out, including after a restart. This is release-blocking.Problem
Found by the storage scenarios harness (#8426, scenarios A and J) and confirmed on disk.
profiles/persistence.rs→storage::secrets::current()), and the default backend belongs to the profile. Every credential change rebinds the process to the next profile's database (ops/user_scope.rsrebind_after_credential_change→storage::rebind).set_credential(security/credentials/ops/credential.rs) calledstore_provider_token(app-session)beforerebind_after_credential_change. So the session was written tousers/local/storage.dbwhile the user's ownusers/<id>/storage.dbstayed empty.users/local/storage.dbheld the secret<user>:auth:app-session:default. The user's database held only their provider credential.clear_session_credentialremovesapp-sessionfrom the user's database, where it isn't, so it returnsremoved = false. Inclear_credential, the carry-forward of the API key (refactor(auth): the core takes a credential; hosts own login and the backend session #6318) only runsif removed_session, so it was skipped. Meanwhile the teardown rebinds tousers/local, where the key isn't.auth_clear_credential {kind: "session"}→{removed: false, removedSession: false, removedApiKey: false}, thenauth_get_state→isAuthenticated: false, also after a restart.shares_storespecial case added for fix(credentials): keep the API key on sign-out when profiles live in storage #8366 assumed one record for every profile. With per-user databases, even a carried key would have left a stale copy in the user's database.storage::rebind). So none of them could see a record written to the wrong database.Solution
set_credentialrebinds first, then stores the session, so the session lands in the user's own database. A failed rebind stores nothing, so a retry takes the install path again rather than the refresh fast-path that skips the bind. That's the invariant the old rollback protected.clear_credentialremoves the API key from its source while that database is still installed, runs the teardown, then carries the key into the signed-out profile.restore_api_key, logged), so a failed sign-out does not lose it.removed_session. The teardown rebinds to the signed-out workspace either way.Semantics, matching #6318 and v0.64.15
users/local), and survives a restart.kind) still removes the key everywhere, unchanged.Tests
New
ops_credential_per_user_storage_tests.rs(storage-sqlite, so part of the product test set). It installs the profile's default backend from its URL, as the desktop host does, so every sign-in and sign-out really rebinds between two databases:a_session_is_stored_in_the_users_own_database: the token reads back exactly, andauth.get_stateis a session for that user.a_session_sign_out_keeps_the_api_key_on_per_user_storage:removedSession: true; the key is in effect from the signed-out profile, also after the store is reopened from its URL; and signing back in finds no stale key in the user's database.another_user_signing_in_does_not_get_the_first_users_api_key: the negative multi-user check.Red/green on cuttlefish: nextest,
-p openhuman --lib, core product features, filtercredential_per_user_storage_tests, 5 runs per side.credential.rs(412075c)a_session_is_stored_in_the_users_own_databaseget_session_tokenis None (:120)a_session_sign_out_keeps_the_api_key_on_per_user_storageremovedSessionis false (:162)another_user_signing_in_does_not_get_the_first_users_api_keysecurity::credentials::tests: 269/269 pass. That includes the existing storage-backend testsclearing_the_session_keeps_the_api_key_on_a_storage_backendandclearing_everything_removes_the_api_key_on_a_storage_backend.--driver sqlite --only A,J, core built at 73204a2): J is 6/6, withremovedSession: trueand the key in effect after the sign-out and after a restart. A-secret-roundtrip passes, and the session reads back exactly.cargo fmtorder of the test module lines on top of 73204a2.Submission Checklist
credential.rsare exercised by the new and existing credential tests. The restore-on-failure branches are only logging.Impact
users/localand reads as signed out: one sign-in stores it in the user's database. The strayapp-sessionrecord left inusers/localis not cleaned up here. Its token secret is keyed to the other user, so it reads back with no token and reports unauthenticated (seen in the scenario run).Related
Summary by CodeRabbit