Feat/migrate password reset token to app token table (#5051)

# This PR - Fix #5021 - Migrates `passwordResetToken` and `passwordResetTokenExpiresAt` fields from `core.users` to `core.appToken` - Marks those fields as `deprecated` so we can remove them later if we are happy with the transition -- I took this decision on my own, @FellipeMTX let me know what you think about it, we can also remove them straight away if you think it's better - Fixed the `database:migration` script from the `twenty-server` to: ```json "database:migrate": { "executor": "nx:run-commands", "dependsOn": ["build"], // added this line "options": { "cwd": "packages/twenty-server", "commands": [ "nx typeorm -- migration:run -d src/database/typeorm/metadata/metadata.datasource", "nx typeorm -- migration:run -d src/database/typeorm/core/core.datasource" ], "parallel": false } }, ``` The migration script wasn't running because the builds were not executed - [x] Added unit tests for the token.service file's changes Looking forward to hearing feedback from you cc: @charlesBochet --------- Co-authored-by: Weiko <corentin@twenty.com>
2024-08-16 09:20:30 +03:00 · 2024-05-06 15:30:03 +02:00 · 2024-05-06 15:30:03 +02:00 · ff77a4ee21
commit ff77a4ee21
parent b207d10312
6 changed files with 248 additions and 40 deletions
--- a/packages/twenty-front/src/generated/graphql.tsx
+++ b/packages/twenty-front/src/generated/graphql.tsx
@ -751,7 +751,9 @@ export type User = {
  id: Scalars['UUID'];
  lastName: Scalars['String'];
  passwordHash?: Maybe<Scalars['String']>;
+  /** @deprecated field migrated into the AppTokens Table ref: https://github.com/twentyhq/twenty/issues/5021 */
  passwordResetToken?: Maybe<Scalars['String']>;
+  /** @deprecated field migrated into the AppTokens Table ref: https://github.com/twentyhq/twenty/issues/5021 */
  passwordResetTokenExpiresAt?: Maybe<Scalars['DateTime']>;
  supportUserHash?: Maybe<Scalars['String']>;
  updatedAt: Scalars['DateTime'];
--- a/packages/twenty-server/project.json
+++ b/packages/twenty-server/project.json
@ -118,6 +118,7 @@
    },
    "database:migrate": {
      "executor": "nx:run-commands",
+      "dependsOn": ["build"],
      "options": {
        "cwd": "packages/twenty-server",
        "commands": [
--- a/packages/twenty-server/src/engine/core-modules/app-token/app-token.entity.ts
+++ b/packages/twenty-server/src/engine/core-modules/app-token/app-token.entity.ts
@ -20,6 +20,7 @@ export enum AppTokenType {
  RefreshToken = 'REFRESH_TOKEN',
  CodeChallenge = 'CODE_CHALLENGE',
  AuthorizationCode = 'AUTHORIZATION_CODE',
+  PasswordResetToken = 'PASSWORD_RESET_TOKEN',
 }

@Entity({ name: 'appToken', schema: 'core' })
--- a/packages/twenty-server/src/engine/core-modules/auth/services/token.service.spec.ts
+++ b/packages/twenty-server/src/engine/core-modules/auth/services/token.service.spec.ts
@ -1,9 +1,21 @@
 import { Test, TestingModule } from '@nestjs/testing';
 import { JwtService } from '@nestjs/jwt';
 import { getRepositoryToken } from '@nestjs/typeorm';
+import {
+  BadRequestException,
+  InternalServerErrorException,
+  NotFoundException,
+} from '@nestjs/common';
+
+import crypto from 'crypto';
+
+import { IsNull, MoreThan, Repository } from 'typeorm';

 import { EnvironmentService } from 'src/engine/integrations/environment/environment.service';
-import { AppToken } from 'src/engine/core-modules/app-token/app-token.entity';
+import {
+  AppToken,
+  AppTokenType,
+} from 'src/engine/core-modules/app-token/app-token.entity';
 import { User } from 'src/engine/core-modules/user/user.entity';
 import { JwtAuthStrategy } from 'src/engine/core-modules/auth/strategies/jwt.auth.strategy';
 import { EmailService } from 'src/engine/integrations/email/email.service';
@ -13,6 +25,9 @@ import { TokenService } from './token.service';

 describe('TokenService', () => {
  let service: TokenService;
+  let environmentService: EnvironmentService;
+  let userRepository: Repository<User>;
+  let appTokenRepository: Repository<AppToken>;

  beforeEach(async () => {
    const module: TestingModule = await Test.createTestingModule({
@ -20,7 +35,9 @@ describe('TokenService', () => {
        TokenService,
        {
          provide: JwtService,
-          useValue: {},
+          useValue: {
+            sign: jest.fn().mockReturnValue('mock-jwt-token'),
+          },
        },
        {
          provide: JwtAuthStrategy,
@ -28,19 +45,28 @@ describe('TokenService', () => {
        },
        {
          provide: EnvironmentService,
-          useValue: {},
+          useValue: {
+            get: jest.fn().mockReturnValue('some-value'),
+          },
        },
        {
          provide: EmailService,
-          useValue: {},
+          useValue: {
+            send: jest.fn(),
+          },
        },
        {
          provide: getRepositoryToken(User, 'core'),
-          useValue: {},
+          useValue: {
+            findOneBy: jest.fn(),
+          },
        },
        {
          provide: getRepositoryToken(AppToken, 'core'),
-          useValue: {},
+          useValue: {
+            findOne: jest.fn(),
+            save: jest.fn(),
+          },
        },
        {
          provide: getRepositoryToken(Workspace, 'core'),
@ -50,9 +76,167 @@ describe('TokenService', () => {
    }).compile();

    service = module.get<TokenService>(TokenService);
+    environmentService = module.get<EnvironmentService>(EnvironmentService);
+    userRepository = module.get(getRepositoryToken(User, 'core'));
+    appTokenRepository = module.get(getRepositoryToken(AppToken, 'core'));
  });

  it('should be defined', () => {
    expect(service).toBeDefined();
  });
+
+  describe('generatePasswordResetToken', () => {
+    it('should generate a new password reset token when no existing token is found', async () => {
+      const mockUser = { id: '1', email: 'test@example.com' } as User;
+      const expiresIn = '3600000'; // 1 hour in ms
+
+      jest.spyOn(userRepository, 'findOneBy').mockResolvedValue(mockUser);
+      jest.spyOn(appTokenRepository, 'findOne').mockResolvedValue(null);
+      jest.spyOn(environmentService, 'get').mockReturnValue(expiresIn);
+      jest
+        .spyOn(appTokenRepository, 'save')
+        .mockImplementation(async (token) => token as AppToken);
+
+      const result = await service.generatePasswordResetToken(mockUser.email);
+
+      expect(userRepository.findOneBy).toHaveBeenCalledWith({
+        email: mockUser.email,
+      });
+      expect(appTokenRepository.findOne).toHaveBeenCalled();
+      expect(appTokenRepository.save).toHaveBeenCalled();
+      expect(result.passwordResetToken).toBeDefined();
+      expect(result.passwordResetTokenExpiresAt).toBeDefined();
+    });
+
+    it('should throw BadRequestException if an existing valid token is found', async () => {
+      const mockUser = { id: '1', email: 'test@example.com' } as User;
+      const mockToken = {
+        userId: '1',
+        type: AppTokenType.PasswordResetToken,
+        expiresAt: new Date(Date.now() + 10000), // expires 10 seconds in the future
+      } as AppToken;
+
+      jest.spyOn(userRepository, 'findOneBy').mockResolvedValue(mockUser);
+      jest.spyOn(appTokenRepository, 'findOne').mockResolvedValue(mockToken);
+      jest.spyOn(environmentService, 'get').mockReturnValue('3600000');
+
+      await expect(
+        service.generatePasswordResetToken(mockUser.email),
+      ).rejects.toThrow(BadRequestException);
+    });
+
+    it('should throw NotFoundException if no user is found', async () => {
+      jest.spyOn(userRepository, 'findOneBy').mockResolvedValue(null);
+
+      await expect(
+        service.generatePasswordResetToken('nonexistent@example.com'),
+      ).rejects.toThrow(NotFoundException);
+    });
+
+    it('should throw InternalServerErrorException if environment variable is not found', async () => {
+      const mockUser = { id: '1', email: 'test@example.com' } as User;
+
+      jest.spyOn(userRepository, 'findOneBy').mockResolvedValue(mockUser);
+      jest.spyOn(environmentService, 'get').mockReturnValue(''); // No environment variable set
+
+      await expect(
+        service.generatePasswordResetToken(mockUser.email),
+      ).rejects.toThrow(InternalServerErrorException);
+    });
+  });
+
+  describe('validatePasswordResetToken', () => {
+    it('should return user data for a valid and active token', async () => {
+      const resetToken = 'valid-reset-token';
+      const hashedToken = crypto
+        .createHash('sha256')
+        .update(resetToken)
+        .digest('hex');
+      const mockToken = {
+        userId: '1',
+        value: hashedToken,
+        type: AppTokenType.PasswordResetToken,
+        expiresAt: new Date(Date.now() + 10000), // Valid future date
+      };
+      const mockUser = { id: '1', email: 'user@example.com' };
+
+      jest
+        .spyOn(appTokenRepository, 'findOne')
+        .mockResolvedValue(mockToken as AppToken);
+      jest
+        .spyOn(userRepository, 'findOneBy')
+        .mockResolvedValue(mockUser as User);
+
+      const result = await service.validatePasswordResetToken(resetToken);
+
+      expect(appTokenRepository.findOne).toHaveBeenCalledWith({
+        where: {
+          value: hashedToken,
+          type: AppTokenType.PasswordResetToken,
+          expiresAt: MoreThan(new Date()),
+          revokedAt: IsNull(),
+        },
+      });
+      expect(userRepository.findOneBy).toHaveBeenCalledWith({
+        id: mockToken.userId,
+      });
+      expect(result).toEqual({ id: mockUser.id, email: mockUser.email });
+    });
+
+    it('should throw NotFoundException if token is invalid or expired', async () => {
+      const resetToken = 'invalid-reset-token';
+
+      jest.spyOn(appTokenRepository, 'findOne').mockResolvedValue(null);
+
+      await expect(
+        service.validatePasswordResetToken(resetToken),
+      ).rejects.toThrow(NotFoundException);
+    });
+
+    it('should throw NotFoundException if user does not exist for a valid token', async () => {
+      const resetToken = 'orphan-token';
+      const hashedToken = crypto
+        .createHash('sha256')
+        .update(resetToken)
+        .digest('hex');
+      const mockToken = {
+        userId: 'nonexistent-user',
+        value: hashedToken,
+        type: AppTokenType.PasswordResetToken,
+        expiresAt: new Date(Date.now() + 10000), // Valid future date
+        revokedAt: null,
+      };
+
+      jest
+        .spyOn(appTokenRepository, 'findOne')
+        .mockResolvedValue(mockToken as AppToken);
+      jest.spyOn(userRepository, 'findOneBy').mockResolvedValue(null);
+
+      await expect(
+        service.validatePasswordResetToken(resetToken),
+      ).rejects.toThrow(NotFoundException);
+    });
+
+    it('should throw NotFoundException if token is revoked', async () => {
+      const resetToken = 'revoked-token';
+      const hashedToken = crypto
+        .createHash('sha256')
+        .update(resetToken)
+        .digest('hex');
+      const mockToken = {
+        userId: '1',
+        value: hashedToken,
+        type: AppTokenType.PasswordResetToken,
+        expiresAt: new Date(Date.now() + 10000),
+        revokedAt: new Date(),
+      };
+
+      jest
+        .spyOn(appTokenRepository, 'findOne')
+        .mockResolvedValue(mockToken as AppToken);
+      await expect(
+        service.validatePasswordResetToken(resetToken),
+      ).rejects.toThrow(NotFoundException);
+    });
+  });
 });
--- a/packages/twenty-server/src/engine/core-modules/auth/services/token.service.ts
+++ b/packages/twenty-server/src/engine/core-modules/auth/services/token.service.ts
@ -12,10 +12,10 @@ import { InjectRepository } from '@nestjs/typeorm';

 import crypto from 'crypto';

-import { addMilliseconds, differenceInMilliseconds, isFuture } from 'date-fns';
+import { addMilliseconds, differenceInMilliseconds } from 'date-fns';
 import ms from 'ms';
 import { JsonWebTokenError, TokenExpiredError } from 'jsonwebtoken';
-import { Repository } from 'typeorm';
+import { IsNull, MoreThan, Repository } from 'typeorm';
 import { Request } from 'express';
 import { ExtractJwt } from 'passport-jwt';
 import { render } from '@react-email/render';
@ -520,22 +520,24 @@ export class TokenService {
      InternalServerErrorException,
    );

-    if (
-      user.passwordResetToken &&
-      user.passwordResetTokenExpiresAt &&
-      isFuture(user.passwordResetTokenExpiresAt)
-    ) {
+    const existingToken = await this.appTokenRepository.findOne({
+      where: {
+        userId: user.id,
+        type: AppTokenType.PasswordResetToken,
+        expiresAt: MoreThan(new Date()),
+        revokedAt: IsNull(),
+      },
+    });
+
+    if (existingToken) {
+      const timeToWait = ms(
+        differenceInMilliseconds(existingToken.expiresAt, new Date()),
+        { long: true },
+      );
+
      assert(
        false,
-        `Token has been already generated. Please wait for ${ms(
-          differenceInMilliseconds(
-            user.passwordResetTokenExpiresAt,
-            new Date(),
-          ),
-          {
-            long: true,
-          },
-        )} to generate again.`,
+        `Token has already been generated. Please wait for ${timeToWait} to generate again.`,
        BadRequestException,
      );
    }
@ -548,9 +550,11 @@ export class TokenService {

    const expiresAt = addMilliseconds(new Date().getTime(), ms(expiresIn));

-    await this.userRepository.update(user.id, {
-      passwordResetToken: hashedResetToken,
-      passwordResetTokenExpiresAt: expiresAt,
+    await this.appTokenRepository.save({
+      userId: user.id,
+      value: hashedResetToken,
+      expiresAt,
+      type: AppTokenType.PasswordResetToken,
    });

    return {
@ -615,20 +619,23 @@ export class TokenService {
      .update(resetToken)
      .digest('hex');

+    const token = await this.appTokenRepository.findOne({
+      where: {
+        value: hashedResetToken,
+        type: AppTokenType.PasswordResetToken,
+        expiresAt: MoreThan(new Date()),
+        revokedAt: IsNull(),
+      },
+    });
+
+    assert(token, 'Token is invalid', NotFoundException);
+
    const user = await this.userRepository.findOneBy({
-      passwordResetToken: hashedResetToken,
+      id: token.userId,
    });

    assert(user, 'Token is invalid', NotFoundException);

-    const tokenExpiresAt = user.passwordResetTokenExpiresAt;
-
-    assert(
-      tokenExpiresAt && isFuture(tokenExpiresAt),
-      'Token has expired. Please regenerate',
-      NotFoundException,
-    );
-
    return {
      id: user.id,
      email: user.email,
@ -644,10 +651,15 @@ export class TokenService {

    assert(user, 'User not found', NotFoundException);

-    await this.userRepository.update(user.id, {
-      passwordResetToken: '',
-      passwordResetTokenExpiresAt: undefined,
-    });
+    await this.appTokenRepository.update(
+      {
+        userId,
+        type: AppTokenType.PasswordResetToken,
+      },
+      {
+        revokedAt: new Date(),
+      },
+    );

    return { success: true };
  }
--- a/packages/twenty-server/src/engine/core-modules/user/user.entity.ts
+++ b/packages/twenty-server/src/engine/core-modules/user/user.entity.ts
@ -79,11 +79,19 @@ export class User {
  @Column()
  defaultWorkspaceId: string;

-  @Field({ nullable: true })
+  @Field({
+    nullable: true,
+    deprecationReason:
+      'field migrated into the AppTokens Table ref: https://github.com/twentyhq/twenty/issues/5021',
+  })
  @Column({ nullable: true })
  passwordResetToken: string;

-  @Field({ nullable: true })
+  @Field({
+    nullable: true,
+    deprecationReason:
+      'field migrated into the AppTokens Table ref: https://github.com/twentyhq/twenty/issues/5021',
+  })
  @Column({ nullable: true, type: 'timestamptz' })
  passwordResetTokenExpiresAt: Date;