Refactor login flow

src/controllers/login_controller.rs

@@ -7,13 +7,14 @@ use crate::actors::{bruteforce_actor, users_actor};
 use crate::actors::bruteforce_actor::BruteForceActor;
 use crate::actors::users_actor::{ChangePasswordResult, LoginResult, UsersActor};
 use crate::constants::{APP_NAME, MAX_FAILED_LOGIN_ATTEMPTS, MIN_PASS_LEN};
-use crate::controllers::base_controller::{FatalErrorPage, redirect_user};
+use crate::controllers::base_controller::{FatalErrorPage, redirect_user, redirect_user_for_login};
+use crate::data::login_redirect_query::LoginRedirectQuery;
 use crate::data::remote_ip::RemoteIP;
 use crate::data::session_identity::{SessionIdentity, SessionStatus};
 
 struct BaseLoginPage {
-    danger: String,
+    danger: Option<String>,
-    success: String,
+    success: Option<String>,
     page_title: &'static str,
     app_name: &'static str,
     redirect_uri: String,
@@ -42,7 +43,8 @@ pub struct LoginRequestBody {
 #[derive(serde::Deserialize)]
 pub struct LoginRequestQuery {
     logout: Option<bool>,
-    redirect: Option<String>,
+    #[serde(default)]
+    redirect: LoginRedirectQuery,
 }
 
 /// Authenticate user
@@ -54,11 +56,10 @@ pub async fn login_route(
     req: Option<web::Form<LoginRequestBody>>,
     id: Identity,
 ) -> impl Responder {
-    let mut danger = String::new();
+    let mut danger = None;
-    let mut success = String::new();
+    let mut success = None;
     let mut login = String::new();
 
-
     let failed_attempts = bruteforce.send(bruteforce_actor::CountFailedAttempt { ip: remote_ip.into() })
         .await.unwrap();
 
@@ -70,49 +71,24 @@ pub async fn login_route(
         );
     }
 
-    let redirect_uri = match query.redirect.as_deref() {
-        None => "/",
-        Some(s) => match s.starts_with('/') && !s.starts_with("//") {
-            true => s,
-            false => "/",
-        },
-    };
-
     // Check if user session must be closed
     if let Some(true) = query.logout {
         id.forget();
-        success = "Goodbye!".to_string();
+        success = Some("Goodbye!".to_string());
     }
 
     // Check if user is already authenticated
     if SessionIdentity(&id).is_authenticated() {
-        return redirect_user(redirect_uri);
+        return redirect_user(query.redirect.get());
     }
 
-    // Check if user is setting a new  password
+    // Check if the password of the user has to be changed
-    if let (Some(req), true) = (&req, SessionIdentity(&id).need_new_password()) {
+    if SessionIdentity(&id).need_new_password() {
-        if req.password.len() < MIN_PASS_LEN {
+        return redirect_user(&format!("/reset_password?redirect={}", query.redirect.get_encoded()));
-            danger = "Password is too short!".to_string();
-        } else {
-            let res: ChangePasswordResult = users
-                .send(users_actor::ChangePasswordRequest {
-                    user_id: SessionIdentity(&id).user_id(),
-                    new_password: req.password.clone(),
-                    temporary: false,
-                })
-                .await
-                .unwrap();
-
-            if !res.0 {
-                danger = "Failed to change password!".to_string();
-            } else {
-                SessionIdentity(&id).set_status(SessionStatus::SignedIn);
-                return redirect_user(redirect_uri);
-            }
-        }
     }
+
     // Try to authenticate user
-    else if let Some(req) = &req {
+    if let Some(req) = &req {
         login = req.login.clone();
         let response: LoginResult = users
             .send(users_actor::LoginRequest {
@@ -126,45 +102,28 @@ pub async fn login_route(
             LoginResult::Success(user) => {
                 SessionIdentity(&id).set_user(&user);
 
-                if user.need_reset_password {
+                return if user.need_reset_password {
                     SessionIdentity(&id).set_status(SessionStatus::NeedNewPassword);
+                    redirect_user(&format!("/reset_password?redirect={}", query.redirect.get_encoded()))
                 } else {
-                    return redirect_user(redirect_uri);
+                    redirect_user(query.redirect.get())
-                }
+                };
             }
 
             LoginResult::AccountDisabled => {
                 log::warn!("Failed login for username {} : account is disabled", login);
-                danger = "Your account is disabled!".to_string();
+                danger = Some("Your account is disabled!".to_string());
             }
 
             c => {
                 log::warn!("Failed login for ip {:?} /  username {}: {:?}", remote_ip, login, c);
-                danger = "Login failed.".to_string();
+                danger = Some("Login failed.".to_string());
 
                 bruteforce.send(bruteforce_actor::RecordFailedAttempt { ip: remote_ip.into() }).await.unwrap();
             }
         }
     }
 
-    // Display password reset form if it is appropriate
-    if SessionIdentity(&id).need_new_password() {
-        return HttpResponse::Ok().content_type("text/html").body(
-            PasswordResetTemplate {
-                _p: BaseLoginPage {
-                    page_title: "Password reset",
-                    danger,
-                    success,
-                    app_name: APP_NAME,
-                    redirect_uri: urlencoding::encode(redirect_uri).to_string(),
-                },
-                min_pass_len: MIN_PASS_LEN,
-            }
-                .render()
-                .unwrap(),
-        );
-    }
-
     HttpResponse::Ok().content_type("text/html").body(
         LoginTemplate {
             _p: BaseLoginPage {
@@ -172,7 +131,7 @@ pub async fn login_route(
                 danger,
                 success,
                 app_name: APP_NAME,
-                redirect_uri: urlencoding::encode(redirect_uri).to_string(),
+                redirect_uri: query.redirect.get_encoded(),
             },
             login,
         }
@@ -185,3 +144,63 @@ pub async fn login_route(
 pub async fn logout_route() -> impl Responder {
     redirect_user("/login?logout=true")
 }
+
+#[derive(serde::Deserialize)]
+pub struct ChangePasswordRequestBody {
+    password: String,
+}
+
+#[derive(serde::Deserialize)]
+pub struct PasswordResetQuery {
+    #[serde(default)]
+    redirect: LoginRedirectQuery,
+}
+
+/// Reset user password route
+pub async fn reset_password_route(id: Identity, query: web::Query<PasswordResetQuery>,
+                                  req: Option<web::Form<ChangePasswordRequestBody>>,
+                                  users: web::Data<Addr<UsersActor>>, ) -> impl Responder {
+    let mut danger = None;
+
+    if !SessionIdentity(&id).need_new_password() {
+        return redirect_user_for_login(query.redirect.get());
+    }
+
+    // Check if user is setting a new  password
+    if let Some(req) = &req {
+        if req.password.len() < MIN_PASS_LEN {
+            danger = Some("Password is too short!".to_string());
+        } else {
+            let res: ChangePasswordResult = users
+                .send(users_actor::ChangePasswordRequest {
+                    user_id: SessionIdentity(&id).user_id(),
+                    new_password: req.password.clone(),
+                    temporary: false,
+                })
+                .await
+                .unwrap();
+
+            if !res.0 {
+                danger = Some("Failed to change password!".to_string());
+            } else {
+                SessionIdentity(&id).set_status(SessionStatus::SignedIn);
+                return redirect_user(query.redirect.get());
+            }
+        }
+    }
+
+    HttpResponse::Ok().content_type("text/html").body(
+        PasswordResetTemplate {
+            _p: BaseLoginPage {
+                page_title: "Password reset",
+                danger,
+                success: None,
+                app_name: APP_NAME,
+                redirect_uri: query.redirect.get_encoded(),
+            },
+            min_pass_len: MIN_PASS_LEN,
+        }
+            .render()
+            .unwrap(),
+    )
+}

src/data/login_redirect_query.rs

@@ -0,0 +1,21 @@
+#[derive(serde::Serialize, serde::Deserialize, Debug, Eq, PartialEq, Clone)]
+pub struct LoginRedirectQuery(String);
+
+impl LoginRedirectQuery {
+    pub fn get(&self) -> &str {
+        match self.0.starts_with('/') && !self.0.starts_with("//") {
+            true => self.0.as_str(),
+            false => "/",
+        }
+    }
+
+    pub fn get_encoded(&self) -> String {
+        urlencoding::encode(self.get()).to_string()
+    }
+}
+
+impl Default for LoginRedirectQuery {
+    fn default() -> Self {
+        Self("/".to_string())
+    }
+}

src/data/mod.rs

@@ -11,4 +11,5 @@ pub mod id_token;
 pub mod code_challenge;
 pub mod open_id_user_info;
 pub mod access_token;
-pub mod totp_key;
+pub mod totp_key;
+pub mod login_redirect_query;

src/main.rs

@@ -12,7 +12,6 @@ use basic_oidc::actors::users_actor::UsersActor;
 use basic_oidc::constants::*;
 use basic_oidc::controllers::*;
 use basic_oidc::controllers::assets_controller::assets_route;
-use basic_oidc::controllers::login_controller::{login_route, logout_route};
 use basic_oidc::data::app_config::AppConfig;
 use basic_oidc::data::client::ClientManager;
 use basic_oidc::data::entity_manager::EntityManager;
@@ -108,11 +107,13 @@ async fn main() -> std::io::Result<()> {
             .route("/assets/{path:.*}", web::get().to(assets_route))
 
             // Login page
-            .route("/login", web::get().to(login_route))
+            .route("/login", web::get().to(login_controller::login_route))
-            .route("/login", web::post().to(login_route))
+            .route("/login", web::post().to(login_controller::login_route))
+            .route("/reset_password", web::get().to(login_controller::reset_password_route))
+            .route("/reset_password", web::post().to(login_controller::reset_password_route))
 
             // Logout page
-            .route("/logout", web::get().to(logout_route))
+            .route("/logout", web::get().to(login_controller::logout_route))
 
             // Settings routes
             .route("/settings", web::get().to(settings_controller::account_settings_details_route))

templates/login/base_login_page.html

@@ -43,13 +43,17 @@
 
     <h1 class="h3 mb-3 fw-normal">{{ _p.page_title }}</h1>
 
+    {% if let Some(danger) = _p.danger %}
     <div class="alert alert-danger" role="alert">
-        {{ _p.danger }}
+        {{ danger }}
     </div>
+    {% endif %}
 
+    {% if let Some(success) = _p.success %}
     <div class="alert alert-success" role="alert">
-        {{ _p.success }}
+        {{ success }}
     </div>
+    {% endif %}
 
     {% block content %}
     TO_REPLACE

templates/login/password_reset.html

@@ -1,14 +1,11 @@
 {% extends "base_login_page.html" %}
 {% block content %}
-<form action="/login?redirect={{ _p.redirect_uri }}" method="post" id="reset_password_form">
+<form action="/reset_password?redirect={{ _p.redirect_uri }}" method="post" id="reset_password_form">
     <div>
         <p>You need to configure a new password:</p>
 
         <p style="color:red" id="err_target"></p>
 
-        <!-- Needed for controller -->
-        <input type="hidden" name="login" value="."/>
-
         <div class="form-floating">
             <input name="password" type="password" required class="form-control" id="pass1"
                    placeholder="Password"/>
@@ -46,8 +43,6 @@
         else
             form.submit();
     })
-
-
 </script>