From: Dessalines <tyhou13@gmx.com>
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/%22https:/image.com/%22%7B%7D/static/%7Bicon?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<String>,
    pool: &DbPool,
 -) -> Result<Option<User_>, LemmyError> {
 +) -> Result<Option<LocalUserView>, 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<String>,
    pool: &DbPool,
 -) -> Result<Option<UserSafeSettings>, LemmyError> {
 +) -> Result<Option<LocalUserSettingsView>, 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<LocalUser, Error>;
 +  fn update_password(
 +    conn: &PgConnection,
 +    local_user_id: LocalUserId,
 +    new_password: &str,
 +  ) -> Result<LocalUser, Error>;
 +  fn add_admin(conn: &PgConnection, person_id: PersonId, added: bool) -> Result<LocalUser, Error>;
 +}
 +
 +impl LocalUser_ for LocalUser {
 +  fn register(conn: &PgConnection, form: &LocalUserForm) -> Result<Self, Error> {
 +    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<Self, Error> {
 +    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::<Self>(conn)
 +  }
 +
 +  fn add_admin(conn: &PgConnection, for_person_id: PersonId, added: bool) -> Result<Self, Error> {
 +    diesel::update(local_user.filter(person_id.eq(for_person_id)))
 +      .set(admin.eq(added))
 +      .get_result::<Self>(conn)
 +  }
 +}
 +
 +impl Crud<LocalUserForm, LocalUserId> for LocalUser {
 +  fn read(conn: &PgConnection, local_user_id: LocalUserId) -> Result<Self, Error> {
 +    local_user.find(local_user_id).first::<Self>(conn)
 +  }
 +  fn delete(conn: &PgConnection, local_user_id: LocalUserId) -> Result<usize, Error> {
 +    diesel::delete(local_user.find(local_user_id)).execute(conn)
 +  }
 +  fn create(conn: &PgConnection, form: &LocalUserForm) -> Result<Self, Error> {
 +    insert_into(local_user)
 +      .values(form)
 +      .get_result::<Self>(conn)
 +  }
 +  fn update(
 +    conn: &PgConnection,
 +    local_user_id: LocalUserId,
 +    form: &LocalUserForm,
 +  ) -> Result<Self, Error> {
 +    diesel::update(local_user.find(local_user_id))
 +      .set(form)
 +      .get_result::<Self>(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<Text>,
 +        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<Text>,
++        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<String>,
 +  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<String>,
++  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<Option<String>>,
 +  pub admin: Option<bool>,
 +  pub show_nsfw: Option<bool>,
 +  pub theme: Option<String>,
 +  pub default_sort_type: Option<i16>,
 +  pub default_listing_type: Option<i16>,
 +  pub lang: Option<String>,
 +  pub show_avatars: Option<bool>,
 +  pub send_notifications_to_email: Option<bool>,
 +  pub matrix_user_id: Option<Option<String>>,
 +}
 +
 +/// 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<String>,
 +  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<String>,
++  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<ChannelBuilder, LemmyError> {
    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<ChannelBuilder, LemmyError> {
    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<Jwt, jsonwebtoken::errors::Error> {
 -  pub fn jwt(user_id: i32, hostname: String) -> Result<Jwt, jsonwebtoken::errors::Error> {
++  pub fn jwt(local_user_id: i32) -> Result<Jwt, jsonwebtoken::errors::Error> {
      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<Settings> = 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();