jaredhanson / passport

Simple, unobtrusive authentication for Node.js.
https://www.passportjs.org?utm_source=github&utm_medium=referral&utm_campaign=passport&utm_content=about
MIT License
22.93k stars 1.24k forks source link

`req.isAuthenticated()` returning false immediately after login #482

Closed jamesplease closed 8 years ago

jamesplease commented 8 years ago

StackOverflow question here.

I've been fighting for quite awhile with this bug: immediately after the user authenticates with, say, Google, req.isAuthenticated() is returning false. I'm persisting sessions to a Postgres DB, which seems to be working fine. It's just the call to isAuthenticated which leads me to wonder if my Passport configuration might be wrong, or something.

My code for the callback looks like:

const redirects = {
  successRedirect: '/success',
  failureRedirect: '/failure'
};
app.get('/auth/google/callback', passport.authenticate('google', redirects));

My very last middleware logs the value of req.isAuthenticated(). It logs false when Google redirects back to my page, but if the user manually refreshes then it returns true.

Here are detailed logs of the logging in process:

# this is the first request to the login page. Not logged in yet.
4:59:54 PM web.1 |  -----New request-----
4:59:54 PM web.1 |  route: /login authenticated: false

# they've clicked the link to `/login`, and are immediately forwarded to Google
4:59:59 PM web.1 |  -----New request-----

# they've granted across through Google; Google redirects back to app.
# Using the Google profile, we get the user's account from the user account table
5:00:06 PM web.1 |  -----New request-----
5:00:06 PM web.1 |  about to fetch from DB
5:00:07 PM web.1 |  retrieved user from DB

# redirection to the success page...`authenticated` is false? It should now be true!
5:00:07 PM web.1 |  -----New request-----
5:00:07 PM web.1 |  route: /success authenticated: false

# here's a manual refresh...now they're showing as authenticated
5:05:34 PM web.1 |  successful deserialize
5:05:34 PM web.1 |  -----New request-----
5:05:34 PM web.1 |  route: /success authenticated: true

It looks like deserialize isn't being called when Google redirects back to my app; could that be the source of the issue?

Source code:

jamesplease commented 8 years ago

Very slowly working my way through the issue. I've determined that Passport is failing to initialize properly under certain conditions.

The furthest back I've tracked the difference between a successful login and a bad one is these lines.

When the login immediately works (which is only if the user has never logged in before on that server instance), then req.session[passport._key].user is set in that conditional.

When the login fails until the user refreshes, then req.session[passport._key].user is undefined.


The downstream consequences of this are as follows:

  1. the session strategy never finds su
  2. because of that, it never sets req[property]
  3. and lastly, isAuthenticated() can't find the property, and returns false

The search continues...


I've gone a little further back.

The order goes:

  1. http.req#logIn
  2. authenticate#initialize
  3. session#authenticate

Step 1: logIn takes req._passport.session and assigns it to req.session._passport. Step 2: authenticate takes req.session._passport and assigns it to req._passport.session Step 3: session searches for req._passport.session.user

I have no idea why 1 and 2 are circular, or where any additional value comes from outside of this loop. @jaredhanson , any guidance on how I can make sense of this? 😛

jamesplease commented 8 years ago

logIn always finds req._passport.session.user in both situations. It then always sets it to req.session._passport. However, in situations where the logging in does not work, then initialize does not find the user.

So something must be intercepting req.session._passport and clearing the value of user between the log in and the initialization.


Maybe not. The two objects aren't the same (I used a global variable to test), so there must be some magic going on...not really sure tho'.


There are three types of states, when checked in initialize:

  1. the req.session._passport set by #logIn is deeply equal to the one accessed by initialize. In this case, logging in works.
  2. the req.session_passport set by #logIn is undefined. This seems to happen before logging in.
  3. The req.sesion._passport set by #login does not equal the one accessed by initialize, which is an empty obj; in this case, logging in does not work until I load another route.

The requests themselves don't seem to be the same between logIn and initialize, which is unexpected...

TitaneBoy commented 8 years ago

Hi.. I have the same issue. isAuthenticated() returns always "false" after authentication successfully, and after a redirect to an URL that needs to verify if the user is connected. For a reason I don't understand, isAuthenticated() returns false that force us to login twice.

Here is my code:

var sequelizeStore = require('connect-session-sequelize')(session.Store);
var path = require('path');
var customFunctions = require(path.join(process.cwd() + '/scripts/custom.js'));
var bodyParser = require('body-parser');
var auth = require(path.join(process.cwd(), 'scripts/auth.js'));

var strategy = 'local';
var passport = auth.configurePassportLocalBasicDigestStrategy(strategy);
var SequelizeVar = customFunctions.MySequelize();
SequelizeVar.import(path.join(process.cwd() , '/models/Sessions.js'));
var mystore = new sequelizeStore({db: SequelizeVar,
                                  table: "Sessions",
                                  checkExpirationInterval: 30000,
                                  expiration: 300000}); //3600000 for 1 hour
var app = express();

mystore.sync();
app.use(bodyParser.json());
app.use(bodyParser.urlencoded({extended:true}));
app.use(session({genid: function(req){return customFunctions.genuuid()},
                 secret: 'keyboard cat',
                 resave: false,
                 saveUninitialized: true,
                 store: mystore,
                 cookie: {maxAge: 300000}})); //3600000 for 1 hour

app.use(passport.initialize());
app.use(passport.session());

app.get('/loginFailure', function(req, res, next) {
    res.sendFile(require('path').join(process.cwd(), '/views/failureLogin.html'));
    next();
});
app.get('/login', function(req, res, next) {
    res.sendFile(require('path').join(process.cwd(), '/views/login.html'));
    next();
});
app.post('/login', function(req, res, next) {
  passport.authenticate(strategy, function(err, user, info) {
    if (err) {
      console.log("Error: " + err); 
      return next(err); 
    }
    if (!user) {
      console.log("Error: User does not exist"); 
      return res.redirect('/loginFailure'); 
    }
    req.logIn(user, function(err_login) {
      if (err_login) {
        console.log("Error while login: " + err_login); 
        return next(err_login); 
      }

      req.session.messages = "Login successfull";
      req.session.authenticated = true;
      req.authenticated = true;

      if (req.session.returnTo){
        return res.redirect(req.session.returnTo);
      }
      return res.redirect('/');
    });
  })(req, res, next);
});

Here is a part of my routes:

api_user.get("/", auth.isAuthenticated, function(req, res, next){
  res.send("Welcome to my API"); 
  next();
}, customFunctions.storeReadedDataPerSession);

Here is the definition of MY "auth.isAuthenticated()" function:

exports.isAuthenticated = function (req, res, next) {
  if (req.isAuthenticated()){
    console.log("Authenticated");
    return next();
  }

  console.log("Not Authenticated");
  req.session.returnTo = req.originalUrl;
  // IF A USER ISN'T LOGGED IN, THEN REDIRECT THEM SOMEWHERE
  res.redirect('/login');
}

So even if the user has a successfull authentication, it has to login twice before being redirected correctly.

Did you find any solution or any workaround to solve the bad "isAuthenticated()" returned value ?

Thank you in advance for your answer.

Marak commented 8 years ago

@TitaneBoy @jmeas

Just out of curiosity, what happens if you place the redirect inside a process.nextTick block? Maybe there is bug where you need to let the event loop process once before session sticks.

jamesplease commented 8 years ago

@Marak since logging is in asynchronous, nothing. It needs to wait for the request to complete. still haven't had time o set up an isolated case though :)

Marak commented 8 years ago

Are you sure the request needs to complete?

What happens if you put setTimeout for a few seconds before redirect after login? Does that affect it all?

jamesplease commented 8 years ago

A carefully placed setTimeout of a few seconds did fix it for me.

Marak commented 8 years ago

Yeah, I think perhaps one process.nextTick in the right place might fix it for you. Even a setTimeout of 1 or 0.

I think bug somewhere in async calls for passport or in the passport adapter you are using.

Marak commented 8 years ago

Also possible bug in your implementation code of passport. I didn't really read the code too much.

TitaneBoy commented 8 years ago

The "setTimeout" worked for me only once...and it was after 10s. I am not able anymore to reproduce it, event after 15 sec. I am not sure that setTimeout is a solution, even it looks a good idea. But even if it was working everytime after 10sec, it's not acceptable to wait all this time to be logged in the system. What do you think ?

Marak commented 8 years ago

It's not a solution...it's way to diagnosis problem. Is race condition with async calls. Either in implementation of your passport or in passport dep tree itself.

jamesplease commented 8 years ago

I've got it on my todo to spend more time trying to figure this out. I'll post an update when I've got one :v:

Yeah, I think perhaps one process.nextTick in the right place might fix it for you. Even a setTimeout of 1 or 0.

That'd be great.

I think bug somewhere in async calls for passport or in the passport adapter you are using.

Yeah, that might be.

Also possible bug in your implementation code of passport. I didn't really read the code too much.

Npnp. Yeah, that's definitely a possibility. I've tried a few different configurations based on existing projects. They all had the same problem.

arzavj commented 8 years ago

Hey @jmeas! Curious to know if you figured it out, I'm running into the same issue. Thanks a bunch!

jamesplease commented 8 years ago

No updates, but it's still on my todo. It's been a pretty nasty issue to debug when I did look into it. Trust me -- I'll be sure to check back in here once I figure out more. I admit I've turned my attention toward other parts of the project (auth is just one small piece), so it might be some time before I look back into it (weeks or more 🤐)

