Allow passport to return authentication failure messages via variable

[EvHaus]

As per my discussion with Jared via IM, this issue is to require the ability to pass back authentication failure messages to the passport.authenticate function. Perhaps via:

passport.authenticate(... func(err, user, info))

The issue right now is that there is no good way to return back an error message from the LocalStrategy. The only option is to either return a server failure via 'err' which will cause the server to simply return a 500 HTTP error, or to use a workaround which uses the user object to return back the error message.

As a result, the done function will probably look something more like this:

done(null, false, 'bad password')

Here is a jsfiddle where I'm running into the issue. In this example, the server simply returns a 500 error for all failures -- and I want it to return an error string instead:

http://jsfiddle.net/nd8LZ/1/

[nickjj]

That's basically how I'm doing it too, except I'm not returning json. As long as the new function allows you to return any variable and not just a string and you can pass that variable to res.xxx() in your router (so you can pass any auth errors to your views easily) I'd be happy.

Edit: Jared, I read through a few related issues and you mentioned flashing a message by default. Are you talking about using express' req.flash()? I would stay away from that since it's deprecated in express 3.0: https://github.com/visionmedia/express/wiki/Migrating-from-2.x-to-3.x (check the removed section)

[jaredhanson]

passport-local v0.1.2 address this issue. See this comment on Passport issue #12 for details.

Also note that the above syntax will work

done(null, false, 'bad password')

but the preferred form is to use an options hash

done(null, false, { message: 'bad password' })

as it is more extensible for future use.

[nickjj]

Can you change around how the message is returned? Using req.flash() is silly. Perhaps you can just make it throw an err and let the developer decide how to handle it?

[jaredhanson]

@nickjj I agree completely with all your points. The arguments to done in the verify callback are carried through exactly to the custom authenticate callback, so when you call done():

done(null, false, { message: 'bad password' })

That last argument is the info argument to the authenticate callback:

app.get('/login', function(req, res, next) {
  passport.authenticate('local', function(err, user, info) {
    if (err) { return next(err) }
    if (!user) {
      // *** Display message without using flash option
      // re-render the login form with a message
      return res.render('login', { message: info.message })
    }
    req.logIn(user, function(err) {
      if (err) { return next(err); }
      return res.redirect('/users/' + user.username);
    });
  })(req, res, next);
});

From there you can call any message you want on the request (render in this case), and use any details provided in info.

