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: allow overriding showLoginPage config via request header #336

Conversation

whipermr5
Copy link
Contributor

Problem

  • Mockpass is very suitable as a mock Singpass server for writing E2E tests against, except for the HTML login page
  • We can set the SHOW_LOGIN_PAGE environment variable to false to overcome this, but then it becomes less suitable for manual testing
  • We then need to maintain two separate configurations / instances of Mockpass, one for automated E2E tests, and another for manual tests

Solution

  • Allow the showLoginPage config to be overridden per request via an X-Show-Login-Page HTTP header
  • This way, E2E tests can send X-Show-Login-Page: false in the request, while manual testing can still be done in the browser, all against the same running instance of Mockpass

@whipermr5 whipermr5 force-pushed the allow-overriding-showLoginPage-config-via-request-header branch from 2ed2aef to ffa7b8d Compare January 28, 2022 08:43
Copy link
Collaborator

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

lgtm otherwise!

index.js Outdated
Comment on lines 65 to 68
if (req.header('X-Show-Login-Page')) {
return req.header('X-Show-Login-Page') === 'true'
}
return process.env.SHOW_LOGIN_PAGE === 'true'
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about something more concise like..

Suggested change
if (req.header('X-Show-Login-Page')) {
return req.header('X-Show-Login-Page') === 'true'
}
return process.env.SHOW_LOGIN_PAGE === 'true'
return process.env.SHOW_LOGIN_PAGE === 'true' || req.header('X-Show-Login-Page') === 'true'

Copy link
Contributor Author

@whipermr5 whipermr5 Feb 3, 2022

Choose a reason for hiding this comment

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

It was actually to support a possible use case where the SHOW_LOGIN_PAGE env is true but the user wants to override it to false in a request via a header X-Show-Login-Page: false.

This was my main use case:

This way, E2E tests can send X-Show-Login-Page: false in the request, while manual testing can still be done in the browser, all against the same running instance of Mockpass

Copy link
Contributor Author

@whipermr5 whipermr5 Feb 3, 2022

Choose a reason for hiding this comment

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

I considered this form, but thought it might be a bit hard to understand:

showLoginPage: (req) =>
    req.header('X-Show-Login-Page')
      ? req.header('X-Show-Login-Page') === 'true'
      : process.env.SHOW_LOGIN_PAGE === 'true',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh we could also do something like this:

showLoginPage: (req) =>
    (req.header('X-Show-Login-Page') || process.env.SHOW_LOGIN_PAGE) === 'true',

@LoneRifle LoneRifle merged commit 1a327b6 into opengovsg:master Feb 3, 2022
@whipermr5 whipermr5 deleted the allow-overriding-showLoginPage-config-via-request-header branch February 3, 2022 02:58
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