Skip to content

Commit 44de286

Browse files
authored
test: Fix flaky e2e test with passkeys (dfinity#3700)
Fix flaky e2e test with passkeys: - `addAuthenticatorForIdentity` removes existing authenticators on the page before creating a new one. - New `discoverableCredentialId` on `Identity` controls which credential is resident in discoverable flows through `setDiscoverableCredentialForIdentity`. - `setCredentialsForIdentity` and `removeAuthenticatorForIdentity` no longer take `page`, they sync all pages via `Map` (was `WeakMap`). - Stale page references handled via try/catch.
1 parent 81af29d commit 44de286

4 files changed

Lines changed: 89 additions & 67 deletions

File tree

src/frontend/tests/e2e-playwright/fixtures/identity.ts

Lines changed: 83 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ interface Identity {
2828
identityNumber: bigint;
2929
name: string;
3030
credentials: Protocol.WebAuthn.Credential[];
31-
authenticatorIds: WeakMap<Page, string>;
31+
discoverableCredentialId: string | undefined;
32+
authenticatorIds: Map<Page, string>;
3233
}
3334

3435
const credentialCreateTrackingInstalled = new WeakSet<Page>();
@@ -280,15 +281,15 @@ export const test = base.extend<{
280281
page: Page,
281282
identityNumber: bigint,
282283
) => Promise<void>;
283-
removeAuthenticatorForIdentity: (
284-
page: Page,
285-
identityNumber: bigint,
286-
) => Promise<void>;
284+
removeAuthenticatorForIdentity: (identityNumber: bigint) => Promise<void>;
287285
setCredentialsForIdentity: (
288-
page: Page,
289286
identityNumber: bigint,
290287
credentials: Protocol.WebAuthn.Credential[],
291288
) => Promise<void>;
289+
setDiscoverableCredentialForIdentity: (
290+
identityNumber: bigint,
291+
credentialId: string,
292+
) => Promise<void>;
292293
}>({
293294
identityConfig: {
294295
createIdentities: [{ name: DEFAULT_NAME }],
@@ -327,8 +328,9 @@ export const test = base.extend<{
327328
canisterId,
328329
name,
329330
credentials,
331+
discoverableCredentialId: credentials[0].credentialId,
330332
identityNumber,
331-
authenticatorIds: new WeakMap(),
333+
authenticatorIds: new Map(),
332334
});
333335
}
334336
return use(identities);
@@ -339,70 +341,97 @@ export const test = base.extend<{
339341
const wizard = new IdentityWizard(page);
340342
await wizard.signIn();
341343
}),
342-
addAuthenticatorForIdentity: ({ identities }, use) =>
344+
addAuthenticatorForIdentity: (
345+
{ identities, setCredentialsForIdentity },
346+
use,
347+
) =>
343348
use(async (page, identityNumber) => {
344349
const identity = getIdentityByNumber(identities, identityNumber);
345350

351+
// Remove any existing authenticators on this page to avoid conflicts
352+
for (const otherIdentity of identities) {
353+
const existingAuthenticatorId =
354+
otherIdentity.authenticatorIds.get(page);
355+
if (existingAuthenticatorId !== undefined) {
356+
await removeVirtualAuthenticator(page, existingAuthenticatorId);
357+
otherIdentity.authenticatorIds.delete(page);
358+
}
359+
}
360+
346361
// Add virtual authenticator and populate it with the identity's credentials
347362
const authenticatorId = await addVirtualAuthenticator(page);
348363
identity.authenticatorIds.set(page, authenticatorId);
349-
for (const credential of identity.credentials) {
350-
await addCredentialToVirtualAuthenticator(
351-
page,
352-
authenticatorId,
353-
credential,
354-
);
355-
}
364+
await setCredentialsForIdentity(identityNumber, identity.credentials);
356365

357366
// Keep this identity synchronized with passkeys created in page code.
358367
await trackIdentityCredentialCreation(page, identity);
359368
}),
360369
removeAuthenticatorForIdentity: ({ identities }, use) =>
361-
use(async (page, identityNumber) => {
370+
use(async (identityNumber) => {
362371
const identity = getIdentityByNumber(identities, identityNumber);
363-
const authenticatorId = identity.authenticatorIds.get(page);
364-
if (authenticatorId === undefined) {
365-
throw new Error(
366-
`No authenticator found for identity ${identityNumber} on the provided page`,
367-
);
372+
for (const [page, authenticatorId] of identity.authenticatorIds) {
373+
try {
374+
await removeVirtualAuthenticator(page, authenticatorId);
375+
} catch (error) {
376+
// Page may have been closed; only swallow errors in that case.
377+
if (!page.isClosed()) {
378+
throw error;
379+
}
380+
} finally {
381+
// Ensure our internal map does not get out of sync.
382+
identity.authenticatorIds.delete(page);
383+
}
368384
}
369-
await removeVirtualAuthenticator(page, authenticatorId);
370-
identity.authenticatorIds.delete(page);
371385
}),
372386
setCredentialsForIdentity: ({ identities }, use) =>
373-
use(async (page, identityNumber, credentials) => {
387+
use(async (identityNumber, credentials) => {
374388
const identity = getIdentityByNumber(identities, identityNumber);
375-
const authenticatorId = identity.authenticatorIds.get(page);
376-
if (authenticatorId !== undefined) {
377-
const existingCredentialIds = new Set(
378-
identity.credentials.map((cred) => cred.credentialId),
379-
);
380-
const newCredentialIds = new Set(
381-
credentials.map((cred) => cred.credentialId),
382-
);
383-
const credentialToRemove = identity.credentials.filter(
384-
(existingCredential) =>
385-
!newCredentialIds.has(existingCredential.credentialId),
386-
);
387-
const credentialToAdd = credentials.filter(
388-
(newCredential) =>
389-
!existingCredentialIds.has(newCredential.credentialId),
390-
);
391-
for (const credential of credentialToRemove) {
392-
await removeCredentialFromVirtualAuthenticator(
393-
page,
394-
authenticatorId,
395-
credential.credentialId,
396-
);
397-
}
398-
for (const credential of credentialToAdd) {
399-
await addCredentialToVirtualAuthenticator(
400-
page,
401-
authenticatorId,
402-
credential,
403-
);
404-
}
389+
// Update discoverable choice if the current one is no longer present
390+
const hasDiscoverable = credentials.some(
391+
(cred) => cred.credentialId === identity.discoverableCredentialId,
392+
);
393+
if (!hasDiscoverable) {
394+
identity.discoverableCredentialId = credentials[0]?.credentialId;
405395
}
406396
identity.credentials = credentials;
397+
398+
// Sync all authenticators for this identity
399+
for (const [page, authenticatorId] of identity.authenticatorIds) {
400+
try {
401+
const existingCredentials =
402+
await getCredentialsFromVirtualAuthenticator(page, authenticatorId);
403+
for (const credential of existingCredentials) {
404+
await removeCredentialFromVirtualAuthenticator(
405+
page,
406+
authenticatorId,
407+
credential.credentialId,
408+
);
409+
}
410+
for (const credential of credentials) {
411+
await addCredentialToVirtualAuthenticator(page, authenticatorId, {
412+
...credential,
413+
isResidentCredential:
414+
credential.credentialId === identity.discoverableCredentialId,
415+
});
416+
}
417+
} catch (error) {
418+
// Only ignore errors when the page has been closed; otherwise rethrow
419+
if (page.isClosed()) {
420+
// Page may have been closed; clean up stale reference
421+
identity.authenticatorIds.delete(page);
422+
} else {
423+
throw error;
424+
}
425+
}
426+
}
427+
}),
428+
setDiscoverableCredentialForIdentity: (
429+
{ identities, setCredentialsForIdentity },
430+
use,
431+
) =>
432+
use(async (identityNumber, credentialId) => {
433+
const identity = getIdentityByNumber(identities, identityNumber);
434+
identity.discoverableCredentialId = credentialId;
435+
await setCredentialsForIdentity(identityNumber, identity.credentials);
407436
}),
408437
});

src/frontend/tests/e2e-playwright/routes/index.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ test.describe("Last used identities listed", () => {
290290
await page.goto(II_URL);
291291
await signInWithIdentity(page, identities[0].identityNumber);
292292
await managePage.signOut();
293-
await removeAuthenticatorForIdentity(page, identities[0].identityNumber);
293+
await removeAuthenticatorForIdentity(identities[0].identityNumber);
294294

295295
// Now sign up for a new identity
296296
await addVirtualAuthenticator(page);

src/frontend/tests/e2e-playwright/routes/manage/access.spec.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ test.describe("Access methods", () => {
3232
expect(newCredential).toBeDefined();
3333

3434
// Verify we can still sign in with the existing passkey
35-
await setCredentialsForIdentity(page, identities[0].identityNumber, [
35+
await setCredentialsForIdentity(identities[0].identityNumber, [
3636
existingCredential,
3737
]);
3838
await managePage.signOut();
@@ -42,7 +42,7 @@ test.describe("Access methods", () => {
4242

4343
// Verify we can now also sign in with the new passkey
4444
await managePage.signOut();
45-
await setCredentialsForIdentity(page, identities[0].identityNumber, [
45+
await setCredentialsForIdentity(identities[0].identityNumber, [
4646
newCredential,
4747
]);
4848
await manageAccessPage.goto();
@@ -132,7 +132,6 @@ test.describe("Access methods", () => {
132132

133133
// Remove the credential corresponding to the removed passkey
134134
await setCredentialsForIdentity(
135-
page,
136135
identities[0].identityNumber,
137136
identities[0].credentials.slice(1),
138137
);

src/frontend/tests/e2e-playwright/routes/manage/index.spec.ts

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,10 @@ test.describe("Dashboard Navigation", () => {
4545
await page.goto(II_URL);
4646
await signInWithIdentity(page, identities[1].identityNumber);
4747
await managePage.signOut();
48-
await removeAuthenticatorForIdentity(
49-
page,
50-
identities[1].identityNumber,
51-
);
48+
await removeAuthenticatorForIdentity(identities[1].identityNumber);
5249
await signInWithIdentity(page, identities[0].identityNumber);
5350
await managePage.signOut();
54-
await removeAuthenticatorForIdentity(
55-
page,
56-
identities[0].identityNumber,
57-
);
51+
await removeAuthenticatorForIdentity(identities[0].identityNumber);
5852
},
5953
);
6054

@@ -89,7 +83,7 @@ test.describe("Dashboard Navigation", () => {
8983
await page.getByRole("link", { name: "Access and recovery" }).click();
9084

9185
// Switch to second identity
92-
await removeAuthenticatorForIdentity(page, identities[0].identityNumber);
86+
await removeAuthenticatorForIdentity(identities[0].identityNumber);
9387
await addAuthenticatorForIdentity(page, identities[1].identityNumber);
9488
await page.getByRole("button", { name: "Switch identity" }).click();
9589
await page.getByRole("button", { name: identities[1].name }).click();

0 commit comments

Comments
 (0)