What's the use case for passing a callback to a strategy?

[kara-ryli]

I'm working on a custom Passport strategy, and documenting how to use it. I'm seeing this in the code:

lib/middleware/authenticate.js

module.exports = function authenticate(passport, name, options, callback) {
  // ...
  return function authenticate(req, res, next) {
      // ...
      strategy.success = function(user, info) {
        if (callback) {
          return callback(null, user, info);
        }
      }
     // ...
  })
};

Which, if I read this correctly, means that the request will timeout because the request-ending methods of the res and next arguments are never invoked. This callback is used for success and error, but not failure, pass or redirect results.

Is there a use-case for this I don't understand, or is this a bug?

EDIT: looks like it is called for fail if all strategies fail.

[kara-ryli]

Ok, so it looks like this is intentional, given the example in the comments:

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);
});

Although this is a little bit unintuitive since your code needs both a callback and pass req/res/next to middleware in order to cover all possible cases. Plus it relies on generating a new middleware function for every request, which feels like unnecessary overhead. If you'd entertain a PR to smooth this out a bit, I'd be happy to give it a go.

[jaredhanson]

What would the PR do differently?

A custom callback should be used only in rare instances, so being unintuitive is not a big concern.

[jaredhanson]

Also, use of custom callbacks by an application developer should not impact how a custom strategy is implemented. The custom callback is handled by Passport core, independent of the strategy itself.

[kara-ryli]

My initial thought would be simply to pass a req/res/next as well, so your code could look like:

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');
}));

I think the main use case for this would be logging information about the request based on its success/failure for auditing, regardless of the other options you pass to the strategy. I also think using the API consistently will also help avoid user error; I had to dig around in the source code quite a bit before understanding how it works.

If i was feeling ambitious, it'd want to have the callback argument, if absent, default to a function which does all of the stuff that Passport is doing already, so the logic would look more like:

if (typeof callback !== 'function') {
  callback = function (err, user, info, status, req, res, next) {
    if (err) {
      return allTheStuffPassportDoesForErrors(err, user, info, status, req, res, next);
    }
    if (user) {
      return allTheStuffPassportDoesForSuccess(err, user, info, status, req, res, next);
    }
    return alltheStuffPassportDoesForFailure(err, user, info, status, req, res, next);
  }
}
// ...
strategy.success = function(user, info) {
  callback(null, user, info, 200, req, res, next);
};

Additionally, the assignProperty option and authInfo option appear to be mutually exclusive; I wouldn't mind fixing that.

[jaredhanson]

I like the idea of factoring all the stuff passport does into a separate function, and assigning that to the callback. I don't like the idea of passing req, res, next as arguments into the callback.

[kara-ryli]

Cool. I'll put some think together; functioning code is much easier to talk about.

[kara-ryli]

Add #423 -- I know you mentioned you don't like passing req, res, next to the callback, but it seems like it's necessary to avoid the

function (req,res,next) {
   passport.authenticate('strategy', function () {
     // stuff
   })(req,res,next);
}

structure. I hope my rationale in the PR makes sense.

[jaredhanson]

I don't think that structure needs avoiding

Sent from my iPhone

On Oct 16, 2015, at 12:36 AM, Ryan Cannon notifications@github.com wrote:

Add #423 -- I know you mentioned you don't like passing req, res, next to the callback, but it seems like it's necessary to avoid the

function (req,res,next) {
passport.authenticate('strategy', function () {
// stuff
})(req,res,next);
}
structure. I hope my rationale in the PR makes sense.

—
Reply to this email directly or view it on GitHub.

[kara-ryli]

Well, I'd love to understand why, but I'm not going to badger you if you're going to reject the PR. The way I see it, the existing structure:

• Requires creating a middleware function in memory on each request, when it could be reused
• Requires checking for the presence of a callback on every request, when that will never change
• Frequently confuses engineers that are still learning or coming from other languages

While the optimizations are small, I don't see any drawbacks of using them.

[jaredhanson]

Frequently confuses engineers that are still learning or coming from other languages

This one isn't a priority for me. Passport is written in JavaScript, and uses the middleware idioms established by Express/Connect. The learning curve is small, in my opinion, and worth climbing for any engineer who is serious about developing web applications in JavaScript. Further, once the middleware pattern is understood, it allows applications to be composed very easily. Passport itself takes advantage of this composibility.

The other two points are valid, and I'm considering them. However, I think they can be accomplished while remaining standard middleware and without changing the callback's function signature.

[kara-ryli]

I think that's a fair point; I'm starting to try and think through what the callback can even give you that other passport options do not, and what I'm getting is:

• It gives you access to error, success, info/challenge, status before triggering error middleware.
• It allows you to short-circuit req.logIn if for some reason you don't want to call it and still get authInfo
• It gives you access to information about failed logins without setting the WWW-Authenticate header

Doesn't seem like much. You can get pretty much the same behavior with something like:

var AuthenticationError = require('passport/lib/authenticationerror');

app.get('/protected',
  passport.authenticate('local', {
    authInfo: true,
    failWithError: true,
    session: false
  }),
  function(req, res) {
    // req.user and req.authInfo are available
    res.redirect('/account');
  },
  function(err, req, res, next) {
    if (err instanceof AuthenticationError) {
      // status is available on res.statusCode if it was truthy
      // challenge is accessible with res.getHeader('WWW-Authenticate') if it was a string
      // but has been cast into an array
      return res.redirect('/signin');
    }
    next(err);
  }
);

...which feels like better code than the callback version anyway. So maybe for my docs I'll just ignore the callback option.

If you are interested in some PRs that avoid checking properties more than once, I'm happy to contribute. It would break passport's ability to support something like:

var options  = {};
setInterval(function () {
  options.authInfo = !options.authInfo;
}, 100);
app.get('/protected', passport.authenticate('local', options);

...but that seems like an enhancement, not a regression.

[Bajix]

Welp, not sure how I found myself here, but anyhow.

You're trying to re-define conventions here which are very well established, and your proposed approach doesn't actually help in many of the use cases in which developers would use this sort of approach in the first place. Conventions around error handling and middleware aren't changing anytime soon.

In any case, learn lexical scope & take advantage of it. Creating functions in JS is super cheap.

Here's a more realistic example of how these pieces would fit together:

function authenticate( email, password, next ) {
  if (!validator.isEmail(email) || !validator.isLength(password, 6, 32)) {
   return next(new handler.InvalidCredentialsError('Password invalid'), false);
  }

  User.findOne({
    email: email.toLowerCase()
  }).select('+password').exec(function( err, user ) {
    if (err || !user) {
      return next(err || new handler.ResourceNotFoundError('Email not registered'), false);
    }

    user.verifyPassword(password, function( err, verified ) {
      if (err || !verified) {
        return next(new handler.InvalidCredentialsError('Password invalid'), false);
      }

      return next(null, user);
    });
  });
}

var strategy = new LocalStrategy({
  usernameField: 'email',
  passwordField: 'password'
}, authenticate);

exports.authenticate = function( req, res, next ) {

  req.checkBody('email', 'required').notEmpty();

  var errors = req.validationErrors();

  if (errors) {
    return next(errors);
  }

  passport.authenticate('local', function( err, user ) {
    if (err || !user) {
      return next(err || new handler.UnauthorizedError('Invalid login credentials'));
    }

    req.logIn(user, function(err) {
      if (err) {
        return next(err);
      }

      res.status(200).send(user);
    });

  })(req, res, next);
};

As you can see, your approach kind of defeats the point considering most people would need to do a custom wrapper anyway

I'm using express-error-funnel as my error middleware in order to do meaningful parsing, especially of errors created using node-restify-errors.

By the way, your PR is dependent on express flash, which was deprecated

[kara-ryli]

learn lexical scope

Ah, that's my problem. Thanks.

It doesn't matter, since the PR isn't going to get accepted. While I still think the API is unintuitive, the author doesn't believe that to be a problem, so it won't change.

If performance ever gets to be a problem, I can submit my load test results with a PR, but for now it's fine.

I just had some spare cycles and thought it would be fun to contribute to a good project, even if in a small way. Not interested in a flame war.