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

fix: vault recovery & invalid password error #6957

Merged
merged 6 commits into from
Aug 16, 2023
Merged

fix: vault recovery & invalid password error #6957

merged 6 commits into from
Aug 16, 2023

Conversation

jpcloureiro
Copy link
Contributor

@jpcloureiro jpcloureiro commented Aug 2, 2023 •
edited

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add the appropiate QA label when dev review is completed
    • needs-qa: PR requires manual QA.
    • No QA/E2E only: PR does not require any manual QA effort. Prior to merging, ensure that you have successful end-to-end test runs in Bitrise.
    • Spot check on release build: PR does not require feature QA but needs non-automated verification. In the description section, provide test scenarios. Add screenshots, and or recordings of what was tested.
  5. Add QA Passed label when QA has signed off (Only required if the PR was labeled with needs-qa)
  6. Add your team's label, i.e. label starting with team- (or external-contributor label if your not a MetaMask employee)

Description

Setting state.user.passwordSet on userAuth & appAuth prevents "Protect Your Wallet" modal from redirecting the user to "Create Password" flow when there's already a password set.

Resetting this.engineInitialized ensures that the action 'INIT_BG_STATE' gets dispatched after vault recovery. This will place vault value in the KeyringController redux store state.

This PR aims to fix the following cases

[Case 1]

Vault Recovery forces user to go through the flow of creating a new password. When trying to set the new password the error "Invalid Password" appears
Steps to Reproduce:

Install Apk build 1127 v7.1.0
Create new account
Navigate to main wallet view and kill app
Install Apk build
Go through the vault recovery and navigate to main wallet view
"Protect your wallet" modal pops up. Click "Protect wallet" button
Type password & confirm password
Error "Invalid password" appears

[Case 2]

Vault corruption flow is stuck in a loop. When the user kills and relaunches the app, user is taken through the vault recovery flow again repeatedly.

Steps to reproduce:

Install v7.1.0 (1127) on IOS device using TestFlight
Create wallet and navigate to main wallet view
Kill app, and update to using TestFlight
Relaunch app, input password on login page
Go through the vault recovery and navigate to main wallet view
Kill app, then relaunch app

Resources

https://recordit.co/4heyZtR52I
Demo on iOS simulator running main branch.
First run shows app running without issues.
Simulate an app upgrade with a bad redux store state migration by adding a new migration without returning the state
See vault recovery screens & "Protect your wallet" modal after recovery.
Click "Protect wallet" takes the user to create a password. This is where we see the "Invalid password" error. [Case 1]
Relaunching the app will bring back the vault recovery. It will happen on every relaunch [Case 2]

https://recordit.co/0GcKBCXHmR
Demo on iOS simulator running fix-invalid-password-error branch.
Show fix for [Case 1] isolated from [Case 2] by checking previous commit.
See that "Protect your wallet" does not appear after vault recovery.

Show fix for [Case 2] by checking out latest commit of PR branch.
See that after a vault recovery we can relaunch the app without triggering vault recovery again.

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/1160

Related to #6584

Checklist

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

jpcloureiro and others added 3 commits August 1, 2023 16:20
@jpcloureiro
ResetPassword: remove unused redux action passwordUnset
685cf03
@owencraston @jpcloureiro
Authentication: set state.user.passwordSet boolean after successful…
7a74142 
… auth
@jpcloureiro
Vault Recovery: reset engine initialization
b087c15 
set EngineService engineInitialized instance to false to ensure we
dispatch INIT_BG_STATE_KEY. This will save KeyringController state object
to be saved into redux store state
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

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.

@jpcloureiro jpcloureiro changed the title Fix invalid password error fix: vault recovery Aug 2, 2023
@jpcloureiro jpcloureiro changed the title fix: vault recovery fix: vault recovery & invalid password error Aug 2, 2023
@jpcloureiro jpcloureiro self-assigned this Aug 2, 2023
@jpcloureiro jpcloureiro marked this pull request as ready for review August 2, 2023 21:03
@jpcloureiro jpcloureiro requested a review from a team as a code owner August 2, 2023 21:03
@jpcloureiro jpcloureiro added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 2, 2023
@gauthierpetetin gauthierpetetin added in-progress needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) in-progress labels Aug 2, 2023
NicolasMassart
NicolasMassart approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

Please ask for another approval but for what I read it looks good.

Cal-L
Cal-L requested changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Could you provide info for expected behavior on both scenario 1 and 2 + provide videos demonstrating the fix?

@Cal-L
Copy link
Contributor

Cal-L commented Aug 3, 2023

Could you provide info for expected behavior on both scenario 1 and 2 + provide videos demonstrating the fix?

@jpcloureiro Are the 2 cases listed steps to reproduce or steps to verify the fix? In both scenarios, it states to install the APK/Testflight build but doesn't have the build numbers associated with them

@jpcloureiro
Copy link
Contributor Author

jpcloureiro commented Aug 3, 2023 •
edited

Could you provide info for expected behavior on both scenario 1 and 2 + provide videos demonstrating the fix?

@jpcloureiro Are the 2 cases listed steps to reproduce or steps to verify the fix? In both scenarios, it states to install the APK/Testflight build but doesn't have the build numbers associated with them

I've updated this PR description with videos and brief explanation on how to replicate the problems and fixes running local development builds.

Steps to reproduce will have build numbers to help QA verify the acceptance criteria

Cal-L
Cal-L approved these changes Aug 4, 2023
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Thanks for the videos. LGTM!

@Cal-L Cal-L 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 Aug 4, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead label Aug 14, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

30.0% 30.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Aug 14, 2023
@hesterbruikman
Copy link
Contributor

hesterbruikman commented Aug 15, 2023 •
edited

Manual test findings:

1. Upgrade after vault corruption during wallet creation
Android 11
Samsung Galaxy S10e

  1. Install 6.6
  2. Create wallet
  3. Trigger vault corruption following these steps
  4. Upgrade to 7.4.1 (including PR fix)

Result: ✅ Success
Outcome: Reset password
Recording

2. Upgrade after vault corruption during wallet import
Android 11
Samsung Galaxy S10e

  1. Install 6.6
  2. Create wallet
  3. Trigger vault corruption following these steps
  4. Upgrade to 7.4.1 (including fix)

Result: ✅ Success
Outcome: Back up SRP and set password
Recording

3. Trigger vault corruption on 7.4.1
Android 11
Samsung Galaxy S10e

Android 9
Fairphone 2

  1. Install 7.4.1
  2. Create wallet
  3. Trigger vault corruption following these steps

Result: ✅ Success (can't trigger)
Outcome: Back up SRP and set password
Recording

@AlexHerman1 AlexHerman1 added CS-reported issues reported by CS CS-tracking issues being tracked by / relevant to CS labels Aug 15, 2023
cortisiko
cortisiko approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

Parameters for testing:

  • Imported Wallet Biometrics enabled
  • New Wallet Biometrics enabled
  • Imported Wallet Biometrics disabled.
  • New Wallet Biometrics disabled.

Devices used:

iOS: iPhone Xr, 11, iPhone SE
Android: Pixel 3XL, Xiaomi Redmi Note 10, Samsung S8

Scenarios I covered:

Biometrics enabled (FaceID and FingerPrint):

  • Updating from v6.6 (1113) to v7.1 (1125) to trigger the bug in the vault corruption flow then updating to the build with the fix [v7.4.1 (1161)]

  • Updating from all versions of the app since the vault corruption bug was introduced v7.1 - v7.4 to the build with the fix [v7.4.1 (1161)]

  • Updating from v6.6 (1113) to v7.1 (1125) to trigger the bug in the vault corruption flow then updating to the build with the fix [v7.4.1 (1161)] I ensured that I can successfully enter my password. I went ahead and changed my password, killed and relaunch the app then authenticate with password instead of biometrics.

  • Import wallet on v6.6 (1113) then changed password. Updated to v7.1 (1125) to trigger the bug in the vault corruption flow then updated to the build with the fix [v7.4.1 (1161)].

I can confirm the invalid password error does not appear. I am able to successfully recover my wallet.

No biometrics enabled (NO FaceID and FingerPrint):

  • Updating from v6.6 (1113) to v7.1 (1125) to trigger the bug in the vault corruption flow then updating to the build with the fix [v7.4.1 (1161)]

  • Updating from all versions of the app since the vault corruption bug was introduced v7.1 - v7.4 to the build with the fix [v7.4.1 (1161)]

  • Updating from v6.6 (1113) to v7.1 (1125) to trigger the bug in the vault corruption flow then updating to the build with the fix [v7.4.1 (1161)] I ensured that I can successfully enter my password. I went ahead and changed my password, killed and relaunch the app then authenticate with password instead of biometrics.

  • Import wallet on v6.6 (1113) then changed password. Updated to v7.1 (1125) to trigger the bug in the vault corruption flow then updated to the build with the fix [v7.4.1 (1161)].

I can confirm the invalid password error does not appear. I am able to successfully recover my wallet.

This PR is ✅

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Aug 15, 2023
@jpcloureiro jpcloureiro merged commit 2326d87 into main Aug 16, 2023
16 of 17 checks passed
@jpcloureiro jpcloureiro deleted the fix-invalid-password-error branch August 16, 2023 15:00
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2023
@metamaskbot metamaskbot added the release-7.6.0 Issue or pull request that will be included in release 7.6.0 label Aug 16, 2023
@sethkfman sethkfman added release-7.5.0 and removed release-7.6.0 Issue or pull request that will be included in release 7.6.0 labels Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NicolasMassart NicolasMassart NicolasMassart approved these changes

@Cal-L Cal-L Cal-L approved these changes

@cortisiko cortisiko cortisiko approved these changes

@owencraston owencraston owencraston approved these changes

Assignees

@jpcloureiro jpcloureiro

Labels
CS-reported issues reported by CS CS-tracking issues being tracked by / relevant to CS QA Passed A successful QA run through has been done release-7.5.0 team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@jpcloureiro @Cal-L @hesterbruikman @NicolasMassart @cortisiko @owencraston @gauthierpetetin @sethkfman @metamaskbot @AlexHerman1