jaredhanson / passport-totp

TOTP authentication strategy for Passport and Node.js.
MIT License
151 stars 47 forks source link

Custom error handling #8

Open justusmaier opened 8 years ago

justusmaier commented 8 years ago

I would like to handle the failed login with status 400. But when the secondFactor fails, no error or message are given. My current implementation always grants the login. strategy:L78 fail() does not give an error message, is this intended?

// my current code, which does not send 400 in case of error
function(req, res, next) {
    passport.authenticate('totp', {failWithError: true}, function(err, user, info) {
      console.log('totp login', err, user.name, info);
      // prints: totp login null undefined undefined
      if (err) {
        res.send(400, {error:err, info: info});
        return;
      }
      auth.grantSecondFactor()(req, res);
    })(req, res, next);
  }
justusmaier commented 8 years ago

Found a solution:

app.post('/login-second-factor',
  passport.authenticate('jwt-bearer', {session: false}),
  auth.loginStaticFactor(), // this is a bypass with for a static passcode
  passport.authenticate('totp', {failureRedirect: '/login-failed'}),
  auth.grantSecondFactor()
);
app.get('/login-failed', function(req, res) {
  res.send(400, 'Login failed');
});

I would like to get an answer to my question and the documentation should be improved before this can be closed.

unknowndomain commented 7 years ago

First, not sure exactly why you need 400, which is bad request, 401 would make more sense (unauthorised).

This behaviour comes from passport.js not passport-totp, however there is a short sequence of behaviour, it looks for 'failureFlash' and if it exists, sets an express-flash message, however there is an issue with passport-totp that it has no default message, so unless you set one it will not display a message, i.e. if you put:

failureFlash: true

It does nothing, however:

failureFlash: "Incorrect code"

Would work, there is a pull request for this, but plainly this module has been abandoned as there are issues and pull requests from years ago unanswered.

Next passport.js will check for a failureRedirect and apply that also, as you've discovered.

Finally if a redirect wasn't applied it will send a 401 error, which as above seems logical.

I'm not sure why you want to use 400, but given that this isn't normal use, the only answer is to modify passport.js, it's unlikely anyone will ever add a feature to change the error code.

olivier-soumillon commented 3 years ago

I have the same issue, I'm trying to use the custom callback of passport.authenticate function, as shown below

const verifyOTPMiddleware = (req, res, next) => {
    const callback = (error) => {
        console.log('callback error', error)

        if (error) {
            return next(error)
        }

        next()
    }

    passport.authenticate('totp', callback)(req, res, next)
}

But this does not work (I get a 200 response, when I should have a 401, and the console log returns null as error)

however if I'm using it directly in my route, doing the following app.post('/totp', passport.authenticate('totp'), routeHandler), I do get the 401 error I was expecting.

If I use the option failureRedirect in my verifyOTPMiddleware passport.authenticate('totp', {failureRedirect: '/login-failed'}) , the redirection is attempted as expected.

So it seems for some reason the error does not reach the custom callback of passport.authenticate