feat(be,fe): address SSO review feedback on #3785 (follow-up)#3803
Merged
aterga merged 6 commits intodfinity:mainfrom Apr 24, 2026
Merged
feat(be,fe): address SSO review feedback on #3785 (follow-up)#3803aterga merged 6 commits intodfinity:mainfrom
aterga merged 6 commits intodfinity:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up SSO UX + data-model refinements on top of #3785, including improved labeling (“Continue with SSO”), simplifying user-facing error copy, preventing linking an SSO already attached to the current identity, and moving SSO labeling from FE localStorage to canister-stamped credential metadata.
Changes:
- Backend: make the SSO allowlist depend on
is_production, persist/replay discoverable OIDC configs across upgrades, and stampsso_domain/ optionalsso_nameinto verified credential metadata. - Frontend: add SSO discovery + UI flow, render SSO credentials using stamped metadata, and improve OAuth callback error handling + provider resolution via strict
(iss,aud)matching. - Cleanup: remove deprecated
oidc_configsfrom init/synchronized config surfaces and adjust tests/types/IDL accordingly.
Reviewed changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/internet_identity_interface/src/internet_identity/types.rs | Removes deprecated oidc_configs from init + synchronized config types. |
| src/internet_identity/tests/integration/http.rs | Updates synchronized-config assertion after removing oidc_configs. |
| src/internet_identity/tests/integration/config/oidc_configs.rs | Aligns integration tests with prod/beta allowlist split and new surfacing via discovered_oidc_configs. |
| src/internet_identity/src/openid/generic.rs | Makes allowlist environment-dependent; carries/stamps discovery_domain + optional name into credential metadata. |
| src/internet_identity/src/main.rs | Stops returning oidc_configs in config(); rehydrates in-memory SSO providers from persistent state on init/upgrade. |
| src/internet_identity/src/assets.rs | Ships only openid_configs in /.config.did.bin (frontend no longer needs oidc_configs). |
| src/internet_identity/internet_identity.did | Removes deprecated oidc_configs from init args in DID. |
| src/frontend/src/routes/(new-styling)/manage/(authenticated)/(access-and-recovery)/access/components/OpenIdItem.svelte | Renders SSO credentials via sso_domain marker and shows SSO icon; uses displayName. |
| src/frontend/src/routes/(new-styling)/manage/(authenticated)/(access-and-recovery)/access/+page.svelte | Passes sub/aud into openIdName for better provider resolution. |
| src/frontend/src/routes/(new-styling)/authorize/+page.svelte | Uses aud when resolving provider config. |
| src/frontend/src/lib/utils/ssoDiscovery.ts | Adds two-hop SSO discovery implementation with validation, caching, rate-limits, retries. |
| src/frontend/src/lib/utils/ssoDiscovery.test.ts | Adds unit tests for SSO discovery validation, security checks, caching, and error wrapping. |
| src/frontend/src/lib/utils/openID.ts | Adds OAuthProviderError, extractIdTokenFromCallback, strict (iss,aud) provider resolution, SSO metadata name/logo logic, scope selection helper. |
| src/frontend/src/lib/utils/openID.test.ts | Updates tests for new config resolution + new helpers; adds callback parsing tests. |
| src/frontend/src/lib/locales/en.po | Adds new SSO-related strings and updates account copy to use {displayName}. |
| src/frontend/src/lib/locales/de.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/es.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/fr.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/id.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/it.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/nl.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/pl.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/ru.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/uk.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/locales/ur.po | Adds new SSO-related strings (untranslated placeholders) and updates {displayName} strings. |
| src/frontend/src/lib/globals.ts | Refactors OpenID email verification IDL variant into a reusable constant and formats type. |
| src/frontend/src/lib/generated/internet_identity_types.d.ts | Updates generated types for discoverable OIDC config + discovered configs methods/types. |
| src/frontend/src/lib/generated/internet_identity_idl.js | Updates generated IDL for discoverable OIDC config + discovered configs query. |
| src/frontend/src/lib/flows/authLastUsedFlow.svelte.ts | Calls findConfig with aud as undefined (issuer-only fallback). |
| src/frontend/src/lib/flows/authFlow.svelte.ts | Adds signInWithSso view + continueWithSso using synthetic OpenID config and selected scopes. |
| src/frontend/src/lib/flows/addAccessMethodFlow.svelte.ts | Adds SSO view + linkSsoAccount for access-method linking via discovery result. |
| src/frontend/src/lib/components/wizards/auth/views/SignInWithSso.svelte | Adds SSO domain entry screen with debounced registration+discovery and actionable error mapping. |
| src/frontend/src/lib/components/wizards/auth/views/PickAuthenticationMethod.svelte | Adds SSO entry button and changes button layout; updates aria-label. |
| src/frontend/src/lib/components/wizards/auth/AuthWizard.svelte | Wires SSO view into auth wizard and hooks continueWithSso. |
| src/frontend/src/lib/components/wizards/addAccessMethod/views/AddAccessMethod.svelte | Adds SSO option button in add-access-method wizard. |
| src/frontend/src/lib/components/wizards/addAccessMethod/AddAccessMethodWizard.svelte | Wires SSO view into add-access-method wizard and passes existing credentials for “already linked” check. |
| src/frontend/src/lib/components/utils/error.ts | Adds toast handling for OAuthProviderError surfaced from callback fragments. |
| src/frontend/src/lib/components/ui/ManageIdentities.svelte | Passes sub and aud (undefined) into openIdName for last-used identities. |
| src/frontend/src/lib/components/ui/IdentityAvatar.svelte | Passes sub and aud (undefined) into openIdLogo for last-used identities. |
| src/frontend/src/lib/components/icons/SsoIcon.svelte | Adds an SSO icon component. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The SSO entry in PickAuthenticationMethod and AddAccessMethod leads into a flow that can end in either a sign-in or a sign-up, so "Sign in with SSO" is misleading. Rename the `aria-label` to "Continue with SSO" — matches the copy pattern used by the direct-provider buttons and the Continue button inside the SSO screen itself. The SignInWithSso screen's h1 is updated separately in the following commit. Addresses sea-snake's review comment on dfinity#3785. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four related fixes from sea-snake's review, best reviewed together
since they overlap on the same files:
1. **Slim SSO error mapping to user-actionable copy.** `mapSubmitError`
in `SignInWithSso.svelte` had a ladder of branches for every
provider-misconfiguration shape (hostname mismatch, non-HTTPS
issuer / auth endpoint, malformed discovery doc, ...). These
read as implementation-detail leakage to end users. Keep only the
cases the user (or their SSO admin) can act on — domain-not-
configured, canary allowlist, OAuth provider errors from the
callback fragment, rate limits — and fall back to a generic
"SSO sign-in for <domain> failed" for everything else. Always
`console.error` the raw error so engineers still have the stack.
2. **Disable Continue when the SSO is already linked to this
identity.** `SignInWithSso.svelte` takes an optional
`openIdCredentials` prop (passed in the add-access-method context,
left `undefined` in the sign-in context). After two-hop discovery
reveals the SSO's `(iss, aud)`, a `$derived` `isAlreadyLinked`
checks against the prop and disables the Continue button with
an inline hint. Mirrors the direct-provider (Google/Apple/...)
button-disable in `AddAccessMethod.svelte` — reaching the
canister's `OpenIdCredentialAlreadyRegistered` for this identity
is no longer possible, so the `OpenIdCredentialAlreadyLinkedHereError`
specialization in `linkOpenIdAccount` / `handleError` is removed
as unreachable.
3. **Move SSO domain + name from FE localStorage to canister-stamped
metadata.** The previous iteration used `ssoDomainStorage.ts` (a
per-device localStorage map of `(iss, sub, aud) → domain`) to
label SSO credentials by the domain the user typed. That's
unreliable across devices and a lot of FE bookkeeping for something
the canister already has. The canister now stamps two new metadata
keys on any credential verified by a `DiscoverableProvider`:
- `sso_domain` — the `discovery_domain` the user entered
(canonical SSO label; always present for SSO credentials).
- `sso_name` — the `name` field from
`{domain}/.well-known/ii-openid-configuration` if the domain
publishes one (e.g. `"DFINITY"`). Optional.
The `IIOpenIdConfiguration` hop-1 schema gets a new optional
`name: Option<String>` (serde `#[serde(default)]` so older
deployments that don't publish the field still parse).
`DiscoverableProvider` / `DiscoveryState` carry `discovery_domain`
and a `discovered_name` ref that hop-1 populates each refresh.
4. **Render the SSO `name` (with domain fallback).** `openIdName`
now reads `sso_name` → `sso_domain` → `findConfig(iss, aud).name`,
so an SSO credential linked via a domain that publishes
`name: "DFINITY"` renders as "DFINITY", and one that doesn't
renders as "dfinity.org". `openIdLogo` returns `undefined` when
`sso_domain` is present (generic SSO icon). `OpenIdItem.svelte`
detects SSO via `sso_domain` directly instead of the "no logo
found" heuristic — which was brittle for direct-provider credentials
whose issuer didn't match any `openid_configs` entry.
Removes `ssoDomainStorage.ts` and its callers in
`authFlow.svelte.ts` and `addAccessMethodFlow.svelte.ts`. The SSO
sign-in flow no longer needs to pre-decode the JWT either — the
canister handles labeling — so `continueWithSso` collapses to
`return this.continueWithOpenId(syntheticConfig)`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Locales re-extracted via `npm run extract` after the SSO copy change
("Sign in with SSO" → "Continue with SSO") and the slimmed
`mapSubmitError` output dropped several now-unused strings. New
source strings in en.po + empty msgstr placeholders in the 10
non-English catalogs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aterga
force-pushed
the
sso-review-fixes-3785
branch
from
b5d8994 to
5145d7a
Compare
Five small fixes from Copilot's pass on the follow-up PR:
1. `error.ts`: the `OAuthProviderError` toast branch is hit by both
SSO and direct-provider OAuth failures, so "SSO provider returned"
was misleading for direct Google/Apple/Microsoft. Broadened to
"OAuth provider returned" + generic admin-hint in the description.
2. `openID.test.ts`: dropped the stale comment that still referred
to the removed localStorage SSO map — point at the canister-
stamped `sso_domain` / `sso_name` metadata instead.
3. `ssoDiscovery.ts` header: updated the reference from the now-
removed `ALLOWED_DISCOVERY_DOMAINS` const to the
`allowed_discovery_domains()` function.
4. `OpenIdItem.svelte`: guard against `logo === undefined` in the
non-SSO branch so the generic `SsoIcon` renders as a fallback
instead of `{@html logo}` stringifying `undefined` into the DOM.
This covers direct-provider credentials whose issuer doesn't
match any `openid_configs` entry (e.g. after client_id rotation).
5. `ssoDiscovery.ts` `fetchWithRetry`: classify errors so only
transient failures (network errors, HTTP 408 / 429 / 5xx) are
retried with backoff. Deterministic 4xx (404, 400, 401, 403, ...)
fail fast — retrying a `404` on a domain that doesn't publish
`/.well-known/ii-openid-configuration` three times just wastes
2s + 4s + 8s before the UI can surface the same error. Added a
retry-count assertion to the 404 test (now 1 fetch, not 3) and a
new test for the 503 retry path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sea-snake
reviewed
Apr 24, 2026
sea-snake
reviewed
Apr 24, 2026
…expose as optional fields sea-snake's two remaining threads on dfinity#3803: 1. **"This shouldn't be metadata; look up SSO domain + name by (iss, aud) and return them as optional fields on the credential when requested."** Done. `OpenIdCredentialData` (the Candid type returned by `get_anchor_info` and friends) gains two optional fields, `sso_domain` and `sso_name`. They are NOT stored — `DiscoverableProvider:: verify` no longer stamps them into the credential metadata. Instead, a new helper `openid::generic::sso_fields_for(iss, aud)` consults the current `DISCOVERY_TASKS` state; the `OpenIdCredential → OpenIdCredentialData` conversion in `storage/anchor.rs` calls it for every credential on the way out. Benefits over the previous metadata-stamping approach: - If an SSO admin updates the `name` in `/.well-known/ii-openid- configuration`, existing credentials pick it up on the next `get_anchor_info` — no stored-metadata migration needed. - If an SSO is de-registered from the canister allowlist, old credentials still exist but their `sso_domain` / `sso_name` come back `None`, which is the truthful "we don't know anything about this SSO anymore" signal. - Metadata stays a place for per-credential facts from the JWT (email, name, email_verified, tid, ...), not for cross-credential policy state. 2. **"Fallback should be done where needed in the frontend, not backend. This lets the frontend tell 'has a name' apart from 'only a domain' for future divergent rendering."** Done. Both the canister and the FE now carry `sso_name` and `sso_domain` as independent `Option<>` / `[] | [string]` values — the BE never collapses one into the other. `openIdName` resolves `ssoName → ssoDomain → findConfig(iss, aud).name` on the FE. `openIdLogo` returns `undefined` iff `ssoDomain` is present. Incidental cleanup: - `DiscoverableProvider` drops its `discovery_domain` and `discovered_name` fields; those only need to live on the matching `DiscoveryState` (which is where the discovery timer writes them and where `sso_fields_for` reads them from). - Regenerated `internet_identity.did`, `internet_identity_types.d.ts`, and `internet_identity_idl.js` for the new optional fields. - `openIdName` / `openIdLogo` signatures drop the unused `sub` parameter and take `ssoName` / `ssoDomain` explicitly. `LastUsedIdentity`- backed callers (`IdentityAvatar`, `ManageIdentities`) pass `undefined` — correct for direct providers, imprecise for SSO until dfinity#3795 tracks `aud` + SSO metadata on `LastUsedIdentity`. Tests: `cargo test -p internet_identity --bin internet_identity` → 219/219 pass; `cargo clippy --all-targets -D warnings`, `cargo fmt --check`, `npm run check`, `npm run lint`, `npm run format` all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sea-snake
reviewed
Apr 24, 2026
sea-snake
previously approved these changes
Apr 24, 2026
…ration sea-snake's latest review on dfinity#3803: > These two fields should be together in a single struct assigned to a > field called `sso_configuration`. Introduce `SsoConfiguration { domain: String, name: Option<String> }` and replace the two independent optional fields on `OpenIdCredentialData` with a single `sso_configuration : opt SsoConfiguration`. Semantics: - `sso_configuration: None` → not an SSO credential (direct Google / Apple / Microsoft, or an SSO that's no longer registered on the canister). - `sso_configuration: Some({ domain, name })` → SSO credential; `domain` is always known, `name` only if the domain publishes it. Keeping the two cases distinct inside the inner struct preserves the "has a name vs. only a domain" signal the FE may want to render differently. `openid::generic::sso_fields_for(iss, aud) -> (Option<String>, Option<String>)` is replaced by `openid::generic::sso_configuration_for(iss, aud) -> Option<SsoConfiguration>`; the storage-layer conversion sets the field in one line. `.did` / TS bindings / FE callers (`OpenIdItem`, `access/+page`, `addAccessMethodFlow`, `utils.test.ts`) follow. `openIdName` / `openIdLogo` keep their `(ssoName?, ssoDomain?)` param shape — callers just unwrap `sso_configuration` and pass its parts in. That keeps the fallback (`ssoName → ssoDomain → findConfig`) on the FE side, per the earlier review thread. Tests: cargo test 219/219 pass, clippy + fmt clean, svelte-check 0 errors, lint + prettier clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
dismissed
sea-snake’s stale review
Review dismissed by automation script.
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.
Follow-up to #3785 addressing @sea-snake's review. All five unresolved threads there are addressed here, and will be marked resolved on #3785 with pointers back to this PR.
Stacked on top of #3785: this branch includes all commits from #3785 plus three new commits that cover the review fixes.
Changes
1.
Sign in with SSO→Continue with SSOSSO button
aria-labelinPickAuthenticationMethodandAddAccessMethod, and the h1 inside the SSO screen, now read "Continue with SSO" — the downstream flow can end in either a sign-in or a sign-up, so "Sign in" was misleading.2. Slim
mapSubmitErrorto user-actionable copySignInWithSso.svelte#mapSubmitErrorused to enumerate every shape of provider misconfiguration (hostname mismatch, non-HTTPS endpoints, malformed discovery document, ...). Those read as implementation-detail leakage. Keep only the branches the user or their SSO admin can act on — domain not configured, canary allowlist, OAuth provider errors from the callback fragment, rate limits — and fall back to a generic"SSO sign-in for <domain> failed"for everything else. Every thrown error isconsole.error'd unconditionally so engineers still have the stack.3. Disable Continue when the SSO is already linked to this identity
SignInWithSsotakes a new optionalopenIdCredentialsprop. In the add-access-method flow,AddAccessMethodWizardpasses the identity's current credentials through; once two-hop discovery reveals the SSO's(iss, aud), a$derivedcheck disables the Continue button with an inline hint if that SSO is already linked. Mirrors howAddAccessMethod.sveltealready disables direct-provider buttons for already-linked providers. Leftundefinedin the sign-in flow, where reusing an existing credential is the point.This makes reaching
OpenIdCredentialAlreadyRegisteredfor this identity impossible, so theOpenIdCredentialAlreadyLinkedHereErrorspecialization inlinkOpenIdAccountand the corresponding toaster branch inhandleErrorare dropped as unreachable.4 + 5. Move SSO domain + name from FE localStorage to canister-stamped credential metadata
ssoDomainStorage.ts(a per-device localStorage map of(iss, sub, aud) → domain) is removed. The canister now stamps two new metadata keys on any credential verified by aDiscoverableProvider:sso_domain— thediscovery_domainthe user entered at sign-up. The canonical SSO label; always present for SSO credentials.sso_name— optional human-readable name from{domain}/.well-known/ii-openid-configuration. When the domain publishesname: "DFINITY", the access-methods list reads "DFINITY account" instead of "dfinity.org account".BE:
IIOpenIdConfigurationhop-1 schema getsname: Option<String>(with#[serde(default)]so older deployments still parse).DiscoverableProviderandDiscoveryStatecarrydiscovery_domainand adiscovered_nameref that hop-1 populates each refresh.DiscoverableProvider::verify()inserts the two keys before returning the credential.FE:
openIdNamenow resolvessso_name→sso_domain→findConfig(iss, aud, metadata).name, so an SSO-via-Google credential reads as "DFINITY" (or "dfinity.org" if the domain doesn't publish a name), not "Google".openIdLogoreturnsundefinedwhensso_domainis present (generic SSO icon).OpenIdItem.sveltedetects SSO viasso_domaindirectly, instead of the previous "no logo found" heuristic that was brittle for direct-provider credentials whose issuer didn't match anyopenid_configsentry.authFlow#continueWithSsoandaddAccessMethodFlow#linkSsoAccountboth simplify — no pre-decoding the JWT or remembering the domain locally — because the canister handles the labeling end-to-end, so the mapping survives reloads and crosses devices.Tests
cargo test -p internet_identity --bin internet_identity: 219/219 pass.cargo clippy --all-targets -D warnings: clean.cargo fmt --check: clean.npm run check(tsc + svelte-check): 0 errors (19 pre-existing warnings unchanged).npm run lint+prettier --check: clean.< Previous PR