From 89f44021b919181ad58c555a31071c286b866975 Mon Sep 17 00:00:00 2001 From: Ricardo Espinoza Date: Mon, 24 Jul 2023 17:40:17 -0400 Subject: [PATCH] fix(core): Use JWT as reset password token (#6714) * use jwt to reset password * increase expiration time to 1d * drop user id query string * refactor * use service instead of package in tests * sqlite migration * postgres migration * mysql migration * remove unused properties * remove userId from FE * fix test for users.api * move migration to the common folder * move type assertion to the jwt.service * Add jwt secret as a readonly property * use signData instead of sign in user.controller * remove base class * remove base class * add tests --- packages/cli/src/Server.ts | 4 + .../UserManagement/UserManagementHelper.ts | 10 +-- packages/cli/src/auth/jwt.ts | 1 - .../controllers/passwordReset.controller.ts | 85 ++++++++++++------- .../cli/src/controllers/users.controller.ts | 18 +++- packages/cli/src/databases/entities/User.ts | 7 -- ...690000000030-RemoveResetPasswordColumns.ts | 29 +++++++ .../src/databases/migrations/mysqldb/index.ts | 2 + .../databases/migrations/postgresdb/index.ts | 2 + .../src/databases/migrations/sqlite/index.ts | 2 + packages/cli/src/services/jwt.service.ts | 18 ++++ packages/cli/src/user/user.service.ts | 15 +--- .../integration/passwordReset.api.test.ts | 62 +++----------- .../integration/shared/utils/testServer.ts | 4 + .../cli/test/integration/users.api.test.ts | 8 +- .../test/unit/services/jwt.service.test.ts | 42 +++++++++ packages/editor-ui/src/api/users.ts | 4 +- packages/editor-ui/src/stores/users.store.ts | 8 +- .../src/views/ChangePasswordView.vue | 34 +++----- 19 files changed, 209 insertions(+), 146 deletions(-) create mode 100644 packages/cli/src/databases/migrations/common/1690000000030-RemoveResetPasswordColumns.ts create mode 100644 packages/cli/src/services/jwt.service.ts create mode 100644 packages/cli/test/unit/services/jwt.service.test.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index ac82739c05..43533141ae 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -169,6 +169,7 @@ import { SourceControlService } from '@/environments/sourceControl/sourceControl import { SourceControlController } from '@/environments/sourceControl/sourceControl.controller.ee'; import { ExecutionRepository } from '@db/repositories'; import type { ExecutionEntity } from '@db/entities/ExecutionEntity'; +import { JwtService } from './services/jwt.service'; const exec = promisify(callbackExec); @@ -463,6 +464,7 @@ export class Server extends AbstractServer { const internalHooks = Container.get(InternalHooks); const mailer = Container.get(UserManagementMailer); const postHog = this.postHog; + const jwtService = Container.get(JwtService); const controllers: object[] = [ new EventBusController(), @@ -477,6 +479,7 @@ export class Server extends AbstractServer { mailer, repositories, logger, + jwtService, }), new TagsController({ config, repositories, externalHooks }), new TranslationController(config, this.credentialTypes), @@ -489,6 +492,7 @@ export class Server extends AbstractServer { activeWorkflowRunner, logger, postHog, + jwtService, }), Container.get(SamlController), Container.get(SourceControlController), diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/packages/cli/src/UserManagement/UserManagementHelper.ts index 1f5a3c09aa..6b23f1207c 100644 --- a/packages/cli/src/UserManagement/UserManagementHelper.ts +++ b/packages/cli/src/UserManagement/UserManagementHelper.ts @@ -111,15 +111,7 @@ export function validatePassword(password?: string): string { * Remove sensitive properties from the user to return to the client. */ export function sanitizeUser(user: User, withoutKeys?: string[]): PublicUser { - const { - password, - resetPasswordToken, - resetPasswordTokenExpiration, - updatedAt, - apiKey, - authIdentities, - ...rest - } = user; + const { password, updatedAt, apiKey, authIdentities, ...rest } = user; if (withoutKeys) { withoutKeys.forEach((key) => { // @ts-ignore diff --git a/packages/cli/src/auth/jwt.ts b/packages/cli/src/auth/jwt.ts index 1fb305cfe6..3c80c1d865 100644 --- a/packages/cli/src/auth/jwt.ts +++ b/packages/cli/src/auth/jwt.ts @@ -38,7 +38,6 @@ export function issueJWT(user: User): JwtToken { const signedToken = jwt.sign(payload, config.getEnv('userManagement.jwtSecret'), { expiresIn: expiresIn / 1000 /* in seconds */, - algorithm: 'HS256', }); return { diff --git a/packages/cli/src/controllers/passwordReset.controller.ts b/packages/cli/src/controllers/passwordReset.controller.ts index afb4bf69a5..16aef76648 100644 --- a/packages/cli/src/controllers/passwordReset.controller.ts +++ b/packages/cli/src/controllers/passwordReset.controller.ts @@ -1,4 +1,4 @@ -import { IsNull, MoreThanOrEqual, Not } from 'typeorm'; +import { IsNull, Not } from 'typeorm'; import validator from 'validator'; import { Get, Post, RestController } from '@/decorators'; import { @@ -28,6 +28,8 @@ import { UserService } from '@/user/user.service'; import { License } from '@/License'; import { Container } from 'typedi'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; +import { TokenExpiredError } from 'jsonwebtoken'; +import type { JwtService, JwtPayload } from '@/services/jwt.service'; @RestController() export class PasswordResetController { @@ -43,6 +45,8 @@ export class PasswordResetController { private readonly userRepository: UserRepository; + private readonly jwtService: JwtService; + constructor({ config, logger, @@ -50,6 +54,7 @@ export class PasswordResetController { internalHooks, mailer, repositories, + jwtService, }: { config: Config; logger: ILogger; @@ -57,6 +62,7 @@ export class PasswordResetController { internalHooks: IInternalHooksClass; mailer: UserManagementMailer; repositories: Pick; + jwtService: JwtService; }) { this.config = config; this.logger = logger; @@ -64,6 +70,7 @@ export class PasswordResetController { this.internalHooks = internalHooks; this.mailer = mailer; this.userRepository = repositories.User; + this.jwtService = jwtService; } /** @@ -139,7 +146,15 @@ export class PasswordResetController { const baseUrl = getInstanceBaseUrl(); const { id, firstName, lastName } = user; - const url = await UserService.generatePasswordResetUrl(user); + + const resetPasswordToken = this.jwtService.signData( + { sub: id }, + { + expiresIn: '1d', + }, + ); + + const url = await UserService.generatePasswordResetUrl(baseUrl, resetPasswordToken); try { await this.mailer.passwordReset({ @@ -175,11 +190,11 @@ export class PasswordResetController { */ @Get('/resolve-password-token') async resolvePasswordToken(req: PasswordResetRequest.Credentials) { - const { token: resetPasswordToken, userId: id } = req.query; + const { token: resetPasswordToken } = req.query; - if (!resetPasswordToken || !id) { + if (!resetPasswordToken) { this.logger.debug( - 'Request to resolve password token failed because of missing password reset token or user ID in query string', + 'Request to resolve password token failed because of missing password reset token', { queryString: req.query, }, @@ -187,47 +202,46 @@ export class PasswordResetController { throw new BadRequestError(''); } - // Timestamp is saved in seconds - const currentTimestamp = Math.floor(Date.now() / 1000); + const decodedToken = this.verifyResetPasswordToken(resetPasswordToken); const user = await this.userRepository.findOne({ where: { - id, - resetPasswordToken, - resetPasswordTokenExpiration: MoreThanOrEqual(currentTimestamp), + id: decodedToken.sub, }, relations: ['globalRole'], }); + if (!user?.isOwner && !Container.get(License).isWithinUsersLimit()) { this.logger.debug( 'Request to resolve password token failed because the user limit was reached', - { userId: id }, + { userId: decodedToken.sub }, ); throw new UnauthorizedError(RESPONSE_ERROR_MESSAGES.USERS_QUOTA_REACHED); } + if (!user) { this.logger.debug( - 'Request to resolve password token failed because no user was found for the provided user ID and reset password token', + 'Request to resolve password token failed because no user was found for the provided user ID', { - userId: id, + userId: decodedToken.sub, resetPasswordToken, }, ); throw new NotFoundError(''); } - this.logger.info('Reset-password token resolved successfully', { userId: id }); + this.logger.info('Reset-password token resolved successfully', { userId: user.id }); void this.internalHooks.onUserPasswordResetEmailClick({ user }); } /** - * Verify password reset token and user ID and update password. + * Verify password reset token and update password. */ @Post('/change-password') async changePassword(req: PasswordResetRequest.NewPassword, res: Response) { - const { token: resetPasswordToken, userId, password } = req.body; + const { token: resetPasswordToken, password } = req.body; - if (!resetPasswordToken || !userId || !password) { + if (!resetPasswordToken || !password) { this.logger.debug( 'Request to change password failed because of missing user ID or password or reset password token in payload', { @@ -239,23 +253,17 @@ export class PasswordResetController { const validPassword = validatePassword(password); - // Timestamp is saved in seconds - const currentTimestamp = Math.floor(Date.now() / 1000); + const decodedToken = this.verifyResetPasswordToken(resetPasswordToken); const user = await this.userRepository.findOne({ - where: { - id: userId, - resetPasswordToken, - resetPasswordTokenExpiration: MoreThanOrEqual(currentTimestamp), - }, + where: { id: decodedToken.sub }, relations: ['authIdentities'], }); if (!user) { this.logger.debug( - 'Request to resolve password token failed because no user was found for the provided user ID and reset password token', + 'Request to resolve password token failed because no user was found for the provided user ID', { - userId, resetPasswordToken, }, ); @@ -264,13 +272,11 @@ export class PasswordResetController { const passwordHash = await hashPassword(validPassword); - await this.userRepository.update(userId, { + await this.userRepository.update(user.id, { password: passwordHash, - resetPasswordToken: null, - resetPasswordTokenExpiration: null, }); - this.logger.info('User password updated successfully', { userId }); + this.logger.info('User password updated successfully', { userId: user.id }); await issueCookie(res, user); @@ -290,4 +296,23 @@ export class PasswordResetController { await this.externalHooks.run('user.password.update', [user.email, passwordHash]); } + + private verifyResetPasswordToken(resetPasswordToken: string) { + let decodedToken: JwtPayload; + try { + decodedToken = this.jwtService.verifyToken(resetPasswordToken); + return decodedToken; + } catch (e) { + if (e instanceof TokenExpiredError) { + this.logger.debug('Reset password token expired', { + resetPasswordToken, + }); + throw new NotFoundError(''); + } + this.logger.debug('Error verifying token', { + resetPasswordToken, + }); + throw new BadRequestError(''); + } + } } diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index d97aa55fd4..cef9e9fa71 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -49,6 +49,7 @@ import { plainToInstance } from 'class-transformer'; import { License } from '@/License'; import { Container } from 'typedi'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; +import type { JwtService } from '@/services/jwt.service'; @Authorized(['global', 'owner']) @RestController('/users') @@ -73,6 +74,8 @@ export class UsersController { private mailer: UserManagementMailer; + private jwtService: JwtService; + private postHog?: PostHogClient; constructor({ @@ -83,6 +86,7 @@ export class UsersController { repositories, activeWorkflowRunner, mailer, + jwtService, postHog, }: { config: Config; @@ -95,6 +99,7 @@ export class UsersController { >; activeWorkflowRunner: ActiveWorkflowRunner; mailer: UserManagementMailer; + jwtService: JwtService; postHog?: PostHogClient; }) { this.config = config; @@ -107,6 +112,7 @@ export class UsersController { this.sharedWorkflowRepository = repositories.SharedWorkflow; this.activeWorkflowRunner = activeWorkflowRunner; this.mailer = mailer; + this.jwtService = jwtService; this.postHog = postHog; } @@ -382,7 +388,17 @@ export class UsersController { if (!user) { throw new NotFoundError('User not found'); } - const link = await UserService.generatePasswordResetUrl(user); + + const resetPasswordToken = this.jwtService.signData( + { sub: user.id }, + { + expiresIn: '1d', + }, + ); + + const baseUrl = getInstanceBaseUrl(); + + const link = await UserService.generatePasswordResetUrl(baseUrl, resetPasswordToken); return { link, }; diff --git a/packages/cli/src/databases/entities/User.ts b/packages/cli/src/databases/entities/User.ts index 1ee1570f8f..029275902f 100644 --- a/packages/cli/src/databases/entities/User.ts +++ b/packages/cli/src/databases/entities/User.ts @@ -55,13 +55,6 @@ export class User extends AbstractEntity implements IUser { @IsString({ message: 'Password must be of type string.' }) password: string; - @Column({ type: String, nullable: true }) - resetPasswordToken?: string | null; - - // Expiration timestamp saved in seconds - @Column({ type: Number, nullable: true }) - resetPasswordTokenExpiration?: number | null; - @Column({ type: jsonColumnType, nullable: true, diff --git a/packages/cli/src/databases/migrations/common/1690000000030-RemoveResetPasswordColumns.ts b/packages/cli/src/databases/migrations/common/1690000000030-RemoveResetPasswordColumns.ts new file mode 100644 index 0000000000..73f572c168 --- /dev/null +++ b/packages/cli/src/databases/migrations/common/1690000000030-RemoveResetPasswordColumns.ts @@ -0,0 +1,29 @@ +import type { MigrationContext, ReversibleMigration } from '@db/types'; +import { TableColumn } from 'typeorm'; + +export class RemoveResetPasswordColumns1690000000030 implements ReversibleMigration { + async up({ queryRunner, tablePrefix }: MigrationContext) { + await queryRunner.dropColumn(`${tablePrefix}user`, 'resetPasswordToken'); + await queryRunner.dropColumn(`${tablePrefix}user`, 'resetPasswordTokenExpiration'); + } + + async down({ queryRunner, tablePrefix }: MigrationContext) { + await queryRunner.addColumn( + `${tablePrefix}user`, + new TableColumn({ + name: 'resetPasswordToken', + type: 'varchar', + isNullable: true, + }), + ); + + await queryRunner.addColumn( + `${tablePrefix}user`, + new TableColumn({ + name: 'resetPasswordTokenExpiration', + type: 'int', + isNullable: true, + }), + ); + } +} diff --git a/packages/cli/src/databases/migrations/mysqldb/index.ts b/packages/cli/src/databases/migrations/mysqldb/index.ts index 18e666a145..c450adf946 100644 --- a/packages/cli/src/databases/migrations/mysqldb/index.ts +++ b/packages/cli/src/databases/migrations/mysqldb/index.ts @@ -42,6 +42,7 @@ import { MigrateIntegerKeysToString1690000000001 } from './1690000000001-Migrate import { SeparateExecutionData1690000000030 } from './1690000000030-SeparateExecutionData'; import { FixExecutionDataType1690000000031 } from './1690000000031-FixExecutionDataType'; import { RemoveSkipOwnerSetup1681134145997 } from './1681134145997-RemoveSkipOwnerSetup'; +import { RemoveResetPasswordColumns1690000000030 } from '../common/1690000000030-RemoveResetPasswordColumns'; export const mysqlMigrations: Migration[] = [ InitialMigration1588157391238, @@ -87,4 +88,5 @@ export const mysqlMigrations: Migration[] = [ SeparateExecutionData1690000000030, FixExecutionDataType1690000000031, RemoveSkipOwnerSetup1681134145997, + RemoveResetPasswordColumns1690000000030, ]; diff --git a/packages/cli/src/databases/migrations/postgresdb/index.ts b/packages/cli/src/databases/migrations/postgresdb/index.ts index 460c33213c..53066755a9 100644 --- a/packages/cli/src/databases/migrations/postgresdb/index.ts +++ b/packages/cli/src/databases/migrations/postgresdb/index.ts @@ -39,6 +39,7 @@ import { AddUserActivatedProperty1681134145996 } from './1681134145996-AddUserAc import { MigrateIntegerKeysToString1690000000000 } from './1690000000000-MigrateIntegerKeysToString'; import { SeparateExecutionData1690000000020 } from './1690000000020-SeparateExecutionData'; import { RemoveSkipOwnerSetup1681134145997 } from './1681134145997-RemoveSkipOwnerSetup'; +import { RemoveResetPasswordColumns1690000000030 } from '../common/1690000000030-RemoveResetPasswordColumns'; export const postgresMigrations: Migration[] = [ InitialMigration1587669153312, @@ -81,4 +82,5 @@ export const postgresMigrations: Migration[] = [ MigrateIntegerKeysToString1690000000000, SeparateExecutionData1690000000020, RemoveSkipOwnerSetup1681134145997, + RemoveResetPasswordColumns1690000000030, ]; diff --git a/packages/cli/src/databases/migrations/sqlite/index.ts b/packages/cli/src/databases/migrations/sqlite/index.ts index f23ce87665..b5f554107a 100644 --- a/packages/cli/src/databases/migrations/sqlite/index.ts +++ b/packages/cli/src/databases/migrations/sqlite/index.ts @@ -39,6 +39,7 @@ import { MigrateIntegerKeysToString1690000000002 } from './1690000000002-Migrate import { SeparateExecutionData1690000000010 } from './1690000000010-SeparateExecutionData'; import { RemoveSkipOwnerSetup1681134145997 } from './1681134145997-RemoveSkipOwnerSetup'; import { FixMissingIndicesFromStringIdMigration1690000000020 } from './1690000000020-FixMissingIndicesFromStringIdMigration'; +import { RemoveResetPasswordColumns1690000000030 } from '../common/1690000000030-RemoveResetPasswordColumns'; const sqliteMigrations: Migration[] = [ InitialMigration1588102412422, @@ -81,6 +82,7 @@ const sqliteMigrations: Migration[] = [ SeparateExecutionData1690000000010, RemoveSkipOwnerSetup1681134145997, FixMissingIndicesFromStringIdMigration1690000000020, + RemoveResetPasswordColumns1690000000030, ]; export { sqliteMigrations }; diff --git a/packages/cli/src/services/jwt.service.ts b/packages/cli/src/services/jwt.service.ts new file mode 100644 index 0000000000..094b595d8d --- /dev/null +++ b/packages/cli/src/services/jwt.service.ts @@ -0,0 +1,18 @@ +import { Service } from 'typedi'; +import * as jwt from 'jsonwebtoken'; +import config from '@/config'; + +@Service() +export class JwtService { + private readonly userManagementSecret = config.getEnv('userManagement.jwtSecret'); + + public signData(payload: object, options: jwt.SignOptions = {}): string { + return jwt.sign(payload, this.userManagementSecret, options); + } + + public verifyToken(token: string, options: jwt.VerifyOptions = {}) { + return jwt.verify(token, this.userManagementSecret, options) as jwt.JwtPayload; + } +} + +export type JwtPayload = jwt.JwtPayload; diff --git a/packages/cli/src/user/user.service.ts b/packages/cli/src/user/user.service.ts index 6183a97e59..cb04758e3b 100644 --- a/packages/cli/src/user/user.service.ts +++ b/packages/cli/src/user/user.service.ts @@ -1,10 +1,8 @@ import type { EntityManager, FindOptionsWhere } from 'typeorm'; import { In } from 'typeorm'; -import { v4 as uuid } from 'uuid'; import * as Db from '@/Db'; import { User } from '@db/entities/User'; import type { IUserSettings } from 'n8n-workflow'; -import { getInstanceBaseUrl } from '../UserManagement/UserManagementHelper'; export class UserService { static async get(where: FindOptionsWhere): Promise { @@ -25,16 +23,9 @@ export class UserService { return Db.collections.User.update(id, { settings: { ...currentSettings, ...userSettings } }); } - static async generatePasswordResetUrl(user: User): Promise { - user.resetPasswordToken = uuid(); - const { id, resetPasswordToken } = user; - const resetPasswordTokenExpiration = Math.floor(Date.now() / 1000) + 7200; - await Db.collections.User.update(id, { resetPasswordToken, resetPasswordTokenExpiration }); - - const baseUrl = getInstanceBaseUrl(); - const url = new URL(`${baseUrl}/change-password`); - url.searchParams.append('userId', id); - url.searchParams.append('token', resetPasswordToken); + static async generatePasswordResetUrl(instanceBaseUrl: string, token: string): Promise { + const url = new URL(`${instanceBaseUrl}/change-password`); + url.searchParams.append('token', token); return url.toString(); } } diff --git a/packages/cli/test/integration/passwordReset.api.test.ts b/packages/cli/test/integration/passwordReset.api.test.ts index 0e9e345cc6..61a7c891f5 100644 --- a/packages/cli/test/integration/passwordReset.api.test.ts +++ b/packages/cli/test/integration/passwordReset.api.test.ts @@ -10,13 +10,17 @@ import { randomEmail, randomInvalidPassword, randomName, + randomString, randomValidPassword, } from './shared/random'; import * as testDb from './shared/testDb'; import { setCurrentAuthenticationMethod } from '@/sso/ssoHelpers'; import { ExternalHooks } from '@/ExternalHooks'; +import { JwtService } from '@/services/jwt.service'; +import { Container } from 'typedi'; jest.mock('@/UserManagement/email/NodeMailer'); +config.set('userManagement.jwtSecret', randomString(5, 10)); let globalOwnerRole: Role; let globalMemberRole: Role; @@ -24,6 +28,7 @@ let owner: User; const externalHooks = utils.mockInstance(ExternalHooks); const testServer = utils.setupTestServer({ endpointGroups: ['passwordReset'] }); +const jwtService = Container.get(JwtService); beforeAll(async () => { globalOwnerRole = await testDb.getGlobalOwnerRole(); @@ -51,10 +56,6 @@ describe('POST /forgot-password', () => { expect(response.statusCode).toBe(200); expect(response.body).toEqual({}); - - const user = await Db.collections.User.findOneByOrFail({ email: payload.email }); - expect(user.resetPasswordToken).toBeDefined(); - expect(user.resetPasswordTokenExpiration).toBeGreaterThan(Math.ceil(Date.now() / 1000)); }), ); }); @@ -66,9 +67,6 @@ describe('POST /forgot-password', () => { .post('/forgot-password') .send({ email: owner.email }) .expect(500); - - const storedOwner = await Db.collections.User.findOneByOrFail({ email: owner.email }); - expect(storedOwner.resetPasswordToken).toBeNull(); }); test('should fail if SAML is authentication method', async () => { @@ -84,8 +82,6 @@ describe('POST /forgot-password', () => { .send({ email: member.email }) .expect(403); - const storedOwner = await Db.collections.User.findOneByOrFail({ email: member.email }); - expect(storedOwner.resetPasswordToken).toBeNull(); await setCurrentAuthenticationMethod('email'); }); @@ -100,8 +96,6 @@ describe('POST /forgot-password', () => { expect(response.statusCode).toBe(200); expect(response.body).toEqual({}); - const storedOwner = await Db.collections.User.findOneByOrFail({ email: owner.email }); - expect(storedOwner.resetPasswordToken).not.toBeNull(); await setCurrentAuthenticationMethod('email'); }); @@ -119,9 +113,6 @@ describe('POST /forgot-password', () => { for (const invalidPayload of invalidPayloads) { const response = await testServer.authlessAgent.post('/forgot-password').send(invalidPayload); expect(response.statusCode).toBe(400); - - const storedOwner = await Db.collections.User.findOneByOrFail({ email: owner.email }); - expect(storedOwner.resetPasswordToken).toBeNull(); } }); @@ -142,13 +133,7 @@ describe('GET /resolve-password-token', () => { }); test('should succeed with valid inputs', async () => { - const resetPasswordToken = uuid(); - const resetPasswordTokenExpiration = Math.floor(Date.now() / 1000) + 100; - - await Db.collections.User.update(owner.id, { - resetPasswordToken, - resetPasswordTokenExpiration, - }); + const resetPasswordToken = jwtService.signData({ sub: owner.id }); const response = await testServer.authlessAgent .get('/resolve-password-token') @@ -172,21 +157,17 @@ describe('GET /resolve-password-token', () => { }); test('should fail if user is not found', async () => { + const token = jwtService.signData({ sub: 'test' }); + const response = await testServer.authlessAgent .get('/resolve-password-token') - .query({ userId: owner.id, token: uuid() }); + .query({ userId: owner.id, token }); expect(response.statusCode).toBe(404); }); test('should fail if token is expired', async () => { - const resetPasswordToken = uuid(); - const resetPasswordTokenExpiration = Math.floor(Date.now() / 1000) - 1; - - await Db.collections.User.update(owner.id, { - resetPasswordToken, - resetPasswordTokenExpiration, - }); + const resetPasswordToken = jwtService.signData({ sub: owner.id }, { expiresIn: '-1h' }); const response = await testServer.authlessAgent .get('/resolve-password-token') @@ -197,17 +178,10 @@ describe('GET /resolve-password-token', () => { }); describe('POST /change-password', () => { - const resetPasswordToken = uuid(); const passwordToStore = randomValidPassword(); test('should succeed with valid inputs', async () => { - const resetPasswordTokenExpiration = Math.floor(Date.now() / 1000) + 100; - - await Db.collections.User.update(owner.id, { - resetPasswordToken, - resetPasswordTokenExpiration, - }); - + const resetPasswordToken = jwtService.signData({ sub: owner.id }); const response = await testServer.authlessAgent.post('/change-password').send({ token: resetPasswordToken, userId: owner.id, @@ -234,12 +208,7 @@ describe('POST /change-password', () => { }); test('should fail with invalid inputs', async () => { - const resetPasswordTokenExpiration = Math.floor(Date.now() / 1000) + 100; - - await Db.collections.User.update(owner.id, { - resetPasswordToken, - resetPasswordTokenExpiration, - }); + const resetPasswordToken = jwtService.signData({ sub: owner.id }); const invalidPayloads = [ { token: uuid() }, @@ -272,12 +241,7 @@ describe('POST /change-password', () => { }); test('should fail when token has expired', async () => { - const resetPasswordTokenExpiration = Math.floor(Date.now() / 1000) - 1; - - await Db.collections.User.update(owner.id, { - resetPasswordToken, - resetPasswordTokenExpiration, - }); + const resetPasswordToken = jwtService.signData({ sub: owner.id }, { expiresIn: '-1h' }); const response = await testServer.authlessAgent.post('/change-password').send({ token: resetPasswordToken, diff --git a/packages/cli/test/integration/shared/utils/testServer.ts b/packages/cli/test/integration/shared/utils/testServer.ts index ac7f8076b6..cf86bcda6b 100644 --- a/packages/cli/test/integration/shared/utils/testServer.ts +++ b/packages/cli/test/integration/shared/utils/testServer.ts @@ -50,6 +50,7 @@ import * as testDb from '../../shared/testDb'; import { AUTHLESS_ENDPOINTS, PUBLIC_API_REST_PATH_SEGMENT, REST_PATH_SEGMENT } from '../constants'; import type { EndpointGroup, SetupProps, TestServer } from '../types'; import { mockInstance } from './mocking'; +import { JwtService } from '@/services/jwt.service'; /** * Plugin to prefix a path segment into a request URL pathname. @@ -182,6 +183,7 @@ export const setupTestServer = ({ const externalHooks = Container.get(ExternalHooks); const internalHooks = Container.get(InternalHooks); const mailer = Container.get(UserManagementMailer); + const jwtService = Container.get(JwtService); const repositories = Db.collections; for (const group of functionEndpoints) { @@ -238,6 +240,7 @@ export const setupTestServer = ({ internalHooks, mailer, repositories, + jwtService, }), ); break; @@ -260,6 +263,7 @@ export const setupTestServer = ({ repositories, activeWorkflowRunner: Container.get(ActiveWorkflowRunner), logger, + jwtService, }), ); break; diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index f0baf4e26d..0f56258883 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/packages/cli/test/integration/users.api.test.ts @@ -78,7 +78,6 @@ describe('GET /users', () => { personalizationAnswers, globalRole, password, - resetPasswordToken, isPending, apiKey, } = user; @@ -89,7 +88,6 @@ describe('GET /users', () => { expect(lastName).toBeDefined(); expect(personalizationAnswers).toBeUndefined(); expect(password).toBeUndefined(); - expect(resetPasswordToken).toBeUndefined(); expect(isPending).toBe(false); expect(globalRole).toBeDefined(); expect(apiKey).not.toBeDefined(); @@ -254,7 +252,6 @@ describe('POST /users/:id', () => { lastName, personalizationAnswers, password, - resetPasswordToken, globalRole, isPending, apiKey, @@ -266,7 +263,6 @@ describe('POST /users/:id', () => { expect(lastName).toBe(memberData.lastName); expect(personalizationAnswers).toBeNull(); expect(password).toBeUndefined(); - expect(resetPasswordToken).toBeUndefined(); expect(isPending).toBe(false); expect(globalRole).toBeDefined(); expect(apiKey).not.toBeDefined(); @@ -404,14 +400,12 @@ describe('POST /users', () => { } const storedUser = await Db.collections.User.findOneByOrFail({ id }); - const { firstName, lastName, personalizationAnswers, password, resetPasswordToken } = - storedUser; + const { firstName, lastName, personalizationAnswers, password } = storedUser; expect(firstName).toBeNull(); expect(lastName).toBeNull(); expect(personalizationAnswers).toBeNull(); expect(password).toBeNull(); - expect(resetPasswordToken).toBeNull(); } }); diff --git a/packages/cli/test/unit/services/jwt.service.test.ts b/packages/cli/test/unit/services/jwt.service.test.ts new file mode 100644 index 0000000000..c3dde6be5d --- /dev/null +++ b/packages/cli/test/unit/services/jwt.service.test.ts @@ -0,0 +1,42 @@ +import config from '@/config'; +import { JwtService } from '@/services/jwt.service'; +import { randomString } from '../../integration/shared/random'; +import * as jwt from 'jsonwebtoken'; + +describe('JwtService', () => { + config.set('userManagement.jwtSecret', randomString(5, 10)); + + const jwtService = new JwtService(); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + test('Should sign input with user management secret', async () => { + const userId = 1; + + const token = jwtService.signData({ sub: userId }); + expect(typeof token).toBe('string'); + + const secret = config.get('userManagement.jwtSecret'); + + const decodedToken = jwt.verify(token, secret); + + expect(decodedToken).toHaveProperty('sub'); + expect(decodedToken).toHaveProperty('iat'); + expect(decodedToken?.sub).toBe(userId); + }); + + test('Should verify token with user management secret', async () => { + const userId = 1; + + const secret = config.get('userManagement.jwtSecret'); + + const token = jwt.sign({ sub: userId }, secret); + + const decodedToken = jwt.verify(token, secret); + + expect(decodedToken).toHaveProperty('sub'); + expect(decodedToken?.sub).toBe(userId); + }); +}); diff --git a/packages/editor-ui/src/api/users.ts b/packages/editor-ui/src/api/users.ts index 18da881afe..74b717f280 100644 --- a/packages/editor-ui/src/api/users.ts +++ b/packages/editor-ui/src/api/users.ts @@ -67,14 +67,14 @@ export async function sendForgotPasswordEmail( export async function validatePasswordToken( context: IRestApiContext, - params: { token: string; userId: string }, + params: { token: string }, ): Promise { await makeRestApiRequest(context, 'GET', '/resolve-password-token', params); } export async function changePassword( context: IRestApiContext, - params: { token: string; password: string; userId: string }, + params: { token: string; password: string }, ): Promise { await makeRestApiRequest(context, 'POST', '/change-password', params); } diff --git a/packages/editor-ui/src/stores/users.store.ts b/packages/editor-ui/src/stores/users.store.ts index ff54eebc1c..74e3672969 100644 --- a/packages/editor-ui/src/stores/users.store.ts +++ b/packages/editor-ui/src/stores/users.store.ts @@ -226,15 +226,11 @@ export const useUsersStore = defineStore(STORES.USERS, { const rootStore = useRootStore(); await sendForgotPasswordEmail(rootStore.getRestApiContext, params); }, - async validatePasswordToken(params: { token: string; userId: string }): Promise { + async validatePasswordToken(params: { token: string }): Promise { const rootStore = useRootStore(); await validatePasswordToken(rootStore.getRestApiContext, params); }, - async changePassword(params: { - token: string; - password: string; - userId: string; - }): Promise { + async changePassword(params: { token: string; password: string }): Promise { const rootStore = useRootStore(); await changePassword(rootStore.getRestApiContext, params); }, diff --git a/packages/editor-ui/src/views/ChangePasswordView.vue b/packages/editor-ui/src/views/ChangePasswordView.vue index 92aeee733d..8975566c80 100644 --- a/packages/editor-ui/src/views/ChangePasswordView.vue +++ b/packages/editor-ui/src/views/ChangePasswordView.vue @@ -75,23 +75,15 @@ export default defineComponent({ }, ], }; - const token = - !this.$route.query.token || typeof this.$route.query.token !== 'string' - ? null - : this.$route.query.token; - const userId = - !this.$route.query.userId || typeof this.$route.query.userId !== 'string' - ? null - : this.$route.query.userId; + + const token = this.getResetToken(); + try { if (!token) { throw new Error(this.$locale.baseText('auth.changePassword.missingTokenError')); } - if (!userId) { - throw new Error(this.$locale.baseText('auth.changePassword.missingUserIdError')); - } - await this.usersStore.validatePasswordToken({ token, userId }); + await this.usersStore.validatePasswordToken({ token }); } catch (e) { this.showMessage({ title: this.$locale.baseText('auth.changePassword.tokenValidationError'), @@ -118,20 +110,18 @@ export default defineComponent({ this.password = e.value; } }, + getResetToken(): string | null { + return !this.$route.query.token || typeof this.$route.query.token !== 'string' + ? null + : this.$route.query.token; + }, async onSubmit() { try { this.loading = true; - const token = - !this.$route.query.token || typeof this.$route.query.token !== 'string' - ? null - : this.$route.query.token; - const userId = - !this.$route.query.userId || typeof this.$route.query.userId !== 'string' - ? null - : this.$route.query.userId; + const token = this.getResetToken(); - if (token && userId) { - await this.usersStore.changePassword({ token, userId, password: this.password }); + if (token) { + await this.usersStore.changePassword({ token, password: this.password }); this.showMessage({ type: 'success',