Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(mrf): backend validation for field locking #7143

Merged
merged 6 commits into from
Mar 25, 2024
Merged

feat(mrf): backend validation for field locking #7143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1d7695c
feat(mrf): backend validation for field locking
justynoh Mar 18, 2024
5297d1a
refactor: move BE typeguards to its own file mimicking FE structure
KenLSM Mar 20, 2024
5a64176
refactor: rename logic* to shared*
KenLSM Mar 20, 2024
b4479ec
refactor: use break instead of continue, clean up type assertions
justynoh Mar 25, 2024
3e882c6
refactor: remove redundant existence check, clean up comments in vali…
justynoh Mar 25, 2024
721fd0a
refactor: use lodash isEqual instead of writing custom comparators, r…
justynoh Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions frontend/src/features/public-form/PublicFormContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { UseQueryResult } from 'react-query'
import { MultirespondentSubmissionDto } from '~shared/types'
import { PublicFormViewDto } from '~shared/types/form'

import { decryptSubmission } from './utils/decryptSubmission'

export type SubmissionData = {
/** Submission id */
id: string | undefined
Expand Down Expand Up @@ -65,6 +67,10 @@ export interface PublicFormContextProps
setNumVisibleFields?: Dispatch<SetStateAction<number>>

encryptedPreviousSubmission?: MultirespondentSubmissionDto
previousSubmission?: ReturnType<typeof decryptSubmission>
setPreviousSubmission: (
KenLSM marked this conversation as resolved.
Show resolved Hide resolved
previousSubmission: ReturnType<typeof decryptSubmission>,
) => void
}

export const PublicFormContext = createContext<
Expand Down
12 changes: 11 additions & 1 deletion frontend/src/features/public-form/PublicFormProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {
} from '~features/verifiable-fields'

