-
Notifications
You must be signed in to change notification settings - Fork 109
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
add user_id field in session table #72
Comments
I don't like the idea of adding a
Hope that helps! |
@chill117 What if the "user_id" property was optional and such a column was only created and set by express-mysql-session if it was defined in the columnNames dict? |
@mo Maybe, but only if the other solutions are not workable. Even if a user of this module wanted to set a |
@mo How did this one end up? |
If express-mysql-session creates the table and the optional columnNames.user_id is defined, then it should create a column field without a foreign key. This would make it possible for me to pre-create the session table WITH the foreign key constraint, and then I would pass "columnNames.user_id" to express-mysql-session ensuring that all rows will always have a correct value in the user_id field. This way, express-mysql-session doesn't have to know about the user table at all. Does this sound OK for a PR @chill117 ? For now, I'm doing a second query that writes the user_id field and I have set my foreign key constraint to be in effect only when the value is NOT NULL. However, I really don't want to allow sessions with a NULL value so I would be more much comfortable if my database schema enforced this. |
Please tell me see if I understand correctly. When a new option flag is
At least one issue I see with this:
Instead of making this a one-off solution for user IDs, let's try a more general solution. A new option that defines keys in the session var sessionStore = new MySQLStore({
dataWithOwnColumns: [ 'user_id', 'some_other_field' ]
}); Where the database table looks like this: CREATE TABLE IF NOT EXISTS `sessions` (
`session_id` varchar(128) COLLATE utf8mb4_bin NOT NULL,
`expires` int(11) unsigned NOT NULL,
`data` text COLLATE utf8mb4_bin,
`user_id` int(11) unsigned,
`some_other_field` varchar(255),
PRIMARY KEY (`session_id`)
) ENGINE=InnoDB The module user (developer) would be responsible for creating their own sessions table with the correct columns. Then whenever |
Is there still interest in this issue? |
I'm still interested in this feature. The field that passport puts into "data" for me is called "user" but the database column I have is called "user_id". Ideally, I think it's good if the session storage does not mandate certain column names so it would be nice to find a solution that ensures this, even for "dataWithOwnColumns" columns. Did you mean that dataWithOwnColumns could be combined with columns, like this:
I suppose another option would be to simply omit the "dataWithOwnColumns" member, and instead make a rule that says: If there is any property KEY:VAL inside columnNames dict where KEY is not "session_id", "expires" or "data", then the session store will create a column called VAL and assume that the data for it is in data[KEY]
|
Hey guys, this function is still needed. I would like to write to this new column user_uuid value (binary16). |
It's still not clear to me how to proceed with this one. I don't want to introduce foot-guns with a feature that most users of this module won't need. But if you have some suggestion for how to implement this, I am willing to listen. |
This solution appears to work fine @demjanich |
I'm not sure I completely understand what @mo is suggesting here, but wouldn't an implementation of #91 fix the inefficiency problem? |
The things I would like are:
The two last points are important if you want your db schema to ensure that the data is consistent. My proposed solution is; if express-mysql-session gets:
...then it will insert the user id into a column called "user_id". This would make it possible to pre-create the session table with the foreign key / not null. |
My first workaround attempt was to run "res.end()" so that express-mysql-session creates the session in the db. Then I used an UPDATE query to fill in the user_id column. One drawback of this was that user_id could not be NOT NULL and after running this in prod I had a few junk sessions lying around with user_id=NULL. When I investigated why, I noticed that about 1 times out of 50 the express-mysql-session INSERT query would be done after the UPDATE query due to a race condition between the two. To reproduce this bug I had to run my E2E testsuite hundreds of times consecutively. This is why db schemas that enforce consistency are useful. A slightly better solution (still hacky but it works) is to do something like: await db.query(
'INSERT INTO your_session_table(session_id, expires, user_id) VALUES (?, ?, ?) ON DUPLICATE KEY UPDATE user_id = VALUES(user_id)',
[req.session.id, sessionExpiresSeconds, user.user_id]) ...because then it doesn't matter if the INSERT or the UPDATE happens first. However, the best solution would be to fix express-mysql-session so that it allows users to have a strong consistent db schema. |
I agree with HoldYourWaffle, this could be partially solved along with #91 by implementing an opt-in JSON data type for the But has fast reads, no JSON lookups.
And for cascading deletes you could workaround with a trigger |
I've recently rolled out a project that uses express with a postgreSQL database and a compatible express session store. I wanted to achieve the same result as @mo as discussed in this issue - i.e. to be able to clear all sessions for a single user. I was able to do this by using a reference table (user sessions) which has two columns:
I insert a new row into this reference table whenever a user authenticates a new session. Whenever it is needed to logout the user from all their active sessions, a single SQL query can get all session IDs from the reference table and delete all matching sessions in the sessions table. This requires no changes to either the users table nor the sessions table. |
For the record, this is functionality I was looking for as well and had to hand roll a solution. I disagree this wouldn't be useful for most users. To be clear, I'm not saying we would have to set foreign keys, but the data is all already there in the session info. Just tap into that And of course, we could make this an optional field/functionality as well if people really didn't want it. |
So, is there now a way to define new columns with their own data? Since in the README under Custom database table schema is stated:
If so, how does it work? Can we do something like: const storeOptions = {
host: ...,
port: ...,
...,
createDatabaseTable: false,
schema: {
tableName: 'sessions',
columnNames: {
session_id: 'session_id',
expires: 'expires',
data: 'data',
someCustomData: 'custom_data'
}
}
} |
When a user does a password reset / changes his password, it makes sense to drop all sessions for that user. Currently the user id is stored inside a json blob on the session, so it becomes very inefficient to query for all sessions that belong to a specific user.
In issue #53 ( #53 ), it was suggested that one way to workaround this issue is to, A) not use createDatabaseTable: true, B) add a user_id column, and C) right after login, update the session with the appropriate user_id.
However, this approach has the downside that "user_id" cannot be a "NOT NULL" column with a foreign key constraint pointing to the user table. This in turn would enable cascading deletes for sessions when a user is deleted.
Do you agree that having such a column is a good idea, i.e. would you accept a PR to add one?
The text was updated successfully, but these errors were encountered: