From 638a6be25bd616ec19a32bca5842419e6e27c282 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:01 +0100 Subject: [PATCH 01/21] add test for cli/auth/jwt --- packages/cli/test/unit/auth/jwt.test.ts | 45 +++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 packages/cli/test/unit/auth/jwt.test.ts diff --git a/packages/cli/test/unit/auth/jwt.test.ts b/packages/cli/test/unit/auth/jwt.test.ts new file mode 100644 index 0000000000000..c21e52a77e4cc --- /dev/null +++ b/packages/cli/test/unit/auth/jwt.test.ts @@ -0,0 +1,45 @@ +import { Container } from 'typedi'; +import { v4 as uuid } from 'uuid'; + +import { JwtService } from '@/services/jwt.service'; +import { License } from '@/License'; +import { User } from '@db/entities/User'; +import { issueJWT } from '@/auth/jwt'; + +import { mockInstance } from '../../shared/mocking'; + +// TODO: is the the right level of testing? +// Should I mock more or less? +// Should I write an integration test that checks that a user is actually logged out? +describe('jwt', () => { + const jwtService = Container.get(JwtService); + + describe('issueJWT', () => { + beforeEach(() => { + // TODO: Why do I need to mock the License but not for example the JwtService 🤷 + mockInstance(License); + }); + + it('should default to expire in 168 hours', () => { + const defaultInSeconds = 7 * 24 * 60 * 60; + const defaultInMS = defaultInSeconds * 1000; + // TODO: is this the right way to create a mock user? + const mockUser = Object.assign(new User(), { + id: uuid(), + password: 'passwordHash', + mfaEnabled: false, + mfaSecret: 'test', + mfaRecoveryCodes: ['test'], + updatedAt: new Date(), + authIdentities: [], + }); + const { token, expiresIn } = issueJWT(mockUser); + + expect(expiresIn).toBe(defaultInMS); + const decodedToken = jwtService.verify(token); + expect(decodedToken.exp).toBeDefined(); + expect(decodedToken.iat).toBeDefined(); + expect(decodedToken.exp! - decodedToken.iat!).toBe(defaultInSeconds); + }); + }); +}); From 00f337c04f7d20a05621f4cf4607143d560e5553 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:24 +0100 Subject: [PATCH 02/21] rename expiresIn and convert it to seconds Rename `JwtToken.expiresIn` to `JwtToken.expiresInSeconds` to make it clear what unit it's supposed to be. Also it's now seconds and not milliseconds. Reason for that is that seconds are a base unit of the SI, which makes it a good default. Also it's what the JWTs expect in `expireIn` and what the cookie Max-Age property expects. Until now the function returned milliseconds and that was passed into the Max-Age property in `auth/jwt.issueCookie`. This caused no real issue, but it meant that the JWT would always expire before the cookie. --- packages/cli/src/Interfaces.ts | 3 +- packages/cli/src/auth/jwt.ts | 9 ++--- packages/cli/src/config/schema.ts | 7 ++-- packages/cli/test/unit/auth/jwt.test.ts | 44 ++++++++++++++++++++++--- 4 files changed, 50 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts index 88a0c2f610d7f..662bbb6e1028c 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -667,7 +667,8 @@ export interface ILicensePostResponse extends ILicenseReadResponse { export interface JwtToken { token: string; - expiresIn: number; + // TODO: is it ok to rename this? + expiresInSeconds: number; } export interface JwtPayload { diff --git a/packages/cli/src/auth/jwt.ts b/packages/cli/src/auth/jwt.ts index c79479008d113..009f8b1bbfbdf 100644 --- a/packages/cli/src/auth/jwt.ts +++ b/packages/cli/src/auth/jwt.ts @@ -14,7 +14,8 @@ import { ApplicationError } from 'n8n-workflow'; export function issueJWT(user: User): JwtToken { const { id, email, password } = user; - const expiresIn = 7 * 86400000; // 7 days + const expiresInHours = config.get('userManagement.jwtSessionDurationHours'); + const expiresInSeconds = expiresInHours * 60 * 60; const isWithinUsersLimit = Container.get(License).isWithinUsersLimit(); const payload: JwtPayload = { @@ -37,12 +38,12 @@ export function issueJWT(user: User): JwtToken { } const signedToken = Container.get(JwtService).sign(payload, { - expiresIn: expiresIn / 1000 /* in seconds */, + expiresIn: expiresInSeconds, }); return { token: signedToken, - expiresIn, + expiresInSeconds, }; } @@ -86,7 +87,7 @@ export async function resolveJwt(token: string): Promise { export async function issueCookie(res: Response, user: User): Promise { const userData = issueJWT(user); res.cookie(AUTH_COOKIE_NAME, userData.token, { - maxAge: userData.expiresIn, + maxAge: userData.expiresInSeconds, httpOnly: true, sameSite: 'lax', }); diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index fca700fc7890d..01e0f08eb4a88 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -762,11 +762,12 @@ export const schema = { default: '', env: 'N8N_USER_MANAGEMENT_JWT_SECRET', }, - jwtDuration: { - doc: 'Set a specific JWT secret (optional - n8n can generate one)', // Generated @ start.ts + // TODO: Is it ok to add units here? + jwtSessionDurationHours: { + doc: 'Set a specific expiration date for the JWTs.', format: Number, default: 168, - env: 'N8N_USER_MANAGEMENT_JWT_DURATION', + env: 'N8N_USER_MANAGEMENT_JWT_DURATION_HOURS', }, isInstanceOwnerSetUp: { // n8n loads this setting from DB on startup diff --git a/packages/cli/test/unit/auth/jwt.test.ts b/packages/cli/test/unit/auth/jwt.test.ts index c21e52a77e4cc..ed8f220652594 100644 --- a/packages/cli/test/unit/auth/jwt.test.ts +++ b/packages/cli/test/unit/auth/jwt.test.ts @@ -1,6 +1,7 @@ import { Container } from 'typedi'; import { v4 as uuid } from 'uuid'; +import config from '@/config'; import { JwtService } from '@/services/jwt.service'; import { License } from '@/License'; import { User } from '@db/entities/User'; @@ -11,10 +12,10 @@ import { mockInstance } from '../../shared/mocking'; // TODO: is the the right level of testing? // Should I mock more or less? // Should I write an integration test that checks that a user is actually logged out? -describe('jwt', () => { +describe('jwt.issueJWT', () => { const jwtService = Container.get(JwtService); - describe('issueJWT', () => { + describe('when not setting userManagement.jwtSessionDuration', () => { beforeEach(() => { // TODO: Why do I need to mock the License but not for example the JwtService 🤷 mockInstance(License); @@ -22,7 +23,6 @@ describe('jwt', () => { it('should default to expire in 168 hours', () => { const defaultInSeconds = 7 * 24 * 60 * 60; - const defaultInMS = defaultInSeconds * 1000; // TODO: is this the right way to create a mock user? const mockUser = Object.assign(new User(), { id: uuid(), @@ -33,13 +33,47 @@ describe('jwt', () => { updatedAt: new Date(), authIdentities: [], }); - const { token, expiresIn } = issueJWT(mockUser); + const { token, expiresInSeconds } = issueJWT(mockUser); - expect(expiresIn).toBe(defaultInMS); + expect(expiresInSeconds).toBe(defaultInSeconds); const decodedToken = jwtService.verify(token); expect(decodedToken.exp).toBeDefined(); expect(decodedToken.iat).toBeDefined(); expect(decodedToken.exp! - decodedToken.iat!).toBe(defaultInSeconds); }); }); + + describe('when setting userManagement.jwtSessionDuration', () => { + let oldDuration: number; + let testDuration = 1; + + beforeEach(() => { + mockInstance(License); + oldDuration = config.get('userManagement.jwtSessionDurationHours'); + config.set('userManagement.jwtSessionDurationHours', testDuration); + }); + + afterEach(() => { + config.set('userManagement.jwtSessionDuration', oldDuration); + }); + + it('should apply it to tokens', () => { + const mockUser = Object.assign(new User(), { + id: uuid(), + password: 'passwordHash', + mfaEnabled: false, + mfaSecret: 'test', + mfaRecoveryCodes: ['test'], + updatedAt: new Date(), + authIdentities: [], + }); + const { token, expiresInSeconds } = issueJWT(mockUser); + + expect(expiresInSeconds).toBe(testDuration * 60 * 60); + const decodedToken = jwtService.verify(token); + expect(decodedToken.exp).toBeDefined(); + expect(decodedToken.iat).toBeDefined(); + expect(decodedToken.exp! - decodedToken.iat!).toBe(testDuration * 60 * 60); + }); + }); }); From a9a674922412da4a79d649e91d976b6e2b75d600 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:28 +0100 Subject: [PATCH 03/21] add units to the doc string --- packages/cli/src/config/schema.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 01e0f08eb4a88..8fe8de3ef393f 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -764,7 +764,7 @@ export const schema = { }, // TODO: Is it ok to add units here? jwtSessionDurationHours: { - doc: 'Set a specific expiration date for the JWTs.', + doc: 'Set a specific expiration date for the JWTs in hours.', format: Number, default: 168, env: 'N8N_USER_MANAGEMENT_JWT_DURATION_HOURS', From 19f43b21a3a2e7566c2bcc44e6f9bfd37b39586a Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:30 +0100 Subject: [PATCH 04/21] move mockInstance call to module root no need to have it in the beforeEach --- packages/cli/test/unit/auth/jwt.test.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/cli/test/unit/auth/jwt.test.ts b/packages/cli/test/unit/auth/jwt.test.ts index ed8f220652594..bb106c46c1c61 100644 --- a/packages/cli/test/unit/auth/jwt.test.ts +++ b/packages/cli/test/unit/auth/jwt.test.ts @@ -9,6 +9,10 @@ import { issueJWT } from '@/auth/jwt'; import { mockInstance } from '../../shared/mocking'; +// TODO: Why do I need to mock the License but not for example the JwtService 🤷 +// A: because of the server not being initialized +mockInstance(License); + // TODO: is the the right level of testing? // Should I mock more or less? // Should I write an integration test that checks that a user is actually logged out? @@ -16,11 +20,6 @@ describe('jwt.issueJWT', () => { const jwtService = Container.get(JwtService); describe('when not setting userManagement.jwtSessionDuration', () => { - beforeEach(() => { - // TODO: Why do I need to mock the License but not for example the JwtService 🤷 - mockInstance(License); - }); - it('should default to expire in 168 hours', () => { const defaultInSeconds = 7 * 24 * 60 * 60; // TODO: is this the right way to create a mock user? From 067f433d6538720b0d9cbda82a868a42725648dc Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:31 +0100 Subject: [PATCH 05/21] exporting `refreshExpiringCookie` to unit test it Additionally I had to wrangle the types a bit. I want to call `await refreshExpiringCookie(...)` in the unit tests, but to do this without TS complaining the type needs account for the function being async. Right now adding the type to the variable binding hides that fact. My workaround for that is to let TS infer the type of `refreshExpiringCookie`. That lead to TS not being able to infer the types of `res` and `next`. To fix that I used `satisfied`. Happy to apply other solutions to this like extending the RequestHandler type and creating an `AsyncRequestHandler` or adding an `Async` type helper and using that. ```ts type Async unknown> = ( ...args: Parameters ) => Promise>; ``` --- packages/cli/src/middlewares/auth.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/middlewares/auth.ts b/packages/cli/src/middlewares/auth.ts index 195b174b74115..7a61072fdcda0 100644 --- a/packages/cli/src/middlewares/auth.ts +++ b/packages/cli/src/middlewares/auth.ts @@ -41,7 +41,7 @@ const userManagementJwtAuth = (): RequestHandler => { /** * middleware to refresh cookie before it expires */ -const refreshExpiringCookie: RequestHandler = async (req: AuthenticatedRequest, res, next) => { +export const refreshExpiringCookie = (async (req: AuthenticatedRequest, res, next) => { const cookieAuth = jwtFromRequest(req); if (cookieAuth && req.user) { const cookieContents = jwt.decode(cookieAuth) as JwtPayload & { exp: number }; @@ -51,7 +51,7 @@ const refreshExpiringCookie: RequestHandler = async (req: AuthenticatedRequest, } } next(); -}; +}) satisfies RequestHandler; const passportMiddleware = passport.authenticate('jwt', { session: false }) as RequestHandler; From fd55dfeb888a217146be497a3a7c801266d49c57 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:33 +0100 Subject: [PATCH 06/21] adding unit test for middleware/auth.ts --- .../cli/test/unit/middleware/auth.test.ts | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 packages/cli/test/unit/middleware/auth.test.ts diff --git a/packages/cli/test/unit/middleware/auth.test.ts b/packages/cli/test/unit/middleware/auth.test.ts new file mode 100644 index 0000000000000..2eac435e85bbb --- /dev/null +++ b/packages/cli/test/unit/middleware/auth.test.ts @@ -0,0 +1,71 @@ +import { v4 as uuid } from 'uuid'; + +import config from '@/config'; +import { AUTH_COOKIE_NAME } from '@/constants'; +import { License } from '@/License'; +import { User } from '@/databases/entities/User'; +import { issueJWT } from '@/auth/jwt'; +import { refreshExpiringCookie } from '@/middlewares'; + +import { mockInstance } from '../../shared/mocking'; + +import type { AuthenticatedRequest } from '@/requests'; +import type { Response } from 'express'; + +mockInstance(License); + +describe('refreshExpiringCookie', () => { + const oldDuration: number = config.get('userManagement.jwtSessionDurationHours'); + let mockUser: User; + + beforeEach(() => { + mockUser = Object.assign(new User(), { + id: uuid(), + password: 'passwordHash', + mfaEnabled: false, + mfaSecret: 'test', + mfaRecoveryCodes: ['test'], + updatedAt: new Date(), + authIdentities: [], + }); + }); + + afterEach(() => { + config.set('userManagement.jwtSessionDuration', oldDuration); + }); + + it('does not do anything if the user is not authorized', async () => { + const req = {} as AuthenticatedRequest; + const res = { cookie: jest.fn() } as unknown as Response; + const next = jest.fn(); + + await refreshExpiringCookie(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).not.toHaveBeenCalled(); + }); + + it('does not do anything if the cookie is still valid', async () => { + config.set('userManagement.jwtSessionDurationHours', 73); + const { token } = issueJWT(mockUser); + + const req = { cookies: { [AUTH_COOKIE_NAME]: token }, user: mockUser } as AuthenticatedRequest; + const res = { cookie: jest.fn() } as unknown as Response; + const next = jest.fn(); + await refreshExpiringCookie(req, res, next); + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).not.toHaveBeenCalled(); + }); + + it('refreshes the cookie if it has less than 3 days to live', async () => { + config.set('userManagement.jwtSessionDurationHours', 71); + const { token } = issueJWT(mockUser); + const req = { cookies: { [AUTH_COOKIE_NAME]: token }, user: mockUser } as AuthenticatedRequest; + const res = { cookie: jest.fn() } as unknown as Response; + const next = jest.fn(); + + await refreshExpiringCookie(req, res, next); + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).toHaveBeenCalledTimes(1); + }); +}); From 567766b759a4dcb3a5cb2496f79728b5312291bc Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:34 +0100 Subject: [PATCH 07/21] remove TODO commment --- packages/cli/src/config/schema.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 8fe8de3ef393f..11ae088ca772e 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -762,7 +762,6 @@ export const schema = { default: '', env: 'N8N_USER_MANAGEMENT_JWT_SECRET', }, - // TODO: Is it ok to add units here? jwtSessionDurationHours: { doc: 'Set a specific expiration date for the JWTs in hours.', format: Number, From 2509261f6fea74b3dfa5890e38fad6d9b55303de Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:40 +0100 Subject: [PATCH 08/21] use Time constant to convert between units of time --- packages/cli/src/auth/jwt.ts | 7 ++++--- packages/cli/src/constants.ts | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/auth/jwt.ts b/packages/cli/src/auth/jwt.ts index 009f8b1bbfbdf..857b01d691656 100644 --- a/packages/cli/src/auth/jwt.ts +++ b/packages/cli/src/auth/jwt.ts @@ -1,6 +1,6 @@ import type { Response } from 'express'; import { createHash } from 'crypto'; -import { AUTH_COOKIE_NAME, RESPONSE_ERROR_MESSAGES } from '@/constants'; +import { AUTH_COOKIE_NAME, RESPONSE_ERROR_MESSAGES, Time } from '@/constants'; import type { JwtPayload, JwtToken } from '@/Interfaces'; import type { User } from '@db/entities/User'; import config from '@/config'; @@ -14,8 +14,9 @@ import { ApplicationError } from 'n8n-workflow'; export function issueJWT(user: User): JwtToken { const { id, email, password } = user; - const expiresInHours = config.get('userManagement.jwtSessionDurationHours'); - const expiresInSeconds = expiresInHours * 60 * 60; + const expiresInHours = config.getEnv('userManagement.jwtSessionDurationHours'); + const expiresInSeconds = expiresInHours * Time.hours.toSeconds; + const isWithinUsersLimit = Container.get(License).isWithinUsersLimit(); const payload: JwtPayload = { diff --git a/packages/cli/src/constants.ts b/packages/cli/src/constants.ts index 8603d2996330d..5b0e87ca22260 100644 --- a/packages/cli/src/constants.ts +++ b/packages/cli/src/constants.ts @@ -103,6 +103,7 @@ export const UM_FIX_INSTRUCTION = /** * Units of time in milliseconds + * @deprecated Please use constants.Time instead. */ export const TIME = { SECOND: 1000, @@ -111,6 +112,25 @@ export const TIME = { DAY: 24 * 60 * 60 * 1000, } as const; +/** + * Convert time from any unit to any other unit + * + * Please amend conversions as necessary. + * Eventually this will superseed `TIME` above + */ +export const Time = { + minutes: { + toMilliseconds: 60 * 1000, + }, + hours: { + toMilliseconds: 60 * 60 * 1000, + toSeconds: 60 * 60, + }, + days: { + toSeconds: 24 * 60 * 60, + }, +}; + export const MIN_PASSWORD_CHAR_LENGTH = 8; export const MAX_PASSWORD_CHAR_LENGTH = 64; From 45c90c6eccb2e249591e8051bb6594ea7fdf25c9 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:42 +0100 Subject: [PATCH 09/21] add jwtRefreshTimeoutHours and test that it resets to 0 if it's larger than jwtSessionDurationHours --- packages/cli/src/config/index.ts | 12 ++++++++++++ packages/cli/src/config/schema.ts | 6 ++++++ packages/cli/test/unit/config/index.test.ts | 9 +++++++++ 3 files changed, 27 insertions(+) create mode 100644 packages/cli/test/unit/config/index.test.ts diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index 5fa96e75aba82..d800ee391fe61 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -62,9 +62,21 @@ if (!inE2ETests && !inTest) { }); } +// Validate Configuration config.validate({ allowed: 'strict', }); +const userManagement = config.get('userManagement'); +if (userManagement.jwtRefreshTimeoutHours >= userManagement.jwtSessionDurationHours) { + // Can't use the logger here, because it depends on the config. + // I could validate it in `Start.init()`, but that feels like the wrong seperation of concerns + // I could also do it in `refreshExpiringCookie`, but that would then print the message on every request. + console.warn( + 'N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS needs to smaller than N8N_USER_MANAGEMENT_JWT_DURATION_HOURS. Setting N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS to 0 for now.', + ); + + config.set('userManagement.jwtRefreshTimeoutHours', 0); +} setGlobalState({ defaultTimezone: config.getEnv('generic.timezone'), diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 11ae088ca772e..8d523fb8ad2a7 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -768,6 +768,12 @@ export const schema = { default: 168, env: 'N8N_USER_MANAGEMENT_JWT_DURATION_HOURS', }, + jwtRefreshTimeoutHours: { + doc: '', + format: Number, + default: 0, + env: 'N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS', + }, isInstanceOwnerSetUp: { // n8n loads this setting from DB on startup doc: "Whether the instance owner's account has been set up", diff --git a/packages/cli/test/unit/config/index.test.ts b/packages/cli/test/unit/config/index.test.ts new file mode 100644 index 0000000000000..d49a4e0a3c3ef --- /dev/null +++ b/packages/cli/test/unit/config/index.test.ts @@ -0,0 +1,9 @@ +describe('userManagement.jwtRefreshTimeoutHours', () => { + it("resets jwtRefreshTimeoutHours to 0 if it's greater than or equal to jwtSessionDurationHours", async () => { + process.env.N8N_USER_MANAGEMENT_JWT_DURATION_HOURS = '1'; + process.env.N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS = '1'; + const { default: config } = await import('@/config'); + + expect(config.getEnv('userManagement.jwtRefreshTimeoutHours')).toBe(0); + }); +}); From 7543c9abe2074d7f9da5f77b3d914bef8936d318 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:44 +0100 Subject: [PATCH 10/21] clean up test 1. use Time constant for more readable time conversion 2. use jest-mock-extended to create users and requests 3. use type guards and jest.fail to remove unecessary assertions --- packages/cli/test/unit/auth/jwt.test.ts | 55 +++++++++---------------- 1 file changed, 19 insertions(+), 36 deletions(-) diff --git a/packages/cli/test/unit/auth/jwt.test.ts b/packages/cli/test/unit/auth/jwt.test.ts index bb106c46c1c61..4419bfe51934f 100644 --- a/packages/cli/test/unit/auth/jwt.test.ts +++ b/packages/cli/test/unit/auth/jwt.test.ts @@ -1,55 +1,45 @@ import { Container } from 'typedi'; -import { v4 as uuid } from 'uuid'; +import { mock } from 'jest-mock-extended'; import config from '@/config'; import { JwtService } from '@/services/jwt.service'; import { License } from '@/License'; -import { User } from '@db/entities/User'; +import { Time } from '@/constants'; import { issueJWT } from '@/auth/jwt'; import { mockInstance } from '../../shared/mocking'; -// TODO: Why do I need to mock the License but not for example the JwtService 🤷 -// A: because of the server not being initialized +import type { User } from '@db/entities/User'; + mockInstance(License); -// TODO: is the the right level of testing? -// Should I mock more or less? -// Should I write an integration test that checks that a user is actually logged out? describe('jwt.issueJWT', () => { const jwtService = Container.get(JwtService); describe('when not setting userManagement.jwtSessionDuration', () => { it('should default to expire in 168 hours', () => { - const defaultInSeconds = 7 * 24 * 60 * 60; - // TODO: is this the right way to create a mock user? - const mockUser = Object.assign(new User(), { - id: uuid(), - password: 'passwordHash', - mfaEnabled: false, - mfaSecret: 'test', - mfaRecoveryCodes: ['test'], - updatedAt: new Date(), - authIdentities: [], - }); + const defaultInSeconds = 7 * Time.days.toSeconds; + const mockUser = mock({ password: 'passwordHash' }); const { token, expiresInSeconds } = issueJWT(mockUser); expect(expiresInSeconds).toBe(defaultInSeconds); const decodedToken = jwtService.verify(token); - expect(decodedToken.exp).toBeDefined(); - expect(decodedToken.iat).toBeDefined(); - expect(decodedToken.exp! - decodedToken.iat!).toBe(defaultInSeconds); + if (decodedToken.exp === undefined || decodedToken.iat === undefined) { + fail('Expected exp and iat to be defined'); + } + + expect(decodedToken.exp - decodedToken.iat).toBe(defaultInSeconds); }); }); describe('when setting userManagement.jwtSessionDuration', () => { let oldDuration: number; - let testDuration = 1; + let testDurationHours = 1; beforeEach(() => { mockInstance(License); oldDuration = config.get('userManagement.jwtSessionDurationHours'); - config.set('userManagement.jwtSessionDurationHours', testDuration); + config.set('userManagement.jwtSessionDurationHours', testDurationHours); }); afterEach(() => { @@ -57,22 +47,15 @@ describe('jwt.issueJWT', () => { }); it('should apply it to tokens', () => { - const mockUser = Object.assign(new User(), { - id: uuid(), - password: 'passwordHash', - mfaEnabled: false, - mfaSecret: 'test', - mfaRecoveryCodes: ['test'], - updatedAt: new Date(), - authIdentities: [], - }); + const mockUser = mock({ password: 'passwordHash' }); const { token, expiresInSeconds } = issueJWT(mockUser); - expect(expiresInSeconds).toBe(testDuration * 60 * 60); + expect(expiresInSeconds).toBe(testDurationHours * Time.hours.toSeconds); const decodedToken = jwtService.verify(token); - expect(decodedToken.exp).toBeDefined(); - expect(decodedToken.iat).toBeDefined(); - expect(decodedToken.exp! - decodedToken.iat!).toBe(testDuration * 60 * 60); + if (decodedToken.exp === undefined || decodedToken.iat === undefined) { + fail('Expected exp and iat to be defined on decodedToken'); + } + expect(decodedToken.exp - decodedToken.iat).toBe(testDurationHours * Time.hours.toSeconds); }); }); }); From 4e9bd04e62f51b8d50cd823ca33844286165a603 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:45 +0100 Subject: [PATCH 11/21] make the refresh timeout configurable --- packages/cli/src/middlewares/auth.ts | 18 ++- .../cli/test/unit/middleware/auth.test.ts | 151 ++++++++++++++---- 2 files changed, 134 insertions(+), 35 deletions(-) diff --git a/packages/cli/src/middlewares/auth.ts b/packages/cli/src/middlewares/auth.ts index 7a61072fdcda0..4f4c220d3b9fe 100644 --- a/packages/cli/src/middlewares/auth.ts +++ b/packages/cli/src/middlewares/auth.ts @@ -11,6 +11,7 @@ import { issueCookie, resolveJwtContent } from '@/auth/jwt'; import { canSkipAuth } from '@/decorators/registerController'; import { Logger } from '@/Logger'; import { JwtService } from '@/services/jwt.service'; +import config from '@/config'; const jwtFromRequest = (req: Request) => { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access @@ -42,10 +43,23 @@ const userManagementJwtAuth = (): RequestHandler => { * middleware to refresh cookie before it expires */ export const refreshExpiringCookie = (async (req: AuthenticatedRequest, res, next) => { + const jwtRefreshTimeoutHours = config.get('userManagement.jwtRefreshTimeoutHours'); + + let jwtRefreshTimeoutMilliSeconds: number; + + if (jwtRefreshTimeoutHours === 0) { + const jwtSessionDurationHours = config.get('userManagement.jwtSessionDurationHours'); + + jwtRefreshTimeoutMilliSeconds = Math.floor(jwtSessionDurationHours * 0.25 * 60 * 60 * 1000); + } else { + jwtRefreshTimeoutMilliSeconds = Math.floor(jwtRefreshTimeoutHours * 60 * 60 * 1000); + } + const cookieAuth = jwtFromRequest(req); - if (cookieAuth && req.user) { + + if (cookieAuth && req.user && jwtRefreshTimeoutHours !== -1) { const cookieContents = jwt.decode(cookieAuth) as JwtPayload & { exp: number }; - if (cookieContents.exp * 1000 - Date.now() < 259200000) { + if (cookieContents.exp * 1000 - Date.now() < jwtRefreshTimeoutMilliSeconds) { // if cookie expires in < 3 days, renew it. await issueCookie(res, req.user); } diff --git a/packages/cli/test/unit/middleware/auth.test.ts b/packages/cli/test/unit/middleware/auth.test.ts index 2eac435e85bbb..38b8c11e59d39 100644 --- a/packages/cli/test/unit/middleware/auth.test.ts +++ b/packages/cli/test/unit/middleware/auth.test.ts @@ -1,42 +1,37 @@ -import { v4 as uuid } from 'uuid'; +import { mock } from 'jest-mock-extended'; import config from '@/config'; -import { AUTH_COOKIE_NAME } from '@/constants'; +import { AUTH_COOKIE_NAME, Time } from '@/constants'; import { License } from '@/License'; -import { User } from '@/databases/entities/User'; import { issueJWT } from '@/auth/jwt'; import { refreshExpiringCookie } from '@/middlewares'; import { mockInstance } from '../../shared/mocking'; import type { AuthenticatedRequest } from '@/requests'; -import type { Response } from 'express'; +import type { NextFunction, Response } from 'express'; +import type { User } from '@/databases/entities/User'; mockInstance(License); +jest.useFakeTimers(); + describe('refreshExpiringCookie', () => { const oldDuration: number = config.get('userManagement.jwtSessionDurationHours'); let mockUser: User; beforeEach(() => { - mockUser = Object.assign(new User(), { - id: uuid(), - password: 'passwordHash', - mfaEnabled: false, - mfaSecret: 'test', - mfaRecoveryCodes: ['test'], - updatedAt: new Date(), - authIdentities: [], - }); + mockUser = mock({ password: 'passwordHash' }); }); afterEach(() => { config.set('userManagement.jwtSessionDuration', oldDuration); + config.set('userManagement.jwtRefreshTimeoutHours', 0); }); it('does not do anything if the user is not authorized', async () => { - const req = {} as AuthenticatedRequest; - const res = { cookie: jest.fn() } as unknown as Response; + const req = mock(); + const res = mock({ cookie: jest.fn() }); const next = jest.fn(); await refreshExpiringCookie(req, res, next); @@ -45,27 +40,117 @@ describe('refreshExpiringCookie', () => { expect(res.cookie).not.toHaveBeenCalled(); }); - it('does not do anything if the cookie is still valid', async () => { - config.set('userManagement.jwtSessionDurationHours', 73); - const { token } = issueJWT(mockUser); + // TODO: structure tests + describe('with N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS=-1', () => { + it('does not refresh the cookie, ever', async () => { + config.set('userManagement.jwtSessionDurationHours', 1); + config.set('userManagement.jwtRefreshTimeoutHours', -1); + const { token } = issueJWT(mockUser); - const req = { cookies: { [AUTH_COOKIE_NAME]: token }, user: mockUser } as AuthenticatedRequest; - const res = { cookie: jest.fn() } as unknown as Response; - const next = jest.fn(); - await refreshExpiringCookie(req, res, next); - expect(next).toHaveBeenCalledTimes(1); - expect(res.cookie).not.toHaveBeenCalled(); + jest.advanceTimersByTime(1000 * 60 * 55); /* 55 minutes */ + + const req = mock({ + cookies: { [AUTH_COOKIE_NAME]: token }, + user: mockUser, + }); + const res = mock({ cookie: jest.fn() }); + const next = jest.fn(); + await refreshExpiringCookie(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).not.toHaveBeenCalled(); + }); }); - it('refreshes the cookie if it has less than 3 days to live', async () => { - config.set('userManagement.jwtSessionDurationHours', 71); - const { token } = issueJWT(mockUser); - const req = { cookies: { [AUTH_COOKIE_NAME]: token }, user: mockUser } as AuthenticatedRequest; - const res = { cookie: jest.fn() } as unknown as Response; - const next = jest.fn(); + describe('with N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS=0', () => { + let token: string; + let req: AuthenticatedRequest; + let res: Response; + let next: NextFunction; - await refreshExpiringCookie(req, res, next); - expect(next).toHaveBeenCalledTimes(1); - expect(res.cookie).toHaveBeenCalledTimes(1); + beforeEach(() => { + // ARRANGE + config.set('userManagement.jwtSessionDurationHours', 1); + config.set('userManagement.jwtRefreshTimeoutHours', 0); + token = issueJWT(mockUser).token; + + req = mock({ + cookies: { [AUTH_COOKIE_NAME]: token }, + user: mockUser, + }); + res = mock({ cookie: jest.fn() }); + next = jest.fn(); + }); + + it('does not refresh the cookie when more than 1/4th of time is left', async () => { + // ARRANGE + jest.advanceTimersByTime(44 * Time.minutes.toMilliseconds); /* 44 minutes */ + + // ACT + await refreshExpiringCookie(req, res, next); + + // ASSERT + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).not.toHaveBeenCalled(); + }); + + it('refreshes the cookie when 1/4th of time is left', async () => { + // ARRANGE + jest.advanceTimersByTime(46 * Time.minutes.toMilliseconds); /* 46 minutes */ + + // ACT + await refreshExpiringCookie(req, res, next); + + // ASSERT + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).toHaveBeenCalledTimes(1); + }); + }); + + describe('with N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS=50', () => { + let token: string; + let req: AuthenticatedRequest; + let res: Response; + let next: NextFunction; + + // ARRANGE + beforeEach(() => { + config.set('userManagement.jwtSessionDurationHours', 51); + config.set('userManagement.jwtRefreshTimeoutHours', 50); + + token = issueJWT(mockUser).token; + req = mock({ + cookies: { [AUTH_COOKIE_NAME]: token }, + user: mockUser, + }); + res = mock({ cookie: jest.fn() }); + next = jest.fn(); + }); + + it('does not do anything if the cookie is still valid', async () => { + // ARRANGE + // cookie has 50.5 hours to live: 51 - 0.5 + jest.advanceTimersByTime(30 * Time.minutes.toMilliseconds); + + // ACT + await refreshExpiringCookie(req, res, next); + + // ASSERT + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).not.toHaveBeenCalled(); + }); + + it('refreshes the cookie if it has less than 50 hours to live', async () => { + // ARRANGE + // cookie has 49.5 hours to live: 51 - 1.5 + jest.advanceTimersByTime(1.5 * Time.hours.toMilliseconds); + + // ACT + await refreshExpiringCookie(req, res, next); + + // ASSERT + expect(next).toHaveBeenCalledTimes(1); + expect(res.cookie).toHaveBeenCalledTimes(1); + }); }); }); From 928a3ac5a4f5123ee9518356ff5c2290195e8300 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:47 +0100 Subject: [PATCH 12/21] remove comment I'll add this as a comment in the review, where we can agree on where the warning should be emitted. --- packages/cli/src/config/index.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index d800ee391fe61..7300b0dcf739f 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -68,9 +68,6 @@ config.validate({ }); const userManagement = config.get('userManagement'); if (userManagement.jwtRefreshTimeoutHours >= userManagement.jwtSessionDurationHours) { - // Can't use the logger here, because it depends on the config. - // I could validate it in `Start.init()`, but that feels like the wrong seperation of concerns - // I could also do it in `refreshExpiringCookie`, but that would then print the message on every request. console.warn( 'N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS needs to smaller than N8N_USER_MANAGEMENT_JWT_DURATION_HOURS. Setting N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS to 0 for now.', ); From bf3df406f284f49a0e42783f448882a872e8dbd1 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:48 +0100 Subject: [PATCH 13/21] remove TODO comment --- packages/cli/src/Interfaces.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts index 662bbb6e1028c..bef298b6d2cff 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -667,7 +667,6 @@ export interface ILicensePostResponse extends ILicenseReadResponse { export interface JwtToken { token: string; - // TODO: is it ok to rename this? expiresInSeconds: number; } From a76c0717ea4e3b94851897cdcb7280c77a3938e0 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:49 +0100 Subject: [PATCH 14/21] don't hard code default --- packages/cli/test/unit/middleware/auth.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cli/test/unit/middleware/auth.test.ts b/packages/cli/test/unit/middleware/auth.test.ts index 38b8c11e59d39..251308fbc7592 100644 --- a/packages/cli/test/unit/middleware/auth.test.ts +++ b/packages/cli/test/unit/middleware/auth.test.ts @@ -17,7 +17,8 @@ mockInstance(License); jest.useFakeTimers(); describe('refreshExpiringCookie', () => { - const oldDuration: number = config.get('userManagement.jwtSessionDurationHours'); + const oldDuration = config.getEnv('userManagement.jwtSessionDurationHours'); + const oldTimeout = config.getEnv('userManagement.jwtRefreshTimeoutHours'); let mockUser: User; beforeEach(() => { @@ -26,7 +27,7 @@ describe('refreshExpiringCookie', () => { afterEach(() => { config.set('userManagement.jwtSessionDuration', oldDuration); - config.set('userManagement.jwtRefreshTimeoutHours', 0); + config.set('userManagement.jwtRefreshTimeoutHours', oldTimeout); }); it('does not do anything if the user is not authorized', async () => { From fa4dee9e94faebb84a8a14a0f6196472820ad720 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:51 +0100 Subject: [PATCH 15/21] don't use 2 different units in the test title and in the test's code hours vs days --- packages/cli/test/unit/auth/jwt.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/test/unit/auth/jwt.test.ts b/packages/cli/test/unit/auth/jwt.test.ts index 4419bfe51934f..f5bcfb11d2d4f 100644 --- a/packages/cli/test/unit/auth/jwt.test.ts +++ b/packages/cli/test/unit/auth/jwt.test.ts @@ -17,7 +17,7 @@ describe('jwt.issueJWT', () => { const jwtService = Container.get(JwtService); describe('when not setting userManagement.jwtSessionDuration', () => { - it('should default to expire in 168 hours', () => { + it('should default to expire in 7 days', () => { const defaultInSeconds = 7 * Time.days.toSeconds; const mockUser = mock({ password: 'passwordHash' }); const { token, expiresInSeconds } = issueJWT(mockUser); From 1762e686e15af58894e568e23a32d1215631d48f Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:52 +0100 Subject: [PATCH 16/21] don't recalculate the same value multiple times --- packages/cli/test/unit/auth/jwt.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/cli/test/unit/auth/jwt.test.ts b/packages/cli/test/unit/auth/jwt.test.ts index f5bcfb11d2d4f..ce60628a18b41 100644 --- a/packages/cli/test/unit/auth/jwt.test.ts +++ b/packages/cli/test/unit/auth/jwt.test.ts @@ -33,12 +33,12 @@ describe('jwt.issueJWT', () => { }); describe('when setting userManagement.jwtSessionDuration', () => { - let oldDuration: number; - let testDurationHours = 1; + const oldDuration = config.get('userManagement.jwtSessionDurationHours'); + const testDurationHours = 1; + const testDurationSeconds = testDurationHours * Time.hours.toSeconds; beforeEach(() => { mockInstance(License); - oldDuration = config.get('userManagement.jwtSessionDurationHours'); config.set('userManagement.jwtSessionDurationHours', testDurationHours); }); @@ -50,12 +50,12 @@ describe('jwt.issueJWT', () => { const mockUser = mock({ password: 'passwordHash' }); const { token, expiresInSeconds } = issueJWT(mockUser); - expect(expiresInSeconds).toBe(testDurationHours * Time.hours.toSeconds); + expect(expiresInSeconds).toBe(testDurationSeconds); const decodedToken = jwtService.verify(token); if (decodedToken.exp === undefined || decodedToken.iat === undefined) { fail('Expected exp and iat to be defined on decodedToken'); } - expect(decodedToken.exp - decodedToken.iat).toBe(testDurationHours * Time.hours.toSeconds); + expect(decodedToken.exp - decodedToken.iat).toBe(testDurationSeconds); }); }); }); From 24472c158d2761b73fbddb65b046009f334e1a02 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 15:07:56 +0100 Subject: [PATCH 17/21] remove todo comment --- packages/cli/test/unit/middleware/auth.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/test/unit/middleware/auth.test.ts b/packages/cli/test/unit/middleware/auth.test.ts index 251308fbc7592..f3b4db4b04b9c 100644 --- a/packages/cli/test/unit/middleware/auth.test.ts +++ b/packages/cli/test/unit/middleware/auth.test.ts @@ -41,7 +41,6 @@ describe('refreshExpiringCookie', () => { expect(res.cookie).not.toHaveBeenCalled(); }); - // TODO: structure tests describe('with N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS=-1', () => { it('does not refresh the cookie, ever', async () => { config.set('userManagement.jwtSessionDurationHours', 1); From 27ad95e6868ba0e27e780b5b9de6152b9284b553 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 17:21:53 +0100 Subject: [PATCH 18/21] fix: maxAge needs to be milliseconds See: https://expressjs.com/en/api.html#res.cookie --- packages/cli/src/auth/jwt.ts | 2 +- packages/cli/src/constants.ts | 3 +++ packages/cli/test/unit/middleware/auth.test.ts | 8 +++++++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/auth/jwt.ts b/packages/cli/src/auth/jwt.ts index 857b01d691656..daee8371ae4fd 100644 --- a/packages/cli/src/auth/jwt.ts +++ b/packages/cli/src/auth/jwt.ts @@ -88,7 +88,7 @@ export async function resolveJwt(token: string): Promise { export async function issueCookie(res: Response, user: User): Promise { const userData = issueJWT(user); res.cookie(AUTH_COOKIE_NAME, userData.token, { - maxAge: userData.expiresInSeconds, + maxAge: userData.expiresInSeconds * Time.seconds.toMilliseconds, httpOnly: true, sameSite: 'lax', }); diff --git a/packages/cli/src/constants.ts b/packages/cli/src/constants.ts index 5b0e87ca22260..9e65e4c342bfa 100644 --- a/packages/cli/src/constants.ts +++ b/packages/cli/src/constants.ts @@ -119,6 +119,9 @@ export const TIME = { * Eventually this will superseed `TIME` above */ export const Time = { + seconds: { + toMilliseconds: 1000, + }, minutes: { toMilliseconds: 60 * 1000, }, diff --git a/packages/cli/test/unit/middleware/auth.test.ts b/packages/cli/test/unit/middleware/auth.test.ts index f3b4db4b04b9c..dfd6e0e8c13e4 100644 --- a/packages/cli/test/unit/middleware/auth.test.ts +++ b/packages/cli/test/unit/middleware/auth.test.ts @@ -108,6 +108,7 @@ describe('refreshExpiringCookie', () => { }); describe('with N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS=50', () => { + const jwtSessionDurationHours = 51; let token: string; let req: AuthenticatedRequest; let res: Response; @@ -115,7 +116,7 @@ describe('refreshExpiringCookie', () => { // ARRANGE beforeEach(() => { - config.set('userManagement.jwtSessionDurationHours', 51); + config.set('userManagement.jwtSessionDurationHours', jwtSessionDurationHours); config.set('userManagement.jwtRefreshTimeoutHours', 50); token = issueJWT(mockUser).token; @@ -151,6 +152,11 @@ describe('refreshExpiringCookie', () => { // ASSERT expect(next).toHaveBeenCalledTimes(1); expect(res.cookie).toHaveBeenCalledTimes(1); + expect(res.cookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME, expect.any(String), { + httpOnly: true, + maxAge: jwtSessionDurationHours * Time.hours.toMilliseconds, + sameSite: 'lax', + }); }); }); }); From d9b6140ed0694de2badd5e1d384ab5f31935420c Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 18:03:55 +0100 Subject: [PATCH 19/21] remove outdated comment --- packages/cli/src/middlewares/auth.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/src/middlewares/auth.ts b/packages/cli/src/middlewares/auth.ts index 4f4c220d3b9fe..12c1d78d655dc 100644 --- a/packages/cli/src/middlewares/auth.ts +++ b/packages/cli/src/middlewares/auth.ts @@ -60,7 +60,6 @@ export const refreshExpiringCookie = (async (req: AuthenticatedRequest, res, nex if (cookieAuth && req.user && jwtRefreshTimeoutHours !== -1) { const cookieContents = jwt.decode(cookieAuth) as JwtPayload & { exp: number }; if (cookieContents.exp * 1000 - Date.now() < jwtRefreshTimeoutMilliSeconds) { - // if cookie expires in < 3 days, renew it. await issueCookie(res, req.user); } } From 29c4496a792d2398c923371af84590b1f7c8adfa Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 22:41:48 +0100 Subject: [PATCH 20/21] use JSDoc to document the unit instead of suffixing the variable name --- packages/cli/src/Interfaces.ts | 3 ++- packages/cli/src/auth/jwt.ts | 4 ++-- packages/cli/test/unit/auth/jwt.test.ts | 8 ++++---- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts index bef298b6d2cff..5dcf0272f92c2 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -667,7 +667,8 @@ export interface ILicensePostResponse extends ILicenseReadResponse { export interface JwtToken { token: string; - expiresInSeconds: number; + /** The amount of seconds after which the JWT will expire. **/ + expiresIn: number; } export interface JwtPayload { diff --git a/packages/cli/src/auth/jwt.ts b/packages/cli/src/auth/jwt.ts index daee8371ae4fd..cf2a415b75196 100644 --- a/packages/cli/src/auth/jwt.ts +++ b/packages/cli/src/auth/jwt.ts @@ -44,7 +44,7 @@ export function issueJWT(user: User): JwtToken { return { token: signedToken, - expiresInSeconds, + expiresIn: expiresInSeconds, }; } @@ -88,7 +88,7 @@ export async function resolveJwt(token: string): Promise { export async function issueCookie(res: Response, user: User): Promise { const userData = issueJWT(user); res.cookie(AUTH_COOKIE_NAME, userData.token, { - maxAge: userData.expiresInSeconds * Time.seconds.toMilliseconds, + maxAge: userData.expiresIn * Time.seconds.toMilliseconds, httpOnly: true, sameSite: 'lax', }); diff --git a/packages/cli/test/unit/auth/jwt.test.ts b/packages/cli/test/unit/auth/jwt.test.ts index ce60628a18b41..dd9c65116ab77 100644 --- a/packages/cli/test/unit/auth/jwt.test.ts +++ b/packages/cli/test/unit/auth/jwt.test.ts @@ -20,9 +20,9 @@ describe('jwt.issueJWT', () => { it('should default to expire in 7 days', () => { const defaultInSeconds = 7 * Time.days.toSeconds; const mockUser = mock({ password: 'passwordHash' }); - const { token, expiresInSeconds } = issueJWT(mockUser); + const { token, expiresIn } = issueJWT(mockUser); - expect(expiresInSeconds).toBe(defaultInSeconds); + expect(expiresIn).toBe(defaultInSeconds); const decodedToken = jwtService.verify(token); if (decodedToken.exp === undefined || decodedToken.iat === undefined) { fail('Expected exp and iat to be defined'); @@ -48,9 +48,9 @@ describe('jwt.issueJWT', () => { it('should apply it to tokens', () => { const mockUser = mock({ password: 'passwordHash' }); - const { token, expiresInSeconds } = issueJWT(mockUser); + const { token, expiresIn } = issueJWT(mockUser); - expect(expiresInSeconds).toBe(testDurationSeconds); + expect(expiresIn).toBe(testDurationSeconds); const decodedToken = jwtService.verify(token); if (decodedToken.exp === undefined || decodedToken.iat === undefined) { fail('Expected exp and iat to be defined on decodedToken'); From 01e15359f97f4d377def0157827cc570bedfac4c Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 19 Jan 2024 23:02:35 +0100 Subject: [PATCH 21/21] add missing documentation --- packages/cli/src/config/schema.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 8d523fb8ad2a7..8b77f36cb3d0d 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -769,7 +769,7 @@ export const schema = { env: 'N8N_USER_MANAGEMENT_JWT_DURATION_HOURS', }, jwtRefreshTimeoutHours: { - doc: '', + doc: 'How long before the JWT expires to automatically refresh it. 0 means 25% of N8N_USER_MANAGEMENT_JWT_DURATION_HOURS. -1 means it will never refresh, which forces users to login again after the defined period in N8N_USER_MANAGEMENT_JWT_DURATION_HOURS.', format: Number, default: 0, env: 'N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS',