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.

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

fix: Ensure phishing stale list network request is not sent before onboarding and if basic functionality is off #25306

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Jun 13, 2024

Description

This fix ensures that network requests for the phishing list are sent in two cases:

  1. before onboarding is complete
  2. after onboarding is complete if either the basic functionality toggle or the use phishing detection toggle is off

To ensure the first, a call to this.phishingController.maybeUpdateState() in the metamask controller constructor was moved to the postOnboardingInitialization function.

To ensure #2, two fixes were needed:

  • prevent the aforementioned call from occury of the usePhishDetect preference property is false
  • have the setUsePhishDetect call that is made in handleSubmit of onboarding-flow/privacy-settings/privacy-settings.js submit false if the basic functionality toggle is off and the user has not independently set the phishing detection toggle to on. This requires, in the advanced section of the onboarding flow, to default the phishing detection toggle setting to the basic functionality toggle value (externalServicesOnboardingToggleState), but then only using the phishing toggle value if the user sets that independently.

the basic-functionality.spec.js e2e tests were updated to ensure that when the basic functionality toggle is off, no network request is sent to the phishing endpoint, and that a request is sent when the toggle is on.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2625

Manual testing steps

  1. Install and open the service worker (background) console, and open the network tab
  2. There should be no request to the phishing endpoint
  3. Go through onboarding and toggle off the basic functionality toggle in advanced settings, there should still be not network request to the phishing endpoint
  4. Uninstall and re-install and go through the above steps, but do not toggle off the basic functionality toggle. There should now be a network request to the phishing endpoint after the user completes onboarding.

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

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.

…nboarding if basic functionality toggle is off
@danjm danjm force-pushed the prevent-phishing-request-before-onboarding branch from 33d7499 to 05c04fd Compare June 14, 2024 12:49
@danjm danjm changed the title Move initial phishingController.maybeUpdateState call to the postOnboardingInitialization method Ensure phishing stale list network request is not sent before onboarding and if basic functionality is off Jun 14, 2024
@danjm danjm changed the title Ensure phishing stale list network request is not sent before onboarding and if basic functionality is off fix: Ensure phishing stale list network request is not sent before onboarding and if basic functionality is off Jun 14, 2024
@danjm danjm marked this pull request as ready for review June 14, 2024 12:59
@danjm danjm requested a review from a team as a code owner June 14, 2024 12:59
Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

Worked as expected!

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 65.68%. Comparing base (6eee01a) to head (7297b1b).

Files Patch % Lines
app/scripts/metamask-controller.js 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25306      +/-   ##
===========================================
- Coverage    65.69%   65.68%   -0.00%     
===========================================
  Files         1377     1377              
  Lines        54624    54623       -1     
  Branches     14317    14320       +3     
===========================================
- Hits         35882    35879       -3     
- Misses       18742    18744       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [7297b1b]
Page Load Metrics (132 ± 166 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6910984105
domContentLoaded9241242
load421642132347166
domInteractive9241242
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 71 Bytes (0.00%)
  • ui: 3 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@danjm danjm merged commit 743f5ec into develop Jun 14, 2024
74 checks passed
@danjm danjm deleted the prevent-phishing-request-before-onboarding branch June 14, 2024 16:49
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-wallet-ux
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants