[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
2024-11-22 09:57:02 +01:00 · 2023-03-29 15:17:57 -04:00 · 2023-03-29 15:17:57 -04:00 · c2b2f2685e
commit c2b2f2685e
parent 27f3af80bd
10 changed files with 197 additions and 116 deletions
--- 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": {
--- 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',
--- 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',
--- 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',
--- 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,
+      }),
    ),
  );

--- 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;

--- 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 (
      <div className="snap-install-warning__footer">
-        <Button
-          className="snap-install-warning__footer-button"
-          type="default"
-          onClick={onCancel}
-        >
-          {t('cancel')}
-        </Button>
        <Button
          className="snap-install-warning__footer-button"
          type="primary"
@ -67,8 +65,6 @@ export default function SnapInstallWarning({ onCancel, onSubmit, warnings }) {
  return (
    <Popover
      className="snap-install-warning"
-      title={t('snapInstallWarningHeading')}
-      centerTitle
      footer={<SnapInstallWarningFooter />}
      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}
    >
+      <Box justifyContent={JustifyContent.center} marginBottom={6}>
+        <AvatarIcon
+          iconName={ICON_NAMES.DANGER}
+          backgroundColor={BackgroundColor.warningMuted}
+          color={IconColor.warningDefault}
+          size={Size.LG}
+        />
+      </Box>
+      <Text
+        paddingBottom={6}
+        textAlign={TEXT_ALIGN.CENTER}
+        variant={TextVariant.headingSm}
+        as="h2"
+      >
+        {t('snapInstallWarningHeading')}
+      </Text>
      <Text paddingBottom={6} textAlign={TEXT_ALIGN.CENTER}>
        {warnings.length > 1
          ? t('snapInstallWarningCheckPlural')
--- 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', [
+          <Text
+            key="1"
+            color={Color.primaryDefault}
+            fontWeight={FONT_WEIGHT.BOLD}
+            variant={TextVariant.bodySm}
+            as="span"
+          >
+            {getSnapName(targetSubjectMetadata?.origin)}
+          </Text>,
+          <b key="2">
+            {getSnapDerivationPathName(path, curve) ??
+              `${path.join('/')} (${curve})`}
+          </b>,
+        ]),
      };

      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', [
+          <Text
+            key="1"
+            color={Color.primaryDefault}
+            fontWeight={FONT_WEIGHT.BOLD}
+            variant={TextVariant.bodySm}
+            as="span"
+          >
+            {getSnapName(targetSubjectMetadata?.origin)}
+          </Text>,
+          <b key="2">
+            {getSnapDerivationPathName(path, curve) ??
+              `${path.join('/')} (${curve})`}
+          </b>,
+        ]),
      };

      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', [
        <span className="permission-label-item" key={`coin-type-${coinType}`}>
          {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', [
+        <Text
+          key="1"
+          color={Color.primaryDefault}
+          fontWeight={FONT_WEIGHT.BOLD}
+          variant={TextVariant.bodySm}
+          as="span"
+        >
+          {getSnapName(targetSubjectMetadata?.origin)}
+        </Text>,
+        <b key="2">
+          {coinTypeToProtocolName(coinType) ||
+            t('unrecognizedProtocol', [coinType])}
+        </b>,
+      ]),
    })),
-  [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 }),
        ),
      [],
    )
--- 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) {
--- 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', [
-          <Text
-            key="1"
-            color={Color.primaryDefault}
-            fontWeight={FONT_WEIGHT.BOLD}
-            variant={TextVariant.bodySm}
-            as="span"
-          >
-            {getSnapName(targetSubjectMetadata.origin)}
-          </Text>,
-          <b key="2">
-            {getSnapDerivationPathName(path, curve) ??
-              `${path.join('/')} (${curve})`}
-          </b>,
-        ]),
-      })),
-    ),
-    ...bip44EntropyPermissions.flatMap((permission, i) =>
-      permission.caveats[0].value.map(({ coinType }) => ({
-        id: `key-access-bip44-${coinType}-${i}`,
-        message: t('snapInstallWarningKeyAccess', [
-          <Text
-            key="1"
-            color={Color.primaryDefault}
-            fontWeight={FONT_WEIGHT.BOLD}
-            variant={TextVariant.bodySm}
-            as="span"
-          >
-            {getSnapName(targetSubjectMetadata.origin)}
-          </Text>,
-          <b key="2">
-            {coinTypeToProtocolName(coinType) ||
-              t('unrecognizedProtocol', [coinType])}
-          </b>,
-        ]),
-      })),
-    ),
-  ];
+  return weightOneWarnings;
 }