Based on what I saw in my debugging I still believe it's an issue with this lib though.

arzavj commented 8 years ago

Got it! Thanks so much @jmeas! I too spent quite a bit of time looking into it but to no avail.

jamesplease commented 8 years ago

I've dedicated this whole day to solving this issue. So far, here's what I've got. Versions of the libs I'm using:

Passport 0.3.2 Google Strategy for Passport 1.0.0 Express-session 1.14.0 node-connect-pg-simple 3.1.0 (for persisting sessions to Postgres)


Description of the problem

When the user signs in with Google, they are sent back to my application. However, m application shows them as logged out. If they refresh the app, then they are displayed as logged in.


The problem begins with the callback route from logging in through Google. In my app, that URL is auth/google/callback. Let's walk through the middleware to see if we can find out where unexpected behavior occurs.

The first relevant middleware is express-session. Sometimes, there's an existing session in the DB. Other times, there isn't. Either way, it doesn't matter. If there is a session, then there is no user data because Passport hasn't confirmed that the user is logged in. We would expect the session to get updated after Passport does its thing.

Next up is the Passport middleware. Passport automatically has a Session Strategy set up (you, as the developer, do not need to do anything). Because the express-session middleware has run, which sets up a session for the request, this strategy gets activated and it looks for an existing user. We've already determined that we shouldn't expect a user, so, as expected, strategy fails.

Next up is the Google Strategy that we've configured for this route. This one succeeds, because the user clicked "Allow" on the Google page. The Passport success process begins.

This is where things get interesting, so I'm going to slow down a bit.

  1. Passport: strategy.succeed (src)

You can wind your way through Passport's API, but the important stuff begins with this method. This is called when a Strategy succeeds. It's a big function, but we're only concerned about a few things.

What we need to know is that at this time, the Google Strategy successfully parsed the response from Google, and knows who you are. It then calls this method, passing the user to it. Everything is good so far. Next up, we let Passport log us in.

  1. Passport: req.logIn (src)

This is the first interesting thing that strategy.succeed does.

Before we talk about it, an important thing to know is that Passport maintains a special attr on the session called passport. And by default it sets who the user is under the key user. So a very important piece of our request is req.session.passport.user.

The role of logIn is to set that up for us. It does that using serializeUser, which delegates to the method that you, the developer, configure in your app (example here). For me, this is a synchronous operation that just returns user.id.

my `deserializeUser()` (click to expand) ``` js passport.serializeUser((user, done) => { console.log('Serializing user'); done(null, user.id); }); ```

Alright, so, what's going on now is that our session has been written to. It has a key that can be used to identify our user in the future. Pretty dope. We're sent back to the strategy.succeed method...

  1. Passport: strategy.succeed (logIn callback) (src)

Here's where the issue comes in (I think). If you set up a redirect URL via the successRedirect option, then it's immediately called. If you don't set one up, then you're probably using another middleware that immediately redirects like so. This is then called.

Alright, so, let's assume that we're redirecting somehow, and jump over to Express.

  1. Express: response.redirect (src)

The important bit here is that the request is ended, always.

  1. express-session: end

Somewhat surprisingly, this lands us back into the very first middleware: express-session. This middleware replaces res.end with its own version, which is used to persist session data.

I don't think the the source of express-session was optimized for readability, but the important thing to know is that the session will save itself if its been modified. Back in logIn, the session was modified, so the save begins now.

This happens in connect-pg-simple, but the important bit is that the Express redirect happens before the save completes.

Instead, a new request begins while the save is in progress...

  1. Express: new request to /success

Let's start over. The first thing that happens is that the session is initialized. It immediately begins a request for the session, which hits the DB.

This is where the race condition stuff comes in. A get and a save are in flight at the same time. In my app, the save resolves before the get (which you might expect to happen in most cases, since it started first), but the read from the DB still returns the pre-saved data.

If you remember, the pre-saved data didn't have a user (because Passport never logged them in), so the user ends up being considered logged off.


The cause of the issue

The ultimate cause of the issue seems to be that Express begins the new request before the old request is completely done. I'm not sure why this is!

jamesplease commented 8 years ago

@dougwilson ultimately provided the answer over here.

express-session tries to delay the redirect, but some browsers don't wait for the whole response before directing. So you need to manually save before redirecting. In my app, this looks like:

app.get('/auth/google/callback', passport.authenticate('google', redirects),
  function(req, res) {
    // Explicitly save the session before redirecting!
    req.session.save(() => {
      res.redirect('/success');
    })
  });
arzavj commented 8 years ago

Ahh got it! That fixed it for me! Thanks so much @jmeas!!!! 👍 💯

jamesplease commented 8 years ago

All thanks goes to @dougwilson honestly : )

arzavj commented 8 years ago

Haha yes! Thanks so much @dougwilson! :)

vhmth commented 8 years ago

@jmeas boom thank you!

lerit commented 8 years ago

i think when use express-session and store session to db will cause this issue.i can resolve it by call 'req.session.save' before res.redirect;but i think you should call 'req.session.save' when call 'failureRedirect' or 'successRedirect' function too.if i set failureFlash:true, the failureRedirect can not read req.flash('error') too. sorry for my poor english! @jmeas

dman777 commented 7 years ago

@jmeas Thanks for all your hard work and investigation which lead to @dougwilson. I was stuck on this for a long time.

zelflod commented 7 years ago

The solution doesn't work with 'passport-facebook'. Namely, after

app.get('/auth/facebook/callback', 
passport.authenticate('facebook', {failureRedirect : '/'}),
function(req, res) {
    res.redirect('/profile');
});

the req.isAuthenticated() is false and after

app.get('/auth/facebook/callback', 
passport.authenticate('facebook', {failureRedirect : '/'}),
function(req, res) {
    res.json(req.user);
});

req.isAuthenticated() is true I've tried setTimeOut, req.session.save, but nothing works with redirect. So I'm stuck)

jakubrpawlowski commented 7 years ago

@nozimy are you testing it on localhost? Localhost is too fast so redirect happens too fast. In production it will be all good! Fear not!

mjquito commented 6 years ago

Unfortunely, the workaround ends up calling "res.session.save()" twice. One from the app and the other from the library.

app.get('/auth/google/callback', passport.authenticate('google', redirects),
  function(req, res) {
    // Explicitly save the session before redirecting!
    req.session.save(() => {
      res.redirect('/success');
    })
  });
sowston commented 6 years ago

+1 I have secured routes that I would like the user to redirect "back" and passport.isAuthenticated() always returns false when redirected back to. The SAML Strategy works well in doing this. I also cannot get the "workaround" to work (req.session.save(...))

sowston commented 6 years ago

I have opted to add an "unsecured" redirect route that is redirected to from the '/login/callback' route:

app.get( '/redirect', function (req, res) { res.status(200).send('<html><head><meta http-equiv="refresh" content="0; url=' + req.query['redirect_uri'] + '" /></head><body>Redirecting...</script></body></html>') } ) It appears that once the callback route fully completes, the passport.isAuthenticated() method will finally return true.

jakubrpawlowski commented 6 years ago

Again, from my experience it all works just fine as long as you use it and test it on the web. Testing it on localhost will often result in too fast redirects.

sowston commented 6 years ago

Than you for the reply @jakubrpawlowski . I actually am duplicating the issue in a DEV environment in OpenStack on our Corp network.

RicardoGonzalezJ commented 5 years ago

The solution works for me. I'm using a local strategy with passport, a custom callback and saving the session manually fix the bug. I tested on localhost and it works fine. Thanks to @jamesplease and @dougwilson.

Here is my code: this is my passport config:

passport.use(new LocalStrategy(option, (req, username, password, done) => {
  findUser(username, password, (err, userData) => {
    if (err) {
      console.log('error on passportConfig.js LocalStrategy', err);
      throw done(err);
    } else {
      if (isEmptyObject(userData.rows[0])) {
        return done(null, false, req.flash('info', 'user not found'));
      }
      if (!userData.rows[0].validpass) {
        return done(null, false, req.flash('info', 'password is incorrect'));
      }
      return done(null, userData.rows[0]);
    }
  });
}));

This is my login route:

router.post('/login', (req, res, next) => {
  passport.authenticate('local', (err, user) => {
    if (err) {
      console.log('error on userController.js post /login err', err);
      return err;
    }
    console.log('user', user);
    if (!user) {
      req.flash('info');
      return res.redirect('/users/login');
    }
    req.logIn(user, (logInErr) => {
      if (logInErr) {
        console.log('error on userController.js post /login logInErr', logInErr); return logInErr;
      }
      // return res.status(200).json(user[0]);
      req.flash('info', 'Bienvenido');
      req.session.save(() => res.redirect('/'));
    });
  })(req, res, next);
});

I hope this will help other to solve this bug. This is the the repository passport_auth

mahdi-farnia commented 2 years ago

Please consider this to prevent breaking your stuff that is around you: ( this might help )

Please note that secure: true is a recommended option. However, it requires an https-enabled website, i.e., HTTPS is necessary for secure cookies. If secure is set, and you access your site over HTTP, the cookie will not be set.

If you enable cookie.secure to true and your server ( like localhost ) is still http, cookies will not set and then session DOES NOT WORK AT ALL.

have good day!

otokoAbuali commented 1 year ago

I set the sameSite option to false, and it works for me cookie: { path: "/", httpOnly: true, secure: true, sameSite: false } app.get('/auth/google',passport.authenticate('google',{failureRedirect:"/login",successRedirect:"/"}));