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.

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

Conversation

justynoh
Copy link
Contributor

@justynoh justynoh commented Mar 18, 2024

Problem

Field locking currently only has frontend validation, but we need to implement backend validation for this as well.

Closes FRM-1640

Solution

This PR implements the MultirespondentSubmissionMiddleware.validateMultirespondentSubmission function as it's main focus. Everything else is auxiliary and enables this function to be defined.

Shared logic functions are used to replicate evaluation of submission logic. A logic adaptor is built for the backend to make use of these shared functions. It uses the client version of the schema since FieldResponseV3 is more similar to frontend FormFieldValues than they are to the older backend FieldResponse.

Additionally, most notably, as a result of needing to validate that non-editable fields are untouched, a notion of "response equality" is defined in shared/utils/response-v3.ts. Additionally, as the submission secret key is required for the backend to decrypt the existing submission in the database, the submission secret key is now sent to the backend from the frontend. This accounts for the frontend modifications seen in this PR.

Note that this PR does not account for field-level validation. This has been marked as a TODO in code, where each visible and editable field itself needs to be validated with it's own validation rules (e.g. required/optional, etc).

Breaking Changes

  • No - this PR is backwards compatible

Tests

Create a multirespondent form with a simple workflow (e.g. two respondents). Make sure there are a mix of shared and unshared fields (in this case "shared" means editable by both respondent 1 and respondent 2). In this example, I created a Q1 radio, Q2 dropdown, Q3 email, Q4 checkbox, Q5 yes/no, Q6 short answer. Respondent 1 is assigned Q1-4 and respondent 2 is assigned Q3-6, so that Q3 and Q4 are shared.

Initial submission

  • Send a valid submission via cURL. This should succeed.
curl 'http://localhost:3000/api/v3/forms/65680f30df564e005e446e2d/submissions/multirespondent?captchaResponse=null' \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Accept-Language: en-US,en;q=0.9' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: multipart/form-data; boundary=----WebKitFormBoundarygrSB7FoxtaZ5Ycg7' \
  -H 'DNT: 1' \
  -H 'Origin: http://localhost:3000' \
  -H 'Referer: http://localhost:3000/65680f30df564e005e446e2d' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.0.0 Safari/537.36' \
  -H 'sec-ch-ua: "Not(A:Brand";v="24", "Chromium";v="122"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "macOS"' \
  --data-raw $'------WebKitFormBoundarygrSB7FoxtaZ5Ycg7\r\nContent-Disposition: form-data; name="body"\r\n\r\n{"responses":{"6571f03219eb850112ae990e":{"fieldType":"radiobutton","answer":{"value":"Radio 1"}},"65d4238b1230effbfd6f74cb":{"fieldType":"dropdown","answer":"Dropdown 1"},"65f9257217e471530f22bcd9":{"fieldType":"email","answer":{"value":"justyn@open.gov.sg"}},"65d423891230effbfd6f74b7":{"fieldType":"checkbox","answer":{"value":["Checkbox 1"]}}},"responseMetadata":{"responseTimeMs":33230,"numVisibleFields":6},"version":3}\r\n------WebKitFormBoundarygrSB7FoxtaZ5Ycg7--\r\n'
{"message":"Form submission successful.","submissionId":"65f926ca17e471530f22bea5","timestamp":1710827210246}
  • Send a submission where a respondent 2-only field (e.g. Q5/6) is filled. This should be rejected by the server.
