From c2b2f2685e71a4b1289216324c39535532caa436 Mon Sep 17 00:00:00 2001
From: Hassan Malik <41640681+hmalik88@users.noreply.github.com>
Date: Wed, 29 Mar 2023 15:17:57 -0400
Subject: [PATCH] [FLASK] Redesign key management modal (#18263)
* added the rest of the sev1 warnings to getSnapInstallWarnings
* added and updated translations
* Updated getSnapInstallWarnings to include warnings for all weight-1 permissions
* Updated snap install warning and css according to designs
* fix snaps tests
* fixed alignment/spacing
* updated e2e tests to click through the newly displayed public key access warning
* lint fix
* fixed update snap test
* refactored getSnapInstallWarnings, moved logic to PERMISSION_DESCRIPTIONS
* fix logic to account for objects & arrays
* update function description
* add missing properties to ethereum provider description
* moved id and message to baseDescription to fix error
* add optional chaining to fix undefined error
* more optional chaining
* more optional chaining
---
app/_locales/en/messages.json | 10 +-
test/e2e/snaps/test-snap-bip-32.spec.js | 5 +-
test/e2e/snaps/test-snap-rpc.spec.js | 5 +-
test/e2e/snaps/test-snap-update.spec.js | 5 +-
.../connected-accounts-permissions.js | 6 +-
.../app/flask/snap-install-warning/index.scss | 4 +
.../snap-install-warning.js | 33 ++--
ui/helpers/utils/permission.js | 147 ++++++++++++++----
ui/helpers/utils/util.js | 2 +-
ui/pages/permissions-connect/flask/util.js | 96 ++++--------
10 files changed, 197 insertions(+), 116 deletions(-)
diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json
index b194112a5..17a25f726 100644
--- a/app/_locales/en/messages.json
+++ b/app/_locales/en/messages.json
@@ -1365,6 +1365,10 @@
"ethGasPriceFetchWarning": {
"message": "Backup gas price is provided as the main gas estimation service is unavailable right now."
},
+ "ethereumProviderAccess": {
+ "message": "Grant Ethereum provider access to $1",
+ "description": "The parameter is the name of the requesting origin"
+ },
"ethereumPublicAddress": {
"message": "Ethereum public address"
},
@@ -3572,7 +3576,11 @@
"message": "Proceed with caution"
},
"snapInstallWarningKeyAccess": {
- "message": "Grant $2 key access to $1",
+ "message": "Grant $2 account control to $1",
+ "description": "The first parameter is the name of the snap and the second one is the protocol"
+ },
+ "snapInstallWarningPublicKeyAccess": {
+ "message": "Grant $2 public key access to $1",
"description": "The first parameter is the name of the snap and the second one is the protocol"
},
"snapResultError": {
diff --git a/test/e2e/snaps/test-snap-bip-32.spec.js b/test/e2e/snaps/test-snap-bip-32.spec.js
index 2a5174498..2babba866 100644
--- a/test/e2e/snaps/test-snap-bip-32.spec.js
+++ b/test/e2e/snaps/test-snap-bip-32.spec.js
@@ -63,7 +63,10 @@ describe('Test Snap bip-32', function () {
// wait for permissions popover, click checkboxes and confirm
await driver.delay(1000);
await driver.clickElement('#key-access-bip32-m-44h-0h-secp256k1-0');
- await driver.clickElement('#key-access-bip32-m-44h-0h-ed25519-0');
+ await driver.clickElement('#key-access-bip32-m-44h-0h-ed25519-1');
+ await driver.clickElement(
+ '#public-key-access-bip32-m-44h-0h-secp256k1-0',
+ );
await driver.clickElement({
text: 'Confirm',
tag: 'button',
diff --git a/test/e2e/snaps/test-snap-rpc.spec.js b/test/e2e/snaps/test-snap-rpc.spec.js
index 0f512c6fe..3c596ba08 100644
--- a/test/e2e/snaps/test-snap-rpc.spec.js
+++ b/test/e2e/snaps/test-snap-rpc.spec.js
@@ -64,7 +64,10 @@ describe('Test Snap RPC', function () {
// wait for permissions popover, click checkboxes and confirm
await driver.delay(1000);
await driver.clickElement('#key-access-bip32-m-44h-0h-secp256k1-0');
- await driver.clickElement('#key-access-bip32-m-44h-0h-ed25519-0');
+ await driver.clickElement('#key-access-bip32-m-44h-0h-ed25519-1');
+ await driver.clickElement(
+ '#public-key-access-bip32-m-44h-0h-secp256k1-0',
+ );
await driver.clickElement({
text: 'Confirm',
tag: 'button',
diff --git a/test/e2e/snaps/test-snap-update.spec.js b/test/e2e/snaps/test-snap-update.spec.js
index 523356bac..eaf0b847c 100644
--- a/test/e2e/snaps/test-snap-update.spec.js
+++ b/test/e2e/snaps/test-snap-update.spec.js
@@ -64,7 +64,10 @@ describe('Test Snap update', function () {
// wait for permissions popover, click checkboxes and confirm
await driver.delay(1000);
await driver.clickElement('#key-access-bip32-m-44h-0h-secp256k1-0');
- await driver.clickElement('#key-access-bip32-m-44h-0h-ed25519-0');
+ await driver.clickElement('#key-access-bip32-m-44h-0h-ed25519-1');
+ await driver.clickElement(
+ '#public-key-access-bip32-m-44h-0h-secp256k1-0',
+ );
await driver.clickElement({
text: 'Confirm',
tag: 'button',
diff --git a/ui/components/app/connected-accounts-permissions/connected-accounts-permissions.js b/ui/components/app/connected-accounts-permissions/connected-accounts-permissions.js
index 641dc6e70..10f032424 100644
--- a/ui/components/app/connected-accounts-permissions/connected-accounts-permissions.js
+++ b/ui/components/app/connected-accounts-permissions/connected-accounts-permissions.js
@@ -20,7 +20,11 @@ const ConnectedAccountsPermissions = ({ permissions }) => {
const permissionLabels = flatten(
permissions.map(({ key, value }) =>
- getPermissionDescription(t, key, value),
+ getPermissionDescription({
+ t,
+ permissionName: key,
+ permissionValue: value,
+ }),
),
);
diff --git a/ui/components/app/flask/snap-install-warning/index.scss b/ui/components/app/flask/snap-install-warning/index.scss
index bc7d078bd..ddbf9c8d4 100644
--- a/ui/components/app/flask/snap-install-warning/index.scss
+++ b/ui/components/app/flask/snap-install-warning/index.scss
@@ -1,4 +1,8 @@
.snap-install-warning {
+ .popover-header {
+ padding-bottom: 0;
+ }
+
.checkbox-label {
@include H7;
diff --git a/ui/components/app/flask/snap-install-warning/snap-install-warning.js b/ui/components/app/flask/snap-install-warning/snap-install-warning.js
index 70cedf0d2..15c593c74 100644
--- a/ui/components/app/flask/snap-install-warning/snap-install-warning.js
+++ b/ui/components/app/flask/snap-install-warning/snap-install-warning.js
@@ -6,12 +6,17 @@ import { useI18nContext } from '../../../../hooks/useI18nContext';
import CheckBox from '../../../ui/check-box/check-box.component';
import {
+ BackgroundColor,
+ IconColor,
TextVariant,
TEXT_ALIGN,
+ Size,
+ JustifyContent,
} from '../../../../helpers/constants/design-system';
import Popover from '../../../ui/popover';
import Button from '../../../ui/button';
-import { Text } from '../../../component-library';
+import { AvatarIcon, ICON_NAMES, Text } from '../../../component-library';
+import Box from '../../../ui/box/box';
/**
* a very simple reducer using produce from Immer to keep checkboxes state manipulation
@@ -45,13 +50,6 @@ export default function SnapInstallWarning({ onCancel, onSubmit, warnings }) {
const SnapInstallWarningFooter = () => {
return (
-
}
headerProps={{ padding: [6, 6, 0] }}
contentProps={{
@@ -78,7 +74,24 @@ export default function SnapInstallWarning({ onCancel, onSubmit, warnings }) {
paddingBottom: [6, 4],
}}
footerProps={{ padding: [4, 6] }}
+ onClose={onCancel}
>
+
+
+
+
+ {t('snapInstallWarningHeading')}
+
{warnings.length > 1
? t('snapInstallWarningCheckPlural')
diff --git a/ui/helpers/utils/permission.js b/ui/helpers/utils/permission.js
index 41d8df434..6987b1d7f 100644
--- a/ui/helpers/utils/permission.js
+++ b/ui/helpers/utils/permission.js
@@ -18,12 +18,18 @@ import {
AvatarIcon,
///: BEGIN:ONLY_INCLUDE_IN(flask)
Icon,
+ Text,
///: END:ONLY_INCLUDE_IN
ICON_NAMES,
ICON_SIZES,
} from '../../components/component-library';
///: BEGIN:ONLY_INCLUDE_IN(flask)
-import { IconColor } from '../constants/design-system';
+import {
+ IconColor,
+ Color,
+ FONT_WEIGHT,
+ TextVariant,
+} from '../constants/design-system';
import {
coinTypeToProtocolName,
getSnapDerivationPathName,
@@ -63,41 +69,63 @@ function getLeftIcon(iconName) {
);
}
-const PERMISSION_DESCRIPTIONS = deepFreeze({
- [RestrictedMethods.eth_accounts]: (t) => ({
+export const PERMISSION_DESCRIPTIONS = deepFreeze({
+ [RestrictedMethods.eth_accounts]: ({ t }) => ({
label: t('permission_ethereumAccounts'),
leftIcon: getLeftIcon(ICON_NAMES.EYE),
rightIcon: null,
weight: 2,
}),
///: BEGIN:ONLY_INCLUDE_IN(flask)
- [RestrictedMethods.snap_confirm]: (t) => ({
+ [RestrictedMethods.snap_confirm]: ({ t }) => ({
label: t('permission_customConfirmation'),
description: t('permission_customConfirmationDescription'),
leftIcon: getLeftIcon(ICON_NAMES.SECURITY_TICK),
rightIcon: RIGHT_INFO_ICON,
weight: 3,
}),
- [RestrictedMethods.snap_dialog]: (t) => ({
+ [RestrictedMethods.snap_dialog]: ({ t }) => ({
label: t('permission_dialog'),
description: t('permission_dialogDescription'),
leftIcon: getLeftIcon(ICON_NAMES.MESSAGES),
rightIcon: RIGHT_INFO_ICON,
weight: 3,
}),
- [RestrictedMethods.snap_notify]: (t) => ({
+ [RestrictedMethods.snap_notify]: ({ t }) => ({
label: t('permission_notifications'),
description: t('permission_notificationsDescription'),
leftIcon: getLeftIcon(ICON_NAMES.NOTIFICATION),
rightIcon: RIGHT_INFO_ICON,
weight: 3,
}),
- [RestrictedMethods.snap_getBip32PublicKey]: (t, _, permissionValue) =>
- permissionValue.caveats[0].value.map(({ path, curve }) => {
+ [RestrictedMethods.snap_getBip32PublicKey]: ({
+ t,
+ permissionValue,
+ targetSubjectMetadata,
+ }) =>
+ permissionValue.caveats[0].value.map(({ path, curve }, i) => {
const baseDescription = {
leftIcon: getLeftIcon(ICON_NAMES.SECURITY_SEARCH),
rightIcon: RIGHT_WARNING_ICON,
weight: 1,
+ id: `public-key-access-bip32-${path
+ .join('-')
+ ?.replace(/'/gu, 'h')}-${curve}-${i}`,
+ message: t('snapInstallWarningPublicKeyAccess', [
+
+ {getSnapName(targetSubjectMetadata?.origin)}
+ ,
+
+ {getSnapDerivationPathName(path, curve) ??
+ `${path.join('/')} (${curve})`}
+ ,
+ ]),
};
const friendlyName = getSnapDerivationPathName(path, curve);
@@ -141,12 +169,34 @@ const PERMISSION_DESCRIPTIONS = deepFreeze({
]),
};
}),
- [RestrictedMethods.snap_getBip32Entropy]: (t, _, permissionValue) =>
- permissionValue.caveats[0].value.map(({ path, curve }) => {
+ [RestrictedMethods.snap_getBip32Entropy]: ({
+ t,
+ permissionValue,
+ targetSubjectMetadata,
+ }) =>
+ permissionValue.caveats[0].value.map(({ path, curve }, i) => {
const baseDescription = {
leftIcon: getLeftIcon(ICON_NAMES.KEY),
rightIcon: RIGHT_WARNING_ICON,
weight: 1,
+ id: `key-access-bip32-${path
+ .join('-')
+ ?.replace(/'/gu, 'h')}-${curve}-${i}`,
+ message: t('snapInstallWarningKeyAccess', [
+
+ {getSnapName(targetSubjectMetadata?.origin)}
+ ,
+
+ {getSnapDerivationPathName(path, curve) ??
+ `${path.join('/')} (${curve})`}
+ ,
+ ]),
};
const friendlyName = getSnapDerivationPathName(path, curve);
@@ -190,8 +240,12 @@ const PERMISSION_DESCRIPTIONS = deepFreeze({
]),
};
}),
- [RestrictedMethods.snap_getBip44Entropy]: (t, _, permissionValue) =>
- permissionValue.caveats[0].value.map(({ coinType }) => ({
+ [RestrictedMethods.snap_getBip44Entropy]: ({
+ t,
+ permissionValue,
+ targetSubjectMetadata,
+ }) =>
+ permissionValue.caveats[0].value.map(({ coinType }, i) => ({
label: t('permission_manageBip44Keys', [
{coinTypeToProtocolName(coinType) ||
@@ -210,22 +264,38 @@ const PERMISSION_DESCRIPTIONS = deepFreeze({
leftIcon: getLeftIcon(ICON_NAMES.KEY),
rightIcon: RIGHT_WARNING_ICON,
weight: 1,
+ id: `key-access-bip44-${coinType}-${i}`,
+ message: t('snapInstallWarningKeyAccess', [
+
+ {getSnapName(targetSubjectMetadata?.origin)}
+ ,
+
+ {coinTypeToProtocolName(coinType) ||
+ t('unrecognizedProtocol', [coinType])}
+ ,
+ ]),
})),
- [RestrictedMethods.snap_getEntropy]: (t) => ({
+ [RestrictedMethods.snap_getEntropy]: ({ t }) => ({
label: t('permission_getEntropy'),
description: t('permission_getEntropyDescription'),
leftIcon: getLeftIcon(ICON_NAMES.SECURITY_KEY),
rightIcon: RIGHT_INFO_ICON,
weight: 3,
}),
- [RestrictedMethods.snap_manageState]: (t) => ({
+ [RestrictedMethods.snap_manageState]: ({ t }) => ({
label: t('permission_manageState'),
description: t('permission_manageStateDescription'),
leftIcon: getLeftIcon(ICON_NAMES.ADD_SQUARE),
rightIcon: RIGHT_INFO_ICON,
weight: 3,
}),
- [RestrictedMethods.wallet_snap]: (t, _, permissionValue) => {
+ [RestrictedMethods.wallet_snap]: ({ t, permissionValue }) => {
const snaps = permissionValue.caveats[0].value;
const baseDescription = {
leftIcon: getLeftIcon(ICON_NAMES.FLASH),
@@ -253,32 +323,31 @@ const PERMISSION_DESCRIPTIONS = deepFreeze({
};
});
},
- [EndowmentPermissions['endowment:network-access']]: (t) => ({
+ [EndowmentPermissions['endowment:network-access']]: ({ t }) => ({
label: t('permission_accessNetwork'),
description: t('permission_accessNetworkDescription'),
leftIcon: getLeftIcon(ICON_NAMES.GLOBAL),
rightIcon: RIGHT_INFO_ICON,
weight: 2,
}),
- [EndowmentPermissions['endowment:webassembly']]: (t) => ({
+ [EndowmentPermissions['endowment:webassembly']]: ({ t }) => ({
label: t('permission_webAssembly'),
description: t('permission_webAssemblyDescription'),
leftIcon: 'fas fa-microchip',
rightIcon: null,
weight: 2,
}),
- [EndowmentPermissions['endowment:long-running']]: (t) => ({
+ [EndowmentPermissions['endowment:long-running']]: ({ t }) => ({
label: t('permission_longRunning'),
description: t('permission_longRunningDescription'),
leftIcon: getLeftIcon(ICON_NAMES.LINK),
rightIcon: RIGHT_INFO_ICON,
weight: 3,
}),
- [EndowmentPermissions['endowment:transaction-insight']]: (
+ [EndowmentPermissions['endowment:transaction-insight']]: ({
t,
- _,
permissionValue,
- ) => {
+ }) => {
const baseDescription = {
leftIcon: getLeftIcon(ICON_NAMES.SPEEDOMETER),
rightIcon: RIGHT_INFO_ICON,
@@ -308,21 +377,26 @@ const PERMISSION_DESCRIPTIONS = deepFreeze({
return result;
},
- [EndowmentPermissions['endowment:cronjob']]: (t) => ({
+ [EndowmentPermissions['endowment:cronjob']]: ({ t }) => ({
label: t('permission_cronjob'),
description: t('permission_cronjobDescription'),
leftIcon: getLeftIcon(ICON_NAMES.CLOCK),
rightIcon: RIGHT_INFO_ICON,
weight: 2,
}),
- [EndowmentPermissions['endowment:ethereum-provider']]: (t) => ({
+ [EndowmentPermissions['endowment:ethereum-provider']]: ({
+ t,
+ targetSubjectMetadata,
+ }) => ({
label: t('permission_ethereumProvider'),
description: t('permission_ethereumProviderDescription'),
leftIcon: getLeftIcon(ICON_NAMES.ETHEREUM),
rightIcon: RIGHT_INFO_ICON,
weight: 1,
+ id: 'ethereum-provider-access',
+ message: t('ethereumProviderAccess', [targetSubjectMetadata?.origin]),
}),
- [EndowmentPermissions['endowment:rpc']]: (t, _, permissionValue) => {
+ [EndowmentPermissions['endowment:rpc']]: ({ t, permissionValue }) => {
const baseDescription = {
leftIcon: getLeftIcon(ICON_NAMES.HIERARCHY),
rightIcon: RIGHT_INFO_ICON,
@@ -351,7 +425,7 @@ const PERMISSION_DESCRIPTIONS = deepFreeze({
return results;
},
///: END:ONLY_INCLUDE_IN
- [UNKNOWN_PERMISSION]: (t, permissionName) => ({
+ [UNKNOWN_PERMISSION]: ({ t, permissionName }) => ({
label: t('permission_unknown', [permissionName ?? 'undefined']),
leftIcon: getLeftIcon(ICON_NAMES.QUESTION),
rightIcon: null,
@@ -372,23 +446,32 @@ const PERMISSION_DESCRIPTIONS = deepFreeze({
*/
/**
- * @param {Function} t - The translation function
- * @param {string} permissionName - The name of the permission to request
- * @param {object} permissionValue - The value of the permission to request
+ * @typedef {object} PermissionDescriptionParamsObject
+ * @property {Function} t - The translation function.
+ * @property {string} permissionName - The name of the permission.
+ * @property {object} permissionValue - The permission object.
+ * @property {object} targetSubjectMetadata - Subject metadata.
+ */
+
+/**
+ * @param {PermissionDescriptionParamsObject} params - The permission description params object.
+ * @param {Function} params.t - The translation function.
+ * @param {string} params.permissionName - The name of the permission to request
+ * @param {object} params.permissionValue - The value of the permission to request
* @returns {PermissionLabelObject[]}
*/
-export const getPermissionDescription = (
+export const getPermissionDescription = ({
t,
permissionName,
permissionValue,
-) => {
+}) => {
let value = PERMISSION_DESCRIPTIONS[UNKNOWN_PERMISSION];
if (Object.hasOwnProperty.call(PERMISSION_DESCRIPTIONS, permissionName)) {
value = PERMISSION_DESCRIPTIONS[permissionName];
}
- const result = value(t, permissionName, permissionValue);
+ const result = value({ t, permissionName, permissionValue });
if (!Array.isArray(result)) {
return [{ ...result, permissionName, permissionValue }];
}
@@ -413,7 +496,7 @@ export function getWeightedPermissions(t, permissions) {
.reduce(
(target, [permissionName, permissionValue]) =>
target.concat(
- getPermissionDescription(t, permissionName, permissionValue),
+ getPermissionDescription({ t, permissionName, permissionValue }),
),
[],
)
diff --git a/ui/helpers/utils/util.js b/ui/helpers/utils/util.js
index c82988d73..868ab46fb 100644
--- a/ui/helpers/utils/util.js
+++ b/ui/helpers/utils/util.js
@@ -556,7 +556,7 @@ export function getSnapDerivationPathName(path, curve) {
}
export const removeSnapIdPrefix = (snapId) =>
- snapId.replace(getSnapPrefix(snapId), '');
+ snapId?.replace(getSnapPrefix(snapId), '');
export const getSnapName = (snapId, subjectMetadata) => {
if (SNAPS_METADATA[snapId]?.name) {
diff --git a/ui/pages/permissions-connect/flask/util.js b/ui/pages/permissions-connect/flask/util.js
index 8c3ad9364..6711b98a5 100644
--- a/ui/pages/permissions-connect/flask/util.js
+++ b/ui/pages/permissions-connect/flask/util.js
@@ -1,72 +1,32 @@
-import React from 'react';
-import { Text } from '../../../components/component-library';
-import {
- Color,
- FONT_WEIGHT,
- TextVariant,
-} from '../../../helpers/constants/design-system';
-
-import {
- coinTypeToProtocolName,
- getSnapName,
- getSnapDerivationPathName,
-} from '../../../helpers/utils/util';
+import { isObject } from '@metamask/utils';
+import { PERMISSION_DESCRIPTIONS } from '../../../helpers/utils/permission';
export function getSnapInstallWarnings(permissions, targetSubjectMetadata, t) {
- const bip32EntropyPermissions =
- permissions &&
- Object.entries(permissions)
- .filter(([key]) => key === 'snap_getBip32Entropy')
- .map(([, value]) => value);
+ const weightOneWarnings = Object.entries(permissions).reduce(
+ (filteredWarnings, [permissionName, permissionValue]) => {
+ const permissionDescription = PERMISSION_DESCRIPTIONS[permissionName]({
+ t,
+ permissionValue,
+ targetSubjectMetadata,
+ });
+ if (Array.isArray(permissionDescription)) {
+ permissionDescription.forEach((description) => {
+ if (description.weight === 1) {
+ const { id, message } = description;
+ filteredWarnings.push({ id, message });
+ }
+ });
+ } else if (
+ isObject(permissionDescription) &&
+ permissionDescription.weight === 1
+ ) {
+ const { id, message } = permissionDescription;
+ filteredWarnings.push({ id, message });
+ }
+ return filteredWarnings;
+ },
+ [],
+ );
- const bip44EntropyPermissions =
- permissions &&
- Object.entries(permissions)
- .filter(([key]) => key === 'snap_getBip44Entropy')
- .map(([, value]) => value);
-
- return [
- ...bip32EntropyPermissions.flatMap((permission, i) =>
- permission.caveats[0].value.map(({ path, curve }) => ({
- id: `key-access-bip32-${path
- .join('-')
- .replace(/'/gu, 'h')}-${curve}-${i}`,
- message: t('snapInstallWarningKeyAccess', [
-
- {getSnapName(targetSubjectMetadata.origin)}
- ,
-
- {getSnapDerivationPathName(path, curve) ??
- `${path.join('/')} (${curve})`}
- ,
- ]),
- })),
- ),
- ...bip44EntropyPermissions.flatMap((permission, i) =>
- permission.caveats[0].value.map(({ coinType }) => ({
- id: `key-access-bip44-${coinType}-${i}`,
- message: t('snapInstallWarningKeyAccess', [
-
- {getSnapName(targetSubjectMetadata.origin)}
- ,
-
- {coinTypeToProtocolName(coinType) ||
- t('unrecognizedProtocol', [coinType])}
- ,
- ]),
- })),
- ),
- ];
+ return weightOneWarnings;
}