Refactor OIDC state management to use a temporary login flow state cookie, enhancing nonce and redirect handling. Update cookie creation and retrieval methods for improved clarity and maintainability.

fixes #1014

Author: lovasoa

src/webserver/oidc.rs

@@ -41,11 +41,13 @@ type LocalBoxFuture<T> = Pin<Box<dyn Future<Output = T> + 'static>>;
 const SQLPAGE_AUTH_COOKIE_NAME: &str = "sqlpage_auth";
 const SQLPAGE_REDIRECT_URI: &str = "/sqlpage/oidc_callback";
 const SQLPAGE_NONCE_COOKIE_NAME: &str = "sqlpage_oidc_nonce";
-const SQLPAGE_REDIRECT_URL_COOKIE_PREFIX: &str = "sqlpage_oidc_redirect_url_";
+const SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX: &str = "sqlpage_oidc_state_";
 const OIDC_CLIENT_MAX_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60);
 const OIDC_CLIENT_MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5);
 const AUTH_COOKIE_EXPIRATION: awc::cookie::time::Duration =
     actix_web::cookie::time::Duration::days(7);
+const LOGIN_FLOW_STATE_COOKIE_EXPIRATION: awc::cookie::time::Duration =
+    actix_web::cookie::time::Duration::minutes(10);
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
 #[serde(transparent)]
@@ -413,26 +415,29 @@ async fn process_oidc_callback(
         .with_context(|| format!("{SQLPAGE_REDIRECT_URI}: invalid url parameters"))?
         .into_inner();
     log::debug!("Processing OIDC callback with params: {params:?}. Requesting token...");
-    let mut redirect_url_cookie = get_redirect_url_cookie(request, &params.state)?;
+    let mut tmp_login_flow_state_cookie = get_tmp_login_flow_state_cookie(request, &params.state)?;
     let client = oidc_state.get_client().await;
     let http_client = get_http_client_from_appdata(request)?;
     let id_token = exchange_code_for_token(&client, http_client, params.clone()).await?;
     log::debug!("Received token response: {id_token:?}");
-    let nonce = get_nonce_from_cookie(request)?;
-    let redirect_target = validate_redirect_url(redirect_url_cookie.value().to_string());
+    let LoginFlowState {
+        nonce,
+        redirect_target,
+    } = parse_login_flow_state(&tmp_login_flow_state_cookie)?;
+    let redirect_target = validate_redirect_url(redirect_target.to_string());
 
     log::info!("Redirecting to {redirect_target} after a successful login");
     let mut response = build_redirect_response(redirect_target);
-    set_auth_cookie(&mut response, &id_token).context("Failed to set auth cookie")?;
+    set_auth_cookie(&mut response, &id_token);
     let claims = oidc_state
         .get_token_claims(id_token, &nonce)
         .await
         .context("The identity provider returned an invalid ID token")?;
     log::debug!("{} successfully logged in", claims.subject().as_str());
-    let nonce_cookie = create_nonce_cookie(&nonce);
+    let nonce_cookie = create_final_nonce_cookie(&nonce);
     response.add_cookie(&nonce_cookie)?;
-    redirect_url_cookie.set_path("/"); // Required to clean up the cookie
-    response.add_removal_cookie(&redirect_url_cookie)?;
+    tmp_login_flow_state_cookie.set_path("/"); // Required to clean up the cookie
+    response.add_removal_cookie(&tmp_login_flow_state_cookie)?;
     Ok(response)
 }
 
@@ -456,7 +461,7 @@ async fn exchange_code_for_token(
     Ok(id_token.clone())
 }
 
