Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nice UX when passkey is not found in that RP ID #2754

Merged
merged 2 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions src/frontend/src/components/authenticateBox/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
InvalidCaller,
LoginSuccess,
NoRegistrationFlow,
PossiblyWrongRPID,
RateLimitExceeded,
RegisterNoSpace,
UnexpectedCall,
Expand All @@ -50,6 +51,8 @@ import {
import { DerEncodedPublicKey } from "@dfinity/agent";
import { isNullish, nonNullish } from "@dfinity/utils";
import { TemplateResult, html, render } from "lit-html";
import { infoToastTemplate } from "../infoToast";
import infoToastCopy from "../infoToast/copy.json";

/** Template used for rendering specific authentication screens. See `authnScreens` below
* for meaning of "firstTime", "useExisting" and "pick". */
Expand Down Expand Up @@ -189,7 +192,12 @@ export const authenticateBoxFlow = async <I>({
loginPasskey: (
userNumber: bigint
) => Promise<
LoginSuccess | AuthFail | WebAuthnFailed | UnknownUser | ApiError
| LoginSuccess
| AuthFail
| WebAuthnFailed
| PossiblyWrongRPID
| UnknownUser
| ApiError
>;
loginPinIdentityMaterial: ({
userNumber,
Expand Down Expand Up @@ -218,6 +226,7 @@ export const authenticateBoxFlow = async <I>({
newAnchor: boolean;
authnMethod: "pin" | "passkey" | "recovery";
})
| PossiblyWrongRPID
| FlowError
| { tag: "canceled" }
| { tag: "deviceAdded" }
Expand Down Expand Up @@ -267,6 +276,7 @@ export const authenticateBoxFlow = async <I>({
newAnchor: boolean;
authnMethod: "pin" | "passkey" | "recovery";
})
| PossiblyWrongRPID
| FlowError
| { tag: "canceled" }
| { tag: "deviceAdded" }
Expand Down Expand Up @@ -345,7 +355,7 @@ export type FlowError =
| RegisterNoSpace;

export const handleLoginFlowResult = async <E>(
result: (LoginSuccess & E) | FlowError
result: (LoginSuccess & E) | PossiblyWrongRPID | FlowError
): Promise<
({ userNumber: bigint; connection: AuthenticatedConnection } & E) | undefined
> => {
Expand All @@ -354,6 +364,21 @@ export const handleLoginFlowResult = async <E>(
return result;
}

if (result.kind === "possiblyWrongRPID") {
const i18n = new I18n();
const copy = i18n.i18n(infoToastCopy);
toast.info(
infoToastTemplate({
title: copy.title_possibly_wrong_rp_id,
messages: [
copy.message_possibly_wrong_rp_id_1,
copy.message_possibly_wrong_rp_id_2,
],
})
);
return undefined;
}

result satisfies FlowError;

toast.error(flowErrorToastTemplate(result));
Expand Down Expand Up @@ -657,7 +682,12 @@ const useIdentityFlow = async <I>({
loginPasskey: (
userNumber: bigint
) => Promise<
LoginSuccess | AuthFail | WebAuthnFailed | UnknownUser | ApiError
| LoginSuccess
| AuthFail
| WebAuthnFailed
| PossiblyWrongRPID
| UnknownUser
| ApiError
>;
allowPinLogin: boolean;
verifyPinValidity: (opts: {
Expand All @@ -680,6 +710,7 @@ const useIdentityFlow = async <I>({
})
| AuthFail
| WebAuthnFailed
| PossiblyWrongRPID
| UnknownUser
| ApiError
| BadPin
Expand Down
7 changes: 7 additions & 0 deletions src/frontend/src/components/infoToast/copy.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"en": {
"title_possibly_wrong_rp_id": "Please try again",
"message_possibly_wrong_rp_id_1": "The wrong domain was set for the passkey and the browser couldn't find it.",
"message_possibly_wrong_rp_id_2": "Try again and another domain will be used."
}
}
16 changes: 16 additions & 0 deletions src/frontend/src/components/infoToast/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { DynamicKey } from "$src/i18n";
import { html } from "lit-html";

export const infoToastTemplate = ({
title,
messages,
}: {
title: string | DynamicKey;
messages: string[] | DynamicKey[];
}) => html`
<h3 class="t-title c-card__title">${title}</h3>
${messages.map(
(message) =>
html`<p data-role="info-message" class="t-paragraph">${message}</p>`
)}
`;
20 changes: 20 additions & 0 deletions src/frontend/src/flows/recovery/recoverWith/device.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { CredentialId, DeviceData } from "$generated/internet_identity_types";
import { infoToastTemplate } from "$src/components/infoToast";
import infoToastCopy from "$src/components/infoToast/copy.json";
import { promptUserNumberTemplate } from "$src/components/promptUserNumber";
import { toast } from "$src/components/toast";
import { I18n } from "$src/i18n";
import { convertToCredentialData } from "$src/utils/credential-devices";
import {
AuthFail,
Connection,
LoginSuccess,
PossiblyWrongRPID,
WebAuthnFailed,
} from "$src/utils/iiConnection";
import { renderPage } from "$src/utils/lit-html";
Expand Down Expand Up @@ -50,6 +54,21 @@ export const recoverWithDevice = ({
}

if (result.kind !== "loginSuccess") {
if (result.kind === "possiblyWrongRPID") {
const i18n = new I18n();
const copy = i18n.i18n(infoToastCopy);
toast.info(
infoToastTemplate({
title: copy.title_possibly_wrong_rp_id,
messages: [
copy.message_possibly_wrong_rp_id_1,
copy.message_possibly_wrong_rp_id_2,
],
})
);
return;
}

result satisfies AuthFail | WebAuthnFailed;
toast.error("Could not authenticate using the device");
return;
Expand All @@ -72,6 +91,7 @@ const attemptRecovery = async ({
}): Promise<
| LoginSuccess
| WebAuthnFailed
| PossiblyWrongRPID
| AuthFail
| { kind: "noRecovery" }
| { kind: "tooManyRecovery" }
Expand Down
106 changes: 103 additions & 3 deletions src/frontend/src/utils/iiConnection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ describe("Connection.login", () => {
}
});

it("connection exludes rpId when user cancels", async () => {
it("connection excludes rpId when user cancels", async () => {
// This one would fail because it's not the device the user is using at the moment.
const currentOriginDevice: DeviceData = createMockDevice(currentOrigin);
const currentOriginCredentialData =
Expand All @@ -193,7 +193,7 @@ describe("Connection.login", () => {
failSign = true;
const firstLoginResult = await connection.login(BigInt(12345));

expect(firstLoginResult.kind).toBe("webAuthnFailed");
expect(firstLoginResult.kind).toBe("possiblyWrongRPID");
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledTimes(1);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledWith(
expect.arrayContaining([
Expand Down Expand Up @@ -327,7 +327,107 @@ describe("Connection.login", () => {
}
});

it("connection does not exlud rpId when user cancels", async () => {
it("connection does not exclude rpId when user cancels", async () => {
const currentOriginDevice: DeviceData = createMockDevice(currentOrigin);
const currentOriginCredentialData =
convertToCredentialData(currentOriginDevice);
const currentDevice: DeviceData = createMockDevice();
const currentDeviceCredentialData =
convertToCredentialData(currentDevice);
const mockActor = {
identity_info: vi.fn().mockResolvedValue({ Ok: { metadata: [] } }),
lookup: vi.fn().mockResolvedValue([currentOriginDevice, currentDevice]),
} as unknown as ActorSubclass<_SERVICE>;
const connection = new Connection("aaaaa-aa", mockActor);

failSign = true;
const firstLoginResult = await connection.login(BigInt(12345));

expect(firstLoginResult.kind).toBe("webAuthnFailed");
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledTimes(1);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledWith(
expect.arrayContaining([
currentOriginCredentialData,
currentDeviceCredentialData,
]),
undefined
);

failSign = false;
const secondLoginResult = await connection.login(BigInt(12345));

expect(secondLoginResult.kind).toBe("loginSuccess");
if (secondLoginResult.kind === "loginSuccess") {
expect(secondLoginResult.connection).toBeInstanceOf(
AuthenticatedConnection
);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledTimes(2);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenNthCalledWith(
2,
expect.arrayContaining([
currentDeviceCredentialData,
currentOriginCredentialData,
]),
undefined
);
}
});
});

describe("domains compatibility flag enabled and browser doesn't support", () => {
beforeEach(() => {
DOMAIN_COMPATIBILITY.set(true);
vi.stubGlobal("navigator", {
// Does NOT Supports RoR
userAgent:
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:133.0) Gecko/20100101 Firefox/133.0",
});
});

it("login returns authenticated connection without rpID if browser doesn't support it", async () => {
const connection = new Connection("aaaaa-aa", mockActor);

const loginResult = await connection.login(BigInt(12345));

expect(loginResult.kind).toBe("loginSuccess");
if (loginResult.kind === "loginSuccess") {
expect(loginResult.connection).toBeInstanceOf(AuthenticatedConnection);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledTimes(1);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledWith(
[convertToCredentialData(mockDevice)],
undefined
);
}
});
});

describe("domains compatibility flag disabled", () => {
beforeEach(() => {
DOMAIN_COMPATIBILITY.set(false);
vi.stubGlobal("navigator", {
// Supports RoR
userAgent:
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.0 Safari/605.1.15",
});
});

it("login returns authenticated connection without rpID if flag is not enabled", async () => {
const connection = new Connection("aaaaa-aa", mockActor);

const loginResult = await connection.login(BigInt(12345));

expect(loginResult.kind).toBe("loginSuccess");
if (loginResult.kind === "loginSuccess") {
expect(loginResult.connection).toBeInstanceOf(AuthenticatedConnection);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledTimes(1);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledWith(
[convertToCredentialData(mockDevice)],
undefined
);
}
});

it("connection does not exclude rpId when user cancels", async () => {
const currentOriginDevice: DeviceData = createMockDevice(currentOrigin);
const currentOriginCredentialData =
convertToCredentialData(currentOriginDevice);
Expand Down
12 changes: 10 additions & 2 deletions src/frontend/src/utils/iiConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ export type RegisterNoSpace = { kind: "registerNoSpace" };
export type NoSeedPhrase = { kind: "noSeedPhrase" };
export type SeedPhraseFail = { kind: "seedPhraseFail" };
export type WebAuthnFailed = { kind: "webAuthnFailed" };
export type PossiblyWrongRPID = { kind: "possiblyWrongRPID" };
export type InvalidAuthnMethod = {
kind: "invalidAuthnMethod";
message: string;
Expand Down Expand Up @@ -360,7 +361,12 @@ export class Connection {
login = async (
userNumber: bigint
): Promise<
LoginSuccess | AuthFail | WebAuthnFailed | UnknownUser | ApiError
| LoginSuccess
| AuthFail
| WebAuthnFailed
| PossiblyWrongRPID
| UnknownUser
| ApiError
> => {
let devices: Omit<DeviceData, "alias">[];
try {
Expand All @@ -386,7 +392,7 @@ export class Connection {
fromWebauthnCredentials = async (
userNumber: bigint,
credentials: CredentialData[]
): Promise<LoginSuccess | WebAuthnFailed | AuthFail> => {
): Promise<LoginSuccess | WebAuthnFailed | PossiblyWrongRPID | AuthFail> => {
const cancelledRpIds = this._cancelledRpIds.get(userNumber) ?? new Set();
const currentOrigin = window.location.origin;
const dynamicRPIdEnabled =
Expand Down Expand Up @@ -426,6 +432,8 @@ export class Connection {
} else {
this._cancelledRpIds.set(userNumber, new Set([rpId]));
}
// We want to user to retry again and a new RP ID will be used.
return { kind: "possiblyWrongRPID" };
}
return { kind: "webAuthnFailed" };
}
Expand Down
Loading