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: store prefill in frontend for spcp/myinfo forms, enable prefill for SGID forms #3920

Merged
merged 7 commits into from
Jul 1, 2022

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented May 30, 2022

Problem

  • Currently, when there are prefill query parameters for an SP/CP/Myinfo form, to preserve the prefill query during the authentication flow the prefill query parameters are relayed through our backend and SPCP/Myinfo servers, before they are returned to the client in the form of a redirect after authentication is complete
  • The prefilled query parameters can be arbitrarily long. With the transition to OIDC, this approach is no longer possible as NDI only accepts state of up to 255 characters
  • In addition, prefill is a purely frontend feature for user's convenience, and ideally, prefilled form values should not pass through our servers until they have been reviewed and submitted by the user

Solution

  • The workflow has been revised as follows:
    • When user attempts to login to SP/CP/Myinfo form with prefilled query parameter, the client generates a random queryId and stores the queryId and query parameter in local storage
    • The queryId is then relayed through our servers
    • Thereafter, after authentication, the client is redirected back to the form with the queryId provided in the redirect. Using the queryId, the client can then retrieve the stored query parameters and reconstruct the original url
  • This change is backwards compatible because there is no change to the backend logic

Others

  • Separately, this PR enables prefill for SGID forms (which was somehow left out previously)

Tests

  • Create Singpass form. Open in public view with prefilled query parameters. Login to Singpass. On redirect back to the form, check that the prefill displays correctly. Check that storedQuery has been deleted from sessionStorage
  • Logout. With the prefilled query parameters still in the url, click login again. Do not complete the Singpass authentication flow. Return to the form and enter a new set of prefilled query parameters. Login using this new set of query parameters and complete the authentication flow. Check that on redirect back to the form, the correct prefill is displayed and storedQuery has been deleted from sessionStorage
  • Repeat above for CP, MyInfo and SGID forms
  • Create normal prefill form and check that it works normally

@tshuli tshuli temporarily deployed to staging-al2 May 30, 2022 06:34 Inactive
@tshuli tshuli marked this pull request as ready for review May 30, 2022 07:18
@tshuli tshuli requested a review from mantariksh June 8, 2022 03:06
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.

would it be possible to have more detailed documentation of the considerations behind this feature, perhaps in code comments or in a google doc? some questions which I thought through when reviewing:

  • why do we use 'storedQuery' as the sessionStorage key rather than just using the ObjectId? probably because we only one person can be logging in at a time in a given session, so we should only store one set of prefill params
  • then why have an ID at all, since we could just store the prefill params in sessionStorage and not pass anything to the backend? tbh I'm not 100% clear on this, but I imagine there's some security benefit which I missed (?) so would be good to document it

@tshuli
Copy link
Contributor Author

tshuli commented Jun 13, 2022

would it be possible to have more detailed documentation of the considerations behind this feature, perhaps in code comments or in a google doc? some questions which I thought through when reviewing:

  • why do we use 'storedQuery' as the sessionStorage key rather than just using the ObjectId? probably because we only one person can be logging in at a time in a given session, so we should only store one set of prefill params

Yes, that's correct. will document.

  • then why have an ID at all, since we could just store the prefill params in sessionStorage and not pass anything to the backend? tbh I'm not 100% clear on this, but I imagine there's some security benefit which I missed (?) so would be good to document it

The use of queryId param maintains the form state pre- and post-authentication flow, and ensures that the stored query is not otherwise loaded in other situations. for example, if we just store the prefill params in session storage, suppose the user initiates the login flow but does not complete it, and then opens the public form view in the same session but without any prefills. The prefill would still load from the stored query, which is incorrect behavior

Will document

@mantariksh
Copy link
Contributor

if we just store the prefill params in session storage, suppose the user initiates the login flow but does not complete it, and then opens the public form view in the same session but without any prefills. The prefill would still load from the stored query, which is incorrect behavior

sorry still don't understand this - in this case the 2nd time the user clicks login, we would see that the URL has no query parameters and hence clear the stored query, resulting in correct behaviour right?

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.

lgtm other than previous comments!

@tshuli tshuli merged commit e3b2ec2 into develop Jul 1, 2022
@tshuli tshuli deleted the feat/store-prefill-frontend branch July 1, 2022 05:51
@tshuli tshuli mentioned this pull request Jul 1, 2022
1 task
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.

2 participants