-fn set_auth_cookie(response: &mut HttpResponse, id_token: &OidcToken) -> anyhow::Result<()> {
+fn set_auth_cookie(response: &mut HttpResponse, id_token: &OidcToken) {
     let id_token_str = id_token.to_string();
     log::trace!("Setting auth cookie: {SQLPAGE_AUTH_COOKIE_NAME}=\"{id_token_str}\"");
     let id_token_size_kb = id_token_str.len() / 1024;
@@ -475,20 +480,17 @@ fn set_auth_cookie(response: &mut HttpResponse, id_token: &OidcToken) -> anyhow:
         .finish();
 
     response.add_cookie(&cookie).unwrap();
-    Ok(())
 }
 
 async fn build_auth_provider_redirect_response(
     oidc_state: &OidcState,
     initial_url: &str,
 ) -> HttpResponse {
     let AuthUrl { url, params } = build_auth_url(oidc_state).await;
-    let nonce_cookie = create_nonce_cookie(&params.nonce);
-    let redirect_cookie = create_redirect_cookie(&params.csrf_token, initial_url);
+    let tmp_login_flow_state_cookie = create_tmp_login_flow_state_cookie(&params, initial_url);
     HttpResponse::TemporaryRedirect()
         .append_header((header::LOCATION, url.to_string()))
-        .cookie(nonce_cookie)
-        .cookie(redirect_cookie)
+        .cookie(tmp_login_flow_state_cookie)
         .body("Redirecting...")
 }
 
@@ -510,7 +512,7 @@ async fn get_authenticated_user_info(
     let id_token = OidcToken::from_str(&cookie_value)
         .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?;
 
-    let nonce = get_nonce_from_cookie(request)?;
+    let nonce = get_final_nonce_from_cookie(request)?;
     log::debug!("Verifying id token: {id_token:?}");
     let claims = oidc_state.get_token_claims(id_token, &nonce).await?;
     log::debug!("The current user is: {claims:?}");
@@ -742,7 +744,7 @@ fn nonce_matches(id_token_nonce: &Nonce, state_nonce: &Nonce) -> Result<(), Stri
     Ok(())
 }
 
-fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> {
+fn create_final_nonce_cookie(nonce: &Nonce) -> Cookie<'_> {
     Cookie::build(SQLPAGE_NONCE_COOKIE_NAME, nonce.secret())
         .secure(true)
         .http_only(true)
@@ -752,34 +754,55 @@ fn create_nonce_cookie(nonce: &Nonce) -> Cookie<'_> {
         .finish()
 }
 
-fn create_redirect_cookie<'a>(csrf_token: &CsrfToken, initial_url: &'a str) -> Cookie<'a> {
-    let cookie_name = SQLPAGE_REDIRECT_URL_COOKIE_PREFIX.to_owned() + csrf_token.secret();
-    Cookie::build(cookie_name, initial_url)
+fn create_tmp_login_flow_state_cookie<'a>(
+    params: &'a AuthUrlParams,
+    initial_url: &'a str,
+) -> Cookie<'a> {
+    let csrf_token = &params.csrf_token;
+    let cookie_name = SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX.to_owned() + csrf_token.secret();
+    let cookie_value = serde_json::to_string(&LoginFlowState {
+        nonce: params.nonce.clone(),
+        redirect_target: initial_url,
+    })
+    .expect("login flow state is always serializable");
+    Cookie::build(cookie_name, cookie_value)
         .secure(true)
         .http_only(true)
         .same_site(actix_web::cookie::SameSite::Lax)
         .path("/")
-        .max_age(actix_web::cookie::time::Duration::minutes(10))
+        .max_age(LOGIN_FLOW_STATE_COOKIE_EXPIRATION)
         .finish()
 }
 
-fn get_nonce_from_cookie(request: &ServiceRequest) -> anyhow::Result<Nonce> {
+fn get_final_nonce_from_cookie(request: &ServiceRequest) -> anyhow::Result<Nonce> {
     let cookie = request
         .cookie(SQLPAGE_NONCE_COOKIE_NAME)
         .with_context(|| format!("No {SQLPAGE_NONCE_COOKIE_NAME} cookie found"))?;
     Ok(Nonce::new(cookie.value().to_string()))
 }
 
-fn get_redirect_url_cookie(
+fn get_tmp_login_flow_state_cookie(
     request: &ServiceRequest,
     csrf_token: &CsrfToken,
 ) -> anyhow::Result<Cookie<'static>> {
-    let cookie_name = SQLPAGE_REDIRECT_URL_COOKIE_PREFIX.to_owned() + csrf_token.secret();
+    let cookie_name = SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX.to_owned() + csrf_token.secret();
     request
         .cookie(&cookie_name)
         .with_context(|| format!("No {cookie_name} cookie found"))
 }
 
+#[derive(Debug, Serialize, Deserialize, Clone)]
+struct LoginFlowState<'a> {
+    #[serde(rename = "n")]
+    nonce: Nonce,
+    #[serde(rename = "r")]
+    redirect_target: &'a str,
+}
+
+fn parse_login_flow_state<'a>(cookie: &'a Cookie<'_>) -> anyhow::Result<LoginFlowState<'a>> {
+    Ok(serde_json::from_str(cookie.value())?)
+}
+
 /// Given an audience, verify if it is trusted. The `client_id` is always trusted, independently of this function.
 #[derive(Clone, Debug)]
 pub struct AudienceVerifier(Option<HashSet<String>>);