From a9fa88d031ef79720bac4891e8506f68f7b717f0 Mon Sep 17 00:00:00 2001 From: Andrew Folga Date: Mon, 6 Oct 2025 12:39:53 +0100 Subject: [PATCH 01/19] PoC: move onRequest from prehandler to get and post handlers --- docs/PLUGIN_OPTIONS.md | 16 +-- src/server/plugins/engine/plugin.ts | 6 +- src/server/plugins/engine/routes/index.ts | 17 ++- src/server/plugins/engine/routes/questions.ts | 128 ++++++++++-------- .../engine/routes/repeaters/item-delete.ts | 72 +++++----- .../engine/routes/repeaters/summary.ts | 50 ++++--- src/server/plugins/engine/types.ts | 13 +- 7 files changed, 166 insertions(+), 136 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index b74fa6c34..9921afa5d 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -235,9 +235,8 @@ If provided, the `onRequest` plugin option will be invoked on each request to an ```ts export type OnRequestCallback = ( request: AnyFormRequest, - params: FormParams, - definition: FormDefinition, - metadata: FormMetadata + h: ResponseToolkit, + context: FormContext ) => void ``` @@ -247,12 +246,13 @@ Here's an example of how it could be used to secure access to forms: await server.register({ plugin, options: { - onRequest: (request , params, definition, metadata) => { - const { auth } = request + onRequest: (request , h, context) => { + const redirectUrl = mapStatusToUrl(applicationStatus, grantCode) + if (request.path === redirectUrl) { + return h.continue + } - if (!auth.isAuthenticated) { - throw Boom.unauthorized() - } + return h.redirect(redirectUrl).takeover() } } }) diff --git a/src/server/plugins/engine/plugin.ts b/src/server/plugins/engine/plugin.ts index 2c783d38d..ade2b53eb 100644 --- a/src/server/plugins/engine/plugin.ts +++ b/src/server/plugins/engine/plugin.ts @@ -34,7 +34,8 @@ export const plugin = { saveAndExit, nunjucks: nunjucksOptions, viewContext, - preparePageEventRequestOptions + preparePageEventRequestOptions, + onRequest } = options const cacheService = @@ -83,7 +84,8 @@ export const plugin = { ...getQuestionRoutes( getRouteOptions, postRouteOptions, - preparePageEventRequestOptions + preparePageEventRequestOptions, + onRequest ), ...getRepeaterSummaryRoutes(getRouteOptions, postRouteOptions), ...getRepeaterItemDeleteRoutes(getRouteOptions, postRouteOptions), diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 93ea2584c..2da078ec4 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -23,6 +23,7 @@ import * as defaultServices from '~/src/server/plugins/engine/services/index.js' import { type AnyFormRequest, type FormContext, + type OnRequestCallback, type PluginOptions } from '~/src/server/plugins/engine/types.js' import { @@ -33,6 +34,7 @@ import { export async function redirectOrMakeHandler( request: AnyFormRequest, h: FormResponseToolkit, + onRequest: OnRequestCallback | undefined, makeHandler: ( page: PageControllerClass, context: FormContext @@ -69,6 +71,14 @@ export async function redirectOrMakeHandler( const relevantPath = page.getRelevantPath(request, context) const summaryPath = page.getSummaryPath() + // Call the onRequest callback if it has been supplied + if (onRequest) { + const result = await onRequest(request, h as ResponseToolkit, context) + if (result) { + return result // no need to check for `isTakeover`, TypeScript is happy + } + } + // Return handler for relevant pages or preview URL direct access if (relevantPath.startsWith(page.path) || context.isForceAccess) { return makeHandler(page, context) @@ -89,7 +99,7 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- hapi types are wrong const prefix = server.realm.modifiers.route.prefix ?? '' - const { services = defaultServices, controllers, onRequest } = options + const { services = defaultServices, controllers } = options const { formsService } = services @@ -166,11 +176,6 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { server.app.models.set(key, item) } - // Call the onRequest callback if it has been supplied - if (onRequest) { - onRequest(request, params, item.model.def, metadata) - } - // Assign the model to the request data // for use in the downstream handler request.app.model = item.model diff --git a/src/server/plugins/engine/routes/questions.ts b/src/server/plugins/engine/routes/questions.ts index bdc9a9659..c6499b67e 100644 --- a/src/server/plugins/engine/routes/questions.ts +++ b/src/server/plugins/engine/routes/questions.ts @@ -26,6 +26,7 @@ import { import { type AnyFormRequest, type FormContext, + type OnRequestCallback, type PreparePageEventRequestOptions } from '~/src/server/plugins/engine/types.js' import { @@ -74,7 +75,8 @@ async function handleHttpEvent( } export function makeGetHandler( - preparePageEventRequestOptions?: PreparePageEventRequestOptions + preparePageEventRequestOptions?: PreparePageEventRequestOptions, + onRequest?: OnRequestCallback ) { return function getHandler(request: FormRequest, h: FormResponseToolkit) { const { params } = request @@ -83,34 +85,40 @@ export function makeGetHandler( return dispatchHandler(request, h) } - return redirectOrMakeHandler(request, h, async (page, context) => { - // Check for a page onLoad HTTP event and if one exists, - // call it and assign the response to the context data - const { events } = page - const { model } = request.app + return redirectOrMakeHandler( + request, + h, + onRequest, + async (page, context) => { + // Check for a page onLoad HTTP event and if one exists, + // call it and assign the response to the context data + const { events } = page + const { model } = request.app - if (!model) { - throw Boom.notFound(`No model found for /${params.path}`) - } + if (!model) { + throw Boom.notFound(`No model found for /${params.path}`) + } - if (events?.onLoad && events.onLoad.type === 'http') { - await handleHttpEvent( - request, - page, - context, - events.onLoad, - model, - preparePageEventRequestOptions - ) - } + if (events?.onLoad && events.onLoad.type === 'http') { + await handleHttpEvent( + request, + page, + context, + events.onLoad, + model, + preparePageEventRequestOptions + ) + } - return page.makeGetRouteHandler()(request, context, h) - }) + return page.makeGetRouteHandler()(request, context, h) + } + ) } } export function makePostHandler( - preparePageEventRequestOptions?: PreparePageEventRequestOptions + preparePageEventRequestOptions?: PreparePageEventRequestOptions, + onRequest?: OnRequestCallback ) { return function postHandler( request: FormRequestPayload, @@ -118,40 +126,45 @@ export function makePostHandler( ) { const { query } = request - return redirectOrMakeHandler(request, h, async (page, context) => { - const { pageDef } = page - const { isForceAccess } = context - const { model } = request.app - const { events } = page + return redirectOrMakeHandler( + request, + h, + onRequest, + async (page, context) => { + const { pageDef } = page + const { isForceAccess } = context + const { model } = request.app + const { events } = page - // Redirect to GET for preview URL direct access - if (isForceAccess && !hasFormComponents(pageDef)) { - return proceed(request, h, redirectPath(page.href, query)) - } + // Redirect to GET for preview URL direct access + if (isForceAccess && !hasFormComponents(pageDef)) { + return proceed(request, h, redirectPath(page.href, query)) + } - if (!model) { - throw Boom.notFound(`No model found for /${request.params.path}`) - } + if (!model) { + throw Boom.notFound(`No model found for /${request.params.path}`) + } - const response = await page.makePostRouteHandler()(request, context, h) + const response = await page.makePostRouteHandler()(request, context, h) - if ( - events?.onSave && - events.onSave.type === 'http' && - isSuccessful(response) - ) { - await handleHttpEvent( - request, - page, - context, - events.onSave, - model, - preparePageEventRequestOptions - ) - } + if ( + events?.onSave && + events.onSave.type === 'http' && + isSuccessful(response) + ) { + await handleHttpEvent( + request, + page, + context, + events.onSave, + model, + preparePageEventRequestOptions + ) + } - return response - }) + return response + } + ) } } @@ -164,13 +177,14 @@ function isSuccessful(response: ResponseObject): boolean { export function getRoutes( getRouteOptions: RouteOptions, postRouteOptions: RouteOptions, - preparePageEventRequestOptions?: PreparePageEventRequestOptions + preparePageEventRequestOptions?: PreparePageEventRequestOptions, + onRequest?: OnRequestCallback ): (ServerRoute | ServerRoute)[] { return [ { method: 'get', path: '/{slug}', - handler: makeGetHandler(preparePageEventRequestOptions), + handler: makeGetHandler(preparePageEventRequestOptions, onRequest), options: { ...getRouteOptions, validate: { @@ -197,7 +211,7 @@ export function getRoutes( { method: 'get', path: '/{slug}/{path}/{itemId?}', - handler: makeGetHandler(preparePageEventRequestOptions), + handler: makeGetHandler(preparePageEventRequestOptions, onRequest), options: { ...getRouteOptions, validate: { @@ -212,7 +226,7 @@ export function getRoutes( { method: 'get', path: '/preview/{state}/{slug}/{path}/{itemId?}', - handler: makeGetHandler(preparePageEventRequestOptions), + handler: makeGetHandler(preparePageEventRequestOptions, onRequest), options: { ...getRouteOptions, validate: { @@ -228,7 +242,7 @@ export function getRoutes( { method: 'post', path: '/{slug}/{path}/{itemId?}', - handler: makePostHandler(preparePageEventRequestOptions), + handler: makePostHandler(preparePageEventRequestOptions, onRequest), options: { ...postRouteOptions, validate: { @@ -250,7 +264,7 @@ export function getRoutes( { method: 'post', path: '/preview/{state}/{slug}/{path}/{itemId?}', - handler: makePostHandler(preparePageEventRequestOptions), + handler: makePostHandler(preparePageEventRequestOptions, onRequest), options: { ...postRouteOptions, validate: { diff --git a/src/server/plugins/engine/routes/repeaters/item-delete.ts b/src/server/plugins/engine/routes/repeaters/item-delete.ts index f1d06cd40..fef152f85 100644 --- a/src/server/plugins/engine/routes/repeaters/item-delete.ts +++ b/src/server/plugins/engine/routes/repeaters/item-delete.ts @@ -6,6 +6,7 @@ import Joi from 'joi' import { FileUploadPageController } from '~/src/server/plugins/engine/pageControllers/FileUploadPageController.js' import { RepeatPageController } from '~/src/server/plugins/engine/pageControllers/RepeatPageController.js' import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js' +import { type OnRequestCallback } from '~/src/server/plugins/engine/types.js' import { type FormRequest, type FormRequestPayload, @@ -23,52 +24,57 @@ import { } from '~/src/server/schemas/index.js' // Item delete GET route -function getHandler(request: FormRequest, h: FormResponseToolkit) { - const { params } = request +function getHandler(onRequest?: OnRequestCallback) { + return async function (request: FormRequest, h: FormResponseToolkit) { + const { params } = request - return redirectOrMakeHandler(request, h, (page, context) => { - if ( - !( - page instanceof RepeatPageController || - page instanceof FileUploadPageController - ) - ) { - throw Boom.notFound(`No page found for /${params.path}`) - } + return redirectOrMakeHandler(request, h, onRequest, (page, context) => { + if ( + !( + page instanceof RepeatPageController || + page instanceof FileUploadPageController + ) + ) { + throw Boom.notFound(`No page found for /${params.path}`) + } - return page.makeGetItemDeleteRouteHandler()(request, context, h) - }) + return page.makeGetItemDeleteRouteHandler()(request, context, h) + }) + } } -function postHandler(request: FormRequestPayload, h: FormResponseToolkit) { - const { params } = request +function postHandler(onRequest?: OnRequestCallback) { + return async function (request: FormRequestPayload, h: FormResponseToolkit) { + const { params } = request - return redirectOrMakeHandler(request, h, (page, context) => { - const { isForceAccess } = context + return redirectOrMakeHandler(request, h, onRequest, (page, context) => { + const { isForceAccess } = context - if ( - isForceAccess || - !( - page instanceof RepeatPageController || - page instanceof FileUploadPageController - ) - ) { - throw Boom.notFound(`No page found for /${params.path}`) - } + if ( + isForceAccess || + !( + page instanceof RepeatPageController || + page instanceof FileUploadPageController + ) + ) { + throw Boom.notFound(`No page found for /${params.path}`) + } - return page.makePostItemDeleteRouteHandler()(request, context, h) - }) + return page.makePostItemDeleteRouteHandler()(request, context, h) + }) + } } export function getRoutes( getRouteOptions: RouteOptions, - postRouteOptions: RouteOptions + postRouteOptions: RouteOptions, + onRequest?: OnRequestCallback ): (ServerRoute | ServerRoute)[] { return [ { method: 'get', path: '/{slug}/{path}/{itemId}/confirm-delete', - handler: getHandler, + handler: getHandler(onRequest), options: { ...getRouteOptions, validate: { @@ -84,7 +90,7 @@ export function getRoutes( { method: 'get', path: '/preview/{state}/{slug}/{path}/{itemId}/confirm-delete', - handler: getHandler, + handler: getHandler(onRequest), options: { ...getRouteOptions, validate: { @@ -101,7 +107,7 @@ export function getRoutes( { method: 'post', path: '/{slug}/{path}/{itemId}/confirm-delete', - handler: postHandler, + handler: postHandler(onRequest), options: { ...postRouteOptions, validate: { @@ -124,7 +130,7 @@ export function getRoutes( { method: 'post', path: '/preview/{state}/{slug}/{path}/{itemId}/confirm-delete', - handler: postHandler, + handler: postHandler(onRequest), options: { ...postRouteOptions, validate: { diff --git a/src/server/plugins/engine/routes/repeaters/summary.ts b/src/server/plugins/engine/routes/repeaters/summary.ts index 3126ed9d2..9c52f7c5b 100644 --- a/src/server/plugins/engine/routes/repeaters/summary.ts +++ b/src/server/plugins/engine/routes/repeaters/summary.ts @@ -6,6 +6,7 @@ import Joi from 'joi' import { RepeatPageController } from '~/src/server/plugins/engine/pageControllers/RepeatPageController.js' import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js' +import { type OnRequestCallback } from '~/src/server/plugins/engine/types.js' import { type FormRequest, type FormRequestPayload, @@ -20,41 +21,46 @@ import { stateSchema } from '~/src/server/schemas/index.js' -function getHandler(request: FormRequest, h: FormResponseToolkit) { - const { params } = request +function getHandler(onRequest?: OnRequestCallback) { + return async function (request: FormRequest, h: FormResponseToolkit) { + const { params } = request - return redirectOrMakeHandler(request, h, (page, context) => { - if (!(page instanceof RepeatPageController)) { - throw Boom.notFound(`No repeater page found for /${params.path}`) - } + return redirectOrMakeHandler(request, h, onRequest, (page, context) => { + if (!(page instanceof RepeatPageController)) { + throw Boom.notFound(`No repeater page found for /${params.path}`) + } - return page.makeGetListSummaryRouteHandler()(request, context, h) - }) + return page.makeGetListSummaryRouteHandler()(request, context, h) + }) + } } -function postHandler(request: FormRequestPayload, h: FormResponseToolkit) { - const { params } = request +function postHandler(onRequest?: OnRequestCallback) { + return async function (request: FormRequestPayload, h: FormResponseToolkit) { + const { params } = request - return redirectOrMakeHandler(request, h, (page, context) => { - const { isForceAccess } = context + return redirectOrMakeHandler(request, h, onRequest, (page, context) => { + const { isForceAccess } = context - if (isForceAccess || !(page instanceof RepeatPageController)) { - throw Boom.notFound(`No repeater page found for /${params.path}`) - } + if (isForceAccess || !(page instanceof RepeatPageController)) { + throw Boom.notFound(`No repeater page found for /${params.path}`) + } - return page.makePostListSummaryRouteHandler()(request, context, h) - }) + return page.makePostListSummaryRouteHandler()(request, context, h) + }) + } } export function getRoutes( getRouteOptions: RouteOptions, - postRouteOptions: RouteOptions + postRouteOptions: RouteOptions, + onRequest?: OnRequestCallback ): (ServerRoute | ServerRoute)[] { return [ { method: 'get', path: '/{slug}/{path}/summary', - handler: getHandler, + handler: getHandler(onRequest), options: { ...getRouteOptions, validate: { @@ -69,7 +75,7 @@ export function getRoutes( { method: 'get', path: '/preview/{state}/{slug}/{path}/summary', - handler: getHandler, + handler: getHandler(onRequest), options: { ...getRouteOptions, validate: { @@ -85,7 +91,7 @@ export function getRoutes( { method: 'post', path: '/{slug}/{path}/summary', - handler: postHandler, + handler: postHandler(onRequest), options: { ...postRouteOptions, validate: { @@ -106,7 +112,7 @@ export function getRoutes( { method: 'post', path: '/preview/{state}/{slug}/{path}/summary', - handler: postHandler, + handler: postHandler(onRequest), options: { ...postRouteOptions, validate: { diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 1ff21d013..52b14a0b3 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -1,8 +1,6 @@ import { type ComponentDef, type Event, - type FormDefinition, - type FormMetadata, type FormVersionMetadata, type Item, type List, @@ -11,7 +9,8 @@ import { import { type PluginProperties, type Request, - type ResponseObject + type ResponseObject, + type ResponseToolkit } from '@hapi/hapi' import { type JoiExpression, type ValidationErrorItem } from 'joi' @@ -38,7 +37,6 @@ import { import { type ViewContext } from '~/src/server/plugins/nunjucks/types.js' import { type FormAction, - type FormParams, type FormRequest, type FormRequestPayload, type FormResponseToolkit, @@ -368,10 +366,9 @@ export type PreparePageEventRequestOptions = ( export type OnRequestCallback = ( request: AnyFormRequest, - params: FormParams, - definition: FormDefinition, - metadata: FormMetadata -) => void + h: ResponseToolkit, + context: FormContext +) => Promise export type SaveAndExitHandler = ( request: FormRequestPayload, From 9e89127d9e79eb8862a9449e88e0620d9609681d Mon Sep 17 00:00:00 2001 From: Andrew Folga Date: Wed, 8 Oct 2025 11:00:29 +0100 Subject: [PATCH 02/19] add typings --- src/server/plugins/engine/routes/index.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 2da078ec4..2c7eeb45f 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -74,8 +74,12 @@ export async function redirectOrMakeHandler( // Call the onRequest callback if it has been supplied if (onRequest) { const result = await onRequest(request, h as ResponseToolkit, context) - if (result) { - return result // no need to check for `isTakeover`, TypeScript is happy + if ( + result && + typeof result === 'object' && + (result as unknown as Record)._takeover === true + ) { + return result } } From 8578f98bcd3ae714d501c900f337e337e945fde1 Mon Sep 17 00:00:00 2001 From: Alan Platt Date: Fri, 10 Oct 2025 16:54:05 +0100 Subject: [PATCH 03/19] Pass onRequest to repeater route functions so that it runs on all pages and update tests --- src/server/plugins/engine/plugin.ts | 8 +++- .../plugins/engine/routes/questions.test.ts | 48 ++++++++++++++----- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/server/plugins/engine/plugin.ts b/src/server/plugins/engine/plugin.ts index ade2b53eb..a20451f16 100644 --- a/src/server/plugins/engine/plugin.ts +++ b/src/server/plugins/engine/plugin.ts @@ -87,8 +87,12 @@ export const plugin = { preparePageEventRequestOptions, onRequest ), - ...getRepeaterSummaryRoutes(getRouteOptions, postRouteOptions), - ...getRepeaterItemDeleteRoutes(getRouteOptions, postRouteOptions), + ...getRepeaterSummaryRoutes(getRouteOptions, postRouteOptions, onRequest), + ...getRepeaterItemDeleteRoutes( + getRouteOptions, + postRouteOptions, + onRequest + ), ...getFileUploadStatusRoutes() ] diff --git a/src/server/plugins/engine/routes/questions.test.ts b/src/server/plugins/engine/routes/questions.test.ts index c9940ed5d..cc00d81b8 100644 --- a/src/server/plugins/engine/routes/questions.test.ts +++ b/src/server/plugins/engine/routes/questions.test.ts @@ -40,7 +40,14 @@ jest.mock('~/src/server/plugins/engine/routes/index') describe('makeGetHandler', () => { const hMock: FormResponseToolkit = { - redirect: jest.fn(), + redirect: jest.fn().mockReturnValue({ + takeover: jest + .fn() + .mockReturnValue({ + statusCode: 302, + headers: { location: '/redirect-url' } + }) + }), view: jest.fn() } @@ -84,8 +91,9 @@ describe('makeGetHandler', () => { jest .mocked(redirectOrMakeHandler) - .mockImplementation((_req: AnyFormRequest, _h: FormResponseToolkit, fn) => - Promise.resolve(fn(pageMock, contextMock)) + .mockImplementation( + (_req: AnyFormRequest, _h: FormResponseToolkit, _onRequest, fn) => + Promise.resolve(fn(pageMock, contextMock)) ) await makeGetHandler()(requestMock, hMock) @@ -126,8 +134,9 @@ describe('makeGetHandler', () => { jest .mocked(redirectOrMakeHandler) - .mockImplementation((_req: AnyFormRequest, _h: FormResponseToolkit, fn) => - Promise.resolve(fn(pageMock, contextMock)) + .mockImplementation( + (_req: AnyFormRequest, _h: FormResponseToolkit, _onRequest, fn) => + Promise.resolve(fn(pageMock, contextMock)) ) await makeGetHandler()(requestMock, hMock) @@ -164,7 +173,12 @@ describe('makeGetHandler', () => { jest .mocked(redirectOrMakeHandler) .mockImplementation( - async (_req: AnyFormRequest, _h: FormResponseToolkit, fn) => { + async ( + _req: AnyFormRequest, + _h: FormResponseToolkit, + _onRequest, + fn + ) => { try { await fn(pageMock, contextMock) } catch (err) { @@ -230,8 +244,9 @@ describe('makePostHandler', () => { jest .mocked(redirectOrMakeHandler) - .mockImplementation((_req: AnyFormRequest, _h: FormResponseToolkit, fn) => - Promise.resolve(fn(pageMock, contextMock)) + .mockImplementation( + (_req: AnyFormRequest, _h: FormResponseToolkit, _onRequest, fn) => + Promise.resolve(fn(pageMock, contextMock)) ) const response = await makePostHandler()(requestMock, hMock) @@ -269,8 +284,9 @@ describe('makePostHandler', () => { jest .mocked(redirectOrMakeHandler) - .mockImplementation((_req: AnyFormRequest, _h: FormResponseToolkit, fn) => - Promise.resolve(fn(pageMock, contextMock)) + .mockImplementation( + (_req: AnyFormRequest, _h: FormResponseToolkit, _onRequest, fn) => + Promise.resolve(fn(pageMock, contextMock)) ) await makePostHandler()(requestMock, hMock) @@ -310,8 +326,9 @@ describe('makePostHandler', () => { jest .mocked(redirectOrMakeHandler) - .mockImplementation((_req: AnyFormRequest, _h: FormResponseToolkit, fn) => - Promise.resolve(fn(pageMock, contextMock)) + .mockImplementation( + (_req: AnyFormRequest, _h: FormResponseToolkit, _onRequest, fn) => + Promise.resolve(fn(pageMock, contextMock)) ) await makePostHandler()(requestMock, hMock) @@ -349,7 +366,12 @@ describe('makePostHandler', () => { jest .mocked(redirectOrMakeHandler) .mockImplementation( - async (_req: AnyFormRequest, _h: FormResponseToolkit, fn) => { + async ( + _req: AnyFormRequest, + _h: FormResponseToolkit, + _onRequest, + fn + ) => { try { await fn(pageMock, contextMock) } catch (err) { From d8c1169c6fde732d1e4568f8feca437684122545 Mon Sep 17 00:00:00 2001 From: Alan Platt Date: Mon, 13 Oct 2025 18:03:07 +0100 Subject: [PATCH 04/19] Update PLUGIN_OPTIONS.md onRequest docs to include simple example --- docs/PLUGIN_OPTIONS.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index 9921afa5d..a94b2541e 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -237,7 +237,7 @@ export type OnRequestCallback = ( request: AnyFormRequest, h: ResponseToolkit, context: FormContext -) => void +) => Promise ``` Here's an example of how it could be used to secure access to forms: @@ -246,14 +246,17 @@ Here's an example of how it could be used to secure access to forms: await server.register({ plugin, options: { - onRequest: (request , h, context) => { - const redirectUrl = mapStatusToUrl(applicationStatus, grantCode) - if (request.path === redirectUrl) { - return h.continue - } + onRequest: async (request, h, context) => { + const { auth } = request - return h.redirect(redirectUrl).takeover() + // Check if user is authenticated + if (!auth.isAuthenticated) { + return h.redirect('/login').takeover() } + + // Return undefined to continue with normal form processing + return undefined + } } }) ``` From 432c584c0f8ae827fc8b7c2b0ef6e123f3fae201 Mon Sep 17 00:00:00 2001 From: Alan Platt Date: Mon, 13 Oct 2025 19:30:07 +0100 Subject: [PATCH 05/19] Add unit tests for redirectOrMakeHandler and repeater routes - Introduced tests for redirectOrMakeHandler to validate onRequest callback functionality and response handling. - Added tests for repeater item-delete and summary routes to ensure correct route configuration and handler creation with onRequest callbacks. - Enhanced existing tests for makeGetHandler and makePostHandler to verify onRequest callback passing. --- .../plugins/engine/routes/index.test.ts | 314 ++++++++++++++++++ .../plugins/engine/routes/questions.test.ts | 99 +++++- .../routes/repeaters/item-delete.test.ts | 83 +++++ .../engine/routes/repeaters/summary.test.ts | 75 +++++ 4 files changed, 565 insertions(+), 6 deletions(-) create mode 100644 src/server/plugins/engine/routes/index.test.ts create mode 100644 src/server/plugins/engine/routes/repeaters/item-delete.test.ts create mode 100644 src/server/plugins/engine/routes/repeaters/summary.test.ts diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts new file mode 100644 index 000000000..5a34c8412 --- /dev/null +++ b/src/server/plugins/engine/routes/index.test.ts @@ -0,0 +1,314 @@ +import Boom from '@hapi/boom' +import { type ResponseObject, type ResponseToolkit } from '@hapi/hapi' + +import { + findPage, + getCacheService, + getPage, + proceed +} from '~/src/server/plugins/engine/helpers.ts' +import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' +import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js' +import { + type AnyFormRequest, + type OnRequestCallback +} from '~/src/server/plugins/engine/types.js' +import { type FormResponseToolkit } from '~/src/server/routes/types.js' + +jest.mock('~/src/server/plugins/engine/helpers.ts') + +describe('redirectOrMakeHandler', () => { + const mockServer = {} as unknown as Parameters< + typeof redirectOrMakeHandler + >[0]['server'] + const mockRequest: AnyFormRequest = { + server: mockServer, + app: {}, + params: { path: 'test-path' }, + query: {} + } as unknown as AnyFormRequest + + const mockH: FormResponseToolkit = { + redirect: jest.fn(), + view: jest.fn() + } as unknown as FormResponseToolkit + + const mockPage: PageControllerClass = { + getState: jest.fn().mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + mergeState: jest.fn().mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + getRelevantPath: jest.fn().mockReturnValue('/test-path'), + getSummaryPath: jest.fn().mockReturnValue('/summary'), + getHref: jest.fn().mockReturnValue('/test-href'), + path: '/test-path' + } as unknown as PageControllerClass + + const mockModel: FormModel = { + def: { metadata: {} }, + getFormContext: jest.fn().mockReturnValue({ + isForceAccess: false, + data: {} + }) + } as unknown as FormModel + + const mockMakeHandler = jest + .fn() + .mockResolvedValue({ statusCode: 200 } as ResponseObject) + + beforeEach(() => { + jest.clearAllMocks() + mockRequest.app = { model: mockModel } + + // Reset mock page + mockPage.getState = jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }) + mockPage.mergeState = jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }) + mockPage.getRelevantPath = jest.fn().mockReturnValue('/test-path') + mockPage.getSummaryPath = jest.fn().mockReturnValue('/summary') + mockPage.getHref = jest.fn().mockReturnValue('/test-href') + mockPage.path = '/test-path' + + // Reset mock model + mockModel.getFormContext = jest.fn().mockReturnValue({ + isForceAccess: false, + data: {} + }) + + // Setup mocks + ;(getCacheService as jest.Mock).mockReturnValue({ + getFlash: jest.fn().mockReturnValue({ errors: [] }) + }) + ;(getPage as jest.Mock).mockReturnValue(mockPage) + ;(findPage as jest.Mock).mockReturnValue({ next: [] }) + ;(proceed as jest.Mock).mockReturnValue({ statusCode: 302 }) + }) + + describe('onRequest callback functionality', () => { + it('should call onRequest callback when provided', async () => { + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(undefined) + + await redirectOrMakeHandler( + mockRequest, + mockH, + onRequestCallback, + mockMakeHandler + ) + + expect(onRequestCallback).toHaveBeenCalledWith( + mockRequest, + mockH as ResponseToolkit, + expect.objectContaining({ + isForceAccess: false, + data: {} + }) + ) + }) + + it('should not call onRequest callback when not provided', async () => { + const onRequestCallback = jest.fn() + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(onRequestCallback).not.toHaveBeenCalled() + }) + + it('should return takeover response when onRequest returns takeover response', async () => { + const takeoverResponse = { + statusCode: 302, + headers: { location: '/redirect-url' }, + _takeover: true + } as unknown as ResponseObject + + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(takeoverResponse) + + const result = await redirectOrMakeHandler( + mockRequest, + mockH, + onRequestCallback, + mockMakeHandler + ) + + expect(result).toBe(takeoverResponse) + expect(mockMakeHandler).not.toHaveBeenCalled() + }) + + it('should continue processing when onRequest returns undefined', async () => { + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(undefined) + + await redirectOrMakeHandler( + mockRequest, + mockH, + onRequestCallback, + mockMakeHandler + ) + + expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) + }) + + it('should continue processing when onRequest returns non-takeover response', async () => { + const nonTakeoverResponse = { + statusCode: 200, + _takeover: false + } as unknown as ResponseObject + + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(nonTakeoverResponse) + + await redirectOrMakeHandler( + mockRequest, + mockH, + onRequestCallback, + mockMakeHandler + ) + + expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) + }) + + it('should continue processing when onRequest returns response without _takeover property', async () => { + const responseWithoutTakeover = { + statusCode: 200 + } as unknown as ResponseObject + + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(responseWithoutTakeover) + + await redirectOrMakeHandler( + mockRequest, + mockH, + onRequestCallback, + mockMakeHandler + ) + + expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) + }) + + it('should handle onRequest callback errors', async () => { + const error = new Error('onRequest callback error') + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockRejectedValue(error) + + await expect( + redirectOrMakeHandler( + mockRequest, + mockH, + onRequestCallback, + mockMakeHandler + ) + ).rejects.toThrow('onRequest callback error') + }) + }) + + describe('existing functionality', () => { + it('should throw error when model is missing', async () => { + mockRequest.app = {} + + await expect( + redirectOrMakeHandler(mockRequest, mockH, undefined, mockMakeHandler) + ).rejects.toThrow(Boom.notFound('No model found for /test-path')) + }) + + it('should call makeHandler when page is relevant', async () => { + mockPage.getRelevantPath = jest.fn().mockReturnValue('/test-path') + mockPage.path = '/test-path' + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) + }) + + it('should call makeHandler when context has force access', async () => { + mockModel.getFormContext = jest.fn().mockReturnValue({ + isForceAccess: true, + data: {} + }) + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) + }) + + it('should redirect when page is not relevant', async () => { + mockPage.getRelevantPath = jest.fn().mockReturnValue('/other-path') + mockPage.path = '/test-path' + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href') + expect(mockMakeHandler).not.toHaveBeenCalled() + }) + + it('should set returnUrl when redirecting and next pages exist', async () => { + mockPage.getRelevantPath = jest.fn().mockReturnValue('/other-path') + mockPage.path = '/test-path' + mockPage.getHref = jest + .fn() + .mockReturnValueOnce('/summary-href') // First call: for summaryPath (returnUrl) + .mockReturnValueOnce('/relevant-path-href') // Second call: for relevantPath (redirect) + ;(findPage as jest.Mock).mockReturnValue({ next: ['next-page'] }) + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(mockRequest.query.returnUrl).toBe('/summary-href') + expect(proceed).toHaveBeenCalledWith( + mockRequest, + mockH, + '/relevant-path-href' + ) + }) + + it('should not set returnUrl when redirecting and no next pages exist', async () => { + mockPage.getRelevantPath = jest.fn().mockReturnValue('/other-path') + mockPage.path = '/test-path' + const returnUrlBefore = mockRequest.query.returnUrl + ;(findPage as jest.Mock).mockReturnValue({ next: [] }) + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + // returnUrl should not be set if next pages don't exist + expect(mockRequest.query.returnUrl).toBe(returnUrlBefore) + expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href') + }) + }) +}) diff --git a/src/server/plugins/engine/routes/questions.test.ts b/src/server/plugins/engine/routes/questions.test.ts index cc00d81b8..135076c99 100644 --- a/src/server/plugins/engine/routes/questions.test.ts +++ b/src/server/plugins/engine/routes/questions.test.ts @@ -41,12 +41,10 @@ jest.mock('~/src/server/plugins/engine/routes/index') describe('makeGetHandler', () => { const hMock: FormResponseToolkit = { redirect: jest.fn().mockReturnValue({ - takeover: jest - .fn() - .mockReturnValue({ - statusCode: 302, - headers: { location: '/redirect-url' } - }) + takeover: jest.fn().mockReturnValue({ + statusCode: 302, + headers: { location: '/redirect-url' } + }) }), view: jest.fn() } @@ -193,6 +191,50 @@ describe('makeGetHandler', () => { expect(error).toEqual(Boom.notFound('No model found for /some-path')) }) + + it('should pass onRequest callback to redirectOrMakeHandler', async () => { + const onRequestCallback = jest.fn().mockResolvedValue(undefined) + const modelMock = { + basePath: 'some-base-path', + def: { name: 'Hello world' } + } as FormModel + + const pageMock = createMockPageController( + modelMock, + ( + _request: FormRequest, + _context: FormContext, + _h: FormResponseToolkit + ) => { + return Promise.resolve({} as unknown as ResponseObject) + } + ) + + const contextMock = { data: {}, model: {} } as unknown as FormContext + + const requestMock = { + params: { path: 'some-path' }, + app: { model: modelMock } + } as FormRequest + + jest + .mocked(redirectOrMakeHandler) + .mockImplementation( + (_req: AnyFormRequest, _h: FormResponseToolkit, onRequest, fn) => { + expect(onRequest).toBe(onRequestCallback) + return Promise.resolve(fn(pageMock, contextMock)) + } + ) + + await makeGetHandler(undefined, onRequestCallback)(requestMock, hMock) + + expect(redirectOrMakeHandler).toHaveBeenCalledWith( + requestMock, + hMock, + onRequestCallback, + expect.any(Function) + ) + }) }) describe('makePostHandler', () => { @@ -386,6 +428,51 @@ describe('makePostHandler', () => { expect(error).toEqual(Boom.notFound('No model found for /some-path')) }) + + it('should pass onRequest callback to redirectOrMakeHandler', async () => { + const onRequestCallback = jest.fn().mockResolvedValue(undefined) + const modelMock = { + basePath: 'some-base-path', + def: { name: 'Hello world' } + } as FormModel + + const pageMock = createMockPageController( + modelMock, + ( + _request: FormRequest, + _context: FormContext, + _h: FormResponseToolkit + ) => { + return Promise.resolve({} as unknown as ResponseObject) + } + ) + + const contextMock = { data: {}, model: {} } as unknown as FormContext + + const requestMock = { + params: { path: 'some-path' }, + app: { model: modelMock }, + payload: { some: 'payload' } + } as unknown as FormRequestPayload + + jest + .mocked(redirectOrMakeHandler) + .mockImplementation( + (_req: AnyFormRequest, _h: FormResponseToolkit, onRequest, fn) => { + expect(onRequest).toBe(onRequestCallback) + return Promise.resolve(fn(pageMock, contextMock)) + } + ) + + await makePostHandler(undefined, onRequestCallback)(requestMock, hMock) + + expect(redirectOrMakeHandler).toHaveBeenCalledWith( + requestMock, + hMock, + onRequestCallback, + expect.any(Function) + ) + }) }) function createMockPageController( diff --git a/src/server/plugins/engine/routes/repeaters/item-delete.test.ts b/src/server/plugins/engine/routes/repeaters/item-delete.test.ts new file mode 100644 index 000000000..7d272e691 --- /dev/null +++ b/src/server/plugins/engine/routes/repeaters/item-delete.test.ts @@ -0,0 +1,83 @@ +import { getRoutes } from '~/src/server/plugins/engine/routes/repeaters/item-delete.js' +import { type OnRequestCallback } from '~/src/server/plugins/engine/types.js' + +describe('repeater item-delete routes', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + describe('route configuration', () => { + it('should return all expected routes', () => { + const routes = getRoutes({}, {}, undefined) + + expect(routes).toHaveLength(4) + + const expectedPaths = [ + '/{slug}/{path}/{itemId}/confirm-delete', + '/preview/{state}/{slug}/{path}/{itemId}/confirm-delete' + ] + + expectedPaths.forEach((path) => { + const getRoute = routes.find( + (route) => route.method === 'get' && route.path === path + ) + const postRoute = routes.find( + (route) => route.method === 'post' && route.path === path + ) + + expect(getRoute).toBeDefined() + expect(postRoute).toBeDefined() + }) + }) + + it('should pass onRequest callback to handlers', () => { + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(undefined) + const routes = getRoutes({}, {}, onRequestCallback) + + // Test that the handlers are created with the onRequest callback + const getRoute = routes.find( + (route) => + route.method === 'get' && + route.path === '/{slug}/{path}/{itemId}/confirm-delete' + ) + const postRoute = routes.find( + (route) => + route.method === 'post' && + route.path === '/{slug}/{path}/{itemId}/confirm-delete' + ) + + expect(getRoute?.handler).toBeDefined() + expect(postRoute?.handler).toBeDefined() + }) + }) + + describe('handler functionality', () => { + it('should create handlers that accept onRequest callback', () => { + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(undefined) + const routes = getRoutes({}, {}, onRequestCallback) + + // Test that the handlers are created with the onRequest callback + const getRoute = routes.find( + (route) => + route.method === 'get' && + route.path === '/{slug}/{path}/{itemId}/confirm-delete' + ) + const postRoute = routes.find( + (route) => + route.method === 'post' && + route.path === '/{slug}/{path}/{itemId}/confirm-delete' + ) + + expect(getRoute?.handler).toBeDefined() + expect(postRoute?.handler).toBeDefined() + + // Test that handlers are functions + expect(typeof getRoute?.handler).toBe('function') + expect(typeof postRoute?.handler).toBe('function') + }) + }) +}) diff --git a/src/server/plugins/engine/routes/repeaters/summary.test.ts b/src/server/plugins/engine/routes/repeaters/summary.test.ts new file mode 100644 index 000000000..379f97f4c --- /dev/null +++ b/src/server/plugins/engine/routes/repeaters/summary.test.ts @@ -0,0 +1,75 @@ +import { getRoutes } from '~/src/server/plugins/engine/routes/repeaters/summary.js' +import { type OnRequestCallback } from '~/src/server/plugins/engine/types.js' + +describe('repeater summary routes', () => { + describe('route configuration', () => { + it('should return all expected routes', () => { + const routes = getRoutes({}, {}, undefined) + + expect(routes).toHaveLength(4) + + const expectedPaths = [ + '/{slug}/{path}/summary', + '/preview/{state}/{slug}/{path}/summary' + ] + + expectedPaths.forEach((path) => { + const getRoute = routes.find( + (route) => route.method === 'get' && route.path === path + ) + const postRoute = routes.find( + (route) => route.method === 'post' && route.path === path + ) + + expect(getRoute).toBeDefined() + expect(postRoute).toBeDefined() + }) + }) + + it('should pass onRequest callback to handlers', () => { + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(undefined) + const routes = getRoutes({}, {}, onRequestCallback) + + // Test that the handlers are created with the onRequest callback + const getRoute = routes.find( + (route) => + route.method === 'get' && route.path === '/{slug}/{path}/summary' + ) + const postRoute = routes.find( + (route) => + route.method === 'post' && route.path === '/{slug}/{path}/summary' + ) + + expect(getRoute?.handler).toBeDefined() + expect(postRoute?.handler).toBeDefined() + }) + }) + + describe('handler functionality', () => { + it('should create handlers that accept onRequest callback', () => { + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(undefined) + const routes = getRoutes({}, {}, onRequestCallback) + + // Test that the handlers are created with the onRequest callback + const getRoute = routes.find( + (route) => + route.method === 'get' && route.path === '/{slug}/{path}/summary' + ) + const postRoute = routes.find( + (route) => + route.method === 'post' && route.path === '/{slug}/{path}/summary' + ) + + expect(getRoute?.handler).toBeDefined() + expect(postRoute?.handler).toBeDefined() + + // Test that handlers are functions + expect(typeof getRoute?.handler).toBe('function') + expect(typeof postRoute?.handler).toBe('function') + }) + }) +}) From 4a03f064667c9521a2db9dd1c2149d5746f0fe84 Mon Sep 17 00:00:00 2001 From: Alan Platt Date: Mon, 13 Oct 2025 19:50:00 +0100 Subject: [PATCH 06/19] Fix eslint errors --- .../plugins/engine/routes/index.test.ts | 102 ++++++++++++------ test/form/auth.test.js | 10 +- 2 files changed, 75 insertions(+), 37 deletions(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index 5a34c8412..62364e0b0 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -6,7 +6,7 @@ import { getCacheService, getPage, proceed -} from '~/src/server/plugins/engine/helpers.ts' +} from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js' @@ -16,7 +16,7 @@ import { } from '~/src/server/plugins/engine/types.js' import { type FormResponseToolkit } from '~/src/server/routes/types.js' -jest.mock('~/src/server/plugins/engine/helpers.ts') +jest.mock('~/src/server/plugins/engine/helpers') describe('redirectOrMakeHandler', () => { const mockServer = {} as unknown as Parameters< @@ -34,14 +34,7 @@ describe('redirectOrMakeHandler', () => { view: jest.fn() } as unknown as FormResponseToolkit - const mockPage: PageControllerClass = { - getState: jest.fn().mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - mergeState: jest.fn().mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - getRelevantPath: jest.fn().mockReturnValue('/test-path'), - getSummaryPath: jest.fn().mockReturnValue('/summary'), - getHref: jest.fn().mockReturnValue('/test-href'), - path: '/test-path' - } as unknown as PageControllerClass + let mockPage: PageControllerClass const mockModel: FormModel = { def: { metadata: {} }, @@ -60,16 +53,16 @@ describe('redirectOrMakeHandler', () => { mockRequest.app = { model: mockModel } // Reset mock page - mockPage.getState = jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }) - mockPage.mergeState = jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }) - mockPage.getRelevantPath = jest.fn().mockReturnValue('/test-path') - mockPage.getSummaryPath = jest.fn().mockReturnValue('/summary') - mockPage.getHref = jest.fn().mockReturnValue('/test-href') - mockPage.path = '/test-path' + mockPage = { + getState: jest.fn().mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + mergeState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + getRelevantPath: jest.fn().mockReturnValue('/test-path'), + getSummaryPath: jest.fn().mockReturnValue('/summary'), + getHref: jest.fn().mockReturnValue('/test-href'), + path: '/test-path' + } as unknown as PageControllerClass // Reset mock model mockModel.getFormContext = jest.fn().mockReturnValue({ @@ -225,8 +218,19 @@ describe('redirectOrMakeHandler', () => { }) it('should call makeHandler when page is relevant', async () => { - mockPage.getRelevantPath = jest.fn().mockReturnValue('/test-path') - mockPage.path = '/test-path' + const testPage = { + getState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + mergeState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + getSummaryPath: jest.fn().mockReturnValue('/summary'), + getHref: jest.fn().mockReturnValue('/test-href'), + getRelevantPath: jest.fn().mockReturnValue('/test-path'), + path: '/test-path' + } as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) await redirectOrMakeHandler( mockRequest, @@ -235,7 +239,7 @@ describe('redirectOrMakeHandler', () => { mockMakeHandler ) - expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) + expect(mockMakeHandler).toHaveBeenCalledWith(testPage, expect.any(Object)) }) it('should call makeHandler when context has force access', async () => { @@ -255,8 +259,19 @@ describe('redirectOrMakeHandler', () => { }) it('should redirect when page is not relevant', async () => { - mockPage.getRelevantPath = jest.fn().mockReturnValue('/other-path') - mockPage.path = '/test-path' + const testPage = { + getState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + mergeState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + getSummaryPath: jest.fn().mockReturnValue('/summary'), + getHref: jest.fn().mockReturnValue('/test-href'), + getRelevantPath: jest.fn().mockReturnValue('/other-path'), + path: '/test-path' + } as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) await redirectOrMakeHandler( mockRequest, @@ -270,12 +285,22 @@ describe('redirectOrMakeHandler', () => { }) it('should set returnUrl when redirecting and next pages exist', async () => { - mockPage.getRelevantPath = jest.fn().mockReturnValue('/other-path') - mockPage.path = '/test-path' - mockPage.getHref = jest - .fn() - .mockReturnValueOnce('/summary-href') // First call: for summaryPath (returnUrl) - .mockReturnValueOnce('/relevant-path-href') // Second call: for relevantPath (redirect) + const testPage = { + getState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + mergeState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + getSummaryPath: jest.fn().mockReturnValue('/summary'), + getRelevantPath: jest.fn().mockReturnValue('/other-path'), + path: '/test-path', + getHref: jest + .fn() + .mockReturnValueOnce('/summary-href') // First call: for summaryPath (returnUrl) + .mockReturnValueOnce('/relevant-path-href') // Second call: for relevantPath (redirect) + } as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) ;(findPage as jest.Mock).mockReturnValue({ next: ['next-page'] }) await redirectOrMakeHandler( @@ -294,8 +319,19 @@ describe('redirectOrMakeHandler', () => { }) it('should not set returnUrl when redirecting and no next pages exist', async () => { - mockPage.getRelevantPath = jest.fn().mockReturnValue('/other-path') - mockPage.path = '/test-path' + const testPage = { + getState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + mergeState: jest + .fn() + .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), + getSummaryPath: jest.fn().mockReturnValue('/summary'), + getHref: jest.fn().mockReturnValue('/test-href'), + getRelevantPath: jest.fn().mockReturnValue('/other-path'), + path: '/test-path' + } as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) const returnUrlBefore = mockRequest.query.returnUrl ;(findPage as jest.Mock).mockReturnValue({ next: [] }) diff --git a/test/form/auth.test.js b/test/form/auth.test.js index 3d55666a0..9667e3959 100644 --- a/test/form/auth.test.js +++ b/test/form/auth.test.js @@ -1,7 +1,6 @@ import { join } from 'node:path' import basic from '@hapi/basic' -import Boom from '@hapi/boom' import { StatusCodes } from 'http-status-codes' import { FORM_PREFIX } from '~/src/server/constants.js' @@ -82,12 +81,14 @@ describe('Auth', () => { server = await createServer({ services, - onRequest: (request /*, params, definition, metadata */) => { + onRequest: (request, h /*, context */) => { const { auth } = request if (!auth.isAuthenticated) { - throw Boom.unauthorized() + return Promise.resolve(h.redirect('/unauthorized').takeover()) } + + return Promise.resolve(undefined) } }) @@ -112,7 +113,8 @@ describe('Auth', () => { url: `${basePath}/first-page` }) - expect(response.statusCode).toBe(StatusCodes.UNAUTHORIZED) + expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(response.headers.location).toBe('/unauthorized') }) test('authenticated get request to restricted form returns 200 OK', async () => { From f60e4e8647d709065913d300976b05d298469c81 Mon Sep 17 00:00:00 2001 From: Andrew Folga Date: Wed, 15 Oct 2025 17:35:13 +0100 Subject: [PATCH 07/19] Pass in metadata instead of grantCode to make it more generic --- src/server/plugins/engine/routes/index.ts | 7 ++++++- src/server/plugins/engine/types.ts | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 2c7eeb45f..5e99f2222 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -73,7 +73,12 @@ export async function redirectOrMakeHandler( // Call the onRequest callback if it has been supplied if (onRequest) { - const result = await onRequest(request, h as ResponseToolkit, context) + const result = await onRequest( + request, + h as ResponseToolkit, + context, + model.def.metadata ?? {} + ) if ( result && typeof result === 'object' && diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 52b14a0b3..3ecc9e0f4 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -367,7 +367,8 @@ export type PreparePageEventRequestOptions = ( export type OnRequestCallback = ( request: AnyFormRequest, h: ResponseToolkit, - context: FormContext + context: FormContext, + metadata: Record ) => Promise export type SaveAndExitHandler = ( From 112f343322b6fcc7c41123b6ad88d20a32dab524 Mon Sep 17 00:00:00 2001 From: Andrew Folga Date: Wed, 15 Oct 2025 18:36:02 +0100 Subject: [PATCH 08/19] Update tests --- .../plugins/engine/routes/index.test.ts | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index 62364e0b0..2ae7aea90 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -1,3 +1,4 @@ +import { type FormDefinition } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, type ResponseToolkit } from '@hapi/hapi' @@ -37,7 +38,11 @@ describe('redirectOrMakeHandler', () => { let mockPage: PageControllerClass const mockModel: FormModel = { - def: { metadata: {} }, + def: { + metadata: { + submission: { grantCode: 'TEST-GRANT' } + } as { submission: { grantCode: string } } + }, getFormContext: jest.fn().mockReturnValue({ isForceAccess: false, data: {} @@ -98,7 +103,46 @@ describe('redirectOrMakeHandler', () => { expect.objectContaining({ isForceAccess: false, data: {} + }), + mockModel.def.metadata + ) + }) + + it('should call onRequest callback with metadata as empty object if it does not exist', async () => { + const testModel: Partial = { + def: { + pages: [], + conditions: [], + lists: [], + sections: [], + metadata: undefined + } as FormDefinition, + getFormContext: jest.fn().mockReturnValue({ + isForceAccess: false, + data: {} }) + } + mockRequest.app = { model: testModel as unknown as FormModel } + + const onRequestCallback: OnRequestCallback = jest + .fn() + .mockResolvedValue(undefined) + + await redirectOrMakeHandler( + mockRequest, + mockH, + onRequestCallback, + mockMakeHandler + ) + + expect(onRequestCallback).toHaveBeenCalledWith( + mockRequest, + mockH as ResponseToolkit, + expect.objectContaining({ + isForceAccess: false, + data: {} + }), + expect.objectContaining({}) ) }) From e170e9b743628a6ba60d73882069ae9d57d9f0fb Mon Sep 17 00:00:00 2001 From: Andrew Folga Date: Wed, 15 Oct 2025 18:36:50 +0100 Subject: [PATCH 09/19] Update readme --- docs/PLUGIN_OPTIONS.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index a94b2541e..5a4417ec6 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -236,7 +236,8 @@ If provided, the `onRequest` plugin option will be invoked on each request to an export type OnRequestCallback = ( request: AnyFormRequest, h: ResponseToolkit, - context: FormContext + context: FormContext, + metadata: Record ) => Promise ``` @@ -246,7 +247,7 @@ Here's an example of how it could be used to secure access to forms: await server.register({ plugin, options: { - onRequest: async (request, h, context) => { + onRequest: async (request, h, context, metadata) => { const { auth } = request // Check if user is authenticated From b2dba8069999a3e1529458823d5f7a1dec71189f Mon Sep 17 00:00:00 2001 From: Andrew Folga Date: Thu, 16 Oct 2025 09:58:18 +0100 Subject: [PATCH 10/19] Pass form definition instead of metadata for entire config access --- docs/PLUGIN_OPTIONS.md | 4 +- .../plugins/engine/routes/index.test.ts | 41 +------------------ src/server/plugins/engine/routes/index.ts | 2 +- src/server/plugins/engine/types.ts | 3 +- 4 files changed, 6 insertions(+), 44 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index 5a4417ec6..1de2fe49b 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -237,7 +237,7 @@ export type OnRequestCallback = ( request: AnyFormRequest, h: ResponseToolkit, context: FormContext, - metadata: Record + formDefinition: FormDefinition ) => Promise ``` @@ -247,7 +247,7 @@ Here's an example of how it could be used to secure access to forms: await server.register({ plugin, options: { - onRequest: async (request, h, context, metadata) => { + onRequest: async (request, h, context, formDefinition) => { const { auth } = request // Check if user is authenticated diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index 2ae7aea90..c2bbfd891 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -1,4 +1,3 @@ -import { type FormDefinition } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, type ResponseToolkit } from '@hapi/hapi' @@ -104,45 +103,7 @@ describe('redirectOrMakeHandler', () => { isForceAccess: false, data: {} }), - mockModel.def.metadata - ) - }) - - it('should call onRequest callback with metadata as empty object if it does not exist', async () => { - const testModel: Partial = { - def: { - pages: [], - conditions: [], - lists: [], - sections: [], - metadata: undefined - } as FormDefinition, - getFormContext: jest.fn().mockReturnValue({ - isForceAccess: false, - data: {} - }) - } - mockRequest.app = { model: testModel as unknown as FormModel } - - const onRequestCallback: OnRequestCallback = jest - .fn() - .mockResolvedValue(undefined) - - await redirectOrMakeHandler( - mockRequest, - mockH, - onRequestCallback, - mockMakeHandler - ) - - expect(onRequestCallback).toHaveBeenCalledWith( - mockRequest, - mockH as ResponseToolkit, - expect.objectContaining({ - isForceAccess: false, - data: {} - }), - expect.objectContaining({}) + mockModel.def ) }) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 5e99f2222..4a9459c17 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -77,7 +77,7 @@ export async function redirectOrMakeHandler( request, h as ResponseToolkit, context, - model.def.metadata ?? {} + model.def ) if ( result && diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 3ecc9e0f4..18f4529fb 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -1,6 +1,7 @@ import { type ComponentDef, type Event, + type FormDefinition, type FormVersionMetadata, type Item, type List, @@ -368,7 +369,7 @@ export type OnRequestCallback = ( request: AnyFormRequest, h: ResponseToolkit, context: FormContext, - metadata: Record + definition: FormDefinition ) => Promise export type SaveAndExitHandler = ( From ed4bbe089dd7bb8a1ca912cc975931302410b0b6 Mon Sep 17 00:00:00 2001 From: Andrew Folga Date: Thu, 16 Oct 2025 10:17:06 +0100 Subject: [PATCH 11/19] Remove formDefinition as it can be fetched from request --- docs/PLUGIN_OPTIONS.md | 5 ++--- src/server/plugins/engine/routes/index.test.ts | 3 +-- src/server/plugins/engine/routes/index.ts | 7 +------ src/server/plugins/engine/types.ts | 4 +--- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index 1de2fe49b..a94b2541e 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -236,8 +236,7 @@ If provided, the `onRequest` plugin option will be invoked on each request to an export type OnRequestCallback = ( request: AnyFormRequest, h: ResponseToolkit, - context: FormContext, - formDefinition: FormDefinition + context: FormContext ) => Promise ``` @@ -247,7 +246,7 @@ Here's an example of how it could be used to secure access to forms: await server.register({ plugin, options: { - onRequest: async (request, h, context, formDefinition) => { + onRequest: async (request, h, context) => { const { auth } = request // Check if user is authenticated diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index c2bbfd891..07dcc831d 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -102,8 +102,7 @@ describe('redirectOrMakeHandler', () => { expect.objectContaining({ isForceAccess: false, data: {} - }), - mockModel.def + }) ) }) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 4a9459c17..2c7eeb45f 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -73,12 +73,7 @@ export async function redirectOrMakeHandler( // Call the onRequest callback if it has been supplied if (onRequest) { - const result = await onRequest( - request, - h as ResponseToolkit, - context, - model.def - ) + const result = await onRequest(request, h as ResponseToolkit, context) if ( result && typeof result === 'object' && diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 18f4529fb..52b14a0b3 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -1,7 +1,6 @@ import { type ComponentDef, type Event, - type FormDefinition, type FormVersionMetadata, type Item, type List, @@ -368,8 +367,7 @@ export type PreparePageEventRequestOptions = ( export type OnRequestCallback = ( request: AnyFormRequest, h: ResponseToolkit, - context: FormContext, - definition: FormDefinition + context: FormContext ) => Promise export type SaveAndExitHandler = ( From 3eddbbb90a3cfa1ca07479e500cc2316c9073f4a Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 17 Oct 2025 11:06:29 +0100 Subject: [PATCH 12/19] Update Toolkit type --- docs/PLUGIN_OPTIONS.md | 2 +- src/server/plugins/engine/types.ts | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index a94b2541e..5c91e3d7a 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -235,7 +235,7 @@ If provided, the `onRequest` plugin option will be invoked on each request to an ```ts export type OnRequestCallback = ( request: AnyFormRequest, - h: ResponseToolkit, + h: FormResponseToolkit, context: FormContext ) => Promise ``` diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 52b14a0b3..a31f88a14 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -9,8 +9,7 @@ import { import { type PluginProperties, type Request, - type ResponseObject, - type ResponseToolkit + type ResponseObject } from '@hapi/hapi' import { type JoiExpression, type ValidationErrorItem } from 'joi' @@ -366,7 +365,7 @@ export type PreparePageEventRequestOptions = ( export type OnRequestCallback = ( request: AnyFormRequest, - h: ResponseToolkit, + h: FormResponseToolkit, context: FormContext ) => Promise From 66d4e293bc3a26d78f4229317637c3379a4942d6 Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 17 Oct 2025 11:35:16 +0100 Subject: [PATCH 13/19] Always return if we get a result from onRequest --- src/server/plugins/engine/routes/index.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 2c7eeb45f..be29829a5 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -74,11 +74,7 @@ export async function redirectOrMakeHandler( // Call the onRequest callback if it has been supplied if (onRequest) { const result = await onRequest(request, h as ResponseToolkit, context) - if ( - result && - typeof result === 'object' && - (result as unknown as Record)._takeover === true - ) { + if (result !== undefined) { return result } } From 89afb783f3913a5c8248e0b6121cc708e49cf5f4 Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 17 Oct 2025 11:55:03 +0100 Subject: [PATCH 14/19] Remove onRequest takeover test --- .../plugins/engine/routes/index.test.ts | 39 ------------------- src/server/plugins/engine/routes/index.ts | 2 +- 2 files changed, 1 insertion(+), 40 deletions(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index 07dcc831d..b4af8ce5c 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -156,45 +156,6 @@ describe('redirectOrMakeHandler', () => { expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) }) - it('should continue processing when onRequest returns non-takeover response', async () => { - const nonTakeoverResponse = { - statusCode: 200, - _takeover: false - } as unknown as ResponseObject - - const onRequestCallback: OnRequestCallback = jest - .fn() - .mockResolvedValue(nonTakeoverResponse) - - await redirectOrMakeHandler( - mockRequest, - mockH, - onRequestCallback, - mockMakeHandler - ) - - expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) - }) - - it('should continue processing when onRequest returns response without _takeover property', async () => { - const responseWithoutTakeover = { - statusCode: 200 - } as unknown as ResponseObject - - const onRequestCallback: OnRequestCallback = jest - .fn() - .mockResolvedValue(responseWithoutTakeover) - - await redirectOrMakeHandler( - mockRequest, - mockH, - onRequestCallback, - mockMakeHandler - ) - - expect(mockMakeHandler).toHaveBeenCalledWith(mockPage, expect.any(Object)) - }) - it('should handle onRequest callback errors', async () => { const error = new Error('onRequest callback error') const onRequestCallback: OnRequestCallback = jest diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index be29829a5..1cf602b45 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -73,7 +73,7 @@ export async function redirectOrMakeHandler( // Call the onRequest callback if it has been supplied if (onRequest) { - const result = await onRequest(request, h as ResponseToolkit, context) + const result = await onRequest(request, h, context) if (result !== undefined) { return result } From 22a86d0a520d4234eeba57cbfce8769d90000ed5 Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 17 Oct 2025 12:43:12 +0100 Subject: [PATCH 15/19] Update onRequest types --- docs/PLUGIN_OPTIONS.md | 2 +- src/server/plugins/engine/types.ts | 2 +- test/form/auth.test.js | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index 5c91e3d7a..e5957d9d1 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -237,7 +237,7 @@ export type OnRequestCallback = ( request: AnyFormRequest, h: FormResponseToolkit, context: FormContext -) => Promise +) => ResponseObject | undefined | Promise ``` Here's an example of how it could be used to secure access to forms: diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index a31f88a14..46fade7af 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -367,7 +367,7 @@ export type OnRequestCallback = ( request: AnyFormRequest, h: FormResponseToolkit, context: FormContext -) => Promise +) => ResponseObject | undefined | Promise export type SaveAndExitHandler = ( request: FormRequestPayload, diff --git a/test/form/auth.test.js b/test/form/auth.test.js index 9667e3959..a60a5902b 100644 --- a/test/form/auth.test.js +++ b/test/form/auth.test.js @@ -85,10 +85,8 @@ describe('Auth', () => { const { auth } = request if (!auth.isAuthenticated) { - return Promise.resolve(h.redirect('/unauthorized').takeover()) + return h.redirect('/unauthorized').takeover() } - - return Promise.resolve(undefined) } }) From 804b17aa813fc230f2c23c083c1ff448ab559a7c Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 17 Oct 2025 14:54:23 +0100 Subject: [PATCH 16/19] Use h.continue in onRequest --- docs/PLUGIN_OPTIONS.md | 6 +++--- src/server/plugins/engine/helpers.test.ts | 3 ++- .../engine/pageControllers/PageController.test.ts | 3 ++- .../pageControllers/QuestionPageController.test.ts | 12 ++++++++---- .../pageControllers/SummaryPageController.test.ts | 3 ++- src/server/plugins/engine/routes/index.ts | 2 +- src/server/plugins/engine/routes/questions.test.ts | 6 ++++-- src/server/plugins/engine/types.ts | 5 ++++- src/server/routes/types.ts | 5 ++++- test/form/auth.test.js | 4 +++- 10 files changed, 33 insertions(+), 16 deletions(-) diff --git a/docs/PLUGIN_OPTIONS.md b/docs/PLUGIN_OPTIONS.md index e5957d9d1..9cf4599e0 100644 --- a/docs/PLUGIN_OPTIONS.md +++ b/docs/PLUGIN_OPTIONS.md @@ -237,7 +237,7 @@ export type OnRequestCallback = ( request: AnyFormRequest, h: FormResponseToolkit, context: FormContext -) => ResponseObject | undefined | Promise +) => ResponseObject | FormResponseToolkit['continue'] | Promise ``` Here's an example of how it could be used to secure access to forms: @@ -254,8 +254,8 @@ await server.register({ return h.redirect('/login').takeover() } - // Return undefined to continue with normal form processing - return undefined + // Return h.continue to resume with normal form processing + return h.continue } } }) diff --git a/src/server/plugins/engine/helpers.test.ts b/src/server/plugins/engine/helpers.test.ts index 323d645aa..6d8abc603 100644 --- a/src/server/plugins/engine/helpers.test.ts +++ b/src/server/plugins/engine/helpers.test.ts @@ -76,7 +76,8 @@ describe('Helpers', () => { h = { redirect: jest.fn().mockImplementation(() => response), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } }) diff --git a/src/server/plugins/engine/pageControllers/PageController.test.ts b/src/server/plugins/engine/pageControllers/PageController.test.ts index 8b30b4e98..10d5c52c8 100644 --- a/src/server/plugins/engine/pageControllers/PageController.test.ts +++ b/src/server/plugins/engine/pageControllers/PageController.test.ts @@ -160,7 +160,8 @@ describe('PageController', () => { const h: FormResponseToolkit = { redirect: jest.fn(), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } it('returns default route options', () => { diff --git a/src/server/plugins/engine/pageControllers/QuestionPageController.test.ts b/src/server/plugins/engine/pageControllers/QuestionPageController.test.ts index 32ef8a4e9..42c252ae8 100644 --- a/src/server/plugins/engine/pageControllers/QuestionPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/QuestionPageController.test.ts @@ -805,7 +805,8 @@ describe('QuestionPageController', () => { const h: FormResponseToolkit = { redirect: jest.fn().mockReturnValue(response), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } it('returns default route options', () => { @@ -1373,7 +1374,8 @@ describe('QuestionPageController V2', () => { const h: FormResponseToolkit = { redirect: jest.fn().mockReturnValue(response), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } it('returns default route options', () => { @@ -1532,7 +1534,8 @@ describe('Save and Exit functionality', () => { const h: FormResponseToolkit = { redirect: jest.fn().mockReturnValue(response), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } beforeEach(() => { @@ -1663,7 +1666,8 @@ describe('Save and Exit functionality', () => { const mockH = { redirect: jest.fn().mockReturnValue(mockResponse), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } const postHandler = controller1.makePostRouteHandler() diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 3e3a367ef..2241cf805 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -20,7 +20,8 @@ describe('SummaryPageController', () => { } const h: FormResponseToolkit = { redirect: jest.fn().mockReturnValue(response), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } beforeEach(() => { diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 1cf602b45..37225060a 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -74,7 +74,7 @@ export async function redirectOrMakeHandler( // Call the onRequest callback if it has been supplied if (onRequest) { const result = await onRequest(request, h, context) - if (result !== undefined) { + if (result !== h.continue) { return result } } diff --git a/src/server/plugins/engine/routes/questions.test.ts b/src/server/plugins/engine/routes/questions.test.ts index 135076c99..9b5e0ebe3 100644 --- a/src/server/plugins/engine/routes/questions.test.ts +++ b/src/server/plugins/engine/routes/questions.test.ts @@ -46,7 +46,8 @@ describe('makeGetHandler', () => { headers: { location: '/redirect-url' } }) }), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } beforeEach(() => { @@ -240,7 +241,8 @@ describe('makeGetHandler', () => { describe('makePostHandler', () => { const hMock: FormResponseToolkit = { redirect: jest.fn(), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } beforeEach(() => { diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 46fade7af..00f0e394f 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -367,7 +367,10 @@ export type OnRequestCallback = ( request: AnyFormRequest, h: FormResponseToolkit, context: FormContext -) => ResponseObject | undefined | Promise +) => + | ResponseObject + | FormResponseToolkit['continue'] + | Promise export type SaveAndExitHandler = ( request: FormRequestPayload, diff --git a/src/server/routes/types.ts b/src/server/routes/types.ts index 5639840fb..704fccac6 100644 --- a/src/server/routes/types.ts +++ b/src/server/routes/types.ts @@ -37,7 +37,10 @@ export interface FormRequestPayloadRefs extends FormRequestRefs { export type FormRequest = Request export type FormRequestPayload = Request -export type FormResponseToolkit = Pick +export type FormResponseToolkit = Pick< + ResponseToolkit, + 'redirect' | 'view' | 'continue' +> export enum FormAction { Continue = 'continue', diff --git a/test/form/auth.test.js b/test/form/auth.test.js index a60a5902b..372de169f 100644 --- a/test/form/auth.test.js +++ b/test/form/auth.test.js @@ -81,12 +81,14 @@ describe('Auth', () => { server = await createServer({ services, - onRequest: (request, h /*, context */) => { + onRequest: (request, h, _context) => { const { auth } = request if (!auth.isAuthenticated) { return h.redirect('/unauthorized').takeover() } + + return h.continue } }) From f707b84757d92e99af6fe69096b118b4fbad1d52 Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 17 Oct 2025 15:04:30 +0100 Subject: [PATCH 17/19] Add h.continue test --- src/server/plugins/engine/routes/index.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index b4af8ce5c..b18dfb420 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -31,7 +31,8 @@ describe('redirectOrMakeHandler', () => { const mockH: FormResponseToolkit = { redirect: jest.fn(), - view: jest.fn() + view: jest.fn(), + continue: Symbol('continue') } as unknown as FormResponseToolkit let mockPage: PageControllerClass @@ -141,10 +142,10 @@ describe('redirectOrMakeHandler', () => { expect(mockMakeHandler).not.toHaveBeenCalled() }) - it('should continue processing when onRequest returns undefined', async () => { + it('should continue processing when onRequest returns h.continue', async () => { const onRequestCallback: OnRequestCallback = jest .fn() - .mockResolvedValue(undefined) + .mockResolvedValue(mockH.continue) await redirectOrMakeHandler( mockRequest, From ab5ee8cc34bebc118c93f71cc9cb8e5b94e5e7e7 Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 17 Oct 2025 15:06:01 +0100 Subject: [PATCH 18/19] Remove "grants" code --- src/server/plugins/engine/routes/index.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index b18dfb420..826ccef48 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -40,8 +40,8 @@ describe('redirectOrMakeHandler', () => { const mockModel: FormModel = { def: { metadata: { - submission: { grantCode: 'TEST-GRANT' } - } as { submission: { grantCode: string } } + submission: { code: 'TEST-CODE' } + } as { submission: { code: string } } }, getFormContext: jest.fn().mockReturnValue({ isForceAccess: false, From f850f93620141ac2c2b1db5ba07b11cefbeab83e Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Mon, 20 Oct 2025 10:29:15 +0100 Subject: [PATCH 19/19] chore(release): #major