fix: getIdentity() from a subscriber waits for the identity of the sign-in it was told about - #198
Merged
Merged
Conversation
…gn-in it was told about A subscribe() listener runs when the record changes, which for a sign-in is before this client installs the identity that goes with it. getIdentity() called from the listener threw SessionNotHeldError: in this tab because the ceremony writes the record before opening the session, and for a peer's sign-in because the restore was queued only after the listeners ran. The ceremony now publishes the write-and-install step as a promise that getIdentity() waits for, and the subscription queues the restore before notifying, so a listener's getIdentity() awaits the restore that installs it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved disposal and commit/restore race issues remain in src/client/auth-client.ts.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes subscriber getIdentity() timing during local and peer sign-ins.
Changes:
- Tracks sign-in commit completion.
- Queues restores before notifying subscribers.
- Adds regression tests for both sign-in paths.
| File | Description |
|---|---|
tests/client/auth-client.test.ts |
Adds subscriber identity regression tests. |
src/client/auth-client.ts |
Coordinates commits, restores, and subscriber notifications. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
MRmarioruci
approved these changes
Sep 23, 2026
marc0olo
added a commit
to dfinity/icskills
that referenced
this pull request
Sep 25, 2026
The skill's local-II fallback didn't work with `@icp-sdk/auth` 10: it set only `identityProvider`, so sign-in against a local II (`ii: true`) failed after the ceremony. Found while migrating dfinity/examples to auth 10 (dfinity/examples#1487), where we keep local II. ## Verification Checked against the `@icp-sdk/auth` 10.0.0 source and reproduced in a browser against a local network: - The client mints app delegations itself via `app_prepare_delegation` / `app_get_delegation` on the II canister (`session-minter.ts`), through an `HttpAgent` built from `agentOptions`. Its default host already resolves to the page origin on `localhost`, so the mint reaches the local replica, but without the local root key it fails with `TrustError: Certificate verification error` (`"Invalid signature"`). Reproduced both: no `agentOptions` → that error; `agentOptions: { rootKey }` only → sign-in, authenticated call, reload and logout work. `host` is not needed. - The II in older network launchers lacks those methods (checked the local II's `candid:service`); the launcher shipping PocketIC 2026-09-18 has them. `icp network update` fetches it. - The delegation is short-lived and rotated by `SessionIdentity`, so a logout timer set from its expiration fires after minutes. The `subscribe()` / `getIdentity()` pitfall from the first revision was dropped: the ordering it described is fixed in [dfinity/icp-js-auth#198](dfinity/icp-js-auth#198) (patch release pending), so the skill does not document it. ## Changes - `skills/internet-identity/SKILL.md` — local-II fallback: `agentOptions` + snippet, and the `icp network update` requirement; pitfalls 18–19 (local II without `agentOptions`, delegation-expiry logout timers). - `evaluations/internet-identity.json` — new adversarial cases 27–28, one per pitfall. ## Evals Cases 27–28 are new, so they are run with baseline. Cases 2 and 12 cover the local-II section and were re-run as a regression check (with skill only). The other cases were not re-run — none assert what changed. Re-checked on the final commit (70c4c61): cases 2, 12 and 28 still pass with the skill (4/4, 3/3, 4/4); case 27's content is unchanged since its run below. Case 27 was updated after review (root key only, no `host`), and its mainnet-II check was split out and scoped to *the fix*: the skill currently recommends mainnet II by default, so some with-skill runs add it as an aside, which the old wording counted as a failure. Before rescoping, 4 runs gave 2/4, 4/4, 4/4, 3/4, all misses from that aside or run variance. Two earlier baseline runs timed out; the result below is the first valid baseline. <details> <summary>Eval 27 — Adversarial: local II sign-in without agentOptions — WITH 5/5 | WITHOUT 2/5</summary> ``` WITH skill: 5/5 passed ✅ Adds agentOptions to the AuthClient constructor with the root key from the ic_env cookie (IC_ROOT_KEY) ✅ Explains that the client mints its delegations by calling the II canister and that, without the local root key, it verifies the local replica's response against the mainnet root key and fails ✅ Mentions that an outdated local II may lack the minting methods, fixed by running `icp network update` and restarting the network ✅ Does NOT set host (e.g. window.location.origin) in agentOptions, and does NOT suggest fetchRootKey() or shouldFetchRootKey ✅ Fixes the local II setup rather than replacing it: switching to mainnet II is NOT presented as the fix (mentioning it as the default alternative is fine) WITHOUT skill: 2/5 passed ❌ Adds agentOptions to the AuthClient constructor with the root key from the ic_env cookie (IC_ROOT_KEY) → The output never mentions agentOptions or IC_ROOT_KEY at all; it focuses entirely on a fabricated constructor/API misuse issue. ❌ Explains that the client mints its delegations by calling the II canister and that, without the local root key, it verifies the local replica's response against the mainnet root key and fails → No mention of delegation minting, root key verification, or mainnet vs local root key mismatch anywhere in the output. ❌ Mentions that an outdated local II may lack the minting methods, fixed by running `icp network update` and restarting the network → The output never discusses outdated local II canister versions or the `icp network update` remedy. ✅ Does NOT set host (e.g. window.location.origin) in agentOptions, and does NOT suggest fetchRootKey() or shouldFetchRootKey ✅ Fixes the local II setup rather than replacing it: switching to mainnet II is NOT presented as the fix ``` </details> <details> <summary>Eval 28 — Adversarial: logout timer from the delegation expiration — WITH 4/4 | WITHOUT 1/4</summary> ``` WITH skill: 4/4 passed ✅ Explains that in 9.x and later the delegation getIdentity() signs with is short-lived and replaced by the client as it ages, so its expiration is not the end of the session ✅ Removes the timer ✅ Reacts to the session ending through authClient.subscribe(), checking isAuthenticated() or an 'expired' status from getStatus() ✅ Does NOT suggest re-scheduling the timer whenever the delegation is refreshed WITHOUT skill: 1/4 passed ❌ Explains that in 9.x and later the delegation getIdentity() signs with is short-lived and replaced by the client as it ages, so its expiration is not the end of the session → The output attributes the early logout entirely to the default IdleManager and never mentions that the delegation itself is short-lived and rotated by the client. ❌ Removes the timer → Both fixes explicitly retain the user's own setTimeout-based delegation-expiration timer ('keep your own delegation-expiration timer as the sole logout trigger') rather than removing it. ❌ Reacts to the session ending through authClient.subscribe(), checking isAuthenticated() or an 'expired' status from getStatus() → The output never mentions authClient.subscribe(), isAuthenticated(), or getStatus(); it only proposes idleOptions configuration. ✅ Does NOT suggest re-scheduling the timer whenever the delegation is refreshed ``` </details> <details> <summary>Eval 2 — Local II URL — WITH 4/4 | WITHOUT skipped</summary> ``` WITH skill: 4/4 passed ✅ identityProvider is an object with both authorizeUrl and canisterId, not a bare URL string ✅ Local authorizeUrl uses the well-known II frontend alias: http://id.ai.localhost:8000/authorize ✅ canisterId is the well-known II backend canister, rdmx6-jaaaa-aaaaa-aaadq-cai ✅ Does NOT suggest dynamic lookup via ic_env cookie, environment variables, or dfx commands for the II canister ID ``` </details> <details> <summary>Eval 12 — local vs mainnet II usage — WITH 3/3 | WITHOUT skipped</summary> ``` WITH skill: 3/3 passed ✅ Suggest using mainnet's id.ai because the network launcher after icp-cli 0.2.4 supports verifying mainnet signatures ✅ Explains that a local instance of internet identity can be deployed by setting `ii: true` in icp.yaml ✅ Explains that the local instance of internet identity is accessible at http://id.ai.localhost:8000 ``` </details>
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
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.
A
subscribe()listener that callsgetIdentity()on asigned-instatus getsSessionNotHeldError. The listener runs when the record changes, and for a sign-in that happens before the client installs the identity:signIn()writes the record in#persistSession, and only installs the identity after promoting the app credential and opening the session.#restoreAgain(), so the listener'sgetIdentity()awaited the previous, already settled restore.Changes:
signIn()wraps the write-and-install step in a#committingpromise, andgetIdentity()waits for it after#init(). Only that step counts, so agetIdentity()while the signer window is open does not wait on the user. A ceremony failing partway still ends inSessionNotHeldError.getStatus()still returns the new record inside a listener.Tests: two regression tests, one per case. Each fails with
SessionNotHeldErrorwithout its half of the fix. Full suite, typecheck and biome pass.Reported in dfinity/icskills#404 (comment).
🤖 Generated with Claude Code