Skip to content

Commit e580440

Browse files
authored
Merge feat/openid-sso-cache-verify into feat/sso-allow-any-domain
Bring in the 3 base commits (incl. the TS2367 SSO discovery abort fix) that landed on #4022 after this branch was cut.
2 parents 468189c + 677d6e3 commit e580440

5 files changed

Lines changed: 91 additions & 25 deletions

File tree

src/frontend/src/lib/utils/openidPoll.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,27 @@
99
export const POLL_INTERVAL_MS = 500;
1010
export const MAX_POLL_ATTEMPTS = 60;
1111

12-
export const pollDelay = (): Promise<void> =>
13-
new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS));
12+
/**
13+
* Sleep one poll interval. Resolves early when `signal` aborts so the caller's
14+
* next abort check runs without waiting out the full delay (and, in turn,
15+
* without firing another canister call for an already-cancelled lookup).
16+
*/
17+
export const pollDelay = (signal?: AbortSignal): Promise<void> =>
18+
new Promise((resolve) => {
19+
if (signal?.aborted === true) {
20+
resolve();
21+
return;
22+
}
23+
const onAbort = (): void => {
24+
clearTimeout(timer);
25+
resolve();
26+
};
27+
const timer = setTimeout(() => {
28+
signal?.removeEventListener("abort", onAbort);
29+
resolve();
30+
}, POLL_INTERVAL_MS);
31+
signal?.addEventListener("abort", onAbort, { once: true });
32+
});
1433

