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: MyInfo over sgID backend #6337

Merged
merged 23 commits into from
Jun 9, 2023
Merged

Conversation

kenjin-work
Copy link
Contributor

@kenjin-work kenjin-work commented May 16, 2023

Problem

Retrieving MyInfo data over sgID is currently not supported.

Closes #6323

Solution

  • Extend sgID client to convert MyInfo fields to sgID OAuth scopes.
  • Reuse pre-existing MyInfo infrastructure to process and store sgID's MyInfo data.
  • For now, we preserve the current behavior for sgID authentication mode (just fill up NRIC, without filling other fields.)
  • This PR is mostly a refactor to split sgID into two parts: Authentication, and MyInfo.
  • I will add tests in a follow up PR for the sgID MyInfo parts.

Breaking Changes

  • No - this PR is backwards compatible

Features:

  • The full publicly available OAuth scopes supported by sgID are now supported. Enabling support of this in the frontend is only changing 10 lines of code in the backend to expose this feature.

Before & After Screenshots

(We want no difference, since it's disabled on the frontend).

BEFORE:
image

AFTER:
image

Tests

I've added tests to the sgID adapter to make sure it works. It should automatically be run as part of the existing test suite.

@kenjin-work kenjin-work marked this pull request as draft May 16, 2023 17:13
@kenjin-work kenjin-work reopened this May 17, 2023
@kenjin-work kenjin-work marked this pull request as ready for review May 17, 2023 07:01
Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

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

nice work, some clarifying questions!

@kenjin-work
Copy link
Contributor Author

Thanks for the extremely thorough and insightful review Shu Li. I tried to address it. Now, submitting SGID-MyInfo forms don't log out an already logged in SGID-Auth form :).

Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

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

Nice work and thanks for the quick turnaround!
Went through again in more detail and have some further comments for consideration :)

src/app/modules/sgid/sgid.controller.ts Outdated Show resolved Hide resolved
src/app/modules/sgid/sgid.controller.ts Outdated Show resolved Hide resolved
src/app/modules/sgid/sgid.service.ts Outdated Show resolved Hide resolved
src/app/modules/sgid/sgid.controller.ts Outdated Show resolved Hide resolved
src/app/modules/sgid/sgid.service.ts Outdated Show resolved Hide resolved
src/app/modules/form/public-form/public-form.controller.ts Outdated Show resolved Hide resolved
src/app/modules/sgid/sgid.service.ts Show resolved Hide resolved
src/app/modules/form/public-form/public-form.controller.ts Outdated Show resolved Hide resolved
@tshuli
Copy link
Contributor

tshuli commented May 25, 2023

Couple of minor comments, but lgtm otherwise!

@tshuli tshuli requested a review from timotheeg May 25, 2023 03:28
@tshuli
Copy link
Contributor

tshuli commented May 25, 2023

@timotheeg would be good if you could have another look through before this is merged in

Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

Looking good KenJin! 🥳

I only have some super very comments, so I think it's pretty much ready to go! 💪

And I will try to do a second pass tomorrow morning to make sure I understand correctly all the typing magic with the generics (I'm not very good at reading generics 😅)

src/app/modules/sgid/__tests__/sgid.service.spec.ts Outdated Show resolved Hide resolved
src/app/modules/myinfo/myinfo.adapter.ts Outdated Show resolved Hide resolved
src/app/modules/sgid/sgid.adapter.ts Outdated Show resolved Hide resolved
meta,
error: jwtResult.error,
})
res.cookie('isLoginError', true)
Copy link
Contributor

Choose a reason for hiding this comment

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

/notYourPR-DontChangeAnything-JustNoting: putting an error flag in a client cookie feels a bit dirty 😅 ... Ideally, we should have sessions for respondent, and the error details should be set and retrieved from that. The client should not be able to drive error handling behaviour from cookies

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 unfortunately that's how the existing code does this :(.

@kenjin-work
Copy link
Contributor Author

Thanks for the thorough review Tim!

Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

LGTM

@kenjin-work kenjin-work merged commit a3ac48f into develop Jun 9, 2023
@kenjin-work kenjin-work deleted the feat/sgid-myinfo-backend-beta branch June 9, 2023 06:12
@LinHuiqing LinHuiqing mentioned this pull request Jun 12, 2023
51 tasks
@linear linear bot mentioned this pull request Oct 6, 2023
7 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.

Add backend code to support more sgID scopes
3 participants