From 393bc48b6b5a4cf4d5cd63dc039472b6cd712a8e Mon Sep 17 00:00:00 2001 From: grdddj Date: Wed, 10 May 2023 15:59:17 +0200 Subject: [PATCH] fix(core): improve error popups --- core/src/apps/common/request_pin.py | 4 +- core/src/apps/management/change_wipe_code.py | 8 +-- core/src/apps/webauthn/fido2.py | 16 +++--- core/src/trezor/ui/layouts/tr/__init__.py | 2 +- core/src/trezor/ui/layouts/tt_v2/__init__.py | 52 ++++++++----------- .../test_msg_change_wipe_code_t2.py | 4 +- tests/device_tests/test_msg_changepin_t2.py | 2 +- tests/input_flows.py | 5 +- tests/ui_tests/fixtures.json | 26 +++++----- 9 files changed, 58 insertions(+), 61 deletions(-) diff --git a/core/src/apps/common/request_pin.py b/core/src/apps/common/request_pin.py index 4441f81223..2d1e3d5949 100644 --- a/core/src/apps/common/request_pin.py +++ b/core/src/apps/common/request_pin.py @@ -56,7 +56,7 @@ async def request_pin( async def request_pin_confirm(ctx: Context, *args: Any, **kwargs: Any) -> str: - from trezor.ui.layouts import confirm_reenter_pin, pin_mismatch + from trezor.ui.layouts import confirm_reenter_pin, pin_mismatch_popup while True: pin1 = await request_pin(ctx, "Enter new PIN", *args, **kwargs) @@ -64,7 +64,7 @@ async def request_pin_confirm(ctx: Context, *args: Any, **kwargs: Any) -> str: pin2 = await request_pin(ctx, "Re-enter new PIN", *args, **kwargs) if pin1 == pin2: return pin1 - await pin_mismatch(ctx) + await pin_mismatch_popup(ctx) async def request_pin_and_sd_salt( diff --git a/core/src/apps/management/change_wipe_code.py b/core/src/apps/management/change_wipe_code.py index 4d9c95ec51..2256540f8f 100644 --- a/core/src/apps/management/change_wipe_code.py +++ b/core/src/apps/management/change_wipe_code.py @@ -102,17 +102,17 @@ async def _request_wipe_code_confirm(ctx: Context, pin: str) -> str: from apps.common.request_pin import request_pin from trezor.ui.layouts import ( confirm_reenter_pin, - pin_mismatch, - wipe_code_same_as_pin, + pin_mismatch_popup, + wipe_code_same_as_pin_popup, ) while True: code1 = await request_pin(ctx, "Enter new wipe code") if code1 == pin: - await wipe_code_same_as_pin(ctx) + await wipe_code_same_as_pin_popup(ctx) continue await confirm_reenter_pin(ctx, br_type="set_wipe_code", is_wipe_code=True) code2 = await request_pin(ctx, "Re-enter wipe code") if code1 == code2: return code1 - await pin_mismatch(ctx, br_type="set_wipe_code", is_wipe_code=True) + await pin_mismatch_popup(ctx, is_wipe_code=True) diff --git a/core/src/apps/webauthn/fido2.py b/core/src/apps/webauthn/fido2.py index 8e0f64fb50..7ee477ed50 100644 --- a/core/src/apps/webauthn/fido2.py +++ b/core/src/apps/webauthn/fido2.py @@ -8,7 +8,7 @@ import storage.device as storage_device from trezor import config, io, log, loop, utils, wire, workflow from trezor.crypto import hashlib from trezor.crypto.curve import nist256p1 -from trezor.ui.layouts import show_popup +from trezor.ui.layouts import show_error_popup from apps.base import set_homescreen from apps.common import cbor @@ -611,14 +611,14 @@ async def _confirm_fido(title: str, credential: Credential) -> bool: async def _confirm_bogus_app(title: str) -> None: if _last_auth_valid: - await show_popup( + await show_error_popup( title, "This device is already registered with this application.", "Already registered.", timeout_ms=_POPUP_TIMEOUT_MS, ) else: - await show_popup( + await show_error_popup( title, "This device is not registered with this application.", "Not registered.", @@ -834,12 +834,12 @@ class Fido2ConfirmExcluded(Fido2ConfirmMakeCredential): await send_cmd(cmd, self.iface) self.finished = True - await show_popup( + await show_error_popup( "FIDO2 Register", "This device is already registered with {}.", "Already registered.", self._cred.rp_id, # description_param - _POPUP_TIMEOUT_MS, + timeout_ms=_POPUP_TIMEOUT_MS, ) @@ -917,7 +917,7 @@ class Fido2ConfirmNoPin(State): await send_cmd(cmd, self.iface) self.finished = True - await show_popup( + await show_error_popup( "FIDO2 Verify User", "Please enable PIN protection.", "Unable to verify user.", @@ -940,12 +940,12 @@ class Fido2ConfirmNoCredentials(Fido2ConfirmGetAssertion): await send_cmd(cmd, self.iface) self.finished = True - await show_popup( + await show_error_popup( "FIDO2 Authenticate", "This device is not registered with\n{}.", "Not registered.", self._creds[0].app_name(), # description_param - _POPUP_TIMEOUT_MS, + timeout_ms=_POPUP_TIMEOUT_MS, ) diff --git a/core/src/trezor/ui/layouts/tr/__init__.py b/core/src/trezor/ui/layouts/tr/__init__.py index f13955df7f..7812ac4293 100644 --- a/core/src/trezor/ui/layouts/tr/__init__.py +++ b/core/src/trezor/ui/layouts/tr/__init__.py @@ -280,7 +280,7 @@ async def show_error_and_raise( raise NotImplementedError -async def show_popup( +async def show_error_popup( title: str, description: str, subtitle: str | None = None, diff --git a/core/src/trezor/ui/layouts/tt_v2/__init__.py b/core/src/trezor/ui/layouts/tt_v2/__init__.py index 7c02f92638..95273a1596 100644 --- a/core/src/trezor/ui/layouts/tt_v2/__init__.py +++ b/core/src/trezor/ui/layouts/tt_v2/__init__.py @@ -487,8 +487,8 @@ async def show_error_and_raise( ctx, RustLayout( trezorui2.show_error( - title=content, - description=subheader or "", + title=subheader or "", + description=content, button=button.upper(), allow_cancel=False, ) @@ -1098,21 +1098,27 @@ async def confirm_signverify( ) -async def show_popup( +async def show_error_popup( title: str, description: str, subtitle: str | None = None, description_param: str = "", - timeout_ms: int = 3000, + *, + button: str = "", + timeout_ms: int = 0, ) -> None: + if not button and not timeout_ms: + raise ValueError("Either button or timeout_ms must be set") + if subtitle: title += f"\n{subtitle}" await RustLayout( trezorui2.show_error( title=title, description=description.format(description_param), - button="", + button=button, time_ms=timeout_ms, + allow_cancel=False, ) ) @@ -1192,38 +1198,26 @@ async def confirm_reenter_pin( ) -async def pin_mismatch( +async def pin_mismatch_popup( ctx: GenericContext, - br_type: str = "set_pin", - br_code: ButtonRequestType = BR_TYPE_OTHER, is_wipe_code: bool = False, ) -> None: - title = "WIPE CODE MISMATCH" if is_wipe_code else "PIN MISMATCH" + await button_request(ctx, "pin_mismatch", code=BR_TYPE_OTHER) + title = "Wipe code mismatch" if is_wipe_code else "PIN mismatch" description = "wipe codes" if is_wipe_code else "PINs" - return await confirm_action( - ctx, - br_type, + return await show_error_popup( title, - action=f"The {description} you entered do not match.\n\nPlease try again.", - verb="TRY AGAIN", - verb_cancel=None, - br_code=br_code, + f"The {description} you entered do not match.", + button="TRY AGAIN", ) -async def wipe_code_same_as_pin( - ctx: GenericContext, - br_type: str = "set_wipe_code", - br_code: ButtonRequestType = BR_TYPE_OTHER, -) -> None: - return await confirm_action( - ctx, - br_type, - "INVALID WIPE CODE", - action="The wipe code must be different from your PIN.\n\nPlease try again.", - verb="TRY AGAIN", - verb_cancel=None, - br_code=br_code, +async def wipe_code_same_as_pin_popup(ctx: GenericContext) -> None: + await button_request(ctx, "wipe_code_same_as_pin", code=BR_TYPE_OTHER) + return await show_error_popup( + "Invalid wipe code", + "The wipe code must be different from your PIN.", + button="TRY AGAIN", ) diff --git a/tests/device_tests/test_msg_change_wipe_code_t2.py b/tests/device_tests/test_msg_change_wipe_code_t2.py index 4272b93b57..7a4a47c041 100644 --- a/tests/device_tests/test_msg_change_wipe_code_t2.py +++ b/tests/device_tests/test_msg_change_wipe_code_t2.py @@ -19,7 +19,7 @@ import pytest from trezorlib import btc, device, messages from trezorlib.client import MAX_PIN_LENGTH, PASSPHRASE_TEST_PATH from trezorlib.debuglink import TrezorClientDebugLink as Client -from trezorlib.exceptions import Cancelled, TrezorFailure +from trezorlib.exceptions import TrezorFailure from ..input_flows import InputFlowNewCodeMismatch @@ -97,7 +97,7 @@ def test_set_remove_wipe_code(client: Client): def test_set_wipe_code_mismatch(client: Client): - with client, pytest.raises(Cancelled): + with client, pytest.raises(TrezorFailure): IF = InputFlowNewCodeMismatch(client, WIPE_CODE4, WIPE_CODE6) client.set_input_flow(IF.get()) diff --git a/tests/device_tests/test_msg_changepin_t2.py b/tests/device_tests/test_msg_changepin_t2.py index 515bb729ce..faad372f19 100644 --- a/tests/device_tests/test_msg_changepin_t2.py +++ b/tests/device_tests/test_msg_changepin_t2.py @@ -122,7 +122,7 @@ def test_set_failed(client: Client): # Check that there's no PIN protection _check_no_pin(client) - with client, pytest.raises(Cancelled): + with client, pytest.raises(TrezorFailure): IF = InputFlowNewCodeMismatch(client, PIN4, PIN60) client.set_input_flow(IF.get()) diff --git a/tests/input_flows.py b/tests/input_flows.py index a10054c7df..d06d894b68 100644 --- a/tests/input_flows.py +++ b/tests/input_flows.py @@ -133,6 +133,10 @@ class InputFlowNewCodeMismatch(InputFlowBase): yield from input_two_different_pins() yield # PIN mismatch + self.debug.press_yes() # try again + + yield # PIN entry again + self.debug.press_no() # cancel @@ -158,7 +162,6 @@ class InputFlowCodeChangeFail(InputFlowBase): yield # enter new pin again (but different) self.debug.input(self.new_pin_2) - # So the mismatch screen is visible yield # PIN mismatch self.debug.press_yes() # try again diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index c74e5f3182..45c2b3c31c 100644 --- a/tests/ui_tests/fixtures.json +++ b/tests/ui_tests/fixtures.json @@ -726,11 +726,11 @@ "TT_test_pin.py::test_pin_long": "34692638906727bd3ebf89468aa8d4658866f1aac7775e8cae28bb337d588b3d", "TT_test_pin.py::test_pin_long_delete": "eea0b6485700cce2bc6c13e6d1971e4add11355aa155edf2fcb9bb699dfb0654", "TT_test_pin.py::test_pin_longer_than_max": "6a7e55fd6d3ff117ee5deb40f1c07cf8d3248931a29e4de6498d813744af2981", -"TT_test_pin.py::test_pin_same_as_wipe_code": "8a4174aa065036cd6f111edd668384951fd6184bd18e174cf177925e3933083b", +"TT_test_pin.py::test_pin_same_as_wipe_code": "f309e8ffbb4e55a934038b73c8c4f1d250f66b6136d806ddb82fda88aa62648b", "TT_test_pin.py::test_pin_setup": "470ba3c1e991f14b99ef0256774dcb25ec8efa2e24951f923e9439e1f5d9e71d", -"TT_test_pin.py::test_pin_setup_mismatch": "bc3d883b79a2edef4cc38f0284afe8afcb5f7cc49d601a371a131859bfb628cb", +"TT_test_pin.py::test_pin_setup_mismatch": "2f4de6411cddb4cc29dad201de1b388267aa0a579652c87ada7b22661debe343", "TT_test_pin.py::test_pin_short": "b5377990e4a1f324133601e3ca4726cde7af50b3e2c3f53738022ed9108a6a79", -"TT_test_pin.py::test_wipe_code_same_as_pin": "c674e074339d04370bfc99c846bc055cf1ba135ee1c44d22f7307c42057451d3", +"TT_test_pin.py::test_wipe_code_same_as_pin": "d6257f611689fa0e5616bd47861dcc4dad6c48d2d1f642ce923a66cae3dbb236", "TT_test_pin.py::test_wipe_code_setup": "677797c95ed903fbf03f6e872610855aec253ac5a45bd211dbda96e02464df60", "TT_test_recovery.py::test_recovery_bip39": "614e9204c01c379b4b88ba618addd50f22cf9f75492f2363cdca14acee1c883f", "TT_test_recovery.py::test_recovery_slip39_basic": "73e2c4e4c4cb75206a5b739e4196035a4a3eef83a434a880ac27a8c7b5a3a0e2", @@ -1696,7 +1696,7 @@ "TT_reset_recovery-test_reset_backup.py::test_skip_backup_msg[BackupType.Slip39_Advanced-backup-dcbda5cf": "dbadedde1fd638f72f33590ae32425a0a645c9a0818a5030f0bd9adbe961c2bf", "TT_reset_recovery-test_reset_backup.py::test_skip_backup_msg[BackupType.Slip39_Basic-backup_fl-1577de4d": "245fafadd5b9181f1a512414ce9c486fff2b9c76044c4855bc143e4f5fd28592", "TT_reset_recovery-test_reset_bip39_t2.py::test_already_initialized": "80a6e289138a604cf351a29511cf6f85e2243591317894703152787e1351a1a3", -"TT_reset_recovery-test_reset_bip39_t2.py::test_failed_pin": "722134783cb2abc73cd695ee1d1ede6f0de8d5402409e656f722208343d81a39", +"TT_reset_recovery-test_reset_bip39_t2.py::test_failed_pin": "1902731b7775c17a45864707b5609c61b5f3de702391357b89dc85a424774301", "TT_reset_recovery-test_reset_bip39_t2.py::test_reset_device": "fa804eaa19c56485ae7f156ba47a3c9642aa70e65295a19e018cf7e0c7156d25", "TT_reset_recovery-test_reset_bip39_t2.py::test_reset_device_192": "cf6b6b9b6c760f229725028cb9413ffc8fe31563045d8b13361a17c040e8a41f", "TT_reset_recovery-test_reset_bip39_t2.py::test_reset_device_pin": "73f2cc80c5a9d03fb31b6508cbe20832db16dbb32b6edab5aaf108ba062fd840", @@ -1799,15 +1799,15 @@ "TT_test_msg_backup_device.py::test_interrupt_backup_fails": "cdf801e16b046079569e4055f43a86a45d7eace58793427ef5433f71e75961f9", "TT_test_msg_backup_device.py::test_no_backup_fails": "fada9d38ec099b3c6a4fd8bf994bb1f3431e40085128b4e0cd9deb8344dec53e", "TT_test_msg_backup_device.py::test_no_backup_show_entropy_fails": "e00c46a70bc5bbfdd3bb6da5a712698e7c79bc8ebd78262ec0f13ef9ee6aec95", -"TT_test_msg_change_wipe_code_t2.py::test_set_pin_to_wipe_code": "f0c7493358ebfca355a677822efb1bb9a860715f6746ee76a16311f9e4c5dd7f", -"TT_test_msg_change_wipe_code_t2.py::test_set_remove_wipe_code": "d397af1119fdbdca182ca58ae47f978f66a4cb05b91c0b54922cf4914a58beca", -"TT_test_msg_change_wipe_code_t2.py::test_set_wipe_code_mismatch": "6a4e349e960b95e75d29f96c5619906a23c63283fdf6b933026a546596f08d54", -"TT_test_msg_change_wipe_code_t2.py::test_set_wipe_code_to_pin": "e98b69f7a44b1f19cf579ea48ebadecd84d979c4e6852b46320b4e04572a8429", -"TT_test_msg_changepin_t2.py::test_change_failed": "6ab25151a51ff5f57fbdb01e4c7af8cbeb10508d6c91277f7c5defc6e50d236f", -"TT_test_msg_changepin_t2.py::test_change_invalid_current": "8a94b2df5c14297b9ffc7529bac4af9c867efa5a87796db7aa78750be400062e", +"TT_test_msg_change_wipe_code_t2.py::test_set_pin_to_wipe_code": "a9bd775d5fa279ae44eb9498b59eb6af5a78f6ebcae46563ab0d33962d50c82f", +"TT_test_msg_change_wipe_code_t2.py::test_set_remove_wipe_code": "36ce2fa2d74131cc8fed47d335ff485524402d30fe0f5de14e6f9e590ddc00e4", +"TT_test_msg_change_wipe_code_t2.py::test_set_wipe_code_mismatch": "4882e50989dd4dcebaba685785c713f0ee54b80be13cf66c8c2d4ab111062f33", +"TT_test_msg_change_wipe_code_t2.py::test_set_wipe_code_to_pin": "a98ad32b9cf93579b798a76aa60370e2444493307c5743093351edd03b15b008", +"TT_test_msg_changepin_t2.py::test_change_failed": "565e26dcb8d24f1ad18f4fe03ac8d114fda94a46a0c31f7960497777718845b4", +"TT_test_msg_changepin_t2.py::test_change_invalid_current": "c11d3f8d788f61a7213bb2dba71ae0517e686e6cb73653addafa5f60d611afbe", "TT_test_msg_changepin_t2.py::test_change_pin": "4897e28024f677075b5a2fe941876e87bf6ffe5a1ec40d3658d643f6c042aabb", "TT_test_msg_changepin_t2.py::test_remove_pin": "1a415b1d0d626bf52c08f948f8761ad84f18550931ba34e422c0ee58862f060e", -"TT_test_msg_changepin_t2.py::test_set_failed": "a0377c8b0d194f65bf7b6532aa04488cb5a2c81b9cb43e351d5a31c0013c4d0b", +"TT_test_msg_changepin_t2.py::test_set_failed": "59e8d307d78714fabc1fabf86d0ff763407b951ccb90883df5e1f2ef03d2ea09", "TT_test_msg_changepin_t2.py::test_set_pin": "9c9787d1fe80cf530e6c92079a36d440effca54587eb9f5a6b7fdd34406dae17", "TT_test_msg_loaddevice.py::test_load_device_1": "3643774c15de3f99b82fff125bc05b9af066f5c08b7861a117f0c05da79ba1c5", "TT_test_msg_loaddevice.py::test_load_device_2": "e37151e13b8a880033a82c065a41271d19f8c4a2b3c34985a7f5f04c1f4f0010", @@ -1845,7 +1845,7 @@ "TT_test_protection_levels.py::test_wipe_device": "af075bdd0d287154aeee0aa6f8a1066be24f2aab8a6f5f4ab7e1bdd26af380e9", "TT_test_sdcard.py::test_sd_format": "45731c43d45fd5d86cdc462a1dbbde1ad83a02fdb5abffa2e8bf54af2f0cd1ee", "TT_test_sdcard.py::test_sd_no_format": "3590feffa9193c304105fba0afae84bc93e54f1a4706490bd63b9d56a202a1a7", -"TT_test_sdcard.py::test_sd_protect_unlock": "1e500d3abe7d308a451267df9da4b0a3167ffe25e16c42ee219ca449c38fd500", +"TT_test_sdcard.py::test_sd_protect_unlock": "674dd28470342a9cffe39da611f55dd0ae217db3d965da99bc3cbef8f8521bbf", "TT_test_session.py::test_cannot_resume_ended_session": "80a6e289138a604cf351a29511cf6f85e2243591317894703152787e1351a1a3", "TT_test_session.py::test_clear_session": "4978f02d2c97e997eaf8417e511dfdfdbe9675f82e7d849117ba5cf6ce653c53", "TT_test_session.py::test_derive_cardano_empty_session": "80a6e289138a604cf351a29511cf6f85e2243591317894703152787e1351a1a3", @@ -1880,7 +1880,7 @@ "TT_tezos-test_sign_tx.py::test_tezos_smart_contract_delegation": "90ed61bc1f075503fa7fc1d9a4137d38b8c9a0ff12a86ed19d936c6758ccd9f0", "TT_tezos-test_sign_tx.py::test_tezos_smart_contract_transfer": "bec3ec92587c50ee286e1257954a9248c28dfd5eb09cefbbd89f35357ce35021", "TT_tezos-test_sign_tx.py::test_tezos_smart_contract_transfer_to_contract": "b4aee29d5b8c16f60cc2eb87cc4a5c63402141a9409ddcc7ec0d2af272aab79d", -"TT_webauthn-test_msg_webauthn.py::test_add_remove": "48e680f749e86f04a5da52a23b1ec23aee8d526af66b30438d423dda7b5be53a", +"TT_webauthn-test_msg_webauthn.py::test_add_remove": "40f10f46427749ccb4b5573ccbb7e6ba5d2871abfca6ce201bfa20b97150b19f", "TT_webauthn-test_u2f_counter.py::test_u2f_counter": "d46453a7846a8fd8a5f513937104c82cf8662f76be39017dbc217abf0eb3600a", "TT_zcash-test_sign_tx.py::test_external_presigned": "0020164adcd1f8558664940a171683e1c6a43da9612344a84822d1bebc68a654", "TT_zcash-test_sign_tx.py::test_one_two": "d2975b997628fb3a82c3fcb5c792f79c7fa64cba3c350517408fb863db8e759e",