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

Use new warning page and change the rendering logic #2551

Merged
merged 9 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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
204 changes: 204 additions & 0 deletions src/frontend/src/flows/recovery/recoveryWizard.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
import {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests! 👍

AnchorCredentials,
CredentialId,
PublicKey,
WebAuthnCredential,
} from "$generated/internet_identity_types";
import { PinIdentityMaterial } from "../pin/idb";
import { getDevicesStatus } from "./recoveryWizard";

const ONE_WEEK_MILLIS = 7 * 24 * 60 * 60 * 1000;
const nowInMillis = 1722259851155;
const moreThanAWeekAgo = nowInMillis - ONE_WEEK_MILLIS - 1;
const lessThanAWeekAgo = nowInMillis - 1;

const pinIdentityMaterial: PinIdentityMaterial =
{} as unknown as PinIdentityMaterial;

const noCredentials: AnchorCredentials = {
credentials: [],
recovery_credentials: [],
recovery_phrases: [],
};

const device: WebAuthnCredential = {
pubkey: [] as PublicKey,
credential_id: [] as CredentialId,
};

const oneDeviceOnly: AnchorCredentials = {
credentials: [device],
recovery_credentials: [],
recovery_phrases: [],
};

const oneRecoveryDeviceOnly: AnchorCredentials = {
credentials: [],
recovery_credentials: [device],
recovery_phrases: [],
};

const oneDeviceAndPhrase: AnchorCredentials = {
credentials: [device],
recovery_credentials: [],
recovery_phrases: [[] as PublicKey],
};

const twoDevices: AnchorCredentials = {
credentials: [device, { ...device }],
recovery_credentials: [],
recovery_phrases: [[] as PublicKey],
};

const threeDevices: AnchorCredentials = {
credentials: [device, { ...device }, { ...device }],
recovery_credentials: [],
recovery_phrases: [[] as PublicKey],
};

const oneNormalOneRecovery: AnchorCredentials = {
credentials: [device],
recovery_credentials: [device],
recovery_phrases: [[] as PublicKey],
};

test("getDevicesStatus returns 'pin-only' for user with pin and has seen recovery longer than a week ago", () => {
expect(
getDevicesStatus({
credentials: noCredentials,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial,
nowInMillis,
})
).toBe("pin-only");
});

