Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[IMPROVEMENT] Warn when exporting SRP #3547

Merged
merged 26 commits into from
Feb 23, 2022
Merged

[IMPROVEMENT] Warn when exporting SRP #3547

merged 26 commits into from
Feb 23, 2022

Conversation

andreahaku
Copy link
Contributor

@andreahaku andreahaku commented Jan 7, 2022 •
edited
Loading

Description

Goal: Reduce exposure of SRP.
Intended effect: Users do not share their SRP with phishers/attackers.

User Story: https://www.notion.so/Warn-me-about-risks-when-I-m-exporting-my-seed-phrase-f610550a8c464240a46880b06ebbdc42
Design: https://www.figma.com/file/8z4Csc3wlOIkUz2OP1eTZW/Secure-UX-Improvements?node-id=44%3A1391
Metrics: https://docs.google.com/spreadsheets/d/1hxWnGzJI_8pfa8CwDqqoWAwkzuTyx6AukkmRBl3asC0/edit#gid=1224808665

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #3544

@andreahaku andreahaku self-assigned this Jan 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@andreahaku andreahaku marked this pull request as ready for review January 10, 2022 17:28
@andreahaku andreahaku requested a review from a team as a code owner January 10, 2022 17:28
@andreahaku andreahaku added Code Impact - Medium Average task code change that can relatively safely being applied to the codebase needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jan 10, 2022
@andreahaku andreahaku changed the title Warn when exporting SRP [IMPROVEMENT] Warn when exporting SRP Jan 11, 2022
sethkfman
sethkfman reviewed Jan 19, 2022
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor spelling issue

locales/languages/en.json Outdated Show resolved Hide resolved
@andreahaku andreahaku changed the base branch from develop to main January 26, 2022 18:35
sethkfman
sethkfman approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andreahaku andreahaku force-pushed the feature/reveal_srp_ui_update branch from ffbb950 to c65dec6 Compare February 4, 2022 10:14
@andreahaku andreahaku added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Feb 4, 2022
@andrepimenta andrepimenta added the Priority - High Task with high priority label Feb 4, 2022
@cortisiko cortisiko assigned plasmacorral and unassigned andreahaku Feb 8, 2022
@plasmacorral plasmacorral removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Feb 8, 2022
@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Feb 22, 2022
@bschorchit
Copy link

bschorchit commented Feb 22, 2022 •
edited
Loading

  1. The 60 second clipboard timer does not expire on Android

Is 9. still a problem?

plasmacorral
plasmacorral suggested changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@plasmacorral plasmacorral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tap and hold to reveal functionality is not currently working on android. I have directly shared state logs and an android bug report. On iOS the button can be tapped and held to reveal SRP.

Will pause further QA tests until there are some changes made for the android build.

@plasmacorral plasmacorral added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Feb 22, 2022
@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Feb 22, 2022
@plasmacorral
Copy link
Contributor

  1. The 60 second clipboard timer does not expire on Android

Is 9. still a problem?

Yes. Its a problem in that the expectation is set for the user that the clipboard will be purged after 60 seconds, but that is still not happening.

There is suspicion that this may be a limitation within android and if that's the case, we will at least need a copy update before this can be merged. @andreahaku is planning to investigate this further and will advise on our path forward.

@plasmacorral plasmacorral added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA in Progress QA has started on the feature. and removed QA in Progress QA has started on the feature. labels Feb 22, 2022
plasmacorral
plasmacorral suggested changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@plasmacorral plasmacorral left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm that (2) is addressed and functioning on both iOS and Android.

(9) is still a concern that I would not approve merging in the current state. For android devices we wither need to adjust the copy to remove the 60 second clipboard purge, or figure out how to make it more effective. I can still copy/paste well after 60 seconds.

I am still experiencing (3) sporadically. I suspect that it has to do with sensitivity to the position of where the user is touching and holding. If I am slow and deliberate with a consistent pressure, everything seems to work fine. However, if there is variation in the pressure applied or even a slight of shift of the held position, the hold is released even if the finger/stylus is not picked up.

I further explored this after changing Settings>Accessibility>Touch & hold delay. I tested short, medium and long. Changing the settings did not notably change the experience.

As-is, I could accept that I am not executing the "ideal" tap and hold with consistent pressure in the exact same position. However, I am curious if within the app we can adjust the sensitivity for the button and make this more user friendly? If I am aggressive or punchy with my tap, the button does not respond as a (this) user might hope.

Below are footage and recent logs for (3):
https://recordit.co/oziYihVwnm
State logs:
MetaMask State logs - v4.1-1.0 (816).zip
Android bug report:
bugreport-guamna_t-RZAS31.Q2-146-14-5-2022-02-22-17-17-12-1.zip

@plasmacorral plasmacorral removed the QA in Progress QA has started on the feature. label Feb 22, 2022
@andreahaku
Copy link
Contributor Author

andreahaku commented Feb 23, 2022 •
edited
Loading

@bschorchit @plasmacorral
Last commit updates the copy for Android when copying SRP ("Secret Recovery Phrase copied to clipboard" plain and simple). Let me know what you think about it.

There's not way to set the "sensibility" for the touch on touchable items on RN.
I personally tested on two physical devices (old Android 8 and new with Android 12) as well as emulator and had no issues with the 'Hold to reveal SRP' animated button.

@bschorchit
Copy link

Last commit updates the copy for Android when copying SRP ("Secret Recovery Phrase copied to clipboard" plain and simple). Let me know what you think about it.

Looks good to me! Thanks, Andrea!

@plasmacorral plasmacorral added the QA in Progress QA has started on the feature. label Feb 23, 2022
plasmacorral
plasmacorral approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@plasmacorral plasmacorral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observations 1,2,4,5,7, & 9 have been fixed and verified.
Observation 8 is out of scope for this change
Observation 6 appears throughout the app and is not a blocker here
Observation 3 is something I still occasionally experience, but I believe it to be related to variable position/pressure in the held tap and do not consider it to be a blocker.

With all that said, this is my approval.

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA in Progress QA has started on the feature. labels Feb 23, 2022
@andreahaku andreahaku merged commit 1db76ec into main Feb 23, 2022
@andreahaku andreahaku deleted the feature/reveal_srp_ui_update branch February 23, 2022 18:43
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sethkfman sethkfman sethkfman approved these changes

@plasmacorral plasmacorral plasmacorral approved these changes

Assignees

@plasmacorral plasmacorral

Labels
Code Impact - Medium Average task code change that can relatively safely being applied to the codebase Priority - High Task with high priority QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn when exporting SRP
6 participants
@andreahaku @bschorchit @plasmacorral @sethkfman @andrepimenta @Cal-L