Prevent from typing a derivation with ///password since it is ignored (#622)

* implementation and test * Update packages/extension-ui/src/Popup/Derive/SelectParent.tsx * Update packages/extension-ui/src/Popup/Derive/SelectParent.tsx * add path check and tests
2021-01-14 13:54:07 +01:00
parent 791db0e84a
commit 56d9554e73
3 changed files with 170 additions and 66 deletions
@@ -18,9 +18,10 @@ interface Props extends ThemeProps{
  onChange: (suri: string) => void;
  parentAddress: string;
  parentPassword: string;
+  withSoftPath: boolean;
 }

-function DerivationPath ({ className, defaultPath, isError, onChange }: Props): React.ReactElement<Props> {
+function DerivationPath ({ className, defaultPath, isError, onChange, withSoftPath }: Props): React.ReactElement<Props> {
  const { t } = useTranslation();
  const [path, setPath] = useState<string>(defaultPath);
  const [isDisabled, setIsDisabled] = useState(true);
@@ -46,7 +47,10 @@ function DerivationPath ({ className, defaultPath, isError, onChange }: Props):
                : t('Derivation Path')
            }
            onChange={_onChange}
-            placeholder={t<string>('//hard/soft')}
+            placeholder={withSoftPath
+              ? t<string>('//hard/soft')
+              : t<string>('//hard')
+            }
            value={path}
          />
        </div>
@@ -33,19 +33,19 @@ const accounts = [
  { address: '5D2TPhGEy2FhznvzaNYW9AkuMBbg3cyRemnPsBvBY4ZhkZXA', name: 'BB', parentAddress: '5GYmFzQCuC5u3tQNiMZNbFGakrz3Jq31NmMg4D2QAkSoQ2g5', type: 'sr25519' },
  { address: '5GhGENSJBWQZ8d8mARKgqEkiAxiW3hHeznQDW2iG4XzNieb6', isExternal: true, name: 'C', type: 'sr25519' },
  { address: '0xd5D81CD4236a43F48A983fc5B895975c511f634D', name: 'Ethereum', type: 'ethereum' },
-  { address: '5EeaoDj4VDk8V6yQngKBaCD5MpJUCHrhYjVhBjgMHXoYon1s', isExternal: false, name: 'D', type: 'sr25519' },
-  { address: '5HRKYp5anSNGtqC7cq9ftiaq4y8Mk7uHk7keaXUrQwZqDWLJ', name: 'DD', parentAddress: '5EeaoDj4VDk8V6yQngKBaCD5MpJUCHrhYjVhBjgMHXoYon1s', type: 'sr25519' }
+  { address: '5EeaoDj4VDk8V6yQngKBaCD5MpJUCHrhYjVhBjgMHXoYon1s', isExternal: false, name: 'D', type: 'ed25519' },
+  { address: '5HRKYp5anSNGtqC7cq9ftiaq4y8Mk7uHk7keaXUrQwZqDWLJ', name: 'DD', parentAddress: '5EeaoDj4VDk8V6yQngKBaCD5MpJUCHrhYjVhBjgMHXoYon1s', type: 'ed25519' }
 ] as AccountJson[];

 describe('Derive', () => {
-  const mountComponent = async (locked = false): Promise<{
+  const mountComponent = async (locked = false, account = 1): Promise<{
    wrapper: ReactWrapper;
    onActionStub: jest.Mock;
  }> => {
    const onActionStub = jest.fn();

    const wrapper = mount(
-      <MemoryRouter initialEntries={ [`/account/derive/${accounts[1].address}`] }>
+      <MemoryRouter initialEntries={ [`/account/derive/${accounts[account].address}`] }>
        <ActionContext.Provider value={onActionStub}>
          <AccountContext.Provider value={{
            accounts: accounts,
@@ -98,11 +98,12 @@ describe('Derive', () => {
      return { address: derivedAddress, suri: defaultDerivation } as ResponseDeriveValidate;
    });

-    it('Button is disabled and password field visible', () => {
+    it('Button is disabled and password field visible, path field is hidden', () => {
      const button = wrapper.find('[data-button-action="create derived account"] button');

      expect(button.exists()).toBe(true);
      expect(button.prop('disabled')).toBe(true);
+      expect(wrapper.find('.pathInput').exists()).toBe(false);
    });

    it('Password field is visible and not in error state', () => {
@@ -154,6 +155,30 @@ describe('Derive', () => {
      expect(wrapper.find('.warning-message')).toHaveLength(0);
    });

+    it('Derivation path gets visible, is set and locked', async () => {
+      await type(wrapper.find('input[type="password"]'), 'wrong_pass');
+
+      expect(wrapper.find('.pathInput.locked input').prop('disabled')).toBe(true);
+      expect(wrapper.find('.pathInput.locked input').prop('value')).toBe('//1');
+    });
+
+    it('Derivation path can be unlocked', async () => {
+      await type(wrapper.find('input[type="password"]'), 'wrong_pass');
+      wrapper.find('FontAwesomeIcon.lockIcon').simulate('click');
+      await act(flushAllPromises);
+      wrapper.update();
+
+      expect(wrapper.find('.pathInput').exists()).toBe(true);
+      expect(wrapper.find('.pathInput input').prop('disabled')).toBe(false);
+    });
+
+    it('Derivation path placeholder contains //hard/soft', async () => {
+      await type(wrapper.find('input[type="password"]'), parentPassword);
+      const pathInput = wrapper.find('[data-input-suri] input');
+
+      expect(pathInput.first().prop('placeholder')).toEqual('//hard/soft');
+    });
+
    it('An error is visible and the button is disabled when suri is incorrect', async () => {
      await type(wrapper.find('input[type="password"]'), parentPassword);
      await type(wrapper.find('[data-input-suri] input'), '//');
@@ -165,7 +190,29 @@ describe('Derive', () => {

      expect(button.prop('disabled')).toBe(true);
      expect(wrapper.find('.warning-message')).toHaveLength(1);
-      expect(wrapper.find('.warning-message').first().text()).toEqual('Incorrect derivation path');
+      expect(wrapper.find('.warning-message').first().text()).toEqual('Invalid derivation path');
+    });
+
+    it('An error is visible and the button is disabled when suri contains `///`', async () => {
+      await type(wrapper.find('input[type="password"]'), parentPassword);
+      await type(wrapper.find('[data-input-suri] input'), '///');
+
+      const button = wrapper.find('[data-button-action="create derived account"] button');
+
+      expect(button.prop('disabled')).toBe(true);
+      expect(wrapper.find('.warning-message')).toHaveLength(1);
+      // eslint-disable-next-line quotes
+      expect(wrapper.find('.warning-message').first().text()).toEqual("`///password` not supported for derivation");
+    });
+
+    it('No error is shown when suri contains soft derivation `/` with sr25519', async () => {
+      await type(wrapper.find('input[type="password"]'), parentPassword);
+      await type(wrapper.find('[data-input-suri] input'), '//somehard/soft');
+
+      const button = wrapper.find('[data-button-action="create derived account"] button');
+
+      expect(button.prop('disabled')).toBe(false);
+      expect(wrapper.find('.warning-message')).toHaveLength(0);
    });

    it('The error disappears and "Create derived account" is enabled when typing a new suri', async () => {
@@ -228,8 +275,6 @@ describe('Derive', () => {
        await act(flushAllPromises);
        wrapper.update();
        await enterName(newAccount.name).then(password(newAccount.password)).then(repeat(newAccount.password));
-        await act(flushAllPromises);
-        wrapper.update();
        wrapper.find('[data-button-action="add new root"] button').simulate('click');
        await act(flushAllPromises);
        wrapper.update();
@@ -239,4 +284,33 @@ describe('Derive', () => {
      });
    });
  });
+
+  describe('Ed25519 Parent', () => {
+    beforeEach(async () => {
+      const mountedComponent = await mountComponent(false, 5);
+
+      wrapper = mountedComponent.wrapper;
+      onActionStub = mountedComponent.onActionStub;
+      await type(wrapper.find('input[type="password"]'), parentPassword);
+    });
+
+    it('Derivation path placeholder only contains //hard', () => {
+      const pathInput = wrapper.find('[data-input-suri] input');
+
+      expect(pathInput.first().prop('placeholder')).toEqual('//hard');
+    });
+
+    it('An error is shown when suri contains soft derivation `/` with ed25519', async () => {
+      const pathInput = wrapper.find('[data-input-suri] input');
+
+      await type(pathInput, '//somehard/soft');
+
+      const button = wrapper.find('[data-button-action="create derived account"] button');
+
+      expect(button.prop('disabled')).toBe(true);
+      expect(wrapper.find('[data-input-suri]').first().prop('isError')).toBe(true);
+      expect(wrapper.find('.warning-message')).toHaveLength(1);
+      expect(wrapper.find('.warning-message').first().text()).toEqual('Soft derivation is only allowed for sr25519 accounts');
+    });
+  });
 });
@@ -4,7 +4,6 @@
 import React, { useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react';

 import { canDerive } from '@polkadot/extension-base/utils';
-import { assert } from '@polkadot/util';

 import { AccountContext, ActionContext, Address, ButtonArea, InputWithLabel, Label, NextStepButton, VerticalSpace, Warning } from '../../components';
 import useTranslation from '../../hooks/useTranslation';
@@ -21,6 +20,9 @@ interface Props {
  onDerivationConfirmed: (derivation: { account: { address: string; suri: string }; parentPassword: string }) => void;
 }

+// match any single slash
+const singleSlashRegex = /([^/]|^)\/([^/]|$)/;
+
 export default function SelectParent ({ className, isLocked, onDerivationConfirmed, parentAddress, parentGenesis }: Props): React.ReactElement<Props> {
  const { t } = useTranslation();
  const onAction = useContext(ActionContext);
@@ -30,7 +32,24 @@ export default function SelectParent ({ className, isLocked, onDerivationConfirm
  const [suriPath, setSuriPath] = useState<null | string>(defaultPath);
  const [parentPassword, setParentPassword] = useState<string>('');
  const [isProperParentPassword, setIsProperParentPassword] = useState(false);
+  const [pathError, setPathError] = useState('');
  const passwordInputRef = useRef<HTMLDivElement>(null);
+  const allowSoftDerivation = useMemo(() => {
+    const parent = accounts.find(({ address }) => address === parentAddress);
+
+    return parent?.type === 'sr25519';
+  }, [accounts, parentAddress]);
+
+  useEffect(() => {
+    // forbid the use of password since Keyring ignores it
+    if (suriPath?.includes('///')) {
+      setPathError(t('`///password` not supported for derivation'));
+    }
+
+    if (!allowSoftDerivation && suriPath && singleSlashRegex.test(suriPath)) {
+      setPathError(t('Soft derivation is only allowed for sr25519 accounts'));
+    }
+  }, [allowSoftDerivation, suriPath, t]);

  const allAddresses = useMemo(
    () => hierarchy
@@ -48,6 +67,14 @@ export default function SelectParent ({ className, isLocked, onDerivationConfirm
    []
  );

+  const _onSuriPathChange = useCallback(
+    (path: string): void => {
+      setSuriPath(path);
+      setPathError('');
+    },
+    []
+  );
+
  const _onParentChange = useCallback(
    (address: string) => onAction(`/account/derive/${address}`),
    [onAction]
@@ -64,11 +91,11 @@ export default function SelectParent ({ className, isLocked, onDerivationConfirm
          try {
            const account = await validateDerivationPath(parentAddress, suriPath, parentPassword);

-            assert(account, 'Unable to derive');
            onDerivationConfirmed({ account, parentPassword });
          } catch (error) {
            setIsBusy(false);
-            setSuriPath(null);
+            setPathError(t('Invalid derivation path'));
+            console.error(error);
          }
        } else {
          setIsBusy(false);
@@ -76,7 +103,7 @@ export default function SelectParent ({ className, isLocked, onDerivationConfirm
        }
      }
    },
-    [parentAddress, parentPassword, onDerivationConfirmed, suriPath]
+    [parentAddress, parentPassword, onDerivationConfirmed, suriPath, t]
  );

  useEffect(() => {
@@ -89,71 +116,70 @@ export default function SelectParent ({ className, isLocked, onDerivationConfirm
  return (
    <>
      <div className={className}>
-        <div className=''>
-          {isLocked
-            ? (
-              <Address
-                address={parentAddress}
-                genesisHash={parentGenesis}
-              />
-            )
-            : (
-              <Label label={t<string>('Choose Parent Account:')}>
-                <AddressDropdown
-                  allAddresses={allAddresses}
-                  onSelect={_onParentChange}
-                  selectedAddress={parentAddress}
-                  selectedGenesis={parentGenesis}
-                />
-              </Label>
-            )
-          }
-          <div ref={passwordInputRef}>
-            <InputWithLabel
-              data-input-password
-              isError={!!parentPassword && !isProperParentPassword}
-              isFocused
-              label={t<string>('enter the password for the account you want to derive from')}
-              onChange={_onParentPasswordEnter}
-              type='password'
-              value={parentPassword}
+        {isLocked
+          ? (
+            <Address
+              address={parentAddress}
+              genesisHash={parentGenesis}
            />
-            {!!parentPassword && !isProperParentPassword && (
+          )
+          : (
+            <Label label={t<string>('Choose Parent Account:')}>
+              <AddressDropdown
+                allAddresses={allAddresses}
+                onSelect={_onParentChange}
+                selectedAddress={parentAddress}
+                selectedGenesis={parentGenesis}
+              />
+            </Label>
+          )
+        }
+        <div ref={passwordInputRef}>
+          <InputWithLabel
+            data-input-password
+            isError={!!parentPassword && !isProperParentPassword}
+            isFocused
+            label={t<string>('enter the password for the account you want to derive from')}
+            onChange={_onParentPasswordEnter}
+            type='password'
+            value={parentPassword}
+          />
+          {!!parentPassword && !isProperParentPassword && (
+            <Warning
+              isBelowInput
+              isDanger
+            >
+              {t('Wrong password')}
+            </Warning>
+          )}
+        </div>
+        {isProperParentPassword && (
+          <>
+            <DerivationPath
+              defaultPath={defaultPath}
+              isError={!!pathError}
+              onChange={_onSuriPathChange}
+              parentAddress={parentAddress}
+              parentPassword={parentPassword}
+              withSoftPath={allowSoftDerivation}
+            />
+            {(!!pathError) && (
              <Warning
                isBelowInput
                isDanger
              >
-                {t('Wrong password')}
+                {pathError}
              </Warning>
            )}
-          </div>
-          {isProperParentPassword && (
-            <>
-              <DerivationPath
-                defaultPath={defaultPath}
-                isError={!suriPath}
-                onChange={setSuriPath}
-                parentAddress={parentAddress}
-                parentPassword={parentPassword}
-              />
-              {!suriPath && (
-                <Warning
-                  isBelowInput
-                  isDanger
-                >
-                  {t('Incorrect derivation path')}
-                </Warning>
-              )}
-            </>
-          )}
-        </div>
+          </>
+        )}
      </div>
      <VerticalSpace/>
      <ButtonArea>
        <NextStepButton
          data-button-action='create derived account'
          isBusy={isBusy}
-          isDisabled={!isProperParentPassword || !suriPath}
+          isDisabled={!isProperParentPassword || !!pathError}
          onClick={_onSubmit}
        >
          {t<string>('Create a derived account')}