From 38e7c96d2093b4d5ba3ca9c8efceebde55457190 Mon Sep 17 00:00:00 2001
From: Pierre Hubert <pierre.git@communiquons.org>
Date: Tue, 25 Apr 2023 18:28:45 +0200
Subject: [PATCH] Properly handle cb errors

---
 src/controllers/providers_controller.rs | 30 +++++++++++++++++++------
 src/data/action_logger.rs               | 10 +++++++++
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/controllers/providers_controller.rs b/src/controllers/providers_controller.rs
index 16a87c7..22ae6ed 100644
--- a/src/controllers/providers_controller.rs
+++ b/src/controllers/providers_controller.rs
@@ -23,8 +23,8 @@ struct ProviderLoginError<'a> {
 }
 
 impl<'a> ProviderLoginError<'a> {
-    pub fn get(message: &'a str, redirect_uri: &'a LoginRedirect) -> String {
-        Self {
+    pub fn get(message: &'a str, redirect_uri: &'a LoginRedirect) -> HttpResponse {
+        let body = Self {
             _p: BaseLoginPage {
                 danger: None,
                 success: None,
@@ -35,7 +35,11 @@ impl<'a> ProviderLoginError<'a> {
             message,
         }
         .render()
-        .unwrap()
+        .unwrap();
+
+        HttpResponse::Unauthorized()
+            .content_type("text/html")
+            .body(body)
     }
 }
 
@@ -135,10 +139,11 @@ pub async fn finish_login(
                 .map(|e| e.error_description.unwrap_or(e.error))
                 .unwrap_or("Authentication failed (unspecified error)!".to_string());
 
-            return HttpResponse::Unauthorized().body(ProviderLoginError::get(
-                &error_message,
-                &LoginRedirect::default(),
-            ));
+            logger.log(Action::ProviderError {
+                message: error_message.as_str(),
+            });
+
+            return ProviderLoginError::get(&error_message, &LoginRedirect::default());
         }
     };
 
@@ -151,6 +156,17 @@ pub async fn finish_login(
         .await
         .unwrap();
 
+    let state = match state {
+        Some(s) => s,
+        None => {
+            logger.log(Action::ProviderCBInvalidState {
+                state: query.state.as_str(),
+            });
+            log::warn!("User returned invalid state!");
+            return ProviderLoginError::get("Invalid state!", &LoginRedirect::default());
+        }
+    };
+
     // TODO : rate limiting
     // TODO : finish login, get user information
     // TODO : check token signature
diff --git a/src/data/action_logger.rs b/src/data/action_logger.rs
index 2841d81..2f0a6f6 100644
--- a/src/data/action_logger.rs
+++ b/src/data/action_logger.rs
@@ -32,6 +32,12 @@ pub enum Action<'a> {
         provider_id: &'a ProviderID,
         state: &'a str,
     },
+    ProviderError {
+        message: &'a str,
+    },
+    ProviderCBInvalidState {
+        state: &'a str,
+    },
     Signout,
     UserNeed2FAOnLogin(&'a User),
     UserSuccessfullyAuthenticated(&'a User),
@@ -98,6 +104,10 @@ impl<'a> Action<'a> {
             Action::StartLoginAttemptWithOpenIDProvider { provider_id, state } => format!(
                 "started new authentication attempt through an OpenID provider (prov={} / state={state})", provider_id.0
             ),
+            Action::ProviderError { message } =>
+                format!("failed provider authentication with message '{message}'"),
+            Action::ProviderCBInvalidState { state } =>
+                format!("provided invalid callback state after provider authentication: '{state}'"),
             Action::Signout => "signed out".to_string(),
             Action::UserNeed2FAOnLogin(user) => {
                 format!(