From f788121c3bc698ffd8fb0231a74e90d313ad7d71 Mon Sep 17 00:00:00 2001 From: David Drazic Date: Wed, 31 May 2023 14:43:39 +0200 Subject: [PATCH] [FLASK] Add Snaps privacy warning on snap install (#18835) * Add Snaps privacy warning on snap install Add snap install warning status to storage Add storybook Add test for snap-privacy-warning Resolve button type issue Fix popup display logic Update fixture Update popup information and read more handling Replace deprecated button Update unit test * Update buttons and add cancel flow * Refactoring (review 1) * Add more unit tests --- app/_locales/en/messages.json | 31 ++++ app/scripts/controllers/app-state.js | 14 ++ app/scripts/controllers/app-state.test.js | 19 +++ app/scripts/metamask-controller.js | 6 + test/e2e/fixture-builder.js | 1 + .../permission-page-container.component.js | 42 ++++++ .../app/snaps/snap-privacy-warning/index.js | 1 + .../snap-privacy-warning.js | 140 ++++++++++++++++++ .../snap-privacy-warning.stories.js | 22 +++ .../snap-privacy-warning.test.js | 77 ++++++++++ ui/ducks/app/app.ts | 6 + .../permissions-connect.component.js | 17 +++ .../permissions-connect.container.js | 6 + ui/selectors/selectors.js | 19 +++ ui/selectors/selectors.test.js | 14 ++ ui/store/actions.ts | 17 +++ 16 files changed, 432 insertions(+) create mode 100644 ui/components/app/snaps/snap-privacy-warning/index.js create mode 100644 ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.js create mode 100644 ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.stories.js create mode 100644 ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.test.js diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index 91bad4b6d..df2552bb0 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -654,6 +654,9 @@ "clearActivityDescription": { "message": "This resets the account's nonce and erases data from the activity tab in your wallet. Only the current account and network will be affected. Your balances and incoming transactions won't change." }, + "click": { + "message": "Click" + }, "clickToConnectLedgerViaWebHID": { "message": "Click here to connect your Ledger via WebHID", "description": "Text that can be clicked to open a browser popup for connecting the ledger device via webhid" @@ -1562,6 +1565,10 @@ "followUsOnTwitter": { "message": "Follow us on Twitter" }, + "forMoreDetails": { + "message": "for more details.", + "description": "Click for more details message in popup modal displayed when installing a snap for the first time." + }, "forbiddenIpfsGateway": { "message": "Forbidden IPFS Gateway: Please specify a CID gateway" }, @@ -3843,9 +3850,29 @@ "snapsNoInsight": { "message": "The snap didn't return any insight" }, + "snapsPrivacyWarningFirstMessage": { + "message": "Installing a snap retrieves data from third parties. They may collect your personal information.", + "description": "First part of a message in popup modal displayed when installing a snap for the first time." + }, + "snapsPrivacyWarningSecondMessage": { + "message": "MetaMask has no access to this information.", + "description": "Second part of a message in popup modal displayed when installing a snap for the first time." + }, "snapsSettingsDescription": { "message": "Manage your Snaps" }, + "snapsThirdPartyNoticeReadMorePartOne": { + "message": "Any information you share with third-party-developed snaps will be collected directly by those snaps in accordance with their privacy policies. ", + "description": "First part of a tooltip content in popup modal displayed when installing a snap for the first time." + }, + "snapsThirdPartyNoticeReadMorePartThree": { + "message": "MetaMask has no access to information you share with these third parties.", + "description": "Third part of a tooltip content in popup modal displayed when installing a snap for the first time." + }, + "snapsThirdPartyNoticeReadMorePartTwo": { + "message": "During the installation of a snap, npmjs (npmjs.com) and AWS (aws.amazon.com) may collect your IP address. Please refer to their privacy policies for more information.", + "description": "Second part of a tooltip content in popup modal displayed when installing a snap for the first time." + }, "snapsToggle": { "message": "A snap will only run if it is enabled" }, @@ -4466,6 +4493,10 @@ "thingsToKeep": { "message": "Things to keep in mind:" }, + "thirdPartySoftware": { + "message": "Third party software", + "description": "Title of a popup modal displayed when installing a snap for the first time." + }, "thisCollection": { "message": "this collection" }, diff --git a/app/scripts/controllers/app-state.js b/app/scripts/controllers/app-state.js index fd0102666..722f1f56a 100644 --- a/app/scripts/controllers/app-state.js +++ b/app/scripts/controllers/app-state.js @@ -183,6 +183,20 @@ export default class AppStateController extends EventEmitter { }); } + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + /** + * Record if popover for snaps privacy warning has been shown + * on the first install of a snap. + * + * @param {boolean} shown - shown status + */ + setSnapsInstallPrivacyWarningShownStatus(shown) { + this.store.updateState({ + snapsInstallPrivacyWarningShown: shown, + }); + } + ///: END:ONLY_INCLUDE_IN + /** * Record the timestamp of the last time the user has seen the outdated browser warning * diff --git a/app/scripts/controllers/app-state.test.js b/app/scripts/controllers/app-state.test.js index 02d3cda3c..b96d39a4c 100644 --- a/app/scripts/controllers/app-state.test.js +++ b/app/scripts/controllers/app-state.test.js @@ -346,4 +346,23 @@ describe('AppStateController', () => { ); }); }); + + describe('setSnapsInstallPrivacyWarningShownStatus', () => { + it('updates the status of snaps install privacy warning', () => { + appStateController = createAppStateController(); + const updateStateSpy = jest.spyOn( + appStateController.store, + 'updateState', + ); + + appStateController.setSnapsInstallPrivacyWarningShownStatus(true); + + expect(updateStateSpy).toHaveBeenCalledTimes(1); + expect(updateStateSpy).toHaveBeenCalledWith({ + snapsInstallPrivacyWarningShown: true, + }); + + updateStateSpy.mockRestore(); + }); + }); }); diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 09c7b8803..93cd1af2f 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -2207,6 +2207,12 @@ export default class MetamaskController extends EventEmitter { ), setTermsOfUseLastAgreed: appStateController.setTermsOfUseLastAgreed.bind(appStateController), + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + setSnapsInstallPrivacyWarningShownStatus: + appStateController.setSnapsInstallPrivacyWarningShownStatus.bind( + appStateController, + ), + ///: END:ONLY_INCLUDE_IN setOutdatedBrowserWarningLastShown: appStateController.setOutdatedBrowserWarningLastShown.bind( appStateController, diff --git a/test/e2e/fixture-builder.js b/test/e2e/fixture-builder.js index 051d3f02c..41c3b12e8 100644 --- a/test/e2e/fixture-builder.js +++ b/test/e2e/fixture-builder.js @@ -158,6 +158,7 @@ function defaultFixture() { [CHAIN_IDS.GOERLI]: true, [CHAIN_IDS.LOCALHOST]: true, }, + snapsInstallPrivacyWarningShown: true, }, CachedBalancesController: { cachedBalances: { diff --git a/ui/components/app/permission-page-container/permission-page-container.component.js b/ui/components/app/permission-page-container/permission-page-container.component.js index 188290d71..a30825c09 100644 --- a/ui/components/app/permission-page-container/permission-page-container.component.js +++ b/ui/components/app/permission-page-container/permission-page-container.component.js @@ -13,6 +13,7 @@ import { PageContainerFooter } from '../../ui/page-container'; import PermissionsConnectFooter from '../permissions-connect-footer'; ///: BEGIN:ONLY_INCLUDE_IN(snaps) import { RestrictedMethods } from '../../../../shared/constants/permissions'; +import SnapPrivacyWarning from '../snaps/snap-privacy-warning'; ///: END:ONLY_INCLUDE_IN import { PermissionPageContainerContent } from '.'; @@ -24,6 +25,8 @@ export default class PermissionPageContainer extends Component { allIdentitiesSelected: PropTypes.bool, ///: BEGIN:ONLY_INCLUDE_IN(snaps) currentPermissions: PropTypes.object, + snapsInstallPrivacyWarningShown: PropTypes.bool.isRequired, + setSnapsInstallPrivacyWarningShownStatus: PropTypes.func, ///: END:ONLY_INCLUDE_IN request: PropTypes.object, requestMetadata: PropTypes.object, @@ -108,6 +111,12 @@ export default class PermissionPageContainer extends Component { caveats: [{ type: SnapCaveatType.SnapIds, value: dedupedCaveats }], }; } + + showSnapsPrivacyWarning() { + this.setState({ + isShowingSnapsPrivacyWarning: true, + }); + } ///: END:ONLY_INCLUDE_IN getRequestedMethodNames(props) { @@ -123,6 +132,14 @@ export default class PermissionPageContainer extends Component { legacy_event: true, }, }); + + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + if (this.props.request.permissions[WALLET_SNAP_PERMISSION_KEY]) { + if (this.props.snapsInstallPrivacyWarningShown === false) { + this.showSnapsPrivacyWarning(); + } + } + ///: END:ONLY_INCLUDE_IN } onCancel = () => { @@ -167,8 +184,33 @@ export default class PermissionPageContainer extends Component { allIdentitiesSelected, } = this.props; + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + const setIsShowingSnapsPrivacyWarning = (value) => { + this.setState({ + isShowingSnapsPrivacyWarning: value, + }); + }; + + const confirmSnapsPrivacyWarning = () => { + setIsShowingSnapsPrivacyWarning(false); + this.props.setSnapsInstallPrivacyWarningShownStatus(true); + }; + ///: END:ONLY_INCLUDE_IN + return (
+ { + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + <> + {this.state.isShowingSnapsPrivacyWarning && ( + confirmSnapsPrivacyWarning()} + onCanceled={() => this.onCancel()} + /> + )} + + ///: END:ONLY_INCLUDE_IN + } { + setIsDescriptionOpen(true); + }; + + return ( + + + + + + + {t('thirdPartySoftware')} + + + + {t('snapsPrivacyWarningFirstMessage')} + + {!isDescriptionOpen && ( + <> + + {t('snapsPrivacyWarningSecondMessage')} + + + {t('click')} + +  {t('here')}  + + {t('forMoreDetails')} + + + )} + {isDescriptionOpen && ( + <> + + {t('snapsThirdPartyNoticeReadMorePartOne')} + + + {t('snapsThirdPartyNoticeReadMorePartTwo')} + + + {t('snapsThirdPartyNoticeReadMorePartThree')} + + + )} + + + + + + + + ); +} + +SnapPrivacyWarning.propTypes = { + /** + * onAccepted handler + */ + onAccepted: PropTypes.func.isRequired, + /** + * onCanceled handler + */ + onCanceled: PropTypes.func.isRequired, +}; diff --git a/ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.stories.js b/ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.stories.js new file mode 100644 index 000000000..4ade7418e --- /dev/null +++ b/ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.stories.js @@ -0,0 +1,22 @@ +import React from 'react'; + +import SnapPrivacyWarning from '.'; + +export default { + title: 'Components/App/snaps/SnapPrivacyWarning', + component: SnapPrivacyWarning, + argTypes: { + onAccepted: { + action: 'onAccepted', + }, + onCanceled: { + action: 'onCanceled', + }, + }, +}; + +export const DefaultStory = (args) => ; + +DefaultStory.storyName = 'Default'; + +DefaultStory.args = {}; diff --git a/ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.test.js b/ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.test.js new file mode 100644 index 000000000..66034eab8 --- /dev/null +++ b/ui/components/app/snaps/snap-privacy-warning/snap-privacy-warning.test.js @@ -0,0 +1,77 @@ +import React from 'react'; +import { screen } from '@testing-library/react'; +import { renderWithProvider } from '../../../../../test/jest'; +import SnapPrivacyWarning from './snap-privacy-warning'; + +describe('Snap Privacy Warning Popover', () => { + it('renders snaps privacy warning popover and works with accept flow', () => { + const mockOnAcceptCallback = jest.fn(); + const { getByTestId } = renderWithProvider( + , + ); + + expect(screen.getByText('Third party software')).toBeInTheDocument(); + expect( + screen.getByText( + 'Installing a snap retrieves data from third parties. They may collect your personal information.', + ), + ).toBeInTheDocument(); + expect( + screen.getByText('MetaMask has no access to this information.'), + ).toBeInTheDocument(); + const clickHereToReadMoreButton = getByTestId( + 'snapsPrivacyPopup_readMoreButton', + ); + expect(clickHereToReadMoreButton).toBeDefined(); + clickHereToReadMoreButton.click(); + expect( + screen.getByText( + 'Any information you share with third-party-developed snaps will be collected directly by those snaps in accordance with their privacy policies.', + ), + ).toBeInTheDocument(); + expect( + screen.getByText( + 'During the installation of a snap, npmjs (npmjs.com) and AWS (aws.amazon.com) may collect your IP address. Please refer to their privacy policies for more information.', + ), + ).toBeInTheDocument(); + expect( + screen.getByRole('button', { + name: /Accept/iu, + }), + ).toBeInTheDocument(); + screen + .getByRole('button', { + name: /Accept/iu, + }) + .click(); + expect(mockOnAcceptCallback).toHaveBeenCalled(); + }); + + it('renders snaps privacy warning popover and works with cancel flow', () => { + const mockOnAcceptCallback = jest.fn(); + const mockOnCanceledCallback = jest.fn(); + renderWithProvider( + , + ); + + expect(screen.getByText('Third party software')).toBeInTheDocument(); + expect( + screen.getByRole('button', { + name: /Cancel/iu, + }), + ).toBeInTheDocument(); + screen + .getByRole('button', { + name: /Cancel/iu, + }) + .click(); + expect(mockOnCanceledCallback).toHaveBeenCalled(); + expect(mockOnAcceptCallback).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/ducks/app/app.ts b/ui/ducks/app/app.ts index b019d69f7..e6ea054db 100644 --- a/ui/ducks/app/app.ts +++ b/ui/ducks/app/app.ts @@ -67,6 +67,9 @@ interface AppState { customTokenAmount: string; txId: number | null; accountDetailsAddress: string; + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + snapsInstallPrivacyWarningShown: boolean; + ///: END:ONLY_INCLUDE_IN } interface AppSliceState { @@ -132,6 +135,9 @@ const initialState: AppState = { scrollToBottom: true, txId: null, accountDetailsAddress: '', + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + snapsInstallPrivacyWarningShown: false, + ///: END:ONLY_INCLUDE_IN }; export default function reduceApp( diff --git a/ui/pages/permissions-connect/permissions-connect.component.js b/ui/pages/permissions-connect/permissions-connect.component.js index d1eb45699..af40fbdf7 100644 --- a/ui/pages/permissions-connect/permissions-connect.component.js +++ b/ui/pages/permissions-connect/permissions-connect.component.js @@ -46,6 +46,8 @@ export default class PermissionConnect extends Component { requestState: PropTypes.object.isRequired, approvePendingApproval: PropTypes.func.isRequired, rejectPendingApproval: PropTypes.func.isRequired, + setSnapsInstallPrivacyWarningShownStatus: PropTypes.func.isRequired, + snapsInstallPrivacyWarningShown: PropTypes.bool.isRequired, ///: END:ONLY_INCLUDE_IN totalPages: PropTypes.string.isRequired, page: PropTypes.string.isRequired, @@ -76,6 +78,9 @@ export default class PermissionConnect extends Component { permissionsApproved: null, origin: this.props.origin, targetSubjectMetadata: this.props.targetSubjectMetadata || {}, + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + snapsInstallPrivacyWarningShown: this.props.snapsInstallPrivacyWarningShown, + ///: END:ONLY_INCLUDE_IN }; beforeUnload = () => { @@ -298,6 +303,7 @@ export default class PermissionConnect extends Component { requestState, approvePendingApproval, rejectPendingApproval, + setSnapsInstallPrivacyWarningShownStatus, ///: END:ONLY_INCLUDE_IN } = this.props; const { @@ -305,6 +311,9 @@ export default class PermissionConnect extends Component { permissionsApproved, redirecting, targetSubjectMetadata, + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + snapsInstallPrivacyWarningShown, + ///: END:ONLY_INCLUDE_IN } = this.state; return ( @@ -356,6 +365,14 @@ export default class PermissionConnect extends Component { selectedAccountAddresses.has(account.address), )} targetSubjectMetadata={targetSubjectMetadata} + ///: BEGIN:ONLY_INCLUDE_IN(snaps) + snapsInstallPrivacyWarningShown={ + snapsInstallPrivacyWarningShown + } + setSnapsInstallPrivacyWarningShownStatus={ + setSnapsInstallPrivacyWarningShownStatus + } + ///: END:ONLY_INCLUDE_IN /> )} /> diff --git a/ui/pages/permissions-connect/permissions-connect.container.js b/ui/pages/permissions-connect/permissions-connect.container.js index 584de0258..0995e9574 100644 --- a/ui/pages/permissions-connect/permissions-connect.container.js +++ b/ui/pages/permissions-connect/permissions-connect.container.js @@ -9,6 +9,7 @@ import { ///: BEGIN:ONLY_INCLUDE_IN(snaps) getSnapInstallOrUpdateRequests, getRequestState, + getSnapsInstallPrivacyWarningShown, ///: END:ONLY_INCLUDE_IN getRequestType, getTargetSubjectMetadata, @@ -24,6 +25,7 @@ import { ///: BEGIN:ONLY_INCLUDE_IN(snaps) resolvePendingApproval, rejectPendingApproval, + setSnapsInstallPrivacyWarningShownStatus, ///: END:ONLY_INCLUDE_IN } from '../../store/actions'; import { @@ -133,6 +135,7 @@ const mapStateToProps = (state, ownProps) => { snapResultPath, requestState, isSnap, + snapsInstallPrivacyWarningShown: getSnapsInstallPrivacyWarningShown(state), ///: END:ONLY_INCLUDE_IN permissionsRequest, permissionsRequestId, @@ -162,6 +165,9 @@ const mapDispatchToProps = (dispatch) => { dispatch(resolvePendingApproval(id, value)), rejectPendingApproval: (id, error) => dispatch(rejectPendingApproval(id, error)), + setSnapsInstallPrivacyWarningShownStatus: (shown) => { + dispatch(setSnapsInstallPrivacyWarningShownStatus(shown)); + }, ///: END:ONLY_INCLUDE_IN showNewAccountModal: ({ onCreateNewAccount, newAccountNumber }) => { return dispatch( diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js index d1ff0ef1c..8843f9fb6 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -1492,4 +1492,23 @@ export function getSnapsList(state) { }; }); } + +/** + * To get the state of snaps privacy warning popover. + * + * @param state - Redux state object. + * @returns True if popover has been shown, false otherwise. + */ +export function getSnapsInstallPrivacyWarningShown(state) { + const { snapsInstallPrivacyWarningShown } = state.metamask; + + if ( + snapsInstallPrivacyWarningShown === undefined || + snapsInstallPrivacyWarningShown === null + ) { + return false; + } + + return snapsInstallPrivacyWarningShown; +} ///: END:ONLY_INCLUDE_IN diff --git a/ui/selectors/selectors.test.js b/ui/selectors/selectors.test.js index e5964a73c..f31e73f1e 100644 --- a/ui/selectors/selectors.test.js +++ b/ui/selectors/selectors.test.js @@ -469,4 +469,18 @@ describe('Selectors', () => { )(mockState); expect(isFantomTokenSupported).toBeFalsy(); }); + + it('returns proper values for snaps privacy warning shown status', () => { + mockState.metamask.snapsInstallPrivacyWarningShown = false; + expect(selectors.getSnapsInstallPrivacyWarningShown(mockState)).toBe(false); + + mockState.metamask.snapsInstallPrivacyWarningShown = true; + expect(selectors.getSnapsInstallPrivacyWarningShown(mockState)).toBe(true); + + mockState.metamask.snapsInstallPrivacyWarningShown = undefined; + expect(selectors.getSnapsInstallPrivacyWarningShown(mockState)).toBe(false); + + mockState.metamask.snapsInstallPrivacyWarningShown = null; + expect(selectors.getSnapsInstallPrivacyWarningShown(mockState)).toBe(false); + }); }); diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 845540969..8eb1b67c6 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -4678,3 +4678,20 @@ export async function getCurrentNetworkEIP1559Compatibility(): Promise< } return networkEIP1559Compatibility; } + +///: BEGIN:ONLY_INCLUDE_IN(snaps) +/** + * Set status of popover warning for the first snap installation. + * + * @param shown - True if popover has been shown. + * @returns Promise Resolved on successfully submitted background request. + */ +export function setSnapsInstallPrivacyWarningShownStatus(shown: boolean) { + return async () => { + await submitRequestToBackground( + 'setSnapsInstallPrivacyWarningShownStatus', + [shown], + ); + }; +} +///: END:ONLY_INCLUDE_IN