diff --git a/.env.sample b/.env.sample index 90941a2ed..5d927a6bb 100644 --- a/.env.sample +++ b/.env.sample @@ -54,6 +54,10 @@ OIDC_CLIENT_ID=runner OIDC_CLIENT_PRIVATE_JWK= # Refresh an access token when this many seconds or fewer remain (default 30) OIDC_ACCESS_TOKEN_EXPIRY_GRACE_SECONDS=30 +# Forms whose earlier submissions are linked to the signed-in citizen's account +# when they open the form's homepage. The same list as forms-submission-api +# has. formName is optional and only used in logs. +#SUBMISSION_LINKING_ALLOWED_FORMS=[{"formId":"688131eeff67f889d52c66cc","formName":"Land Use"}] FEEDBACK_VIA_EMAIL=defraforms@defra.gov.uk #PAYMENT_PROVIDER_API_KEY_TEST= diff --git a/README.md b/README.md index 1923f6887..c8c7bfbc1 100644 --- a/README.md +++ b/README.md @@ -199,6 +199,7 @@ See [https://github.com/node-config/node-config#readme](https://github.com/node- | OIDC_CLIENT_PRIVATE_JWK | This service's private assertion key, as a single RS256 JWK. | when signing in | | | Secret. The provider holds the public half. | | OIDC_SUBMISSION_API_RESOURCE | Resource this service asks the provider for tokens for. It becomes the access token's audience. | no | urn:defra:forms:forms-submission-api | | Must match the audience forms-submission-api accepts. | | OIDC_ACCESS_TOKEN_EXPIRY_GRACE_SECONDS | An access token with this many seconds or fewer left is refreshed before use. | no | 30 | | See [Token refresh](#token-refresh). | +| SUBMISSION_LINKING_ALLOWED_FORMS | Forms whose earlier submissions are linked to the account on the form's homepage, as a JSON array. | no | [] | | See [Linking earlier submissions](#linking-earlier-submissions). | ## Citizen sign in @@ -280,6 +281,39 @@ their ID token as the hint. The provider sends them back to `/auth/signed-out`: This service reads both endpoints from the provider's discovery document once, at the first sign in, so it finds a revocation endpoint added later only after a restart. +### Linking earlier submissions + +A citizen may have submitted a form before they had an account. Those submissions are not +linked to the account, so the homepage of the form does not list them. +`SUBMISSION_LINKING_ALLOWED_FORMS` names the forms whose earlier submissions are linked, for +example: + +``` +SUBMISSION_LINKING_ALLOWED_FORMS=[{"formId":"688131eeff67f889d52c66cc","formName":"Order a pizza"}] +``` + +`formName` is optional, and can be empty or null. It is a note for the people who set the +config and is only used in log messages, which call it the "possible form name" because nothing +checks it against the form. The service refuses to start if the value is not a JSON array of +forms with a `formId` each, or names a form twice. No forms are linked by default. + +When a signed-in citizen opens the homepage of a form on the list, live or preview, the first +thing the homepage does after it reads the form is ask forms-submission-api to link the +earlier submissions (`POST /form/submissions/link`). That API finds the submissions of the form +whose confirmation email matches the email in the citizen's access token and that are not yet +linked to an account, and links them. The homepage then reads the saved forms, so the list +includes the submissions just linked. + +The request waits for no more than 5 seconds. If it fails, the homepage logs a warning that +starts with `[linkSubmissions]` and shows the page as usual, without the submissions that +could not be linked. The citizen is not told. The link is tried again on the next visit to the +homepage, and finds nothing more to link once the earlier submissions are linked. + +forms-submission-api has the same setting and refuses any form not on its own list, so set the +same list on both services. forms-identity-ui adds the email to the access token only when +`OIDC_EMAIL_CLAIM_RESOURCE_SERVERS` names forms-submission-api. Without the email, +forms-submission-api refuses the link and the homepage logs a warning. + For proxy options, see https://www.npmjs.com/package/proxy-from-env which is used by https://github.com/TooTallNate/proxy-agents/tree/main/packages/proxy-agent. Example .env contents: @@ -311,6 +345,7 @@ OIDC_ISSUER=http://localhost:3011 OIDC_CLIENT_ID=runner OIDC_CLIENT_PRIVATE_JWK= OIDC_ACCESS_TOKEN_EXPIRY_GRACE_SECONDS=30 +SUBMISSION_LINKING_ALLOWED_FORMS=[{"formId":"","formName":""}] ``` # Testing diff --git a/src/config/index.ts b/src/config/index.ts index 71f7c206d..a1485e5ff 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -1,6 +1,7 @@ import { resolve } from 'node:path' import convict, { type SchemaObj } from 'convict' +import Joi from 'joi' import { type LevelWithSilent } from 'pino' import 'dotenv/config' @@ -9,6 +10,66 @@ const isProduction = process.env.NODE_ENV === 'production' const isDev = process.env.NODE_ENV !== 'production' const isTest = process.env.NODE_ENV === 'test' +const ALLOWED_FORMS_VALIDATOR = 'allowed-forms' + +/** + * A form whose earlier submissions a signed-in citizen can link to their + * account. The same setting as forms-submission-api has. + */ +export interface AllowedForm { + /** The ID of the form */ + formId: string + + /** + * The name of the form, as a note for the people who set the config. + * Nothing checks it against the form. + */ + formName?: string | null +} + +/** + * A list of forms. `formName` is only a note for the people who set the + * config, so it can be missing, empty or null. + */ +const allowedFormsSchema = Joi.array() + .items( + Joi.object({ + formId: Joi.string().required(), + formName: Joi.string().allow('', null) + }) + ) + .unique('formId') + +convict.addFormat({ + name: ALLOWED_FORMS_VALIDATOR, + validate(value: unknown) { + const { error } = allowedFormsSchema.validate(value) + + if (error) { + throw new TypeError(`must be a list of allowed forms: ${error.message}`) + } + }, + coerce(value: string): unknown { + if (value.trim() === '') { + return [] + } + + let forms: unknown + + try { + forms = JSON.parse(value) + } catch { + throw new TypeError('must be a JSON array of allowed forms') + } + + if (!Array.isArray(forms)) { + throw new TypeError('must be a JSON array of allowed forms') + } + + return forms + } +}) + export const config = convict({ appDir: { format: String, @@ -385,6 +446,15 @@ export const config = convict({ } as SchemaObj }, + submissionLinking: { + allowedForms: { + doc: 'The forms whose earlier submissions are linked to the account of a signed-in citizen when they open the homepage of the form, as a JSON array such as [{"formId":"688131eeff67f889d52c66cc","formName":"Order a pizza"}]. `formName` is optional and only helps people identify the form. Set the same list as forms-submission-api, which refuses any other form. No forms are linked by default.', + format: ALLOWED_FORMS_VALIDATOR, + default: [], + env: 'SUBMISSION_LINKING_ALLOWED_FORMS' + } as SchemaObj + }, + feedbackViaEmail: { doc: 'The email address (not including the mailto prefix) for feedback when the built-in CSAT form is disabled.', format: String, diff --git a/src/config/submission-linking.test.ts b/src/config/submission-linking.test.ts new file mode 100644 index 000000000..7af9c5074 --- /dev/null +++ b/src/config/submission-linking.test.ts @@ -0,0 +1,68 @@ +import { config } from '~/src/config/index.js' + +describe('submission linking configuration', () => { + const original = process.env.SUBMISSION_LINKING_ALLOWED_FORMS + + afterEach(() => { + if (original === undefined) { + delete process.env.SUBMISSION_LINKING_ALLOWED_FORMS + } else { + process.env.SUBMISSION_LINKING_ALLOWED_FORMS = original + } + jest.resetModules() + }) + + /** + * Loads the config again, with the allowed forms set to the given value + */ + async function loadWith(value: string) { + process.env.SUBMISSION_LINKING_ALLOWED_FORMS = value + jest.resetModules() + + const fresh = await import('~/src/config/index.js') + + return fresh.config.get('submissionLinking.allowedForms') + } + + it('allows no forms by default', () => { + expect(config.default('submissionLinking.allowedForms')).toEqual([]) + }) + + it('reads an empty value as no forms', async () => { + await expect(loadWith(' ')).resolves.toEqual([]) + }) + + it('reads a JSON array of forms, with or without a name', async () => { + const forms = [ + { formId: 'form-1', formName: 'Order a pizza' }, + { formId: 'form-2', formName: '' }, + { formId: 'form-3', formName: null }, + { formId: 'form-4' } + ] + + await expect(loadWith(JSON.stringify(forms))).resolves.toEqual(forms) + }) + + it.each([ + ['a value that is not JSON', 'form-1,form-2'], + ['a JSON value that is not an array', '{"formId":"form-1"}'] + ])('refuses %s', async (_name, value) => { + await expect(loadWith(value)).rejects.toThrow( + 'must be a JSON array of allowed forms' + ) + }) + + it.each([ + ['a form with no ID', '[{"formName":"Order a pizza"}]'], + [ + 'a form with a name that is not a string', + '[{"formId":"form-1","formName":1}]' + ], + ['the same form twice', '[{"formId":"form-1"},{"formId":"form-1"}]'], + ['an entry that is not a form', '["form-1"]'] + ])('refuses %s', async (_name, value) => { + await expect(loadWith(value)).rejects.toThrow( + 'must be a list of allowed forms' + ) + }) +}) diff --git a/src/server/routes/homepage.js b/src/server/routes/homepage.js index 6c729dcaf..c3026d32f 100644 --- a/src/server/routes/homepage.js +++ b/src/server/routes/homepage.js @@ -6,6 +6,7 @@ import Joi from 'joi' import { config } from '~/src/config/index.js' import { CITIZEN_AUTH_ROUTE_OPTIONS } from '~/src/server/auth/route-options.js' +import { logger } from '~/src/server/common/helpers/logging/logger.js' import { FormRowStatus, HOMEPAGE_PREFIX, @@ -23,7 +24,10 @@ import { getFormDefinition, getFormMetadata } from '~/src/server/services/formsService.js' -import { getFormRecords } from '~/src/server/services/submissionService.js' +import { + getFormRecords, + linkSubmissionsToUser +} from '~/src/server/services/submissionService.js' import { formRequiresAuthentication, getAllLanguages @@ -31,6 +35,7 @@ import { const authBase = config.get('oidc.issuer') const runnerBase = config.get('baseUrl') +const allowedForms = config.get('submissionLinking.allowedForms') /** * A saved form as the table shows it. The dates are formatted and the status @@ -80,6 +85,42 @@ function mapToRow(savedForm, translator, formId) { * @typedef { FormParams & { action?: string } } HomepageParams */ +/** + * Links the earlier submissions of the form to the account of the citizen, + * when the form is one of the forms whose submissions can be linked. A + * failure is logged and otherwise ignored, so the citizen still gets their + * homepage, without the submissions that could not be linked. + * @param {string} accessToken - the citizen's access token + * @param {string} formId - the form the homepage is for + */ +async function linkEarlierSubmissions(accessToken, formId) { + const allowedForm = allowedForms.find((form) => form.formId === formId) + + if (!allowedForm) { + return + } + + // The name is a note in the config, and nothing checks it against the form + const formNameNote = allowedForm.formName + ? ` (possible form name '${allowedForm.formName}')` + : '' + + try { + const { linked } = await linkSubmissionsToUser(accessToken, formId) + + if (linked > 0) { + logger.info( + `[linkSubmissions] Linked ${linked} earlier submissions of form ${formId}${formNameNote} to the account of the citizen` + ) + } + } catch (err) { + logger.warn( + err, + `[linkSubmissions] Could not link the earlier submissions of form ${formId}${formNameNote} to the account of the citizen` + ) + } +} + /** * Construct the tabs * @param {FormMetadata} form @@ -183,6 +224,11 @@ async function homepageHandler(request, h) { return Boom.notFound(`Form ${form.id} state ${state} not found`) } + const { accessToken } = request.auth.credentials + + // Before the saved forms are read, so they include the submissions linked + await linkEarlierSubmissions(accessToken, form.id) + const { translator } = await getFormTranslator( request, form, @@ -192,8 +238,6 @@ async function homepageHandler(request, h) { const nav = buildNavigation(form, translator, { isPreview, state, slug }) - const { accessToken } = request.auth.credentials - const savedForms = await getFormRecords(accessToken, form.id, previewStatus) const savedAndInFlightForms = addInFlightForms( diff --git a/src/server/services/submissionService.js b/src/server/services/submissionService.js index 24afd59bd..20eb15e7f 100644 --- a/src/server/services/submissionService.js +++ b/src/server/services/submissionService.js @@ -11,6 +11,12 @@ import { const submissionUrl = config.get('submissionUrl') +/** + * How long the homepage waits for earlier submissions to be linked, so a slow + * submission API delays the page by no more than this + */ +const LINK_SUBMISSIONS_TIMEOUT_MS = 5000 + /** * Persists uploaded files so they are kept beyond the initial upload expiry. * The retention period is set by the S3 lifecycle rules for the submission @@ -145,6 +151,40 @@ export async function getFormRecords(accessToken, formId, preview) { return /** @type {SavedForm[]} */ (payload) } +/** + * Links the earlier submissions of a form to the account of the citizen. The + * submission API finds them by the email in the access token, and only for + * the forms it allows. + * @param {string} accessToken - the citizen's access token + * @param {string} formId - the form the submissions belong to + * @returns {Promise} + */ +export async function linkSubmissionsToUser(accessToken, formId) { + const { res, error, payload } = await postJson( + `${submissionUrl}/form/submissions/link`, + { + payload: { formId }, + headers: { authorization: `Bearer ${accessToken}` }, + timeout: LINK_SUBMISSIONS_TIMEOUT_MS + } + ) + + if (error) { + throw new Error( + `Could not link the submissions of form ${formId} - ${res.statusCode}`, + { cause: error } + ) + } + + return /** @type {LinkSubmissionsResult} */ (payload) +} + +/** + * The result of linking earlier submissions to the account of a citizen + * @typedef {object} LinkSubmissionsResult + * @property {number} linked - the number of submissions linked + */ + /** * One saved form, as forms-submission-api describes it. * @typedef {object} SavedForm diff --git a/src/server/services/submissionService.test.js b/src/server/services/submissionService.test.js index 7f606d31d..77f7e3442 100644 --- a/src/server/services/submissionService.test.js +++ b/src/server/services/submissionService.test.js @@ -14,6 +14,7 @@ import { getSaveAndExitDetails, getSavedFormState, getSubmissionRecord, + linkSubmissionsToUser, persistFiles, submit, validateSaveAndExitCredentials @@ -362,6 +363,47 @@ describe('Submission service', () => { ) }) }) + + describe('linkSubmissionsToUser', () => { + it('asks the submission API to link the submissions of the form, naming the citizen by the token alone', async () => { + jest.mocked(postJson).mockResolvedValue({ + res: /** @type {IncomingMessage} */ ({ statusCode: StatusCodes.OK }), + payload: { linked: 2 } + }) + + await expect( + linkSubmissionsToUser(ACCESS_TOKEN, FORM_ID) + ).resolves.toEqual({ linked: 2 }) + + expect(postJson).toHaveBeenCalledWith( + `${SUBMISSION_URL}/form/submissions/link`, + { + payload: { formId: FORM_ID }, + headers: { authorization: `Bearer ${ACCESS_TOKEN}` }, + timeout: 5000 + } + ) + }) + + it('reports an error response rather than returning it as a result', async () => { + const error = new Error('Forbidden') + jest.mocked(postJson).mockResolvedValue( + /** @type {Awaited>} */ ({ + res: { statusCode: StatusCodes.FORBIDDEN }, + error + }) + ) + + await expect( + linkSubmissionsToUser(ACCESS_TOKEN, FORM_ID) + ).rejects.toThrow( + new Error( + `Could not link the submissions of form ${FORM_ID} - ${StatusCodes.FORBIDDEN}`, + { cause: error } + ) + ) + }) + }) }) /** diff --git a/test/auth/homepage.test.js b/test/auth/homepage.test.js index d2d5fa8a0..95efe6987 100644 --- a/test/auth/homepage.test.js +++ b/test/auth/homepage.test.js @@ -8,6 +8,7 @@ import { StatusCodes } from 'http-status-codes' import * as client from 'openid-client' import { config } from '~/src/config/index.js' +import { logger } from '~/src/server/common/helpers/logging/logger.js' import { FormRowStatus } from '~/src/server/constants.js' import { createServer } from '~/src/server/index.js' import { addInFlightForms } from '~/src/server/routes/homepage.js' @@ -15,11 +16,28 @@ import { getFormDefinition, getFormMetadata } from '~/src/server/services/formsService.js' -import { getFormRecords } from '~/src/server/services/submissionService.js' +import { + getFormRecords, + linkSubmissionsToUser +} from '~/src/server/services/submissionService.js' import * as fixtures from '~/test/fixtures/index.js' import { renderResponse } from '~/test/helpers/component-helpers.js' import { citizenSession } from '~/test/utils/citizen-session.js' +// The homepage reads the allowed forms once, when it loads, so they are set +// here, before anything imports it +jest.mock('~/src/config/index', () => { + const actual = jest.requireActual('~/src/config/index') + + actual.config.set('submissionLinking.allowedForms', [ + { formId: 'allowed-form-with-name', formName: 'Test form' }, + { formId: 'allowed-form-with-empty-name', formName: '' }, + { formId: 'allowed-form-with-null-name', formName: null }, + { formId: 'allowed-form-with-no-name' } + ]) + + return actual +}) jest.mock('~/src/server/services/formsService.js') jest.mock('~/src/server/services/submissionService.js') jest.mock('openid-client', () => ({ @@ -929,6 +947,105 @@ describe('per-form homepage', () => { }) }) + describe('linking earlier submissions', () => { + /** + * Makes the form of the homepage one of the allowed forms, set at the top + * of this file + * @param {string} formId + */ + function useAllowedForm(formId) { + jest + .mocked(getFormMetadata) + .mockResolvedValue({ ...fixtures.form.metadata, id: formId }) + } + + it('links the earlier submissions of an allowed form before it lists the saved forms', async () => { + useAllowedForm('allowed-form-with-name') + jest.mocked(linkSubmissionsToUser).mockResolvedValue({ linked: 1 }) + jest.mocked(getFormRecords).mockResolvedValue(savedForms) + const info = jest.spyOn(logger, 'info').mockImplementation() + + const { container } = await renderResponse(server, { + method: 'GET', + url: HOMEPAGE_URL, + auth: { strategy: 'citizen-session', credentials } + }) + + expect(linkSubmissionsToUser).toHaveBeenCalledWith( + 'access-1', + 'allowed-form-with-name' + ) + expect( + jest.mocked(linkSubmissionsToUser).mock.invocationCallOrder[0] + ).toBeLessThan(jest.mocked(getFormRecords).mock.invocationCallOrder[0]) + expect(info).toHaveBeenCalledWith( + `[linkSubmissions] Linked 1 earlier submissions of form allowed-form-with-name (possible form name 'Test form') to the account of the citizen` + ) + expect(container.getByRole('table')).toBeInTheDocument() + }) + + it('links the earlier submissions of an allowed form from a preview homepage', async () => { + useAllowedForm('allowed-form-with-no-name') + jest.mocked(linkSubmissionsToUser).mockResolvedValue({ linked: 0 }) + + const response = await server.inject({ + method: 'GET', + url: `/homepage/preview/${FormStatus.Draft}/test-form`, + auth: { strategy: 'citizen-session', credentials } + }) + + expect(response.statusCode).toBe(StatusCodes.OK) + expect(linkSubmissionsToUser).toHaveBeenCalledWith( + 'access-1', + 'allowed-form-with-no-name' + ) + }) + + it('does not link the submissions of a form that is not allowed', async () => { + // The form of the fixtures is not one of the allowed forms + const response = await server.inject({ + method: 'GET', + url: HOMEPAGE_URL, + auth: { strategy: 'citizen-session', credentials } + }) + + expect(response.statusCode).toBe(StatusCodes.OK) + expect(linkSubmissionsToUser).not.toHaveBeenCalled() + expect(getFormRecords).toHaveBeenCalled() + }) + + it.each([ + ['a name', 'allowed-form-with-name', ` (possible form name 'Test form')`], + ['an empty name', 'allowed-form-with-empty-name', ''], + ['a null name', 'allowed-form-with-null-name', ''], + ['no name', 'allowed-form-with-no-name', ''] + ])( + 'logs a warning and still shows the homepage when the link fails, for an allowed form with %s', + async (_name, formId, formNameNote) => { + useAllowedForm(formId) + const error = new Error('Could not link') + jest.mocked(linkSubmissionsToUser).mockRejectedValue(error) + jest.mocked(getFormRecords).mockResolvedValue(savedForms) + const warn = jest.spyOn(logger, 'warn').mockImplementation() + + const { response, container } = await renderResponse(server, { + method: 'GET', + url: HOMEPAGE_URL, + auth: { strategy: 'citizen-session', credentials } + }) + + expect(response.statusCode).toBe(StatusCodes.OK) + expect(warn).toHaveBeenCalledWith( + error, + `[linkSubmissions] Could not link the earlier submissions of form ${formId}${formNameNote} to the account of the citizen` + ) + expect(getFormRecords).toHaveBeenCalled() + expect(container.getByRole('table')).toBeInTheDocument() + expect(container.queryByText(/problem/i)).not.toBeInTheDocument() + } + ) + }) + describe('page width', () => { it('gives the homepage the room its table needs, and leaves other pages alone', async () => { const homepage = await server.inject({