I'm not the biggest fan of flash myself, and am aware that it's being deprecated in Express. (I suspect you'll see middleware to polyfill it back in, though.) That said, if you never use the option, it won't affect you. I wan't to keep Passport framework agnostic, and these options are really just shortcuts for common patterns, Express or otherwise. Flash messages are common enough that it makes sense to include.

I'm open to adding additional options if there are better ways of messaging or other standard operations. Outside of that, custom callbacks are the way to go.

Does that seem OK to you?

[nickjj]

In 3.x, express now has view-specific middleware where you can inject site wide variables to all of your views with app.locals.use(...).

The workflow to use that (for example) is:

In app.js...

app.locals.use(function(req, res) {
   // Expose "error" and "message" to all views that are rendered.
   res.locals.error = req.session.error || '';
   res.locals.message = req.session.message || '';

   // Remove them so they're not displayed on subsequent renders.
   delete req.session.error;
   delete req.session.message;
});

source: https://github.com/visionmedia/express/blob/master/examples/blog/app.js#L32

In one of your routes you would just do req.session.message = info.message with the new setup but the route doesn't have access to info. Are you saying we would still need to use a custom callback to output the message in a way that doesn't depend on req.flash()?

I'm still pretty new to node/express/JS in general, sorry if my questions are retarded.

[jaredhanson]

Your questions are great. You've got a good handle on what is going on.

Are you saying we would still need to use a custom callback to output the message in a way that doesn't depend on req.flash()?

If you are using Express 3, the yes, for the time being. Here's how that would work:

app.get('/login', function(req, res, next) {
  passport.authenticate('local', function(err, user, info) {
    if (err) { return next(err) }
    if (!user) {
      // *** Display message using Express 3 locals
      req.session.message = info.message;
      return res.redirect('login');
    }
    req.logIn(user, function(err) {
      if (err) { return next(err); }
      return res.redirect('/users/' + user.username);
    });
  })(req, res, next);
});

Now, I admit that is a fair amount of extra effort just to set a simple message. Here's my philosophy on this:

Passport is designed to be generic authentication middleware, usable in any app framework. It does this by delegating control back to the application for tasks that are logically under the application's responsibility. Rendering, redirecting, messaging, etc all fall under that category, so any custom work that is unique to your application or your application's underlying framework should be handled in the callback.

Where common patterns emerge across frameworks, Passport will provide options to make those patterns easier (thus the existence of failureRedirect, failureFlash, etc.) Flash messaging is one such common pattern, so its useful to have Passport support it.

I'm personally in favor of @visionmedia's decision to remove req.flash() from Express. I think that decision will result in that functionality being adopted by a lower level piece of middleware that can get dropped in and reused across frameworks. (I'm working on just such middleware at the moment; I'll update here when it is published).

If a different messaging pattern emerges that is useful across a variety of apps, then I'll consider adding a built-in option to Passport (say failureMessage or some such). But until then, the custom callback allows you to accomplish whatever your app needs.

[nickjj]

Ok thank you. That clears up pretty much everything. I'm looking forward to sticking with Passport.

[jaredhanson]

@nickjj I just published connect-flash, which is middleware you can drop into an Express 3.x app: https://github.com/jaredhanson/connect-flash

If you use it, you can also use the successFlash and failureFlash options with passport.authenticate().

[nickjj]

I'm anti req.flash() too, so I likely won't be using that however I do think a lot of people will enjoy it going by the sheer number of times I've seen req.flash() in example express apps. On a side note your connect-flash middleware is a really good/basic example of how to properly implement "real" middleware in express.

[nickjj]

Whenever auth fails it always says "Missing credentials" which seems to be hard coded into the passport lib itself.

When I overwrite it by following the custom callback example and output info.message it always reports "Missing credentials" still, like it's ignoring whatever is in my passport.use(LocalStrategy...).

Also I seem to always get "failed to deserialize user out of session" whenever I successfully login. I've followed your examples and tutorials. I've tried using both the standard express session store and redis. The page I'm working on worked before I upgraded passport/passport-local to the latest version. Any idea what's going? Should I post all of the code?

[jaredhanson]

@nickjj Have you figured out the cause of your issues?

Everything is working fine for me, and if Passport were broken on the whole, I'd be hearing about it from other people. Since I'm not, I'm guessing it is something specific to your setup. Post a gist with code that reproduces the problem, and et me know if I can provide any help.

[nickjj]

Yes, I was using a version of express 3.x that had a buggy version of connect (I think it was 2.0.0 or 2.0.1). Tj fixed a bug in connect recently that did something to fix sessions. After upgrading to the latest connect passport is back to working.

I just ported your example app in your passport-local repo to an express 3.x app and it works. It works using both the standard connect session store and a redis session store (connect-redis module).

The other issue I had was my fault. When I changed your mock data to access a real database (mongodb), it was having issues deserializing the data because I was passing the id from deserializeUser() directly into my mongo findOne() as a string but it needs to be converted to an ObjectID first.

I'm still not 100% comfy with this new stack so when I read your tutorial guide and followed along, it showed you using findOne() during a deserializeUser(). I just figured "oh ok, he's using findOne() so he must be using mongo too and just passes the id no problem".

Everything seems to be good to go now for using passport-local.

The real problem now is, if I'm latched into using a custom callback to get custom messages with express 3.x and I have to define my own local strategy depending on what's checking the data then why am I using passport-local?

Why not just use a standard function to authenticate against my database and write even less code in my route to detect if it's a good or bad login attempt?

I definitely want to use passport-xxx if I decide to support logging in with 3rd party services, but I'm not sure why local (with custom callbacks) is even worth using? Can you sell me on using it?

[silbo]

The only good way to put a failure message I found was this way.

app.post("/login", passport.authenticate("local", 
  { successRedirect: "/", 
    failureRedirect: "/login", 
    failureMessage: "Invalid username or password" }));

Then the message will be stored in req.session.messages, which is an array, the only problem is that it is static and I think there is a bug that the array will concatenate the same message over and over again. This was fixed by the following code, after the response was rendered :P

res.render("login", { login_errors: req.session.messages || [] });
req.session.messages = [];

[Zane-XY]

What's the point of using failureFlash here? The way how to show the errors varies from case to case, and why can't we just use a callback function to deal with it whatever makes sense? Quote from the doc:

Setting the failureFlash option to true instructs Passport to flash an error message using the message given by the strategy's verify callback

• first of all, I don't want to flash an error message
• second, seems passport can't even flash a message without installing another dependency? and I guess I probably just install it to flash one login error message.

[marcogreselin]

How can I return multiple messages, for instance if the login fails I want to return an appropriate error message and the email that was entered so I can feed the email input again.

done(null, false, { message: 'bad password', email: emailVariable});

This does not work.

[dbauszus-glx]

@silps

req.session.messages = [];

What is this supposed to do? Resetting the session messages? I don't think this will work.

I just display the last of the messages in the array. Like so in EJS:

<% if (login_errors.length > 0) { %> <%= login_errors[login_errors.length -1]%><br> <% }%>

`

[silbo]

@GEOLYTIX yes, this approach will work also, it's just that each time I refreshed the site there was one more of the same message in the array, so I cleaned it each time, but your way also works

[riclf]

@jaredhanson I have a programming situation where I want to know if the user exists, because I am gong to take different application action based on if they already exist or not. So I wish there would be a passport.js URL to provide support for /exist , where I provide a username and passport returns a 0 or 1. Ok, this is not available. So I post to /login with a username and password. The password can be anything. In the passport.use function if the username is not found I return-

    if (!user) {
        return done(null, false, { "rtnCode": 1 });    // user not found, user does not exist
    }

If the user was found I instead return with- done(null, false, { "rtnCode": 2 })

                  // check if password is valid     
                   if (!validatePassword(password, passHash)) {
                        //return done(null, false);                         // as per passport.js example
                        return done(null, false, { "rtnCode": 2 });
                   }

Now, in passport.authenticate('local-login', function(err, user, info) info will contain either { "rtnCode": 1 } or { "rtnCode": 2 }

That means that in

        // incorrect Username or password
        if (!user) {
                //return res.redirect('/auth/login');     // as per passport.js example
            return res.json(info);               // rtn code- { "rtnCode": 1 } or { "rtnCode": 2 }
                                                                                 // user does not exist, or does exist
            }

Now I can send a specific return code to my app which provides me the information that the user exists or not. Here is the problem. Even though the execution flow ends with a return and so does not ever run- req.logIn(user, function(err)

If I now issue an ajax GET with the username that I know exists and a bogus password, I get in, I am able to read the database.

That is very unexpected as I have never issued req.logIn(). And of course it is a security issue. The point here is that upon a "bad" username or password in my /login I do not want to go to a webpage, I want to take a different course of action in code.

[Gilbert136]

Im using angular4 with passport, is there anyway i can get res without the redirect feature in passport?

[Gilbert136]

So it happens that passport has a function to do solve that in their documentation, A section call custom callback helped me solved it

[banyustudiodev]

//routes examples router.get('/', function (req, res, next) { res.render('administrator/login', { title: 'Login Admin', alerts: req.flash() }); });

//ejs handler `<% if (alerts.error) { %> <% alerts.error.forEach(function(msg) { %>

<%= msg %>

<% }); %> <% } %> <% if (alerts.success) { %> <% alerts.success.forEach(function(msg) { %>

<%= msg %>

<% }); %> <% } %>`

[itsgratien]

thanks @jaredhanson !! you saved me hhhh

[godmar]

There is abundant confusion on the web regarding how the information passed as info to the verify callback is being used, e.g. here. The main issue that confuses people is that the following does not work:

const express = require('express');
const app = express();
const passport = require('passport');
const { Strategy: LocalStrategy } = require('passport-local');

app.use(express.json());

passport.use(new LocalStrategy({ failWithError: true },
    (username, password, cb) => {
        console.log(`executing local strategy ${username} and ${password}`);
        // user expects 'message: `user not found`' to be forwarded to the 
        // client along with the 401 Unauthorized response
        cb(null, false, { message: `user not found` });
    })
);

app.post('/login', passport.authenticate('local', { session: false }))

app.listen(3000, () => {
  console.log(`Run:`);
  console.log(`curl -v --header "Content-Type: application/json" -d '{"username":"dauser","password":"dapass"}' -X POST http://localhost:3000/login`);
});

