From e3c6e608c6210634fb4331551cd2980975478aaa Mon Sep 17 00:00:00 2001 From: Mat <63294765+matstyler@users.noreply.github.com> Date: Sun, 25 Feb 2024 01:26:59 +0100 Subject: [PATCH] :sparkles: feat: Implemented sign_eth method with MetaMask settings page (#1106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Motivation and context Implemented MetaMask settings page instance to be able to change advanced settings. Then added `sign_eth` method. ## Quality checklist - [x] I have performed a self-review of my code. - [x] If it is a core feature, I have added thorough e2e tests. **⚠️👆 Delete any section you see irrelevant before submitting the pull request 👆⚠️** ---
Generated summary > ## TL;DR > This pull request adds new methods and properties to the MetaMask class, including enabling and disabling the eth_sign feature in advanced settings. It also includes changes to the NotificationPage and SettingsPage classes for interacting with confirmation modals and toggling Ethereum signing. > > ## What changed > - Added new import for SettingsPage and property settingsPage to MetaMask class > - Added confirmSignatureWithRisk method to MetaMask class > - Added enableEthSignUnsafe method to MetaMask class > - Added signMessageWithRisk function to signSimpleMessage.ts > - Added signWithRisk function to signSimpleMessage object > - Added signMessageWithRisk async function to NotificationPage class > - Added new selector for risk modal in NotificationPage > - Added action to disable Ethereum signing in SettingsPage > - Added enableEthSign and disableEthSign functions to SettingsPage > - Added selectors for elements on SettingsPage > - Added confirmation modal object with specific selectors > - Added tests for confirming eth_sign functionality in Metamask > > ## How to test > 1. Enable and disable eth_sign feature in MetaMask advanced settings > 2. Interact with confirmation modals for enabling and disabling Ethereum signing > 3. Confirm signature with risk using the confirmSignatureWithRisk method > 4. Test eth_sign functionality in Metamask with provided test cases > > ## Why make this change > - Enhances MetaMask functionality by adding methods to confirm signatures with risk > - Provides users with the ability to enable and disable eth_sign feature for security purposes > - Improves user experience by adding interaction with confirmation modals > - Ensures proper testing of eth_sign functionality in Metamask for reliability and security
--- wallets/metamask/src/metamask.ts | 31 +++++++++++++++++++ .../actions/signSimpleMessage.ts | 12 ++++--- .../src/pages/NotificationPage/page.ts | 6 ++++ .../selectors/signaturePage.ts | 7 ++++- .../SettingsPage/actions/disableEthSign.ts | 7 +++++ .../SettingsPage/actions/enableEthSign.ts | 18 +++++++++++ .../src/pages/SettingsPage/actions/index.ts | 2 ++ .../metamask/src/pages/SettingsPage/page.ts | 22 +++++++++++++ .../src/pages/SettingsPage/selectors/index.ts | 23 ++++++++++++++ .../e2e/metamask/confirmSignature.spec.ts | 23 ++++++++++++++ 10 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 wallets/metamask/src/pages/SettingsPage/actions/disableEthSign.ts create mode 100644 wallets/metamask/src/pages/SettingsPage/actions/enableEthSign.ts create mode 100644 wallets/metamask/src/pages/SettingsPage/actions/index.ts create mode 100644 wallets/metamask/src/pages/SettingsPage/page.ts create mode 100644 wallets/metamask/src/pages/SettingsPage/selectors/index.ts diff --git a/wallets/metamask/src/metamask.ts b/wallets/metamask/src/metamask.ts index 3f8c9d883..87f6dc19d 100644 --- a/wallets/metamask/src/metamask.ts +++ b/wallets/metamask/src/metamask.ts @@ -3,6 +3,7 @@ import { CrashPage, HomePage, LockPage, NotificationPage, OnboardingPage } from import type { Network } from './pages/HomePage/actions' import { SettingsSidebarMenus } from './pages/HomePage/selectors/settings' import type { GasSetting } from './pages/NotificationPage/actions' +import { SettingsPage } from './pages/SettingsPage/page' const NO_EXTENSION_ID_ERROR = new Error('MetaMask extensionId is not set') @@ -40,6 +41,7 @@ export class MetaMask { * @group Selectors */ readonly notificationPage: NotificationPage + readonly settingsPage: SettingsPage /** * Class constructor. @@ -75,6 +77,7 @@ export class MetaMask { this.lockPage = new LockPage(page) this.homePage = new HomePage(page) this.notificationPage = new NotificationPage(page) + this.settingsPage = new SettingsPage(page) } /** @@ -173,6 +176,17 @@ export class MetaMask { await this.notificationPage.signMessage(this.extensionId) } + /** + * Confirms a signature request with potential risk. + */ + async confirmSignatureWithRisk() { + if (!this.extensionId) { + throw NO_EXTENSION_ID_ERROR + } + + await this.notificationPage.signMessageWithRisk(this.extensionId) + } + /** * Rejects a signature request. This function supports all types of commonly used signatures. */ @@ -346,6 +360,23 @@ export class MetaMask { await this.homePage.resetAccount() } + /** + * Enables the eth_sign feature in MetaMask advanced settings. + * This method is marked as unsafe because enabling eth_sign can have security implications. + */ + async unsafe_enableEthSign() { + await this.homePage.openSettings() + await this.settingsPage.enableEthSign() + } + + /** + * Disables the eth_sign feature in MetaMask advanced settings. + */ + async disableEthSign() { + await this.homePage.openSettings() + await this.settingsPage.disableEthSign() + } + /// ------------------------------------------- /// ---------- EXPERIMENTAL FEATURES ---------- /// ------------------------------------------- diff --git a/wallets/metamask/src/pages/NotificationPage/actions/signSimpleMessage.ts b/wallets/metamask/src/pages/NotificationPage/actions/signSimpleMessage.ts index 8e973acdf..51e752314 100644 --- a/wallets/metamask/src/pages/NotificationPage/actions/signSimpleMessage.ts +++ b/wallets/metamask/src/pages/NotificationPage/actions/signSimpleMessage.ts @@ -9,10 +9,14 @@ const rejectMessage = async (notificationPage: Page) => { await notificationPage.locator(Selectors.ActionFooter.rejectActionButton).click() } -// Used for: -// - `personal_sign` -// - `eth_signTypedData` +const signMessageWithRisk = async (notificationPage: Page) => { + await signMessage(notificationPage) + + await notificationPage.locator(Selectors.SignaturePage.riskModal.signButton).click() +} + export const signSimpleMessage = { sign: signMessage, - reject: rejectMessage + reject: rejectMessage, + signWithRisk: signMessageWithRisk } diff --git a/wallets/metamask/src/pages/NotificationPage/page.ts b/wallets/metamask/src/pages/NotificationPage/page.ts index 83292af8a..59e3bc13f 100644 --- a/wallets/metamask/src/pages/NotificationPage/page.ts +++ b/wallets/metamask/src/pages/NotificationPage/page.ts @@ -57,6 +57,12 @@ export class NotificationPage { } } + async signMessageWithRisk(extensionId: string) { + const { notificationPage } = await this.beforeMessageSignature(extensionId) + + await signSimpleMessage.signWithRisk(notificationPage) + } + async rejectMessage(extensionId: string) { const { notificationPage, isScrollButtonVisible } = await this.beforeMessageSignature(extensionId) diff --git a/wallets/metamask/src/pages/NotificationPage/selectors/signaturePage.ts b/wallets/metamask/src/pages/NotificationPage/selectors/signaturePage.ts index c9f0c5a07..a7b9dee0e 100644 --- a/wallets/metamask/src/pages/NotificationPage/selectors/signaturePage.ts +++ b/wallets/metamask/src/pages/NotificationPage/selectors/signaturePage.ts @@ -11,7 +11,12 @@ const structuredMessage = { rejectButton: `.signature-request-footer ${createDataTestSelector('signature-cancel-button')}` } +const riskModal = { + signButton: createDataTestSelector('signature-warning-sign-button') +} + export default { simpleMessage, - structuredMessage + structuredMessage, + riskModal } diff --git a/wallets/metamask/src/pages/SettingsPage/actions/disableEthSign.ts b/wallets/metamask/src/pages/SettingsPage/actions/disableEthSign.ts new file mode 100644 index 000000000..d4cab2335 --- /dev/null +++ b/wallets/metamask/src/pages/SettingsPage/actions/disableEthSign.ts @@ -0,0 +1,7 @@ +import type { Page } from '@playwright/test' +import Selectors from '../selectors' + +export default async function disableEthSign(page: Page) { + await page.locator(Selectors.settings.advancedSettings).click() + await page.locator(Selectors.settings.ethSignToggle).click() +} diff --git a/wallets/metamask/src/pages/SettingsPage/actions/enableEthSign.ts b/wallets/metamask/src/pages/SettingsPage/actions/enableEthSign.ts new file mode 100644 index 000000000..937ccf1cb --- /dev/null +++ b/wallets/metamask/src/pages/SettingsPage/actions/enableEthSign.ts @@ -0,0 +1,18 @@ +import type { Page } from '@playwright/test' +import Selectors from '../selectors' + +export default async function enableEthSign(page: Page) { + // Settings + await page.locator(Selectors.settings.advancedSettings).click() + await page.locator(Selectors.settings.ethSignToggle).click() + + // Confirmation modal + await page.locator(Selectors.confirmationModal.confirmationCheckbox).click() + await page.locator(Selectors.confirmationModal.continueButton).click() + await page.locator(Selectors.confirmationModal.manualConfirmationInput).focus() + await page.locator(Selectors.confirmationModal.manualConfirmationInput).fill('I only sign what I understand') + await page.locator(Selectors.confirmationModal.enableButton).click() + + // Wait for warning + await page.locator(Selectors.settings.ethSignWarning).isVisible() +} diff --git a/wallets/metamask/src/pages/SettingsPage/actions/index.ts b/wallets/metamask/src/pages/SettingsPage/actions/index.ts new file mode 100644 index 000000000..9dc1575eb --- /dev/null +++ b/wallets/metamask/src/pages/SettingsPage/actions/index.ts @@ -0,0 +1,2 @@ +export { default as enableEthSign } from './enableEthSign' +export { default as disableEthSign } from './disableEthSign' diff --git a/wallets/metamask/src/pages/SettingsPage/page.ts b/wallets/metamask/src/pages/SettingsPage/page.ts new file mode 100644 index 000000000..a9889f5a7 --- /dev/null +++ b/wallets/metamask/src/pages/SettingsPage/page.ts @@ -0,0 +1,22 @@ +import type { Page } from '@playwright/test' +import { enableEthSign } from './actions' +import disableEthSign from './actions/disableEthSign' +import Selectors from './selectors' + +export class SettingsPage { + static readonly selectors = Selectors + + readonly page: Page + + constructor(page: Page) { + this.page = page + } + + async enableEthSign() { + await enableEthSign(this.page) + } + + async disableEthSign() { + await disableEthSign(this.page) + } +} diff --git a/wallets/metamask/src/pages/SettingsPage/selectors/index.ts b/wallets/metamask/src/pages/SettingsPage/selectors/index.ts new file mode 100644 index 000000000..e72f3dac5 --- /dev/null +++ b/wallets/metamask/src/pages/SettingsPage/selectors/index.ts @@ -0,0 +1,23 @@ +import { createDataTestSelector } from '../../../utils/selectors/createDataTestSelector' + +const menuOption = '.settings-page__content__tabs .tab-bar .tab-bar__tab' + +const settings = { + menuOption, + advancedSettings: `${menuOption}:nth-child(2)`, + ethSignToggle: `${createDataTestSelector('advanced-setting-toggle-ethsign')} .eth-sign-toggle`, + ethSignWarning: + '.settings-page__content-row .mm-banner-alert.mm-banner-alert--severity-danger.mm-box--background-color-error-muted' +} + +const confirmationModal = { + confirmationCheckbox: createDataTestSelector('eth-sign__checkbox'), + continueButton: '.modal__content button.mm-button-primary', + manualConfirmationInput: '#enter-eth-sign-text', + enableButton: '.modal__content button.mm-button-primary.mm-button-primary--type-danger' +} + +export default { + settings, + confirmationModal +} diff --git a/wallets/metamask/test/e2e/metamask/confirmSignature.spec.ts b/wallets/metamask/test/e2e/metamask/confirmSignature.spec.ts index 7cf9beccd..acb85a9be 100644 --- a/wallets/metamask/test/e2e/metamask/confirmSignature.spec.ts +++ b/wallets/metamask/test/e2e/metamask/confirmSignature.spec.ts @@ -64,3 +64,26 @@ test('should confirm `eth_signTypedData_v4`', async ({ page, metamask }) => { await expect(page.locator('#signTypedDataV4VerifyResult')).toHaveText('0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266') }) + +test('should confirm `eth_sign`', async ({ page, metamask }) => { + await metamask.unsafe_enableEthSign() + + await page.locator('#ethSign').click() + + await metamask.confirmSignatureWithRisk() + + await expect(page.locator('#ethSignResult')).toContainText( + '0xbfefd81020331aa2869403ba11711f082506b9c9313c29a212975067123ca222536ba40b17d8847356cc4ee448fb088231db98632e745e469f7e3d142e4256541b' + ) +}) + +test('should not be permitted to confirm `eth_sign`', async ({ page, metamask }) => { + await metamask.unsafe_enableEthSign() + await metamask.disableEthSign() + + await page.locator('#ethSign').click() + + await expect(page.locator('#ethSign')).toContainText( + 'Error: eth_sign has been disabled. You must enable it in the advanced settings' + ) +})