metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard

[IMPROVEMENT] Authentication Refactor

Open sethkfman opened this issue 3 years ago • 8 comments

Description

The goal of this refactor was to be able to remove logic from our UI components that handle authentication(login, logout, update password, update biometric) and centralize them in a single service.

This work is preparation to make it easier for additional refactors and improvements. There is still more opportunity to refactor vault creation but that will be completed in future work.

Checklist

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

Issue

Resolves #???

sethkfman avatar Feb 15 '22 16:02 sethkfman

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.

github-actions[bot] avatar Feb 15 '22 16:02 github-actions[bot]

@owencraston and myself paired and dev tested on this PR. We are feeling pretty good about the update but need review from other devs. Thx

sethkfman avatar Mar 23 '22 21:03 sethkfman

I am 35% done with testing.

cortisiko avatar May 10 '22 02:05 cortisiko

Issue 8

When i tried importing an account I am prompted to log in with faceID biometrics. It seems as if faceID biometrics is stuck in a loop for a brief moment then i am taken back to the onboarding view:

To reproduce: import your wallet with “unlock with biometrics” on open the account lists tap on “import account” enter your private key notice after you import the account, you are asked to use your biometrics again and again until you are finally kicked back to the onboarding view.

I also see this error undefined is not an object (evaluating address.toLowerCase) see here: http://recordit.co/1cgeuKBpVn

Issue 9 While going through the backup SRP flow I am prompted to enter my password and when i do enter my password I get an error Couldn’t unlock your account. Please try again”

to reproduce: creating your wallet with remember me/ unlock with biometrics toggled on land on the wallet view then kill the app relaunch the app now while on the wallet view, tap on the text “Protect wallet” in the back up alert notice you are taken to step 1 of the backup SRP page. proceed to step 2 “Secure wallet”. Here you would be asked to enter your password. If your device has biometrics enabled deny it. That way you can enter your password to proceed. enter your password. Notice the error Couldn’t unlock your account. Please try again see here: http://recordit.co/p8f1UpzA0A

Issue 10 This one is sometimes-ish meaning it is not a consistent bug to reproduce. When I attempt to deep link to polygon send flow i get: [TypeError: undefined is not an object (evaluating 'identities[selectedAddress].name')]

to reproduce: add polygon to your list of networks kill the app tap on https://metamask.app.link/send/0x0000000000000000000000000000000000001010@137/transfer?address=0xC5b2b5ae370876c0122910F92a13bef85A133E56&uint256=3e18

the error should appear.

Update on Issue 1B

I am still have to authenticate two times before unlocking the app. see here http://recordit.co/grrcMhTtaJ

Update on Issue 2: I am unable to disabled biometrics. The app logs me out, tries to authenticate using FaceID then prompts me with an error alert “invalid password”

see here: http://recordit.co/x6S1FYX1Nl

cortisiko avatar May 11 '22 00:05 cortisiko

I am 75% done with testing

cortisiko avatar May 11 '22 00:05 cortisiko

@cortisiko I made progress on most of the items but still have to reply to @owencraston feedback. I think this will need to wait until May 24th. Unless @owencraston ends up with some free time. No pressure @owencraston ;-)

  • Outstanding issues:
    • Issue 2
  • New issue 11: If you lock the wallet (biometrics enabled), kill the app, then start the app, in the login view the biometric button on the password window appears but doesn't work (it should be hidden)

sethkfman avatar May 13 '22 23:05 sethkfman

Hey @sethkfman I tested the fixes on your latest commit. Most of the issues are fixed.

Issue 9 is fixed but we have a cosmetic issue. The color of the masked password differs from that of prod. On your PR the color is black while in prod it is white.

I do have another issue to report. Call this one Issue 12.

Issue 12

As a new user when I declined to authenticate using FaceID (here visual representation of what I mean) I get into a broken state. Meaning I am stuck on the Create your wallet view.

To reproduce: Create a new wallet You should be prompted with an alert message asking you to Allow or disallow Face ID (this is what I mean.) Tap on “don’t allow” Notice you are in a broken state.

See here

I will be OOO until June 6th. If you are not too swamped @plasmacorral can you keep things rolling on this? Thanks!

cortisiko avatar May 20 '22 23:05 cortisiko

Break out onLogin to catch separate issues: https://sentry.io/organizations/metamask/issues/2034834647/?project=2299799&query=is%3Aunresolved+release.version%3A5.3.0+release.build%3A927&sort=freq&statsPeriod=30d

sethkfman avatar Jul 12 '22 17:07 sethkfman

Close PR due to inactivity

sethkfman avatar Jan 30 '23 13:01 sethkfman