Skip to content
This repository has been archived by the owner on May 28, 2019. It is now read-only.

Storage: check_pin and unlock functions are the same #534

Closed
tsusanka opened this issue Apr 4, 2019 · 3 comments
Closed

Storage: check_pin and unlock functions are the same #534

tsusanka opened this issue Apr 4, 2019 · 3 comments
Assignees
Milestone

Comments

@tsusanka
Copy link
Contributor

tsusanka commented Apr 4, 2019

The check_pin and unlock function are completely the same.

Does it make sense to have both? If needed, then at least call the first from the other one.

@andrewkozlik would you like to look at this?

Reported by @ciny

@andrewkozlik
Copy link
Contributor

I think it makes sense to differentiate between the two functions for now even though they behave the same. In the future we might want to implement different behavior for each, i.e. we might not want check_pin() to unlock the storage. However, I noticed that during dry run recovery we do not check the PIN if config.has_pin() returns false. We should not trust what config.has_pin() returns, because the stored value is not cryptographically validated.

Changes:

Test cases:
TC1: Perform a dry run recovery ("Check recovery seed" in Trezor Wallet) on a device which does not have a PIN set. Verify that the user is not asked to enter PIN and dry run proceeds as expected.
TC2: Perform a dry run recovery on a device which has a PIN set. Enter the correct PIN. Verify that dry run proceeds as expected.
TC3: Perform a dry run recovery on a device which has a PIN set. Enter an incorrect PIN. Verify that the dry run recovery is aborted.

@tsusanka tsusanka modified the milestones: backlog, 2019-05 Apr 5, 2019
@tsusanka
Copy link
Contributor Author

tsusanka commented Apr 5, 2019

Well done! Waits for review in #536

@tsusanka
Copy link
Contributor Author

tsusanka commented Apr 11, 2019

Closed by #536, test cases moved to #550.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants