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

Published 20 hours ago • verified publisher icon MetaMask

Fix #15050 - MV3: Keep the user logged in when service worker restarts

Open darkwing opened this issue 3 years ago • 1 comments

Explanation

Currently, when the service worker restarts, the user gets logged out. This PR is the extension portion of the login fix. More details will be added as we get further along.

More Information

  • Fixes #15050
  • See: #67890

Depends on:

  • eth-keyring-controller (https://github.com/MetaMask/KeyringController/pull/147)

Manual Testing Steps

  • Start the extension in mv3 mode: yarn start:mv3
  • Login to MetaMask
  • ... wait 5+ minutes for the service worker to restart ...
  • Open the extension
  • See that you're still logged in

Pre-Merge Checklist

  • [ ] PR template is filled out
  • [ ] IF this PR fixes a bug, a test that would have caught the bug has been added
  • [ ] PR is linked to the appropriate GitHub issue
  • [ ] PR has been added to the appropriate release Milestone

+ If there are functional changes:

  • [ ] Manual testing complete & passed
  • [ ] "Extension QA Board" label has been applied

darkwing avatar Aug 11 '22 18:08 darkwing

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 Aug 11 '22 18:08 github-actions[bot]

I've noticed the following behaviors:

  1. If I have the extension full window open, and I wait +5min idle, MM does not logout
  2. If I don't have the extension full window open, and I wait +5min idle, MM does logout
  3. If I terminate the Service Worker manually, and have the extension full window open, MM does logout
  4. If I terminate the Service Worker manually, and I don't have the extension full window open, MM does logout

About 3 and 4, I am not completely sure this would ever happen in a real life scenario, as I don't think anyone would terminate the Service Worker manually. Just including it here in case we do need to consider it @darkwing @danjm

Behavior 2

mm-popup-idle.webm

Behavior 3

mm-full-manual-restart.webm

Behavior 4

mm-popup-manual-restart.webm

seaona avatar Sep 22 '22 08:09 seaona

Some QA finding with last commits, I am getting the error Uncaught (in promise) Error: key is not extractable on the two Onboarding paths:

  • Onboarding Flow with Create Wallet

key-not-extractable-create.webm

  • Onboarding Flow with an Imported Account

key-not-extractable.webm

seaona avatar Oct 11 '22 09:10 seaona

Started re-testing with the last changes and all previous errors mentioned above disappeared :star:

I saw that the error vault.map is not a function is triggered after I log out and try to log in again. Not sure if it's something about my build or a global error for everyone.

Repro steps:

  1. Load MM
  2. Go to Create Wallet onboarding flow
  3. Lock MM
  4. Try to unlock MM -- see error on the UI

vault-map-notfunc.webm

seaona avatar Oct 26 '22 10:10 seaona

Thanks @seaona ! I spotted this issue yesterday, as I was missing an await in the browser-passworder PR; this should be fixed. If you're still seeing it, I think it's because NPM is using cached information

darkwing avatar Oct 26 '22 13:10 darkwing

Overall, the functionality looks great. User is kept logged in after service worker is killed and re-started and I can normally log in and log out.

Exploring some other functionalities, I've encountered the following behaviors, which might/might not be related to this PR:

  1. Auto lock functionality is altered: when I set an auto-lock value, I see that I am logged out v fast, without respecting my input value. Furthermore, when I disable again the functionality, by introducing 0 mins, I see that I am still being logged out after some seconds.

https://user-images.githubusercontent.com/54408225/198875730-dd47f98b-9dc9-4b89-a7a5-dcf48027b0a8.mp4

  1. Logged out after changing networks: I've been logged out after terminating service worker and changing the network. :warning: hard to repro (investigating steps, as it's not deterministic).

https://user-images.githubusercontent.com/54408225/198878983-a9a34d74-2ffa-4992-9b62-a06a66e2ae68.mp4

  1. ~~Token detection functionality is altered: when I terminate the service worker, with the token detection enabled, I see that the user is kept logged in, but it seems that token detection is mixing accounts, when I change accounts a couple of times.~~ I've opened a separate issue for this one

Let me know if I should open separate tickets for solving this issues, or if any of theses issues should be approached by this PR. It's hard for me to tell, as on the develop branch I cannot fully test the flows without this PR being merged on develop. So I would be of the opinion of trying to merge this PR and fixing this separately, except if any of these issues is strictly related to the PR.

seaona avatar Oct 30 '22 11:10 seaona

  1. MetamaskController - No HD Key Tree found: when I terminate the service worker and try to Import an Account with a Private Key, I see the error MetamaskController - No HD Key Tree found on the Import Account page.

image

Repro steps:

  1. Click Create an Account
  2. Terminate the service worker
  3. Change selected Account
  4. Terminate the service worker
  5. Click Create -- nothing happens
  6. Click Create -- account is created
  7. Change Account
  8. Terminate the service worker
  9. Click Import Account -- see error

https://user-images.githubusercontent.com/54408225/199482784-b3e8ade4-63f3-467e-b779-87f0b149c850.mp4

  1. Logged out after changing accounts: when I terminate the service worker in between changing the selected account, I am logged out

Repro steps:

  1. Start from Home page
  2. Terminate the service worker
  3. Change to another account
  4. Terminate the service worker (before account is loaded) -- you are logged out

https://user-images.githubusercontent.com/54408225/199484700-62a3d3b2-238b-430d-a4f2-80990538c541.mp4

seaona avatar Nov 02 '22 11:11 seaona

  1. After reviewing the issue [Bug]: [MV3] Error: Attempting to use a Disconnected port object and MM stays loading indefinetly #16334 with @jpuri , we've noticed that it's only reproduceable on this PR and not in develop. Needs further investigation on what is causing the error

seaona avatar Nov 02 '22 14:11 seaona

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]

Powered by socket.dev

socket-security[bot] avatar Nov 03 '22 14:11 socket-security[bot]

With the recent changes, I am being logged out after terminating the service worker. Do I need to do a manual extra step now?

https://user-images.githubusercontent.com/54408225/200333121-31f9c404-b126-40ff-93b8-ad49594bde6c.mp4

seaona avatar Nov 07 '22 14:11 seaona

i'm getting the following error on the background when building the extension in test mode:

Error: LavaMoat - required package not in allowlist: package "@metamask/controllers>eth-keyring-controller" requested "browserify>events" as "events"

PeterYinusa avatar Nov 10 '22 14:11 PeterYinusa

Weird for Import Account using private key of an already active account should result in an error to fail, as I just tested it manually and I do see the desired error.

darkwing avatar Nov 10 '22 16:11 darkwing

This PR needs https://github.com/MetaMask/KeyringController/pull/161 to be merged and a new version cut.

darkwing avatar Nov 10 '22 18:11 darkwing

Builds ready [b2f26dc]
Page Load Metrics (2146 ± 105 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint912327219484232
domContentLoaded171424492119228109
load173224732146219105
domInteractive171424492119228109
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22289 bytes
  • ui: 87 bytes
  • common: 136 bytes

metamaskbot avatar Nov 11 '22 16:11 metamaskbot

Builds ready [1d44e84]
Page Load Metrics (2135 ± 100 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97140118136
domContentLoaded176024172113207100
load176024332135208100
domInteractive176024172113207100
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23039 bytes
  • ui: 6349 bytes
  • common: 483 bytes

metamaskbot avatar Nov 17 '22 17:11 metamaskbot

Builds ready [043e121]
Page Load Metrics (1992 ± 95 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint8612310194
domContentLoaded16062269198619794
load16232269199219995
domInteractive16062269198619794
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22262 bytes
  • ui: 15 bytes
  • common: 136 bytes

metamaskbot avatar Nov 21 '22 20:11 metamaskbot

Builds ready [71a087f]
Page Load Metrics (2140 ± 87 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint87135107147
domContentLoaded17522393212017182
load17532393214018287
domInteractive17522393212017182
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 21911 bytes
  • ui: 15 bytes
  • common: 136 bytes

metamaskbot avatar Nov 22 '22 22:11 metamaskbot

Builds ready [c2577d9]
Page Load Metrics (2223 ± 113 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint962004216411197
domContentLoaded176024942205231111
load176024942223235113
domInteractive176024942205231111
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22109 bytes
  • ui: 15 bytes
  • common: 136 bytes

metamaskbot avatar Nov 23 '22 00:11 metamaskbot

@darkwing : Code changes look good and seems to work well 👍

I see one scenario breaking. If I login in MV2 and then start extension in MV3, I get this error while trying to login again using password:

Screenshot 2022-11-23 at 8 46 36 AM

jpuri avatar Nov 23 '22 03:11 jpuri

I see one scenario breaking. If I login in MV2 and then start extension in MV3, I get this error while trying to login again using password

That should be the Ledger hardware wallet error, correct? That's a separate issue being worked on by the hardware wallets team.

darkwing avatar Nov 23 '22 14:11 darkwing

Specifically, commenting out this line prevents the problem:

https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/main/index.js#L40

The issue described isn't specific to or introduced by this PR.

darkwing avatar Nov 23 '22 14:11 darkwing

Builds ready [62fc4a2]
Page Load Metrics (2342 ± 170 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint911141183221106
domContentLoaded172734872327363175
load172734872342354170
domInteractive172734872327363175
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22121 bytes
  • ui: 15 bytes
  • common: 136 bytes

metamaskbot avatar Nov 23 '22 17:11 metamaskbot