From 7a3f0223eaf505819d8dff4a00b884b31430b6a1 Mon Sep 17 00:00:00 2001 From: Piotr Orzechowski Date: Fri, 12 Apr 2024 08:01:56 +0200 Subject: [PATCH] Refactor authentication and session handling --- src/{session => authentication}/extract.rs | 16 ++--- src/{session => authentication}/middleware.rs | 60 +++++++------------ src/{session => authentication}/mod.rs | 2 +- .../password.rs} | 0 src/lib.rs | 2 +- src/routes/admin/dashboard.rs | 2 +- src/routes/admin/logout.rs | 2 +- src/routes/admin/mod.rs | 15 +++-- src/routes/admin/password/action.rs | 6 +- src/routes/login/action.rs | 4 +- src/routes/newsletters.rs | 2 +- src/{session/state.rs => session_state.rs} | 0 src/startup.rs | 5 -- 13 files changed, 50 insertions(+), 66 deletions(-) rename src/{session => authentication}/extract.rs (53%) rename src/{session => authentication}/middleware.rs (58%) rename src/{session => authentication}/mod.rs (67%) rename src/{authentication.rs => authentication/password.rs} (100%) rename src/{session/state.rs => session_state.rs} (100%) diff --git a/src/session/extract.rs b/src/authentication/extract.rs similarity index 53% rename from src/session/extract.rs rename to src/authentication/extract.rs index b3fe62d..7b6b055 100644 --- a/src/session/extract.rs +++ b/src/authentication/extract.rs @@ -5,6 +5,7 @@ use axum::{ }; use uuid::Uuid; +#[derive(Clone, Debug)] pub struct SessionUserId(pub Uuid); #[async_trait] @@ -12,17 +13,12 @@ impl FromRequestParts for SessionUserId where S: Send + Sync, { - type Rejection = (StatusCode, &'static str); + type Rejection = StatusCode; async fn from_request_parts(parts: &mut Parts, _state: &S) -> Result { - parts - .extensions - .get::() - .cloned() - .map(SessionUserId) - .ok_or(( - StatusCode::INTERNAL_SERVER_ERROR, - "User id not present in session", - )) + parts.extensions.get::().cloned().ok_or({ + tracing::error!("User id not found in session"); + StatusCode::INTERNAL_SERVER_ERROR + }) } } diff --git a/src/session/middleware.rs b/src/authentication/middleware.rs similarity index 58% rename from src/session/middleware.rs rename to src/authentication/middleware.rs index 3d3940f..79953b2 100644 --- a/src/session/middleware.rs +++ b/src/authentication/middleware.rs @@ -1,4 +1,5 @@ -use crate::session::state::TypedSession; +use super::extract::SessionUserId; +use crate::session_state::TypedSession; use anyhow::anyhow; use axum::http::{header::LOCATION, HeaderValue, Request, Response, StatusCode}; use std::{ @@ -10,32 +11,20 @@ use tower::{Layer, Service}; use tower_sessions::Session; use tracing::Instrument; -#[derive(Debug, Clone)] -pub struct AuthorizedSessionLayer { - protected_paths: &'static [&'static str], -} - -impl AuthorizedSessionLayer { - pub fn new(protected_paths: &'static [&'static str]) -> Self { - Self { protected_paths } - } -} +#[derive(Clone, Debug)] +pub struct AuthorizedSessionLayer; impl Layer for AuthorizedSessionLayer { type Service = AuthorizedSession; fn layer(&self, inner: S) -> Self::Service { - AuthorizedSession { - inner, - protected_paths: self.protected_paths, - } + AuthorizedSession { inner } } } -#[derive(Debug, Clone)] +#[derive(Clone, Debug)] pub struct AuthorizedSession { inner: S, - protected_paths: &'static [&'static str], } impl AuthorizedSession { @@ -43,7 +32,7 @@ impl AuthorizedSession { where ResBody: Default, { - tracing::info!("User id not found in session"); + tracing::info!("User is not logged in"); let mut res = Response::default(); *res.status_mut() = StatusCode::SEE_OTHER; res.headers_mut() @@ -81,31 +70,28 @@ where fn call(&mut self, mut req: Request) -> Self::Future { let span = tracing::info_span!("call"); - let protected_paths = self.protected_paths; let clone = self.inner.clone(); let mut inner = std::mem::replace(&mut self.inner, clone); Box::pin( async move { - if protected_paths.contains(&req.uri().path()) { - let Some(session) = req - .extensions() - .get::() - .cloned() - .map(TypedSession::new) - else { - return Ok(Self::internal_server_error(anyhow!("Session not found"))); - }; + let Some(session) = req + .extensions() + .get::() + .cloned() + .map(TypedSession::new) + else { + return Ok(Self::internal_server_error(anyhow!("Session not found"))); + }; - match session.get_user_id().await { - Ok(Some(user_id)) => { - tracing::info!("User id `{user_id}` found in session"); - req.extensions_mut().insert(user_id); - } - Ok(None) => return Ok(Self::see_other()), - Err(e) => return Ok(Self::internal_server_error(e)), - }; - } + match session.get_user_id().await { + Ok(Some(user_id)) => { + tracing::info!("User id `{user_id}` found in session"); + req.extensions_mut().insert(SessionUserId(user_id)); + } + Ok(None) => return Ok(Self::see_other()), + Err(e) => return Ok(Self::internal_server_error(e)), + }; inner.call(req).await } diff --git a/src/session/mod.rs b/src/authentication/mod.rs similarity index 67% rename from src/session/mod.rs rename to src/authentication/mod.rs index 2aea949..e8e9a05 100644 --- a/src/session/mod.rs +++ b/src/authentication/mod.rs @@ -1,3 +1,3 @@ pub mod extract; pub mod middleware; -pub mod state; +pub mod password; diff --git a/src/authentication.rs b/src/authentication/password.rs similarity index 100% rename from src/authentication.rs rename to src/authentication/password.rs diff --git a/src/lib.rs b/src/lib.rs index f2d657f..306d74d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,7 +5,7 @@ pub mod domain; pub mod email_client; pub mod request_id; pub mod routes; -pub mod session; +pub mod session_state; pub mod startup; pub mod telemetry; pub mod utils; diff --git a/src/routes/admin/dashboard.rs b/src/routes/admin/dashboard.rs index 7b5e6a8..e62b8d8 100644 --- a/src/routes/admin/dashboard.rs +++ b/src/routes/admin/dashboard.rs @@ -1,6 +1,6 @@ use crate::{ app_state::AppState, - session::extract::SessionUserId, + authentication::extract::SessionUserId, utils::{e500, HttpError}, }; use anyhow::{Context, Error}; diff --git a/src/routes/admin/logout.rs b/src/routes/admin/logout.rs index b804db0..17cbeae 100644 --- a/src/routes/admin/logout.rs +++ b/src/routes/admin/logout.rs @@ -1,5 +1,5 @@ use crate::{ - session::state::TypedSession, + session_state::TypedSession, utils::{e500, HttpError}, }; use axum::response::Redirect; diff --git a/src/routes/admin/mod.rs b/src/routes/admin/mod.rs index 0a8c94c..f042179 100644 --- a/src/routes/admin/mod.rs +++ b/src/routes/admin/mod.rs @@ -1,4 +1,4 @@ -use crate::app_state::AppState; +use crate::{app_state::AppState, authentication::middleware::AuthorizedSessionLayer}; use axum::{ routing::{get, post}, Router, @@ -13,8 +13,13 @@ mod password; pub fn router() -> Router { Router::new() - .route("/admin/dashboard", get(admin_dashboard)) - .route("/admin/password", get(change_password_form)) - .route("/admin/password", post(change_password)) - .route("/admin/logout", post(log_out)) + .nest( + "/admin", + Router::new() + .route("/dashboard", get(admin_dashboard)) + .route("/password", get(change_password_form)) + .route("/password", post(change_password)) + .route("/logout", post(log_out)), + ) + .layer(AuthorizedSessionLayer) } diff --git a/src/routes/admin/password/action.rs b/src/routes/admin/password/action.rs index 48dc448..a813a29 100644 --- a/src/routes/admin/password/action.rs +++ b/src/routes/admin/password/action.rs @@ -1,10 +1,12 @@ use crate::{ app_state::AppState, authentication::{ - change_password as auth_change_password, validate_credentials, AuthError, Credentials, + extract::SessionUserId, + password::{ + change_password as auth_change_password, validate_credentials, AuthError, Credentials, + }, }, routes::admin::dashboard::get_username, - session::extract::SessionUserId, utils::{e500, HttpError}, }; use axum::{extract::State, response::Redirect, Form}; diff --git a/src/routes/login/action.rs b/src/routes/login/action.rs index ca6f0f2..c72a46c 100644 --- a/src/routes/login/action.rs +++ b/src/routes/login/action.rs @@ -1,7 +1,7 @@ use crate::{ app_state::AppState, - authentication::{validate_credentials, AuthError, Credentials}, - session::state::TypedSession, + authentication::password::{validate_credentials, AuthError, Credentials}, + session_state::TypedSession, }; use axum::{ extract::State, diff --git a/src/routes/newsletters.rs b/src/routes/newsletters.rs index 804e38b..d016eb5 100644 --- a/src/routes/newsletters.rs +++ b/src/routes/newsletters.rs @@ -1,6 +1,6 @@ use crate::{ app_state::AppState, - authentication::{validate_credentials, AuthError, Credentials}, + authentication::password::{validate_credentials, AuthError, Credentials}, domain::{SubscriberEmail, SubscriptionStatus}, }; use anyhow::Context; diff --git a/src/session/state.rs b/src/session_state.rs similarity index 100% rename from src/session/state.rs rename to src/session_state.rs diff --git a/src/startup.rs b/src/startup.rs index 54087ec..eb4492e 100644 --- a/src/startup.rs +++ b/src/startup.rs @@ -4,7 +4,6 @@ use crate::{ email_client::EmailClient, request_id::RequestUuid, routes::{admin, health_check, home, login, newsletters, subscriptions, subscriptions_confirm}, - session::middleware::AuthorizedSessionLayer, telemetry::request_span, }; use anyhow::anyhow; @@ -137,10 +136,6 @@ async fn run( .merge(login::router()) .merge(admin::router()) .with_state(app_state) - .layer(AuthorizedSessionLayer::new(&[ - "/admin/dashboard", - "/admin/password", - ])) .layer(MessagesManagerLayer) .layer( SessionManagerLayer::new(RedisStore::new(redis_pool))