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

Experiment go <> sgid integration #2247

Merged
merged 17 commits into from
Aug 3, 2023
Merged

Conversation

PikkaPikkachu
Copy link
Contributor

@PikkaPikkachu PikkaPikkachu commented Jul 27, 2023

Problem

We are trying to integrate SGID with GO and will be testing it with OGP officers. If the integration is successful, we will open this up to other public officers as well.

Solution

Integration doc with the detailed solution here, for summary refer below:

Followed developer docs here

  1. Add a new login page for SGID login /ogp-login
  2. Upon user click we redirect to SGID authentication url (opens up in the same window)
  3. Once user allows sign in using GO SGID client, we fetch their relevant data i.e. officer's work email.
  4. There's a validation check on valid officer's email ending in .gov.sg
  5. If the validation succeeds we create or find the user in the DB and set the user session
  6. Else we redirect the back to sgid login page and show the relevant error message.

Features:
Login feature with SGID

Before & After Screenshots

AFTER:

Screen.Recording.2023-07-28.at.12.07.46.AM.mov

Tested on health and edu domains, we get a 400 response, with this error message on the UI:

Screenshot 2023-08-03 at 5 59 32 PM

Tests

What tests should be run to confirm functionality?

  1. Check login using SGID manually
  2. Check remaining functionality remains intact [regression testing]
  3. Check invalid authentication or invalid email does not succeed login
  4. Check that edu and health domains are still intact, without the SGID client being initialised

Deploy Notes

Need to create the relevant environment variables before deploying.
There might be a need to use the AWS key manager, in case env variable store in EB does not work.

New environment variables:

  • SGID Client
  • SGID Private key
  • SGID API hostname
  • SGID Client Secret

Copy link

@raynerljm raynerljm left a comment

Choose a reason for hiding this comment

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

the sgID implementation looks like it should work! left some clarifications and some questions - thanks Prakriti! 🎉

src/client/sgidLogin/index.tsx Show resolved Hide resolved
src/client/sgidLogin/SgidLoginForm.tsx Outdated Show resolved Hide resolved
src/client/sgidLogin/actions/index.ts Outdated Show resolved Hide resolved
src/server/services/sgid.ts Show resolved Hide resolved
src/server/services/sgid.ts Outdated Show resolved Hide resolved
src/server/services/sgid.ts Outdated Show resolved Hide resolved
src/server/services/sgid.ts Outdated Show resolved Hide resolved
src/server/services/sgid.ts Outdated Show resolved Hide resolved
Copy link

@PrawiraGenestonlia PrawiraGenestonlia left a comment

Choose a reason for hiding this comment

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

Left minor nit comments. Overall, good job @PikkaPikkachu with the implementation! Awesome work!

.platform/hooks/postdeploy/01_fetch_ssm_parameters.sh Outdated Show resolved Hide resolved
src/client/sgidLogin/actions/index.ts Outdated Show resolved Hide resolved
src/client/sgidLogin/actions/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

lgtm, thanks so much for this 🥳
the overall flow looks great, left a bunch of minor comments below but they're very much non-blocking so feel free to merge!

src/server/services/sgid.ts Outdated Show resolved Hide resolved
src/client/sgidLogin/index.tsx Outdated Show resolved Hide resolved
src/client/sgidLogin/index.tsx Show resolved Hide resolved
src/client/sgidLogin/actions/index.ts Outdated Show resolved Hide resolved
src/client/sgidLogin/index.tsx Outdated Show resolved Hide resolved
src/server/modules/auth/SgidLoginController.ts Outdated Show resolved Hide resolved
src/server/services/sgid.ts Outdated Show resolved Hide resolved
src/server/modules/auth/SgidLoginController.ts Outdated Show resolved Hide resolved
src/server/modules/auth/SgidLoginController.ts Outdated Show resolved Hide resolved
.ebextensions/25-load-sgid-env.config Outdated Show resolved Hide resolved
@gweiying
Copy link
Contributor

gweiying commented Aug 2, 2023

One additional comment from me, let's check the behavior on staging.for.edu.sg and staging.for.sg for the sgid login request endpoint to verify that it's not throwing any weird errors!

Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

lgtm, apart from the first comment below!

src/server/modules/auth/SgidLoginController.ts Outdated Show resolved Hide resolved
@PikkaPikkachu PikkaPikkachu merged commit 90479be into develop Aug 3, 2023
32 of 33 checks passed
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

5 participants