From f463787289dde99ae8ea81de8840496d8cf809bb Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Mon, 27 Jan 2025 14:16:16 +0000 Subject: [PATCH 1/3] Disallow concurrent sessions by only allowing the latest. Tracks a new 'flowId' and declines any cookie authentication with an older value. --- .../src/common/constants/session-names.js | 3 +- .../src/common/helpers/auth/session-cookie.js | 31 +++++++++++++++++-- .../src/common/helpers/auth/user-session.js | 2 ++ designer/server/src/routes/account/auth.js | 5 ++- designer/server/src/typings/hapi/index.d.ts | 11 +++++++ designer/server/test/fixtures/auth.js | 3 +- package-lock.json | 30 +++++++++++++----- package.json | 3 ++ 8 files changed, 75 insertions(+), 13 deletions(-) diff --git a/designer/server/src/common/constants/session-names.js b/designer/server/src/common/constants/session-names.js index 2380e5bc7..d66ae3640 100644 --- a/designer/server/src/common/constants/session-names.js +++ b/designer/server/src/common/constants/session-names.js @@ -18,5 +18,6 @@ export const sessionNames = { ) }, successNotification: /** @type {const} */ ('successNotification'), - errorList: /** @type {const} */ ('errorList') + errorList: /** @type {const} */ ('errorList'), + forceSignOut: /** @type {const} */ ('forceSignOut') } diff --git a/designer/server/src/common/helpers/auth/session-cookie.js b/designer/server/src/common/helpers/auth/session-cookie.js index d55db84c0..00da25ea5 100644 --- a/designer/server/src/common/helpers/auth/session-cookie.js +++ b/designer/server/src/common/helpers/auth/session-cookie.js @@ -8,8 +8,11 @@ import { } from '~/src/common/helpers/auth/get-user-session.js' import { refreshAccessToken } from '~/src/common/helpers/auth/refresh-token.js' import { createUserSession } from '~/src/common/helpers/auth/user-session.js' +import { createLogger } from '~/src/common/helpers/logging/logger.js' import config from '~/src/config.js' +const logger = createLogger() + /** * @type {ServerRegisterPluginObject} */ @@ -37,6 +40,9 @@ const sessionCookie = { * Redirect invalid session to callback route */ redirectTo(request) { + const forceSignOut = + request?.yar.flash(sessionNames.forceSignOut).at(0) ?? false + if (request) { const { url, yar } = request @@ -44,14 +50,15 @@ const sessionCookie = { yar.flash(sessionNames.redirectTo, url.pathname) } - // Redirect to callback route - return '/auth/callback' + // If we're forcing them out (e.g. duplicate session), show them an error + // else they might be renewing/refreshing their session, so go to the callback + return forceSignOut ? '/account/signed-out' : '/auth/callback' }, /** * Validate session using auth credentials * @param {Request<{ AuthArtifactsExtra: AuthArtifacts }>} [request] - * @param {{ sessionId: string, user: UserCredentials }} [session] - Session cookie state + * @param {{ sessionId: string, flowId: string, user: UserCredentials }} [session] - Session cookie state */ async validate(request, session) { if (!request) { @@ -60,6 +67,20 @@ const sessionCookie = { let credentials = await getUserSession(request, session) + if ( + session && + credentials && + session.flowId !== credentials.flowId + ) { + logger.debug( + `Found duplicate session for user ${credentials.user?.id}. Dropping old session` + ) + + request.yar.flash(sessionNames.forceSignOut, true) // so we can detect the force sign out in the redirectTo function + + return { isValid: false } + } + if (hasUser(credentials)) { const { auth } = request @@ -68,6 +89,10 @@ const sessionCookie = { // Refresh session when token has expired if (hasExpired(credentials)) { + logger.debug( + `User ${credentials.user.id}'s session has expired. Refreshing.` + ) + const { body: artifacts } = await refreshAccessToken(request) credentials = await createUserSession(request, artifacts) } diff --git a/designer/server/src/common/helpers/auth/user-session.js b/designer/server/src/common/helpers/auth/user-session.js index 3947ed783..6685f3d2a 100644 --- a/designer/server/src/common/helpers/auth/user-session.js +++ b/designer/server/src/common/helpers/auth/user-session.js @@ -1,4 +1,5 @@ import { DateTime } from 'luxon' +import { v4 as uuidv4 } from 'uuid' import { getUserClaims, @@ -48,6 +49,7 @@ export async function createUserSession(request, artifacts) { credentials.idToken = artifacts.id_token credentials.refreshToken = artifacts.refresh_token credentials.expiresIn = artifacts.expires_in + credentials.flowId = uuidv4() // a unique ID for the current session if (!hasAuthenticated(credentials)) { throw new Error('Missing user authentication tokens') diff --git a/designer/server/src/routes/account/auth.js b/designer/server/src/routes/account/auth.js index f53bdee23..21f8cbf22 100644 --- a/designer/server/src/routes/account/auth.js +++ b/designer/server/src/routes/account/auth.js @@ -23,7 +23,10 @@ export default [ } // Add to authentication cookie for session validation - cookieAuth.set({ sessionId: credentials.user.id }) + cookieAuth.set({ + sessionId: credentials.user.id, + flowId: credentials.flowId // always store the latest flowId so we can detect stale sessions later + }) const redirect = yar.flash(sessionNames.redirectTo).at(0) ?? formsLibraryPath diff --git a/designer/server/src/typings/hapi/index.d.ts b/designer/server/src/typings/hapi/index.d.ts index f4958c7dd..9def00d5a 100644 --- a/designer/server/src/typings/hapi/index.d.ts +++ b/designer/server/src/typings/hapi/index.d.ts @@ -60,6 +60,7 @@ declare module '@hapi/hapi' { idToken: string refreshToken: string expiresIn: number + flowId: string } interface UserCredentials { @@ -95,6 +96,7 @@ declare module '@hapi/yar' { type RedirectToKey = (typeof sessionNames)['redirectTo'] type SuccessNotification = (typeof sessionNames)['successNotification'] type ErrorListKey = (typeof sessionNames)['errorList'] + type ForceSignOutKey = (typeof sessionNames)['forceSignOut'] // Export known validation session keys type ValidationSession = (typeof sessionNames)['validationFailure'] @@ -144,11 +146,20 @@ declare module '@hapi/yar' { type: ValidationSession['fileDownload'] ): ValidationFailure<{ email: string }>[] + flash( + type: ValidationSession['fileDownload'] + ): ValidationFailure<{ email: string }>[] + /** * Get temporary error messages relating to the current page. */ flash(type: ErrorListKey): ErrorDetailsItem[] + /** + * Get temporary boolean values from the session + */ + flash(type: ForceSignOutKey): boolean[] + /** * Get form metadata from the session */ diff --git a/designer/server/test/fixtures/auth.js b/designer/server/test/fixtures/auth.js index b62edc540..f830975b8 100644 --- a/designer/server/test/fixtures/auth.js +++ b/designer/server/test/fixtures/auth.js @@ -68,7 +68,8 @@ export function credentials(options) { token: tokens.access_token, idToken: tokens.id_token, refreshToken: tokens.refresh_token, - expiresIn: tokens.expires_in + expiresIn: tokens.expires_in, + flowId: '1234' }) } diff --git a/package-lock.json b/package-lock.json index 828e8bb30..e41aaf490 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,6 +12,9 @@ "model", "designer" ], + "dependencies": { + "uuid": "^11.0.5" + }, "devDependencies": { "@babel/cli": "^7.26.4", "@babel/core": "^7.26.0", @@ -13672,6 +13675,18 @@ "which": "^2.0.2" } }, + "node_modules/node-notifier/node_modules/uuid": { + "version": "8.3.2", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.2.tgz", + "integrity": "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==", + "dev": true, + "license": "MIT", + "optional": true, + "peer": true, + "bin": { + "uuid": "dist/bin/uuid" + } + }, "node_modules/node-releases": { "version": "2.0.18", "resolved": "https://registry.npmjs.org/node-releases/-/node-releases-2.0.18.tgz", @@ -18347,15 +18362,16 @@ "integrity": "sha512-EPD5q1uXyFxJpCrLnCc1nHnq3gOa6DZBocAIiI2TaSCA7VCJ1UJDMagCzIkXNsUYfD1daK//LTEQ8xiIbrHtcw==" }, "node_modules/uuid": { - "version": "8.3.2", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.2.tgz", - "integrity": "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==", - "dev": true, + "version": "11.0.5", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-11.0.5.tgz", + "integrity": "sha512-508e6IcKLrhxKdBbcA2b4KQZlLVp2+J5UwQ6F7Drckkc5N9ZJwFa4TgWtsww9UG8fGHbm6gbV19TdM5pQ4GaIA==", + "funding": [ + "https://github.com/sponsors/broofa", + "https://github.com/sponsors/ctavan" + ], "license": "MIT", - "optional": true, - "peer": true, "bin": { - "uuid": "dist/bin/uuid" + "uuid": "dist/esm/bin/uuid" } }, "node_modules/v8-compile-cache-lib": { diff --git a/package.json b/package.json index 8595f0163..6a60f72a4 100644 --- a/package.json +++ b/package.json @@ -70,5 +70,8 @@ "engines": { "node": "^22.11.0", "npm": "^10.9.0" + }, + "dependencies": { + "uuid": "^11.0.5" } } From a77ca08e88de4babeff8b96eb556733dc1623ed8 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 28 Jan 2025 09:29:56 +0000 Subject: [PATCH 2/3] Add tests --- .../server/src/routes/account/auth.test.js | 67 ++++++++++++++++++- designer/server/test/fixtures/auth.js | 2 +- designer/server/test/fixtures/forms.js | 35 ++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 designer/server/test/fixtures/forms.js diff --git a/designer/server/src/routes/account/auth.test.js b/designer/server/src/routes/account/auth.test.js index 91decda60..942859129 100644 --- a/designer/server/src/routes/account/auth.test.js +++ b/designer/server/src/routes/account/auth.test.js @@ -1,9 +1,13 @@ +import { StatusCodes } from 'http-status-codes' + import { createServer } from '~/src/createServer.js' +import * as forms from '~/src/lib/forms.js' import { auth, authGroupsInvalid, authScopesEmpty } from '~/test/fixtures/auth.js' +import { forms as formMetadataList } from '~/test/fixtures/forms.js' jest.mock('~/src/lib/forms.js') @@ -52,8 +56,69 @@ describe('Authentiation', () => { }) } ) + + it('Should handle stale sessions', async () => { + jest.mocked(forms.list).mockResolvedValueOnce({ + data: formMetadataList, + meta: {} + }) + + // log in with session one + const callbackResponseS1 = await doAuthCallback(server, auth) + const cookiesS1 = callbackResponseS1.headers['set-cookie'] + expect(callbackResponseS1.headers.location).toBe('/library') + + // log in with session two + // should be allowed, but now S2 is marked as the 'active' session + const callbackResponseS2 = await doAuthCallback(server, auth) + const cookiesS2 = callbackResponseS2.headers['set-cookie'] + expect(callbackResponseS2.headers.location).toBe('/library') + + // attempt to access a page with session one (now marked as stale) + // this is invalid - session should not be accepted + const libraryResponse2S1 = await doCallLibrary(server, cookiesS1) + expect(libraryResponse2S1.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(libraryResponse2S1.headers.location).toBe('/account/signed-out') // redirect to signed out + + // attempt to access a page with session two + // this is valid as it's the active session + const libraryResponse2S2 = await doCallLibrary(server, cookiesS2) + expect(libraryResponse2S2.statusCode).toBe(StatusCodes.OK) // this remains the active session, so it should continue + }) }) /** - * @import { Server, ServerInjectResponse } from '@hapi/hapi' + * @param {Server} server + * @param {ServerInjectOptions['auth']} auth + */ +function doAuthCallback(server, auth) { + return server.inject({ + method: 'get', + url: '/auth/callback', + auth + }) +} + +/** + * @param {Server} server + * @param {string | string[] | undefined} cookies + */ +function doCallLibrary(server, cookies) { + if (!cookies || typeof cookies === 'string') { + throw new Error('Invalid cookies') + } + + const headers = { + cookie: cookies.join('; ') + } + + return server.inject({ + method: 'get', + url: '/library', + headers + }) +} + +/** + * @import { Server, ServerInjectOptions, ServerInjectResponse } from '@hapi/hapi' */ diff --git a/designer/server/test/fixtures/auth.js b/designer/server/test/fixtures/auth.js index f830975b8..6070ba303 100644 --- a/designer/server/test/fixtures/auth.js +++ b/designer/server/test/fixtures/auth.js @@ -69,7 +69,7 @@ export function credentials(options) { idToken: tokens.id_token, refreshToken: tokens.refresh_token, expiresIn: tokens.expires_in, - flowId: '1234' + flowId: '' }) } diff --git a/designer/server/test/fixtures/forms.js b/designer/server/test/fixtures/forms.js new file mode 100644 index 000000000..91cd74a45 --- /dev/null +++ b/designer/server/test/fixtures/forms.js @@ -0,0 +1,35 @@ +const now = new Date() +const authorId = 'f50ceeed-b7a4-47cf-a498-094efc99f8bc' +const authorDisplayName = 'Enrique Chase' + +/** + * @satisfies {import("@defra/forms-model").FormMetadataAuthor} + */ +const author = { + id: authorId, + displayName: authorDisplayName +} + +/** + * @satisfies {import("@defra/forms-model").FormMetadata[]} + */ +export const forms = [ + { + id: '661e4ca5039739ef2902b214', + slug: 'my-form-slug', + title: 'Test form', + organisation: 'Defra', + teamName: 'Defra Forms', + teamEmail: 'defraforms@defra.gov.uk', + draft: { + createdAt: now, + createdBy: author, + updatedAt: now, + updatedBy: author + }, + createdAt: now, + createdBy: author, + updatedAt: now, + updatedBy: author + } +] From 72b82fc328e39c6e8225b7f789c774e3e1fd8283 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 28 Jan 2025 15:15:35 +0000 Subject: [PATCH 3/3] move redirect signout to callback --- .../server/src/common/helpers/auth/session-cookie.js | 7 +------ designer/server/src/routes/account/auth.js | 9 +++++++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/designer/server/src/common/helpers/auth/session-cookie.js b/designer/server/src/common/helpers/auth/session-cookie.js index 00da25ea5..0f2f18931 100644 --- a/designer/server/src/common/helpers/auth/session-cookie.js +++ b/designer/server/src/common/helpers/auth/session-cookie.js @@ -40,9 +40,6 @@ const sessionCookie = { * Redirect invalid session to callback route */ redirectTo(request) { - const forceSignOut = - request?.yar.flash(sessionNames.forceSignOut).at(0) ?? false - if (request) { const { url, yar } = request @@ -50,9 +47,7 @@ const sessionCookie = { yar.flash(sessionNames.redirectTo, url.pathname) } - // If we're forcing them out (e.g. duplicate session), show them an error - // else they might be renewing/refreshing their session, so go to the callback - return forceSignOut ? '/account/signed-out' : '/auth/callback' + return '/auth/callback' }, /** diff --git a/designer/server/src/routes/account/auth.js b/designer/server/src/routes/account/auth.js index 21f8cbf22..d04a85071 100644 --- a/designer/server/src/routes/account/auth.js +++ b/designer/server/src/routes/account/auth.js @@ -15,6 +15,15 @@ export default [ async handler(request, h) { const { cookieAuth, yar } = request + const forceSignOut = yar.flash(sessionNames.forceSignOut).at(0) ?? false + + if (forceSignOut) { + // If we're forcing them out (e.g. duplicate session), show them an error + // else they might be renewing/refreshing their session, so go to the callback + request.cookieAuth.clear() + return h.redirect('/account/signed-out') + } + // Create user session const credentials = await createUserSession(request)