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

Call res.redirect after user's handler code for login callback #3

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Sep 13, 2021

Internal ref: OKTA-306438
Resolves okta/okta-oidc-js#340
Overseedes okta/okta-oidc-js#805

This PR improves case when user defines routes.loginCallback.handler
Description of this function in readme:

A function that is called after a successful authentication callback, but before the final redirect within your application. Useful for requirements such as conditional post-authentication redirects, or sending data to logging systems.

const oidc = new ExpressOIDC({
  // ...
  routes: {
    loginCallback: {
      handler: (req, res, next) => {
        // Perform custom logic before final redirect, then call next()
      },
    },

Current behavior

The developer needs to manually call res.redirect() in handler.
This requirement is not explicitly covered in readme and can lead to confusion (see okta/okta-oidc-js#340)
However, it can be useful to give developer the power to manually set redirect path after authentication.
For example, developer can use req.userContext.userinfo.locale

New behavior

If developer did not call res.redirect() in handler, it will be called automatically with correct value (routes.loginCallback.afterCallback) in next().
Otherwise, if developer have manually called res.redirect() with the value he needs, next() will do nothing.

@denysoblohin-okta denysoblohin-okta force-pushed the od-OKTA-306438-login-callback-handler-improvement branch from 039e92d to 1b98d17 Compare October 8, 2021 09:18
@denysoblohin-okta denysoblohin-okta force-pushed the od-OKTA-306438-login-callback-handler-improvement branch from 1b98d17 to 13c839f Compare November 17, 2021 01:00
const afterCustomNextHandler = (err) => {
if (err) {
next(err);
} else if (!res._headerSent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the res._headerSent? looks like a private field, should the middleware depend on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: express v4 seems to have a public accessor for headersSent

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, we probably should be able to keep it.
https://github.com/okta/okta-oidc-middleware/blob/master/package.json#L44

denysoblohin-okta and others added 5 commits November 19, 2021 03:35
Call `res.redirect` after user's `handler` code for login callback
@denysoblohin-okta denysoblohin-okta force-pushed the od-OKTA-306438-login-callback-handler-improvement branch from 559cd0d to b7e00ac Compare November 19, 2021 01:42
eng-prod-CI-bot-okta pushed a commit that referenced this pull request Nov 19, 2021
OKTA-306438
<<<Jenkins Check-In of Tested SHA: b7e00ac for [email protected]>>>
Artifact: okta-oidc-middleware
Files changed count: 3
PR Link: "#3"
oleksandrpravosudko-okta pushed a commit that referenced this pull request Nov 19, 2021
OKTA-306438
<<<Jenkins Check-In of Tested SHA: b7e00ac for [email protected]>>>
Artifact: okta-oidc-middleware
Files changed count: 3
PR Link: "#3"
oleksandrpravosudko-okta pushed a commit that referenced this pull request Nov 19, 2021
OKTA-306438
<<<Jenkins Check-In of Tested SHA: b7e00ac for [email protected]>>>
Artifact: okta-oidc-middleware
Files changed count: 3
PR Link: "#3"
@denysoblohin-okta denysoblohin-okta mentioned this pull request Jan 21, 2022
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.

Problem defining custom callback route
5 participants