Merge pull request #61 from yougotwill/fix/no-ref/display_name_validation

ts/components/dialog/edit-profile/EditProfileDialog.tsx

@@ -18,8 +18,10 @@ import { Spacer2XL, Spacer3XL, SpacerLG, SpacerSM, SpacerXL } from '../../basic/
 import { CopyToClipboardButton } from '../../buttons/CopyToClipboardButton';
 import { SessionInput } from '../../inputs';
 import { SessionSpinner } from '../../loading';
-import { sanitizeDisplayNameOrToast } from '../../registration/utils';
 import { ProfileHeader, ProfileName, QRView } from './components';
+import { EmptyDisplayNameError, RetrieveDisplayNameError } from '../../../session/utils/errors';
+import { localize } from '../../../localization/localeTools';
+import { sanitizeDisplayNameOrToast } from '../../registration/utils';
 
 // #region Shortcuts
 const handleKeyQRMode = (
@@ -173,6 +175,7 @@ export const EditProfileDialog = () => {
   const [profileName, setProfileName] = useState(_profileName);
   const [updatedProfileName, setUpdateProfileName] = useState(profileName);
   const [profileNameError, setProfileNameError] = useState<string | undefined>(undefined);
+  const [cannotContinue, setCannotContinue] = useState(true);
 
   const copyButtonRef = useRef<HTMLButtonElement>(null);
   const inputRef = useRef<HTMLInputElement>(null);
@@ -207,12 +210,15 @@ export const EditProfileDialog = () => {
       : undefined;
 
   const onClickOK = async () => {
-    if (isEmpty(profileName) || !isEmpty(profileNameError)) {
-      return;
-    }
-
     try {
       setLoading(true);
+      const sanitizedName = sanitizeDisplayNameOrToast(profileName);
+
+      // this should never happen, but just in case
+      if (isEmpty(sanitizedName)) {
+        return;
+      }
+
       // Note: this will not throw, but just truncate the display name if it is too long.
       // I guess it is expected as there is no UI to show anything else than a generic error?
       const validName = await ProfileManager.updateOurProfileDisplayName(profileName);
@@ -221,7 +227,13 @@ export const EditProfileDialog = () => {
       setMode('default');
     } catch (err) {
       window.log.error('Profile update error', err);
-      setProfileNameError(window.i18n('errorUnknown'));
+      setCannotContinue(true);
+
+      if (err instanceof EmptyDisplayNameError || err instanceof RetrieveDisplayNameError) {
+        setProfileNameError(localize('displayNameErrorDescription').toString());
+      } else {
+        setProfileNameError(localize('errorUnknown').toString());
+      }
     } finally {
       setLoading(false);
     }
@@ -324,8 +336,8 @@ export const EditProfileDialog = () => {
             placeholder={window.i18n('displayNameEnter')}
             value={profileName}
             onValueChanged={(name: string) => {
-              const sanitizedName = sanitizeDisplayNameOrToast(name, setProfileNameError);
+              setProfileName(name);
-              setProfileName(sanitizedName);
+              setCannotContinue(false);
             }}
             editable={!loading}
             tabIndex={0}
@@ -381,7 +393,7 @@ export const EditProfileDialog = () => {
               <SessionButton
                 text={window.i18n('save')}
                 onClick={onClickOK}
-                disabled={loading}
+                disabled={cannotContinue}
                 dataTestId="save-button-profile-update"
               />
             )

ts/components/registration/stages/CreateAccount.tsx

@@ -1,6 +1,7 @@
 import { isEmpty } from 'lodash';
 import { useDispatch } from 'react-redux';
 import useMount from 'react-use/lib/useMount';
+import { useState } from 'react';
 import { SettingsKey } from '../../../data/settings-key';
 import { mnDecode } from '../../../session/crypto/mnemonic';
 import { ProfileManager } from '../../../session/profile_manager/ProfileManager';
@@ -32,22 +33,21 @@ import { SessionInput } from '../../inputs';
 import { resetRegistration } from '../RegistrationStages';
 import { ContinueButton, OnboardDescription, OnboardHeading } from '../components';
 import { BackButtonWithinContainer } from '../components/BackButton';
-import { displayNameIsValid, sanitizeDisplayNameOrToast } from '../utils';
+import { sanitizeDisplayNameOrToast } from '../utils';
-import { RetrieveDisplayNameError } from '../../../session/utils/errors';
+import { EmptyDisplayNameError, RetrieveDisplayNameError } from '../../../session/utils/errors';
 import { localize } from '../../../localization/localeTools';
 
-export type AccountDetails = {
+type AccountCreateDetails = {
   recoveryPassword: string;
-  displayName?: string;
+  displayName: string;
 };
 
-async function signUp(signUpDetails: AccountDetails) {
+async function signUp(signUpDetails: AccountCreateDetails) {
   const { displayName, recoveryPassword } = signUpDetails;
 
   try {
-    const validDisplayName = displayNameIsValid(displayName);
     await resetRegistration();
-    await registerSingleDevice(recoveryPassword, 'english', validDisplayName);
+    await registerSingleDevice(recoveryPassword, 'english', displayName);
     await Storage.put(SettingsKey.hasSyncedInitialConfigurationItem, Date.now());
     await setSignWithRecoveryPhrase(false);
     trigger('openInbox');
@@ -64,6 +64,8 @@ export const CreateAccount = () => {
 
   const dispatch = useDispatch();
 
+  const [cannotContinue, setCannotContinue] = useState(true);
+
   const generateMnemonicAndKeyPair = async () => {
     if (recoveryPassword === '') {
       const mnemonic = await generateMnemonic();
@@ -89,13 +91,16 @@ export const CreateAccount = () => {
   });
 
   const signUpWithDetails = async () => {
-    if (isEmpty(displayName) || !isEmpty(displayNameError)) {
-      return;
-    }
-
     try {
+      const sanitizedName = sanitizeDisplayNameOrToast(displayName);
+
+      // this should never happen, but just in case
+      if (isEmpty(sanitizedName)) {
+        return;
+      }
+
       // this throws if the display name is too long
-      const validName = await ProfileManager.updateOurProfileDisplayNameOnboarding(displayName);
+      const validName = await ProfileManager.updateOurProfileDisplayNameOnboarding(sanitizedName);
 
       await signUp({
         displayName: validName,
@@ -107,16 +112,17 @@ export const CreateAccount = () => {
       window.log.error(
         `[onboarding] create account: signUpWithDetails failed! Error: ${err.message || String(err)}`
       );
+
+      setCannotContinue(true);
       dispatch(setAccountCreationStep(AccountCreation.DisplayName));
 
-      if (err instanceof RetrieveDisplayNameError) {
+      if (err instanceof EmptyDisplayNameError || err instanceof RetrieveDisplayNameError) {
         dispatch(setDisplayNameError(localize('displayNameErrorDescription').toString()));
-        return;
+      } else {
+        // Note: we have to assume here that libsession threw an error because the name was too long since we covered the other cases.
+        // The error reported by libsession is not localized
+        dispatch(setDisplayNameError(localize('displayNameErrorDescriptionShorter').toString()));
       }
-
-      // Note: we have to assume here that libsession threw an error because the name was too long since we covered the other cases.
-      // The error reported by libsession is not localized
-      dispatch(setDisplayNameError(localize('displayNameErrorDescriptionShorter').toString()));
     }
   };
 
@@ -150,18 +156,15 @@ export const CreateAccount = () => {
           placeholder={window.i18n('displayNameEnter')}
           value={displayName}
           onValueChanged={(name: string) => {
-            const sanitizedName = sanitizeDisplayNameOrToast(name, setDisplayNameError, dispatch);
+            dispatch(setDisplayName(name));
-            dispatch(setDisplayName(sanitizedName));
+            setCannotContinue(false);
           }}
           onEnterPressed={signUpWithDetails}
           error={displayNameError}
           inputDataTestId="display-name-input"
         />
         <SpacerLG />
-        <ContinueButton
+        <ContinueButton onClick={signUpWithDetails} disabled={cannotContinue} />
-          onClick={signUpWithDetails}
-          disabled={isEmpty(displayName) || !isEmpty(displayNameError)}
-        />
       </Flex>
     </BackButtonWithinContainer>
   );

ts/components/registration/stages/RestoreAccount.tsx

@@ -1,12 +1,17 @@
 import { Dispatch } from '@reduxjs/toolkit';
 import { isEmpty } from 'lodash';
 import { useDispatch } from 'react-redux';
+import { useState } from 'react';
 import { ONBOARDING_TIMES } from '../../../session/constants';
 import { InvalidWordsError, NotEnoughWordsError } from '../../../session/crypto/mnemonic';
 import { ProfileManager } from '../../../session/profile_manager/ProfileManager';
 import { PromiseUtils } from '../../../session/utils';
 import { TaskTimedOutError } from '../../../session/utils/Promise';
-import { NotFoundError, RetrieveDisplayNameError } from '../../../session/utils/errors';
+import {
+  EmptyDisplayNameError,
+  NotFoundError,
+  RetrieveDisplayNameError,
+} from '../../../session/utils/errors';
 import { trigger } from '../../../shims/events';
 import {
   AccountRestoration,
@@ -41,11 +46,14 @@ import { resetRegistration } from '../RegistrationStages';
 import { ContinueButton, OnboardDescription, OnboardHeading } from '../components';
 import { BackButtonWithinContainer } from '../components/BackButton';
 import { useRecoveryProgressEffect } from '../hooks';
-import { displayNameIsValid, sanitizeDisplayNameOrToast } from '../utils';
-import { AccountDetails } from './CreateAccount';
 import { localize } from '../../../localization/localeTools';
+import { sanitizeDisplayNameOrToast } from '../utils';
 
-type AccountRestoreDetails = AccountDetails & { dispatch: Dispatch; abortSignal?: AbortSignal };
+type AccountRestoreDetails = {
+  recoveryPassword: string;
+  dispatch: Dispatch;
+  abortSignal?: AbortSignal;
+};
 
 export async function finishRestore(pubkey: string, displayName: string) {
   await setSignWithRecoveryPhrase(true);
@@ -96,21 +104,14 @@ async function signInWithNewDisplayName({
   displayName,
   recoveryPassword,
   dispatch,
-}: AccountRestoreDetails) {
+}: AccountRestoreDetails & { displayName: string }) {
   try {
-    const validDisplayName = displayNameIsValid(displayName);
-
     await resetRegistration();
-    await registerSingleDevice(
+    await registerSingleDevice(recoveryPassword, 'english', displayName, async (pubkey: string) => {
-      recoveryPassword,
+      dispatch(setHexGeneratedPubKey(pubkey));
-      'english',
+      dispatch(setDisplayName(displayName));
-      validDisplayName,
+      await finishRestore(pubkey, displayName);
-      async (pubkey: string) => {
+    });
-        dispatch(setHexGeneratedPubKey(pubkey));
-        dispatch(setDisplayName(validDisplayName));
-        await finishRestore(pubkey, validDisplayName);
-      }
-    );
   } catch (e) {
     await resetRegistration();
     throw e;
@@ -129,6 +130,8 @@ export const RestoreAccount = () => {
 
   const dispatch = useDispatch();
 
+  const [cannotContinue, setCannotContinue] = useState(true);
+
   useRecoveryProgressEffect();
 
   const recoverAndFetchDisplayName = async () => {
@@ -171,18 +174,20 @@ export const RestoreAccount = () => {
   };
 
   const recoverAndEnterDisplayName = async () => {
-    if (
+    if (isEmpty(recoveryPassword) || !isEmpty(recoveryPasswordError)) {
-      isEmpty(recoveryPassword) ||
-      !isEmpty(recoveryPasswordError) ||
-      isEmpty(displayName) ||
-      !isEmpty(displayNameError)
-    ) {
       return;
     }
 
     try {
+      const sanitizedName = sanitizeDisplayNameOrToast(displayName);
+
+      // this should never happen, but just in case
+      if (isEmpty(sanitizedName)) {
+        return;
+      }
+
       // this will throw if the display name is too long
-      const validName = await ProfileManager.updateOurProfileDisplayNameOnboarding(displayName);
+      const validName = await ProfileManager.updateOurProfileDisplayNameOnboarding(sanitizedName);
 
       const trimmedPassword = recoveryPassword.trim();
       setRecoveryPassword(trimmedPassword);
@@ -196,16 +201,17 @@ export const RestoreAccount = () => {
       window.log.error(
         `[onboarding] restore account: Failed with new display name! Error: ${err.message || String(err)}`
       );
+
+      setCannotContinue(true);
       dispatch(setAccountRestorationStep(AccountRestoration.DisplayName));
 
-      if (err instanceof RetrieveDisplayNameError) {
+      if (err instanceof EmptyDisplayNameError || err instanceof RetrieveDisplayNameError) {
         dispatch(setDisplayNameError(localize('displayNameErrorDescription').toString()));
-        return;
+      } else {
+        // Note: we have to assume here that libsession threw an error because the name was too long since we covered the other cases.
+        // The error reported by libsession is not localized
+        dispatch(setDisplayNameError(localize('displayNameErrorDescriptionShorter').toString()));
       }
-
-      // Note: we have to assume here that libsession threw an error because the name was too long since we covered the other cases.
-      // The error reported by libsession is not localized
-      dispatch(setDisplayNameError(localize('displayNameErrorDescriptionShorter').toString()));
     }
   };
 
@@ -313,12 +319,8 @@ export const RestoreAccount = () => {
               placeholder={window.i18n('displayNameEnter')}
               value={displayName}
               onValueChanged={(name: string) => {
-                const sanitizedName = sanitizeDisplayNameOrToast(
+                dispatch(setDisplayName(name));
-                  name,
+                setCannotContinue(false);
-                  setDisplayNameError,
-                  dispatch
-                );
-                dispatch(setDisplayName(sanitizedName));
               }}
               onEnterPressed={recoverAndEnterDisplayName}
               error={displayNameError}
@@ -328,10 +330,7 @@ export const RestoreAccount = () => {
             <ContinueButton
               onClick={recoverAndEnterDisplayName}
               disabled={
-                isEmpty(recoveryPassword) ||
+                isEmpty(recoveryPassword) || !isEmpty(recoveryPasswordError) || cannotContinue
-                !isEmpty(recoveryPasswordError) ||
-                isEmpty(displayName) ||
-                !isEmpty(displayNameError)
               }
             />
           </Flex>

ts/components/registration/utils/index.tsx

@@ -1,37 +1,12 @@
-import { Dispatch } from '@reduxjs/toolkit';
+import { EmptyDisplayNameError } from '../../../session/utils/errors';
 import { sanitizeSessionUsername } from '../../../session/utils/String';
 
-export function sanitizeDisplayNameOrToast(
+export function sanitizeDisplayNameOrToast(displayName: string) {
-  displayName: string,
+  const sanitizedName = sanitizeSessionUsername(displayName).trim();
-  // can be a useState or redux function
+
-  onDisplayNameError: (error: string | undefined) => any,
+  if (!sanitizedName) {
-  dispatch?: Dispatch
+    throw new EmptyDisplayNameError();
-) {
-  const sanitizedName = sanitizeSessionUsername(displayName);
-  const errorString = !sanitizedName ? window.i18n('displayNameErrorDescription') : undefined;
-  if (dispatch) {
-    dispatch(onDisplayNameError(errorString));
-  } else {
-    onDisplayNameError(errorString); // this is is either calling dispatch in the caller or just `setDisplayNameError`
   }
 
   return sanitizedName;
 }
-
-/**
- * Returns undefined if an error happened, or the trim userName.
- *
- * Be sure to use the trimmed userName for creating the account.
- */
-export const displayNameIsValid = (displayName?: string): string => {
-  if (!displayName) {
-    throw new Error(window.i18n('displayNameErrorDescription'));
-  }
-
-  const trimName = displayName.trim();
-  if (!trimName) {
-    throw new Error(window.i18n('displayNameErrorDescription'));
-  }
-
-  return trimName;
-};

ts/session/profile_manager/ProfileManager.ts

@@ -100,18 +100,17 @@ async function updateProfileOfContact(
  * When registering a user/linking a device, we want to enforce a limit on the displayName length.
  * That limit is enforced by libsession when calling `setName` on the `UserConfigWrapper`.
  * `updateOurProfileDisplayNameOnboarding` is used to create a temporary `UserConfigWrapper`, call `setName` on it and release the memory used by the wrapper.
- * @returns the set displayName set if no error where thrown.
+ * @returns the set displayName set if no error where thrown
+ * @note Make sure the displayName has been trimmed and validated first.
  */
 async function updateOurProfileDisplayNameOnboarding(newName: string) {
-  const cleanName = sanitizeSessionUsername(newName).trim();
-
   try {
     // create a temp user config wrapper to test the display name with libsession
     const privKey = new Uint8Array(64);
     crypto.getRandomValues(privKey);
     await UserConfigWrapperActions.init(privKey, null);
     // this throws if the name is too long
-    await UserConfigWrapperActions.setName(cleanName);
+    await UserConfigWrapperActions.setName(newName);
     const appliedName = await UserConfigWrapperActions.getName();
 
     if (isNil(appliedName)) {

ts/session/utils/errors.ts

@@ -67,12 +67,17 @@ export class HTTPError extends Error {
   }
 }
 
+/**
+ * Base error class for all errors in the session module.
+ *
+ * @note if you make a custom error with a custom message, make sure to restore the prototype chain again using the new class prototype.
+ */
 class BaseError extends Error {
   constructor(message: string) {
     super(message);
     this.name = this.constructor.name;
-    // restore prototype chain
+    // NOTE Restores prototype chain. Make sure to reference the new class prototype!
-    Object.setPrototypeOf(this, SnodeResponseError.prototype);
+    Object.setPrototypeOf(this, BaseError.prototype);
   }
 }
 
@@ -85,12 +90,19 @@ export class InvalidMessage extends BaseError {}
 export class SnodeResponseError extends BaseError {
   constructor(message = 'sessionRpc could not talk to node') {
     super(message);
+    Object.setPrototypeOf(this, SnodeResponseError.prototype);
   }
 }
-export class RetrieveDisplayNameError extends Error {
+export class RetrieveDisplayNameError extends BaseError {
   constructor(message = 'failed to retrieve display name after setting it') {
     super(message);
-    // restore prototype chain
+    Object.setPrototypeOf(this, RetrieveDisplayNameError.prototype);
-    Object.setPrototypeOf(this, SnodeResponseError.prototype);
+  }
+}
+
+export class EmptyDisplayNameError extends BaseError {
+  constructor(message = 'display name is empty') {
+    super(message);
+    Object.setPrototypeOf(this, EmptyDisplayNameError.prototype);
   }
 }

ts/test/session/unit/onboarding/Onboarding_test.ts

@@ -1,6 +1,5 @@
 import { expect } from 'chai';
 import Sinon from 'sinon';
-import { displayNameIsValid } from '../../../../components/registration/utils';
 import { getSwarmPollingInstance } from '../../../../session/apis/snode_api';
 import { PubKey } from '../../../../session/types';
 import {
@@ -10,6 +9,8 @@ import {
 } from '../../../../util/accountManager';
 import { TestUtils } from '../../../test-utils';
 import { stubWindow } from '../../../test-utils/utils';
+import { sanitizeDisplayNameOrToast } from '../../../../components/registration/utils';
+import { EmptyDisplayNameError } from '../../../../session/utils/errors';
 
 describe('Onboarding', () => {
   const polledDisplayName = 'Hello World';
@@ -28,31 +29,28 @@ describe('Onboarding', () => {
     Sinon.restore();
   });
 
-  describe('displayNameIsValid', () => {
+  describe('sanitizeDisplayNameOrToast', () => {
     it('should throw an error if the display name is undefined', async () => {
       try {
-        displayNameIsValid(undefined);
+        sanitizeDisplayNameOrToast('');
       } catch (error) {
-        error.should.be.an.instanceOf(Error);
+        error.should.be.an.instanceOf(EmptyDisplayNameError);
-        error.message.should.equal(window.i18n('displayNameErrorDescription'));
       }
     });
     it('should throw an error if the display name is empty after trimming', async () => {
       try {
-        displayNameIsValid('    ');
+        sanitizeDisplayNameOrToast('    ');
       } catch (error) {
-        error.should.be.an.instanceOf(Error);
+        error.should.be.an.instanceOf(EmptyDisplayNameError);
-        error.message.should.equal(window.i18n('displayNameErrorDescription'));
       }
     });
     it('if the display name is valid it should be returned', async () => {
       try {
         const displayName = 'Hello World';
-        const validDisplayName = displayNameIsValid(displayName);
+        const validDisplayName = sanitizeDisplayNameOrToast(displayName);
         expect(validDisplayName, `should equal ${displayName}`).to.equal(displayName);
       } catch (error) {
-        error.should.not.be.an.instanceOf(Error);
+        error.should.not.be.an.instanceOf(EmptyDisplayNameError);
-        error.message.should.not.equal(window.i18n('displayNameErrorDescription'));
       }
     });
   });