From d44fafde97b2e69cd53e7fb566eb9b082a3f5df2 Mon Sep 17 00:00:00 2001 From: Yuanruo Liang Date: Mon, 25 Jan 2021 21:41:22 +0800 Subject: [PATCH 1/7] refactor(controllerState): set variable on scope; behavior maybe undefined otherwise --- .../base/directives/submit-form.directive.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/public/modules/forms/base/directives/submit-form.directive.js b/src/public/modules/forms/base/directives/submit-form.directive.js index 30eb428241..96323f3339 100644 --- a/src/public/modules/forms/base/directives/submit-form.directive.js +++ b/src/public/modules/forms/base/directives/submit-form.directive.js @@ -83,9 +83,11 @@ function submitFormDirective( progressModal: null, submitPrevented: false, } + // Also used to store a backup of the form state during submission, the state // of the progress modal - const controllerState = {} + scope.controllerState = {} + scope.hasMyInfoFields = FormFields.containsMyInfoFields(scope.form) scope.formLogin = function (authType, rememberMe) { // Fire GA tracking event @@ -211,7 +213,7 @@ function submitFormDirective( } break case FORM_STATES.SUBMITTING: - controllerState.savedForm = cloneDeep(scope.form) + scope.controllerState.savedForm = cloneDeep(scope.form) scope.form.lockFields() scope.uiState.submitButtonClicked = true if (scope.form.hasAttachments()) { @@ -231,7 +233,7 @@ function submitFormDirective( } break case FORM_STATES.SUBMISSION_ERROR: - scope.form = controllerState.savedForm + scope.form = scope.controllerState.savedForm setFormState(FORM_STATES.DEFAULT) break case FORM_STATES.SUBMIT_PREVENTED: @@ -251,7 +253,7 @@ function submitFormDirective( * Opens the submission progress modal. */ const openProgressModal = function () { - controllerState.progressModal = $uibModal.open({ + scope.controllerState.progressModal = $uibModal.open({ animation: true, backdrop: 'static', keyboard: false, @@ -265,9 +267,9 @@ function submitFormDirective( * Closes the submission progress modal if it is currently open. */ const closeProgressModal = () => { - if (controllerState.progressModal) { - controllerState.progressModal.close() - controllerState.progressModal = null + if (scope.controllerState.progressModal) { + scope.controllerState.progressModal.close() + scope.controllerState.progressModal = null } } From 2bab63b44293ae37ca72a4cdc9dc0b24460d6dcc Mon Sep 17 00:00:00 2001 From: Yuanruo Liang Date: Mon, 25 Jan 2021 22:10:44 +0800 Subject: [PATCH 2/7] refactor(submitFormMain): remove cloneDeep of form object to avoid possibility of disparity --- .../forms/base/directives/submit-form.directive.js | 8 ++++---- src/public/modules/forms/viewmodels/Form.class.js | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/public/modules/forms/base/directives/submit-form.directive.js b/src/public/modules/forms/base/directives/submit-form.directive.js index 96323f3339..8049f8bc77 100644 --- a/src/public/modules/forms/base/directives/submit-form.directive.js +++ b/src/public/modules/forms/base/directives/submit-form.directive.js @@ -278,9 +278,8 @@ function submitFormDirective( * and handle any errors. */ scope.submitForm = () => { - let form = cloneDeep(scope.form) try { - submitFormMain(form) + submitFormMain(scope.form) } catch (error) { handleSubmitFailure(error, 'Please try again later.') } @@ -294,8 +293,9 @@ function submitFormDirective( // Disable UI and optionally open progress modal while processing setFormState(FORM_STATES.SUBMITTING) + let attachments try { - form.attachments = await form.getAttachments() + attachments = await form.getAttachments() } catch (err) { return handleSubmitFailure( err, @@ -307,7 +307,7 @@ function submitFormDirective( // submissionContent is the POST body to backend when we submit the form let submissionContent = { - attachments: form.attachments, + attachments, captchaResponse: captchaService.response, isPreview: form.isPreview, responses, diff --git a/src/public/modules/forms/viewmodels/Form.class.js b/src/public/modules/forms/viewmodels/Form.class.js index af632387d6..834dbe1fa1 100644 --- a/src/public/modules/forms/viewmodels/Form.class.js +++ b/src/public/modules/forms/viewmodels/Form.class.js @@ -47,6 +47,8 @@ class Form { /** * Creates a map of field ID to attachment file. + * The values of the map are encrypted for Storage Mode + * forms. * @returns {Object} Map of { id: file } */ getAttachments() { From e77102b7017ef9ce867f646d2aebb30b26c30535 Mon Sep 17 00:00:00 2001 From: Yuanruo Liang Date: Mon, 25 Jan 2021 22:20:12 +0800 Subject: [PATCH 3/7] refactor: inline encryption code for clarity --- .../base/directives/submit-form.directive.js | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/public/modules/forms/base/directives/submit-form.directive.js b/src/public/modules/forms/base/directives/submit-form.directive.js index 8049f8bc77..1ac1bd06e2 100644 --- a/src/public/modules/forms/base/directives/submit-form.directive.js +++ b/src/public/modules/forms/base/directives/submit-form.directive.js @@ -314,7 +314,22 @@ function submitFormDirective( } if (form.responseMode === responseModeEnum.ENCRYPT && form.publicKey) { try { - encryptSubmissionContent(submissionContent, form.publicKey) + submissionContent.encryptedContent = FormSgSdk.crypto.encrypt( + submissionContent.responses, + form.publicKey, + ) + + // Edge case: We still send mobile and email fields in the plaintext for + // end-to-end encryption because of SMS and email autoreplies + submissionContent.responses = submissionContent.responses + .filter((item) => ['mobile', 'email'].includes(item.fieldType)) + .map((item) => { + return _(item) + .pick(['fieldType', '_id', 'answer', 'signature']) + .omitBy(_.isNull) + .value() + }) + // Version the data in case of any backwards incompatibility submissionContent.version = ENCRYPT_VERSION } catch (err) { return handleSubmitFailure( @@ -333,26 +348,6 @@ function submitFormDirective( ).then(handleSubmitSuccess, handleSubmitFailure) } - /** - * Encrypts submission content for end-to-end encryption. - * @param {Object} submissionContent Content to encrypt - * @param {String} publicKey Encryption public key - */ - const encryptSubmissionContent = (submissionContent, publicKey) => { - submissionContent.encryptedContent = FormSgSdk.crypto.encrypt( - submissionContent.responses, - publicKey, - ) - submissionContent.responses = submissionContent.responses - .filter((item) => ['mobile', 'email'].includes(item.fieldType)) - .map((item) => { - return _(item) - .pick(['fieldType', '_id', 'answer', 'signature']) - .omitBy(_.isNull) - .value() - }) - } - /** * Helper function for Google Analytics to check for persistent login. */ From fdb500446358ed61f436c862e5718bbd6545ea7e Mon Sep 17 00:00:00 2001 From: Yuanruo Liang Date: Mon, 25 Jan 2021 22:47:54 +0800 Subject: [PATCH 4/7] refactor: move encryption into Form class --- .../forms/base/directives/submit-form.directive.js | 5 +---- src/public/modules/forms/viewmodels/Form.class.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/public/modules/forms/base/directives/submit-form.directive.js b/src/public/modules/forms/base/directives/submit-form.directive.js index 1ac1bd06e2..5e31b75aa8 100644 --- a/src/public/modules/forms/base/directives/submit-form.directive.js +++ b/src/public/modules/forms/base/directives/submit-form.directive.js @@ -314,10 +314,7 @@ function submitFormDirective( } if (form.responseMode === responseModeEnum.ENCRYPT && form.publicKey) { try { - submissionContent.encryptedContent = FormSgSdk.crypto.encrypt( - submissionContent.responses, - form.publicKey, - ) + submissionContent.encryptedContent = form.getEncryptedContent() // Edge case: We still send mobile and email fields in the plaintext for // end-to-end encryption because of SMS and email autoreplies diff --git a/src/public/modules/forms/viewmodels/Form.class.js b/src/public/modules/forms/viewmodels/Form.class.js index 834dbe1fa1..27ce8de75b 100644 --- a/src/public/modules/forms/viewmodels/Form.class.js +++ b/src/public/modules/forms/viewmodels/Form.class.js @@ -1,3 +1,5 @@ +const formsg = require('@opengovsg/formsg-sdk')() + const FieldFactory = require('../helpers/field-factory') const { getEncryptedAttachmentsMap, @@ -67,6 +69,16 @@ class Form { hasAttachments() { return this.form_fields.some(fieldHasAttachment) } + + /** + * Gets the encrypted responses + */ + getEncryptedContent() { + if (this.responseMode === 'encrypt') { + return formsg.crypto.encrypt(this.getResponses(), this.publicKey) + } + return null + } } module.exports = Form From 8b11438f89a1761089a7c015338429aedb0e7e1e Mon Sep 17 00:00:00 2001 From: Yuanruo Liang Date: Mon, 25 Jan 2021 23:00:47 +0800 Subject: [PATCH 5/7] refactor(response): abstract away edge cases into Form class --- .../base/directives/submit-form.directive.js | 16 ++---------- .../modules/forms/viewmodels/Form.class.js | 26 ++++++++++++++++--- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/public/modules/forms/base/directives/submit-form.directive.js b/src/public/modules/forms/base/directives/submit-form.directive.js index 5e31b75aa8..1d967ac630 100644 --- a/src/public/modules/forms/base/directives/submit-form.directive.js +++ b/src/public/modules/forms/base/directives/submit-form.directive.js @@ -303,29 +303,17 @@ function submitFormDirective( ) } - const responses = form.getResponses() - // submissionContent is the POST body to backend when we submit the form let submissionContent = { attachments, captchaResponse: captchaService.response, isPreview: form.isPreview, - responses, + responses: form.getResponsesForSubmission(), } + if (form.responseMode === responseModeEnum.ENCRYPT && form.publicKey) { try { submissionContent.encryptedContent = form.getEncryptedContent() - - // Edge case: We still send mobile and email fields in the plaintext for - // end-to-end encryption because of SMS and email autoreplies - submissionContent.responses = submissionContent.responses - .filter((item) => ['mobile', 'email'].includes(item.fieldType)) - .map((item) => { - return _(item) - .pick(['fieldType', '_id', 'answer', 'signature']) - .omitBy(_.isNull) - .value() - }) // Version the data in case of any backwards incompatibility submissionContent.version = ENCRYPT_VERSION } catch (err) { diff --git a/src/public/modules/forms/viewmodels/Form.class.js b/src/public/modules/forms/viewmodels/Form.class.js index 27ce8de75b..9ae4f4e57a 100644 --- a/src/public/modules/forms/viewmodels/Form.class.js +++ b/src/public/modules/forms/viewmodels/Form.class.js @@ -1,3 +1,4 @@ +const _ = require('lodash') const formsg = require('@opengovsg/formsg-sdk')() const FieldFactory = require('../helpers/field-factory') @@ -33,10 +34,11 @@ class Form { } /** - * Gets the submitted values of all fields. + * Internal helper function to get the submitted values of all fields + * that are supposed to have answers. * @returns {Array} Array of response objects. */ - getResponses() { + _getResponses() { return this.form_fields .filter((field) => !(field instanceof NoAnswerField)) .map((field) => { @@ -75,10 +77,28 @@ class Form { */ getEncryptedContent() { if (this.responseMode === 'encrypt') { - return formsg.crypto.encrypt(this.getResponses(), this.publicKey) + return formsg.crypto.encrypt(this._getResponses(), this.publicKey) } return null } + + /** + * Method to abstract away edge cases for submission responses in email vs encrypt mode + */ + getResponsesForSubmission() { + if (this.responseMode === 'encrypt') { + // Edge case: We still send mobile and email fields to the server in plaintext + // even with end-to-end encryption in order to support SMS and email autoreplies + return this._getResponses() + .filter((item) => ['mobile', 'email'].includes(item.fieldType)) + .map((item) => { + return _(item) + .pick(['fieldType', '_id', 'answer', 'signature']) + .omitBy(_.isNull) + .value() + }) + } else return this._getResponses() + } } module.exports = Form From 8a45c2cb7871c03d81633b5918eb137f56191868 Mon Sep 17 00:00:00 2001 From: Yuanruo Liang Date: Mon, 25 Jan 2021 23:18:47 +0800 Subject: [PATCH 6/7] refactor(submissionContent): extract the logic of preparing a submission into the Form class --- .../base/directives/submit-form.directive.js | 34 +++++-------------- .../modules/forms/viewmodels/Form.class.js | 22 ++++++++++++ 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/public/modules/forms/base/directives/submit-form.directive.js b/src/public/modules/forms/base/directives/submit-form.directive.js index 1d967ac630..4e9eba4c4f 100644 --- a/src/public/modules/forms/base/directives/submit-form.directive.js +++ b/src/public/modules/forms/base/directives/submit-form.directive.js @@ -6,8 +6,6 @@ const { getLogicUnitPreventingSubmit, } = require('../../../../../shared/util/logic') -// The current encrypt version to assign to the encrypted submission. -const ENCRYPT_VERSION = 1 /** * @typedef {number} FormState */ @@ -293,37 +291,21 @@ function submitFormDirective( // Disable UI and optionally open progress modal while processing setFormState(FORM_STATES.SUBMITTING) - let attachments + // submissionContent is the POST body to backend when we submit the form + let submissionContent + try { - attachments = await form.getAttachments() + submissionContent = Object.assign( + { captchaResponse: captchaService.response }, + await form.getSubmissionContent(), + ) } catch (err) { return handleSubmitFailure( err, - 'Could not encrypt your attachments. Please contact the form administrator.', + 'Could not prepare your submission. Please contact the form administrator.', ) } - // submissionContent is the POST body to backend when we submit the form - let submissionContent = { - attachments, - captchaResponse: captchaService.response, - isPreview: form.isPreview, - responses: form.getResponsesForSubmission(), - } - - if (form.responseMode === responseModeEnum.ENCRYPT && form.publicKey) { - try { - submissionContent.encryptedContent = form.getEncryptedContent() - // Version the data in case of any backwards incompatibility - submissionContent.version = ENCRYPT_VERSION - } catch (err) { - return handleSubmitFailure( - err, - 'Could not encrypt your submission. Please contact the form administrator.', - ) - } - } - Submissions.post( { formId: scope.form._id, diff --git a/src/public/modules/forms/viewmodels/Form.class.js b/src/public/modules/forms/viewmodels/Form.class.js index 9ae4f4e57a..25875315a9 100644 --- a/src/public/modules/forms/viewmodels/Form.class.js +++ b/src/public/modules/forms/viewmodels/Form.class.js @@ -9,6 +9,11 @@ const { } = require('../helpers/attachments-map') const { NoAnswerField } = require('./Fields') +// The current encrypt version to assign to the encrypted submission. +// This is needed if we ever break backwards compatibility with +// end-to-end encryption +const ENCRYPT_VERSION = 1 + /** * Deserialises raw form object returned by backend and * manages form-wide operations. @@ -99,6 +104,23 @@ class Form { }) } else return this._getResponses() } + + /** + * Method to determine what to POST to the backend submission endpoint. + * Does not include captcha verification. + */ + async getSubmissionContent() { + const submissionContent = { + attachments: await this.getAttachments(), + isPreview: this.isPreview, + responses: this.getResponsesForSubmission(), + } + if (this.responseMode === 'encrypt') { + submissionContent.encryptedContent = this.getEncryptedContent() + submissionContent.version = ENCRYPT_VERSION + } + return submissionContent + } } module.exports = Form From b1678e5c94b5b9948741bca2968caa433f95587a Mon Sep 17 00:00:00 2001 From: Yuanruo Liang Date: Mon, 25 Jan 2021 23:23:48 +0800 Subject: [PATCH 7/7] refactor: mark internal helper methods, improve docs in Form class --- .../modules/forms/viewmodels/Form.class.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/public/modules/forms/viewmodels/Form.class.js b/src/public/modules/forms/viewmodels/Form.class.js index 25875315a9..35d77c37d4 100644 --- a/src/public/modules/forms/viewmodels/Form.class.js +++ b/src/public/modules/forms/viewmodels/Form.class.js @@ -55,12 +55,12 @@ class Form { } /** - * Creates a map of field ID to attachment file. - * The values of the map are encrypted for Storage Mode + * Internal helper function that creates a map of field ID to attachment file. + * The values of the map are encrypted for Storage Mode. * forms. * @returns {Object} Map of { id: file } */ - getAttachments() { + _getAttachments() { const attachmentsMap = getAttachmentsMap(this.form_fields) if (this.responseMode === 'encrypt') { return getEncryptedAttachmentsMap(attachmentsMap, this.publicKey) @@ -78,9 +78,10 @@ class Form { } /** - * Gets the encrypted responses + * Gets the encrypted values of the responses. Only applicable to + * Storage Mode forms. */ - getEncryptedContent() { + _getEncryptedContent() { if (this.responseMode === 'encrypt') { return formsg.crypto.encrypt(this._getResponses(), this.publicKey) } @@ -90,7 +91,7 @@ class Form { /** * Method to abstract away edge cases for submission responses in email vs encrypt mode */ - getResponsesForSubmission() { + _getResponsesForSubmission() { if (this.responseMode === 'encrypt') { // Edge case: We still send mobile and email fields to the server in plaintext // even with end-to-end encryption in order to support SMS and email autoreplies @@ -111,12 +112,12 @@ class Form { */ async getSubmissionContent() { const submissionContent = { - attachments: await this.getAttachments(), + attachments: await this._getAttachments(), isPreview: this.isPreview, - responses: this.getResponsesForSubmission(), + responses: this._getResponsesForSubmission(), } if (this.responseMode === 'encrypt') { - submissionContent.encryptedContent = this.getEncryptedContent() + submissionContent.encryptedContent = this._getEncryptedContent() submissionContent.version = ENCRYPT_VERSION } return submissionContent