From: Dessalines Date: Fri, 19 Mar 2021 04:31:49 +0000 (-0400) Subject: Merge branch 'Mart-Bogdan-1462-jwt-revocation-on-pwd-change' into jwt_revocation_dess X-Git-Url: http://these/git/%7B%60/feeds/u/static/%7Bpost.ap_id%7D?a=commitdiff_plain;h=05b485b6786e4946d6773e0ef59a94266b342a74;p=lemmy.git Merge branch 'Mart-Bogdan-1462-jwt-revocation-on-pwd-change' into jwt_revocation_dess --- 05b485b6786e4946d6773e0ef59a94266b342a74 diff --cc crates/api/src/lib.rs index a2e9bfed,3b87b998..4f2251fd --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@@ -100,22 -92,26 +100,38 @@@ pub(crate) async fn get_local_user_view Ok(claims) => claims.claims, Err(_e) => return Err(ApiError::err("not_logged_in").into()), }; - let local_user_id = LocalUserId(claims.local_user_id); - let user_id = claims.id; - let user = blocking(pool, move |conn| User_::read(conn, user_id)).await??; ++ let local_user_id = LocalUserId(claims.sub); + let local_user_view = + blocking(pool, move |conn| LocalUserView::read(conn, local_user_id)).await??; // Check for a site ban - if user.banned { + if local_user_view.person.banned { return Err(ApiError::err("site_ban").into()); } - // if user's token was issued before user's password reset. - let user_validation_time = user.validator_time.timestamp_millis() / 1000; ++ ++ check_validator_time(&local_user_view.local_user.validator_time, &claims)?; ++ + Ok(local_user_view) +} + ++/// Checks if user's token was issued before user's password reset. ++pub(crate) fn check_validator_time( ++ validator_time: &chrono::NaiveDateTime, ++ claims: &Claims, ++) -> Result<(), LemmyError> { ++ let user_validation_time = validator_time.timestamp_millis() / 1000; + if user_validation_time > claims.iat { - return Err(ApiError::err("not_logged_in").into()); ++ Err(ApiError::err("not_logged_in").into()) ++ } else { ++ Ok(()) + } - Ok(user) + } + -pub(crate) async fn get_user_from_jwt_opt( +pub(crate) async fn get_local_user_view_from_jwt_opt( jwt: &Option, pool: &DbPool, -) -> Result, LemmyError> { +) -> Result, LemmyError> { match jwt { - Some(jwt) => Ok(Some(get_user_from_jwt(jwt, pool).await?)), + Some(jwt) => Ok(Some(get_local_user_view_from_jwt(jwt, pool).await?)), None => Ok(None), } } @@@ -128,26 -124,26 +144,29 @@@ pub(crate) async fn get_local_user_sett Ok(claims) => claims.claims, Err(_e) => return Err(ApiError::err("not_logged_in").into()), }; - let local_user_id = LocalUserId(claims.local_user_id); - let user_id = claims.id; - let user = blocking(pool, move |conn| UserSafeSettings::read(conn, user_id)).await??; ++ let local_user_id = LocalUserId(claims.sub); + let local_user_view = blocking(pool, move |conn| { + LocalUserSettingsView::read(conn, local_user_id) + }) + .await??; // Check for a site ban - if user.banned { + if local_user_view.person.banned { return Err(ApiError::err("site_ban").into()); } - // if user's token was issued before user's password reset. - let user_validation_time = user.validator_time.timestamp_millis() / 1000; - if user_validation_time > claims.iat { - return Err(ApiError::err("not_logged_in").into()); - } - Ok(user) ++ ++ check_validator_time(&local_user_view.local_user.validator_time, &claims)?; ++ + Ok(local_user_view) } -pub(crate) async fn get_user_safe_settings_from_jwt_opt( +pub(crate) async fn get_local_user_settings_view_from_jwt_opt( jwt: &Option, pool: &DbPool, -) -> Result, LemmyError> { +) -> Result, LemmyError> { match jwt { - Some(jwt) => Ok(Some(get_user_safe_settings_from_jwt(jwt, pool).await?)), + Some(jwt) => Ok(Some( + get_local_user_settings_view_from_jwt(jwt, pool).await?, + )), None => Ok(None), } } @@@ -491,7 -490,90 +514,70 @@@ pub(crate) fn password_length_check(pas #[cfg(test)] mod tests { - use crate::captcha_espeak_wav_base64; - use crate::{captcha_espeak_wav_base64, get_user_from_jwt, get_user_safe_settings_from_jwt}; - use lemmy_db_queries::{ - establish_pooled_connection, - source::user::User, - Crud, - ListingType, - SortType, ++ use crate::{captcha_espeak_wav_base64, check_validator_time}; ++ use lemmy_db_queries::{establish_unpooled_connection, source::local_user::LocalUser_, Crud}; ++ use lemmy_db_schema::source::{ ++ local_user::{LocalUser, LocalUserForm}, ++ person::{Person, PersonForm}, + }; - use lemmy_db_schema::source::user::{UserForm, User_}; + use lemmy_utils::claims::Claims; - use std::{ - env::{current_dir, set_current_dir}, - path::PathBuf, - }; - - #[actix_rt::test] - async fn test_should_not_validate_user_token_after_password_change() { - struct CwdGuard(PathBuf); - impl Drop for CwdGuard { - fn drop(&mut self) { - let _ = set_current_dir(&self.0); - } - } - - let _dir_bkp = CwdGuard(current_dir().unwrap()); + - // so configs could be read - let _ = set_current_dir("../.."); - - let conn = establish_pooled_connection(); ++ #[test] ++ fn test_should_not_validate_user_token_after_password_change() { ++ let conn = establish_unpooled_connection(); + - let new_user = UserForm { - name: "user_df342sgf".into(), ++ let new_person = PersonForm { ++ name: "Gerry9812".into(), + preferred_username: None, - password_encrypted: "nope".into(), - email: None, - matrix_user_id: None, + avatar: None, + banner: None, - admin: false, - banned: Some(false), ++ banned: None, ++ deleted: None, + published: None, + updated: None, - show_nsfw: false, - theme: "browser".into(), - default_sort_type: SortType::Hot as i16, - default_listing_type: ListingType::Subscribed as i16, - lang: "browser".into(), - show_avatars: true, - send_notifications_to_email: false, + actor_id: None, + bio: None, - local: true, ++ local: None, + private_key: None, + public_key: None, + last_refreshed_at: None, + inbox_url: None, + shared_inbox_url: None, + }; + - let inserted_user: User_ = User_::create(&conn.get().unwrap(), &new_user).unwrap(); - - let jwt_token = Claims::jwt(inserted_user.id, String::from("my-host.com")).unwrap(); - - get_user_from_jwt(&jwt_token, &conn) - .await - .expect("User should be decoded"); ++ let inserted_person = Person::create(&conn, &new_person).unwrap(); + - get_user_safe_settings_from_jwt(&jwt_token, &conn) - .await - .expect("User should be decoded"); ++ let local_user_form = LocalUserForm { ++ person_id: inserted_person.id, ++ email: None, ++ matrix_user_id: None, ++ password_encrypted: "123456".to_string(), ++ admin: None, ++ show_nsfw: None, ++ theme: None, ++ default_sort_type: None, ++ default_listing_type: None, ++ lang: None, ++ show_avatars: None, ++ send_notifications_to_email: None, ++ }; + - std::thread::sleep(std::time::Duration::from_secs(1)); ++ let inserted_local_user = LocalUser::create(&conn, &local_user_form).unwrap(); + - User_::update_password(&conn.get().unwrap(), inserted_user.id, &"password111").unwrap(); ++ let jwt = Claims::jwt(inserted_local_user.id.0).unwrap(); ++ let claims = Claims::decode(&jwt).unwrap().claims; ++ let check = check_validator_time(&inserted_local_user.validator_time, &claims); ++ assert!(check.is_ok()); + - get_user_from_jwt(&jwt_token, &conn) - .await - .expect_err("JWT decode should fail after password change"); ++ // The check should fail, since the validator time is now newer than the jwt issue time ++ let updated_local_user = ++ LocalUser::update_password(&conn, inserted_local_user.id, &"password111").unwrap(); ++ let check_after = check_validator_time(&updated_local_user.validator_time, &claims); ++ assert!(check_after.is_err()); + - get_user_safe_settings_from_jwt(&jwt_token, &conn) - .await - .expect_err("JWT decode should fail after password change"); ++ let num_deleted = Person::delete(&conn, inserted_person.id).unwrap(); ++ assert_eq!(1, num_deleted); + } #[test] fn test_espeak() { diff --cc crates/api/src/local_user.rs index e9daa819,93ffdfff..266d28ee --- a/crates/api/src/local_user.rs +++ b/crates/api/src/local_user.rs @@@ -130,7 -122,7 +130,7 @@@ impl Perform for Login // Return the jwt Ok(LoginResponse { - jwt: Claims::jwt(local_user_view.local_user.id.0, Settings::get().hostname())?, - jwt: Claims::jwt(user.id, Settings::get().hostname())?, ++ jwt: Claims::jwt(local_user_view.local_user.id.0)?, }) } } @@@ -336,7 -300,7 +336,7 @@@ impl Perform for Register // Return the jwt Ok(LoginResponse { - jwt: Claims::jwt(inserted_local_user.id.0, Settings::get().hostname())?, - jwt: Claims::jwt(inserted_user.id, Settings::get().hostname())?, ++ jwt: Claims::jwt(inserted_local_user.id.0)?, }) } } @@@ -526,7 -471,7 +526,7 @@@ impl Perform for SaveUserSettings // Return the jwt Ok(LoginResponse { - jwt: Claims::jwt(updated_local_user.id.0, Settings::get().hostname())?, - jwt: Claims::jwt(updated_user.id, Settings::get().hostname())?, ++ jwt: Claims::jwt(updated_local_user.id.0)?, }) } } @@@ -1078,7 -1009,7 +1078,7 @@@ impl Perform for PasswordChange // Return the jwt Ok(LoginResponse { - jwt: Claims::jwt(updated_local_user.id.0, Settings::get().hostname())?, - jwt: Claims::jwt(updated_user.id, Settings::get().hostname())?, ++ jwt: Claims::jwt(updated_local_user.id.0)?, }) } } diff --cc crates/db_queries/src/source/local_user.rs index cd93d3fb,00000000..eabd067d mode 100644,000000..100644 --- a/crates/db_queries/src/source/local_user.rs +++ b/crates/db_queries/src/source/local_user.rs @@@ -1,127 -1,0 +1,119 @@@ +use crate::Crud; +use bcrypt::{hash, DEFAULT_COST}; +use diesel::{dsl::*, result::Error, *}; +use lemmy_db_schema::{ ++ naive_now, + schema::local_user::dsl::*, + source::local_user::{LocalUser, LocalUserForm}, + LocalUserId, + PersonId, +}; + - mod safe_type { - use crate::ToSafe; - use lemmy_db_schema::{schema::local_user::columns::*, source::local_user::LocalUser}; - - type Columns = (id, person_id, admin, matrix_user_id); - - impl ToSafe for LocalUser { - type SafeColumns = Columns; - fn safe_columns_tuple() -> Self::SafeColumns { - (id, person_id, admin, matrix_user_id) - } - } - } - +mod safe_settings_type { + use crate::ToSafeSettings; + use lemmy_db_schema::{schema::local_user::columns::*, source::local_user::LocalUser}; + + type Columns = ( + id, + person_id, + email, + admin, + show_nsfw, + theme, + default_sort_type, + default_listing_type, + lang, + show_avatars, + send_notifications_to_email, + matrix_user_id, ++ validator_time, + ); + + impl ToSafeSettings for LocalUser { + type SafeSettingsColumns = Columns; + + /// Includes everything but the hashed password + fn safe_settings_columns_tuple() -> Self::SafeSettingsColumns { + ( + id, + person_id, + email, + admin, + show_nsfw, + theme, + default_sort_type, + default_listing_type, + lang, + show_avatars, + send_notifications_to_email, + matrix_user_id, ++ validator_time, + ) + } + } +} + +pub trait LocalUser_ { + fn register(conn: &PgConnection, form: &LocalUserForm) -> Result; + fn update_password( + conn: &PgConnection, + local_user_id: LocalUserId, + new_password: &str, + ) -> Result; + fn add_admin(conn: &PgConnection, person_id: PersonId, added: bool) -> Result; +} + +impl LocalUser_ for LocalUser { + fn register(conn: &PgConnection, form: &LocalUserForm) -> Result { + let mut edited_user = form.clone(); + let password_hash = + hash(&form.password_encrypted, DEFAULT_COST).expect("Couldn't hash password"); + edited_user.password_encrypted = password_hash; + + Self::create(&conn, &edited_user) + } + + fn update_password( + conn: &PgConnection, + local_user_id: LocalUserId, + new_password: &str, + ) -> Result { + let password_hash = hash(new_password, DEFAULT_COST).expect("Couldn't hash password"); + + diesel::update(local_user.find(local_user_id)) - .set((password_encrypted.eq(password_hash),)) ++ .set(( ++ password_encrypted.eq(password_hash), ++ validator_time.eq(naive_now()), ++ )) + .get_result::(conn) + } + + fn add_admin(conn: &PgConnection, for_person_id: PersonId, added: bool) -> Result { + diesel::update(local_user.filter(person_id.eq(for_person_id))) + .set(admin.eq(added)) + .get_result::(conn) + } +} + +impl Crud for LocalUser { + fn read(conn: &PgConnection, local_user_id: LocalUserId) -> Result { + local_user.find(local_user_id).first::(conn) + } + fn delete(conn: &PgConnection, local_user_id: LocalUserId) -> Result { + diesel::delete(local_user.find(local_user_id)).execute(conn) + } + fn create(conn: &PgConnection, form: &LocalUserForm) -> Result { + insert_into(local_user) + .values(form) + .get_result::(conn) + } + fn update( + conn: &PgConnection, + local_user_id: LocalUserId, + form: &LocalUserForm, + ) -> Result { + diesel::update(local_user.find(local_user_id)) + .set(form) + .get_result::(conn) + } +} diff --cc crates/db_schema/src/schema.rs index c5bf7d2f,665e5e68..9bb3fe2d --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@@ -140,24 -140,6 +140,25 @@@ table! } } +table! { + local_user (id) { + id -> Int4, + person_id -> Int4, + password_encrypted -> Text, + email -> Nullable, + admin -> Bool, + show_nsfw -> Bool, + theme -> Varchar, + default_sort_type -> Int2, + default_listing_type -> Int2, + lang -> Varchar, + show_avatars -> Bool, + send_notifications_to_email -> Bool, + matrix_user_id -> Nullable, ++ validator_time -> Timestamp, + } +} + table! { mod_add (id) { id -> Int4, diff --cc crates/db_schema/src/source/local_user.rs index 750a2255,00000000..11dac6c9 mode 100644,000000..100644 --- a/crates/db_schema/src/source/local_user.rs +++ b/crates/db_schema/src/source/local_user.rs @@@ -1,56 -1,0 +1,58 @@@ +use crate::{schema::local_user, LocalUserId, PersonId}; +use serde::Serialize; + +#[derive(Clone, Queryable, Identifiable, PartialEq, Debug, Serialize)] +#[table_name = "local_user"] +pub struct LocalUser { + pub id: LocalUserId, + pub person_id: PersonId, + pub password_encrypted: String, + pub email: Option, + pub admin: bool, + pub show_nsfw: bool, + pub theme: String, + pub default_sort_type: i16, + pub default_listing_type: i16, + pub lang: String, + pub show_avatars: bool, + pub send_notifications_to_email: bool, + pub matrix_user_id: Option, ++ pub validator_time: chrono::NaiveDateTime, +} + +// TODO redo these, check table defaults +#[derive(Insertable, AsChangeset, Clone)] +#[table_name = "local_user"] +pub struct LocalUserForm { + pub person_id: PersonId, + pub password_encrypted: String, + pub email: Option>, + pub admin: Option, + pub show_nsfw: Option, + pub theme: Option, + pub default_sort_type: Option, + pub default_listing_type: Option, + pub lang: Option, + pub show_avatars: Option, + pub send_notifications_to_email: Option, + pub matrix_user_id: Option>, +} + +/// A local user view that removes password encrypted +#[derive(Clone, Queryable, Identifiable, PartialEq, Debug, Serialize)] +#[table_name = "local_user"] +pub struct LocalUserSettings { + pub id: LocalUserId, + pub person_id: PersonId, + pub email: Option, + pub admin: bool, + pub show_nsfw: bool, + pub theme: String, + pub default_sort_type: i16, + pub default_listing_type: i16, + pub lang: String, + pub show_avatars: bool, + pub send_notifications_to_email: bool, + pub matrix_user_id: Option, ++ pub validator_time: chrono::NaiveDateTime, +} diff --cc crates/routes/src/feeds.rs index 47aca46f,105f9662..6fc370ed --- a/crates/routes/src/feeds.rs +++ b/crates/routes/src/feeds.rs @@@ -227,8 -224,7 +227,8 @@@ fn get_feed_front jwt: String, ) -> Result { let site_view = SiteView::read(&conn)?; - let local_user_id = LocalUserId(Claims::decode(&jwt)?.claims.local_user_id); - let user_id = Claims::decode(&jwt)?.claims.id; ++ let local_user_id = LocalUserId(Claims::decode(&jwt)?.claims.sub); + let person_id = LocalUser::read(&conn, local_user_id)?.person_id; let posts = PostQueryBuilder::create(&conn) .listing_type(&ListingType::Subscribed) @@@ -254,8 -250,7 +254,8 @@@ fn get_feed_inbox(conn: &PgConnection, jwt: String) -> Result { let site_view = SiteView::read(&conn)?; - let local_user_id = LocalUserId(Claims::decode(&jwt)?.claims.local_user_id); - let user_id = Claims::decode(&jwt)?.claims.id; ++ let local_user_id = LocalUserId(Claims::decode(&jwt)?.claims.sub); + let person_id = LocalUser::read(&conn, local_user_id)?.person_id; let sort = SortType::New; diff --cc crates/utils/src/claims.rs index e84f34f9,e62109d4..fc3c4357 --- a/crates/utils/src/claims.rs +++ b/crates/utils/src/claims.rs @@@ -6,8 -6,14 +6,11 @@@ type Jwt = String #[derive(Debug, Serialize, Deserialize)] pub struct Claims { - pub local_user_id: i32, - /// User id, for backward compatibility with client apps. - /// Claim [sub](Claims::sub) is used in server-side checks. - pub id: i32, - /// User id, standard claim by RFC 7519. ++ /// local_user_id, standard claim by RFC 7519. + pub sub: i32, pub iss: String, + /// Time when this token was issued as UNIX-timestamp in seconds + pub iat: i64, } impl Claims { @@@ -23,10 -29,12 +26,11 @@@ ) } - pub fn jwt(local_user_id: i32, hostname: String) -> Result { - pub fn jwt(user_id: i32, hostname: String) -> Result { ++ pub fn jwt(local_user_id: i32) -> Result { let my_claims = Claims { - local_user_id, - id: user_id, - sub: user_id, -- iss: hostname, ++ sub: local_user_id, ++ iss: Settings::get().hostname(), + iat: chrono::Utc::now().timestamp_millis() / 1000, }; encode( &Header::default(), diff --cc crates/utils/src/settings/mod.rs index 3911a18f,3911a18f..d64884bf --- a/crates/utils/src/settings/mod.rs +++ b/crates/utils/src/settings/mod.rs @@@ -24,7 -24,7 +24,7 @@@ static CONFIG_FILE: &str = "config/conf lazy_static! { static ref SETTINGS: RwLock = RwLock::new(match Settings::init() { Ok(c) => c, -- Err(e) => panic!("{}", e), ++ Err(_) => Settings::default(), }); } diff --cc migrations/2021-03-19-014144_add_col_local_user_validator_time/down.sql index 00000000,00000000..657bfb3d new file mode 100644 --- /dev/null +++ b/migrations/2021-03-19-014144_add_col_local_user_validator_time/down.sql @@@ -1,0 -1,0 +1,1 @@@ ++alter table local_user drop column validator_time; diff --cc migrations/2021-03-19-014144_add_col_local_user_validator_time/up.sql index 00000000,00000000..4fbb5eb9 new file mode 100644 --- /dev/null +++ b/migrations/2021-03-19-014144_add_col_local_user_validator_time/up.sql @@@ -1,0 -1,0 +1,1 @@@ ++alter table local_user add column validator_time timestamp not null default now();