From 6ef8c01e5dbc0fa4e13cdb68fd069ac13f906b3e Mon Sep 17 00:00:00 2001 From: lovasoa Date: Thu, 31 Jul 2025 13:26:03 +0200 Subject: [PATCH 01/16] WIP: sso oidc metadata refresh introduces an async oidc_state.get_client --- src/webserver/oidc.rs | 56 ++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index ca7a15a4..3711fd07 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -132,7 +132,14 @@ fn get_app_host(config: &AppConfig) -> String { pub struct OidcState { pub config: OidcConfig, - pub client: OidcClient, + client: OidcClient, +} + +impl OidcState { + pub async fn get_client(&self) -> &OidcClient { + todo!(); + &self.client + } } pub async fn initialize_oidc_state( @@ -239,12 +246,15 @@ where log::debug!("Redirecting to OIDC provider"); - let response = build_auth_provider_redirect_response( - &self.oidc_state.client, - &self.oidc_state.config, - &request, - ); - Box::pin(async move { Ok(request.into_response(response)) }) + let oidc_state = Arc::clone(&self.oidc_state); + Box::pin(async move { + let response = build_auth_provider_redirect_response( + oidc_state.get_client().await, + &oidc_state.config, + &request, + ); + Ok(request.into_response(response)) + }) } fn handle_oidc_callback( @@ -255,22 +265,13 @@ where Box::pin(async move { let query_string = request.query_string(); - match process_oidc_callback( - &oidc_state.client, - &oidc_state.config, - query_string, - &request, - ) - .await - { + let client = oidc_state.get_client().await; + match process_oidc_callback(client, &oidc_state.config, query_string, &request).await { Ok(response) => Ok(request.into_response(response)), Err(e) => { log::error!("Failed to process OIDC callback with params {query_string}: {e}"); - let resp = build_auth_provider_redirect_response( - &oidc_state.client, - &oidc_state.config, - &request, - ); + let resp = + build_auth_provider_redirect_response(client, &oidc_state.config, &request); Ok(request.into_response(resp)) } } @@ -305,9 +306,7 @@ where fn call(&self, request: ServiceRequest) -> Self::Future { log::trace!("Started OIDC middleware request handling"); - let oidc_client = &self.oidc_state.client; - let oidc_config = &self.oidc_state.config; - match get_authenticated_user_info(oidc_client, oidc_config, &request) { + match get_authenticated_user_info(&self.oidc_state, &request) { Ok(Some(claims)) => { if request.path() == SQLPAGE_REDIRECT_URI { return handle_authenticated_oidc_callback(request); @@ -330,11 +329,7 @@ where return self.handle_unauthenticated_request(request); } } - let future = self.service.call(request); - Box::pin(async move { - let response = future.await?; - Ok(response) - }) + Box::pin(self.service.call(request)) } } @@ -446,8 +441,7 @@ fn build_redirect_response(target_url: String) -> HttpResponse { /// Returns the claims from the ID token in the `SQLPage` auth cookie. fn get_authenticated_user_info( - oidc_client: &OidcClient, - config: &OidcConfig, + oidc_state: &Arc, request: &ServiceRequest, ) -> anyhow::Result> { let Some(cookie) = request.cookie(SQLPAGE_AUTH_COOKIE_NAME) else { @@ -456,6 +450,8 @@ fn get_authenticated_user_info( let cookie_value = cookie.value().to_string(); let state = get_state_from_cookie(request)?; + let config = oidc_state.config; + let oidc_client = oidc_state.get_client().await; let verifier = config.create_id_token_verifier(oidc_client); let id_token = OidcToken::from_str(&cookie_value) .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; From 061f540946cc5e9b6bbe95e59461d6f61e7b8d6a Mon Sep 17 00:00:00 2001 From: lovasoa Date: Thu, 31 Jul 2025 22:38:49 +0200 Subject: [PATCH 02/16] Refresh OIDC client periodically --- src/webserver/oidc.rs | 123 ++++++++++++++++++++++++++++++++---------- 1 file changed, 96 insertions(+), 27 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 3711fd07..3faca26f 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -1,5 +1,7 @@ use std::collections::HashSet; use std::future::ready; +use std::ops::Deref; +use std::time::{Duration, Instant}; use std::{future::Future, pin::Pin, str::FromStr, sync::Arc}; use crate::webserver::http_client::get_http_client_from_appdata; @@ -15,12 +17,15 @@ use actix_web::{ use anyhow::{anyhow, Context}; use awc::Client; use chrono::Utc; +use openidconnect::core::CoreJsonWebKey; +use openidconnect::IdTokenVerifier; use openidconnect::{ core::CoreAuthenticationFlow, url::Url, AsyncHttpClient, Audience, CsrfToken, EndpointMaybeSet, EndpointNotSet, EndpointSet, IssuerUrl, Nonce, OAuth2TokenResponse, RedirectUrl, Scope, TokenResponse, }; use serde::{Deserialize, Serialize}; +use std::sync::Mutex; use super::http_client::make_http_client; @@ -99,7 +104,7 @@ impl OidcConfig { fn create_id_token_verifier<'a>( &'a self, oidc_client: &'a OidcClient, - ) -> openidconnect::IdTokenVerifier<'a, openidconnect::core::CoreJsonWebKey> { + ) -> IdTokenVerifier<'a, CoreJsonWebKey> { oidc_client .id_token_verifier() .set_other_audience_verifier_fn(self.additional_audience_verifier.as_fn()) @@ -130,15 +135,78 @@ fn get_app_host(config: &AppConfig) -> String { host } +pub struct ClientWithTime { + client: OidcClient, + last_update: Instant, +} + pub struct OidcState { pub config: OidcConfig, - client: OidcClient, + app_config: AppConfig, + client: Mutex, } +const OIDC_CLIENT_REFRESH_INTERVAL: Duration = Duration::from_secs(600); + impl OidcState { - pub async fn get_client(&self) -> &OidcClient { - todo!(); - &self.client + pub async fn new(oidc_cfg: OidcConfig, app_config: AppConfig) -> anyhow::Result { + let http_client = make_http_client(&app_config)?; + let client = build_oidc_client(&oidc_cfg, &http_client).await?; + + Ok(Self { + config: oidc_cfg, + app_config, + client: Mutex::new(ClientWithTime { + client, + last_update: Instant::now(), + }), + }) + } + + async fn refresh(&self) { + let Ok(http_client) = make_http_client(&self.app_config) else { + log::error!("Failed to create HTTP client"); + return; + }; + match build_oidc_client(&self.config, &http_client).await { + Ok(client) => { + *self.client.lock().expect("oidc client") = ClientWithTime { + client, + last_update: Instant::now(), + }; + } + Err(e) => { + log::error!("Failed to refresh OIDC client: {e}"); + } + } + } + + /// Gets a reference to the oidc client, potentially generating a new one if needed + pub async fn get_client(&self) -> impl Deref + '_ { + { + let client_lock = self.client.lock().expect("oidc client"); + if client_lock.last_update.elapsed() < OIDC_CLIENT_REFRESH_INTERVAL { + return client_lock; + } + } + self.refresh().await; + self.client.lock().expect("oidc client") + } + + fn get_token_claims( + &self, + id_token: &OidcToken, + state: &OidcLoginState, + ) -> anyhow::Result { + // Do not refresh the client on every check + let client = &self.client.lock().expect("oidc client").client; + let verifier = self.config.create_id_token_verifier(client); + let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, &state.nonce); + let claims: OidcClaims = id_token + .claims(&verifier, nonce_verifier) + .with_context(|| format!("Could not verify the ID token: {id_token:?}"))? + .clone(); + Ok(claims) } } @@ -151,15 +219,19 @@ pub async fn initialize_oidc_state( Err(Some(e)) => return Err(anyhow::anyhow!(e)), }; - let http_client = make_http_client(app_config)?; - let provider_metadata = - discover_provider_metadata(&http_client, oidc_cfg.issuer_url.clone()).await?; - let client = make_oidc_client(&oidc_cfg, provider_metadata)?; + Ok(Some(Arc::new( + OidcState::new(oidc_cfg, app_config.clone()).await?, + ))) +} - Ok(Some(Arc::new(OidcState { - config: oidc_cfg, - client, - }))) +async fn build_oidc_client( + oidc_cfg: &OidcConfig, + http_client: &Client, +) -> anyhow::Result { + let provider_metadata = + discover_provider_metadata(http_client, oidc_cfg.issuer_url.clone()).await?; + let client = make_oidc_client(oidc_cfg, provider_metadata)?; + Ok(client) } pub struct OidcMiddleware { @@ -249,7 +321,7 @@ where let oidc_state = Arc::clone(&self.oidc_state); Box::pin(async move { let response = build_auth_provider_redirect_response( - oidc_state.get_client().await, + &oidc_state.get_client().await.client, &oidc_state.config, &request, ); @@ -266,12 +338,17 @@ where Box::pin(async move { let query_string = request.query_string(); let client = oidc_state.get_client().await; - match process_oidc_callback(client, &oidc_state.config, query_string, &request).await { + match process_oidc_callback(&client.client, &oidc_state.config, query_string, &request) + .await + { Ok(response) => Ok(request.into_response(response)), Err(e) => { log::error!("Failed to process OIDC callback with params {query_string}: {e}"); - let resp = - build_auth_provider_redirect_response(client, &oidc_state.config, &request); + let resp = build_auth_provider_redirect_response( + &client.client, + &oidc_state.config, + &request, + ); Ok(request.into_response(resp)) } } @@ -448,19 +525,11 @@ fn get_authenticated_user_info( return Ok(None); }; let cookie_value = cookie.value().to_string(); - - let state = get_state_from_cookie(request)?; - let config = oidc_state.config; - let oidc_client = oidc_state.get_client().await; - let verifier = config.create_id_token_verifier(oidc_client); let id_token = OidcToken::from_str(&cookie_value) .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; - let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, &state.nonce); - let claims: OidcClaims = id_token - .claims(&verifier, nonce_verifier) - .with_context(|| format!("Could not verify the ID token: {cookie_value:?}"))? - .clone(); + let state = get_state_from_cookie(request)?; + let claims = oidc_state.get_token_claims(&id_token, &state)?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) } From df080ec42620126dabaa3364b01ddd333e8eb920 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Thu, 31 Jul 2025 23:06:26 +0200 Subject: [PATCH 03/16] Increase OIDC client refresh interval to 1 hour --- src/webserver/oidc.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 3faca26f..a2ab6563 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -34,6 +34,7 @@ type LocalBoxFuture = Pin + 'static>>; const SQLPAGE_AUTH_COOKIE_NAME: &str = "sqlpage_auth"; const SQLPAGE_REDIRECT_URI: &str = "/sqlpage/oidc_callback"; const SQLPAGE_STATE_COOKIE_NAME: &str = "sqlpage_oidc_state"; +const OIDC_CLIENT_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60); #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(transparent)] @@ -146,8 +147,6 @@ pub struct OidcState { client: Mutex, } -const OIDC_CLIENT_REFRESH_INTERVAL: Duration = Duration::from_secs(600); - impl OidcState { pub async fn new(oidc_cfg: OidcConfig, app_config: AppConfig) -> anyhow::Result { let http_client = make_http_client(&app_config)?; @@ -189,10 +188,12 @@ impl OidcState { return client_lock; } } + log::debug!("OIDC client is older than {OIDC_CLIENT_REFRESH_INTERVAL:?}, refreshing..."); self.refresh().await; self.client.lock().expect("oidc client") } + /// Validate and decode the claims of an OIDC token, without refreshing the client. fn get_token_claims( &self, id_token: &OidcToken, @@ -228,8 +229,8 @@ async fn build_oidc_client( oidc_cfg: &OidcConfig, http_client: &Client, ) -> anyhow::Result { - let provider_metadata = - discover_provider_metadata(http_client, oidc_cfg.issuer_url.clone()).await?; + let issuer_url = oidc_cfg.issuer_url.clone(); + let provider_metadata = discover_provider_metadata(http_client, issuer_url.clone()).await?; let client = make_oidc_client(oidc_cfg, provider_metadata)?; Ok(client) } From 5bd3ac8407e9b5a2be3a70c5a9db2ee34a445512 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 00:36:18 +0200 Subject: [PATCH 04/16] feat: Add support for custom claims in OIDC JWT tokens This enhancement allows SQLPage to read and utilize custom claims from JWT tokens generated by OIDC providers, enabling users to store additional information like roles or permissions that can be used in SQL queries. Key changes: - Modified get_token_claims() to accept optional login state, allowing token verification without requiring active login session state - Updated nonce verification logic to handle cases where no login state exists - Refactored OIDC client and token response types to use more specific generics that support additional claims through OidcAdditionalClaims - Simplified function signatures by passing oidc_state instead of separate client and config parameters - Enhanced error handling in OIDC callback processing with automatic client refresh - Updated type definitions to properly support custom claim extraction from tokens This enables SSO providers to include custom user metadata that SQLPage can access and use for authorization and personalization in database queries. --- CHANGELOG.md | 1 + src/webserver/oidc.rs | 111 +++++++++++++++++++++++++----------------- 2 files changed, 68 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f71b0cba..0c08b7ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ ``` - The file-based routing system was improved. Now, requests to `/xxx` redirect to `/xxx/` only if `/xxx/index.sql` exists. - fix: When single sign on is enabled, and an anonymous user visits a page with URL parameters, the user is correctly redirected to the page with the parameters after login. +- Added support for reading custom claims in JWT tokens generated by OIDC providers. This means that you can configure your Single-Sign-On provider to store custom pieces of information about your users, such as roles or permissions, and use them in your SQL queries in SQLPage. ## v0.35.2 - Fix a bug with zero values being displayed with a non-zero height in stacked bar charts. diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index a2ab6563..e1f1d6dc 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -17,13 +17,20 @@ use actix_web::{ use anyhow::{anyhow, Context}; use awc::Client; use chrono::Utc; -use openidconnect::core::CoreJsonWebKey; -use openidconnect::IdTokenVerifier; +use openidconnect::core::{ + CoreAuthDisplay, CoreAuthPrompt, CoreErrorResponseType, CoreGenderClaim, CoreJsonWebKey, + CoreJweContentEncryptionAlgorithm, CoreJwsSigningAlgorithm, CoreRevocableToken, + CoreRevocationErrorResponse, CoreTokenIntrospectionResponse, CoreTokenType, +}; use openidconnect::{ core::CoreAuthenticationFlow, url::Url, AsyncHttpClient, Audience, CsrfToken, EndpointMaybeSet, EndpointNotSet, EndpointSet, IssuerUrl, Nonce, OAuth2TokenResponse, RedirectUrl, Scope, TokenResponse, }; +use openidconnect::{ + EmptyExtraTokenFields, IdTokenFields, IdTokenVerifier, StandardErrorResponse, + StandardTokenResponse, +}; use serde::{Deserialize, Serialize}; use std::sync::Mutex; @@ -197,12 +204,12 @@ impl OidcState { fn get_token_claims( &self, id_token: &OidcToken, - state: &OidcLoginState, + state: Option<&OidcLoginState>, ) -> anyhow::Result { // Do not refresh the client on every check let client = &self.client.lock().expect("oidc client").client; let verifier = self.config.create_id_token_verifier(client); - let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, &state.nonce); + let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, state); let claims: OidcClaims = id_token .claims(&verifier, nonce_verifier) .with_context(|| format!("Could not verify the ID token: {id_token:?}"))? @@ -321,11 +328,7 @@ where let oidc_state = Arc::clone(&self.oidc_state); Box::pin(async move { - let response = build_auth_provider_redirect_response( - &oidc_state.get_client().await.client, - &oidc_state.config, - &request, - ); + let response = build_auth_provider_redirect_response(&oidc_state, &request); Ok(request.into_response(response)) }) } @@ -338,18 +341,12 @@ where Box::pin(async move { let query_string = request.query_string(); - let client = oidc_state.get_client().await; - match process_oidc_callback(&client.client, &oidc_state.config, query_string, &request) - .await - { + match process_oidc_callback(&oidc_state, query_string, &request).await { Ok(response) => Ok(request.into_response(response)), Err(e) => { log::error!("Failed to process OIDC callback with params {query_string}: {e}"); - let resp = build_auth_provider_redirect_response( - &client.client, - &oidc_state.config, - &request, - ); + oidc_state.refresh().await; + let resp = build_auth_provider_redirect_response(&oidc_state, &request); Ok(request.into_response(resp)) } } @@ -412,8 +409,7 @@ where } async fn process_oidc_callback( - oidc_client: &OidcClient, - oidc_config: &OidcConfig, + oidc_state: &OidcState, query_string: &str, request: &ServiceRequest, ) -> anyhow::Result { @@ -434,14 +430,15 @@ async fn process_oidc_callback( return Err(anyhow!("Invalid CSRF token: {}", params.state.secret())); } + let client = oidc_state.get_client().await; log::debug!("Processing OIDC callback with params: {params:?}. Requesting token..."); - let token_response = exchange_code_for_token(oidc_client, http_client, params).await?; + let token_response = exchange_code_for_token(&client.client, http_client, params).await?; log::debug!("Received token response: {token_response:?}"); let redirect_target = validate_redirect_url(state.initial_url); log::info!("Redirecting to {redirect_target} after a successful login"); let mut response = build_redirect_response(redirect_target); - set_auth_cookie(&mut response, &token_response, oidc_client, oidc_config)?; + set_auth_cookie(&mut response, &token_response, oidc_state)?; Ok(response) } @@ -449,7 +446,7 @@ async fn exchange_code_for_token( oidc_client: &OidcClient, http_client: &awc::Client, oidc_callback_params: OidcCallbackParams, -) -> anyhow::Result { +) -> anyhow::Result { let token_response = oidc_client .exchange_code(openidconnect::AuthorizationCode::new( oidc_callback_params.code, @@ -461,9 +458,8 @@ async fn exchange_code_for_token( fn set_auth_cookie( response: &mut HttpResponse, - token_response: &openidconnect::core::CoreTokenResponse, - oidc_client: &OidcClient, - oidc_config: &OidcConfig, + token_response: &OidcTokenResponse, + oidc_state: &OidcState, ) -> anyhow::Result<()> { let access_token = token_response.access_token(); log::trace!("Received access token: {}", access_token.secret()); @@ -471,9 +467,7 @@ fn set_auth_cookie( .id_token() .context("No ID token found in the token response. You may have specified an oauth2 provider that does not support OIDC.")?; - let id_token_verifier = oidc_config.create_id_token_verifier(oidc_client); - let nonce_verifier = |_nonce: Option<&Nonce>| Ok(()); // The nonce will be verified in request handling - let claims = id_token.claims(&id_token_verifier, nonce_verifier)?; + let claims = oidc_state.get_token_claims(id_token, None)?; let expiration = claims.expiration(); let max_age_seconds = expiration.signed_duration_since(Utc::now()).num_seconds(); @@ -499,11 +493,10 @@ fn set_auth_cookie( } fn build_auth_provider_redirect_response( - oidc_client: &OidcClient, - oidc_config: &OidcConfig, + oidc_state: &OidcState, request: &ServiceRequest, ) -> HttpResponse { - let AuthUrl { url, params } = build_auth_url(oidc_client, &oidc_config.scopes); + let AuthUrl { url, params } = build_auth_url(oidc_state); let state_cookie = create_state_cookie(request, params); HttpResponse::TemporaryRedirect() .append_header(("Location", url.to_string())) @@ -530,7 +523,7 @@ fn get_authenticated_user_info( .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; let state = get_state_from_cookie(request)?; - let claims = oidc_state.get_token_claims(&id_token, &state)?; + let claims = oidc_state.get_token_claims(&id_token, Some(&state))?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) } @@ -606,7 +599,30 @@ impl std::fmt::Display for AwcWrapperError { std::fmt::Display::fmt(&self.0, f) } } -type OidcClient = openidconnect::core::CoreClient< + +type OidcTokenResponse = StandardTokenResponse< + IdTokenFields< + OidcAdditionalClaims, + EmptyExtraTokenFields, + CoreGenderClaim, + CoreJweContentEncryptionAlgorithm, + CoreJwsSigningAlgorithm, + >, + CoreTokenType, +>; + +type OidcClient = openidconnect::Client< + OidcAdditionalClaims, + CoreAuthDisplay, + CoreGenderClaim, + CoreJweContentEncryptionAlgorithm, + CoreJsonWebKey, + CoreAuthPrompt, + StandardErrorResponse, + OidcTokenResponse, + CoreTokenIntrospectionResponse, + CoreRevocableToken, + CoreRevocationErrorResponse, EndpointSet, EndpointNotSet, EndpointNotSet, @@ -614,6 +630,7 @@ type OidcClient = openidconnect::core::CoreClient< EndpointMaybeSet, EndpointMaybeSet, >; + impl std::error::Error for AwcWrapperError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { self.0.source() @@ -650,12 +667,9 @@ fn make_oidc_client( ))?; } log::info!("OIDC redirect URL for {}: {redirect_url}", config.client_id); - let client = openidconnect::core::CoreClient::from_provider_metadata( - provider_metadata, - client_id, - Some(client_secret), - ) - .set_redirect_uri(redirect_url); + let client = + OidcClient::from_provider_metadata(provider_metadata, client_id, Some(client_secret)) + .set_redirect_uri(redirect_url); Ok(client) } @@ -676,10 +690,13 @@ struct AuthUrlParams { nonce: Nonce, } -fn build_auth_url(oidc_client: &OidcClient, scopes: &[Scope]) -> AuthUrl { +fn build_auth_url(oidc_state: &OidcState) -> AuthUrl { let nonce_source = Nonce::new_random(); let hashed_nonce = Nonce::new(hash_nonce(&nonce_source)); - let (url, csrf_token, _nonce) = oidc_client + let scopes = &oidc_state.config.scopes; + let client_lock = oidc_state.client.lock().unwrap(); + let (url, csrf_token, _nonce) = client_lock + .client .authorize_url( CoreAuthenticationFlow::AuthorizationCode, CsrfToken::new_random, @@ -722,9 +739,15 @@ fn hash_nonce(nonce: &Nonce) -> String { hash.to_string() } -fn check_nonce(id_token_nonce: Option<&Nonce>, state_nonce: &Nonce) -> Result<(), String> { +fn check_nonce( + id_token_nonce: Option<&Nonce>, + login_state: Option<&OidcLoginState>, +) -> Result<(), String> { + let Some(state) = login_state else { + return Ok(()); // No login state, no nonce to check + }; match id_token_nonce { - Some(id_token_nonce) => nonce_matches(id_token_nonce, state_nonce), + Some(id_token_nonce) => nonce_matches(id_token_nonce, &state.nonce), None => Err("No nonce found in the ID token".to_string()), } } From 5cf33e719cff301270a599ec73634fc3e50daf88 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 01:56:16 +0200 Subject: [PATCH 05/16] refactor: restructure OIDC middleware for better modularity and ownership - Extract request handling logic into standalone async functions - Replace service ownership with Rc for better memory management - Simplify get_client() return type to use MutexGuard directly - Add MiddlewareResponse enum to clarify response handling flow - Move public path checking to beginning of request pipeline - Reorganize code to separate concerns and improve readability --- src/webserver/oidc.rs | 144 +++++++++++++++++++++--------------------- 1 file changed, 73 insertions(+), 71 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index e1f1d6dc..cb052453 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -1,6 +1,6 @@ use std::collections::HashSet; use std::future::ready; -use std::ops::Deref; +use std::rc::Rc; use std::time::{Duration, Instant}; use std::{future::Future, pin::Pin, str::FromStr, sync::Arc}; @@ -32,7 +32,7 @@ use openidconnect::{ StandardTokenResponse, }; use serde::{Deserialize, Serialize}; -use std::sync::Mutex; +use std::sync::{Mutex, MutexGuard}; use super::http_client::make_http_client; @@ -188,7 +188,7 @@ impl OidcState { } /// Gets a reference to the oidc client, potentially generating a new one if needed - pub async fn get_client(&self) -> impl Deref + '_ { + pub async fn get_client(&self) -> MutexGuard<'_, ClientWithTime> { { let client_lock = self.client.lock().expect("oidc client"); if client_lock.last_update.elapsed() < OIDC_CLIENT_REFRESH_INTERVAL { @@ -290,7 +290,7 @@ where #[derive(Clone)] pub struct OidcService { - service: S, + service: Rc, oidc_state: Arc, } @@ -301,56 +301,72 @@ where { pub fn new(service: S, oidc_state: Arc) -> Self { Self { - service, + service: Rc::new(service), oidc_state, } } +} - fn handle_unauthenticated_request( - &self, - request: ServiceRequest, - ) -> LocalBoxFuture, Error>> { - log::debug!("Handling unauthenticated request to {}", request.path()); - if request.path() == SQLPAGE_REDIRECT_URI { - log::debug!("The request is the OIDC callback"); - return self.handle_oidc_callback(request); - } +enum MiddlewareResponse { + Forward(ServiceRequest), + Respond(ServiceResponse), +} - if self.oidc_state.config.is_public_path(request.path()) { - log::debug!( - "The request path {} is not in a public path, skipping OIDC authentication", - request.path() - ); - return Box::pin(self.service.call(request)); +async fn handle_request( + oidc_state: &OidcState, + request: ServiceRequest, +) -> actix_web::Result { + log::trace!("Started OIDC middleware request handling"); + let response = match get_authenticated_user_info(oidc_state, &request) { + Ok(Some(claims)) => { + if request.path() != SQLPAGE_REDIRECT_URI { + log::trace!("Storing authenticated user info in request extensions: {claims:?}"); + request.extensions_mut().insert(claims); + return Ok(MiddlewareResponse::Forward(request)); + } + handle_authenticated_oidc_callback(request).await } + Ok(None) => { + log::trace!("No authenticated user found"); + handle_unauthenticated_request(oidc_state, request).await + } + Err(e) => { + log::debug!("An auth cookie is present but could not be verified. Redirecting to OIDC provider to re-authenticate. {e:?}"); + handle_unauthenticated_request(oidc_state, request).await + } + }; + response.map(MiddlewareResponse::Respond) +} - log::debug!("Redirecting to OIDC provider"); - - let oidc_state = Arc::clone(&self.oidc_state); - Box::pin(async move { - let response = build_auth_provider_redirect_response(&oidc_state, &request); - Ok(request.into_response(response)) - }) +async fn handle_unauthenticated_request( + oidc_state: &OidcState, + request: ServiceRequest, +) -> Result, Error> { + log::debug!("Handling unauthenticated request to {}", request.path()); + if request.path() == SQLPAGE_REDIRECT_URI { + log::debug!("The request is the OIDC callback"); + return handle_oidc_callback(oidc_state, request).await; } - fn handle_oidc_callback( - &self, - request: ServiceRequest, - ) -> LocalBoxFuture, Error>> { - let oidc_state = Arc::clone(&self.oidc_state); + log::debug!("Redirecting to OIDC provider"); - Box::pin(async move { - let query_string = request.query_string(); - match process_oidc_callback(&oidc_state, query_string, &request).await { - Ok(response) => Ok(request.into_response(response)), - Err(e) => { - log::error!("Failed to process OIDC callback with params {query_string}: {e}"); - oidc_state.refresh().await; - let resp = build_auth_provider_redirect_response(&oidc_state, &request); - Ok(request.into_response(resp)) - } - } - }) + let response = build_auth_provider_redirect_response(oidc_state, &request); + Ok(request.into_response(response)) +} + +async fn handle_oidc_callback( + oidc_state: &OidcState, + request: ServiceRequest, +) -> Result, Error> { + let query_string = request.query_string(); + match process_oidc_callback(oidc_state, query_string, &request).await { + Ok(response) => Ok(request.into_response(response)), + Err(e) => { + log::error!("Failed to process OIDC callback with params {query_string}: {e}"); + oidc_state.refresh().await; + let resp = build_auth_provider_redirect_response(oidc_state, &request); + Ok(request.into_response(resp)) + } } } @@ -369,7 +385,7 @@ fn handle_authenticated_oidc_callback( impl Service for OidcService where - S: Service, Error = Error>, + S: Service, Error = Error> + 'static, S::Future: 'static, { type Response = ServiceResponse; @@ -379,32 +395,18 @@ where forward_ready!(service); fn call(&self, request: ServiceRequest) -> Self::Future { - log::trace!("Started OIDC middleware request handling"); - - match get_authenticated_user_info(&self.oidc_state, &request) { - Ok(Some(claims)) => { - if request.path() == SQLPAGE_REDIRECT_URI { - return handle_authenticated_oidc_callback(request); - } - log::trace!("Storing authenticated user info in request extensions: {claims:?}"); - request.extensions_mut().insert(claims); - } - Ok(None) => { - log::trace!("No authenticated user found"); - return self.handle_unauthenticated_request(request); - } - Err(e) => { - log::debug!( - "{:?}", - e.context( - "An auth cookie is present but could not be verified. \ - Redirecting to OIDC provider to re-authenticate." - ) - ); - return self.handle_unauthenticated_request(request); - } + if self.oidc_state.config.is_public_path(request.path()) { + return Box::pin(self.service.call(request)); } - Box::pin(self.service.call(request)) + let srv = Rc::clone(&self.service); + let oidc_state = Arc::clone(&self.oidc_state); + Box::pin(async move { + match handle_request(&oidc_state, request).await { + Ok(MiddlewareResponse::Respond(response)) => Ok(response), + Ok(MiddlewareResponse::Forward(request)) => srv.call(request).await, + Err(err) => Err(err), + } + }) } } @@ -512,7 +514,7 @@ fn build_redirect_response(target_url: String) -> HttpResponse { /// Returns the claims from the ID token in the `SQLPage` auth cookie. fn get_authenticated_user_info( - oidc_state: &Arc, + oidc_state: &OidcState, request: &ServiceRequest, ) -> anyhow::Result> { let Some(cookie) = request.cookie(SQLPAGE_AUTH_COOKIE_NAME) else { From 45540e7994dc6039bf3df84ea8cdd3147e753eac Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 11:12:05 +0200 Subject: [PATCH 06/16] Replace Mutex with RwLock for OIDC client This change switches from std::sync::Mutex to tokio::sync::RwLock for the OIDC client to avoid deadlocks. The read operations now use a shared lock while writes remain exclusive. --- src/webserver/oidc.rs | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index cb052453..e613e201 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -32,7 +32,7 @@ use openidconnect::{ StandardTokenResponse, }; use serde::{Deserialize, Serialize}; -use std::sync::{Mutex, MutexGuard}; +use tokio::sync::{RwLock, RwLockReadGuard}; use super::http_client::make_http_client; @@ -151,7 +151,7 @@ pub struct ClientWithTime { pub struct OidcState { pub config: OidcConfig, app_config: AppConfig, - client: Mutex, + client: RwLock, } impl OidcState { @@ -162,7 +162,7 @@ impl OidcState { Ok(Self { config: oidc_cfg, app_config, - client: Mutex::new(ClientWithTime { + client: RwLock::new(ClientWithTime { client, last_update: Instant::now(), }), @@ -174,9 +174,10 @@ impl OidcState { log::error!("Failed to create HTTP client"); return; }; + let mut write_guard = self.client.write().await; match build_oidc_client(&self.config, &http_client).await { Ok(client) => { - *self.client.lock().expect("oidc client") = ClientWithTime { + *write_guard = ClientWithTime { client, last_update: Instant::now(), }; @@ -188,26 +189,26 @@ impl OidcState { } /// Gets a reference to the oidc client, potentially generating a new one if needed - pub async fn get_client(&self) -> MutexGuard<'_, ClientWithTime> { + pub async fn get_client(&self) -> RwLockReadGuard<'_, ClientWithTime> { { - let client_lock = self.client.lock().expect("oidc client"); + let client_lock = self.client.read().await; if client_lock.last_update.elapsed() < OIDC_CLIENT_REFRESH_INTERVAL { return client_lock; } } log::debug!("OIDC client is older than {OIDC_CLIENT_REFRESH_INTERVAL:?}, refreshing..."); self.refresh().await; - self.client.lock().expect("oidc client") + self.client.read().await } /// Validate and decode the claims of an OIDC token, without refreshing the client. - fn get_token_claims( + async fn get_token_claims( &self, id_token: &OidcToken, state: Option<&OidcLoginState>, ) -> anyhow::Result { // Do not refresh the client on every check - let client = &self.client.lock().expect("oidc client").client; + let client = &self.client.read().await.client; let verifier = self.config.create_id_token_verifier(client); let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, state); let claims: OidcClaims = id_token @@ -317,7 +318,7 @@ async fn handle_request( request: ServiceRequest, ) -> actix_web::Result { log::trace!("Started OIDC middleware request handling"); - let response = match get_authenticated_user_info(oidc_state, &request) { + let response = match get_authenticated_user_info(oidc_state, &request).await { Ok(Some(claims)) => { if request.path() != SQLPAGE_REDIRECT_URI { log::trace!("Storing authenticated user info in request extensions: {claims:?}"); @@ -350,7 +351,7 @@ async fn handle_unauthenticated_request( log::debug!("Redirecting to OIDC provider"); - let response = build_auth_provider_redirect_response(oidc_state, &request); + let response = build_auth_provider_redirect_response(oidc_state, &request).await; Ok(request.into_response(response)) } @@ -364,7 +365,7 @@ async fn handle_oidc_callback( Err(e) => { log::error!("Failed to process OIDC callback with params {query_string}: {e}"); oidc_state.refresh().await; - let resp = build_auth_provider_redirect_response(oidc_state, &request); + let resp = build_auth_provider_redirect_response(oidc_state, &request).await; Ok(request.into_response(resp)) } } @@ -440,7 +441,7 @@ async fn process_oidc_callback( let redirect_target = validate_redirect_url(state.initial_url); log::info!("Redirecting to {redirect_target} after a successful login"); let mut response = build_redirect_response(redirect_target); - set_auth_cookie(&mut response, &token_response, oidc_state)?; + set_auth_cookie(&mut response, &token_response, oidc_state).await?; Ok(response) } @@ -458,7 +459,7 @@ async fn exchange_code_for_token( Ok(token_response) } -fn set_auth_cookie( +async fn set_auth_cookie( response: &mut HttpResponse, token_response: &OidcTokenResponse, oidc_state: &OidcState, @@ -469,7 +470,7 @@ fn set_auth_cookie( .id_token() .context("No ID token found in the token response. You may have specified an oauth2 provider that does not support OIDC.")?; - let claims = oidc_state.get_token_claims(id_token, None)?; + let claims = oidc_state.get_token_claims(id_token, None).await?; let expiration = claims.expiration(); let max_age_seconds = expiration.signed_duration_since(Utc::now()).num_seconds(); @@ -494,11 +495,11 @@ fn set_auth_cookie( Ok(()) } -fn build_auth_provider_redirect_response( +async fn build_auth_provider_redirect_response( oidc_state: &OidcState, request: &ServiceRequest, ) -> HttpResponse { - let AuthUrl { url, params } = build_auth_url(oidc_state); + let AuthUrl { url, params } = build_auth_url(oidc_state).await; let state_cookie = create_state_cookie(request, params); HttpResponse::TemporaryRedirect() .append_header(("Location", url.to_string())) @@ -513,7 +514,7 @@ fn build_redirect_response(target_url: String) -> HttpResponse { } /// Returns the claims from the ID token in the `SQLPage` auth cookie. -fn get_authenticated_user_info( +async fn get_authenticated_user_info( oidc_state: &OidcState, request: &ServiceRequest, ) -> anyhow::Result> { @@ -525,7 +526,7 @@ fn get_authenticated_user_info( .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; let state = get_state_from_cookie(request)?; - let claims = oidc_state.get_token_claims(&id_token, Some(&state))?; + let claims = oidc_state.get_token_claims(&id_token, Some(&state)).await?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) } @@ -692,11 +693,11 @@ struct AuthUrlParams { nonce: Nonce, } -fn build_auth_url(oidc_state: &OidcState) -> AuthUrl { +async fn build_auth_url(oidc_state: &OidcState) -> AuthUrl { let nonce_source = Nonce::new_random(); let hashed_nonce = Nonce::new(hash_nonce(&nonce_source)); let scopes = &oidc_state.config.scopes; - let client_lock = oidc_state.client.lock().unwrap(); + let client_lock = oidc_state.get_client().await; let (url, csrf_token, _nonce) = client_lock .client .authorize_url( From b0ca79d5a1eb12b047d565056c98b4ce034497c1 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 11:15:20 +0200 Subject: [PATCH 07/16] Remove state refresh on OIDC callback error --- src/webserver/oidc.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index e613e201..93241c48 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -364,7 +364,6 @@ async fn handle_oidc_callback( Ok(response) => Ok(request.into_response(response)), Err(e) => { log::error!("Failed to process OIDC callback with params {query_string}: {e}"); - oidc_state.refresh().await; let resp = build_auth_provider_redirect_response(oidc_state, &request).await; Ok(request.into_response(resp)) } From 4578719f97853e2f2fe1cf6bef38e631047f7762 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 11:19:15 +0200 Subject: [PATCH 08/16] OIDC client wrapper exposure --- src/webserver/oidc.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 93241c48..cd522b90 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -189,16 +189,17 @@ impl OidcState { } /// Gets a reference to the oidc client, potentially generating a new one if needed - pub async fn get_client(&self) -> RwLockReadGuard<'_, ClientWithTime> { + pub async fn get_client(&self) -> RwLockReadGuard<'_, OidcClient> { { let client_lock = self.client.read().await; if client_lock.last_update.elapsed() < OIDC_CLIENT_REFRESH_INTERVAL { - return client_lock; + return RwLockReadGuard::map(client_lock, |ClientWithTime { client, .. }| client); } } log::debug!("OIDC client is older than {OIDC_CLIENT_REFRESH_INTERVAL:?}, refreshing..."); self.refresh().await; - self.client.read().await + let with_time = self.client.read().await; + RwLockReadGuard::map(with_time, |ClientWithTime { client, .. }| client) } /// Validate and decode the claims of an OIDC token, without refreshing the client. @@ -434,7 +435,7 @@ async fn process_oidc_callback( let client = oidc_state.get_client().await; log::debug!("Processing OIDC callback with params: {params:?}. Requesting token..."); - let token_response = exchange_code_for_token(&client.client, http_client, params).await?; + let token_response = exchange_code_for_token(&client, http_client, params).await?; log::debug!("Received token response: {token_response:?}"); let redirect_target = validate_redirect_url(state.initial_url); @@ -698,7 +699,6 @@ async fn build_auth_url(oidc_state: &OidcState) -> AuthUrl { let scopes = &oidc_state.config.scopes; let client_lock = oidc_state.get_client().await; let (url, csrf_token, _nonce) = client_lock - .client .authorize_url( CoreAuthenticationFlow::AuthorizationCode, CsrfToken::new_random, From 1667de9eead5be8c4eac5f78a6a5aca56aa81faa Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 11:40:23 +0200 Subject: [PATCH 09/16] Refactor OIDC client refresh mechanism Moves client refresh logic into a separate method and uses request-scoped HTTP client instead of storing AppConfig. This simplifies the state struct and improves the refresh mechanism's reliability. --- src/webserver/oidc.rs | 57 ++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index cd522b90..dd7d1492 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -150,7 +150,6 @@ pub struct ClientWithTime { pub struct OidcState { pub config: OidcConfig, - app_config: AppConfig, client: RwLock, } @@ -161,7 +160,6 @@ impl OidcState { Ok(Self { config: oidc_cfg, - app_config, client: RwLock::new(ClientWithTime { client, last_update: Instant::now(), @@ -169,37 +167,32 @@ impl OidcState { }) } - async fn refresh(&self) { - let Ok(http_client) = make_http_client(&self.app_config) else { - log::error!("Failed to create HTTP client"); - return; - }; - let mut write_guard = self.client.write().await; - match build_oidc_client(&self.config, &http_client).await { - Ok(client) => { - *write_guard = ClientWithTime { - client, + async fn refresh(&self, service_request: &ServiceRequest) { + match build_oidc_client_from_appdata(&self.config, service_request).await { + Ok(http_client) => { + *self.client.write().await = ClientWithTime { + client: http_client, last_update: Instant::now(), - }; - } - Err(e) => { - log::error!("Failed to refresh OIDC client: {e}"); + } } + Err(e) => log::error!("Failed to refresh OIDC client: {e}"), + } + } + + /// Refreshes the OIDC client from the provider metadata URL if it has expired. + /// Most providers update their signing keys periodically. + pub async fn refresh_if_expired(&self, service_request: &ServiceRequest) { + if self.client.read().await.last_update.elapsed() > OIDC_CLIENT_REFRESH_INTERVAL { + self.refresh(service_request).await; } } /// Gets a reference to the oidc client, potentially generating a new one if needed pub async fn get_client(&self) -> RwLockReadGuard<'_, OidcClient> { - { - let client_lock = self.client.read().await; - if client_lock.last_update.elapsed() < OIDC_CLIENT_REFRESH_INTERVAL { - return RwLockReadGuard::map(client_lock, |ClientWithTime { client, .. }| client); - } - } - log::debug!("OIDC client is older than {OIDC_CLIENT_REFRESH_INTERVAL:?}, refreshing..."); - self.refresh().await; - let with_time = self.client.read().await; - RwLockReadGuard::map(with_time, |ClientWithTime { client, .. }| client) + RwLockReadGuard::map( + self.client.read().await, + |ClientWithTime { client, .. }| client, + ) } /// Validate and decode the claims of an OIDC token, without refreshing the client. @@ -208,8 +201,7 @@ impl OidcState { id_token: &OidcToken, state: Option<&OidcLoginState>, ) -> anyhow::Result { - // Do not refresh the client on every check - let client = &self.client.read().await.client; + let client = &self.get_client().await; let verifier = self.config.create_id_token_verifier(client); let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, state); let claims: OidcClaims = id_token @@ -234,6 +226,14 @@ pub async fn initialize_oidc_state( ))) } +async fn build_oidc_client_from_appdata( + cfg: &OidcConfig, + req: &ServiceRequest, +) -> anyhow::Result { + let http_client = get_http_client_from_appdata(req)?; + build_oidc_client(cfg, http_client).await +} + async fn build_oidc_client( oidc_cfg: &OidcConfig, http_client: &Client, @@ -319,6 +319,7 @@ async fn handle_request( request: ServiceRequest, ) -> actix_web::Result { log::trace!("Started OIDC middleware request handling"); + oidc_state.refresh_if_expired(&request).await; let response = match get_authenticated_user_info(oidc_state, &request).await { Ok(Some(claims)) => { if request.path() != SQLPAGE_REDIRECT_URI { From fd2335804613dda690705f80e9d2b7dcdd85a6eb Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 11:43:32 +0200 Subject: [PATCH 10/16] Fix multiple OIDC client refresh --- src/webserver/oidc.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index dd7d1492..085cac5d 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -16,6 +16,7 @@ use actix_web::{ }; use anyhow::{anyhow, Context}; use awc::Client; +use base64::write; use chrono::Utc; use openidconnect::core::{ CoreAuthDisplay, CoreAuthPrompt, CoreErrorResponseType, CoreGenderClaim, CoreJsonWebKey, @@ -168,9 +169,11 @@ impl OidcState { } async fn refresh(&self, service_request: &ServiceRequest) { + // Obtain a write lock to prevent concurrent OIDC client refreshes. + let mut write_guard = self.client.write().await; match build_oidc_client_from_appdata(&self.config, service_request).await { Ok(http_client) => { - *self.client.write().await = ClientWithTime { + *write_guard = ClientWithTime { client: http_client, last_update: Instant::now(), } From 7e0d067b848d9d6ea832df384832712d74251c2f Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 12:13:24 +0200 Subject: [PATCH 11/16] Refactor OIDC authentication flow and error handling --- src/webserver/oidc.rs | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 085cac5d..9085d3f0 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -16,7 +16,6 @@ use actix_web::{ }; use anyhow::{anyhow, Context}; use awc::Client; -use base64::write; use chrono::Utc; use openidconnect::core::{ CoreAuthDisplay, CoreAuthPrompt, CoreErrorResponseType, CoreGenderClaim, CoreJsonWebKey, @@ -323,14 +322,15 @@ async fn handle_request( ) -> actix_web::Result { log::trace!("Started OIDC middleware request handling"); oidc_state.refresh_if_expired(&request).await; - let response = match get_authenticated_user_info(oidc_state, &request).await { + match get_authenticated_user_info(oidc_state, &request).await { Ok(Some(claims)) => { if request.path() != SQLPAGE_REDIRECT_URI { log::trace!("Storing authenticated user info in request extensions: {claims:?}"); request.extensions_mut().insert(claims); return Ok(MiddlewareResponse::Forward(request)); } - handle_authenticated_oidc_callback(request).await + let response = handle_authenticated_oidc_callback(request); + Ok(MiddlewareResponse::Respond(response)) } Ok(None) => { log::trace!("No authenticated user found"); @@ -340,24 +340,28 @@ async fn handle_request( log::debug!("An auth cookie is present but could not be verified. Redirecting to OIDC provider to re-authenticate. {e:?}"); handle_unauthenticated_request(oidc_state, request).await } - }; - response.map(MiddlewareResponse::Respond) + } } async fn handle_unauthenticated_request( oidc_state: &OidcState, request: ServiceRequest, -) -> Result, Error> { +) -> actix_web::Result { log::debug!("Handling unauthenticated request to {}", request.path()); if request.path() == SQLPAGE_REDIRECT_URI { log::debug!("The request is the OIDC callback"); - return handle_oidc_callback(oidc_state, request).await; + let response = handle_oidc_callback(oidc_state, request).await?; + return Ok(MiddlewareResponse::Respond(response)); + } + + if oidc_state.config.is_public_path(request.path()) { + return Ok(MiddlewareResponse::Forward(request)); } log::debug!("Redirecting to OIDC provider"); let response = build_auth_provider_redirect_response(oidc_state, &request).await; - Ok(request.into_response(response)) + Ok(MiddlewareResponse::Respond(request.into_response(response))) } async fn handle_oidc_callback( @@ -376,16 +380,13 @@ async fn handle_oidc_callback( } /// When an user has already authenticated (potentially in another tab), we ignore the callback and redirect to the initial URL. -fn handle_authenticated_oidc_callback( - request: ServiceRequest, -) -> LocalBoxFuture, Error>> { +fn handle_authenticated_oidc_callback(request: ServiceRequest) -> ServiceResponse { let redirect_url = match get_state_from_cookie(&request) { Ok(state) => state.initial_url, Err(_) => "/".to_string(), }; log::debug!("OIDC callback received for authenticated user. Redirecting to {redirect_url}"); - let response = request.into_response(build_redirect_response(redirect_url)); - Box::pin(ready(Ok(response))) + request.into_response(build_redirect_response(redirect_url)) } impl Service for OidcService @@ -400,9 +401,6 @@ where forward_ready!(service); fn call(&self, request: ServiceRequest) -> Self::Future { - if self.oidc_state.config.is_public_path(request.path()) { - return Box::pin(self.service.call(request)); - } let srv = Rc::clone(&self.service); let oidc_state = Arc::clone(&self.oidc_state); Box::pin(async move { From e8b604d583d57ff0e14d583e8951f1e91be108c3 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 12:31:14 +0200 Subject: [PATCH 12/16] Change OIDC token verification to take ownership --- src/webserver/oidc.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 9085d3f0..779777e1 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -200,16 +200,15 @@ impl OidcState { /// Validate and decode the claims of an OIDC token, without refreshing the client. async fn get_token_claims( &self, - id_token: &OidcToken, + id_token: OidcToken, state: Option<&OidcLoginState>, ) -> anyhow::Result { let client = &self.get_client().await; let verifier = self.config.create_id_token_verifier(client); let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, state); let claims: OidcClaims = id_token - .claims(&verifier, nonce_verifier) - .with_context(|| format!("Could not verify the ID token: {id_token:?}"))? - .clone(); + .into_claims(&verifier, nonce_verifier) + .with_context(|| format!("Could not verify the ID token"))?; Ok(claims) } } @@ -472,7 +471,7 @@ async fn set_auth_cookie( .id_token() .context("No ID token found in the token response. You may have specified an oauth2 provider that does not support OIDC.")?; - let claims = oidc_state.get_token_claims(id_token, None).await?; + let claims = oidc_state.get_token_claims(id_token.clone(), None).await?; let expiration = claims.expiration(); let max_age_seconds = expiration.signed_duration_since(Utc::now()).num_seconds(); @@ -528,7 +527,8 @@ async fn get_authenticated_user_info( .with_context(|| format!("Invalid SQLPage auth cookie: {cookie_value:?}"))?; let state = get_state_from_cookie(request)?; - let claims = oidc_state.get_token_claims(&id_token, Some(&state)).await?; + log::debug!("Verifying id token: {id_token:?}"); + let claims = oidc_state.get_token_claims(id_token, Some(&state)).await?; log::debug!("The current user is: {claims:?}"); Ok(Some(claims)) } From 7189cc157c351456a0b0c56a5588928fd86b21af Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 12:37:31 +0200 Subject: [PATCH 13/16] Improve ID token error message with details --- src/webserver/oidc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 779777e1..baddd287 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -208,7 +208,7 @@ impl OidcState { let nonce_verifier = |nonce: Option<&Nonce>| check_nonce(nonce, state); let claims: OidcClaims = id_token .into_claims(&verifier, nonce_verifier) - .with_context(|| format!("Could not verify the ID token"))?; + .map_err(|e| anyhow::anyhow!("Could not verify the ID token: {}", e))?; Ok(claims) } } From b96eb10a243c52c2571a4204b961f267a9cf2c57 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 14:06:00 +0200 Subject: [PATCH 14/16] Add fast OIDC client refresh on auth errors --- src/webserver/oidc.rs | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index baddd287..eb136729 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -41,7 +41,8 @@ type LocalBoxFuture = Pin + 'static>>; const SQLPAGE_AUTH_COOKIE_NAME: &str = "sqlpage_auth"; const SQLPAGE_REDIRECT_URI: &str = "/sqlpage/oidc_callback"; const SQLPAGE_STATE_COOKIE_NAME: &str = "sqlpage_oidc_state"; -const OIDC_CLIENT_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60); +const OIDC_CLIENT_MAX_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60); +const OIDC_CLIENT_MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5); #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(transparent)] @@ -184,7 +185,14 @@ impl OidcState { /// Refreshes the OIDC client from the provider metadata URL if it has expired. /// Most providers update their signing keys periodically. pub async fn refresh_if_expired(&self, service_request: &ServiceRequest) { - if self.client.read().await.last_update.elapsed() > OIDC_CLIENT_REFRESH_INTERVAL { + if self.client.read().await.last_update.elapsed() > OIDC_CLIENT_MAX_REFRESH_INTERVAL { + self.refresh(service_request).await; + } + } + + /// When an authentication error is encountered, refresh the OIDC client info faster + pub async fn refresh_on_error(&self, service_request: &ServiceRequest) { + if self.client.read().await.last_update.elapsed() > OIDC_CLIENT_MIN_REFRESH_INTERVAL { self.refresh(service_request).await; } } @@ -337,6 +345,7 @@ async fn handle_request( } Err(e) => { log::debug!("An auth cookie is present but could not be verified. Redirecting to OIDC provider to re-authenticate. {e:?}"); + oidc_state.refresh_on_error(&request).await; handle_unauthenticated_request(oidc_state, request).await } } @@ -419,7 +428,7 @@ async fn process_oidc_callback( ) -> anyhow::Result { let http_client = get_http_client_from_appdata(request)?; - let state = get_state_from_cookie(request)?; + let state = get_state_from_cookie(request).context("Failed to read oidc state cookie")?; let params = Query::::from_query(query_string) .with_context(|| { @@ -442,7 +451,9 @@ async fn process_oidc_callback( let redirect_target = validate_redirect_url(state.initial_url); log::info!("Redirecting to {redirect_target} after a successful login"); let mut response = build_redirect_response(redirect_target); - set_auth_cookie(&mut response, &token_response, oidc_state).await?; + set_auth_cookie(&mut response, &token_response, oidc_state) + .await + .context("Failed to set auth cookie")?; Ok(response) } @@ -456,7 +467,8 @@ async fn exchange_code_for_token( oidc_callback_params.code, ))? .request_async(&AwcHttpClient::from_client(http_client)) - .await?; + .await + .context("Failed to exchange code for token")?; Ok(token_response) } @@ -471,7 +483,10 @@ async fn set_auth_cookie( .id_token() .context("No ID token found in the token response. You may have specified an oauth2 provider that does not support OIDC.")?; - let claims = oidc_state.get_token_claims(id_token.clone(), None).await?; + let claims = oidc_state + .get_token_claims(id_token.clone(), None) + .await + .context("Invalid token returned by OIDC provider")?; let expiration = claims.expiration(); let max_age_seconds = expiration.signed_duration_since(Utc::now()).num_seconds(); From 630e007e5ac4794b5e2b8f5fd8e4b0b72e153a90 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 14:40:50 +0200 Subject: [PATCH 15/16] Fix OIDC SSO redirect loop after provider key rotation This commit adds automatic OIDC provider metadata refresh when token validation fails. Previously, tokens would become invalid when providers rotated their signing keys, requiring a manual SQLPage restart to refresh the metadata. --- CHANGELOG.md | 1 + src/webserver/oidc.rs | 16 +++++++--------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c08b7ec..beff7b22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ - The file-based routing system was improved. Now, requests to `/xxx` redirect to `/xxx/` only if `/xxx/index.sql` exists. - fix: When single sign on is enabled, and an anonymous user visits a page with URL parameters, the user is correctly redirected to the page with the parameters after login. - Added support for reading custom claims in JWT tokens generated by OIDC providers. This means that you can configure your Single-Sign-On provider to store custom pieces of information about your users, such as roles or permissions, and use them in your SQL queries in SQLPage. +- Implement OIDC provider metadata refresh. This fixes a bug where leaving a SQLPage instance running with SSO enabled would cause infinite redirect loops on login after some time. Since most providers rotate their signing keys regularly and sqlpage only fetched the metadata once at startup, the only way to fix the issue was to restart SQLPage manually. ## v0.35.2 - Fix a bug with zero values being displayed with a non-zero height in stacked bar charts. diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index eb136729..59181403 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -178,7 +178,7 @@ impl OidcState { last_update: Instant::now(), } } - Err(e) => log::error!("Failed to refresh OIDC client: {e}"), + Err(e) => log::error!("Failed to refresh OIDC client: {e:#}"), } } @@ -375,12 +375,13 @@ async fn handle_unauthenticated_request( async fn handle_oidc_callback( oidc_state: &OidcState, request: ServiceRequest, -) -> Result, Error> { +) -> actix_web::Result { let query_string = request.query_string(); match process_oidc_callback(oidc_state, query_string, &request).await { Ok(response) => Ok(request.into_response(response)), Err(e) => { - log::error!("Failed to process OIDC callback with params {query_string}: {e}"); + log::error!("Failed to process OIDC callback with params {query_string}: {e:#}"); + oidc_state.refresh_on_error(&request).await; let resp = build_auth_provider_redirect_response(oidc_state, &request).await; Ok(request.into_response(resp)) } @@ -483,11 +484,8 @@ async fn set_auth_cookie( .id_token() .context("No ID token found in the token response. You may have specified an oauth2 provider that does not support OIDC.")?; - let claims = oidc_state - .get_token_claims(id_token.clone(), None) - .await - .context("Invalid token returned by OIDC provider")?; - let expiration = claims.expiration(); + let claims_res = oidc_state.get_token_claims(id_token.clone(), None).await; + let expiration = claims_res.context("Parsing ID token claims")?.expiration(); let max_age_seconds = expiration.signed_duration_since(Utc::now()).num_seconds(); let id_token_str = id_token.to_string(); @@ -847,7 +845,7 @@ impl AudienceVerifier { /// Validate that a redirect URL is safe to use (prevents open redirect attacks) fn validate_redirect_url(url: String) -> String { - if url.starts_with('/') && !url.starts_with("//") { + if url.starts_with('/') && !url.starts_with("//") && !url.starts_with(SQLPAGE_REDIRECT_URI) { return url; } log::warn!("Refusing to redirect to {url}"); From 78fa85f942046a701b111d31fe1a4a514f7bc5f6 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Fri, 1 Aug 2025 16:11:14 +0200 Subject: [PATCH 16/16] Fix OIDC flow: Redirect to initial URL on error - Store initial URL in OidcLoginState. - Use initial URL when building redirect response on callback error. --- src/webserver/oidc.rs | 51 +++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 59181403..b7fdb8ac 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -368,7 +368,8 @@ async fn handle_unauthenticated_request( log::debug!("Redirecting to OIDC provider"); - let response = build_auth_provider_redirect_response(oidc_state, &request).await; + let initial_url = request.uri().to_string(); + let response = build_auth_provider_redirect_response(oidc_state, initial_url).await; Ok(MiddlewareResponse::Respond(request.into_response(response))) } @@ -380,9 +381,11 @@ async fn handle_oidc_callback( match process_oidc_callback(oidc_state, query_string, &request).await { Ok(response) => Ok(request.into_response(response)), Err(e) => { - log::error!("Failed to process OIDC callback with params {query_string}: {e:#}"); + let redirect_url = + get_state_from_cookie(&request).map_or_else(|_| "/".into(), |s| s.initial_url); + log::error!("Failed to process OIDC callback. Refreshing oidc provider metadata, then redirecting to {redirect_url}: {e:#}"); oidc_state.refresh_on_error(&request).await; - let resp = build_auth_provider_redirect_response(oidc_state, &request).await; + let resp = build_auth_provider_redirect_response(oidc_state, redirect_url).await; Ok(request.into_response(resp)) } } @@ -511,10 +514,11 @@ async fn set_auth_cookie( async fn build_auth_provider_redirect_response( oidc_state: &OidcState, - request: &ServiceRequest, + initial_url: String, ) -> HttpResponse { let AuthUrl { url, params } = build_auth_url(oidc_state).await; - let state_cookie = create_state_cookie(request, params); + let state = OidcLoginState::new(initial_url, params); + let state_cookie = create_state_cookie(&state); HttpResponse::TemporaryRedirect() .append_header(("Location", url.to_string())) .cookie(state_cookie) @@ -730,20 +734,6 @@ async fn build_auth_url(oidc_state: &OidcState) -> AuthUrl { } } -#[derive(Debug, Serialize, Deserialize)] -struct OidcLoginState { - /// The URL to redirect to after the login process is complete. - #[serde(rename = "u")] - initial_url: String, - /// The CSRF token to use for the login process. - #[serde(rename = "c")] - csrf_token: CsrfToken, - /// The source nonce to use for the login process. It must be checked against the hash - /// stored in the ID token. - #[serde(rename = "n")] - nonce: Nonce, -} - fn hash_nonce(nonce: &Nonce) -> String { use argon2::password_hash::{rand_core::OsRng, PasswordHasher, SaltString}; let salt = SaltString::generate(&mut OsRng); @@ -791,19 +781,32 @@ fn nonce_matches(id_token_nonce: &Nonce, state_nonce: &Nonce) -> Result<(), Stri Ok(()) } +#[derive(Debug, Serialize, Deserialize)] +struct OidcLoginState { + /// The URL to redirect to after the login process is complete. + #[serde(rename = "u")] + initial_url: String, + /// The CSRF token to use for the login process. + #[serde(rename = "c")] + csrf_token: CsrfToken, + /// The source nonce to use for the login process. It must be checked against the hash + /// stored in the ID token. + #[serde(rename = "n")] + nonce: Nonce, +} + impl OidcLoginState { - fn new(request: &ServiceRequest, auth_url: AuthUrlParams) -> Self { + fn new(initial_url: String, auth_url: AuthUrlParams) -> Self { Self { - initial_url: request.uri().to_string(), + initial_url, csrf_token: auth_url.csrf_token, nonce: auth_url.nonce, } } } -fn create_state_cookie(request: &ServiceRequest, auth_url: AuthUrlParams) -> Cookie { - let state = OidcLoginState::new(request, auth_url); - let state_json = serde_json::to_string(&state).unwrap(); +fn create_state_cookie(login_state: &OidcLoginState) -> Cookie { + let state_json = serde_json::to_string(login_state).unwrap(); Cookie::build(SQLPAGE_STATE_COOKIE_NAME, state_json) .secure(true) .http_only(true)