curl 'http://localhost:3000/api/v3/forms/65680f30df564e005e446e2d/submissions/multirespondent?captchaResponse=null' \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Accept-Language: en-US,en;q=0.9' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: multipart/form-data; boundary=----WebKitFormBoundarygrSB7FoxtaZ5Ycg7' \
  -H 'DNT: 1' \
  -H 'Origin: http://localhost:3000' \
  -H 'Referer: http://localhost:3000/65680f30df564e005e446e2d' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.0.0 Safari/537.36' \
  -H 'sec-ch-ua: "Not(A:Brand";v="24", "Chromium";v="122"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "macOS"' \
  --data-raw $'------WebKitFormBoundarygrSB7FoxtaZ5Ycg7\r\nContent-Disposition: form-data; name="body"\r\n\r\n{"responses":{"6571f03219eb850112ae990e":{"fieldType":"radiobutton","answer":{"value":"Radio 1"}},"65d4238b1230effbfd6f74cb":{"fieldType":"dropdown","answer":"Dropdown 1"},"65f9257217e471530f22bcd9":{"fieldType":"email","answer":{"value":"justyn@open.gov.sg"}},"65d423891230effbfd6f74b7":{"fieldType":"checkbox","answer":{"value":["Checkbox 1"]}},"65f9258417e471530f22bd46":{"fieldType":"yes_no","answer":"Yes"}},"responseMetadata":{"responseTimeMs":33230,"numVisibleFields":6},"version":3}\r\n------WebKitFormBoundarygrSB7FoxtaZ5Ycg7--\r\n'
{"message":"There is something wrong with your form submission. Please check your responses and try again. If the problem persists, please refresh the page."}

Respondent 2 submission
(if you're copying cURL from a browser network tab, don't forget to change the submissionId in the URL, the response itself, and the submission secret key in the data section)

  • Send a valid submission via cURL. This should succeed.
  • Send a submission where a shared field (e.g. Q3/4) is modified. This should succeed.
curl 'http://localhost:3000/api/v3/forms/65680f30df564e005e446e2d/submissions/65f926ca17e471530f22bea5?captchaResponse=null' \
  -X 'PUT' \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Accept-Language: en-US,en;q=0.9' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryMAkuNxQsfBvsmprX' \
  -H 'DNT: 1' \
  -H 'Origin: http://localhost:3000' \
  -H 'Referer: http://localhost:3000/65680f30df564e005e446e2d/edit/65f9265b17e471530f22be96?key=o2QsH57uSqkqdkHelkoJt2UboIwb7alecrA7IOLlsJE%3D' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.0.0 Safari/537.36' \
  -H 'sec-ch-ua: "Not(A:Brand";v="24", "Chromium";v="122"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "macOS"' \
  --data-raw $'------WebKitFormBoundaryMAkuNxQsfBvsmprX\r\nContent-Disposition: form-data; name="body"\r\n\r\n{"responses":{"6571f03219eb850112ae990e":{"fieldType":"radiobutton","answer":{"value":"Radio 1"}},"65d4238b1230effbfd6f74cb":{"fieldType":"dropdown","answer":"Dropdown 1"},"65f9257217e471530f22bcd9":{"fieldType":"email","answer":{"value":"justyn@open.gov.sg"}},"65d423891230effbfd6f74b7":{"fieldType":"checkbox","answer":{"value":["Checkbox 1","Checkbox 2"]}},"65f9258417e471530f22bd46":{"fieldType":"yes_no","answer":"No"},"65d423821230effbfd6f7464":{"fieldType":"textfield","answer":"abcde"}},"responseMetadata":{"responseTimeMs":7236,"numVisibleFields":6},"submissionSecretKey":"Z2T/aqbaHECMe+JX1Wpwlz0weEA9xm/m8PoSUCvR60U=","version":3}\r\n------WebKitFormBoundaryMAkuNxQsfBvsmprX--\r\n'
{"message":"Form submission successful.","submissionId":"65f926ca17e471530f22bea5","timestamp":1710827210246}
  • Send a submission where a respondent 1-only field is modified. This should be rejected by the server.
