Skip to content

Commit f07204c

Browse files
atergaclaude
andauthored
feat(be,fe): address SSO review feedback on dfinity#3785 (follow-up) (dfinity#3803)
Follow-up to dfinity#3785 addressing [@sea-snake's review](dfinity#3785 (review)). All five unresolved threads there are addressed here, and will be marked resolved on dfinity#3785 with pointers back to this PR. Stacked on top of dfinity#3785: this branch includes all commits from dfinity#3785 plus three new commits that cover the review fixes. # Changes ## 1. `Sign in with SSO` → `Continue with SSO` SSO button `aria-label` in `PickAuthenticationMethod` and `AddAccessMethod`, 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 `mapSubmitError` to user-actionable copy `SignInWithSso.svelte#mapSubmitError` used 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 is `console.error`'d unconditionally so engineers still have the stack. ## 3. Disable Continue when the SSO is already linked to this identity `SignInWithSso` takes a new optional `openIdCredentials` prop. In the add-access-method flow, `AddAccessMethodWizard` passes the identity's current credentials through; once two-hop discovery reveals the SSO's `(iss, aud)`, a `$derived` check disables the Continue button with an inline hint if that SSO is already linked. Mirrors how `AddAccessMethod.svelte` already disables direct-provider buttons for already-linked providers. Left `undefined` in the sign-in flow, where reusing an existing credential is the point. This makes reaching `OpenIdCredentialAlreadyRegistered` for *this* identity impossible, so the `OpenIdCredentialAlreadyLinkedHereError` specialization in `linkOpenIdAccount` and the corresponding toaster branch in `handleError` are 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 a `DiscoverableProvider`: - `sso_domain` — the `discovery_domain` the 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 publishes `name: "DFINITY"`, the access-methods list reads "DFINITY account" instead of "dfinity.org account". BE: - `IIOpenIdConfiguration` hop-1 schema gets `name: Option<String>` (with `#[serde(default)]` so older deployments still parse). - `DiscoverableProvider` and `DiscoveryState` carry `discovery_domain` and a `discovered_name` ref that hop-1 populates each refresh. - `DiscoverableProvider::verify()` inserts the two keys before returning the credential. FE: - `openIdName` now resolves `sso_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". - `openIdLogo` returns `undefined` when `sso_domain` is present (generic SSO icon). - `OpenIdItem.svelte` detects SSO via `sso_domain` directly, instead of the previous "no logo found" heuristic that was brittle for direct-provider credentials whose issuer didn't match any `openid_configs` entry. - `authFlow#continueWithSso` and `addAccessMethodFlow#linkSsoAccount` both 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](dfinity#3785) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent eb19dfe commit f07204c

35 files changed

Lines changed: 646 additions & 614 deletions

File tree

src/frontend/src/lib/components/ui/IdentityAvatar.svelte

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
identity.authMethod.openid.metadata !== undefined
1818
? openIdLogo(
1919
identity.authMethod.openid.iss,
20-
// `sub` and `aud` not currently tracked on `LastUsedIdentity`;
21-
// see #3795. Falls back to issuer-only matching in findConfig —
22-
// correct for direct providers, imprecise for SSO until that's
23-
// fixed.
24-
identity.authMethod.openid.sub,
20+
// `aud` not currently tracked on `LastUsedIdentity` (see
21+
// #3795) — issuer-only fallback in `findConfig` is correct
22+
// for direct providers, imprecise for SSO until that's
23+
// fixed. `ssoDomain` is unknown here for the same reason,
24+
// so SSO-linked last-used identities render with the
25+
// underlying IdP's logo.
2526
undefined,
2627
identity.authMethod.openid.metadata,
28+
undefined,
2729
)
2830
: undefined,
2931
);

src/frontend/src/lib/components/ui/ManageIdentities.svelte

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,14 @@
4444
identity.authMethod.openid.metadata !== undefined
4545
? openIdName(
4646
identity.authMethod.openid.iss,
47-
identity.authMethod.openid.sub,
48-
// `aud` not tracked on `LastUsedIdentity`; see #3795.
47+
// `aud`, `ssoName`, `ssoDomain` not tracked on
48+
// `LastUsedIdentity`; see #3795. Fall through to issuer-only
49+
// `findConfig` — correct for direct providers, imprecise for
50+
// SSO until that's fixed.
4951
undefined,
5052
identity.authMethod.openid.metadata,
53+
undefined,
54+
undefined,
5155
)
5256
: undefined}
5357
<div class="flex flex-col gap-8">

