Fix recovery device type checks based on alias #1222
Conversation
This PR fixes checks for recovery devices and recovery phrases that were based on the device name. This change is required because the public endpoints no longer give information about the alias. This PR also includes some typing improvements around the alias being stripped on the public 'lookup' endpoint.
dd2b8de to
bf87c11
Compare
|
FYI: Screenshot changes happen because the |
| import { DeviceData } from "../../generated/internet_identity_types"; | ||
|
|
||
| export const recoveryDeviceToLabel = ( | ||
| device: Omit<DeviceData, "alias"> |
There was a problem hiding this comment.
I think you can enforce that device has the correct purpose and key_type here and mostly avoid runtime errors
There was a problem hiding this comment.
I implemented that in commit 517e9e4f0c51d68828fa4029e087770018a83674 but it seems quite involved. Please have a look, maybe there is a way to simplify.
766b53a to
cf11d31
Compare
cf11d31 to
517e9e4
Compare
| @@ -0,0 +1,17 @@ | |||
| import { DeviceData, Purpose } from "../../generated/internet_identity_types"; | |||
|
|
|||
| export type RecoveryDevice = Omit<DeviceData, "alias"> & { | |||
There was a problem hiding this comment.
Do RecoveryDevices never have an alias? Also, there are a few occurrences of RecoveryDevice & Omit<DeviceData, "alias"> and RecoveryDevice & DeviceData here and in other modules. Shouldn't these all just be RecoveryDevice?
There was a problem hiding this comment.
Technically they do have an alias, in particular when interacting with authenticated canister calls (i.e. add and update) the alias is required.
Agreed, RecoveryDevice & Omit<DeviceData, "alias"> is redundant. Thanks for pointing it out. However, RecoveryDevice & DeviceData is not, as this combines the existence of alias with the restriction on purpose.
d3ce482 to
24b23df
Compare
Co-authored-by: Nicolas Mattia <nicolas.mattia@dfinity.org>
| @@ -118,8 +128,14 @@ export const deviceSettings = async ( | |||
| device, | |||
| isOnlyDevice, | |||
| back: resolve, | |||
| protectDevice: () => | |||
| protectDevice(userNumber, connection, device, isOnlyDevice, resolve), | |||
| protectDevice: (recoveryDevice: DeviceData & RecoveryDevice) => | |||
There was a problem hiding this comment.
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.
This PR fixes checks for recovery devices and recovery phrases that
were based on the device name. This change is required because the
public endpoints no longer give information about the alias.
This PR also includes some typing improvements around the alias being
stripped on the public 'lookup' endpoint.