In other words, the default action of the passport-local middleware is to ignore what's being passed to verify(null, false, info) in the info slot, unless the user invokes the middleware function produced by passport.authenticate() directly via passport.authenticate(...)(req, res, next) and sends the response themselves. @jaredhanson comment from 2012 seems to indicate that this is the desired behavior, at least as of 2012.

This confuses users who expect to use passport.authenticate() as middleware before their own request handlers.

Exacerbating the matter is the fact that passport.authenticate takes several options, none of which have the desired behavior of passing along the information to the client.

I'm wondering if this could/should be revised. It seems, to me, a perfectly established pattern to - by default - take the object passed as info and send it as a JSON body to the client in the response. If not by default, an option sendErrorInfoToClient could be provided. Or am I wrong, and it's best practice to not send anything with a 401?

[mattcarlotta]

It's been 7 years and I don't think this feature will ever get implemented. That said, you can create a workaround to keep the strategy as a middleware function and still have custom messages sent back to the client (and the function can be made reusable for other controllers if desired):

Express route:

const { login } = require("../controllers/login)";
const localLogin = require("../services/strategies)";

module.exports = app => {
  app.post("/api/login", localLogin, login);
};

Passport Middleware

const bcrypt = require("bcryptjs");
const passport = require("passport");

// reusable func to return errors to client
const sendError = (err, res) => res.status(400).json({ err: err.toString() });

const badCredentials = "There was a problem with your login credentials. Please make sure your username and password are correct.";

passport.use(
  "local-login",
  new LocalStrategy(
    {
      usernameField: "email",
      passwordField: "password",
      passReqToCallback: true,
    },
    async (req, email, password, done) => {
      try {
        // check to see if the user already exists
        const existingUser = await User.findOne({ email });
        if (!existingUser) return done(badCredentials, false);

        // compare password to existingUser password
        const validPassword = await bcrypt.compare(
          password,
          existingUser.password,
        );
        if (!validPassword) return done(badCredentials, false);

        return done(null, existingUser);
      } catch (err) {
        return done(err, false);
      }
    },
  ),
);

// exporting a wrapper function that will invoke the passport.authenticate method
module.exports = (req, res, next) => {
  const { email, password } = req.body;

  // if email or password are missing, send an error back to the client
  if (!email || !password) return sendError(badCredentials, res);

  passport.authenticate("local-login", (err, user) => {
    // if an error was returned by the strategy, send it to the client
    if (err) return sendError(err, res);

    // manually setting the logged in user to req.user 
    // optionally, you can set it to "req.session" if you're using some sort of session
    req.user = user;

   // invoking "next" to continue to the controller
    next();
  })(req, res, next);
};

// alternatively, you can use a promise as well...
module.exports = async (req, res, next) => {
  const { email, password } = req.body;

  if (!email || !password) return sendError(badCredentials, res);

  try {
    const user = await new Promise((resovle, reject) => {
      passport.authenticate("local-login", (err, user) =>
        err ? reject(err) : resolve(user),
      )(req, res, next);
    });

    req.user = user;
    next();
  } catch (err) {
    return sendError(err, res);
  }
}

Express Controller

exports.login = (req, res, done) => {
  // do any additional aggregation with "req.user"

  res.status(202).json({ ...req.user, ...etc });
};

The biggest advantage of this approach is that you can now unit test the authenticate strategy separately from the controller. The disadvantage is that this still doesn't support passport sessions (which doesn't appear to be supported when utilizing any custom callbacks).

[vigansokoli]

It's been 7 years and I don't think this feature will ever get implemented. That said, you can create a workaround to keep the strategy as a middleware function and still have custom messages sent back to the client (and the function can be made reusable for other controllers if desired):

Express route:

const { login } = require("../controllers/login)";
const localLogin = require("../services/strategies)";

module.exports = app => {
  app.post("/api/login", localLogin, login);
};

Passport Middleware

const bcrypt = require("bcryptjs");
const passport = require("passport");

// reusable func to return errors to client
const sendError = (err, res) => res.status(400).json({ err: err.toString() });

const badCredentials = "There was a problem with your login credentials. Please make sure your username and password are correct.";

passport.use(
  "local-login",
  new LocalStrategy(
    {
      usernameField: "email",
      passwordField: "password",
      passReqToCallback: true,
    },
    async (req, email, password, done) => {
      try {
        // check to see if the user already exists
        const existingUser = await User.findOne({ email });
        if (!existingUser) return done(badCredentials, false);

        // compare password to existingUser password
        const validPassword = await bcrypt.compare(
          password,
          existingUser.password,
        );
        if (!validPassword) return done(badCredentials, false);

        return done(null, existingUser);
      } catch (err) {
        return done(err, false);
      }
    },
  ),
);

// exporting a wrapper function that will invoke the passport.authenticate method
module.exports = (req, res, next) => {
  const { email, password } = req.body;

  // if email or password are missing, send an error back to the client
  if (!email || !password) return sendError(badCredentials, res);

  passport.authenticate("local-login", (err, user) => {
    // if an error was returned by the strategy, send it to the client
    if (err) return sendError(err, res);

    // manually setting the logged in user to req.user 
    // optionally, you can set it to "req.session" if you're using some sort of session
    req.user = user;

   // invoking "next" to continue to the controller
    next();
  })(req, res, next);
};

// alternatively, you can use a promise as well...
module.exports = async (req, res, next) => {
  const { email, password } = req.body;

  if (!email || !password) return sendError(badCredentials, res);

  try {
    const user = await new Promise((resovle, reject) => {
      passport.authenticate("local-login", (err, user) =>
        err ? reject(err) : resolve(user),
      )(req, res, next);
    });

    req.user = user;
    next();
  } catch (err) {
    return sendError(err, res);
  }
}

Express Controller

exports.login = (req, res, done) => {
  // do any additional aggregation with "req.user"

  res.status(202).json({ ...req.user, ...etc });
};

The biggest advantage of this approach is that you can now unit test the authenticate strategy separately from the controller. The disadvantage is that this still doesn't support passport sessions (which doesn't appear to be supported when utilizing any custom callbacks).

Thank you, this was very useful and clean.