feat: implement requireEmailVerification, fix ambiguousErrorMessages and enableAutologin relationship (#1261)

darkbasic · web-flow · commit b297d77f0ede · 2023-11-15T21:53:51.000+01:00
diff --git a/modules/module-password/__tests__/resolvers/mutation.ts b/modules/module-password/__tests__/resolvers/mutation.ts
@@ -14,6 +14,7 @@ describe('accounts-password resolvers mutation', () => {
     options: {},
   };
   const accountsPasswordMock = {
+    options: {},
     addEmail: jest.fn(),
     changePassword: jest.fn(),
     createUser: jest.fn(),
@@ -90,6 +91,63 @@ describe('accounts-password resolvers mutation', () => {
       expect(accountsPasswordMock.createUser).toHaveBeenCalledWith(user);
     });
 
+    it('should call createUser and return the userId', async () => {
+      const createdUserMock = jest.fn(() => user.id);
+      injector.get = jest.fn((arg) =>
+        arg === AccountsPassword
+          ? {
+              options: {},
+              createUser: createdUserMock,
+            }
+          : {
+              options: {},
+            }
+      );
+      const res = await Mutation.createUser!({}, { user } as any, { injector } as any, {} as any);
+      expect(injector.get).toHaveBeenCalledWith(AccountsPassword);
+      expect(injector.get).toHaveBeenCalledWith(AccountsServer);
+      expect(createdUserMock).toHaveBeenCalledWith(user);
+      expect(res).toEqual({ userId: user.id });
+    });
+
+    it('should return a null userId if both ambiguousErrorMessages and requireEmailVerification are enabled', async () => {
+      const createdUserMock = jest.fn(() => user.id);
+      injector.get = jest.fn((arg) =>
+        arg === AccountsPassword
+          ? {
+              options: { requireEmailVerification: true },
+              createUser: createdUserMock,
+            }
+          : {
+              options: { ambiguousErrorMessages: true },
+            }
+      );
+      const res = await Mutation.createUser!({}, { user } as any, { injector } as any, {} as any);
+      expect(injector.get).toHaveBeenCalledWith(AccountsPassword);
+      expect(injector.get).toHaveBeenCalledWith(AccountsServer);
+      expect(createdUserMock).toHaveBeenCalledWith(user);
+      expect(res).toEqual({ userId: null });
+    });
+
+    it('should return the userId despite ambiguousErrorMessages being enabled if requireEmailVerification is disabled', async () => {
+      const createdUserMock = jest.fn(() => user.id);
+      injector.get = jest.fn((arg) =>
+        arg === AccountsPassword
+          ? {
+              options: { requireEmailVerification: false },
+              createUser: createdUserMock,
+            }
+          : {
+              options: { ambiguousErrorMessages: true },
+            }
+      );
+      const res = await Mutation.createUser!({}, { user } as any, { injector } as any, {} as any);
+      expect(injector.get).toHaveBeenCalledWith(AccountsPassword);
+      expect(injector.get).toHaveBeenCalledWith(AccountsServer);
+      expect(createdUserMock).toHaveBeenCalledWith(user);
+      expect(res).toEqual({ userId: user.id });
+    });
+
     it('should call createUser and obfuscate EmailAlreadyExists error if server have ambiguousErrorMessages', async () => {
       const createdUserMock = jest.fn(() => {
         throw new AccountsJsError('EmailAlreadyExists', 'EmailAlreadyExists');
diff --git a/modules/module-password/src/resolvers/mutation.ts b/modules/module-password/src/resolvers/mutation.ts
@@ -69,11 +69,15 @@ export const Mutation: MutationResolvers = {
 
     if (!accountsServer.options.enableAutologin) {
       return {
-        userId: accountsServer.options.ambiguousErrorMessages ? null : userId,
+        userId:
+          accountsServer.options.ambiguousErrorMessages &&
+          accountsPassword.options.requireEmailVerification
+            ? null
+            : userId,
       };
     }
 
-    // When initializing AccountsServer we check that enableAutologin and ambiguousErrorMessages options
+    // When initializing AccountsPassword we check that enableAutologin and requireEmailVerification options
     // are not enabled at the same time
 
     const createdUser = await accountsServer.findUserById(userId);
diff --git a/modules/module-password/src/schema/types.ts b/modules/module-password/src/schema/types.ts
@@ -2,9 +2,9 @@ import gql from 'graphql-tag';
 
 export default gql`
   type CreateUserResult {
-    # Will be returned only if ambiguousErrorMessages is set to false.
+    # Will be returned only if ambiguousErrorMessages is set to false or requireEmailVerification is set to false.
     userId: ID
-    # Will be returned only if enableAutologin is set to true.
+    # Will be returned only if enableAutologin is set to true and requireEmailVerification is set to false.
     loginResult: LoginResult
   }
 
diff --git a/packages/e2e/__tests__/password.ts b/packages/e2e/__tests__/password.ts
@@ -19,8 +19,7 @@ Object.keys(servers).forEach((key) => {
           email: user.email,
           password: user.password,
         });
-        // User id will be null for graphql and undefined for rest
-        expect(userId === null || userId === undefined).toBeTruthy();
+        expect(typeof userId).toEqual('string');
       });
     });
 
diff --git a/packages/password/__tests__/accounts-password.ts b/packages/password/__tests__/accounts-password.ts
@@ -1,6 +1,7 @@
 import 'reflect-metadata';
 import set from 'lodash.set';
 import { AccountsPassword } from '../src';
+import { AccountsServer } from '@accounts/server';
 
 describe('AccountsPassword', () => {
   const server: any = {
@@ -19,6 +20,20 @@ describe('AccountsPassword', () => {
   });
 
   describe('config', () => {
+    it('should throw if requireEmailVerification and enableAutologin flags enabled at the same time', async () => {
+      expect(
+        () =>
+          new AccountsPassword(
+            { requireEmailVerification: true },
+            undefined,
+            undefined,
+            new AccountsServer({ enableAutologin: true, tokenSecret: 'secret' }, {}, {} as any)
+          )
+      ).toThrow(
+        "Can't enable autologin when requireEmailVerification is enabled. Please set either of them to false."
+      );
+    });
+
     it('should have default options', async () => {
       expect((password as any).options.passwordEnrollTokenExpiration).toBe(2592000000);
     });
@@ -50,6 +65,34 @@ describe('AccountsPassword', () => {
       expect(ret).toEqual(user);
     });
 
+    it('throws when email has not been verified and requireEmailVerification is true', async () => {
+      const findUserByEmail = jest.fn(() =>
+        Promise.resolve({
+          emails: [
+            {
+              address: 'toto@toto.com',
+              verified: false,
+            },
+          ],
+          id: '1',
+          deactivated: false,
+        })
+      );
+      const findPasswordHash = jest.fn(() => Promise.resolve('hashed'));
+      const verifyPassword = jest.fn(() => true);
+      const tmpAccountsPassword = new AccountsPassword({});
+      tmpAccountsPassword.options.verifyPassword = verifyPassword as any;
+      tmpAccountsPassword.options.requireEmailVerification = true;
+      tmpAccountsPassword.setUserStore({ findUserByEmail, findPasswordHash } as any);
+      tmpAccountsPassword.setSessionsStore();
+      await expect(
+        tmpAccountsPassword.authenticate({
+          user: 'toto@toto.com',
+          password: 'toto',
+        } as any)
+      ).rejects.toThrow('Email not verified');
+    });
+
     it('throws when user not found', async () => {
       const findUserByEmail = jest.fn(() => Promise.resolve());
       password.setUserStore({ findUserByEmail } as any);
diff --git a/packages/password/src/accounts-password.ts b/packages/password/src/accounts-password.ts
@@ -50,6 +50,13 @@ export interface AccountsPasswordOptions {
    * Two factor options passed down to the @accounts/two-factor service.
    */
   twoFactor?: AccountsTwoFactorOptions;
+  /**
+   * Whether the email needs to be verified in order to allow authentication.
+   * From an user enumeration perspective changes what is safe to return when ambiguousErrorMessages are enabled.
+   * Can be enabled only if enableAutologin is set to false.
+   * Defaults to false.
+   */
+  requireEmailVerification?: boolean;
   /**
    * The number of milliseconds from when a link to verify the user email is sent until token expires and user can't verify his email with the link anymore.
    * Defaults to 3 days.
@@ -136,6 +143,7 @@ export interface AccountsPasswordOptions {
 }
 
 const defaultOptions = {
+  requireEmailVerification: false,
   // 3 days - 3 * 24 * 60 * 60 * 1000
   verifyEmailTokenExpiration: 259200000,
   // 3 days - 3 * 24 * 60 * 60 * 1000
@@ -185,7 +193,7 @@ export default class AccountsPassword<CustomUser extends User = User>
   public serviceName = 'password';
   public server!: AccountsServer;
   public twoFactor: TwoFactor;
-  private options: AccountsPasswordOptions & typeof defaultOptions;
+  options: AccountsPasswordOptions & typeof defaultOptions;
   private db!: DatabaseInterfaceUser<CustomUser>;
   private dbSessions!: DatabaseInterfaceSessions;
 
@@ -196,7 +204,21 @@ export default class AccountsPassword<CustomUser extends User = User>
     @Inject(DatabaseInterfaceSessionsToken) dbSessions?: DatabaseInterfaceSessions,
     server?: AccountsServer
   ) {
-    this.options = { ...defaultOptions, ...options };
+    this.options = { ...defaultOptions, ...options } as typeof options & typeof defaultOptions;
+    if (this.options.requireEmailVerification) {
+      if (server?.options.enableAutologin) {
+        throw new Error(
+          "Can't enable autologin when requireEmailVerification is enabled. Please set either of them to false."
+        );
+      }
+      // AccountsPassword has been manually instantiated so there is no way to access the AccountsServer options
+      if (!server) {
+        console.log(
+          "Please ensure that 'enableAutologin' has not been set to true in AccountsServer."
+        );
+      }
+    }
+
     this.twoFactor = new TwoFactor(options.twoFactor);
     if (db) {
       this.db = db;
@@ -716,6 +738,19 @@ export default class AccountsPassword<CustomUser extends User = User>
         );
       }
     }
+    if (this.options.requireEmailVerification) {
+      // If the user logs in using the email it must be a verified address, if he provided an username at least one of the associated emails must be verified.
+      if (
+        !foundUser.emails?.find(({ address, verified }) =>
+          email ? address === email && verified : verified
+        )
+      ) {
+        throw new AccountsJsError(
+          this.options.errors.emailNotVerified,
+          PasswordAuthenticatorErrors.EmailNotVerified
+        );
+      }
+    }
 
     const hash = await this.db.findPasswordHash(foundUser.id);
     if (!hash) {
diff --git a/packages/password/src/errors.ts b/packages/password/src/errors.ts
@@ -9,6 +9,7 @@ export const errors: ErrorMessages = {
   matchFailed: 'Match failed',
   invalidUsername: 'Invalid username',
   invalidEmail: 'Invalid email',
+  emailNotVerified: 'Email not verified',
   invalidPassword: 'Invalid password',
   invalidNewPassword: 'Invalid new password',
   invalidToken: 'Invalid token',
@@ -95,6 +96,7 @@ export enum ResetPasswordErrors {
 }
 
 export enum PasswordAuthenticatorErrors {
+  EmailNotVerified = 'EmailNotVerified',
   InvalidCredentials = 'InvalidCredentials',
   UserNotFound = 'UserNotFound',
   IncorrectPassword = 'IncorrectPassword',
diff --git a/packages/password/src/types/error-messages.ts b/packages/password/src/types/error-messages.ts
@@ -31,6 +31,10 @@ export interface ErrorMessages {
    * Default to 'Invalid email'
    */
   invalidEmail: string;
+  /**
+   * Default to 'Email not verified'
+   */
+  emailNotVerified: string;
   /**
    * Default to 'Invalid password'
    */
diff --git a/packages/rest-express/__tests__/endpoints/password/register.ts b/packages/rest-express/__tests__/endpoints/password/register.ts
@@ -16,6 +16,7 @@ describe('registerPassword', () => {
   it('calls password.createUser and returns the user json response', async () => {
     const userId = '1';
     const passwordService = {
+      options: {},
       createUser: jest.fn(() => userId),
     };
     const accountsServer = {
@@ -47,9 +48,10 @@ describe('registerPassword', () => {
     expect(res.status).not.toHaveBeenCalled();
   });
 
-  it('calls password.createUser and obfuscate user id if server have ambiguousErrorMessages', async () => {
+  it('calls password.createUser and obfuscate user id if server have both ambiguousErrorMessages and requireEmailVerification', async () => {
     const userId = '1';
     const passwordService = {
+      options: { requireEmailVerification: true },
       createUser: jest.fn(() => userId),
     };
     const accountsServer = {
@@ -81,8 +83,44 @@ describe('registerPassword', () => {
     expect(res.status).not.toHaveBeenCalled();
   });
 
+  it('calls password.createUser and not obfuscate user id if server have ambiguousErrorMessages but not requireEmailVerification', async () => {
+    const userId = '1';
+    const passwordService = {
+      options: {},
+      createUser: jest.fn(() => userId),
+    };
+    const accountsServer = {
+      options: { ambiguousErrorMessages: true },
+      getServices: () => ({
+        password: passwordService,
+      }),
+    };
+    const middleware = registerPassword(accountsServer as any);
+
+    const req = {
+      body: {
+        user: {
+          username: 'toto',
+        },
+        extraFieldThatShouldNotBePassed: 'hey',
+      },
+      headers: {},
+    };
+    const reqCopy = { ...req };
+
+    await middleware(req as any, res);
+
+    expect(req).toEqual(reqCopy);
+    expect(accountsServer.getServices().password.createUser).toHaveBeenCalledWith({
+      username: 'toto',
+    });
+    expect(res.json).toHaveBeenCalledWith({ userId });
+    expect(res.status).not.toHaveBeenCalled();
+  });
+
   it('calls password.createUser and obfuscate EmailAlreadyExists error if server have ambiguousErrorMessages', async () => {
     const passwordService = {
+      options: {},
       createUser: jest.fn(() => {
         throw new AccountsJsError('EmailAlreadyExists', 'EmailAlreadyExists');
       }),
@@ -118,6 +156,7 @@ describe('registerPassword', () => {
 
   it('calls password.createUser and obfuscate UsernameAlreadyExists error if server have ambiguousErrorMessages', async () => {
     const passwordService = {
+      options: {},
       createUser: jest.fn(() => {
         throw new AccountsJsError('UsernameAlreadyExists', 'UsernameAlreadyExists');
       }),
@@ -156,6 +195,7 @@ describe('registerPassword', () => {
     const userEmail = 'test@test.com';
 
     const passwordService = {
+      options: {},
       createUser: jest.fn(() => userId),
     };
 
@@ -212,6 +252,7 @@ describe('registerPassword', () => {
   it('Sends error if it was thrown on loginWithService', async () => {
     const error = { message: 'Could not login' };
     const passwordService = {
+      options: {},
       createUser: jest.fn(() => {
         throw error;
       }),
diff --git a/packages/rest-express/src/endpoints/password/register.ts b/packages/rest-express/src/endpoints/password/register.ts
@@ -29,15 +29,16 @@ export const registerPassword =
 
       if (!accountsServer.options.enableAutologin) {
         return res.json(
-          accountsServer.options.ambiguousErrorMessages
+          accountsServer.options.ambiguousErrorMessages &&
+            accountsPassword.options.requireEmailVerification
             ? ({} as CreateUserResult)
             : ({
                 userId,
               } as CreateUserResult)
         );
       }
 
-      // When initializing AccountsServer we check that enableAutologin and ambiguousErrorMessages options
+      // When initializing AccountsPassword we check that enableAutologin and requireEmailVerification options
       // are not enabled at the same time
 
       const createdUser = await accountsServer.findUserById(userId);
diff --git a/packages/server/__tests__/account-server.ts b/packages/server/__tests__/account-server.ts
diff --git a/packages/server/src/accounts-server.ts b/packages/server/src/accounts-server.ts
diff --git a/packages/server/src/types/accounts-server-options.ts b/packages/server/src/types/accounts-server-options.ts
diff --git a/packages/types/src/types/create-user-result.ts b/packages/types/src/types/create-user-result.ts
