Security Provider cleanup #19694

digiwand · 2023-06-20T20:58:46Z

Explanation

no logic updates. just refactoring the OpenSea security provider logic.

The OpenSea security provider is planned to be deprecated and replaced. Cleaning the code a little will help us implement the new security provider and potentially help us maintain both as we transition.

Created a util function to abstract conditional logic - isSuspiciousSecurityProviderResponse
Moved SECURITY_PROVIDER_MESSAGE_SEVERITIES constant to a new shared/constants file
- previously, the constant lived in a component folder. The problem was it was used outside of the component.
Renamed SECURITY_PROVIDER_MESSAGE_SEVERITIES -> SECURITY_PROVIDER_MESSAGE_SEVERITY
The conditional with a little difficult for me to understand. I updated the code to hopefully be a bit more understandable.
Add test for new util function

Affected Components:

Confirm Pages
Confirm Approve
Signature Requests
Signature Request Original
Signature Request SIWE
Token Allowance

--
Fixes: #19741
Relates to: /~https://github.com/MetaMask/MetaMask-planning/issues/558
Relates to: #17662

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

github-actions · 2023-06-20T20:58:59Z

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.

codecov · 2023-06-20T21:14:07Z

Codecov Report

Merging #19694 (511c077) into develop (4ae483c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #19694      +/-   ##
===========================================
- Coverage    70.79%   70.79%   -0.00%     
===========================================
  Files          988      989       +1     
  Lines        38366    38350      -16     
  Branches     10042    10022      -20     
===========================================
- Hits         27161    27149      -12     
+ Misses       11205    11201       -4

Impacted Files	Coverage Δ
...banner-message/security-provider-banner-message.js	`100.00% <ø> (ø)`
shared/constants/security-provider.ts	`100.00% <100.00%> (ø)`
shared/modules/security-provider.utils.ts	`100.00% <100.00%> (ø)`
...ontent/confirm-page-container-content.component.js	`91.84% <100.00%> (-0.62%)`	⬇️
...t-original/signature-request-original.component.js	`61.11% <100.00%> (-1.65%)`	⬇️
...p/signature-request-siwe/signature-request-siwe.js	`71.93% <100.00%> (+1.93%)`	⬆️
...p/signature-request/signature-request.component.js	`76.71% <100.00%> (-1.21%)`	⬇️
...prove-content/confirm-approve-content.component.js	`77.65% <100.00%> (+0.06%)`	⬆️
ui/pages/token-allowance/token-allowance.js	`65.00% <100.00%> (-0.28%)`	⬇️

metamaskbot · 2023-06-20T21:23:00Z

Builds ready [b5143b4]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1924 ± 102 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	119	233	157	25	12
		domContentLoaded	1580	2270	1892	189	91
		load	1656	2300	1924	212	102
		domInteractive	1579	2270	1892	189	91

Bundle size diffs

background: 0 bytes
ui: -1828 bytes
common: 0 bytes

- util fn returns true if response is not verified and flagged

and support undefined param

- no logic changes

metamaskbot · 2023-06-21T19:35:24Z

Builds ready [511c077]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1651 ± 57 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	105	212	136	28	13
		domContentLoaded	1441	1934	1624	121	58
		load	1508	1949	1651	118	57
		domInteractive	1441	1934	1624	121	58

Bundle size diffs

background: 0 bytes
ui: -1926 bytes
common: 0 bytes

blackdevelopa · 2023-06-22T16:25:24Z

...rm-page-container/confirm-page-container-content/confirm-page-container-content.component.js

-            SECURITY_PROVIDER_MESSAGE_SEVERITIES.NOT_MALICIOUS) ||
-        (txData?.securityProviderResponse &&
-          Object.keys(txData.securityProviderResponse).length === 0) ? (
+        {isSuspiciousResponse(txData?.securityProviderResponse) && (


blackdevelopa

Looks good to me

jpuri · 2023-06-23T17:14:05Z

...rm-page-container/confirm-page-container-content/confirm-page-container-content.component.js

          <SecurityProviderBannerMessage
            securityProviderResponse={txData.securityProviderResponse}
          />
-        ) : null}
+        )}


Another approach could be moving this condition isSuspiciousResponse(txData?.securityProviderResponse) inside SecurityProviderBannerMessage component.

I like this suggestion. We can apply it to the new security provider code

jpuri

Nice cleanup 👍

digiwand added 2 commits June 20, 2023 22:44

security-prov: isFlaggedSecurityProviderResponse

e9ced27

security-prov: create shared/modules/util

b5143b4

digiwand added 4 commits June 21, 2023 20:43

security prov: rn isFlagged -> isSuspcious

d46e31a

- util fn returns true if response is not verified and flagged

security prov: add util test

4bf4fa7

and support undefined param

security prov: reorg util fn

134cc5c

- no logic changes

Merge branch 'develop' into security-provider-cleanup

511c077

digiwand marked this pull request as ready for review June 21, 2023 19:08

digiwand requested a review from a team as a code owner June 21, 2023 19:08

digiwand requested a review from FrederikBolding June 21, 2023 19:08

digiwand added the team-confirmations-secure-ux-PR PRs from the confirmations team label Jun 21, 2023

blackdevelopa reviewed Jun 22, 2023

View reviewed changes

blackdevelopa approved these changes Jun 22, 2023

View reviewed changes

jpuri reviewed Jun 23, 2023

View reviewed changes

jpuri approved these changes Jun 23, 2023

View reviewed changes

digiwand merged commit 34375a5 into develop Jun 23, 2023

digiwand deleted the security-provider-cleanup branch June 23, 2023 18:08

github-actions bot locked and limited conversation to collaborators Jun 23, 2023

metamaskbot added the release-10.34.0 Issue or pull request that will be included in release 10.34.0 label Jun 23, 2023

digiwand linked an issue Jun 23, 2023 that may be closed by this pull request

Clean OpenSea Security Provider Logic #19741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security Provider cleanup #19694

Security Provider cleanup #19694

digiwand commented Jun 20, 2023 •

edited

Loading

github-actions bot commented Jun 20, 2023

codecov bot commented Jun 20, 2023 •

edited

Loading

metamaskbot commented Jun 20, 2023

metamaskbot commented Jun 21, 2023

blackdevelopa Jun 22, 2023

blackdevelopa left a comment

jpuri Jun 23, 2023

digiwand Jun 23, 2023

jpuri left a comment

Security Provider cleanup #19694

Security Provider cleanup #19694

Conversation

digiwand commented Jun 20, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jun 20, 2023

codecov bot commented Jun 20, 2023 • edited Loading

Codecov Report

metamaskbot commented Jun 20, 2023

metamaskbot commented Jun 21, 2023

blackdevelopa Jun 22, 2023

Choose a reason for hiding this comment

blackdevelopa left a comment

Choose a reason for hiding this comment

jpuri Jun 23, 2023

Choose a reason for hiding this comment

digiwand Jun 23, 2023

Choose a reason for hiding this comment

jpuri left a comment

Choose a reason for hiding this comment

digiwand commented Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading