fix(auth)+test(e2e): logout clears local session files completely#197
Merged
Conversation
Contributor
|
Dev build ready — try this branch: |
Contributor
E2E Test Pass · ❌ FAILTag:
Sentry traces: view spans for this run |
Uses createTestSession from @parity/product-sdk-terminal/testing to write valid session files to disk, then asserts dot logout removes them. Exercises the waitForLogout fallback path: the synthesized local account isn't registered on People chain so disconnect() fails with NoAllowanceError, and clearLocalAppStorage() is what actually deletes the files. Closes the only real coverage gap from #50; the rest of that issue's task list is either stale (allowances model changed) or out-of-reach for an automated test (real mobile signing).
The e2e test added by the previous commit was failing on
`dot-cli_SsoSessions.json` not being removed after a clean logout.
Tracked to a real product gap, not a test assertion mismatch.
## What was happening
The SDK's `adapter.sessions.disconnect(session)` does three things:
1. `sendDisconnectMessage()` — publishes a Disconnected statement so
the paired mobile app drops its side
2. `ssoSessionRepository.filter(s => s.id !== session.id)` — removes
this session from the in-memory list and writes the (now possibly
empty) list back to disk
3. `userSecretRepository.clear(sessionId)` — unlinks the per-session
UserSecrets file
Step 2 writes BACK to `${DAPP_ID}_SsoSessions.json` with the filtered
list — never unlinks the file. So after a single-session logout the
file lingers on disk as `[]`. The CLI's `clearLocalAppStorage()` would
unlink it, but `waitForLogout` only invoked it on the failure path —
the happy path returned right after `onStatus({ step: "success" })`.
Net effect across login → logout cycles: empty `dot-cli_SsoSessions.json`
files accumulate in `~/.polkadot-apps/`. Not a security issue (no live
credentials in the empty list), but it contradicts the spec's "dot
logout cleans up the local session" guarantee and makes "is this user
signed in?" harder to answer by inspecting the directory.
## Fix
One-line addition in `src/utils/auth.ts::waitForLogout`: also call
`clearLocalAppStorage()` in the `result.isOk()` branch, mirroring what
the failure branch already does. The cleanup is scoped to `${DAPP_ID}_`
files so it can't touch other polkadot-apps' state.
Doc comment on `waitForLogout` updated to reflect both branches running
the cleanup. The function is still scoped narrower than `rm -rf` and
still best-effort under the hood (per-file unlinks are `.catch(() =>
{})`).
## Tests
- New `src/utils/auth.test.ts` unit test ("clears local DAPP_ID files
on the happy path too, not just the failure path") seeds
`${DAPP_ID}_SsoSessions.json`, `${DAPP_ID}_UserSecrets_abc.json`, and
a foreign-app file in the temp dir, runs `waitForLogout` with a
disconnect adapter that returns Ok, and asserts the two DAPP_ID files
are unlinked while the foreign-app file is preserved. Catches a
future regression that reverts the cleanup call.
- The existing happy-path test still passes (no files seeded, no
cleanup to observe).
- `e2e/cli/session.test.ts` comment refreshed — the test no longer
depends on the disconnect call rejecting; the cleanup invariant
holds on both branches.
497 unit tests pass; format/license/build all clean.
ottovlotto
force-pushed
the
e2e-logout-session-cleanup-test
branch
from
cb3195e to
2c5343c
Compare
ottovlotto
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an e2e test for
dot logoutclearing local session files, and the product fix that makes it actually do so.The test was originally opened on its own (the e2e/cli/session.test.ts commit). When run against current main it surfaces a real product gap: after a clean (happy-path) logout,
${DAPP_ID}_SsoSessions.jsonlingers on disk as[]content. The added commit closes that gap.What was happening
The SDK's
adapter.sessions.disconnect(session)does three things:sendDisconnectMessage()— publishes aDisconnectedstatement so the paired mobile app drops its side.ssoSessionRepository.filter(s => s.id !== session.id)— removes this session from the in-memory list and writes the (now possibly empty) list back to disk.userSecretRepository.clear(sessionId)— unlinks the per-sessionUserSecretsfile.Step 2 writes BACK to
${DAPP_ID}_SsoSessions.jsonwith the filtered list — never unlinks the file. So after a single-session logout the file lingers as[]. The CLI'sclearLocalAppStorage()would unlink it, butwaitForLogoutonly invoked it on the failure path — the happy path returned right afteronStatus({ step: "success" }).Net effect across login → logout cycles: empty
dot-cli_SsoSessions.jsonfiles accumulate. Not a security issue (no live credentials), but contradicts the spec's "dot logout cleans up the local session" guarantee.Fix
One-line addition in
src/utils/auth.ts::waitForLogout: also callclearLocalAppStorage()in theresult.isOk()branch, mirroring the failure branch. The cleanup is scoped to${DAPP_ID}_files so foreign-app state is untouched.Tests
e2e/cli/session.test.ts— new test (original commit) that usescreateTestSessionto synthesize an on-disk session, runsdot logout, asserts alldot-cli_*files are gone. Without the product fix this test fails; with it, it passes regardless of whether the disconnect statement actually round-trips on the testnet.src/utils/auth.test.ts— new unit test ("clears local DAPP_ID files on the happy path too") that seeds${DAPP_ID}_SsoSessions.json,${DAPP_ID}_UserSecrets_abc.json, and a foreign-app file, runswaitForLogoutwith a disconnect adapter that returns Ok, and asserts the DAPP_ID files are unlinked while the foreign-app file is preserved. Catches a future regression that reverts the cleanup call.Verification
pnpm format:check— cleanpnpm lint:license— cleanpnpm build— clean (Bun SEA)pnpm test— 497 passed / 1 skippedpr-init-sessione2e cell — passes (waiting on GH Actions to fire the CI workflows for this PR)Background
Replaces the original test-only version of this PR (opened 2026-05-21, 68 commits behind main at the time of rebase). Closes the only real coverage gap from #50; the rest of that issue's task list is either stale (allowances model changed) or out-of-reach for an automated test (real mobile signing).