fix: backend validation does not prevent responses on hidden fields #809

tshuli · 2020-12-07T04:01:23Z

Problem

Builds on #736
Closes #732
Closes #779

Solution

Add check for isVisible in validateField() for email mode submissions
Also adds / updates unit tests

Tests

Create a email mode form with Yes/No field, and a Number Field hidden behind it. Attempt to submit a number response programatically with the number field still hidden. Submission should fail.
Now unhide the Number Field. Attempt to submit a response on the number field normally. Submission should pass.
Create email mode form with all field types (including Yes/No) hidden behind a Yes/No field. Submit while keeping the fields hidden. Submission should pass.
Create storage mode form with a mobile, email and short text field hidden behind a Yes/No field. Unhide the mobile and email fields and submit a response. Submission should pass.
Create storage mode form with a mobile, email and short text field hidden behind a Yes/No field. Submit a response without unhiding the fields. Submission should pass.

tshuli · 2020-12-08T14:39:51Z

@mantariksh In this PR, I limited the scope of the hidden field response check to email mode forms only; see/src/app/utils/field-validation/index.ts:

  if (
    responseMode === ResponseMode.Email &&
    isResponsePresentOnHiddenField(response)
  ) {
    return err(
      new ValidateFieldError(`Attempted to submit response on a hidden field`),
    )
  }

We discussed that validateField is not appropriate to do logic validation, and we should instead set isVisible to be true for encrypt mode. This can be done in src/app/modules/submission/submission.service.ts:

    const processingResponse: ProcessedFieldResponse = {
      ...response,
      isVisible:
        // Set isVisible as true for Encrypt mode because we cannot tell
        // Prevents downstream validateField from incorrectly preventing
        // encrypt mode submissions with responses on hidden fields
        form.responseMode === ResponseMode.Encrypt
          ? true
          : visibleFieldIds.has(responseId),
      question: formField.getQuestion(),
    }

However for required fields, if the field is visible, we also check if the field is non-empty, for both email and encrypt mode forms - see src/app/utils/field-validation/index.ts. Setting isVisible to be true for all encrypted mode fields leads to an error here as there is no response for the required, 'visible' field. This means that we now instead have to check, in singleAnswerRequiresValidation(), if the form is an email or encrypt mode form.

const singleAnswerRequiresValidation = (
  formField: IField,
  response: ProcessedSingleAnswerResponse,
) => (formField.required && response.isVisible) || response.answer.trim() !== ''

Basically, there is no way around this issue because fundamentally both email and encrypt mode submissions are processed in the same way today . For the scope of this PR which is to implement a bug fix for responses in email mode submissions, I suggest we stick to the more straightforward implementation which is to check for responses on hidden fields only on email mode submissions. When the sharding is done in #780, we can do away with isVisible for encrypt mode and this will solve the problem more fundamentally.

For your consideration

tshuli · 2020-12-09T05:15:14Z

@mantariksh as discussed, i've changed it to set isVisible = true for encrypt mode if there are responses. thanks for reviewing!

mantariksh

main question is about deleted jasmine tests

mantariksh · 2020-12-09T06:05:36Z

src/app/utils/field-validation/index.ts