test("getDevicesStatus returns 'one-device' for user with one passkey and has seen recovery longer than a week ago", () => {
expect(
getDevicesStatus({
credentials: oneDeviceOnly,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("one-device");
});

test("getDevicesStatus returns true for user with one passkey and empty identity metadata", () => {
expect(
getDevicesStatus({
credentials: oneDeviceOnly,
identityMetadata: {},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("one-device");
});

test("getDevicesStatus returns 'one-device' for user with one recovery device and has seen recovery longer than a week ago", () => {
expect(
getDevicesStatus({
credentials: oneRecoveryDeviceOnly,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("one-device");
});

test("getDevicesStatus returns 'one-device' for user with one device and a recovery phrase", () => {
expect(
getDevicesStatus({
credentials: oneDeviceAndPhrase,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("one-device");
});

test("getDevicesStatus returns 'no-warning' for user with pin that has disabled the warning", () => {
expect(
getDevicesStatus({
credentials: noCredentials,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
doNotShowRecoveryPageRequestTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial,
nowInMillis,
})
).toBe("no-warning");
});

test("getDevicesStatus returns 'no-warning' for user with one device that has disabled the warning", () => {
expect(
getDevicesStatus({
credentials: oneDeviceOnly,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
doNotShowRecoveryPageRequestTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("no-warning");
});

test("getDevicesStatus returns 'no-warning' for user with two devices", () => {
expect(
getDevicesStatus({
credentials: twoDevices,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("no-warning");
});

test("getDevicesStatus returns 'no-warning' for user with one normal device and a recovery device", () => {
expect(
getDevicesStatus({
credentials: oneNormalOneRecovery,
identityMetadata: {
recoveryPageShownTimestampMillis: moreThanAWeekAgo,
},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("no-warning");
});

test("getDevicesStatus returns 'no-warning' for user with more than two devices and empty identity metadata", () => {
expect(
getDevicesStatus({
credentials: threeDevices,
identityMetadata: {},
pinIdentityMaterial: undefined,
nowInMillis,
})
).toBe("no-warning");
});

test("getDevicesStatus returns 'no-warning' for user with pin and has seen recovery less than a week ago", () => {
expect(
getDevicesStatus({
credentials: noCredentials,
identityMetadata: {
recoveryPageShownTimestampMillis: lessThanAWeekAgo,
},
pinIdentityMaterial,
nowInMillis,
})
).toBe("no-warning");
});
136 changes: 116 additions & 20 deletions src/frontend/src/flows/recovery/recoveryWizard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,16 @@ import { renderPage } from "$src/utils/lit-html";
import { TemplateResult } from "lit-html";

import { AuthenticatedConnection } from "$src/utils/iiConnection";
import { setupRecovery } from "./setupRecovery";

import { AnchorCredentials } from "$generated/internet_identity_types";
import { infoScreenTemplate } from "$src/components/infoScreen";
import { IdentityMetadata } from "$src/repositories/identityMetadata";
import { isNullish } from "@dfinity/utils";
import { addDevice } from "../addDevice/manage/addDevice";
import {
PinIdentityMaterial,
idbRetrievePinIdentityMaterial,
} from "../pin/idb";
import copyJson from "./recoveryWizard.json";

/* Phrase creation kick-off screen */
Expand Down Expand Up @@ -80,7 +87,7 @@ export const addPhrase = ({
);
};

type DeviceStatus = "pin-only" | "one-passkey";
type DeviceStatus = "pin-only" | "one-device";

const addDeviceWarningTemplate = ({
ok,
Expand All @@ -102,7 +109,7 @@ const addDeviceWarningTemplate = ({
copy.paragraph_add_device_pin_only,
copy.add_device_title_pin_only,
],
"one-passkey": [
"one-device": [
copy.paragraph_add_device_one_passkey,
copy.add_device_title_one_passkey,
],
Expand All @@ -127,36 +134,125 @@ const addDeviceWarningTemplate = ({
// TODO: Create the `addDeviceWarning` page and use it in `recoveryWizard` function.
export const addDeviceWarningPage = renderPage(addDeviceWarningTemplate);

// Prompt the user to create a recovery phrase
export const addDeviceWarning = ({
status,
}: {
status: DeviceStatus;
}): Promise<{ action: "remind-later" | "do-not-remind" | "add-device" }> => {
return new Promise((resolve) =>
addDeviceWarningPage({
i18n: new I18n(),
ok: () => resolve({ action: "add-device" }),
remindLater: () => resolve({ action: "remind-later" }),
doNotRemindAgain: () => resolve({ action: "do-not-remind" }),
status,
})
);
};

/**
* Helper to encapsulate the logic of when and which recovery warning page to show.
*
* Three conditions must be met for the warning page to be shown:
* * Not having seen the recovery page in the last week
* (on registration, the user is not shown the page, but set it as seen to not bother during the onboarding)
* * The user has less than one device.
lmuntaner marked this conversation as resolved.
Show resolved Hide resolved
* (a phrase and pin are not considered a device, only normal devices or recovery devices)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a questionable decision. IMHO, having one device and a recovery phrase should be fine. Otherwise, people owning only one WebAuthn capable device are forced to disable the warning rather than being able to remediate the situation with an appropriate action (namely setting up a recovery phrase).

* * The user has not disabled the warning.
* (users can choose to not see the warning again by clicking "do not remind" button)
*
* If the page is shown, there are two options:
lmuntaner marked this conversation as resolved.
Show resolved Hide resolved
* * User has only the pin authentication method.
* * User has only one device.
*
* @param params {Object}
* @param params.credentials {AnchorCredentials}
* @param params.identityMetadata {IdentityMetadata | undefined}
* @param params.pinIdentityMaterial {PinIdentityMaterial | undefined}
* @param params.nowInMillis {number}
* @returns {DeviceStatus | "no-warning"}
*/
// Exported for testing
export const getDevicesStatus = ({
credentials,
identityMetadata,
pinIdentityMaterial,
nowInMillis,
}: {
credentials: AnchorCredentials;
identityMetadata: IdentityMetadata | undefined;
pinIdentityMaterial: PinIdentityMaterial | undefined;
nowInMillis: number;
}): DeviceStatus | "no-warning" => {
const ONE_WEEK_MILLIS = 7 * 24 * 60 * 60 * 1000;
const oneWeekAgoTimestamp = nowInMillis - ONE_WEEK_MILLIS;
const hasNotSeenRecoveryPageLastWeek =
(identityMetadata?.recoveryPageShownTimestampMillis ?? 0) <
oneWeekAgoTimestamp;
const showWarningPageEnabled = isNullish(
identityMetadata?.doNotShowRecoveryPageRequestTimestampMillis
);
const totalDevicesCount =
credentials.credentials.length + credentials.recovery_credentials.length;
if (
totalDevicesCount <= 1 &&
hasNotSeenRecoveryPageLastWeek &&
showWarningPageEnabled
) {
if (totalDevicesCount === 0 && !pinIdentityMaterial) {
// This should never happen because it means that the user has no devices and no pin.
// But we still handle it to avoid a crash assuming there was an error retrieving the pin material.
return "pin-only";
}
return totalDevicesCount === 0 ? "pin-only" : "one-device";
}
return "no-warning";
};

// TODO: Add e2e test https://dfinity.atlassian.net/browse/GIX-2600
export const recoveryWizard = async (
userNumber: bigint,
connection: AuthenticatedConnection
): Promise<void> => {
// Here, if the user doesn't have any recovery device, we prompt them to add
// one.
const [recoveries, identityMetadata] = await withLoader(() =>
Promise.all([
connection.lookupRecovery(userNumber),
connection.getIdentityMetadata(),
])
const [credentials, identityMetadata, pinIdentityMaterial] = await withLoader(
() =>
Promise.all([
connection.lookupCredentials(userNumber),
connection.getIdentityMetadata(),
idbRetrievePinIdentityMaterial({
userNumber,
}),
])
);

const ONE_WEEK_MILLIS = 7 * 24 * 60 * 60 * 1000;
const nowInMillis = Date.now();
const oneWeekAgoTimestamp = nowInMillis - ONE_WEEK_MILLIS;
const hasNotSeenRecoveryPageLastWeek =
(identityMetadata?.recoveryPageShownTimestampMillis ?? 0) <
oneWeekAgoTimestamp;
if (recoveries.length === 0 && hasNotSeenRecoveryPageLastWeek) {

const devivesStatus = getDevicesStatus({
lmuntaner marked this conversation as resolved.
Show resolved Hide resolved
credentials,
identityMetadata,
pinIdentityMaterial,
nowInMillis,
});

if (devivesStatus !== "no-warning") {
lmuntaner marked this conversation as resolved.
Show resolved Hide resolved
// `await` here doesn't add any waiting time beacause we already got the metadata earlier.
await connection.updateIdentityMetadata({
recoveryPageShownTimestampMillis: nowInMillis,
});
const doAdd = await addPhrase({ intent: "securityReminder" });
if (doAdd !== "cancel") {
doAdd satisfies "ok";

await setupRecovery({ userNumber, connection });
const userChoice = await addDeviceWarning({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These warning screens are now inconsistent with the messaging on the manage page when a user only has a pin identity: the manage page strongly suggests setting up a recovery phrase, whereas this screen only nudges with regards to a passkey.

Furthermore, the warning on the management page even persists after having added 2 passkeys. We should reconsider the warning screens on the management page too.

status: devivesStatus,
});
if (userChoice.action === "add-device") {
await addDevice({ userNumber, connection });
}
if (userChoice.action === "do-not-remind") {
// `await` here doesn't add any waiting time beacause we already got the metadata earlier.
await connection.updateIdentityMetadata({
doNotShowRecoveryPageRequestTimestampMillis: nowInMillis,
});
}
// Do nothing if `"remind-later"`.
}
};
Loading
Loading