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(formsg): clone repo on webhook trigger from forms #947

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Sep 26, 2023

Problem

See here

Solution

  1. add an endpoint at /formsg/clone-repo
  2. add handler to clone at that endpoint based on the form's reply. The form has validation build in (email domain must be .open.gov.sg and must verify email + repo is a required field)

New env vars

  • SITE_CLONE_FORM_KEY: used to decrypt the form sg submissions (already added to 1PW on both staging and prod env vars; not yet added to staging EB yet)

Notes

this was tested locally through updating the EFS_VOL_PATH + originUrl (use https instead of ssh as i was facing auth issues) and found working
Screenshot 2023-09-26 at 7 01 04 PM
Screenshot 2023-09-26 at 7 01 37 PM

@seaerchin seaerchin requested a review from harishv7 September 26, 2023 11:01
src/routes/formsgSiteCreation.ts Outdated Show resolved Hide resolved
src/routes/formsgSiteCreation.ts Outdated Show resolved Hide resolved
const { responses } = res.locals.submission
// NOTE: This is validated by formsg to be of domain `@open.gov.sg`;
// hence, not revalidating here
const requesterEmail = getField(responses, "Email") as string
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just allow whitelisted emails?

our admin email perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, you want to stricten from allowing @open.gov.sg to only [email protected]? I don't see too much of an issue here but what's the extra benefit ah cos this only prevents against internal attacks

Copy link
Contributor

Choose a reason for hiding this comment

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

just suggesting least privilege principle. Not too much issue, since this is all internal, will let it up to you to decide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^not doing, the experience of having prod ops enter their own email then receive a rejection email saying it must be [email protected] seems less than ideal given that this is all internal users anyway (validation of @open.gov.sg domain)

@seaerchin seaerchin force-pushed the feat/is-558-autoclone branch from ea58fda to abe7b67 Compare September 26, 2023 17:41
@seaerchin seaerchin requested review from harishv7 and a team September 26, 2023 17:42
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

This PR has quite a number of infra moving parts -
eg.

  1. formsg
  2. efs
  3. be

Could we do a quick sanity check testing on stg before release?
since this is internal use using post deploy testing is also fine, thoughts?

src/routes/formsgSiteCreation.ts Show resolved Hide resolved
src/routes/formsgSiteCreation.ts Show resolved Hide resolved
src/config/config.ts Show resolved Hide resolved
@seaerchin
Copy link
Contributor Author

This PR has quite a number of infra moving parts - eg.

  1. formsg
  2. efs
  3. be

Could we do a quick sanity check testing on stg before release? since this is internal use using post deploy testing is also fine, thoughts?

tbh, i think the only difference between testing on staging vs not, is that the clone step takes place in a different environment (with all of the annoyances that entails); i think i'll just do post-deploy cos it's not open to public anyway.

@seaerchin seaerchin requested review from harishv7, kishore03109 and a team October 3, 2023 06:17
@seaerchin seaerchin merged commit 5b9b888 into develop Oct 5, 2023
8 checks passed
@seaerchin seaerchin deleted the feat/is-558-autoclone branch October 5, 2023 03:36
@alexanderleegs alexanderleegs mentioned this pull request Oct 5, 2023
4 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.

3 participants