Skip to content

Fix recovery device type checks based on alias #1222

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

Merged
merged 15 commits into from
Feb 14, 2023
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
Binary file modified screenshots/desktop/pickRecoveryDevice.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified screenshots/mobile/pickRecoveryDevice.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
61 changes: 39 additions & 22 deletions src/frontend/src/flows/manage/deviceSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import { unreachable } from "../../utils/utils";
import { DeviceData } from "../../../generated/internet_identity_types";
import { phraseRecoveryPage } from "../recovery/recoverWith/phrase";
import { mainWindow } from "../../components/mainWindow";
import {
isRecoveryDevice,
RecoveryDevice,
recoveryDeviceToLabel,
} from "../../utils/recoveryDevice";

// The "device settings" page where users can view information about a device,
// remove a device, make a recovery phrase protected, etc.
Expand All @@ -24,15 +29,17 @@ const deviceSettingsTemplate = ({
isOnlyDevice,
back,
}: {
protectDevice: () => void;
protectDevice: (device: DeviceData & RecoveryDevice) => void;
deleteDevice: () => void;
device: DeviceData;
isOnlyDevice: boolean;
back: () => void;
}) => {
const pageContentSlot = html` <article id="deviceSettings">
<h1 class="t-title">
${isRecovery(device) ? "" : "Device"} ${device.alias}
${isRecoveryDevice(device)
? recoveryDeviceToLabel(device)
: `Device ${device.alias}`}
</h1>

<div class="l-stack">
Expand All @@ -42,7 +49,7 @@ const deviceSettingsTemplate = ({
your recovery phrase to delete it.
</p>
<button
@click="${() => protectDevice()}"
@click="${() => protectDevice(device)}"
data-action="protect"
class="c-button"
>
Expand All @@ -56,13 +63,17 @@ const deviceSettingsTemplate = ({
data-action="remove"
class="c-button c-button--warning"
>
Delete ${isRecovery(device) ? "Recovery" : "Device"}
Delete ${isRecoveryDevice(device) ? "Recovery" : "Device"}
</button>`
: ""}
${!isOnlyDevice && isProtected(device)
? html`<p class="t-paragraph">
This ${isRecovery(device) ? device.alias : "device"} is protected
and you will be prompted to authenticate with it before removal.
This
${isRecoveryDevice(device)
? recoveryDeviceToLabel(device).toLowerCase()
: "device"}
is protected and you will be prompted to authenticate with it before
removal.
</p>`
: ""}
${isOnlyDevice
Expand All @@ -86,17 +97,16 @@ const deviceSettingsTemplate = ({

// We offer to protect unprotected recovery phrases only, although in the
// future we may offer to protect all devices
const shouldOfferToProtect = (device: DeviceData): boolean =>
const shouldOfferToProtect = (
device: DeviceData
): device is RecoveryDevice & DeviceData =>
"recovery" in device.purpose &&
"seed_phrase" in device.key_type &&
!isProtected(device);

const isProtected = (device: DeviceData): boolean =>
"protected" in device.protection;

const isRecovery = (device: DeviceData): boolean =>
"recovery" in device.purpose;

export const deviceSettingsPage = (
props: Parameters<typeof deviceSettingsTemplate>[0],
container?: HTMLElement
Expand All @@ -118,8 +128,14 @@ export const deviceSettings = async (
device,
isOnlyDevice,
back: resolve,
protectDevice: () =>
protectDevice(userNumber, connection, device, isOnlyDevice, resolve),
protectDevice: (recoveryDevice: DeviceData & RecoveryDevice) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it unfortunate that protectDevice now needs to take a device but I get it that it needs a RecoveryDevice. We'll be remodelling the device settings page soon so not a big problem.

protectDevice(
userNumber,
connection,
recoveryDevice,
isOnlyDevice,
resolve
),
deleteDevice: () =>
deleteDevice(userNumber, connection, device, isOnlyDevice, resolve),
});
Expand All @@ -131,7 +147,7 @@ export const deviceSettings = async (
const deviceConnection = async (
connection: Connection,
userNumber: bigint,
device: DeviceData,
device: DeviceData & RecoveryDevice,
recoveryPhraseMessage: string
): Promise<AuthenticatedConnection | null> => {
try {
Expand Down Expand Up @@ -193,14 +209,15 @@ const deleteDevice = async (

// If the device is protected then we need to be authenticated with the device to remove it
// NOTE: the user may be authenticated with the device already, but for consistency we still ask them to input their recovery phrase
const removalConnection = isProtected(device)
? await deviceConnection(
connection,
userNumber,
device,
"Please input your recovery phrase to remove it."
)
: connection;
const removalConnection =
isRecoveryDevice(device) && isProtected(device)
? await deviceConnection(
connection,
userNumber,
device,
"Please input your recovery phrase to remove it."
)
: connection;

await withLoader(async () => {
// if null then user canceled so we just redraw the manage page
Expand All @@ -226,7 +243,7 @@ const deleteDevice = async (
const protectDevice = async (
userNumber: bigint,
connection: AuthenticatedConnection,
device: DeviceData,
device: DeviceData & RecoveryDevice,
isOnlyDevice: boolean,
back: () => void
) => {
Expand Down
31 changes: 20 additions & 11 deletions src/frontend/src/flows/manage/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ import { chooseDeviceAddFlow } from "../addDevice/manage";
import { addLocalDevice } from "../addDevice/manage/addLocalDevice";
import { warnBox } from "../../components/warnBox";
import { mainWindow } from "../../components/mainWindow";
import {
isRecoveryDevice,
recoveryDeviceToLabel,
} from "../../utils/recoveryDevice";

/* Template for the authbox when authenticating to II */
export const authnTemplateManage = (): AuthnTemplates => {
Expand Down Expand Up @@ -228,17 +232,22 @@ const recoverySection = (
`;
};

const deviceListItem = (device: DeviceData) => html`
<div class="c-action-list__label">${device.alias}</div>
<button
type="button"
aria-label="settings"
data-action="settings"
class="c-action-list__action"
>
${settingsIcon}
</button>
`;
const deviceListItem = (device: DeviceData) => {
const label = isRecoveryDevice(device)
? recoveryDeviceToLabel(device)
: device.alias;
return html`
<div class="c-action-list__label">${label}</div>
<button
type="button"
aria-label="settings"
data-action="settings"
class="c-action-list__action"
>
${settingsIcon}
</button>
`;
};

const recoveryNag = ({ onAddRecovery }: { onAddRecovery: () => void }) =>
warnBox({
Expand Down
15 changes: 10 additions & 5 deletions src/frontend/src/flows/recovery/chooseRecoveryMechanism.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export const chooseRecoveryMechanism = async ({
message,
cancelText,
}: {
devices: DeviceData[];
devices: Omit<DeviceData, "alias">[];
} & ChooseRecoveryProps): Promise<RecoveryMechanism | null> => {
return new Promise((resolve) => {
return chooseRecoveryMechanismPage({
Expand All @@ -104,7 +104,12 @@ export const chooseRecoveryMechanism = async ({
});
};

const hasRecoveryPhrase = (devices: DeviceData[]): boolean =>
devices.some((device) => device.alias === "Recovery phrase");
const hasRecoveryKey = (devices: DeviceData[]): boolean =>
devices.some((device) => device.alias === "Recovery key");
const hasRecoveryPhrase = (devices: Omit<DeviceData, "alias">[]): boolean =>
devices.some(
(device) => "seed_phrase" in device.key_type && "recovery" in device.purpose
);
const hasRecoveryKey = (devices: Omit<DeviceData, "alias">[]): boolean =>
devices.some(
(device) =>
!("seed_phrase" in device.key_type) && "recovery" in device.purpose
);
15 changes: 9 additions & 6 deletions src/frontend/src/flows/recovery/pickRecoveryDevice.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { html, render } from "lit-html";
import { DeviceData } from "../../../generated/internet_identity_types";
import { mainWindow } from "../../components/mainWindow";
import { securityKeyIcon, seedPhraseIcon } from "../../components/icons";
import {
RecoveryDevice,
recoveryDeviceToLabel,
} from "../../utils/recoveryDevice";

const pageContent = () => {
const pageContentSlot = html`
Expand All @@ -22,14 +25,14 @@ const pageContent = () => {
};

export const pickRecoveryDevice = async (
devices: DeviceData[]
): Promise<DeviceData> => {
devices: RecoveryDevice[]
): Promise<RecoveryDevice> => {
const container = document.getElementById("pageContent") as HTMLElement;
render(pageContent(), container);
return init(devices);
};

export const init = (devices: DeviceData[]): Promise<DeviceData> =>
export const init = (devices: RecoveryDevice[]): Promise<RecoveryDevice> =>
new Promise((resolve) => {
const deviceList = document.getElementById("deviceList") as HTMLElement;
deviceList.innerHTML = ``;
Expand All @@ -44,11 +47,11 @@ export const init = (devices: DeviceData[]): Promise<DeviceData> =>
html`<div class="deviceItemAlias">
<button class="c-button c-button--secondary">
<span aria-hidden="true"
>${device.alias === "Recovery Phrase"
>${"seed_phrase" in device.key_type
? seedPhraseIcon
: securityKeyIcon}</span
>
<div class="t-strong">${device.alias}</div>
<div class="t-strong">${recoveryDeviceToLabel(device)}</div>
</button>
</div>`,
identityElement
Expand Down
6 changes: 3 additions & 3 deletions src/frontend/src/flows/recovery/recoverWith/device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import {
LoginFlowSuccess,
LoginFlowCanceled,
} from "../../../utils/flowResult";
import { DeviceData } from "../../../../generated/internet_identity_types";
import { Connection } from "../../../utils/iiConnection";
import { mainWindow } from "../../../components/mainWindow";
import { RecoveryDevice } from "../../../utils/recoveryDevice";

const pageContent = () => {
const pageContentSlot = html`
Expand Down Expand Up @@ -49,7 +49,7 @@ const pageContent = () => {
export const deviceRecoveryPage = async (
userNumber: bigint,
connection: Connection,
device: DeviceData
device: RecoveryDevice
): Promise<LoginFlowSuccess | LoginFlowCanceled> => {
const container = document.getElementById("pageContent") as HTMLElement;
render(pageContent(), container);
Expand All @@ -59,7 +59,7 @@ export const deviceRecoveryPage = async (
const init = (
userNumber: bigint,
connection: Connection,
device: DeviceData
device: RecoveryDevice
): Promise<LoginFlowSuccess | LoginFlowCanceled> =>
new Promise((resolve) => {
const buttonContinue = document.getElementById(
Expand Down
6 changes: 3 additions & 3 deletions src/frontend/src/flows/recovery/recoverWith/phrase.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { html, nothing, render, TemplateResult } from "lit-html";
import { DeviceData } from "../../../../generated/internet_identity_types";
import {
apiResultToLoginFlowResult,
LoginFlowCanceled,
Expand All @@ -17,6 +16,7 @@ import {
} from "../../../crypto/mnemonic";
import { warningIcon } from "../../../components/icons";
import { mainWindow } from "../../../components/mainWindow";
import { RecoveryDevice } from "../../../utils/recoveryDevice";

const pageContent = (userNumber: bigint, message?: string) => {
const pageContentSlot = html`
Expand Down Expand Up @@ -80,7 +80,7 @@ const pageContent = (userNumber: bigint, message?: string) => {
export const phraseRecoveryPage = async (
userNumber: bigint,
connection: Connection,
device: DeviceData,
device: RecoveryDevice,
prefilledPhrase?: string,
message?: string
): Promise<LoginFlowSuccess | LoginFlowCanceled> => {
Expand All @@ -92,7 +92,7 @@ export const phraseRecoveryPage = async (
const init = (
userNumber: bigint,
connection: Connection,
device: DeviceData,
device: RecoveryDevice,
prefilledPhrase?: string /* if set, prefilled as input */,
message?: string
): Promise<LoginFlowSuccess | LoginFlowCanceled> =>
Expand Down
5 changes: 3 additions & 2 deletions src/frontend/src/showcase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ import { displaySafariWarning } from "./flows/recovery/displaySafariWarning";
import { displayError } from "./components/displayError";
import { promptUserNumber } from "./components/promptUserNumber";
import { registerDisabled } from "./flows/registerDisabled";
import { RecoveryDevice } from "./utils/recoveryDevice";

// A "dummy" connection which actually is just undefined, hoping pages won't call it
const dummyConnection = undefined as unknown as AuthenticatedConnection;
const userNumber = BigInt(10000);

const i18n = new I18n("en");

const recoveryPhrase: DeviceData = {
const recoveryPhrase: RecoveryDevice & DeviceData = {
alias: "Recovery Phrase",
protection: { unprotected: null },
pubkey: [1, 2, 3, 4],
Expand All @@ -66,7 +67,7 @@ const recoveryPhrase: DeviceData = {
const recoveryPhraseText =
"10050 mandate vague same suspect eight pet gentle repeat maple actor about legal sword text food print material churn perfect sword blossom sleep vintage blouse";

const recoveryDevice: DeviceData = {
const recoveryDevice: RecoveryDevice & DeviceData = {
alias: "Recovery Device",
protection: { unprotected: null },
pubkey: [1, 2, 3, 4],
Expand Down
1 change: 1 addition & 0 deletions src/frontend/src/test-e2e/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ export const ABOUT_URL = `${II_URL}/about`;

export const DEVICE_NAME1 = "Virtual WebAuthn device";
export const DEVICE_NAME2 = "Other WebAuthn device";
export const RECOVERY_PHRASE_NAME = "Recovery Phrase";
Loading