-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): Custom session timeout and refresh configuration #8342
Changes from all commits
638a6be
00f337c
a9a6749
19f43b2
067f433
fd55dfe
567766b
2509261
45c90c6
7543c9a
4e9bd04
928a3ac
bf3df40
a76c071
fa4dee9
1762e68
24472c1
27ad95e
d9b6140
29c4496
01e1535
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,28 @@ 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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I talked with @ivov about this and he likes it. TIME only allowed converting everything into milliseconds, but I was working with JWT and cookies which all expect seconds, so I thought this here is a bit more versatile. We can also add a library for this, but the problem seems a bit too trivial to justify adding third party code to make the conversions more readable. Feel free to disagree and comment. |
||
seconds: { | ||
toMilliseconds: 1000, | ||
}, | ||
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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -41,17 +42,29 @@ 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 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); | ||
} | ||
Comment on lines
+50
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to use a ternary here instead. Just let me know. |
||
|
||
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 cookie expires in < 3 days, renew it. | ||
if (cookieContents.exp * 1000 - Date.now() < jwtRefreshTimeoutMilliSeconds) { | ||
await issueCookie(res, req.user); | ||
} | ||
} | ||
next(); | ||
}; | ||
}) satisfies RequestHandler; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to wrangle the types a bit. I want to call Right now adding the type to the variable binding hides that fact. My workaround for that is to let TS infer the type of Happy to apply other solutions to this like extending the RequestHandler type Async<T extends (...args: unknown[]) => unknown> = (
...args: Parameters<T>
) => Promise<ReturnType<T>>; Let me know what you think. |
||
|
||
const passportMiddleware = passport.authenticate('jwt', { session: false }) as RequestHandler; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import { Container } from 'typedi'; | ||
import { mock } from 'jest-mock-extended'; | ||
|
||
import config from '@/config'; | ||
import { JwtService } from '@/services/jwt.service'; | ||
import { License } from '@/License'; | ||
import { Time } from '@/constants'; | ||
import { issueJWT } from '@/auth/jwt'; | ||
|
||
import { mockInstance } from '../../shared/mocking'; | ||
|
||
import type { User } from '@db/entities/User'; | ||
|
||
mockInstance(License); | ||
|
||
describe('jwt.issueJWT', () => { | ||
const jwtService = Container.get(JwtService); | ||
|
||
describe('when not setting userManagement.jwtSessionDuration', () => { | ||
it('should default to expire in 7 days', () => { | ||
const defaultInSeconds = 7 * Time.days.toSeconds; | ||
const mockUser = mock<User>({ password: 'passwordHash' }); | ||
const { token, expiresIn } = issueJWT(mockUser); | ||
|
||
expect(expiresIn).toBe(defaultInSeconds); | ||
const decodedToken = jwtService.verify(token); | ||
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', () => { | ||
const oldDuration = config.get('userManagement.jwtSessionDurationHours'); | ||
const testDurationHours = 1; | ||
const testDurationSeconds = testDurationHours * Time.hours.toSeconds; | ||
|
||
beforeEach(() => { | ||
mockInstance(License); | ||
config.set('userManagement.jwtSessionDurationHours', testDurationHours); | ||
}); | ||
|
||
afterEach(() => { | ||
config.set('userManagement.jwtSessionDuration', oldDuration); | ||
}); | ||
|
||
it('should apply it to tokens', () => { | ||
const mockUser = mock<User>({ password: 'passwordHash' }); | ||
const { token, expiresIn } = issueJWT(mockUser); | ||
|
||
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'); | ||
} | ||
expect(decodedToken.exp - decodedToken.iat).toBe(testDurationSeconds); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't use the `Container.get(Logger) here, because it depends on the config, which creates a dependency cycle.
Alternative:
Start.init()
, but that feels like the wrong separation of concernsrefreshExpiringCookie
, but that would then print the message on every request.