From e9cd9e8ef3b87e4d029cd73e6f1eabfb5f59b737 Mon Sep 17 00:00:00 2001 From: grdddj Date: Mon, 14 Nov 2022 15:16:41 +0100 Subject: [PATCH] WIP - show prompts in PIN entry --- .../ui/model_tr/component/changing_text.rs | 30 ++++- .../rust/src/ui/model_tr/component/pin.rs | 105 +++++++++++------- core/embed/rust/src/ui/model_tr/layout.rs | 9 +- core/src/apps/management/change_pin.py | 24 ++-- core/src/apps/management/change_wipe_code.py | 28 ++--- core/src/trezor/ui/layouts/tr/__init__.py | 27 ++--- core/src/trezor/ui/layouts/tt_v2/__init__.py | 4 +- 7 files changed, 134 insertions(+), 93 deletions(-) diff --git a/core/embed/rust/src/ui/model_tr/component/changing_text.rs b/core/embed/rust/src/ui/model_tr/component/changing_text.rs index dd64bd7066..0b3276a130 100644 --- a/core/embed/rust/src/ui/model_tr/component/changing_text.rs +++ b/core/embed/rust/src/ui/model_tr/component/changing_text.rs @@ -14,6 +14,8 @@ pub struct ChangingTextLine { pad: Pad, text: T, font: Font, + /// Whether to show the text. Can be disabled. + show_content: bool, line_alignment: LineAlignment, } @@ -27,6 +29,7 @@ where pad: Pad::with_background(theme::BG), text, font, + show_content: true, line_alignment, } } @@ -35,9 +38,20 @@ where Self::new(text, Font::MONO, LineAlignment::Center) } + pub fn center_bold(text: T) -> Self { + Self::new(text, Font::BOLD, LineAlignment::Center) + } + + // Update the text to be displayed in the line. pub fn update_text(&mut self, text: T) { self.text = text; - self.pad.clear(); + } + + // Whether we should display the text content. + // If not, the whole area (Pad) will still be cleared. + // Is valid until this function is called again. + pub fn show_or_not(&mut self, show: bool) { + self.show_content = show; } /// Gets the height that is needed for this line to fit perfectly @@ -85,11 +99,17 @@ where } fn paint(&mut self) { + // Always re-painting from scratch. + // Effectively clearing the line completely + // when `self.show_content` is set to `false`. + self.pad.clear(); self.pad.paint(); - match self.line_alignment { - LineAlignment::Left => self.paint_left(), - LineAlignment::Center => self.paint_center(), - LineAlignment::Right => self.paint_right(), + if self.show_content { + match self.line_alignment { + LineAlignment::Left => self.paint_left(), + LineAlignment::Center => self.paint_center(), + LineAlignment::Right => self.paint_right(), + } } } } diff --git a/core/embed/rust/src/ui/model_tr/component/pin.rs b/core/embed/rust/src/ui/model_tr/component/pin.rs index d2d01c6d4d..a4e0f6650b 100644 --- a/core/embed/rust/src/ui/model_tr/component/pin.rs +++ b/core/embed/rust/src/ui/model_tr/component/pin.rs @@ -1,5 +1,4 @@ use crate::{ - micropython::buffer::StrBuffer, trezorhal::random, ui::{ component::{text::common::TextBox, Child, Component, ComponentExt, Event, EventCtx}, @@ -21,8 +20,6 @@ pub enum PinEntryMsg { } const MAX_PIN_LENGTH: usize = 50; -const MAX_VISIBLE_DOTS: usize = 18; -const MAX_VISIBLE_DIGITS: usize = 18; const CHOICE_LENGTH: usize = 13; const DELETE_INDEX: usize = 0; @@ -33,13 +30,11 @@ const CHOICES: [&str; CHOICE_LENGTH] = [ "DELETE", "SHOW", "ENTER", "0", "1", "2", "3", "4", "5", "6", "7", "8", "9", ]; -struct ChoiceFactoryPIN { - prompt: StrBuffer, -} +struct ChoiceFactoryPIN {} impl ChoiceFactoryPIN { - fn new(prompt: StrBuffer) -> Self { - Self { prompt } + fn new() -> Self { + Self {} } } @@ -75,23 +70,32 @@ impl ChoiceFactory for ChoiceFactoryPIN { } /// Component for entering a PIN. -pub struct PinEntry { +pub struct PinEntry { choice_page: ChoicePage, - pin_dots: Child>>, + pin_line: Child>>, + subprompt_line: Child>, + prompt: T, show_real_pin: bool, textbox: TextBox, } -impl PinEntry { - pub fn new(prompt: StrBuffer) -> Self { - let choices = ChoiceFactoryPIN::new(prompt); +impl PinEntry +where + T: AsRef + Clone, +{ + pub fn new(prompt: T, subprompt: T) -> Self { + let choices = ChoiceFactoryPIN::new(); Self { // Starting at the digit 0 choice_page: ChoicePage::new(choices) .with_initial_page_counter(NUMBER_START_INDEX as u8) .with_carousel(true), - pin_dots: Child::new(ChangingTextLine::center_mono(String::new())), + pin_line: Child::new(ChangingTextLine::center_bold(String::from( + prompt.clone().as_ref(), + ))), + subprompt_line: Child::new(ChangingTextLine::center_mono(subprompt)), + prompt, show_real_pin: false, textbox: TextBox::empty(), } @@ -106,23 +110,39 @@ impl PinEntry { self.textbox.delete_last(ctx); } - fn update_pin_dots(&mut self, ctx: &mut EventCtx) { - // TODO: this is the same action as for the passphrase entry, - // might do a common component that will handle this part, - // (something like `SecretTextLine`) - // also with things like shifting the dots when too many etc. - // TODO: when the PIN is longer than fits the screen, we might show ellipsis - if self.show_real_pin { - let pin = String::from(self.pin()); - self.pin_dots.inner_mut().update_text(pin); + /// Performs overall update of the screen. + fn update(&mut self, ctx: &mut EventCtx) { + self.update_header_info(ctx); + ctx.request_paint(); + } + + /// Update the header information - (sub)prompt and visible PIN. + /// If PIN is empty, showing prompt in `pin_line` and sub-prompt in the + /// `subprompt_line`. Otherwise disabling the `subprompt_line` and showing + /// the PIN - either in real numbers or masked in asterisks. + fn update_header_info(&mut self, ctx: &mut EventCtx) { + let show_prompts = self.is_empty(); + + let text = if show_prompts { + String::from(self.prompt.as_ref()) + } else if self.show_real_pin { + String::from(self.pin()) } else { let mut dots: String = String::new(); for _ in 0..self.textbox.len() { unwrap!(dots.push_str("*")); } - self.pin_dots.inner_mut().update_text(dots); - } - self.pin_dots.request_complete_repaint(ctx); + dots + }; + + // Putting the current text into the PIN line. + self.pin_line.inner_mut().update_text(text); + // Showing subprompt only conditionally. + self.subprompt_line.inner_mut().show_or_not(show_prompts); + + // Force repaint of the whole header. + self.pin_line.request_complete_repaint(ctx); + self.subprompt_line.request_complete_repaint(ctx); } pub fn pin(&self) -> &str { @@ -138,13 +158,19 @@ impl PinEntry { } } -impl Component for PinEntry { +impl Component for PinEntry +where + T: AsRef + Clone, +{ type Msg = PinEntryMsg; fn place(&mut self, bounds: Rect) -> Rect { - let pin_area_height = self.pin_dots.inner().needed_height(); - let (pin_area, choice_area) = bounds.split_top(pin_area_height); - self.pin_dots.place(pin_area); + let pin_height = self.pin_line.inner().needed_height(); + let subtitle_height = self.subprompt_line.inner().needed_height(); + let (title_area, subtitle_and_choice_area) = bounds.split_top(pin_height); + let (subtitle_area, choice_area) = subtitle_and_choice_area.split_top(subtitle_height); + self.pin_line.place(title_area); + self.subprompt_line.place(subtitle_area); self.choice_page.place(choice_area); bounds } @@ -153,7 +179,7 @@ impl Component for PinEntry { // Any event when showing real PIN should hide it if self.show_real_pin { self.show_real_pin = false; - self.update_pin_dots(ctx); + self.update(ctx) } let msg = self.choice_page.event(ctx, event); @@ -162,19 +188,16 @@ impl Component for PinEntry { match page_counter as usize { DELETE_INDEX => { self.delete_last_digit(ctx); - self.update_pin_dots(ctx); - ctx.request_paint(); + self.update(ctx); } SHOW_INDEX => { self.show_real_pin = true; - self.update_pin_dots(ctx); - ctx.request_paint(); + self.update(ctx); } ENTER_INDEX => return Some(PinEntryMsg::Confirmed), _ => { if !self.is_full() { self.append_new_digit(ctx, page_counter); - self.update_pin_dots(ctx); // Choosing any random digit to be shown next let new_page_counter = random::uniform_between( NUMBER_START_INDEX as u32, @@ -182,7 +205,7 @@ impl Component for PinEntry { ); self.choice_page .set_page_counter(ctx, new_page_counter as u8); - ctx.request_paint(); + self.update(ctx); } } } @@ -191,7 +214,8 @@ impl Component for PinEntry { } fn paint(&mut self) { - self.pin_dots.paint(); + self.pin_line.paint(); + self.subprompt_line.paint(); self.choice_page.paint(); } } @@ -200,7 +224,10 @@ impl Component for PinEntry { use super::{ButtonAction, ButtonPos}; #[cfg(feature = "ui_debug")] -impl crate::trace::Trace for PinEntry { +impl crate::trace::Trace for PinEntry +where + T: AsRef + Clone, +{ fn get_btn_action(&self, pos: ButtonPos) -> String<25> { match pos { ButtonPos::Left => ButtonAction::PrevPage.string(), diff --git a/core/embed/rust/src/ui/model_tr/layout.rs b/core/embed/rust/src/ui/model_tr/layout.rs index bee094c546..5cbf35cec3 100644 --- a/core/embed/rust/src/ui/model_tr/layout.rs +++ b/core/embed/rust/src/ui/model_tr/layout.rs @@ -82,7 +82,10 @@ where } } -impl ComponentMsgObj for PinEntry { +impl ComponentMsgObj for PinEntry +where + T: AsRef + Clone, +{ fn msg_try_into_obj(&self, msg: Self::Msg) -> Result { match msg { PinEntryMsg::Confirmed => self.pin().try_into(), @@ -504,11 +507,11 @@ extern "C" fn tutorial(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj extern "C" fn request_pin(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj { let block = |_args: &[Obj], kwargs: &Map| { let prompt: StrBuffer = kwargs.get(Qstr::MP_QSTR_prompt)?.try_into()?; - let _subprompt: StrBuffer = kwargs.get(Qstr::MP_QSTR_subprompt)?.try_into()?; + let subprompt: StrBuffer = kwargs.get(Qstr::MP_QSTR_subprompt)?.try_into()?; let _allow_cancel: Option = kwargs.get(Qstr::MP_QSTR_allow_cancel)?.try_into_option()?; - let obj = LayoutObj::new(PinEntry::new(prompt))?; + let obj = LayoutObj::new(PinEntry::new(prompt, subprompt))?; Ok(obj.into()) }; unsafe { util::try_with_args_and_kwargs(n_args, args, kwargs, block) } diff --git a/core/src/apps/management/change_pin.py b/core/src/apps/management/change_pin.py index 399877c654..677ce8367f 100644 --- a/core/src/apps/management/change_pin.py +++ b/core/src/apps/management/change_pin.py @@ -50,13 +50,13 @@ async def change_pin(ctx: Context, msg: ChangePin) -> Success: if newpin: if curpin: - msg_screen = "You have successfully changed your PIN." + msg_screen = "PIN changed." msg_wire = "PIN changed" else: - msg_screen = "You have successfully enabled PIN protection." + msg_screen = "PIN protection enabled." msg_wire = "PIN enabled" else: - msg_screen = "You have successfully disabled PIN protection." + msg_screen = "PIN protection disabled." msg_wire = "PIN removed" await show_success(ctx, "success_pin", msg_screen) @@ -68,30 +68,30 @@ def _require_confirm_change_pin(ctx: Context, msg: ChangePin) -> Awaitable[None] has_pin = config.has_pin() + title = "PIN settings" + br_code = "set_pin" + if msg.remove and has_pin: # removing pin return confirm_pin_action( ctx, - "set_pin", - "Remove PIN", + br_code, + title, "disable PIN protection?", - "Do you really want to", ) if not msg.remove and has_pin: # changing pin return confirm_pin_action( ctx, - "set_pin", - "Change PIN", + br_code, + title, "change your PIN?", - "Do you really want to", ) if not msg.remove and not has_pin: # setting new pin return confirm_set_new_pin( ctx, - "set_pin", - "Enable PIN", - "Do you really want to", + br_code, + title, "enable PIN protection?", [ "PIN will be used to access this device.", diff --git a/core/src/apps/management/change_wipe_code.py b/core/src/apps/management/change_wipe_code.py index 6700008279..24a1796ff2 100644 --- a/core/src/apps/management/change_wipe_code.py +++ b/core/src/apps/management/change_wipe_code.py @@ -46,13 +46,13 @@ async def change_wipe_code(ctx: Context, msg: ChangeWipeCode) -> Success: if wipe_code: if has_wipe_code: - msg_screen = "You have successfully changed the wipe code." + msg_screen = "Wipe code changed." msg_wire = "Wipe code changed" else: - msg_screen = "You have successfully set the wipe code." + msg_screen = "Wipe code enabled." msg_wire = "Wipe code set" else: - msg_screen = "You have successfully disabled the wipe code." + msg_screen = "Wipe code disabled." msg_wire = "Wipe code removed" await show_success(ctx, "success_wipe_code", msg_screen) @@ -63,33 +63,35 @@ def _require_confirm_action( ctx: Context, msg: ChangeWipeCode, has_wipe_code: bool ) -> Awaitable[None]: from trezor.wire import ProcessError - from trezor.ui.layouts import confirm_pin_action + from trezor.ui.layouts import confirm_pin_action, confirm_set_new_pin + + title = "Wipe code settings" if msg.remove and has_wipe_code: return confirm_pin_action( ctx, "disable_wipe_code", - "Disable wipe code", + title, "disable wipe code protection?", - "Do you really want to", ) if not msg.remove and has_wipe_code: return confirm_pin_action( ctx, "change_wipe_code", - "Change wipe code", + title, "change the wipe code?", - "Do you really want to", ) if not msg.remove and not has_wipe_code: - return confirm_pin_action( + return confirm_set_new_pin( ctx, "set_wipe_code", - "Set wipe code", - "set the wipe code?", - "Do you really want to", + title, + "enable wipe code?", + [ + "Wipe code will\nbe used to delete this device.", + ], ) # Removing non-existing wipe code. @@ -110,7 +112,7 @@ async def _request_wipe_code_confirm(ctx: Context, pin: str) -> str: ) continue - code2 = await request_pin(ctx, "Re-enter new wipe code") + code2 = await request_pin(ctx, "Re-enter wipe code") if code1 == code2: return code1 # _wipe_code_mismatch diff --git a/core/src/trezor/ui/layouts/tr/__init__.py b/core/src/trezor/ui/layouts/tr/__init__.py index 5c43190ab9..70146d4c57 100644 --- a/core/src/trezor/ui/layouts/tr/__init__.py +++ b/core/src/trezor/ui/layouts/tr/__init__.py @@ -1254,7 +1254,6 @@ async def request_pin_on_device( prompt: str, attempts_remaining: int | None, allow_cancel: bool, - shuffle: bool = False, ) -> str: if attempts_remaining is None: subprompt = "" @@ -1263,18 +1262,6 @@ async def request_pin_on_device( else: subprompt = f"{attempts_remaining} tries left" - if attempts_remaining is not None: - await confirm_action( - ctx, - "pin_device_info", - "PIN entry", - action=prompt, - description=subprompt, - verb="BEGIN", - verb_cancel=None, - br_code=ButtonRequestType.Other, # cannot use BRT.PinEntry, as debuglink would be sending PIN to this screen - ) - await button_request(ctx, "pin_device", code=ButtonRequestType.PinEntry) dialog = RustLayout( @@ -1282,7 +1269,6 @@ async def request_pin_on_device( prompt=prompt, subprompt=subprompt, allow_cancel=allow_cancel, - shuffle=shuffle, # type: ignore [No parameter named "shuffle"] ) ) @@ -1290,9 +1276,6 @@ async def request_pin_on_device( result = await ctx.wait(dialog) if result is trezorui2.CANCELLED: raise wire.PinCancelled - # TODO: strangely sometimes in UI tests, the result is `CONFIRMED` - # For example in `test_set_remove_wipe_code`, `test_set_pin_to_wipe_code` or - # `test_change_pin` assert isinstance(result, str) return result @@ -1350,19 +1333,25 @@ async def confirm_set_new_pin( br_type: str, title: str, action: str, - description: str, information: list[str], + description: str = "Do you want to", br_code: ButtonRequestType = ButtonRequestType.Other, ) -> None: await confirm_action( ctx, br_type, title, - action=f"{description} {action}", + description=f"{description} {action}", verb="ENABLE", br_code=br_code, ) + # TODO: this is a hack to put the next info on new screen in case of wipe code + # TODO: there should be a possibility to give a list of strings and each of them + # would be rendered on a new screen + if len(information) == 1: + information.append("\n") + information.append( "Position of individual numbers will change between entries for more security." ) diff --git a/core/src/trezor/ui/layouts/tt_v2/__init__.py b/core/src/trezor/ui/layouts/tt_v2/__init__.py index b7ef4ad6fe..ba4992e317 100644 --- a/core/src/trezor/ui/layouts/tt_v2/__init__.py +++ b/core/src/trezor/ui/layouts/tt_v2/__init__.py @@ -1134,7 +1134,7 @@ async def confirm_pin_action( br_type: str, title: str, action: str | None, - description: str | None, + description: str | None = "Do you really want to", br_code: ButtonRequestType = ButtonRequestType.Other, ) -> None: return await confirm_action( @@ -1184,8 +1184,8 @@ async def confirm_set_new_pin( br_type: str, title: str, action: str, - description: str, information: list[str], + description: str = "Do you want to", br_code: ButtonRequestType = ButtonRequestType.Other, ) -> None: await confirm_action(