docs: update the Internet Identity guides for @icp-sdk/auth v9 - #395
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review: Internet Identity guides for @icp-sdk/auth v9
Must fix
- Em-dash in a code comment (
docs/guides/authentication/internet-identity.mdx:218):// function that fetches it, which the client calls when it needs the value —.checkEmdashinscripts/validate.jsskips fenced code, so CI cannot see this one, but the ban is repo-wide. A colon or a comma reads the same here. - Internal link is missing its section anchor (
internet-identity.mdx:53):../../references/system-canisters.mdshould be../../references/system-canisters.md#using-system-canisters-in-local-development. The sentence backing the claim is line 250 of that page, inside## Using system canisters in local development, and the page already uses that slug internally (line 192). Without the anchor the reader lands at the top of a 20-heading catalogue of NNS, CMC and ledger canisters.
Suggestions
- State the version requirement in "Install frontend packages":
npm install @icp-sdk/auth @icp-sdk/coreis unpinned, and the page now documents a v9-only API that throws aTypeErroron v8. dfinity/icskills#384 raises its prerequisite to>= 9.0.0; one sentence here keeps the two aligned and gives the "Upgrading to v9" link a reason to be read before the code rather than at the foot of the page. isValidSsoDomainthrows when its signal aborts (internet-identity.mdx:165-172): the library is explicit that an abandoned check is not a verdict, soAbortSignal.timeout(5_000)raises on a slow lookup while the example calls it bare insideif (await ...). The page already carries a Common-mistakes bullet for this exact shape aroundsignIn().signed-in-elsewherehas no way out in the new render example: the branch routes toshowResume(status.principal), butgetIdentity()throwsSessionNotHeldErrorin that state, and acquiring a credential needsprompt: 'none'with ahint. A sentence saying so would close the loop, or a pointer to the shared sessions guide that walks through it.- Give the security page the actual numbers (
identity-and-access-management.mdx:96-98): the section argues for narrowing but never states the defaults (seven days idle, thirty days total), which is what makes the OWASP 30-minute figure land. Worth adding thatmax_idlehas a 10-minute floor, so 30 minutes is comfortably inside what II accepts. Related: the PR description says the page "notes that the provider's defaults are too long for sensitive data", and as written it does not.
Verified
- Every v9 API claim against
dfinity/icp-js-authat tag9.0.0:identityProvideras{ authorizeUrl, canisterId }with both halves required together and a string orURLthrowingTypeError(src/client/auth-client.ts:395-413), the mainnet canister defaultrdmx6-jaaaa-aaaaa-aaadq-cai(:43),nonce: () => Promise<Uint8Array>(:1055), synchronousisAuthenticated()alongsideasync getIdentity(): Promise<Identity>(:562,:529), the fourSessionStatusstates and which of them carry a principal (:296-304),subscribe()anddispose()(:650,:665),openIdProviderandssoDomainas mutually exclusive (:458),isValidSsoDomain(domain, signal)(src/client/sso.ts:93), and both session bounds optional (:248-259). - The seven-day and thirty-day figures against Internet Identity itself rather than the library's JSDoc:
DEFAULT_SESSION_TTL_NSandMAX_SESSION_TTL_NSare both30 * DAY_NS(src/internet_identity/src/sessions.rs:30),DEFAULT_SESSION_IDLE_NSis7 * DAY_NSandMIN_SESSION_IDLE_NSis10 * MINUTE_NS(src/internet_identity/src/storage/account.rs:62,70). The "capped by the organization" claim holds as well:session_max_age_secondsis declared per domain in the well-known config. getIdentity()returnsIdentityand notDelegationIdentity, so thecalling-from-clients.md:310comment fix is right.- Coverage across
docs/: nothing outside this PR still carries v8-era auth API (IdleManager,idleOptions,IdbStorage,keyType,AuthClient.create, a bareidentityProviderstring) or stale delegation-lifetime advice.verifiable-credentials.md:175keepsnew URL(...)correctly, since that is@dfinity/verifiable-credentialsrather than this client. - Mechanical checks: the new link target exists, frontmatter is intact on both pages, no
dfx, nomo:base, no--in prose, the new snippet sits outside anyTabItem, both pages still end with## Next steps, andvalidateandbuild_and_deployare green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — all six points addressed in 74633e8, plus the three suggestions from @MRmarioruci. Must fix
Suggestions
You were also right that the description no longer matched the page: the "defaults are too long for sensitive data" framing was cut on purpose, since how long a sign-in lasts is the provider's policy and the client names no bounds of its own. The description is corrected.
|
marc0olo
left a comment
There was a problem hiding this comment.
Follow-up review: Internet Identity guides for @icp-sdk/auth v9
Fixed
- Em-dash at
internet-identity.mdx:218is now a comma. Re-scanned all three files: zero em-dashes (the one remaining incalling-from-clients.mdis the pre-existing<!-- Upstream: -->line, whichcheckEmdashskips by design). - Section anchor added:
../../references/system-canisters.md#using-system-canisters-in-local-development, matching the heading and the slug that page already uses internally at line 192. - Version requirement stated, and the pin turns out to be necessary rather than cosmetic:
npm install @icp-sdk/auth@9 @icp-sdk/core@6fails outright with ERESOLVE, because v9 declares@icp-sdk/core: ^5as its peer range. The line as written installs cleanly, and the page's own imports (safeGetCanisterEnv,AttributesIdentity) both exist in 5.4.0, so no example here depends on core 6. isValidSsoDomainnow separates "the domain publishes nothing" from "the check timed out", which matches the library's own framing that an abandoned check is not a verdict on the domain.signed-in-elsewherenow namesSessionNotHeldErrorand saysgetIdentity()throws until this origin holds a credential. Correct perauth-client.ts:545-557.- The security page carries the numbers now, and the clamping claim holds in both directions:
MIN_SESSION_TTL_NS(sessions.rs:32) andMIN_SESSION_IDLE_NS(storage/account.rs:62) are both10 * MINUTE_NS, so "both bounds are clamped with a ten-minute floor" is right, and the thirty minutes above sits inside it.
Not this PR's to fix, but now visible: @icp-sdk/core latest is 6.1.0, which is also this repo's watched pin, while guides/frontends/frameworks.md (twice), guides/canister-calls/calling-from-clients.md and guides/digital-assets/wallet-integration.md all carry a bare npm install @icp-sdk/core. A reader who follows one of those and then adds @icp-sdk/auth@9 hits the same ERESOLVE this page now avoids. Worth a separate issue: either pin core alongside auth on those pages, or wait for v9's peer range to catch up with core 6.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
marc0olo
left a comment
There was a problem hiding this comment.
Follow-up review: Internet Identity guides for @icp-sdk/auth v9
Re-reviewing at 2f59b7f. Everything from the previous rounds still holds, and step 4 now covers the page that is already open, which matches what the skill PR does. The new "Sharing a sign-in across sibling subdomains" section needs another pass, though, and the first two points below apply to dfinity/icskills#384 as well.
Still needs work
transport: "redirect"has a prerequisite the section does not mention. Internet Identity delivers a redirect response only to a callback that the callback's own origin declares at/.well-known/ii-auth-callbacks, and this is enforced by the URL transport rather than only by the MCP flow:src/frontend/src/lib/utils/transport/url.ts:55importsmatchDeclaredCallback, andauthCallbacks.tsstates that the name "is deliberately not MCP-specific". The validation fails closed, so as written/reauthnever comes back. What a reader needs: an entry that byte-exactly matcheshttps://<origin>/reauthwith no fragment, served asapplication/jsonwithAccess-Control-Allow-Origin(II reads it cross-origin), under the size cap, and without redirects, on each app origin rather than once on the derivation origin. This page already explains that_headerspattern twice, forii-alternative-originsandii-app-metadata, so a third block would fit the shape it already has. Worth carrying the upstream caveat too: a declared callback must terminate locally, because the response arrives in the URL fragment and a3xxwhoseLocationcarries none re-attaches it to the redirect target.- The reason to pass
hintis not what the section says. "Withouthintthe provider may answer for a different account, signing the user in as someone else" does not happen. Where Internet Identity holds more than one session and the request names none, it refuses:InteractionRequiredError.reasonisaccount_selection_required(session-delegation.ts). Where a mint does answer for an account the client was not expecting, the client rejects it asAccountMismatchError, which is documented as "neither gone nor retryable" with "the caller's own record is what has to give way", so the session is dropped rather than the wrong account adopted. Passinghintis still right; the consequence of omitting it is a failed resume, not a wrong identity. - Step 2's
returnTocontradicts the prose under it. The snippet hardcodesreturnTo: "/", while the next paragraph says/reauthpasses thatnextasreturnTo. Readingnew URLSearchParams(location.search).get("next") ?? "/"is what the prose describes. - The
/reauthsnippet is not self-contained. It readsstatus.principal, butstatusis created in step 3, which runs on the other pages rather than on this route, so the route never reads the status it takes the principal from. Constructing a client and readinggetStatus()inside step 2 would close that.
Smaller, in the same section: CookieStateStorage is used without showing its import, unlike the page's other snippets, and clientOptions omits identityProvider while every other example on the page passes getIdentityProvider(), so in local development these two clients alone would point at mainnet Internet Identity.
I am dismissing my earlier approval, which was recorded against 74633e8 and predates this section.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both confirmed in source, and the callback one was a real hole: a generated app would have deployed and never returned from
Also fixed on the docs side, all four: the hardcoded Eval results, which I owed you from the previous round as well:
Case 22 now asserts the callback document too, and your last point was exactly right: the case had been asking what the derivation origin serves, so it passed while the generated app could not have completed a sign-in. It now asks which origin serves each well-known document, and that expectation failed on the first run after I added it — the model produced Case 24 gained a behavior asserting the failure is a refused resume rather than a wrong identity, so the corrected wording is now tested rather than just written. |
marc0olo
left a comment
There was a problem hiding this comment.
Approving at 96d7b69, which is the head my dismissed approval did not cover.
All four points on the shared-subdomain section are fixed and checked against source: the /.well-known/ii-auth-callbacks document is documented per app origin with the exact-match, no-fragment, application/json and CORS rules and the fails-closed and terminate-locally caveats; the hint paragraph now names the account_selection_required refusal and the client-side AccountMismatchError, so it says what omitting it actually costs; returnTo reads the next parameter the prose describes; and the /reauth snippet constructs its own client and reads getStatus(), with the CookieStateStorage import shown and clientOptions passing getIdentityProvider() like the rest of the page.
Also still holding from the earlier rounds: no em-dashes, the section anchor on the system-canisters.md link, the @icp-sdk/auth@9 and @icp-sdk/core@5 pin that npm actually resolves, the guarded isValidSsoDomain call, and the session figures on the security page.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@icp-sdk/authv9 shipped, and the Internet Identity guides describe v8. Some of it no longer compiles, one recommendation is now the opposite of what the library wants, and the guide had nothing on sharing a sign-in across sibling subdomains.docs/guides/authentication/internet-identity.mdxidentityProvideris a pair. It is{ authorizeUrl, canisterId }in v9, and a string throws aTypeError.getIdentityProviderUrl()becomesgetIdentityProvider()and returns both halves; only the URL differs locally, since system canisters run at their mainnet IDs on the local network. Updated at all four call sites, including thederivationOriginexample, which namedhttps://id.aiwithout the/authorizepath.dispose().maxTimeToLiveto 8 hours" advice is gone. v9 bounds the session (maxTimeToIdle,maxTimeToLive, both optional) and replaces the short-lived delegation for you, so the sign-in example asks for no bounds at all.getStatus()'s four states andsubscribe(), including that a sign-out in one tab reaches the others with no reload, and thatgetIdentity()throwsSessionNotHeldErrorwhile a sign-in exists this origin holds nothing for.ssoDomainandisValidSsoDomainalongside the existingopenIdProvidersection. The check throws when its signal aborts rather than returningfalse, so the example distinguishes "publishes nothing" from "no verdict".derivationOriginauthorized by that origin's alternative-origins document, since principals are per origin and a shared cookie changes nothing on its own; the same cookie domain throughCookieStateStorage, which means trusting every origin under it; and a/reauthroute answeringsigned-in-elsewherewith a second client (transport: "redirect"because the request runs on load with no user gesture,prompt: "none", andhintso the provider cannot answer for a different account). Every page runs the load check, not only protected ones, or a visitor already signed in on a sibling sees a signed-out header. Links the client's shared sessions guide for the rest.requestAttributesnonce. The examples passed a promise; it has been a callback (() => Promise<Uint8Array>) since v8, so neither example would have run.@icp-sdk/auth@9 @icp-sdk/core@5, the pair this page documents, so it does not drift when the next major lands. On npm the core pin is redundant (v9's peer range holds it at^5), but pnpm and yarn takelatestfor a bare request and reduce the conflict to a warning.docs/guides/security/identity-and-access-management.mdxThe session-timeout recommendation linked to
IdleManager, a class v9 removed. It now shows the bounds II actually enforces, with the OWASP 30-minute figure expressed asmaxTimeToIdle, and states the provider's own defaults (seven days idle, thirty days total) and the ten-minute floor both bounds are clamped to — verified againstdfinity/internet-identity@main(sessions.rs:30-32,storage/account.rs:62,70) rather than the client's JSDoc. How long a sign-in lasts is the provider's policy, narrowed by consent and by any organization cap, so leavingsignIn()unbounded is presented as the right default and narrowing as an application's own choice.docs/guides/canister-calls/calling-from-clients.mdOne comment:
getIdentity()returns anIdentity, not aDelegationIdentity.Verification
node scripts/validate.js --allpasses (210 files) andnpm run buildcompletes (210 pages). The first validate run failed on six em-dashes I had written, which is how I learned this repo forbids them in prose; all six are rewritten, and a seventh inside a code comment was removed after review.Left alone deliberately:
docs/references/internet-identity-spec.md, which is synced fromdfinity/internet-identityand would be overwritten.🤖 Generated with Claude Code