From 9fe6a71690154c77bc005a9338e148674e14f519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 22 Aug 2024 09:33:06 +0200 Subject: [PATCH] feat(core): Logout should invalidate the auth token (no-changelog) (#10335) --- .../src/auth/__tests__/auth.service.test.ts | 50 ++++++++++++++++++- packages/cli/src/auth/auth.service.ts | 20 ++++++++ .../__tests__/me.controller.test.ts | 12 +++-- .../cli/src/controllers/auth.controller.ts | 5 +- .../databases/entities/InvalidAuthToken.ts | 11 ++++ packages/cli/src/databases/entities/index.ts | 2 + ...23627610222-CreateInvalidAuthTokenTable.ts | 16 ++++++ .../src/databases/migrations/mysqldb/index.ts | 2 + .../databases/migrations/postgresdb/index.ts | 2 + .../src/databases/migrations/sqlite/index.ts | 2 + .../invalidAuthToken.repository.ts | 10 ++++ packages/cli/src/services/jwt.service.ts | 8 ++- .../cli/test/integration/auth.api.test.ts | 8 ++- packages/editor-ui/src/constants.ts | 3 ++ packages/editor-ui/src/stores/users.store.ts | 4 +- packages/editor-ui/src/utils/apiUtils.ts | 26 +++++----- 16 files changed, 158 insertions(+), 23 deletions(-) create mode 100644 packages/cli/src/databases/entities/InvalidAuthToken.ts create mode 100644 packages/cli/src/databases/migrations/common/1723627610222-CreateInvalidAuthTokenTable.ts create mode 100644 packages/cli/src/databases/repositories/invalidAuthToken.repository.ts diff --git a/packages/cli/src/auth/__tests__/auth.service.test.ts b/packages/cli/src/auth/__tests__/auth.service.test.ts index 60fdd12126..82c820ac9e 100644 --- a/packages/cli/src/auth/__tests__/auth.service.test.ts +++ b/packages/cli/src/auth/__tests__/auth.service.test.ts @@ -6,6 +6,7 @@ import { AuthService } from '@/auth/auth.service'; import config from '@/config'; import { AUTH_COOKIE_NAME, Time } from '@/constants'; import type { User } from '@db/entities/User'; +import type { InvalidAuthTokenRepository } from '@db/repositories/invalidAuthToken.repository'; import type { UserRepository } from '@db/repositories/user.repository'; import { JwtService } from '@/services/jwt.service'; import type { UrlService } from '@/services/url.service'; @@ -26,7 +27,15 @@ describe('AuthService', () => { const jwtService = new JwtService(mock()); const urlService = mock(); const userRepository = mock(); - const authService = new AuthService(mock(), mock(), jwtService, urlService, userRepository); + const invalidAuthTokenRepository = mock(); + const authService = new AuthService( + mock(), + mock(), + jwtService, + urlService, + userRepository, + invalidAuthTokenRepository, + ); const now = new Date('2024-02-01T01:23:45.678Z'); jest.useFakeTimers({ now }); @@ -70,16 +79,36 @@ describe('AuthService', () => { it('should 401 if no cookie is set', async () => { req.cookies[AUTH_COOKIE_NAME] = undefined; + await authService.authMiddleware(req, res, next); + + expect(invalidAuthTokenRepository.existsBy).not.toHaveBeenCalled(); expect(next).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(401); }); it('should 401 and clear the cookie if the JWT is expired', async () => { req.cookies[AUTH_COOKIE_NAME] = validToken; + invalidAuthTokenRepository.existsBy.mockResolvedValue(false); jest.advanceTimersByTime(365 * Time.days.toMilliseconds); await authService.authMiddleware(req, res, next); + + expect(invalidAuthTokenRepository.existsBy).toHaveBeenCalled(); + expect(userRepository.findOne).not.toHaveBeenCalled(); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + expect(res.clearCookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME); + }); + + it('should 401 and clear the cookie if the JWT has been invalidated', async () => { + req.cookies[AUTH_COOKIE_NAME] = validToken; + invalidAuthTokenRepository.existsBy.mockResolvedValue(true); + + await authService.authMiddleware(req, res, next); + + expect(invalidAuthTokenRepository.existsBy).toHaveBeenCalled(); + expect(userRepository.findOne).not.toHaveBeenCalled(); expect(next).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(401); expect(res.clearCookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME); @@ -88,9 +117,11 @@ describe('AuthService', () => { it('should refresh the cookie before it expires', async () => { req.cookies[AUTH_COOKIE_NAME] = validToken; jest.advanceTimersByTime(6 * Time.days.toMilliseconds); + invalidAuthTokenRepository.existsBy.mockResolvedValue(false); userRepository.findOne.mockResolvedValue(user); await authService.authMiddleware(req, res, next); + expect(next).toHaveBeenCalled(); expect(res.cookie).toHaveBeenCalledWith('n8n-auth', expect.any(String), { httpOnly: true, @@ -302,4 +333,21 @@ describe('AuthService', () => { expect(resolvedUser).toEqual(user); }); }); + + describe('invalidateToken', () => { + const req = mock({ + cookies: { + [AUTH_COOKIE_NAME]: validToken, + }, + }); + + it('should invalidate the token', async () => { + await authService.invalidateToken(req); + + expect(invalidAuthTokenRepository.insert).toHaveBeenCalledWith({ + token: validToken, + expiresAt: new Date('2024-02-08T01:23:45.000Z'), + }); + }); + }); }); diff --git a/packages/cli/src/auth/auth.service.ts b/packages/cli/src/auth/auth.service.ts index e2487e6f6f..cb75fa158b 100644 --- a/packages/cli/src/auth/auth.service.ts +++ b/packages/cli/src/auth/auth.service.ts @@ -6,6 +6,7 @@ import { JsonWebTokenError, TokenExpiredError } from 'jsonwebtoken'; import config from '@/config'; import { AUTH_COOKIE_NAME, RESPONSE_ERROR_MESSAGES, Time } from '@/constants'; import type { User } from '@db/entities/User'; +import { InvalidAuthTokenRepository } from '@db/repositories/invalidAuthToken.repository'; import { UserRepository } from '@db/repositories/user.repository'; import { AuthError } from '@/errors/response-errors/auth.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; @@ -53,6 +54,7 @@ export class AuthService { private readonly jwtService: JwtService, private readonly urlService: UrlService, private readonly userRepository: UserRepository, + private readonly invalidAuthTokenRepository: InvalidAuthTokenRepository, ) { // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment this.authMiddleware = this.authMiddleware.bind(this); @@ -62,6 +64,8 @@ export class AuthService { const token = req.cookies[AUTH_COOKIE_NAME]; if (token) { try { + const isInvalid = await this.invalidAuthTokenRepository.existsBy({ token }); + if (isInvalid) throw new AuthError('Unauthorized'); req.user = await this.resolveJwt(token, req, res); } catch (error) { if (error instanceof JsonWebTokenError || error instanceof AuthError) { @@ -80,6 +84,22 @@ export class AuthService { res.clearCookie(AUTH_COOKIE_NAME); } + async invalidateToken(req: AuthenticatedRequest) { + const token = req.cookies[AUTH_COOKIE_NAME]; + if (!token) return; + try { + const { exp } = this.jwtService.decode(token); + if (exp) { + await this.invalidAuthTokenRepository.insert({ + token, + expiresAt: new Date(exp * 1000), + }); + } + } catch (e) { + this.logger.warn('failed to invalidate auth token', { error: (e as Error).message }); + } + } + issueCookie(res: Response, user: User, browserId?: string) { // TODO: move this check to the login endpoint in AuthController // If the instance has exceeded its user quota, prevent non-owners from logging in diff --git a/packages/cli/src/controllers/__tests__/me.controller.test.ts b/packages/cli/src/controllers/__tests__/me.controller.test.ts index 9363cefd0a..54a0f39a7c 100644 --- a/packages/cli/src/controllers/__tests__/me.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/me.controller.test.ts @@ -2,6 +2,7 @@ import type { Response } from 'express'; import { Container } from 'typedi'; import jwt from 'jsonwebtoken'; import { mock, anyObject } from 'jest-mock-extended'; + import type { PublicUser } from '@/Interfaces'; import type { User } from '@db/entities/User'; import { API_KEY_PREFIX, MeController } from '@/controllers/me.controller'; @@ -11,14 +12,16 @@ import { UserService } from '@/services/user.service'; import { ExternalHooks } from '@/ExternalHooks'; import { License } from '@/License'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; -import { UserRepository } from '@/databases/repositories/user.repository'; import { EventService } from '@/events/event.service'; -import { badPasswords } from '@test/testData'; -import { mockInstance } from '@test/mocking'; -import { AuthUserRepository } from '@/databases/repositories/authUser.repository'; +import { AuthUserRepository } from '@db/repositories/authUser.repository'; +import { InvalidAuthTokenRepository } from '@db/repositories/invalidAuthToken.repository'; +import { UserRepository } from '@db/repositories/user.repository'; import { MfaService } from '@/Mfa/mfa.service'; import { InvalidMfaCodeError } from '@/errors/response-errors/invalid-mfa-code.error'; +import { badPasswords } from '@test/testData'; +import { mockInstance } from '@test/mocking'; + const browserId = 'test-browser-id'; describe('MeController', () => { @@ -28,6 +31,7 @@ describe('MeController', () => { const userRepository = mockInstance(UserRepository); const mockMfaService = mockInstance(MfaService); mockInstance(AuthUserRepository); + mockInstance(InvalidAuthTokenRepository); mockInstance(License).isWithinUsersLimit.mockReturnValue(true); const controller = Container.get(MeController); diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index 99b5c52320..bf4ce5a388 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -1,9 +1,9 @@ import validator from 'validator'; +import { Response } from 'express'; import { AuthService } from '@/auth/auth.service'; import { Get, Post, RestController } from '@/decorators'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; -import { Request, Response } from 'express'; import type { User } from '@db/entities/User'; import { AuthenticatedRequest, LoginRequest, UserRequest } from '@/requests'; import type { PublicUser } from '@/Interfaces'; @@ -185,7 +185,8 @@ export class AuthController { /** Log out a user */ @Post('/logout') - logout(_: Request, res: Response) { + async logout(req: AuthenticatedRequest, res: Response) { + await this.authService.invalidateToken(req); this.authService.clearCookie(res); return { loggedOut: true }; } diff --git a/packages/cli/src/databases/entities/InvalidAuthToken.ts b/packages/cli/src/databases/entities/InvalidAuthToken.ts new file mode 100644 index 0000000000..e21860d146 --- /dev/null +++ b/packages/cli/src/databases/entities/InvalidAuthToken.ts @@ -0,0 +1,11 @@ +import { Column, Entity, PrimaryColumn } from '@n8n/typeorm'; +import { datetimeColumnType } from './AbstractEntity'; + +@Entity() +export class InvalidAuthToken { + @PrimaryColumn() + token: string; + + @Column(datetimeColumnType) + expiresAt: Date; +} diff --git a/packages/cli/src/databases/entities/index.ts b/packages/cli/src/databases/entities/index.ts index db8b113baf..bd7d187486 100644 --- a/packages/cli/src/databases/entities/index.ts +++ b/packages/cli/src/databases/entities/index.ts @@ -21,6 +21,7 @@ import { ExecutionData } from './ExecutionData'; import { WorkflowHistory } from './WorkflowHistory'; import { Project } from './Project'; import { ProjectRelation } from './ProjectRelation'; +import { InvalidAuthToken } from './InvalidAuthToken'; export const entities = { AuthIdentity, @@ -31,6 +32,7 @@ export const entities = { ExecutionEntity, InstalledNodes, InstalledPackages, + InvalidAuthToken, Settings, SharedCredentials, SharedWorkflow, diff --git a/packages/cli/src/databases/migrations/common/1723627610222-CreateInvalidAuthTokenTable.ts b/packages/cli/src/databases/migrations/common/1723627610222-CreateInvalidAuthTokenTable.ts new file mode 100644 index 0000000000..f28696c199 --- /dev/null +++ b/packages/cli/src/databases/migrations/common/1723627610222-CreateInvalidAuthTokenTable.ts @@ -0,0 +1,16 @@ +import type { MigrationContext, ReversibleMigration } from '@db/types'; + +const tableName = 'invalid_auth_token'; + +export class CreateInvalidAuthTokenTable1723627610222 implements ReversibleMigration { + async up({ schemaBuilder: { createTable, column } }: MigrationContext) { + await createTable(tableName).withColumns( + column('token').varchar(512).primary, + column('expiresAt').timestamp().notNull, + ); + } + + async down({ schemaBuilder: { dropTable } }: MigrationContext) { + await dropTable(tableName); + } +} diff --git a/packages/cli/src/databases/migrations/mysqldb/index.ts b/packages/cli/src/databases/migrations/mysqldb/index.ts index ecd5f66a7c..b4900eb59d 100644 --- a/packages/cli/src/databases/migrations/mysqldb/index.ts +++ b/packages/cli/src/databases/migrations/mysqldb/index.ts @@ -59,6 +59,7 @@ import { RemoveNodesAccess1712044305787 } from '../common/1712044305787-RemoveNo import { MakeExecutionStatusNonNullable1714133768521 } from '../common/1714133768521-MakeExecutionStatusNonNullable'; import { AddActivatedAtUserSetting1717498465931 } from './1717498465931-AddActivatedAtUserSetting'; import { AddConstraintToExecutionMetadata1720101653148 } from '../common/1720101653148-AddConstraintToExecutionMetadata'; +import { CreateInvalidAuthTokenTable1723627610222 } from '../common/1723627610222-CreateInvalidAuthTokenTable'; export const mysqlMigrations: Migration[] = [ InitialMigration1588157391238, @@ -121,4 +122,5 @@ export const mysqlMigrations: Migration[] = [ MakeExecutionStatusNonNullable1714133768521, AddActivatedAtUserSetting1717498465931, AddConstraintToExecutionMetadata1720101653148, + CreateInvalidAuthTokenTable1723627610222, ]; diff --git a/packages/cli/src/databases/migrations/postgresdb/index.ts b/packages/cli/src/databases/migrations/postgresdb/index.ts index 720c79a8e3..85bd58f371 100644 --- a/packages/cli/src/databases/migrations/postgresdb/index.ts +++ b/packages/cli/src/databases/migrations/postgresdb/index.ts @@ -59,6 +59,7 @@ import { MakeExecutionStatusNonNullable1714133768521 } from '../common/171413376 import { AddActivatedAtUserSetting1717498465931 } from './1717498465931-AddActivatedAtUserSetting'; import { AddConstraintToExecutionMetadata1720101653148 } from '../common/1720101653148-AddConstraintToExecutionMetadata'; import { FixExecutionMetadataSequence1721377157740 } from './1721377157740-FixExecutionMetadataSequence'; +import { CreateInvalidAuthTokenTable1723627610222 } from '../common/1723627610222-CreateInvalidAuthTokenTable'; export const postgresMigrations: Migration[] = [ InitialMigration1587669153312, @@ -121,4 +122,5 @@ export const postgresMigrations: Migration[] = [ AddActivatedAtUserSetting1717498465931, AddConstraintToExecutionMetadata1720101653148, FixExecutionMetadataSequence1721377157740, + CreateInvalidAuthTokenTable1723627610222, ]; diff --git a/packages/cli/src/databases/migrations/sqlite/index.ts b/packages/cli/src/databases/migrations/sqlite/index.ts index 15000a78e0..974c743d0f 100644 --- a/packages/cli/src/databases/migrations/sqlite/index.ts +++ b/packages/cli/src/databases/migrations/sqlite/index.ts @@ -56,6 +56,7 @@ import { RemoveNodesAccess1712044305787 } from '../common/1712044305787-RemoveNo import { MakeExecutionStatusNonNullable1714133768521 } from '../common/1714133768521-MakeExecutionStatusNonNullable'; import { AddActivatedAtUserSetting1717498465931 } from './1717498465931-AddActivatedAtUserSetting'; import { AddConstraintToExecutionMetadata1720101653148 } from '../common/1720101653148-AddConstraintToExecutionMetadata'; +import { CreateInvalidAuthTokenTable1723627610222 } from '../common/1723627610222-CreateInvalidAuthTokenTable'; const sqliteMigrations: Migration[] = [ InitialMigration1588102412422, @@ -115,6 +116,7 @@ const sqliteMigrations: Migration[] = [ MakeExecutionStatusNonNullable1714133768521, AddActivatedAtUserSetting1717498465931, AddConstraintToExecutionMetadata1720101653148, + CreateInvalidAuthTokenTable1723627610222, ]; export { sqliteMigrations }; diff --git a/packages/cli/src/databases/repositories/invalidAuthToken.repository.ts b/packages/cli/src/databases/repositories/invalidAuthToken.repository.ts new file mode 100644 index 0000000000..c6340ba88a --- /dev/null +++ b/packages/cli/src/databases/repositories/invalidAuthToken.repository.ts @@ -0,0 +1,10 @@ +import { Service } from 'typedi'; +import { DataSource, Repository } from '@n8n/typeorm'; +import { InvalidAuthToken } from '../entities/InvalidAuthToken'; + +@Service() +export class InvalidAuthTokenRepository extends Repository { + constructor(dataSource: DataSource) { + super(InvalidAuthToken, dataSource.manager); + } +} diff --git a/packages/cli/src/services/jwt.service.ts b/packages/cli/src/services/jwt.service.ts index 7f9f4fdd84..bcc2cdcbad 100644 --- a/packages/cli/src/services/jwt.service.ts +++ b/packages/cli/src/services/jwt.service.ts @@ -23,11 +23,15 @@ export class JwtService { } } - public sign(payload: object, options: jwt.SignOptions = {}): string { + sign(payload: object, options: jwt.SignOptions = {}): string { return jwt.sign(payload, this.jwtSecret, options); } - public verify(token: string, options: jwt.VerifyOptions = {}) { + decode(token: string) { + return jwt.decode(token) as JwtPayload; + } + + verify(token: string, options: jwt.VerifyOptions = {}) { return jwt.verify(token, this.jwtSecret, options) as T; } } diff --git a/packages/cli/test/integration/auth.api.test.ts b/packages/cli/test/integration/auth.api.test.ts index 08d4eb0a66..887cba4a04 100644 --- a/packages/cli/test/integration/auth.api.test.ts +++ b/packages/cli/test/integration/auth.api.test.ts @@ -386,13 +386,19 @@ describe('GET /resolve-signup-token', () => { describe('POST /logout', () => { test('should log user out', async () => { const owner = await createUser({ role: 'global:owner' }); + const ownerAgent = testServer.authAgentFor(owner); + // @ts-expect-error `accessInfo` types are incorrect + const cookie = ownerAgent.jar.getCookie(AUTH_COOKIE_NAME, { path: '/' }); - const response = await testServer.authAgentFor(owner).post('/logout'); + const response = await ownerAgent.post('/logout'); expect(response.statusCode).toBe(200); expect(response.body).toEqual(LOGGED_OUT_RESPONSE_BODY); const authToken = utils.getAuthToken(response); expect(authToken).toBeUndefined(); + + ownerAgent.jar.setCookie(`${AUTH_COOKIE_NAME}=${cookie!.value}`); + await ownerAgent.get('/login').expect(401); }); }); diff --git a/packages/editor-ui/src/constants.ts b/packages/editor-ui/src/constants.ts index cf3c5bebb8..8d23f2dea1 100644 --- a/packages/editor-ui/src/constants.ts +++ b/packages/editor-ui/src/constants.ts @@ -859,3 +859,6 @@ export const INSECURE_CONNECTION_WARNING = ` export const CanvasNodeKey = 'canvasNode' as unknown as InjectionKey; export const CanvasNodeHandleKey = 'canvasNodeHandle' as unknown as InjectionKey; + +/** Auth */ +export const BROWSER_ID_STORAGE_KEY = 'n8n-browserId'; diff --git a/packages/editor-ui/src/stores/users.store.ts b/packages/editor-ui/src/stores/users.store.ts index ef153bb4b7..613d2ea48d 100644 --- a/packages/editor-ui/src/stores/users.store.ts +++ b/packages/editor-ui/src/stores/users.store.ts @@ -1,6 +1,6 @@ import type { IUpdateUserSettingsReqPayload, UpdateGlobalRolePayload } from '@/api/users'; import * as usersApi from '@/api/users'; -import { PERSONALIZATION_MODAL_KEY, STORES, ROLE } from '@/constants'; +import { BROWSER_ID_STORAGE_KEY, PERSONALIZATION_MODAL_KEY, STORES, ROLE } from '@/constants'; import type { Cloud, IPersonalizationLatestVersion, @@ -180,6 +180,8 @@ export const useUsersStore = defineStore(STORES.USERS, () => { postHogStore.reset(); uiStore.clearBannerStack(); npsSurveyStore.resetNpsSurveyOnLogOut(); + + localStorage.removeItem(BROWSER_ID_STORAGE_KEY); }; const createOwner = async (params: { diff --git a/packages/editor-ui/src/utils/apiUtils.ts b/packages/editor-ui/src/utils/apiUtils.ts index b07f8910b9..fef135769e 100644 --- a/packages/editor-ui/src/utils/apiUtils.ts +++ b/packages/editor-ui/src/utils/apiUtils.ts @@ -1,16 +1,20 @@ import type { AxiosRequestConfig, Method, RawAxiosRequestHeaders } from 'axios'; import axios from 'axios'; import { ApplicationError, jsonParse, type GenericValue, type IDataObject } from 'n8n-workflow'; -import type { IExecutionFlattedResponse, IExecutionResponse, IRestApiContext } from '@/Interface'; import { parse } from 'flatted'; import { assert } from '@/utils/assert'; -const BROWSER_ID_STORAGE_KEY = 'n8n-browserId'; -let browserId = localStorage.getItem(BROWSER_ID_STORAGE_KEY); -if (!browserId && 'randomUUID' in crypto) { - browserId = crypto.randomUUID(); - localStorage.setItem(BROWSER_ID_STORAGE_KEY, browserId); -} +import { BROWSER_ID_STORAGE_KEY } from '@/constants'; +import type { IExecutionFlattedResponse, IExecutionResponse, IRestApiContext } from '@/Interface'; + +const getBrowserId = () => { + let browserId = localStorage.getItem(BROWSER_ID_STORAGE_KEY); + if (!browserId && 'randomUUID' in crypto) { + browserId = crypto.randomUUID(); + localStorage.setItem(BROWSER_ID_STORAGE_KEY, browserId); + } + return browserId!; +}; export const NO_NETWORK_ERROR_CODE = 999; export const STREAM_SEPERATOR = '⧉⇋⇋➽⌑⧉§§\n'; @@ -82,8 +86,8 @@ export async function request(config: { baseURL, headers: headers ?? {}, }; - if (baseURL.startsWith('/') && browserId) { - options.headers!['browser-id'] = browserId; + if (baseURL.startsWith('/')) { + options.headers!['browser-id'] = getBrowserId(); } if ( import.meta.env.NODE_ENV !== 'production' && @@ -204,11 +208,9 @@ export async function streamRequest( separator = STREAM_SEPERATOR, ): Promise { const headers: Record = { + 'browser-id': getBrowserId(), 'Content-Type': 'application/json', }; - if (browserId) { - headers['browser-id'] = browserId; - } const assistantRequest: RequestInit = { headers, method: 'POST',