From b20c30048ca6496409c6dcc4dfc8f033f94647cc Mon Sep 17 00:00:00 2001 From: Matthieu Bessat Date: Sat, 16 Nov 2024 14:30:01 +0100 Subject: [PATCH] fix: better JWT cookie management --- Cargo.lock | 1 + Cargo.toml | 1 + src/consts.rs | 1 + src/controllers/ui/login.rs | 33 ++++++++++++++++++--------------- src/controllers/ui/logout.rs | 12 +++++++----- src/controllers/ui/me.rs | 1 - src/main.rs | 1 + src/middlewares/renderer.rs | 1 + src/middlewares/user_auth.rs | 18 +++++++----------- src/models/token_claims.rs | 5 +++-- src/renderer.rs | 2 +- src/router.rs | 11 ++++++----- 12 files changed, 47 insertions(+), 40 deletions(-) create mode 100644 src/consts.rs diff --git a/Cargo.lock b/Cargo.lock index 683ea07..f19d1ee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1360,6 +1360,7 @@ dependencies = [ "sqlx", "strum", "strum_macros", + "time", "tokio", "toml", "totp-rs", diff --git a/Cargo.toml b/Cargo.toml index eff7740..41357f6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,6 +57,7 @@ totp-rs = "5.6" minijinja-embed = "2.3.1" axum-macros = "0.4.2" jsonwebtoken = "9.3.0" +time = "0.3.36" [build-dependencies] minijinja-embed = "2.3.1" diff --git a/src/consts.rs b/src/consts.rs new file mode 100644 index 0000000..1d9bd00 --- /dev/null +++ b/src/consts.rs @@ -0,0 +1 @@ +pub const WEB_GUI_JWT_COOKIE_NAME: &str = "minauthator_jwt"; diff --git a/src/controllers/ui/login.rs b/src/controllers/ui/login.rs index 886ce5c..6bdca40 100644 --- a/src/controllers/ui/login.rs +++ b/src/controllers/ui/login.rs @@ -1,12 +1,14 @@ -use chrono::{Duration, SecondsFormat, Utc}; +use axum_extra::extract::{cookie::{Cookie, SameSite}, CookieJar}; +use chrono::{SecondsFormat, Utc}; use log::info; use serde::Deserialize; -use axum::{extract::{Query, State}, http::{HeaderMap, HeaderValue, StatusCode}, response::{Html, IntoResponse}, Extension, Form}; +use axum::{extract::{Query, State}, http::{HeaderMap, HeaderValue, StatusCode}, response::{Html, IntoResponse, Redirect}, Extension, Form}; use fully_pub::fully_pub; use minijinja::context; +use time::Duration; use crate::{ - models::{token_claims::UserTokenClaims, user::{User, UserStatus}}, renderer::TemplateRenderer, server::AppState, services::{password::verify_password_hash, session::create_token} + consts::WEB_GUI_JWT_COOKIE_NAME, models::{token_claims::UserTokenClaims, user::{User, UserStatus}}, renderer::TemplateRenderer, server::AppState, services::{password::verify_password_hash, session::create_token} }; pub async fn login_form( @@ -37,6 +39,7 @@ struct LoginQueryParams { pub async fn perform_login( State(app_state): State, Extension(renderer): Extension, + cookies: CookieJar, Query(query_params): Query, Form(login): Form ) -> impl IntoResponse { @@ -87,24 +90,24 @@ pub async fn perform_login( .execute(&app_state.db) .await.unwrap(); - let jwt = create_token(&app_state.secrets, UserTokenClaims::from_user_id(&user.id)); + let jwt_max_age = Duration::days(15); + let claims = UserTokenClaims::new(&user.id, jwt_max_age); + let jwt = create_token(&app_state.secrets, claims); // TODO: handle keep_session boolean from form and specify cookie max age only if this setting // is true - let cookie_max_age = Duration::days(7).num_seconds(); - // enforce SameSite=Lax to avoid CSRF - let jwt_cookie = format!("minauth_jwt={jwt}; SameSite=Lax; Max-Age={cookie_max_age}"); - let mut headers = HeaderMap::new(); - headers.insert("Set-Cookie", HeaderValue::from_str(&jwt_cookie).unwrap()); + let cookie_max_age = jwt_max_age - Duration::seconds(32); + // enforce SameSite=Lax to avoid CSRF + let jwt_cookie = Cookie::build((WEB_GUI_JWT_COOKIE_NAME, jwt)) + .same_site(SameSite::Lax) + .max_age(cookie_max_age); + // TODO: check redirection for arbitrary URL, enforce relative path - headers.insert("Location", HeaderValue::from_str( - &query_params.redirect_to.unwrap_or("/me".to_string()) - ).unwrap()); + let redirection_target = query_params.redirect_to.unwrap_or("/me".to_string()); ( - StatusCode::SEE_OTHER, - headers, - Html("Logged in. Redirecting you.") + cookies.add(jwt_cookie), + Redirect::to(&redirection_target) ).into_response() } diff --git a/src/controllers/ui/logout.rs b/src/controllers/ui/logout.rs index 149a1de..fd55422 100644 --- a/src/controllers/ui/logout.rs +++ b/src/controllers/ui/logout.rs @@ -1,12 +1,14 @@ -use axum::{http::StatusCode, response::Redirect}; +use axum::response::{IntoResponse, Redirect}; use axum_extra::extract::CookieJar; +use crate::consts::WEB_GUI_JWT_COOKIE_NAME; + pub async fn perform_logout( cookies: CookieJar -) -> Result<(CookieJar, Redirect), StatusCode> { - Ok(( - cookies.remove("minauth_jwt"), +) -> impl IntoResponse { + ( + cookies.remove(WEB_GUI_JWT_COOKIE_NAME), Redirect::to("/") - )) + ) } diff --git a/src/controllers/ui/me.rs b/src/controllers/ui/me.rs index 3b810a9..2a6b8c0 100644 --- a/src/controllers/ui/me.rs +++ b/src/controllers/ui/me.rs @@ -15,7 +15,6 @@ pub async fn me_page( Extension(renderer): Extension, Extension(token_claims): Extension ) -> impl IntoResponse { - let user_res = sqlx::query_as::<_, User>("SELECT * FROM users WHERE id = $1") .bind(&token_claims.sub) .fetch_one(&app_state.db) diff --git a/src/main.rs b/src/main.rs index 4d6aa4e..016a7bd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,6 +8,7 @@ pub mod utils; pub mod services; pub mod middlewares; pub mod renderer; +pub mod consts; use std::{env, fs}; use anyhow::{Result, Context, anyhow}; diff --git a/src/middlewares/renderer.rs b/src/middlewares/renderer.rs index 0e01238..21f00f7 100644 --- a/src/middlewares/renderer.rs +++ b/src/middlewares/renderer.rs @@ -12,6 +12,7 @@ pub async fn renderer_middleware( env: app_state.templating_env, token_claims: token_claims_ext.map(|x| x.0) }; + dbg!(&renderer_instance); req.extensions_mut().insert(renderer_instance); Ok(next.run(req).await) } diff --git a/src/middlewares/user_auth.rs b/src/middlewares/user_auth.rs index c178694..095eecf 100644 --- a/src/middlewares/user_auth.rs +++ b/src/middlewares/user_auth.rs @@ -1,16 +1,13 @@ use axum::{ extract::{OriginalUri, Request, State}, - http::{HeaderMap, HeaderValue, StatusCode}, middleware::Next, - response::{Html, IntoResponse, Redirect, Response}, + response::{IntoResponse, Redirect, Response}, Extension }; use axum_extra::extract::CookieJar; use crate::{ - models::token_claims::UserTokenClaims, - server::AppState, - services::session::verify_token + consts::WEB_GUI_JWT_COOKIE_NAME, models::token_claims::UserTokenClaims, server::AppState, services::session::verify_token }; @@ -22,7 +19,7 @@ pub async fn auth_middleware( mut req: Request, next: Next, ) -> Result { - let jwt = match cookies.get("minauth_jwt") { + let jwt = match cookies.get(WEB_GUI_JWT_COOKIE_NAME) { Some(cookie) => cookie.value(), None => { // no auth found, auth may be optional @@ -33,12 +30,11 @@ pub async fn auth_middleware( Ok(val) => val, Err(_e) => { // UserWebGUI: delete invalid JWT cookie - let mut headers = HeaderMap::new(); - let jwt_cookie = "minauth_jwt=deleted; SameSite=Lax; Max-Age=0".to_string(); - headers.insert("Set-Cookie", HeaderValue::from_str(&jwt_cookie).unwrap()); - headers.insert("Location", HeaderValue::from_str(&original_uri.to_string()).unwrap()); return Err( - (StatusCode::SEE_OTHER, headers, Html("Unauthorized: Invalid JWT cookie.")) + ( + cookies.remove(WEB_GUI_JWT_COOKIE_NAME), + Redirect::to(&original_uri.to_string()) + ) ); } }; diff --git a/src/models/token_claims.rs b/src/models/token_claims.rs index 00cf029..66e20d2 100644 --- a/src/models/token_claims.rs +++ b/src/models/token_claims.rs @@ -1,6 +1,7 @@ use fully_pub::fully_pub; use jsonwebtoken::get_current_timestamp; use serde::{Deserialize, Serialize}; +use time::Duration; use super::authorization::AuthorizationScope; @@ -17,10 +18,10 @@ struct UserTokenClaims { } impl UserTokenClaims { - pub fn from_user_id(user_id: &str) -> Self { + pub fn new(user_id: &str, max_age: Duration) -> Self { UserTokenClaims { sub: user_id.into(), - exp: get_current_timestamp() + 86_000, + exp: get_current_timestamp() + max_age.whole_seconds() as u64, iss: "Minauthator".into() } } diff --git a/src/renderer.rs b/src/renderer.rs index f6294c5..9b163a0 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -6,7 +6,7 @@ use minijinja::{context, Environment, Value}; use crate::models::token_claims::UserTokenClaims; -#[derive(Clone)] +#[derive(Debug, Clone)] #[fully_pub] struct TemplateRenderer { env: Environment<'static>, diff --git a/src/router.rs b/src/router.rs index 3cf20ef..9082f6d 100644 --- a/src/router.rs +++ b/src/router.rs @@ -19,6 +19,7 @@ pub fn build_router(server_config: &ServerConfig, app_state: AppState) -> Router .route("/register", post(ui::register::perform_register)) .route("/login", get(ui::login::login_form)) .route("/login", post(ui::login::perform_login)) + .layer(middleware::from_fn_with_state(app_state.clone(), renderer_middleware)) .layer(middleware::from_fn_with_state(app_state.clone(), user_auth::auth_middleware)); let user_routes = Router::new() @@ -28,15 +29,16 @@ pub fn build_router(server_config: &ServerConfig, app_state: AppState) -> Router .route("/logout", get(ui::logout::perform_logout)) .route("/authorize", get(ui::authorize::authorize_form)) .route("/authorize", post(ui::authorize::perform_authorize)) + .layer(middleware::from_fn_with_state(app_state.clone(), renderer_middleware)) .layer(middleware::from_fn_with_state(app_state.clone(), user_auth::enforce_auth_middleware)) .layer(middleware::from_fn_with_state(app_state.clone(), user_auth::auth_middleware)); - let app_routes = Router::new() + let api_app_routes = Router::new() .route("/api/token", post(api::oauth2::access_token::get_access_token)) .layer(middleware::from_fn_with_state(app_state.clone(), app_auth::enforce_basic_auth_middleware)) .layer(middleware::from_fn_with_state(app_state.clone(), app_auth::basic_auth_middleware)); - let app_user_routes = Router::new() + let api_user_routes = Router::new() .route("/api/user", get(api::read_user::read_user_basic)) .layer(middleware::from_fn_with_state(app_state.clone(), app_auth::enforce_jwt_auth_middleware)); @@ -46,10 +48,9 @@ pub fn build_router(server_config: &ServerConfig, app_state: AppState) -> Router Router::new() .merge(public_routes) .merge(user_routes) - .merge(app_routes) - .merge(app_user_routes) + .merge(api_app_routes) + .merge(api_user_routes) .merge(well_known_routes) - .layer(middleware::from_fn_with_state(app_state.clone(), renderer_middleware)) .nest_service( "/assets", ServeDir::new(server_config.assets_path.clone())