1534
/**
1635
* Retry an update call that reports the top-level `Pending` arm while SSO

src/frontend/src/lib/utils/ssoDiscovery.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,5 +152,32 @@ describe("ssoDiscovery", () => {
152152
discoverSsoConfig("dfinity.org", controller.signal),
153153
).rejects.toThrow("aborted");
154154
});
155+
156+
it("aborting mid-sleep stops the poll without firing another update", async () => {
157+
vi.useFakeTimers();
158+
vi.mocked(anonymousActor.get_sso_discovery).mockResolvedValue({
159+
Pending: null,
160+
});
161+
vi.mocked(anonymousActor.discover_sso).mockResolvedValue(undefined);
162+
163+
const controller = new AbortController();
164+
const settled = discoverSsoConfig("dfinity.org", controller.signal).catch(
165+
(e: unknown) => e,
166+
);
167+
168+
// First iteration: query reads Pending → one update → parked in the sleep.
169+
await vi.advanceTimersByTimeAsync(0);
170+
expect(anonymousActor.discover_sso).toHaveBeenCalledTimes(1);
171+
172+
// Abort during the 500ms sleep: it resolves early and the loop-top check
173+
// throws before a second query/update can fire.
174+
controller.abort();
175+
await vi.advanceTimersByTimeAsync(0);
176+
177+
const error = await settled;
178+
expect(error).toBeInstanceOf(Error);
179+
expect((error as Error).message).toContain("aborted");
180+
expect(anonymousActor.discover_sso).toHaveBeenCalledTimes(1);
181+
});
155182
});
156183
});

src/frontend/src/lib/utils/ssoDiscovery.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,12 @@ const toResult = (discovery: SsoDiscovery): SsoDiscoveryResult => ({
108108
},
109109
});
110110

111+
// Wrap the abort check in a function so each call returns a fresh `boolean`.
112+
// Reading `signal?.aborted` inline narrows it to `false` for the rest of the
113+
// iteration, and TypeScript doesn't re-widen across the `await`s — so a second
114+
// inline check on the same `signal` is flagged as always-false (TS2367).
115+
const isAborted = (signal?: AbortSignal): boolean => signal?.aborted === true;
116+
111117
/**
112118
* Resolve a domain's SSO configuration. Validates the domain, then polls
113119
* `get_sso_discovery` (query) for the state; on `Pending` it drives the fetch
@@ -125,7 +131,7 @@ export const discoverSsoConfig = async (
125131
const validatedDomain = validateDomain(domain);
126132

127133
for (let attempt = 0; attempt < MAX_POLL_ATTEMPTS; attempt++) {
128-
if (signal?.aborted === true) {
134+
if (isAborted(signal)) {
129135
throw new Error("SSO discovery aborted");
130136
}
131137
// Read the discovery state via the cheap query.
@@ -136,9 +142,16 @@ export const discoverSsoConfig = async (
136142
if ("NotAllowed" in state) {
137143
throw new DomainNotConfiguredError("rejected");
138144
}
139-
// Pending — drive the fetch with an update, then poll again.
145+
// Re-check before the update: the query above may have spanned an abort,
146+
// and we don't want to drive a fetch for a lookup the user already dropped.
147+
if (isAborted(signal)) {
148+
throw new Error("SSO discovery aborted");
149+
}
150+
// Pending — drive the fetch with an update, then poll again. The sleep is
151+
// abortable so a mid-delay abort skips straight to the next iteration's
152+
// check instead of firing another query.
140153
await anonymousActor.discover_sso(validatedDomain);
141-
await pollDelay();
154+
await pollDelay(signal);
142155
}
143156

144157
throw new DomainNotConfiguredError("timeout");

src/internet_identity/src/main.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1348,7 +1348,7 @@ mod openid_api {
13481348
) -> OpenIdResult<IdRegFinishResult, IdRegFinishError> {
13491349
// Canonicalize the untrusted discovery domain at the boundary so both
13501350
// verification and the credential stored from `arg` see the same value.
1351-
arg.discovery_domain = openid::canonical_discovery_domain(arg.discovery_domain);
1351+
arg.discovery_domain = openid::canonical_discovery_domain_opt(arg.discovery_domain);
13521352
// Verify the JWT (driving the SSO discovery/JWKS fetches it may need)
13531353
// up front: a cold or evicted cache surfaces as the `Pending` retry arm
13541354
// instead of a terminal registration error. The verified credential is
@@ -1375,7 +1375,7 @@ mod openid_api {
13751375
salt: [u8; 32],
13761376
discovery_domain: Option<String>,
13771377
) -> OpenIdResult<(), OpenIdCredentialAddError> {
1378-
let discovery_domain = openid::canonical_discovery_domain(discovery_domain);
1378+
let discovery_domain = openid::canonical_discovery_domain_opt(discovery_domain);
13791379
openid::prefetch_sso(discovery_domain.as_deref());
13801380
let openid_credential = match openid::verify_jwt(&jwt, &salt, discovery_domain.as_deref()) {
13811381
Ok(openid::Cached::Ready(credential)) => credential,
@@ -1428,7 +1428,7 @@ mod openid_api {
14281428
// read the result. A cold cache reads `Pending`; the frontend polls
14291429
// `openid_get_delegation` and re-calls this until the delegation is
14301430
// ready.
1431-
let discovery_domain = openid::canonical_discovery_domain(discovery_domain);
1431+
let discovery_domain = openid::canonical_discovery_domain_opt(discovery_domain);
14321432
openid::prefetch_sso(discovery_domain.as_deref());
14331433
let openid_credential = match openid::verify_jwt(&jwt, &salt, discovery_domain.as_deref()) {
14341434
Ok(openid::Cached::Ready(credential)) => credential,
@@ -1490,7 +1490,7 @@ mod openid_api {
14901490
// caches. A `Pending` means discovery/JWKS isn't cached yet — the
14911491
// frontend re-calls `openid_prepare_delegation` (an update, which drives
14921492
// the fetch) and polls this again.
1493-
let discovery_domain = openid::canonical_discovery_domain(discovery_domain);
1493+
let discovery_domain = openid::canonical_discovery_domain_opt(discovery_domain);
14941494
let openid_credential = match openid::verify_jwt(&jwt, &salt, discovery_domain.as_deref()) {
14951495
Ok(openid::Cached::Ready(credential)) => credential,
14961496
Ok(openid::Cached::Pending) => return OpenIdResult::Pending,
@@ -1517,7 +1517,7 @@ mod openid_api {
15171517
/// query until it returns `Resolved`.
15181518
#[update]
15191519
fn discover_sso(domain: String) {
1520-
openid::discover_sso(&domain)
1520+
openid::discover_sso(&openid::canonical_discovery_domain(&domain))
15211521
}
15221522

15231523
/// Read the state of `domain`'s SSO discovery: `Resolved` with the config,
@@ -1526,7 +1526,7 @@ mod openid_api {
15261526
fn get_sso_discovery(
15271527
domain: String,
15281528
) -> internet_identity_interface::internet_identity::types::SsoDiscoveryState {
1529-
openid::get_sso_discovery(&domain)
1529+
openid::get_sso_discovery(&openid::canonical_discovery_domain(&domain))
15301530
}
15311531
}
15321532

src/internet_identity/src/openid.rs

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -286,12 +286,18 @@ pub fn setup(configs: Vec<OpenIdConfig>) {
286286
/// Canonicalize an untrusted SSO discovery domain received as a canister-call
287287
/// argument: trim surrounding whitespace and lowercase ASCII. Domains are
288288
/// case-insensitive (DNS), but the value is stamped onto the credential as
289-
/// `sso_domain` and used as the equality key for `sso:<domain>` scope routing,
290-
/// so it must be canonical the moment it crosses the trust boundary. Endpoints
291-
/// taking a `discovery_domain` run this before any further use, matching the
292-
/// `sso_discoverable_domains` config setter and the `get_sso_discovery` reply.
293-
pub fn canonical_discovery_domain(domain: Option<String>) -> Option<String> {
294-
domain.map(|domain| domain.trim().to_ascii_lowercase())
289+
/// `sso_domain` and used as the equality key for `sso:<domain>` scope routing
290+
/// and the allowlist gate, so it must be canonical the moment it crosses the
291+
/// trust boundary. Every endpoint taking a `discovery_domain` runs this before
292+
/// any further use, matching the `sso_discoverable_domains` config setter.
293+
pub fn canonical_discovery_domain(domain: &str) -> String {
294+
domain.trim().to_ascii_lowercase()
295+
}
296+
297+
/// [`canonical_discovery_domain`] over an optional argument (the JWT endpoints,
298+
/// where the domain selects an SSO vs configured provider).
299+
pub fn canonical_discovery_domain_opt(domain: Option<String>) -> Option<String> {
300+
domain.map(|domain| canonical_discovery_domain(&domain))
295301
}
296302

297303
/// Drive the on-demand SSO discovery / JWKS fetches for `domain` forward. Only
@@ -550,17 +556,18 @@ mod tests {
550556
fn canonical_discovery_domain_trims_and_lowercases() {
551557
// Untrusted canister-call args: a mixed-case / padded domain that
552558
// passes the case-insensitive allowlist gate must be canonicalized so
553-
// the stamped `sso:<domain>` scope matches the allowlisted value.
554-
assert_eq!(
555-
canonical_discovery_domain(Some(" Example.ORG ".to_string())),
556-
Some("example.org".to_string())
557-
);
559+
// the stamped `sso:<domain>` scope matches the allowlisted value, and
560+
// so the discovery endpoints (`discover_sso` / `get_sso_discovery`) gate
561+
// on the same canonical form as the JWT endpoints.
562+
assert_eq!(canonical_discovery_domain(" Example.ORG "), "example.org");
563+
assert_eq!(canonical_discovery_domain("example.org"), "example.org");
564+
// Optional wrapper for the JWT endpoints: a configured provider supplies
565+
// no domain.
558566
assert_eq!(
559-
canonical_discovery_domain(Some("example.org".to_string())),
567+
canonical_discovery_domain_opt(Some(" Example.ORG ".to_string())),
560568
Some("example.org".to_string())
561569
);
562-
// A configured provider supplies no domain.
563-
assert_eq!(canonical_discovery_domain(None), None);
570+
assert_eq!(canonical_discovery_domain_opt(None), None);
564571
}
565572

566573
#[test]

0 commit comments

Comments
 (0)