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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 3, 2022
Merged

feat: allow overriding showLoginPage config via request header #336

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffa7b8d
feat: allow overriding `showLoginPage` config via request header
whipermr5 Jan 28, 2022
45508b7
refactor: impl showLoginPage() as one-liner
LoneRifle Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ $ export MOCKPASS_PORT=5156
$ export MOCKPASS_NRIC=S8979373D
$ export MOCKPASS_UEN=123456789A
$ export SHOW_LOGIN_PAGE=true # Optional, defaults to `false`
$ export SHOW_LOGIN_PAGE=true # Optional, defaults to `false`; can be overridden per request using `X-Show-Login-Page` HTTP header
# Disable signing/encryption (Optional, by default `true`)
$ export SIGN_ASSERTION=false
Expand Down
7 changes: 6 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@ const options = {
assertEndpoint: process.env.CORPPASS_ASSERT_ENDPOINT,
},
},
showLoginPage: process.env.SHOW_LOGIN_PAGE === 'true',
showLoginPage: (req) => {
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 •
edited
Loading

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 •
edited
Loading

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',

},
encryptMyInfo: process.env.ENCRYPT_MYINFO === 'true',
cryptoConfig,
}
Expand Down
2 changes: 1 addition & 1 deletion lib/express/oidc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function config(app, { showLoginPage, idpConfig, serviceProvider }) {
app.get(`/${idp.toLowerCase()}/authorize`, (req, res) => {
const redirectURI = req.query.redirect_uri
const state = encodeURIComponent(req.query.state)
if (showLoginPage) {
if (showLoginPage(req)) {
const oidc = assertions.oidc[idp]
const values = oidc.map((rawId, index) => {
const code = encodeURIComponent(
Expand Down
2 changes: 1 addition & 1 deletion lib/express/saml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function config(
: idpConfig[idp].assertEndpoint || req.query.PartnerId
const relayState = req.query.Target
const partnerId = idpConfig[idp].id
if (showLoginPage) {
if (showLoginPage(req)) {
const saml = assertions.saml[idp]
const values = saml.map((rawId, index) => {
const samlArt = encodeURIComponent(samlArtifact(partnerId, index))
Expand Down
2 changes: 1 addition & 1 deletion lib/express/sgid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function config(app, { showLoginPage, idpConfig, serviceProvider }) {
app.get(`${PATH_PREFIX}/authorize`, (req, res) => {
const redirectURI = req.query.redirect_uri
const state = encodeURIComponent(req.query.state)
if (showLoginPage) {
if (showLoginPage(req)) {
const oidc = assertions.oidc.singPass
const values = oidc
.filter((rawId) => assertions.myinfo.v3.personas[rawId])
Expand Down