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

Destroying all sessions for a given user? #865

Open
kleydon opened this issue Jan 7, 2022 · 17 comments
Open

Destroying all sessions for a given user? #865

kleydon opened this issue Jan 7, 2022 · 17 comments

Comments

@kleydon
Copy link
Contributor

kleydon commented Jan 7, 2022

Hi,

I'm maintaining the express session store for Prisma (prisma-session-store).

Recently, a developer asked whether there might be a way to destroy all sessions for a given user (as might be desirable when logging out of all devices, changing a password, etc).

While this can be accomplished at the back-end application layer, it requires (I think?) downloading all sessions, and then filtering them, which might not be ideal if there are hundreds or thousands of sessions.

I'm considering adding the ability to destroy all sessions for a given user as a feature specific to the data store that I'm maintaining, but wanted to check in first, to see if something along these lines might be in the cards for the express-session library (and the session store interface it exposes) more generally...

Do you imagine this library would surface this sort of functionality in the future? Or does this seem like the sort of thing that ought to live in each specific session store implementation?

Any advice / recommendations / rationales would be much appreciated.

@revington
Copy link

@kleydon I implemented this in the past by changing the genid function. The idea is to generate a random id that we can find later i.e $userid-random(). Then in your database you can query by all keys starting with user id $userid-*.

The trick here is to overwrite the user session id once they log in.

@kleydon
Copy link
Contributor Author

kleydon commented Jan 17, 2022

@revington - thanks for this!

@ultimate-tester
Copy link

ultimate-tester commented Feb 13, 2022

The solution provided by revington is definitely a good one, it will do what you expect.
Though, I am not a fan of solutions that involve loops. Especially when you have a high-traffic site, you will notice problems. On top of that, having the list of sessions per user is probably not the complete functionality requested. Usually sites that offer the functionality you describe also show what kind of device the session is associated with, the login time, the IP, and other metadata.

Therefore, my suggestion is to leave the requested functionality out of session management but persist this information in the database. Upon login, the user session ID should be stored into the database together with all the metadata you want. You might think "But then you are storing data redundantly as my session ID is already in the database", but I disagree with that. You are merely storing a reference just as you would with other relations between tables.

@kleydon
Copy link
Contributor Author

kleydon commented Feb 15, 2022

@ultimate-tester Thanks; appreciate your insight / experience on this.

@florianwalther-private
Copy link

@ultimate-tester I found this thread through a Google search. Can you confirm that I understand your suggested approach correctly:

  1. When the user logs in, I store the session id in an array on the user object
  2. When the user then changes their password (or does anything else that requires all previous sessions to be terminated), I get that array out of the user database entry and iterate over it, calling session.destroy(sid) for each of them?

So I assume it is safe to store express session ids in the database (sorry for the noob question)?

@ultimate-tester
Copy link

@ultimate-tester I found this thread through a Google search. Can you confirm that I understand your suggested approach correctly:

  1. When the user logs in, I store the session id in an array on the user object

  2. When the user then changes their password (or does anything else that requires all previous sessions to be terminated), I get that array out of the user database entry and iterate over it, calling session.destroy(sid) for each of them?

So I assume it is safe to store express session ids in the database (sorry for the noob question)?

Hi there, you're completely right in understanding. Session IDs are not a secret so they don't need special treatment, they get sent to the client after all (although signed, but that's to prevent people from "guessing" valid session IDs)

@florianwalther-private
Copy link

@ultimate-tester Thank you. Would you additionally check on each request if the sessionId is contained in the user array? I'm a bit afraid of having orphaned sessions because the destroy call failed but the session id array was emptied out.

@ultimate-tester
Copy link

@ultimate-tester Thank you. Would you additionally check on each request if the sessionId is contained in the user array? I'm a bit afraid of having orphaned sessions because the destroy call failed but the session id array was emptied out.

Good question. I currently have a quite naive approach by using user login and logout routes to store and remove the session ID from the database.

@revington
Copy link

@florianwalther-private @ultimate-tester there is no need to loop nor keep a list of ids.

The way I do this:

create a genid function that prefix id with user id

function genid(req){
  const userId = req?.session?.userId || 0;
  const randomId = /* random id without "-" char*/
  return `${userId}-${randomId}`;
}

supply this function to session when you create the session middleware.

When a user logs in just copy all session data into some variable and call regenerate this will terminate previous session and create a new one. copy old data back again to session.

How do you implement destroy all? just go to redis or any other backend and delete ${userid}-* sessions

@florianwalther-private
Copy link

@revington Thank you, that seems like a better idea and should avoid orphaned sessions (which I think can be very dangerous). I'll try that out 👍

@florianwalther-private
Copy link

florianwalther-private commented May 10, 2022

@revington req is undefined when I try this. How did you set this up so the userId is available there?
I put the userId in the session object at login after the credentials were verified.

@revington
Copy link

@florianwalther-private are you passing your genid function to session function? https://github.com/expressjs/session#genid
You don't call genid directly. It is being called by session when you call #req.session.regenerate(). So, after a user logs in, call #req.session.regenerate https://github.com/expressjs/session#sessionregeneratecallback

@florianwalther-private
Copy link

florianwalther-private commented May 10, 2022

@revington

I call this on login:

exports.createSessionForUser = async (req, user) => {
    req.session.userId = user._id;
    const sessionData = req.session;
    await new Promise((resolve, reject) => {
        req.session.regenerate(err => {
            if (err) {
                reject(err);
            } else {
                resolve();
            }
        });
    });
    req.session = sessionData;
};

And this is my session setup:

app.use(session({
    store: new RedisStore({ client: redisClient }),
    secret: process.env.SESSION_SECRET,
    saveUninitialized: false,
    resave: false,
    cookie: {
        maxAge: 31 * 24 * 60 * 60 * 1000,
        httpOnly: true,
    },
    rolling: true,
    genid: (req) => `${req?.session?.userId || 0}-${crypto.randomUUID()}`
}))

But I always get 0 for the userId. Are you sure we can get the session content in the genid function? Have you tried it out yourself?

@florianwalther-private
Copy link

florianwalther-private commented May 14, 2022

@revington Nevermind, I got it to work. I misunderstood your "copy the old session data to the new session". I think my line req.session = sessionData; just overwrote the session (and hence the userId was gone again).

This is my login code now:

const match = await bcrypt.compare(password, user.password);
if (match) {
    req.session.userId = user._id;

    req.session.regenerate(error => {
        if (error) {
            res.status(500).json({ error: error });
        } else {
            res.status(200).json(user);
        }
    });
} else {
    res.sendStatus(401);
}

If I don't have anything but the userId in the session, I don't need to copy any old session data to the new session, right?

@revington
Copy link

revington commented May 14, 2022 via email

@florianwalther-private
Copy link

I'm using Express-Session. But I just noticed that the user is actually logged out after page refresh. The Redis entry with the correct userId is in the database, as well as the connect.id cookie. But they seem to not be connected anymore after calling regenerate. If I remove regenerate the user stays logged in, but of course the session prefix is 0. Any idea why that could be?

@florianwalther-private
Copy link

florianwalther-private commented May 14, 2022

Ok I got it now. I have to put another req.session.userId = user._id after the regenerate call. So it looks like this:

req.session.userId = user._id;
await regenerateSession(req.session);
req.session.userId = user._id;

Is this how you have it too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants