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

Make passport.authenticate() callbacks more useful #423

Closed
wants to merge 1 commit into from

Conversation

kara-ryli
Copy link

The callback argument is challenging to use because passport treats it as an
alternative to its default behavior for Strategy#succes, #fail and #error, but
the callback doesn't have access to the same middleware variables (req, res,
next, multi and failures) that passport does.

This is change moves all passport login logic into a function, and uses that
function as the default value of callback. The only API change is that the
signature of the callback function changes from:

function (err, user, info, status) {}

To

function (err, user, info, status, req, res, next) {}

This changes code that previously needed to do this:

app.get('/protected', function(req, res, next) {
  passport.authenticate('local', function(err, user, info) {
    if (err) { return next(err) }
    if (!user) { return res.redirect('/signin') }
    res.redirect('/account');
  })(req, res, next);
});

To be able to do this:

app.get('/protected',
  passport.authenticate('local',
    function(err, user, info, status, req, res, next) {
      if (err) { return next(err) }
      if (!user) { return res.redirect('/signin') }
      res.redirect('/account');
    }
  )
);

This has three main benefits:

  • It avoids having to recreate the middleware function on every request
  • It makes the syntax more readable to engineers new to JavaScript, Node or
    passport
  • It makes the strategy augmentation logic more understandable to strategy
    developers

I feel this is valuable because the only reason you'd want to use the callback
argument instead of default passport handling is if your application was
delegating session management outside of Express (e.g. to another service), and
performance was a major consideration.

Given the existing workarounds for the issue, the change should also not have
any negative security implications.

The `callback` argument is challenging to use because passport treats it as an
alternative to its default behavior for Strategy#succes, #fail and #error, but
the callback doesn't have access to the same middleware variables (req, res,
next, multi and failures) that passport does.

This is change moves all passport login logic into a function, and uses that
function as the default value of `callback`. The only API change is that the
signature of the `callback` function changes from:

    function (err, user, info, status) {}

To

    function (err, user, info, status, req, res, next) {}

This changes code that previously needed to do this:

```javascript
app.get('/protected', function(req, res, next) {
  passport.authenticate('local', function(err, user, info) {
    if (err) { return next(err) }
    if (!user) { return res.redirect('/signin') }
    res.redirect('/account');
  })(req, res, next);
});
```

To be able to do this:

```javascript
app.get('/protected',
  passport.authenticate('local',
    function(err, user, info, status, req, res, next) {
      if (err) { return next(err) }
      if (!user) { return res.redirect('/signin') }
      res.redirect('/account');
    }
  )
);
```

This has three main benefits:

* It avoids having to recreate the middleware function on every request
* It makes the syntax more readable to engineers new to JavaScript, Node or
  passport
* It makes the strategy augmentation logic more understandable to strategy
  developers

I feel this is valuable because the only reason you'd want to use the callback
argument instead of default passport handling is if your application was
delegating session management outside of Express (e.g. to another service), and
performance was a major consideration.

Given the existing workarounds for the issue, the change should also not have
any negative security implications.
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.

None yet

1 participant