fix: Surface all auth errors during login

This PR patches the following issue: 1. Sign up with Github with user A 2. Sign out 3. Try to sign in with the email of user A 4. The API returns a 400 error not displayed in the Login form. After this PR, all server auth errors will be displayed in the UI. It also contains some minor copywriting fixes and clean-ups.
novuhq · Mar 20, 2024 · 5741bf4 · 5741bf4
1 parent 5b7a721
commit 5741bf4
Show file tree

Hide file tree

Showing 5 changed files with 153 additions and 124 deletions.
diff --git a/apps/api/src/app/auth/e2e/login.e2e.ts b/apps/api/src/app/auth/e2e/login.e2e.ts
@@ -13,154 +13,179 @@ describe('User login - /auth/login (POST)', async () => {
     password: '123Qwerty@',
   };
 
-  before(async () => {
-    session = new UserSession();
-    await session.initialize();
+  context('with email/password', async () => {
+    before(async () => {
+      session = new UserSession();
+      await session.initialize();
+
+      const { body } = await session.testAgent
+        .post('/v1/auth/register')
+        .send({
+          email: userCredentials.email,
+          password: userCredentials.password,
+          firstName: 'Test',
+          lastName: 'User',
+        })
+        .expect(201);
+    });
 
-    const { body } = await session.testAgent
-      .post('/v1/auth/register')
-      .send({
+    it('should login the user correctly', async () => {
+      const { body } = await session.testAgent.post('/v1/auth/login').send({
         email: userCredentials.email,
         password: userCredentials.password,
-        firstName: 'Test',
-        lastName: 'User',
-      })
-      .expect(201);
-  });
+      });
 
-  it('should login the user correctly', async () => {
-    const { body } = await session.testAgent.post('/v1/auth/login').send({
-      email: userCredentials.email,
-      password: userCredentials.password,
+      const jwtContent = (await jwt.decode(body.data.token)) as IJwtPayload;
+
+      expect(jwtContent.firstName).to.equal('test');
+      expect(jwtContent.lastName).to.equal('user');
+      expect(jwtContent.email).to.equal('testytest22@gmail.com');
     });
 
-    const jwtContent = (await jwt.decode(body.data.token)) as IJwtPayload;
+    it('should login the user correctly with uppercase email', async () => {
+      const { body } = await session.testAgent.post('/v1/auth/login').send({
+        email: userCredentials.email.toUpperCase(),
+        password: userCredentials.password,
+      });
 
-    expect(jwtContent.firstName).to.equal('test');
-    expect(jwtContent.lastName).to.equal('user');
-    expect(jwtContent.email).to.equal('testytest22@gmail.com');
-  });
+      const jwtContent = (await jwt.decode(body.data.token)) as IJwtPayload;
 
-  it('should login the user correctly with uppercase email', async () => {
-    const { body } = await session.testAgent.post('/v1/auth/login').send({
-      email: userCredentials.email.toUpperCase(),
-      password: userCredentials.password,
+      expect(jwtContent.firstName).to.equal('test');
+      expect(jwtContent.lastName).to.equal('user');
+      expect(jwtContent.email).to.equal('testytest22@gmail.com');
     });
 
-    const jwtContent = (await jwt.decode(body.data.token)) as IJwtPayload;
-
-    expect(jwtContent.firstName).to.equal('test');
-    expect(jwtContent.lastName).to.equal('user');
-    expect(jwtContent.email).to.equal('testytest22@gmail.com');
-  });
+    it('should throw error on trying to login non-existing user', async () => {
+      const { body } = await session.testAgent.post('/v1/auth/login').send({
+        email: 'nonExistingUser@email.com',
+        password: '123123213123',
+      });
 
-  it('should throw error on trying to login non-existing user', async () => {
-    const { body } = await session.testAgent.post('/v1/auth/login').send({
-      email: 'nonExistingUser@email.com',
-      password: '123123213123',
+      expect(body.statusCode).to.equal(401);
+      expect(body.message).to.contain('Incorrect email or password provided.');
     });
 
-    expect(body.statusCode).to.equal(401);
-    expect(body.message).to.contain('Incorrect email or password provided.');
-  });
+    it('should fail on bad password', async () => {
+      const { body } = await session.testAgent.post('/v1/auth/login').send({
+        email: userCredentials.email,
+        password: '123123213123',
+      });
 
-  it('should fail on bad password', async () => {
-    const { body } = await session.testAgent.post('/v1/auth/login').send({
-      email: userCredentials.email,
-      password: '123123213123',
+      expect(body.statusCode).to.equal(401);
+      expect(body.message).to.contain('Incorrect email or password provided.');
     });
 
-    expect(body.statusCode).to.equal(401);
-    expect(body.message).to.contain('Incorrect email or password provided.');
-  });
+    it('should allow user to log in and reset the failed attempts counter after less than 5 failed attempts within 5 minutes', async () => {
+      const SAFE_FAILED_LOGIN_ATTEMPTS = 3;
 
-  it('should allow user to log in and reset the failed attempts counter after less than 5 failed attempts within 5 minutes', async () => {
-    const SAFE_FAILED_LOGIN_ATTEMPTS = 3;
+      for (let i = 0; i < SAFE_FAILED_LOGIN_ATTEMPTS; i++) {
+        await session.testAgent.post('/v1/auth/login').send({
+          email: userCredentials.email,
+          password: 'wrong-password',
+        });
+      }
 
-    for (let i = 0; i < SAFE_FAILED_LOGIN_ATTEMPTS; i++) {
-      await session.testAgent.post('/v1/auth/login').send({
+      const { body } = await session.testAgent.post('/v1/auth/login').send({
         email: userCredentials.email,
-        password: 'wrong-password',
+        password: userCredentials.password,
       });
-    }
 
-    const { body } = await session.testAgent.post('/v1/auth/login').send({
-      email: userCredentials.email,
-      password: userCredentials.password,
-    });
+      const jwtContent = (await jwt.decode(body.data.token)) as IJwtPayload;
 
-    const jwtContent = (await jwt.decode(body.data.token)) as IJwtPayload;
+      expect(jwtContent.firstName).to.equal('test');
+      expect(jwtContent.lastName).to.equal('user');
+      expect(jwtContent.email).to.equal('testytest22@gmail.com');
 
-    expect(jwtContent.firstName).to.equal('test');
-    expect(jwtContent.lastName).to.equal('user');
-    expect(jwtContent.email).to.equal('testytest22@gmail.com');
+      const { body: wrongCredsBody } = await session.testAgent.post('/v1/auth/login').send({
+        email: userCredentials.email,
+        password: 'wrong-password',
+      });
 
-    const { body: wrongCredsBody } = await session.testAgent.post('/v1/auth/login').send({
-      email: userCredentials.email,
-      password: 'wrong-password',
+      expect(wrongCredsBody.statusCode).to.equal(401);
+      expect(wrongCredsBody.message).to.contain('Incorrect email or password provided.');
     });
 
-    expect(wrongCredsBody.statusCode).to.equal(401);
-    expect(wrongCredsBody.message).to.contain('Incorrect email or password provided.');
-  });
+    it('should block the user account after 5 unsuccessful attempts within 5 minutes', async () => {
+      const MAX_LOGIN_ATTEMPTS = 5;
 
-  it('should block the user account after 5 unsuccessful attempts within 5 minutes', async () => {
-    const MAX_LOGIN_ATTEMPTS = 5;
+      for (let i = 0; i < MAX_LOGIN_ATTEMPTS; i++) {
+        await session.testAgent.post('/v1/auth/login').send({
+          email: userCredentials.email,
+          password: 'wrong-password',
+        });
+      }
 
-    for (let i = 0; i < MAX_LOGIN_ATTEMPTS; i++) {
-      await session.testAgent.post('/v1/auth/login').send({
+      const { body } = await session.testAgent.post('/v1/auth/login').send({
         email: userCredentials.email,
-        password: 'wrong-password',
+        password: userCredentials.password,
       });
-    }
 
-    const { body } = await session.testAgent.post('/v1/auth/login').send({
-      email: userCredentials.email,
-      password: userCredentials.password,
+      expect(body.statusCode).to.equal(401);
+      expect(body.message).to.contain('Account blocked');
     });
 
-    expect(body.statusCode).to.equal(401);
-    expect(body.message).to.contain('Account blocked');
-  });
-
-  it('should reset the account blocked error after 5 minutes and allow for more 5 failed attempts', async () => {
-    const MAX_LOGIN_ATTEMPTS = 5;
-    const BLOCKED_PERIOD_IN_MINUTES = 5;
+    it('should reset the account blocked error after 5 minutes and allow for more 5 failed attempts', async () => {
+      const MAX_LOGIN_ATTEMPTS = 5;
+      const BLOCKED_PERIOD_IN_MINUTES = 5;
 
-    const lastFailedAttempt = subMinutes(new Date(), BLOCKED_PERIOD_IN_MINUTES);
+      const lastFailedAttempt = subMinutes(new Date(), BLOCKED_PERIOD_IN_MINUTES);
 
-    const failedLogin = {
-      lastFailedAttempt: lastFailedAttempt.toISOString(),
-      times: MAX_LOGIN_ATTEMPTS,
-    };
+      const failedLogin = {
+        lastFailedAttempt: lastFailedAttempt.toISOString(),
+        times: MAX_LOGIN_ATTEMPTS,
+      };
 
-    await userRepository.update(
-      {
-        _id: session.user._id,
-      },
-      {
-        $set: {
-          failedLogin,
+      await userRepository.update(
+        {
+          _id: session.user._id,
         },
+        {
+          $set: {
+            failedLogin,
+          },
+        }
+      );
+
+      for (let i = 0; i < MAX_LOGIN_ATTEMPTS - 1; i++) {
+        const { body } = await session.testAgent.post('/v1/auth/login').send({
+          email: session.user.email,
+          password: 'wrong-password',
+        });
+
+        expect(body.message).to.contain('Incorrect email or password provided.');
+        expect(body.statusCode).to.equal(401);
       }
-    );
 
-    for (let i = 0; i < MAX_LOGIN_ATTEMPTS - 1; i++) {
       const { body } = await session.testAgent.post('/v1/auth/login').send({
-        email: session.user.email,
-        password: 'wrong-password',
+        email: userCredentials.email,
+        password: userCredentials.password,
       });
 
-      expect(body.message).to.contain('Incorrect email or password provided.');
       expect(body.statusCode).to.equal(401);
-    }
+      expect(body.message).to.contain('Account blocked');
+    });
+  });
 
-    const { body } = await session.testAgent.post('/v1/auth/login').send({
-      email: userCredentials.email,
-      password: userCredentials.password,
+  context('with OAuth', async () => {
+    const userEmail = 'testoauth@gmail.com';
+
+    before(async () => {
+      // Create a mock OAuth user without a password
+      await userRepository.create({
+        email: userEmail,
+        firstName: 'Testy',
+        lastName: 'Oauth',
+      });
     });
 
-    expect(body.statusCode).to.equal(401);
-    expect(body.message).to.contain('Account blocked');
+    it('should throw an error informing the user to use OAuth instead', async () => {
+      const { body } = await session.testAgent.post('/v1/auth/login').send({
+        email: userEmail,
+        password: 'whatever',
+      });
+
+      expect(body.statusCode).to.equal(400);
+      expect(body.message).to.contain('Please sign in using Github.');
+    });
   });
 });
diff --git a/apps/api/src/app/auth/usecases/login/login.usecase.ts b/apps/api/src/app/auth/usecases/login/login.usecase.ts
@@ -41,7 +41,8 @@ export class Login {
       throw new UnauthorizedException(`Account blocked, Please try again after ${blockedMinutesLeft} minutes`);
     }
 
-    if (!user.password) throw new ApiException('OAuth user login attempt');
+    // TODO: Trigger a password reset flow automatically for existing OAuth users instead of throwing an error
+    if (!user.password) throw new ApiException('Please sign in using Github.');
 
     const isMatching = await bcrypt.compare(command.password, user.password);
     if (!isMatching) {

diff --git a/apps/web/cypress/tests/auth.spec.ts b/apps/web/cypress/tests/auth.spec.ts
@@ -162,7 +162,7 @@ describe('User Sign-up and Login', function () {
       cy.getByTestId('email').type('test-user-1@example.com');
       cy.getByTestId('password').type('123456');
       cy.getByTestId('submit-btn').click();
-      cy.getByTestId('error-alert-banner').contains('Incorrect email or password provided');
+      cy.get('.mantine-TextInput-error').contains('Incorrect email or password provided');
     });
 
     it('should show invalid email error when authenticating with invalid email', function () {
@@ -180,7 +180,7 @@ describe('User Sign-up and Login', function () {
       cy.getByTestId('email').type('test-user-1@example.de');
       cy.getByTestId('password').type('123456');
       cy.getByTestId('submit-btn').click();
-      cy.getByTestId('error-alert-banner').contains('Incorrect email or password provided');
+      cy.get('.mantine-TextInput-error').contains('Incorrect email or password provided');
     });
   });
 

diff --git a/apps/web/src/pages/auth/components/LoginForm.tsx b/apps/web/src/pages/auth/components/LoginForm.tsx
@@ -13,6 +13,7 @@ import { useVercelParams } from '../../../hooks';
 import { useAcceptInvite } from './useAcceptInvite';
 import { ROUTES } from '../../../constants/routes.enum';
 import { OAuth } from './OAuth';
+import { parseServerErrorMessage } from '../../../utils/errors';
 
 type LoginFormProps = {
   invitationToken?: string;
@@ -78,29 +79,28 @@ export function LoginForm({ email, invitationToken }: LoginFormProps) {
     }
   };
 
-  const serverErrorString = useMemo<string>(() => {
-    return Array.isArray(error?.message) ? error?.message[0] : error?.message;
-  }, [error]);
+  const emailClientError = errors.email?.message;
+  let emailServerError = parseServerErrorMessage(error);
 
-  const emailServerError = useMemo<string>(() => {
-    if (serverErrorString === 'email must be an email') return 'Please provide a valid email';
-
-    return '';
-  }, [serverErrorString]);
+  // TODO: Use a more human-friendly message in the IsEmail validator and remove this patch
+  if (emailServerError === 'email must be an email') {
+    emailServerError = 'Please provide a valid email address';
+  }
 
   return (
     <>
       <OAuth />
       <form noValidate onSubmit={handleSubmit(onLogin)}>
         <Input
-          error={errors.email?.message || emailServerError}
+          error={emailClientError || emailServerError}
           {...register('email', {
-            required: 'Please provide an email',
-            pattern: { value: /^\S+@\S+\.\S+$/, message: 'Please provide a valid email' },
+            required: 'Please provide an email address',
+            pattern: { value: /^\S+@\S+\.\S+$/, message: 'Please provide a valid email address' },
           })}
           required
           label="Email"
-          placeholder="Type your email..."
+          type="email"
+          placeholder="Type your email address..."
           disabled={!!invitationToken}
           data-test-id="email"
           mt={5}
@@ -142,12 +142,6 @@ export function LoginForm({ email, invitationToken }: LoginFormProps) {
           </Link>
         </Center>
       </form>
-      {isError && !emailServerError && (
-        <Text data-test-id="error-alert-banner" mt={20} size="lg" weight="bold" align="center" color={colors.error}>
-          {' '}
-          {error?.message}
-        </Text>
-      )}
     </>
   );
 }
diff --git a/apps/web/src/utils/errors.ts b/apps/web/src/utils/errors.ts
@@ -0,0 +1,9 @@
+import type { IResponseError } from '@novu/shared';
+
+export function parseServerErrorMessage(error: IResponseError | null): String {
+  if (!error) {
+    return '';
+  }
+
+  return Array.isArray(error?.message) ? error?.message[0] : error?.message;
+}