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(api): collapse spcp/myinfo redirect endpoint to new endpoint #1672

Merged
merged 35 commits into from
Apr 26, 2021

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Apr 19, 2021

Part 1 of #1517

Problem

Previously, our /auth/redirect endpoints were split between the MyInfo and spcp sections, which made code harder to maintain and our frontend had separated logic.

Separately, this is also part of the /api/v3 migration to shift endpoints to their new homes

Solution

This PR merges the 2 endpoints into 1 and shifts it into a new route rooted at /api/v3

Features:

  1. Created a new controller method under auth.controller to handle the redirects for backend
  2. Created a new router to service the new /api/v3 routes
  3. New AuthService on frontend side as part of react migration

Notes

  1. there is no decision making on the frontend now. instead, decisions are deferred to the backend and the frontend just transparently sends across its data
  2. MyInfoService will be fully deleted in part 2 of this PR. it is left as is first.

Tests

Tests are here to avoid review fatigue

Manual Tests

  1. Create a SP form. Click on activate and go to the form. Click on keep me logged in. Clicking on sign in with singpass should redirect you to the Singpass page. Ensure that after signing in, refreshing the page still keeps you signed in.
  2. Create a SP form. Click on activate and go to the form. Clicking on sign in with singpass should redirect you to the Singpass page.
  3. Create a email mode form with authentication set to myInfo. Click on activate and go to the form. Clicking on sign in with singpass should redirect to the singpass page.

@seaerchin seaerchin marked this pull request as draft April 19, 2021 09:09
@seaerchin seaerchin changed the base branch from develop to refactor/shard-public-forms-router April 19, 2021 09:17
@seaerchin seaerchin changed the base branch from refactor/shard-public-forms-router to develop April 19, 2021 10:01
@seaerchin seaerchin changed the base branch from develop to refactor/shard-public-forms-router April 19, 2021 10:02
@seaerchin seaerchin force-pushed the refactor/collapse-spcp-endpoint branch 2 times, most recently from 831430c to 8941290 Compare April 20, 2021 07:23
Base automatically changed from refactor/shard-public-forms-router to develop April 20, 2021 10:30
collapsed middleware functions and all auth types are now inferred from teh db retrieval of the form
… a directive

apparently angularJS still has the directive registered even if it's unused and this means that it
bugs...
@seaerchin seaerchin force-pushed the refactor/collapse-spcp-endpoint branch from 8edd221 to 5a1b17b Compare April 21, 2021 02:43
@seaerchin seaerchin marked this pull request as ready for review April 21, 2021 02:51
@seaerchin seaerchin requested review from karrui and mantariksh April 21, 2021 02:51
@karrui karrui changed the title refactor(wip): collapse redirect endpoint refactor(api): collapse redirect endpoint Apr 21, 2021
@seaerchin seaerchin requested a review from karrui April 21, 2021 06:41
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

a couple more changes; pls test on staging/local

src/app/modules/spcp/spcp.errors.ts Outdated Show resolved Hide resolved
src/app/modules/spcp/spcp.errors.ts Outdated Show resolved Hide resolved
src/app/modules/spcp/spcp.types.ts Outdated Show resolved Hide resolved
src/app/modules/spcp/spcp.util.ts Outdated Show resolved Hide resolved
removed old myinfo specific type and used general types instead; added a typeguard rather than
casting to preserve information in teh typesystem that would otherwise be lost
…ormAuthRedirectError

adds utility method so business logic for generating endpoint isn't seen by controller. added
separate handler for mapping application errors back to http errors for redirection because it is
sufficiently different from teh normal flow
@seaerchin seaerchin requested review from mantariksh and karrui April 22, 2021 09:48
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm with comments

src/app/modules/myinfo/myinfo.util.ts Show resolved Hide resolved
src/app/modules/spcp/spcp.util.ts Show resolved Hide resolved
src/types/api/auth.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

possible bug with persistent logins. have you tested that persistent logins are working?

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

approving because I don't think the isPersistentLogin thing is a bug, but do test that extended logins work before merging

@seaerchin
Copy link
Contributor Author

seaerchin commented Apr 26, 2021

@mantariksh could you clarify abit on the extended login? it's that for sp forms, clicking on remember me should cause users to be persistently logged in even after refreshing/leaving and returning back to the page right? to be safe, i'll push this to staging and test it there!

@mantariksh
Copy link
Contributor

@seaerchin extended login means the Singpass cookie should expire after 30 days instead of the usual 1 hour

@seaerchin
Copy link
Contributor Author

@mantariksh gotcha, will do!

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