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

ref: migrate encrypt spcp verified content flow to own verified-content module #934

Merged
merged 22 commits into from
Feb 10, 2021

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Dec 22, 2020

Problem

This PR adds validation for the contents of storage mode submissions' verifiedContent key with a type guard at runtime before encrypting for SPCP forms.

Instead of using a generalized method of creating the verified content object for encrypted submissions, inline creation so it is more obvious to the developer how the shape is created.

Whilst doing so, migrate the verified content flow to its own modules/verified-content directory.

Closes https://github.com/datagovsg/formsg-private/issues/103

Solution

Features:

  • add VerifiedContentService#getVerifiedContent function
  • add VerifiedContentService#encryptVerifiedContent function
  • add VerifiedContentFactory to gate access to service functions depending on whether feature is enabled
  • add VerifiedContentMiddlewares#encryptVerifiedSpcpFields
  • inline creation of verified content shapes

Improvements:

  • add better comments on generating verified content key-values
  • move unshared fns to verified-content module

@karrui karrui marked this pull request as draft December 22, 2020 11:31
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

I can tell that the implementation is covering for the general case, but it would be much easier on other developers if the flexibility of the implementation were more constrained. Could we merely pick out the properties necessary to convert between the data types, so that another developer can immediately tell which fields were renamed, or do we really need the power and expressivity of functions such as renameKeys?

I'd recommend checking out the Rule of Least Power. Some programming resources see it as an extension of the KISS principle.

instead of using a generalized method of creating the verified content object for encrypted submissions, inline creation so it is more obvious to the developer how the shape is created.
@karrui
Copy link
Contributor Author

karrui commented Dec 28, 2020

Ready for re-review

@liangyuanruo
Copy link
Contributor

Closing until merge conflicts can be resolved

@liangyuanruo liangyuanruo reopened this Feb 8, 2021
# Conflicts:
#	src/app/modules/form/form.utils.ts
#	src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.routes.spec.ts
#	src/app/routes/admin-forms.server.routes.js
#	src/app/routes/public-forms.server.routes.js
@karrui karrui changed the title wip: migrate encrypt spcp verified content flow to own verified-content module feat: migrate encrypt spcp verified content flow to own verified-content module Feb 9, 2021
@karrui karrui changed the title feat: migrate encrypt spcp verified content flow to own verified-content module ref: migrate encrypt spcp verified content flow to own verified-content module Feb 9, 2021
@karrui karrui marked this pull request as ready for review February 9, 2021 03:12
@karrui
Copy link
Contributor Author

karrui commented Feb 9, 2021

@liangyuanruo added tests for VerifiedContentService, so at least we have some modicum of safety. Feel free to review.

@karrui
Copy link
Contributor Author

karrui commented Feb 9, 2021

Erm, just tested downloading of csv on the frontend and the verified content is not being shown. Will investigate, do not merge yet!!!

EDIT: Seems like false alarm for now, I just needed to refresh my docker. Error was

:5000/public/a1f7f0fbf9ab34f45898.worker.js:1991 Uncaught Error: Cannot find module 'p-queue'

Will confirm

EDIT EDIT: Confirmed, just docker image not being updated, csv is correctly being generated after refreshing image.
Screenshot 2021-02-09 at 11 49 36 AM

@karrui
Copy link
Contributor Author

karrui commented Feb 9, 2021

Confirmed, just docker image not being updated, csv is correctly being generated after refreshing image.
Screenshot 2021-02-09 at 11 49 36 AM

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

lgtm

@liangyuanruo liangyuanruo merged commit 9a57b21 into develop Feb 10, 2021
@liangyuanruo liangyuanruo deleted the feat/verified-field-typeguard branch February 10, 2021 04:44
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.

None yet

2 participants