Warn in Suite if Trezor has safety checks enabled #2282

tsusanka · 2020-09-02T11:58:42Z

Let's add a warning similar to "backup needed" that informs the user that they have "safety checks" enabled.

Related to #2242. Needs trezor/trezor-firmware#1133 and trezor/trezor-firmware#1193.

matejzak · 2021-03-12T11:41:49Z

After a brief discussion, we came to a conclusion (or rather hypothesis that needs to be verified) that since the safety checks Settings automatically reset with device reconnect, we don't need the banner notification.

@tsusanka will kindly check whether this is indeed true and will let us know 👆🏽 🙏🏽

cc @goodhoko

tsusanka · 2021-03-12T12:10:16Z

@mmilata it is true that when safety_checks are set to Prompt they will default to Strict on next Trezor replug, correct? Only PromptTemporarily does not do that but we have decided not to have that in Suite for now.

mmilata · 2021-03-12T12:17:49Z

They are two distinct values, PromptTemporarily and PromptAlways (in addition to Strict), PromptTemporarily is only until next reboot.

In trezorctl we expose these as:

trezorctl set safety-checks prompt
trezorctl set safety-checks prompt --always

i.e. PromptTemporarily is the default

prusnak · 2021-03-12T12:46:16Z

Let's add a warning similar to "backup needed" that informs the user that they have "safety checks" enabled.

@tsusanka by "enabled" you mean set to something else then "strict", right?

tsusanka · 2021-03-12T13:18:24Z

@prusnak yes indeed. Hm, so the user could have set PromptAlways via trezorctl and then go to Suite and they should probably see the warning there @matejzak 😕.

goodhoko · 2021-03-15T13:12:02Z

Hm, so the user could have set PromptAlways via trezorctl and then go to Suite and they should probably see the warning there.

Good point! We definitely want to show the warning when safety-check == PromptAlways.

The remaining question is: should the warning appear for PromptTemporarily too? I'd say, yes, for the sake of consistent UI and principle of least surprise. This is however mainly a product design question @matejzak .)

I'll assign this to myself to do it along #2242. Hope that's ok. .)

matejzak · 2021-03-16T17:55:17Z

The remaining question is: should the warning appear for PromptTemporarily too? I'd say, yes, for the sake of consistent UI and principle of least surprise. This is however mainly a product design question @matejzak .)

Let's do it. Here is the notification design: Zeplin
Currently being implemented by @tomasklim btw. 💡

Color: type-orange (Warning)
CTA link to Device Settings
Banner notification for PromptTemporarily should be dismissible: (1) the user made an action to reach that particular state and (2) it will be set to Strict with the next device reconnect. PromptAlways on the other hand, should not be dismissible: User could have set it via trezorctl.

The SafetyCheckLevel is deserialized as a string by the Link. Patch it's type via the existing patching mechinery. The type `Features.ssafety_checks` field is emitted as optional (ie. is typed as `undefined | SafetyCheckLevel`) but in runtime, it's absence is designated by `null` rather than `undefined`. Patch this manually too. Prerequisite for trezor/trezor-suite#2242 and trezor/trezor-suite#2282

…abled

matejzak · 2021-04-27T16:26:23Z

(1) Agreed — priority #6 sounds reasonable with respect to the scale of other items on the list.
(2) I would still prefer to link the CTA to the Device Settings to 'teach' the user where that particular options is located. That being said, I agree it's problematic (similar to what we are trying to solve in #2208) and it would be great to fix in the future, i.e. to give the user a visual cue where they should focus their attention (component blink, scroll to the page aka anchor link etc.). Putting on my things-to-clean-up list.
(3) I believe so, but no strong opinion, since it is "hard core" feature anyway and the device reconnect will reset it to default.
(4) Feel free to word it as you would and add it to CrowdIn (I guess you might have already), it will be later proofread there.

Many thanks!

When SafetyChecks are not "Strict" display a warning banner that points the user to Safety Checks settings. If the Safety Checks are "PromptTemporarily" allow the waring to be dismissed and not being displayed until either the Suite is restarted/reloaded or the Safety Checks are changed.

goodhoko · 2021-04-28T11:44:14Z

@matejzak

ACK
Ok then. I mentioned this in the issue and I'll also leave a TODO in the code.
I'll opportunistically take the other approach and make the dismissal last only for a session. For two reasons 1) implementation simplicity 2) it's more defensive. The worst case is an user annyoed by the warning. The worst case of persistent dismissal is an user unaware of disabeld safety checks approving non-standard (trans)action.
Ack.

When SafetyChecks are not "Strict" display a warning banner that points the user to Safety Checks settings. If the Safety Checks are "PromptTemporarily" allow the waring to be dismissed and not being displayed until either the Suite is restarted/reloaded or the Safety Checks are changed.

matejzak · 2021-04-28T12:03:31Z

Got it - no objections.

Thanks!

When SafetyChecks are not "Strict" display a warning banner that points the user to Safety Checks settings. If the Safety Checks are "PromptTemporarily" allow the waring to be dismissed and not being displayed until either the Suite is restarted/reloaded or the Safety Checks are changed.

When safety_checks are not "Strict" display a warning banner that points the user to safety_checks settings. If the safety_checks are "PromptTemporarily" allow the waring to be dismissed and not being displayed until either the Suite is restarted/reloaded or the safety_checks are changed.

goodhoko · 2021-05-04T13:18:11Z

Done in #3681. Closing.

bosomt · 2021-05-10T06:10:54Z

QA OK

Info:

Suite version: web 21.5.1 (4d09d88)
Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:88.0) Gecko/20100101 Firefox/88.0
OS: MacIntel
Screen: 1440x900
Device: model T 2.3.6 regular

STew790 · 2021-05-10T15:13:45Z

QA OK

Info:

web 21.5.1 (4d09d88)
Browser: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.66 Safari/537.36
OS: NixOS
Device Model T 2.3.5 regular

wendys-cats · 2021-05-10T18:58:55Z

QA OK

notification panel is visible when safety checks are disabled through Suite or when trezorctl set safet-checks prompt (--always) is used.

Info:

Suite version: web 21.5.1 (4d09d88)
Browser: Chromium Version 88.0.4324.182 (Official Build) (64-bit)
OS: NixOS 21.03
Device: TT 2.3.6 regular

tsusanka added the Feature request label Sep 2, 2020

tsusanka mentioned this issue Dec 15, 2020

Add option to settings to allow safety checks #2242

Closed

tsusanka self-assigned this Dec 15, 2020

matejzak assigned matejzak and unassigned tsusanka Feb 4, 2021

matejzak assigned tsusanka Mar 12, 2021

matejzak added notifications Notifications and removed Feature request labels Mar 12, 2021

matejzak removed their assignment Mar 12, 2021

goodhoko self-assigned this Mar 15, 2021

matejzak unassigned tsusanka Mar 16, 2021

goodhoko added a commit that referenced this issue Apr 23, 2021

fixup! feat: #2282 Warn if Safety Checks are disabled

be8ad43

goodhoko added a commit that referenced this issue Apr 23, 2021

fixup! fixup! feat: #2282 Warn if Safety Checks are disabled

2147890

goodhoko added a commit that referenced this issue Apr 23, 2021

fixup! fixup! fixup! feat: #2282 Warn if Safety Checks are disabled

cf7d4e8

goodhoko added a commit that referenced this issue Apr 26, 2021

fixup! fixup! fixup! fixup! feat: #2282 Warn if Safety Checks are dis…

d23309a

…abled

goodhoko mentioned this issue Apr 28, 2021

Anchor link to pending TX #2208

Closed

goodhoko added a commit that referenced this issue Apr 28, 2021

fixup! feat: #2282 Warn if Safety Checks are disabled

63680ae

goodhoko added a commit that referenced this issue Apr 28, 2021

fixup! feat: #2282 Warn if Safety Checks are disabled

49b7270

goodhoko added a commit that referenced this issue Apr 28, 2021

fixup! fixup! feat: #2282 Warn if Safety Checks are disabled

bebbf6d

goodhoko added a commit that referenced this issue Apr 28, 2021

fixup! fixup! fixup! feat: #2282 Warn if Safety Checks are disabled

6778da6

goodhoko added a commit that referenced this issue May 3, 2021

fixup! feat: #2282 Warn when safety_checks are disabled

6607e36

goodhoko added a commit that referenced this issue May 3, 2021

fixup! fixup! feat: #2282 Warn when safety_checks are disabled

1537c96

goodhoko closed this as completed May 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn in Suite if Trezor has safety checks enabled #2282

Warn in Suite if Trezor has safety checks enabled #2282

tsusanka commented Sep 2, 2020

matejzak commented Mar 12, 2021

tsusanka commented Mar 12, 2021

mmilata commented Mar 12, 2021

prusnak commented Mar 12, 2021

tsusanka commented Mar 12, 2021

goodhoko commented Mar 15, 2021

matejzak commented Mar 16, 2021 •

edited

Loading

matejzak commented Apr 27, 2021 •

edited

Loading

goodhoko commented Apr 28, 2021

matejzak commented Apr 28, 2021

goodhoko commented May 4, 2021

bosomt commented May 10, 2021

STew790 commented May 10, 2021

wendys-cats commented May 10, 2021

Warn in Suite if Trezor has safety checks enabled #2282

Warn in Suite if Trezor has safety checks enabled #2282

Comments

tsusanka commented Sep 2, 2020

matejzak commented Mar 12, 2021

tsusanka commented Mar 12, 2021

mmilata commented Mar 12, 2021

prusnak commented Mar 12, 2021

tsusanka commented Mar 12, 2021

goodhoko commented Mar 15, 2021

matejzak commented Mar 16, 2021 • edited Loading

matejzak commented Apr 27, 2021 • edited Loading

goodhoko commented Apr 28, 2021

matejzak commented Apr 28, 2021

goodhoko commented May 4, 2021

bosomt commented May 10, 2021

STew790 commented May 10, 2021

wendys-cats commented May 10, 2021

matejzak commented Mar 16, 2021 •

edited

Loading

matejzak commented Apr 27, 2021 •

edited

Loading