import { FormNotFound } from './components/FormNotFound'
import { decryptSubmission } from './utils/decryptSubmission'
import { usePublicAuthMutations, usePublicFormMutations } from './mutations'
import { PublicFormContext, SubmissionData } from './PublicFormContext'
import { useEncryptedSubmission, usePublicFormView } from './queries'
Expand Down Expand Up @@ -148,6 +149,9 @@ export const PublicFormProvider = ({
/* enabled= */ !submissionData,
)

const [previousSubmission, setPreviousSubmission] =
useState<ReturnType<typeof decryptSubmission>>()

// Replace form fields, logic, and workflow with the previous version for MRF consistency.
if (data && encryptedPreviousSubmission) {
data.form.form_fields = encryptedPreviousSubmission.form_fields
Expand Down Expand Up @@ -300,7 +304,11 @@ export const PublicFormProvider = ({
submitStorageModeFormFetchMutation,
submitMultirespondentFormMutation,
updateMultirespondentSubmissionMutation,
} = usePublicFormMutations(formId, previousSubmissionId)
} = usePublicFormMutations(
formId,
previousSubmissionId,
previousSubmission?.submissionSecretKey,
)

const { handleLogoutMutation } = usePublicAuthMutations(formId)

Expand Down Expand Up @@ -694,6 +702,8 @@ export const PublicFormProvider = ({
isPreview: false,
setNumVisibleFields,
encryptedPreviousSubmission,
previousSubmission,
setPreviousSubmission,
...commonFormValues,
...data,
...rest,
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/features/public-form/PublicFormService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export type SubmitStorageFormWithVirusScanningArgs =

export type SubmitMultirespondentFormWithVirusScanningArgs =
SubmitEmailFormArgs & {
// publicKey: string
submissionSecretKey?: string
fieldIdToQuarantineKeyMap: FieldIdToQuarantineKeyType[]
}

Expand Down Expand Up @@ -369,6 +369,7 @@ export const updateMultirespondentSubmission = async ({
captchaType = '',
responseMetadata,
fieldIdToQuarantineKeyMap,
submissionSecretKey,
}: SubmitMultirespondentFormWithVirusScanningArgs & {
submissionId?: string
}) => {
Expand All @@ -383,6 +384,7 @@ export const updateMultirespondentSubmission = async ({
formFields,
formInputs: filteredInputs,
responseMetadata,
submissionSecretKey,
version: MULTIRESPONDENT_FORM_SUBMISSION_VERSION,
},
fieldIdToQuarantineKeyMap,
Expand Down
8 changes: 4 additions & 4 deletions frontend/src/features/public-form/components/FormFields/FormFieldsContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useMemo, useState } from 'react'
import { useMemo } from 'react'
import { useSearchParams } from 'react-router-dom'
import { Box } from '@chakra-ui/react'

Expand All @@ -24,11 +24,10 @@ export const FormFieldsContainer = (): JSX.Element | null => {
handleSubmitForm,
submissionData,
encryptedPreviousSubmission,
previousSubmission,
setPreviousSubmission,
} = usePublicFormContext()

KenLSM marked this conversation as resolved.
Show resolved Hide resolved
const [previousSubmission, setPreviousSubmission] =
useState<ReturnType<typeof decryptSubmission>>()

const { submissionPublicKey = null, workflowStep } =
encryptedPreviousSubmission ?? {}
const [searchParams] = useSearchParams()
Expand Down Expand Up @@ -120,6 +119,7 @@ export const FormFieldsContainer = (): JSX.Element | null => {
handleSubmitForm,
submissionPublicKey,
queryParams.key,
setPreviousSubmission,
encryptedPreviousSubmission,
])

Expand Down
5 changes: 4 additions & 1 deletion frontend/src/features/public-form/mutations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export const usePublicAuthMutations = (formId: string) => {
export const usePublicFormMutations = (
formId: string,
submissionId?: string,
submissionSecretKey?: string,
) => {
const submitEmailModeFormMutation = useMutation(
(args: Omit<SubmitEmailFormArgs, 'formId'>) => {
Expand Down Expand Up @@ -176,7 +177,9 @@ export const usePublicFormMutations = (
)

const updateMultirespondentSubmissionMutation =
useSubmitStorageModeFormMutation(updateMultirespondentSubmission)
useSubmitStorageModeFormMutation((args) =>
updateMultirespondentSubmission({ ...args, submissionSecretKey }),
)

return {
submitEmailModeFormMutation,
Expand Down
80 changes: 70 additions & 10 deletions frontend/src/features/public-form/utils/createSubmission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ type CreateStorageSubmissionFormDataArgs = CreateEmailSubmissionFormDataArgs & {

type CreateMultirespondentSubmissionFormDataArgs =
CreateEmailSubmissionFormDataArgs & {
submissionSecretKey?: string
version: number
}

Expand Down Expand Up @@ -268,23 +269,80 @@ const createResponsesV3 = (
case BasicField.Uen:
case BasicField.Date:
case BasicField.CountryRegion:
case BasicField.YesNo:
case BasicField.YesNo: {
const input = formInputs[ff._id] as
| FormFieldValue<typeof ff.fieldType>
| undefined
if (!input) break
returnedInputs[ff._id] = {
fieldType: ff.fieldType,
answer: input,
} as FieldResponseV3
break
}
case BasicField.Email:
case BasicField.Mobile:
case BasicField.Table:
case BasicField.Checkbox:
case BasicField.Mobile: {
const input = formInputs[ff._id] as
| FormFieldValue<typeof ff.fieldType>
| undefined
if (!input?.value) break
returnedInputs[ff._id] = {
fieldType: ff.fieldType,
answer: input,
} as FieldResponseV3
break
}
case BasicField.Table: {
const input = formInputs[ff._id] as
| FormFieldValue<typeof ff.fieldType>
| undefined
if (!input) break
if (input.every((row) => Object.values(row).every((value) => !value))) {
break
}
returnedInputs[ff._id] = {
fieldType: ff.fieldType,
answer: input,
} as FieldResponseV3
break
}
case BasicField.Checkbox: {
const input = formInputs[ff._id] as
| FormFieldValue<typeof ff.fieldType>
| undefined
if (
(!input?.value || input?.value.length === 0) &&
!input?.othersInput
) {
break
}
returnedInputs[ff._id] = {
fieldType: ff.fieldType,
answer: input,
} as FieldResponseV3
break
}
case BasicField.Children: {
const input = formInputs[ff._id] as FormFieldValue<typeof ff.fieldType>
if (!input) continue
const input = formInputs[ff._id] as
| FormFieldValue<typeof ff.fieldType>
| undefined
if (
!input ||
input.child.every((child) => child.every((value) => !value))
) {
break
}
returnedInputs[ff._id] = {
fieldType: ff.fieldType,
answer: input,
} as FieldResponseV3
break
}
case BasicField.Attachment: {
const input = formInputs[ff._id] as FormFieldValue<typeof ff.fieldType>
if (!input) continue
const input = formInputs[ff._id] as
| FormFieldValue<typeof ff.fieldType>
| undefined
if (!input) break
// for each attachment response, find the corresponding quarantine bucket key
const fieldIdToQuarantineKeyEntry = fieldIdToQuarantineKeyMap.find(
(v) => v.fieldId === ff._id,
Expand All @@ -303,8 +361,10 @@ const createResponsesV3 = (
break
}
case BasicField.Radio: {
const input = formInputs[ff._id] as FormFieldValue<typeof ff.fieldType>
if (!input) continue
const input = formInputs[ff._id] as
| FormFieldValue<typeof ff.fieldType>
| undefined
if (!input?.value && !input?.othersInput) break
returnedInputs[ff._id] = {
fieldType: ff.fieldType,
answer: input.othersInput
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/features/public-form/utils/decryptSubmission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const decryptSubmission = ({
}):
| (Omit<MultirespondentSubmissionDto, 'encryptedContent' | 'version'> & {
responses: FieldResponsesV3
submissionSecretKey: string
})
| undefined => {
if (!submission) throw Error('Encrypted submission undefined')
Expand All @@ -28,5 +29,6 @@ export const decryptSubmission = ({
return {
...rest,
responses: decryptedContent.responses as FieldResponsesV3,
submissionSecretKey: secretKey,
}
}
45 changes: 45 additions & 0 deletions shared/utils/response-v3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import _ from 'lodash'
import { BasicField, FieldResponseV3 } from '../types'

export const isFieldResponseV3Equal = (
l: FieldResponseV3,
r: FieldResponseV3,
): boolean => {
if (l.fieldType !== r.fieldType) return false

switch (l.fieldType) {
case BasicField.Number:
case BasicField.Decimal:
case BasicField.ShortText:
case BasicField.LongText:
case BasicField.HomeNo:
case BasicField.Dropdown:
case BasicField.Rating:
case BasicField.Nric:
case BasicField.Uen:
case BasicField.Date:
case BasicField.CountryRegion:
case BasicField.YesNo:
case BasicField.Email:
case BasicField.Mobile:
case BasicField.Table:
case BasicField.Radio:
case BasicField.Checkbox:
case BasicField.Children:
return _.isEqual(l.answer, r.answer)
case BasicField.Attachment: {
const rAnswer = r.answer as typeof l.answer
return (
l.answer.answer === rAnswer.answer &&
l.answer.hasBeenScanned === rAnswer.hasBeenScanned
)
}
case BasicField.Section:
return true
default: {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const _: never = l
return false
}
}
}
11 changes: 5 additions & 6 deletions ...pp/modules/submission/multirespondent-submission/multirespondent-submission.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,9 @@ const updateMultirespondentSubmission = async (
await submission.save()
} catch (err) {
logger.error({
message: 'Encrypt submission save error',
message: 'Multirespondent submission save error',
meta: {
action: 'onEncryptSubmissionFailure',
action: 'onMultirespondentSubmissionFailure',
...createReqMeta(req),
},
error: err,
Expand Down Expand Up @@ -491,8 +491,7 @@ export const handleMultirespondentSubmission = [
MultirespondentSubmissionMiddleware.validateMultirespondentSubmissionParams,
MultirespondentSubmissionMiddleware.createFormsgAndRetrieveForm,
MultirespondentSubmissionMiddleware.scanAndRetrieveAttachments,
// TODO(MRF/FRM-1592): Add validation for FieldResponsesV3
// EncryptSubmissionMiddleware.validateStorageSubmission,
MultirespondentSubmissionMiddleware.validateMultirespondentSubmission,
MultirespondentSubmissionMiddleware.encryptSubmission,
submitMultirespondentForm,
] as ControllerHandler[]
Expand All @@ -501,10 +500,10 @@ export const handleUpdateMultirespondentSubmission = [
CaptchaMiddleware.validateCaptchaParams,
TurnstileMiddleware.validateTurnstileParams,
ReceiverMiddleware.receiveMultirespondentSubmission,
MultirespondentSubmissionMiddleware.validateMultirespondentSubmissionParams,
MultirespondentSubmissionMiddleware.validateUpdateMultirespondentSubmissionParams,
MultirespondentSubmissionMiddleware.createFormsgAndRetrieveForm,
MultirespondentSubmissionMiddleware.scanAndRetrieveAttachments,
// EncryptSubmissionMiddleware.validateStorageSubmission,
MultirespondentSubmissionMiddleware.validateMultirespondentSubmission,
MultirespondentSubmissionMiddleware.setCurrentWorkflowStep,
MultirespondentSubmissionMiddleware.encryptSubmission,
updateMultirespondentSubmission,
Comment on lines +503 to 509
Copy link
Contributor

@KenLSM KenLSM Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping that we could use ensures here as rejections in any of these middlewares will not be grouped correctly on datadog. This can result in us having difficulty tracing back why this request failed.

I would consider these (MultirespondentSubmissionMiddleware.createFormsgAndRetrieveForm onwards) middlewares to be high-information spans that should be grouped together. high-info as they provide more contextual information instead of just plain celebrate/joi validations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this is going to be quite a bit of refactoring. can clarify what in particular we gain out of this change? Encrypt works the same way - i feel like if we're going to change it, we should change everything at once go

Copy link
Contributor

@KenLSM KenLSM Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4216 the middleware spans are no longer tracked causing errors originating from the middlewares to no longer be registered under the same span / trace.

This results in difficulty in tracing bugs as what we'll see on Datadog is simply an error with no further traceble logs. In order to find out what happened, we'll have to search for errors within a short window, which isn't very efficient, or robust, or making good use of DD with the amount of $$ we're paying.

I tried doing something like this locally but I didn't see any change in terms of the dd context object that's attached on the request lifecycle. Didn't try exploring this further since we're running on tight timeline.

+import tracer from 'dd-trace'


-  MultirespondentSubmissionMiddleware.validateMultirespondentSubmissionParams,
+  tracer.wrap(
+    'MultirespondentSubmissionMiddleware.validateMultirespondentSubmissionParams',
+    MultirespondentSubmissionMiddleware.validateMultirespondentSubmissionParams,
+  ),

cc: @timotheeg since we happened to discuss about this in the EngOps earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to open a linear ticket for this! Thanks for bringing it up

Expand Down
Loading
Loading