src/frontend/src/lib/components/utils/error.ts

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,7 @@ import type {
1111
OpenIdCredentialAddError,
1212
OpenIdCredentialRemoveError,
1313
} from "$lib/generated/internet_identity_types";
14-
import {
15-
OAuthProviderError,
16-
OpenIdCredentialAlreadyLinkedHereError,
17-
isOpenIdCancelError,
18-
} from "$lib/utils/openID";
14+
import { OAuthProviderError, isOpenIdCancelError } from "$lib/utils/openID";
1915
import {
2016
AuthenticationV2Events,
2117
authenticationV2Funnel,
@@ -32,30 +28,23 @@ export const handleError = (error: unknown) => {
3228
return;
3329
}
3430

35-
// Specialization of the generic `OpenIdCredentialAlreadyRegistered` error
36-
// below: the credential is already on THIS identity, not some other one.
37-
if (error instanceof OpenIdCredentialAlreadyLinkedHereError) {
38-
toaster.error({
39-
title: "This account is already linked to this identity",
40-
});
41-
return;
42-
}
43-
4431
// OAuth provider returned `error=…` in the callback fragment (RFC 6749
4532
// §4.1.2.1 / 4.2.2.1). Surface the provider's own description so a
46-
// misconfigured SSO app (e.g. Okta set to `response_types=[code]` only)
33+
// misconfigured OAuth/OpenID app (e.g. an Okta SSO set to
34+
// `response_types=[code]` only, or a botched direct-Google config)
4735
// doesn't look like an II bug. The SSO view's `mapSubmitError` gives
4836
// more specific guidance when the error hits inside `SignInWithSso`;
4937
// this branch covers callers (direct-OpenID entry points) that route
50-
// through `handleError` instead.
38+
// through `handleError` instead. Wording is provider-agnostic here
39+
// because this path handles both SSO and direct providers.
5140
if (error instanceof OAuthProviderError) {
5241
toaster.error({
53-
title: `SSO provider returned "${error.error}"`,
42+
title: `OAuth provider returned "${error.error}"`,
5443
description:
5544
error.errorDescription !== undefined &&
5645
error.errorDescription.length > 0
5746
? error.errorDescription
58-
: "Ask your SSO admin to check the app's OAuth configuration.",
47+
: "Ask your administrator to check the provider's OAuth configuration.",
5948
});
6049
return;
6150
}

src/frontend/src/lib/components/wizards/addAccessMethod/AddAccessMethodWizard.svelte

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@
100100
<SignInWithSso
101101
continueWithSso={handleContinueWithSso}
102102
goBack={addAccessMethodFlow.chooseMethod}
103+
{openIdCredentials}
103104
/>
104105
{/if}
105106

src/frontend/src/lib/components/wizards/addAccessMethod/views/AddAccessMethod.svelte

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@
125125
disabled={authenticatingProviderId !== undefined}
126126
size="xl"
127127
class="flex-1"
128-
aria-label={$t`Sign in with SSO`}
128+
aria-label={$t`Continue with SSO`}
129129
>
130130
<SsoIcon class="size-6" />
131131
</Button>

src/frontend/src/lib/components/wizards/auth/views/PickAuthenticationMethod.svelte

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@
9898
disabled={authenticatingProviderId !== undefined}
9999
size="xl"
100100
class="flex-1"
101-
aria-label={$t`Sign in with SSO`}
101+
aria-label={$t`Continue with SSO`}
102102
>
103103
<SsoIcon class="size-6" />
104104
</Button>

src/frontend/src/lib/components/wizards/auth/views/SignInWithSso.svelte

Lines changed: 84 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,27 @@
1313
} from "$lib/utils/ssoDiscovery";
1414
import type { SsoDiscoveryResult } from "$lib/utils/ssoDiscovery";
1515
import { OAuthProviderError } from "$lib/utils/openID";
16+
import type { OpenIdCredential } from "$lib/generated/internet_identity_types";
1617
import { t } from "$lib/stores/locale.store";
1718
1819
interface Props {
1920
continueWithSso: (
2021
result: SsoDiscoveryResult,
2122
) => Promise<void | "cancelled">;
2223
goBack: () => void;
24+
/**
25+
* Credentials already linked to the signed-in identity. When present,
26+
* the Continue button is disabled and an inline hint is shown if the
27+
* discovered SSO's `(iss, aud)` matches an existing credential —
28+
* mirroring how the add-access-method UI disables already-linked
29+
* direct providers (Google / Apple / Microsoft) in
30+
* `AddAccessMethod.svelte`. Leave this `undefined` in the sign-in
31+
* flow, where reusing an already-linked credential is the point.
32+
*/
33+
openIdCredentials?: OpenIdCredential[];
2334
}
2435
25-
const { continueWithSso, goBack }: Props = $props();
36+
const { continueWithSso, goBack, openIdCredentials }: Props = $props();
2637
2738
/**
2839
* Debounce delay before kicking off the (network-heavy) two-hop lookup.
@@ -44,33 +55,63 @@
4455
*/
4556
let preparedResult = $state<SsoDiscoveryResult>();
4657
58+
/**
59+
* True when `preparedResult`'s `(issuer, client_id)` already matches a
60+
* credential in `openIdCredentials` — i.e. this SSO domain is already
61+
* linked to the signed-in identity. Keeps the add-access-method Continue
62+
* button disabled with an inline hint, matching the direct-provider
63+
* behaviour in `AddAccessMethod.svelte`. Always `false` in the sign-in
64+
* flow (where `openIdCredentials` is left undefined).
65+
*/
66+
const isAlreadyLinked = $derived.by(() => {
67+
if (preparedResult === undefined || openIdCredentials === undefined) {
68+
return false;
69+
}
70+
const { discovery, clientId } = preparedResult;
71+
return openIdCredentials.some(
72+
(c) => c.iss === discovery.issuer && c.aud === clientId,
73+
);
74+
});
75+
4776
let debounceTimer: ReturnType<typeof setTimeout> | undefined;
4877
49-
const mapSubmitError = (e: unknown, domainInput: string): string => {
78+
/**
79+
* Map caught errors to user-actionable copy, or `undefined` for
80+
* "nothing useful to tell the user" — the caller will fall back to a
81+
* generic message. Raw errors are always logged to the console so
82+
* engineers have the full stack while end users see concise copy.
83+
*
84+
* Only branches that the user (or their SSO admin) can act on live
85+
* here; provider-misconfiguration details (hostname mismatch, HTTPS,
86+
* malformed discovery document) are dropped to the generic path —
87+
* they're dev-facing and the console log is the right audience.
88+
*/
89+
const mapSubmitError = (
90+
e: unknown,
91+
domainInput: string,
92+
): string | undefined => {
5093
if (e instanceof DomainNotConfiguredError) {
5194
if (e.reason === "http-error" && e.httpStatus !== undefined) {
52-
return $t`${domainInput} didn't serve /.well-known/ii-openid-configuration (HTTP ${String(e.httpStatus)}). The domain owner needs to publish it for II to sign you in.`;
95+
return $t`${domainInput} didn't publish /.well-known/ii-openid-configuration (HTTP ${String(e.httpStatus)}).`;
5396
}
5497
if (e.reason === "network") {
55-
return $t`Couldn't reach ${domainInput}. Check the spelling and your network, then try again.`;
56-
}
57-
if (e.detail !== undefined && e.detail.length > 0) {
58-
return $t`${domainInput}'s /.well-known/ii-openid-configuration is malformed: ${e.detail}`;
98+
return $t`Couldn't reach ${domainInput}. Check the spelling and your network.`;
5999
}
60100
return $t`${domainInput}'s /.well-known/ii-openid-configuration is malformed.`;
61101
}
62102
if (e instanceof OAuthProviderError) {
63-
// `unsupported_response_type` is the signature of an SSO app that
64-
// only allows the plain authorization-code flow. II needs the
65-
// hybrid flow (id_token + code) because it verifies JWTs canister-
66-
// side with no token-endpoint exchange. Spell out the fix so the
67-
// SSO admin can act on it directly.
103+
// `unsupported_response_type` = the SSO app is code-only; II needs
104+
// the hybrid flow because it verifies JWTs canister-side with no
105+
// token-endpoint exchange. Spell out the fix so the SSO admin can
106+
// act on it directly.
68107
if (e.error === "unsupported_response_type") {
69-
return $t`${domainInput}'s SSO app doesn't allow the hybrid OAuth flow II requires. Ask the SSO admin to enable response_type "id_token code" (e.g. in Okta, set the app to Single-Page Application with "Implicit (hybrid)" grant enabled).`;
108+
return $t`${domainInput}'s SSO app doesn't allow the hybrid OAuth flow II requires. Ask the SSO admin to enable response_type "id_token code".`;
70109
}
71110
if (e.error === "access_denied") {
72-
return $t`${domainInput}'s SSO denied the sign-in. Try again, and check with your SSO admin if the problem persists.`;
111+
return $t`${domainInput}'s SSO denied the sign-in.`;
73112
}
113+
// Everything else: show the provider's own words if any, else the
114+
// error code. Useful because these bubble straight from the IdP.
74115
if (e.errorDescription !== undefined && e.errorDescription.length > 0) {
75116
return $t`${domainInput}'s SSO returned "${e.error}": ${e.errorDescription}`;
76117
}
@@ -81,30 +122,28 @@
81122
if (msg.toLowerCase().includes("canary allowlist")) {
82123
return $t`SSO is not available for "${domainInput}" yet. Ask an II admin to register this domain.`;
83124
}
84-
if (msg.includes("Provider issuer hostname mismatch")) {
85-
return $t`SSO provider misconfigured: issuer doesn't match the configured hostname. (${msg})`;
86-
}
87-
if (msg.includes("Provider authorization endpoint hostname mismatch")) {
88-
return $t`SSO provider misconfigured: authorization endpoint points to a different host than the issuer. (${msg})`;
89-
}
90-
if (msg.includes("Provider issuer must use HTTPS")) {
91-
return $t`SSO provider misconfigured: issuer URL is not HTTPS. (${msg})`;
92-
}
93-
if (msg.includes("Provider authorization endpoint must use HTTPS")) {
94-
return $t`SSO provider misconfigured: authorization endpoint is not HTTPS. (${msg})`;
95-
}
96-
if (msg.startsWith("Provider discovery:")) {
97-
return $t`SSO provider's discovery document is malformed: ${msg}`;
98-
}
99125
if (msg.startsWith("Rate limited:")) {
100-
return $t`Too many recent attempts for ${domainInput}. Wait a few minutes and try again.`;
126+
return $t`Too many recent attempts for ${domainInput}. Wait a few minutes.`;
101127
}
102128
if (msg === "Too many concurrent SSO discovery requests") {
103-
return $t`Several SSO sign-ins are in flight already. Wait a moment and try again.`;
129+
return $t`Several SSO sign-ins are in flight already. Wait a moment.`;
104130
}
105-
return msg;
106131
}
107-
return $t`SSO sign-in failed. Please try again.`;
132+
return undefined;
133+
};
134+
135+
/**
136+
* Set `error` from a thrown exception: always `console.error` the raw
137+
* error (so engineers can inspect the stack / non-Error values), then
138+
* surface a user-actionable message if we have one, or a generic
139+
* fallback.
140+
*/
141+
const setErrorFrom = (e: unknown, domainInput: string) => {
142+
// eslint-disable-next-line no-console
143+
console.error("SSO sign-in failed", e);
144+
error =
145+
mapSubmitError(e, domainInput) ??
146+
$t`SSO sign-in for ${domainInput} failed. Please try again.`;
108147
};
109148
110149
const invalidatePrepared = () => {
@@ -160,7 +199,7 @@
160199
}
161200
} catch (e) {
162201
if (matchesCurrent()) {
163-
error = mapSubmitError(e, trimmed);
202+
setErrorFrom(e, trimmed);
164203
}
165204
} finally {
166205
if (matchesCurrent()) {
@@ -187,7 +226,7 @@
187226
// handler.
188227
await continueWithSso(preparedResult);
189228
} catch (e) {
190-
error = mapSubmitError(e, domain.trim().toLowerCase());
229+
setErrorFrom(e, domain.trim().toLowerCase());
191230
} finally {
192231
isSubmitting = false;
193232
}
@@ -207,7 +246,7 @@
207246
<SsoIcon class="size-5" />
208247
</FeaturedIcon>
209248
<div class="flex flex-col gap-3">
210-
<h1 class="text-2xl font-medium">{$t`Sign In With SSO`}</h1>
249+
<h1 class="text-2xl font-medium">{$t`Continue with SSO`}</h1>
211250
<p class="text-text-tertiary text-base font-medium">
212251
{$t`Enter your company domain`}
213252
</p>
@@ -236,11 +275,19 @@
236275
{error}
237276
aria-label={$t`Company domain`}
238277
/>
278+
{#if isAlreadyLinked}
279+
<p class="text-text-tertiary text-sm">
280+
{$t`This SSO is already linked to your identity.`}
281+
</p>
282+
{/if}
239283
<Button
240284
variant="primary"
241285
size="lg"
242286
type="submit"
243-
disabled={preparedResult === undefined || isSubmitting || isLookingUp}
287+
disabled={preparedResult === undefined ||
288+
isSubmitting ||
289+
isLookingUp ||
290+
isAlreadyLinked}
244291
>
245292
{#if isSubmitting}
246293
<ProgressRing />

0 commit comments

Comments
 (0)