Skip to content

Commit

Permalink
fix: Ensure phishing stale list network request is not sent before on…
Browse files Browse the repository at this point in the history
…boarding and if basic functionality is off (#25306)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **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](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25306?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
  • Loading branch information
danjm authored Jun 14, 2024
1 parent b0a05f0 commit 743f5ec
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 7 deletions.
8 changes: 6 additions & 2 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -865,8 +865,6 @@ export default class MetamaskController extends EventEmitter {
stalelistRefreshInterval: process.env.IN_TEST ? 30 * SECOND : undefined,
});

this.phishingController.maybeUpdateState();

///: BEGIN:ONLY_INCLUDE_IF(blockaid)
this.ppomController = new PPOMController({
messenger: this.controllerMessenger.getRestricted({
Expand Down Expand Up @@ -2339,7 +2337,13 @@ export default class MetamaskController extends EventEmitter {
}

postOnboardingInitialization() {
const { usePhishDetect } = this.preferencesController.store.getState();

this.networkController.lookupNetwork();

if (usePhishDetect) {
this.phishingController.maybeUpdateState();
}
}

triggerNetworkrequests() {
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/tests/privacy/basic-functionality.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,17 @@ const {
tinyDelayMs,
defaultGanacheOptions,
} = require('../../helpers');
const { METAMASK_STALELIST_URL } = require('../phishing-controller/helpers');
const FixtureBuilder = require('../../fixture-builder');

async function mockApis(mockServer) {
return [
await mockServer.forGet(METAMASK_STALELIST_URL).thenCallback(() => {
return {
statusCode: 200,
body: [{ fakedata: true }],
};
}),
await mockServer
.forGet('https://token.api.cx.metamask.io/tokens/1')
.thenCallback(() => {
Expand Down
13 changes: 8 additions & 5 deletions ui/pages/onboarding-flow/privacy-settings/privacy-settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ export default function PrivacySettings() {
const defaultState = useSelector((state) => state.metamask);
const {
incomingTransactionsPreferences,
usePhishDetect,
use4ByteResolution,
useTokenDetection,
useCurrencyRateCheck,
Expand All @@ -116,8 +115,7 @@ export default function PrivacySettings() {
const petnamesEnabled = useSelector(getPetnamesEnabled);
const participateInMetaMetrics = useSelector(selectParticipateInMetaMetrics);

const [usePhishingDetection, setUsePhishingDetection] =
useState(usePhishDetect);
const [usePhishingDetection, setUsePhishingDetection] = useState(null);
const [turnOn4ByteResolution, setTurnOn4ByteResolution] =
useState(use4ByteResolution);
const [turnOnTokenDetection, setTurnOnTokenDetection] =
Expand Down Expand Up @@ -146,13 +144,18 @@ export default function PrivacySettings() {
getExternalServicesOnboardingToggleState,
);

const phishingToggleState =
usePhishingDetection === null
? externalServicesOnboardingToggleState
: usePhishingDetection;

const profileSyncingProps = useProfileSyncingProps(
externalServicesOnboardingToggleState,
);

const handleSubmit = () => {
dispatch(toggleExternalServices(externalServicesOnboardingToggleState));
dispatch(setUsePhishDetect(usePhishingDetection));
dispatch(setUsePhishDetect(phishingToggleState));
dispatch(setUse4ByteResolution(turnOn4ByteResolution));
dispatch(setUseTokenDetection(turnOnTokenDetection));
dispatch(
Expand Down Expand Up @@ -320,7 +323,7 @@ export default function PrivacySettings() {
)}

<Setting
value={usePhishingDetection}
value={phishingToggleState}
setValue={setUsePhishingDetection}
title={t('usePhishingDetection')}
description={t('onboardingUsePhishingDetectionDescription', [
Expand Down

0 comments on commit 743f5ec

Please sign in to comment.