-
-
Notifications
You must be signed in to change notification settings - Fork 890
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 db table for login tokens which allows for invalidation #3818
Changes from 1 commit
8da32fb
20e38c7
71f0a59
8bfc202
ea42469
37c2803
246ea92
989bae9
4381bb8
53a1177
f591ee1
053dff9
3763978
b2bacd1
aa405fa
45570a6
2ea45be
414376f
3d0317e
c60b001
ed85328
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,15 +4,20 @@ use crate::schema::login_token; | |
use chrono::{DateTime, Utc}; | ||
use serde::{Deserialize, Serialize}; | ||
|
||
/// Stores data related to a specific user login session. | ||
#[derive(Clone, PartialEq, Eq, Debug, Serialize, Deserialize)] | ||
#[cfg_attr(feature = "full", derive(Queryable, Identifiable))] | ||
#[cfg_attr(feature = "full", diesel(table_name = login_token))] | ||
pub struct LoginToken { | ||
pub id: i32, | ||
/// Jwt token for this login | ||
#[serde(skip)] | ||
pub token: String, | ||
pub user_id: LocalUserId, | ||
/// Time of login | ||
pub published: DateTime<Utc>, | ||
/// IP address where login was made from, allows invalidating logins by IP address. | ||
/// Could be stored in truncated format, or store derived information for better privacy. | ||
pub ip: Option<String>, | ||
pub user_agent: Option<String>, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. usually i think you would want to add a few other fields here:
this is all info you can (a) use to display a list of known sessions to the user and (b) so you can invalidate a session in some cases (e.g. geoip country of the session changes, user agent changes too much) (c) invalidate other sessions remotely There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we'd want to store IP addresses for privacy reasons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @phiresky Good suggestions, I added them. Though Im doubtful about using invalidated timestamp, it means the table will grow forever and I dont see the benefit of keeping years old login info around. Adding another scheduled task for cleanup also doesnt seem worth the trouble. Its true that storing IPs could cause some problems with privacy legislation. But I think its definitely worth it to fight abuse. We just need to be careful that IPs are not leaked to other users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of IP you could also store derived info ( city+country, ASIN) that's a bit less private and closer to what you'd want to use for session invalidation/display. But that would need an external service for the geolocation which is a pain. or you can store it truncated (e.g. the /24 for ipv4 and the /52 for ipv6) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes geoip seems to complicated and error prone. Truncating would require the ip first. Im going to leave it like this and we can make changes in the future if necessary. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,6 +212,7 @@ pub enum LemmyErrorType { | |
CouldntSendWebmention, | ||
ContradictingFilters, | ||
InstanceBlockAlreadyExists, | ||
#[serde(rename = "`jwt` cookie must be marked secure and httponly")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as the other PR: i don't think using "rename" is the correct way to add messages to this enum. (see #4007 (comment) ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed this to a comment instead. |
||
AuthCookieInsecure, | ||
Unknown(String), | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the cookie name back to jwt, just to make it more backward compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest its up to you and @SleeplessOne1917 which way its easier to handle the cookie. So let me know if you prefer to use the same name (
jwt
, means you need to mark the existing cookie as secure and httponly), or rename it (then you can set a new cookie and dont worry about the old one). Though if you use header for auth it doesnt matter at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't matter to me. Thoughts @SleeplessOne1917 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo the hasJwtCookie and setJwtCookie functions should be removed from lemmy-ui, and with the cookie being named jwt here it shouldn't require any further changes there (I think). lemmy-ui shouldn't need to worry about the cookie at all - lemmy_server now sets it and removes it, and with it being HttpOnly it will also make session exfiltration via XSS impossible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with phiresky on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the breaking changes post says that the cookie is called
auth
. So I renamed it back to that.https://lemmy.ml/post/5711722
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it doesn't matter, lemmy-ui should still be able to remove their cookie logic regardless of what the new cookie is named