curl 'http://localhost:3000/api/v3/forms/65680f30df564e005e446e2d/submissions/65f926ca17e471530f22bea5?captchaResponse=null' \
  -X 'PUT' \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Accept-Language: en-US,en;q=0.9' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryMAkuNxQsfBvsmprX' \
  -H 'DNT: 1' \
  -H 'Origin: http://localhost:3000' \
  -H 'Referer: http://localhost:3000/65680f30df564e005e446e2d/edit/65f9265b17e471530f22be96?key=o2QsH57uSqkqdkHelkoJt2UboIwb7alecrA7IOLlsJE%3D' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.0.0 Safari/537.36' \
  -H 'sec-ch-ua: "Not(A:Brand";v="24", "Chromium";v="122"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "macOS"' \
  --data-raw $'------WebKitFormBoundaryMAkuNxQsfBvsmprX\r\nContent-Disposition: form-data; name="body"\r\n\r\n{"responses":{"6571f03219eb850112ae990e":{"fieldType":"radiobutton","answer":{"value":"Radio 2"}},"65d4238b1230effbfd6f74cb":{"fieldType":"dropdown","answer":"Dropdown 1"},"65f9257217e471530f22bcd9":{"fieldType":"email","answer":{"value":"justyn@open.gov.sg"}},"65d423891230effbfd6f74b7":{"fieldType":"checkbox","answer":{"value":["Checkbox 1"]}},"65f9258417e471530f22bd46":{"fieldType":"yes_no","answer":"No"},"65d423821230effbfd6f7464":{"fieldType":"textfield","answer":"abcde"}},"responseMetadata":{"responseTimeMs":7236,"numVisibleFields":6},"submissionSecretKey":"Z2T/aqbaHECMe+JX1Wpwlz0weEA9xm/m8PoSUCvR60U=","version":3}\r\n------WebKitFormBoundaryMAkuNxQsfBvsmprX--\r\n'
{"message":"There is something wrong with your form submission. Please check your responses and try again. If the problem persists, please refresh the page."}

Now, add logic to the form and make some fields optional.

Initial submission

  • With prevent submit logic, send a valid submission via cURL. This should succeed.
  • With prevent submit logic, send a submission via cURL such that submission should have been prevented. This should be rejected by the server.
  • With show fields logic, send a valid submission via cURL. This should succeed.
  • With show fields logic, send a submission via cURL such that a value is provided for a field that should have been hidden. This should be rejected by the server.

Respondent 2 submission
(take note that form fields and logic are saved with a submission lifetime, so you if you change the form logics you will need to make a new initial submission to generate a new edit link)

  • With prevent submit logic, send a valid submission via cURL. This should succeed.
  • With prevent submit logic, send a submission via cURL such that submission should have been prevented. This should be rejected by the server.
  • With show fields logic, send a valid submission via cURL. This should succeed.
  • With show fields logic, send a submission via cURL such that a value is provided for a field that should have been hidden. This should be rejected by the server.

Copy link

linear bot commented Mar 18, 2024

Base automatically changed from refactor/move-logic-to-shared to develop March 19, 2024 03:44
@justynoh justynoh force-pushed the feat/field-locking-backend-validation branch 2 times, most recently from 14890fd to 9f4bdb3 Compare March 19, 2024 06:23
@justynoh justynoh force-pushed the feat/field-locking-backend-validation branch from 9f4bdb3 to 1d7695c Compare March 19, 2024 06:24
@justynoh justynoh requested a review from KenLSM March 19, 2024 06:24
Comment on lines +503 to 509
MultirespondentSubmissionMiddleware.validateUpdateMultirespondentSubmissionParams,
MultirespondentSubmissionMiddleware.createFormsgAndRetrieveForm,
MultirespondentSubmissionMiddleware.scanAndRetrieveAttachments,
// EncryptSubmissionMiddleware.validateStorageSubmission,
MultirespondentSubmissionMiddleware.validateMultirespondentSubmission,
MultirespondentSubmissionMiddleware.setCurrentWorkflowStep,
MultirespondentSubmissionMiddleware.encryptSubmission,
updateMultirespondentSubmission,
Copy link
Contributor

@KenLSM KenLSM Mar 20, 2024

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

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

@justynoh justynoh requested a review from KenLSM March 25, 2024 05:43
Copy link
Contributor

@KenLSM KenLSM left a comment

Choose a reason for hiding this comment

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

LGTM! Merging this in as the attachment PR also needs some parts of this validation!

@KenLSM KenLSM merged commit 58b2474 into develop Mar 25, 2024
22 of 24 checks passed
@KenLSM KenLSM deleted the feat/field-locking-backend-validation branch March 25, 2024 13:32
@KenLSM KenLSM mentioned this pull request Apr 2, 2024
40 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants