-
Notifications
You must be signed in to change notification settings - Fork 136
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
Changes from all commits
aa572e6
fd7ad83
2fc3d0a
95f36cc
7466cdc
3e3371d
8b67700
4cc7bad
83997a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
import { | ||
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"); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 */ | ||
|
@@ -80,7 +87,7 @@ export const addPhrase = ({ | |
); | ||
}; | ||
|
||
type DeviceStatus = "pin-only" | "one-passkey"; | ||
type DeviceStatus = "pin-only" | "one-device"; | ||
|
||
const addDeviceWarningTemplate = ({ | ||
ok, | ||
|
@@ -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, | ||
], | ||
|
@@ -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 at most one device. | ||
* (a phrase and pin are not considered a device, only normal devices or recovery devices) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
* | ||
* When the warning page is shown, two different messages could be displayed: | ||
* * 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 devicesStatus = getDevicesStatus({ | ||
credentials, | ||
identityMetadata, | ||
pinIdentityMaterial, | ||
nowInMillis, | ||
}); | ||
|
||
if (devicesStatus !== "no-warning") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the code would be simpler if this condition was flipped (with early return). |
||
// `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({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: devicesStatus, | ||
}); | ||
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"`. | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests! 👍