From ab947f1f0873adaf90b1dfdca69dd8b00d904346 Mon Sep 17 00:00:00 2001 From: Bogdan Mart Date: Sat, 13 Mar 2021 20:16:35 +0200 Subject: [PATCH 1/4] User token revocation upon password change Added DB column validator_time and chedking that is is less then token's "Issuead at time" Wip on #1462 --- crates/api/src/lib.rs | 95 ++++++++++++++++++- crates/db_queries/src/lib.rs | 27 ++++++ crates/db_queries/src/source/user.rs | 4 + crates/db_schema/src/schema.rs | 1 + crates/db_schema/src/source/user.rs | 2 + crates/utils/src/claims.rs | 8 ++ .../down.sql | 1 + .../up.sql | 1 + 8 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 migrations/2021-02-28-192405_add_col_user_validator_time/down.sql create mode 100644 migrations/2021-02-28-192405_add_col_user_validator_time/up.sql diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs index 5642c4b9c..e1cc44515 100644 --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@ -22,7 +22,7 @@ use lemmy_structs::{blocking, comment::*, community::*, post::*, site::*, user:: use lemmy_utils::{claims::Claims, settings::Settings, ApiError, ConnectionId, LemmyError}; use lemmy_websocket::{serialize_websocket_message, LemmyContext, UserOperation}; use serde::Deserialize; -use std::process::Command; +use std::{env, process::Command}; use url::Url; pub mod comment; @@ -84,6 +84,11 @@ pub(crate) async fn get_user_from_jwt(jwt: &str, pool: &DbPool) -> Result claims.iat { + return Err(ApiError::err("not_logged_in").into()); + } Ok(user) } @@ -111,6 +116,11 @@ pub(crate) async fn get_user_safe_settings_from_jwt( if user.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) } @@ -434,7 +444,11 @@ pub(crate) fn captcha_espeak_wav_base64(captcha: &str) -> Result Result { // Make a temp file path let uuid = uuid::Uuid::new_v4().to_string(); - let file_path = format!("/tmp/lemmy_espeak_{}.wav", &uuid); + let file_path = format!( + "{}/lemmy_espeak_{}.wav", + env::temp_dir().to_string_lossy(), + &uuid + ); // Write the wav file Command::new("espeak") @@ -457,7 +471,82 @@ pub(crate) fn espeak_wav_base64(text: &str) -> Result { #[cfg(test)] mod tests { - use crate::captcha_espeak_wav_base64; + use crate::{captcha_espeak_wav_base64, get_user_from_jwt}; + use lemmy_db_queries::{ + establish_pooled_connection, + source::user::User, + Crud, + ListingType, + SortType, + }; + 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(); + + let new_user = UserForm { + name: "user_df342sgf".into(), + preferred_username: None, + password_encrypted: "nope".into(), + email: None, + matrix_user_id: None, + avatar: None, + banner: None, + admin: false, + banned: Some(false), + 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, + 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"); + + std::thread::sleep(std::time::Duration::from_secs(1)); + + User_::update_password(&conn.get().unwrap(), inserted_user.id, &"password111").unwrap(); + + let jwt_decode_res = get_user_from_jwt(&jwt_token, &conn).await; + + jwt_decode_res.expect_err("JWT decode should fail after password change"); + } #[test] fn test_espeak() { diff --git a/crates/db_queries/src/lib.rs b/crates/db_queries/src/lib.rs index 5667b4262..ad3603c63 100644 --- a/crates/db_queries/src/lib.rs +++ b/crates/db_queries/src/lib.rs @@ -235,6 +235,33 @@ pub fn establish_unpooled_connection() -> PgConnection { conn } +pub fn establish_pooled_connection( +) -> diesel::r2d2::Pool> { + use diesel::r2d2::{ConnectionManager, Pool}; + + // Set up the r2d2 connection pool + let db_url = match get_database_url_from_env() { + Ok(url) => url, + Err(e) => panic!( + "Failed to read database URL from env var LEMMY_DATABASE_URL: {}", + e + ), + }; + + let manager = ConnectionManager::::new(&db_url); + let pool = Pool::builder() + .max_size(1) + .build(manager) + .unwrap_or_else(|_| panic!("Error connecting to {}", db_url)); + + let conn = pool.get().unwrap(); + + // Run the migrations from code + embedded_migrations::run(&conn).unwrap(); + + pool +} + lazy_static! { static ref EMAIL_REGEX: Regex = Regex::new(r"^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$").unwrap(); diff --git a/crates/db_queries/src/source/user.rs b/crates/db_queries/src/source/user.rs index 20b187b46..cb414de06 100644 --- a/crates/db_queries/src/source/user.rs +++ b/crates/db_queries/src/source/user.rs @@ -173,6 +173,7 @@ mod safe_settings_type { last_refreshed_at, banner, deleted, + validator_time, ); impl ToSafeSettings for User_ { @@ -202,6 +203,7 @@ mod safe_settings_type { last_refreshed_at, banner, deleted, + validator_time, ) } } @@ -296,6 +298,7 @@ impl User for User_ { .set(( password_encrypted.eq(password_hash), updated.eq(naive_now()), + validator_time.eq(naive_now()), )) .get_result::(conn) } @@ -446,6 +449,7 @@ mod tests { deleted: false, inbox_url: inserted_user.inbox_url.to_owned(), shared_inbox_url: None, + validator_time: inserted_user.published, }; let read_user = User_::read(&conn, inserted_user.id).unwrap(); diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs index 3786e00ca..665e5e685 100644 --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@ -408,6 +408,7 @@ table! { deleted -> Bool, inbox_url -> Text, shared_inbox_url -> Nullable, + validator_time -> Timestamp, } } diff --git a/crates/db_schema/src/source/user.rs b/crates/db_schema/src/source/user.rs index d72929fa8..47c61c4f4 100644 --- a/crates/db_schema/src/source/user.rs +++ b/crates/db_schema/src/source/user.rs @@ -35,6 +35,7 @@ pub struct User_ { pub deleted: bool, pub inbox_url: Url, pub shared_inbox_url: Option, + pub validator_time: chrono::NaiveDateTime, } /// A safe representation of user, without the sensitive info @@ -86,6 +87,7 @@ pub struct UserSafeSettings { pub last_refreshed_at: chrono::NaiveDateTime, pub banner: Option, pub deleted: bool, + pub validator_time: chrono::NaiveDateTime, } #[derive(Clone, Queryable, Identifiable, PartialEq, Debug, Serialize)] diff --git a/crates/utils/src/claims.rs b/crates/utils/src/claims.rs index dff79d859..5ca77e0bd 100644 --- a/crates/utils/src/claims.rs +++ b/crates/utils/src/claims.rs @@ -6,8 +6,14 @@ type Jwt = String; #[derive(Debug, Serialize, Deserialize)] pub struct Claims { + /// 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. + pub sub: i32, pub iss: String, + /// Time when this token was issued as UNIX-timestamp in seconds + pub iat: i64, } impl Claims { @@ -26,7 +32,9 @@ impl Claims { pub fn jwt(user_id: i32, hostname: String) -> Result { let my_claims = Claims { id: user_id, + sub: user_id, iss: hostname, + iat: chrono::Utc::now().timestamp_millis() / 1000, }; encode( &Header::default(), diff --git a/migrations/2021-02-28-192405_add_col_user_validator_time/down.sql b/migrations/2021-02-28-192405_add_col_user_validator_time/down.sql new file mode 100644 index 000000000..717b80878 --- /dev/null +++ b/migrations/2021-02-28-192405_add_col_user_validator_time/down.sql @@ -0,0 +1 @@ +ALTER TABLE user_ DROP COLUMN validator_time; \ No newline at end of file diff --git a/migrations/2021-02-28-192405_add_col_user_validator_time/up.sql b/migrations/2021-02-28-192405_add_col_user_validator_time/up.sql new file mode 100644 index 000000000..fcba2311e --- /dev/null +++ b/migrations/2021-02-28-192405_add_col_user_validator_time/up.sql @@ -0,0 +1 @@ +ALTER TABLE user_ ADD COLUMN validator_time timestamp not null default now(); \ No newline at end of file From 4426c3176d6033c0c83be6876a694f7a6a31577d Mon Sep 17 00:00:00 2001 From: Bogdan Mart Date: Sat, 13 Mar 2021 22:18:26 +0200 Subject: [PATCH 2/4] fix timestamp condition #1462 --- crates/api/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs index 2dbfb4354..0f77da5e8 100644 --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@ -132,7 +132,7 @@ pub(crate) async fn get_user_safe_settings_from_jwt( } // 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 { + if user_validation_time > claims.iat { return Err(ApiError::err("not_logged_in").into()); } Ok(user) From 74272ed754d1bf7f64c41c91076e95f1348c3abd Mon Sep 17 00:00:00 2001 From: Bogdan Mart Date: Sat, 13 Mar 2021 22:36:40 +0200 Subject: [PATCH 3/4] more correct tests --- crates/api/src/lib.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs index 0f77da5e8..3b87b9981 100644 --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@ -490,7 +490,7 @@ pub(crate) fn password_length_check(pass: &str) -> Result<(), LemmyError> { #[cfg(test)] mod tests { - use crate::{captcha_espeak_wav_base64, get_user_from_jwt}; + 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, @@ -558,13 +558,21 @@ mod tests { .await .expect("User should be decoded"); + get_user_safe_settings_from_jwt(&jwt_token, &conn) + .await + .expect("User should be decoded"); + std::thread::sleep(std::time::Duration::from_secs(1)); User_::update_password(&conn.get().unwrap(), inserted_user.id, &"password111").unwrap(); - let jwt_decode_res = get_user_from_jwt(&jwt_token, &conn).await; + get_user_from_jwt(&jwt_token, &conn) + .await + .expect_err("JWT decode should fail after password change"); - jwt_decode_res.expect_err("JWT decode should fail after password change"); + get_user_safe_settings_from_jwt(&jwt_token, &conn) + .await + .expect_err("JWT decode should fail after password change"); } #[test] From 493598c1ba18077086b58770d96aa36a619ca0be Mon Sep 17 00:00:00 2001 From: Dessalines Date: Fri, 19 Mar 2021 10:02:58 -0400 Subject: [PATCH 4/4] A few suggestion fixes. --- crates/api/src/lib.rs | 2 +- crates/utils/src/claims.rs | 3 ++- crates/utils/src/settings/mod.rs | 9 ++++++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs index 4f2251fde..529a13cfd 100644 --- a/crates/api/src/lib.rs +++ b/crates/api/src/lib.rs @@ -118,7 +118,7 @@ pub(crate) fn check_validator_time( validator_time: &chrono::NaiveDateTime, claims: &Claims, ) -> Result<(), LemmyError> { - let user_validation_time = validator_time.timestamp_millis() / 1000; + let user_validation_time = validator_time.timestamp(); if user_validation_time > claims.iat { Err(ApiError::err("not_logged_in").into()) } else { diff --git a/crates/utils/src/claims.rs b/crates/utils/src/claims.rs index fc3c43579..3a444b9a4 100644 --- a/crates/utils/src/claims.rs +++ b/crates/utils/src/claims.rs @@ -1,4 +1,5 @@ use crate::settings::structs::Settings; +use chrono::Utc; use jsonwebtoken::{decode, encode, DecodingKey, EncodingKey, Header, TokenData, Validation}; use serde::{Deserialize, Serialize}; @@ -30,7 +31,7 @@ impl Claims { let my_claims = Claims { sub: local_user_id, iss: Settings::get().hostname(), - iat: chrono::Utc::now().timestamp_millis() / 1000, + iat: Utc::now().timestamp(), }; encode( &Header::default(), diff --git a/crates/utils/src/settings/mod.rs b/crates/utils/src/settings/mod.rs index d64884bfc..0990a43de 100644 --- a/crates/utils/src/settings/mod.rs +++ b/crates/utils/src/settings/mod.rs @@ -13,6 +13,7 @@ use crate::{ }; use anyhow::{anyhow, Context}; use deser_hjson::from_str; +use log::warn; use merge::Merge; use std::{env, fs, io::Error, net::IpAddr, sync::RwLock}; @@ -24,7 +25,13 @@ static CONFIG_FILE: &str = "config/config.hjson"; lazy_static! { static ref SETTINGS: RwLock = RwLock::new(match Settings::init() { Ok(c) => c, - Err(_) => Settings::default(), + Err(e) => { + warn!( + "Couldn't load settings file, using default settings.\n{}", + e + ); + Settings::default() + } }); }