refactor: the session identity resolves its own account key

Author: sea-snake

src/client/auth-client.ts

@@ -804,17 +804,12 @@ export class AuthClient {
   ): Promise<SessionIdentity> {
     const minter = source ?? (await this.#minterFor(key, sessionChain));
 
-    const stored = await this.#credentialStorage.get(this.#slots.app);
-    let accountKey = stored?.chain?.publicKey;
-    if (accountKey === undefined) {
-      const appKey = await this.#credentialStorage.create();
-      const appChain = await minter.mint(appKey.getPublicKey().toDer());
-      await this.#credentialStorage.set(this.#slots.app, { identity: appKey, chain: appChain });
-      accountKey = appChain.publicKey;
-    }
-
-    const identity = new SessionIdentity({
-      accountKey,
+    // The identity resolves its own account key, through the same locked
+    // read-or-mint every rotation uses. Doing it here instead meant a second
+    // writer of the app slot that took no lock: a load could overwrite a
+    // credential a peer tab had just minted, and would not see the credential
+    // that peer had left for it to adopt.
+    return SessionIdentity.create({
       sessionExpiresAtMs: earliestExpiryMs(sessionChain),
       source: minter,
       storage: this.#credentialStorage,
@@ -826,10 +821,6 @@ export class AuthClient {
         this.#identity = new AnonymousIdentity();
       },
     });
-    // Adopts what is stored, so the identity a caller is handed already holds a
-    // delegation rather than minting on its first request.
-    await identity.refresh();
-    return identity;
   }
 
   // Memoized — only runs #hydrate once, returns the same promise on repeat calls.

tests/client/auth-client.test.ts

@@ -6,6 +6,7 @@ import {
 } from '@icp-sdk/core/identity';
 import { Principal } from '@icp-sdk/core/principal';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { SessionGoneError } from '../../src/client/app-delegation-source.ts';
 import { AuthClient } from '../../src/client/auth-client.ts';
 import {
   APP_PENDING_SLOT,
@@ -604,17 +605,43 @@ describe('AuthClient signIn', () => {
     await expect(client.signIn()).rejects.toThrow('connection failed');
   });
 
+  it('fails the sign-in when the session granted has nothing left to mint against', async () => {
+    const credentialStorage = new MemoryCredentialStorage();
+    const stateStorage = new MemoryStateStorage();
+    const client = new AuthClient({
+      credentialStorage,
+      stateStorage,
+      idleOptions: { disableIdle: true },
+    });
+    handleSignIn(FakeTransport.last());
+
+    // One millisecond of session: the fixture takes the chain's life from the
+    // requested ceiling. Nothing can be minted against it, so an identity built
+    // around it could never sign.
+    await expect(client.signIn({ maxTimeToLive: 1_000_000n })).rejects.toThrow(SessionGoneError);
+
+    // And nothing is recorded. Reporting a sign-in that cannot make a call would
+    // have the application render a signed-in page and fail on its first request,
+    // which is the outcome this refusal exists to avoid.
+    expect(stateStorage.get()).toBeNull();
+    expect(await credentialStorage.get(APP_SLOT)).toBeNull();
+  });
+
   it('asks for a session, carrying maxTimeToLive and no targets', async () => {
     const client = new AuthClient();
     const transport = FakeTransport.last();
     handleSignIn(transport);
 
-    await client.signIn({ maxTimeToLive: 1_000_000n });
+    // An hour, in nanoseconds. The value only has to survive onto the wire, but
+    // the fixture derives the session's own life from it — so a token number here
+    // would build a session already too short to mint against, which is a
+    // different outcome and not the one this test is about.
+    await client.signIn({ maxTimeToLive: 3_600_000_000_000n });
 
     const req = transport.requests[0];
     expect(req.method).toBe('ii_session_delegation');
     expect(req.params?.sessionPublicKey).toEqual(expect.any(String));
-    expect(req.params?.maxTimeToLive).toBe('1000000');
+    expect(req.params?.maxTimeToLive).toBe('3600000000000');
     // A session chain is restricted to Internet Identity, so an application has
     // no targets to ask for: what it may call is decided by the delegations
     // minted from the session, not by the session itself.