+ */
+const isResponsePresentOnHiddenField = (response: FieldResponse): boolean => {
+  if (isProcessedSingleAnswerResponse(response)) {
+    if (!response.isVisible && response.answer.trim() !== '') {


I think this will be much clearer if you return early at the start: if (response.isVisible) return false

thanks! edited

ah but then you can also remove all the `!response.isVisible. subsequently!

mantariksh · 2020-12-09T08:40:19Z

src/app/modules/submission/__tests__/submission/submission.service.spec.ts

+        mobileField,
+        '+6587654321',
+      )
+      mobileResponse.isVisible = false


this made me realise that the tests are no longer behaving as intended. the object which we pass into getProcessedResponses should be a FieldResponse, not some form of ProcessedResponse. might it be better to write a generateFieldResponse function which doesn't include isVisible or isUserVerified? or even better, rename generateSingleAnswerResponse to generateProcessedSingleAnswerResponse?

mantariksh · 2020-12-09T08:40:59Z

src/types/response/index.ts

@@ -25,7 +25,7 @@ export interface ICheckboxResponse extends IBaseResponse {
  answerArray: string[]
 }

-export type ITableRow = string[]
+export type ITableRow = Array<string>


nit: can we standardise to string[]? that's how it's written everywhere else in the codebase

mantariksh · 2020-12-09T08:41:52Z

tests/unit/backend/controllers/email-submissions.server.controller.spec.js

@@ -1068,89 +1058,6 @@ describe('Email Submissions Controller', () => {
      prepareSubmissionThenCompare(expected, done)
    })

-    it('excludes field if isVisible is false for autoReplyData', (done) => {


why are all these tests deleted?

Thanks for highlighting. I removed them initially as they were invalid since they were inputting responses to hidden fields, but on review, I've instead fixed the faulty values

mantariksh

thank you for the comprehensive work on the tests!

mantariksh · 2020-12-21T07:27:03Z

src/app/utils/field-validation/index.ts

+ */
+const isResponsePresentOnHiddenField = (response: FieldResponse): boolean => {
+  if (isProcessedSingleAnswerResponse(response)) {
+    if (!response.isVisible && response.answer.trim() !== '') {


ah but then you can also remove all the `!response.isVisible. subsequently!

tests/unit/backend/helpers/generate-form-data.ts

… length

…t instead of using string concatenation

…sedResponse

tshuli force-pushed the fix/form-logic-validation branch from fa02efb to 4b9ac70 Compare December 7, 2020 07:47

tshuli requested review from liangyuanruo and mantariksh December 7, 2020 12:07

tshuli force-pushed the fix/form-logic-validation branch 3 times, most recently from 4b9ac70 to 21ea9d7 Compare December 8, 2020 14:29

tshuli force-pushed the fix/form-logic-validation branch from 21ea9d7 to 2ac2006 Compare December 9, 2020 05:13

mantariksh suggested changes Dec 9, 2020

View reviewed changes

liangyuanruo removed their request for review December 15, 2020 09:16

tshuli force-pushed the fix/form-logic-validation branch from 2ac2006 to 09a7a6c Compare December 21, 2020 05:20

tshuli requested a review from mantariksh December 21, 2020 05:38

mantariksh approved these changes Dec 21, 2020

View reviewed changes

tshuli added 16 commits December 21, 2020 15:43

fix: add check for responses on hidden fields

48cf9a6

fix: correct isVisible values for tests

0809b65

chore: update status code on ValidateFieldError

c3fca47

chore: add tests for hidden fields in field validator specs

4763da1

refactor: type FieldResponse as union of Response interfaces

a482ce9

refactor: use else if

54e4439

fix: check for table response on hidden field

dea52a5

fix: concatenate flattened array for table response before evaluating…

d139fb4

… length

refactor: beforeAll and afterAll for jasmine clock

17a99e9

refactor: type ITableRow is string array

26e663f

refactor: check if array contains any value by traversing each elemen…

075cd81

…t instead of using string concatenation

chore: fix tests

8b1c526

fix: set isVisible as true for encrypt mode if responses

c0baafc

fix: response interface

870bba4

refactor: add return false if isVisible upfront

183fa4b

refactor: string[] instead of Array<string>

fc19650

tshuli added 4 commits December 21, 2020 15:43

fix: tests

0e50ba0

chore: pass a FieldResponse instead of ProcessedResponse to getProces…

0d4817d

…sedResponse

chore: fix tests

75e0533

refactor: remove unnecessary !response.isVisible

4d8f079

tshuli force-pushed the fix/form-logic-validation branch from 09a7a6c to 4d8f079 Compare December 21, 2020 07:44

tshuli merged commit ae8f5df into develop Dec 21, 2020

tshuli mentioned this pull request Dec 22, 2020

build: Release 4.51.0 #920

Merged

liangyuanruo deleted the fix/form-logic-validation branch January 26, 2021 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: backend validation does not prevent responses on hidden fields #809

fix: backend validation does not prevent responses on hidden fields #809

tshuli commented Dec 7, 2020 •

edited

Loading

tshuli commented Dec 8, 2020

tshuli commented Dec 9, 2020

mantariksh left a comment

mantariksh Dec 9, 2020

tshuli Dec 21, 2020

mantariksh Dec 21, 2020

mantariksh Dec 9, 2020

tshuli Dec 21, 2020

mantariksh Dec 9, 2020

tshuli Dec 21, 2020

mantariksh Dec 9, 2020

tshuli Dec 21, 2020

mantariksh left a comment

mantariksh Dec 21, 2020

fix: backend validation does not prevent responses on hidden fields #809

fix: backend validation does not prevent responses on hidden fields #809

Conversation

tshuli commented Dec 7, 2020 • edited Loading

Problem

Solution

Tests

tshuli commented Dec 8, 2020

tshuli commented Dec 9, 2020

mantariksh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantariksh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshuli commented Dec 7, 2020 •

edited

Loading