From 72d78282ccadacb0850c5ea97f3ee027198de1be Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 28 Jul 2025 23:14:20 +0000 Subject: [PATCH 1/9] Checkpoint before follow-up message --- src/webserver/oidc.rs | 201 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 183 insertions(+), 18 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index d4e53ef0..30519d03 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -1,5 +1,5 @@ use std::future::ready; -use std::{future::Future, pin::Pin, str::FromStr, sync::Arc}; +use std::{future::Future, pin::Pin, str::FromStr, sync::Arc, time::{Duration, Instant}}; use crate::webserver::http_client::get_http_client_from_appdata; use crate::{app_config::AppConfig, AppState}; @@ -20,6 +20,7 @@ use openidconnect::{ TokenResponse, }; use serde::{Deserialize, Serialize}; +use tokio::sync::RwLock; use super::http_client::make_http_client; @@ -29,6 +30,58 @@ 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"; +// Cache configuration based on industry best practices +const PROVIDER_METADATA_CACHE_DURATION: Duration = Duration::from_secs(24 * 60 * 60); // 24 hours +const PROVIDER_METADATA_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60); // 1 hour +const MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5 * 60); // 5 minutes (rate limiting) + +#[derive(Clone, Debug)] +struct CachedProviderMetadata { + metadata: openidconnect::core::CoreProviderMetadata, + cached_at: Instant, + last_refresh_attempt: Option, +} + +impl CachedProviderMetadata { + fn new(metadata: openidconnect::core::CoreProviderMetadata) -> Self { + Self { + metadata, + cached_at: Instant::now(), + last_refresh_attempt: None, + } + } + + fn is_expired(&self) -> bool { + self.cached_at.elapsed() > PROVIDER_METADATA_CACHE_DURATION + } + + fn should_refresh(&self) -> bool { + // Refresh if the cache is older than refresh interval + if self.cached_at.elapsed() > PROVIDER_METADATA_REFRESH_INTERVAL { + return true; + } + false + } + + fn can_attempt_refresh(&self) -> bool { + // Rate limit refresh attempts to prevent excessive requests + match self.last_refresh_attempt { + Some(last_attempt) => last_attempt.elapsed() > MIN_REFRESH_INTERVAL, + None => true, + } + } + + fn mark_refresh_attempt(&mut self) { + self.last_refresh_attempt = Some(Instant::now()); + } + + fn update_metadata(&mut self, metadata: openidconnect::core::CoreProviderMetadata) { + self.metadata = metadata; + self.cached_at = Instant::now(); + self.last_refresh_attempt = None; + } +} + #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(transparent)] pub struct OidcAdditionalClaims(pub(crate) serde_json::Map); @@ -117,7 +170,107 @@ fn get_app_host(config: &AppConfig) -> String { pub struct OidcState { pub config: Arc, - pub client: Arc, + client: Arc>, + cached_metadata: Arc>>, + http_client: Arc, +} + +impl OidcState { + /// Get the current OIDC client, refreshing if necessary + pub async fn get_client(&self) -> Arc { + let should_refresh = { + let cache = self.cached_metadata.read().await; + match cache.as_ref() { + Some(cached) => cached.is_expired() || (cached.should_refresh() && cached.can_attempt_refresh()), + None => true, + } + }; + + if should_refresh { + if let Err(e) = self.refresh_provider_metadata().await { + log::warn!("Failed to refresh OIDC provider metadata: {}", e); + // Continue with current client if available + } + } + + Arc::new(self.client.read().await.clone()) + } + + /// Get the current provider metadata, refreshing if necessary + pub async fn get_provider_metadata(&self) -> anyhow::Result { + let should_refresh = { + let cache = self.cached_metadata.read().await; + match cache.as_ref() { + Some(cached) => cached.is_expired() || (cached.should_refresh() && cached.can_attempt_refresh()), + None => true, + } + }; + + if should_refresh { + if let Err(e) = self.refresh_provider_metadata().await { + log::warn!("Failed to refresh OIDC provider metadata: {}", e); + // Continue with cached data if available + } + } + + let cache = self.cached_metadata.read().await; + match cache.as_ref() { + Some(cached) if !cached.is_expired() => Ok(cached.metadata.clone()), + Some(cached) => { + log::warn!("OIDC provider metadata cache has expired, but refresh failed. Using stale data."); + Ok(cached.metadata.clone()) + } + None => Err(anyhow!("No OIDC provider metadata available and refresh failed")), + } + } + + /// Refresh provider metadata from the OIDC provider + pub async fn refresh_provider_metadata(&self) -> anyhow::Result<()> { + // Mark refresh attempt to prevent excessive requests + { + let mut cache = self.cached_metadata.write().await; + if let Some(cached) = cache.as_mut() { + if !cached.can_attempt_refresh() { + return Err(anyhow!("Rate limited: too soon since last refresh attempt")); + } + cached.mark_refresh_attempt(); + } + } + + log::debug!("Refreshing OIDC provider metadata for {}", self.config.issuer_url); + + let new_metadata = discover_provider_metadata(&self.http_client, self.config.issuer_url.clone()).await?; + + // Create new client with updated metadata + let new_client = make_oidc_client(&self.config, new_metadata.clone())?; + + // Update both cache and client atomically + let mut cache = self.cached_metadata.write().await; + let mut client = self.client.write().await; + + match cache.as_mut() { + Some(cached) => cached.update_metadata(new_metadata), + None => *cache = Some(CachedProviderMetadata::new(new_metadata)), + } + + *client = new_client; + + log::debug!("Successfully refreshed OIDC provider metadata and client"); + Ok(()) + } + + /// Start background task to periodically refresh metadata + pub fn start_background_refresh(state: Arc) { + tokio::spawn(async move { + let mut interval = tokio::time::interval(PROVIDER_METADATA_REFRESH_INTERVAL); + loop { + interval.tick().await; + if let Err(e) = state.refresh_provider_metadata().await { + log::warn!("Background refresh of OIDC provider metadata failed: {}", e); + } + } + }); + } } pub async fn initialize_oidc_state( @@ -129,15 +282,23 @@ 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)?; + let http_client = Arc::new(make_http_client(app_config)?); + + // Initial metadata discovery + let provider_metadata = discover_provider_metadata(&http_client, oidc_cfg.issuer_url.clone()).await?; + let client = make_oidc_client(&oidc_cfg, provider_metadata.clone())?; - Ok(Some(Arc::new(OidcState { + let oidc_state = Arc::new(OidcState { config: oidc_cfg, - client: Arc::new(client), - }))) + client: Arc::new(RwLock::new(client)), + cached_metadata: Arc::new(RwLock::new(Some(CachedProviderMetadata::new(provider_metadata)))), + http_client, + }); + + // Start background refresh task + OidcState::start_background_refresh(Arc::clone(&oidc_state)); + + Ok(Some(oidc_state)) } pub struct OidcMiddleware { @@ -224,29 +385,33 @@ 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 client = oidc_state.get_client().await; + let response = build_auth_provider_redirect_response( + &client, + &oidc_state.config, + &request, + ); + Ok(request.into_response(response)) + }) } fn handle_oidc_callback( &self, request: ServiceRequest, ) -> LocalBoxFuture, Error>> { - let oidc_client = Arc::clone(&self.oidc_state.client); - let oidc_config = Arc::clone(&self.oidc_state.config); + let oidc_state = Arc::clone(&self.oidc_state); Box::pin(async move { + let oidc_client = oidc_state.get_client().await; let query_string = request.query_string(); match process_oidc_callback(&oidc_client, 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_client, &oidc_config, &request); + build_auth_provider_redirect_response(&oidc_client, &oidc_state.config, &request); Ok(request.into_response(resp)) } } From b8c51e2ad3d692a4b0c344d7cf80c06312204c2b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 28 Jul 2025 23:17:35 +0000 Subject: [PATCH 2/9] Refactor OIDC state management for improved caching and refresh logic Co-authored-by: contact --- src/webserver/oidc.rs | 313 +++++++++++++++++------------------------- 1 file changed, 125 insertions(+), 188 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 30519d03..601b04e2 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -1,5 +1,11 @@ use std::future::ready; -use std::{future::Future, pin::Pin, str::FromStr, sync::Arc, time::{Duration, Instant}}; +use std::{ + future::Future, + pin::Pin, + str::FromStr, + sync::Arc, + time::{Duration, Instant}, +}; use crate::webserver::http_client::get_http_client_from_appdata; use crate::{app_config::AppConfig, AppState}; @@ -32,53 +38,43 @@ const SQLPAGE_STATE_COOKIE_NAME: &str = "sqlpage_oidc_state"; // Cache configuration based on industry best practices const PROVIDER_METADATA_CACHE_DURATION: Duration = Duration::from_secs(24 * 60 * 60); // 24 hours -const PROVIDER_METADATA_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60); // 1 hour const MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5 * 60); // 5 minutes (rate limiting) #[derive(Clone, Debug)] -struct CachedProviderMetadata { +struct CachedProvider { + client: OidcClient, metadata: openidconnect::core::CoreProviderMetadata, cached_at: Instant, - last_refresh_attempt: Option, + last_refresh_attempt: Instant, } -impl CachedProviderMetadata { - fn new(metadata: openidconnect::core::CoreProviderMetadata) -> Self { +impl CachedProvider { + fn new(client: OidcClient, metadata: openidconnect::core::CoreProviderMetadata) -> Self { + let now = Instant::now(); Self { + client, metadata, - cached_at: Instant::now(), - last_refresh_attempt: None, + cached_at: now, + last_refresh_attempt: now, } } - fn is_expired(&self) -> bool { + fn is_stale(&self) -> bool { self.cached_at.elapsed() > PROVIDER_METADATA_CACHE_DURATION } - fn should_refresh(&self) -> bool { - // Refresh if the cache is older than refresh interval - if self.cached_at.elapsed() > PROVIDER_METADATA_REFRESH_INTERVAL { - return true; - } - false + fn can_refresh(&self) -> bool { + self.last_refresh_attempt.elapsed() > MIN_REFRESH_INTERVAL } - fn can_attempt_refresh(&self) -> bool { - // Rate limit refresh attempts to prevent excessive requests - match self.last_refresh_attempt { - Some(last_attempt) => last_attempt.elapsed() > MIN_REFRESH_INTERVAL, - None => true, - } + fn update(&mut self, client: OidcClient, metadata: openidconnect::core::CoreProviderMetadata) { + self.client = client; + self.metadata = metadata; + self.cached_at = Instant::now(); } fn mark_refresh_attempt(&mut self) { - self.last_refresh_attempt = Some(Instant::now()); - } - - fn update_metadata(&mut self, metadata: openidconnect::core::CoreProviderMetadata) { - self.metadata = metadata; - self.cached_at = Instant::now(); - self.last_refresh_attempt = None; + self.last_refresh_attempt = Instant::now(); } } @@ -170,106 +166,52 @@ fn get_app_host(config: &AppConfig) -> String { pub struct OidcState { pub config: Arc, - client: Arc>, - cached_metadata: Arc>>, + cached_provider: Arc>, http_client: Arc, } impl OidcState { - /// Get the current OIDC client, refreshing if necessary - pub async fn get_client(&self) -> Arc { - let should_refresh = { - let cache = self.cached_metadata.read().await; - match cache.as_ref() { - Some(cached) => cached.is_expired() || (cached.should_refresh() && cached.can_attempt_refresh()), - None => true, - } - }; - - if should_refresh { - if let Err(e) = self.refresh_provider_metadata().await { - log::warn!("Failed to refresh OIDC provider metadata: {}", e); - // Continue with current client if available + /// Get the current OIDC client, refreshing if stale and possible + pub async fn get_client(&self) -> OidcClient { + // Try to refresh if cache is stale and we haven't tried recently + { + let cache = self.cached_provider.read().await; + if cache.is_stale() && cache.can_refresh() { + // Release read lock before attempting refresh + drop(cache); + if let Err(e) = self.refresh_provider().await { + log::warn!("Failed to refresh OIDC provider: {}", e); + } } } - Arc::new(self.client.read().await.clone()) + self.cached_provider.read().await.client.clone() } - /// Get the current provider metadata, refreshing if necessary - pub async fn get_provider_metadata(&self) -> anyhow::Result { - let should_refresh = { - let cache = self.cached_metadata.read().await; - match cache.as_ref() { - Some(cached) => cached.is_expired() || (cached.should_refresh() && cached.can_attempt_refresh()), - None => true, - } - }; + /// Refresh provider metadata and client from the OIDC provider + async fn refresh_provider(&self) -> anyhow::Result<()> { + let mut cache = self.cached_provider.write().await; - if should_refresh { - if let Err(e) = self.refresh_provider_metadata().await { - log::warn!("Failed to refresh OIDC provider metadata: {}", e); - // Continue with cached data if available - } + // Double-check we can refresh (another thread might have just done it) + if !cache.can_refresh() { + return Ok(()); } - let cache = self.cached_metadata.read().await; - match cache.as_ref() { - Some(cached) if !cached.is_expired() => Ok(cached.metadata.clone()), - Some(cached) => { - log::warn!("OIDC provider metadata cache has expired, but refresh failed. Using stale data."); - Ok(cached.metadata.clone()) - } - None => Err(anyhow!("No OIDC provider metadata available and refresh failed")), - } - } + cache.mark_refresh_attempt(); - /// Refresh provider metadata from the OIDC provider - pub async fn refresh_provider_metadata(&self) -> anyhow::Result<()> { - // Mark refresh attempt to prevent excessive requests - { - let mut cache = self.cached_metadata.write().await; - if let Some(cached) = cache.as_mut() { - if !cached.can_attempt_refresh() { - return Err(anyhow!("Rate limited: too soon since last refresh attempt")); - } - cached.mark_refresh_attempt(); - } - } + log::debug!( + "Refreshing OIDC provider metadata for {}", + self.config.issuer_url + ); - log::debug!("Refreshing OIDC provider metadata for {}", self.config.issuer_url); - - let new_metadata = discover_provider_metadata(&self.http_client, self.config.issuer_url.clone()).await?; - - // Create new client with updated metadata + let new_metadata = + discover_provider_metadata(&self.http_client, self.config.issuer_url.clone()).await?; let new_client = make_oidc_client(&self.config, new_metadata.clone())?; - - // Update both cache and client atomically - let mut cache = self.cached_metadata.write().await; - let mut client = self.client.write().await; - - match cache.as_mut() { - Some(cached) => cached.update_metadata(new_metadata), - None => *cache = Some(CachedProviderMetadata::new(new_metadata)), - } - - *client = new_client; - log::debug!("Successfully refreshed OIDC provider metadata and client"); - Ok(()) - } + cache.update(new_client, new_metadata); - /// Start background task to periodically refresh metadata - pub fn start_background_refresh(state: Arc) { - tokio::spawn(async move { - let mut interval = tokio::time::interval(PROVIDER_METADATA_REFRESH_INTERVAL); - loop { - interval.tick().await; - if let Err(e) = state.refresh_provider_metadata().await { - log::warn!("Background refresh of OIDC provider metadata failed: {}", e); - } - } - }); + log::debug!("Successfully refreshed OIDC provider"); + Ok(()) } } @@ -283,21 +225,18 @@ pub async fn initialize_oidc_state( }; let http_client = Arc::new(make_http_client(app_config)?); - + // Initial metadata discovery - let provider_metadata = discover_provider_metadata(&http_client, oidc_cfg.issuer_url.clone()).await?; + let provider_metadata = + discover_provider_metadata(&http_client, oidc_cfg.issuer_url.clone()).await?; let client = make_oidc_client(&oidc_cfg, provider_metadata.clone())?; let oidc_state = Arc::new(OidcState { config: oidc_cfg, - client: Arc::new(RwLock::new(client)), - cached_metadata: Arc::new(RwLock::new(Some(CachedProviderMetadata::new(provider_metadata)))), + cached_provider: Arc::new(RwLock::new(CachedProvider::new(client, provider_metadata))), http_client, }); - // Start background refresh task - OidcState::start_background_refresh(Arc::clone(&oidc_state)); - Ok(Some(oidc_state)) } @@ -364,58 +303,51 @@ where 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); - } - - 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_unauthenticated_request( + oidc_state: Arc, + request: ServiceRequest, + service: S, +) -> Result, Error> +where + S: Service, Error = Error>, +{ + log::debug!("Handling unauthenticated request to {}", request.path()); - log::debug!("Redirecting to OIDC provider"); + if request.path() == SQLPAGE_REDIRECT_URI { + log::debug!("The request is the OIDC callback"); + return handle_oidc_callback(oidc_state, request).await; + } - let oidc_state = Arc::clone(&self.oidc_state); - Box::pin(async move { - let client = oidc_state.get_client().await; - let response = build_auth_provider_redirect_response( - &client, - &oidc_state.config, - &request, - ); - Ok(request.into_response(response)) - }) + if oidc_state.config.is_public_path(request.path()) { + log::debug!( + "The request path {} is public, skipping OIDC authentication", + request.path() + ); + return service.call(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"); + let client = oidc_state.get_client().await; + let response = build_auth_provider_redirect_response(&client, &oidc_state.config, &request); + Ok(request.into_response(response)) +} - Box::pin(async move { - let oidc_client = oidc_state.get_client().await; - let query_string = request.query_string(); - match process_oidc_callback(&oidc_client, 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_client, &oidc_state.config, &request); - Ok(request.into_response(resp)) - } - } - }) +async fn handle_oidc_callback( + oidc_state: Arc, + request: ServiceRequest, +) -> Result, Error> { + let oidc_client = oidc_state.get_client().await; + let query_string = request.query_string(); + match process_oidc_callback(&oidc_client, 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_client, &oidc_state.config, &request); + Ok(request.into_response(resp)) + } } } @@ -433,31 +365,36 @@ where fn call(&self, request: ServiceRequest) -> Self::Future { log::trace!("Started OIDC middleware request handling"); - let oidc_client = Arc::clone(&self.oidc_state.client); - match get_authenticated_user_info(&oidc_client, &request) { - Ok(Some(claims)) => { - 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); - } - } - let future = self.service.call(request); + let oidc_state = Arc::clone(&self.oidc_state); + let service = self.service.clone(); + Box::pin(async move { - let response = future.await?; - Ok(response) + let oidc_client = oidc_state.get_client().await; + match get_authenticated_user_info(&oidc_client, &request) { + Ok(Some(claims)) => { + log::trace!( + "Storing authenticated user info in request extensions: {claims:?}" + ); + request.extensions_mut().insert(claims); + let future = service.call(request); + let response = future.await?; + Ok(response) + } + Ok(None) => { + log::trace!("No authenticated user found"); + handle_unauthenticated_request(oidc_state, request, service).await + } + Err(e) => { + log::debug!( + "{:?}", + e.context( + "An auth cookie is present but could not be verified. \ + Redirecting to OIDC provider to re-authenticate." + ) + ); + handle_unauthenticated_request(oidc_state, request, service).await + } + } }) } } From f4872201daa99ed88e9843884b7f05af192aaed9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 28 Jul 2025 23:30:24 +0000 Subject: [PATCH 3/9] Refactor OIDC client management and remove unnecessary HTTP client Co-authored-by: contact --- src/webserver/oidc.rs | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 601b04e2..3ba1132c 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -167,19 +167,28 @@ fn get_app_host(config: &AppConfig) -> String { pub struct OidcState { pub config: Arc, cached_provider: Arc>, - http_client: Arc, } impl OidcState { + /// Get the current OIDC client, checking if cache is stale but not attempting refresh + pub fn get_client(&self) -> OidcClient { + // For now, we'll use a simple approach - get the current client + // In a production system, you might want to check if cache is stale + // and trigger an async refresh task + futures_util::executor::block_on(async { + self.cached_provider.read().await.client.clone() + }) + } + /// Get the current OIDC client, refreshing if stale and possible - pub async fn get_client(&self) -> OidcClient { + pub async fn get_client_with_refresh(&self, app_config: &AppConfig) -> OidcClient { // Try to refresh if cache is stale and we haven't tried recently { let cache = self.cached_provider.read().await; if cache.is_stale() && cache.can_refresh() { // Release read lock before attempting refresh drop(cache); - if let Err(e) = self.refresh_provider().await { + if let Err(e) = self.refresh_provider(app_config).await { log::warn!("Failed to refresh OIDC provider: {}", e); } } @@ -189,7 +198,7 @@ impl OidcState { } /// Refresh provider metadata and client from the OIDC provider - async fn refresh_provider(&self) -> anyhow::Result<()> { + async fn refresh_provider(&self, app_config: &AppConfig) -> anyhow::Result<()> { let mut cache = self.cached_provider.write().await; // Double-check we can refresh (another thread might have just done it) @@ -204,8 +213,9 @@ impl OidcState { self.config.issuer_url ); + let http_client = make_http_client(app_config)?; let new_metadata = - discover_provider_metadata(&self.http_client, self.config.issuer_url.clone()).await?; + discover_provider_metadata(&http_client, self.config.issuer_url.clone()).await?; let new_client = make_oidc_client(&self.config, new_metadata.clone())?; cache.update(new_client, new_metadata); @@ -224,7 +234,7 @@ pub async fn initialize_oidc_state( Err(Some(e)) => return Err(anyhow::anyhow!(e)), }; - let http_client = Arc::new(make_http_client(app_config)?); + let http_client = make_http_client(app_config)?; // Initial metadata discovery let provider_metadata = @@ -234,7 +244,6 @@ pub async fn initialize_oidc_state( let oidc_state = Arc::new(OidcState { config: oidc_cfg, cached_provider: Arc::new(RwLock::new(CachedProvider::new(client, provider_metadata))), - http_client, }); Ok(Some(oidc_state)) @@ -329,7 +338,7 @@ where } log::debug!("Redirecting to OIDC provider"); - let client = oidc_state.get_client().await; + let client = oidc_state.get_client(); let response = build_auth_provider_redirect_response(&client, &oidc_state.config, &request); Ok(request.into_response(response)) } @@ -338,7 +347,7 @@ async fn handle_oidc_callback( oidc_state: Arc, request: ServiceRequest, ) -> Result, Error> { - let oidc_client = oidc_state.get_client().await; + let oidc_client = oidc_state.get_client(); let query_string = request.query_string(); match process_oidc_callback(&oidc_client, query_string, &request).await { Ok(response) => Ok(request.into_response(response)), @@ -353,7 +362,7 @@ async fn handle_oidc_callback( impl Service for OidcService where - S: Service, Error = Error>, + S: Service, Error = Error> + Clone, S::Future: 'static, { type Response = ServiceResponse; @@ -369,7 +378,7 @@ where let service = self.service.clone(); Box::pin(async move { - let oidc_client = oidc_state.get_client().await; + let oidc_client = oidc_state.get_client(); match get_authenticated_user_info(&oidc_client, &request) { Ok(Some(claims)) => { log::trace!( From 80c5f8e0a709f68fd110d6fca0125df4ce501f4c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 28 Jul 2025 23:34:56 +0000 Subject: [PATCH 4/9] Store app config in OidcState to simplify client refresh method Co-authored-by: contact --- src/webserver/oidc.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 3ba1132c..f7eec81e 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -167,6 +167,7 @@ fn get_app_host(config: &AppConfig) -> String { pub struct OidcState { pub config: Arc, cached_provider: Arc>, + app_config: Arc, } impl OidcState { @@ -181,14 +182,14 @@ impl OidcState { } /// Get the current OIDC client, refreshing if stale and possible - pub async fn get_client_with_refresh(&self, app_config: &AppConfig) -> OidcClient { + pub async fn get_client_with_refresh(&self) -> OidcClient { // Try to refresh if cache is stale and we haven't tried recently { let cache = self.cached_provider.read().await; if cache.is_stale() && cache.can_refresh() { // Release read lock before attempting refresh drop(cache); - if let Err(e) = self.refresh_provider(app_config).await { + if let Err(e) = self.refresh_provider().await { log::warn!("Failed to refresh OIDC provider: {}", e); } } @@ -198,7 +199,7 @@ impl OidcState { } /// Refresh provider metadata and client from the OIDC provider - async fn refresh_provider(&self, app_config: &AppConfig) -> anyhow::Result<()> { + async fn refresh_provider(&self) -> anyhow::Result<()> { let mut cache = self.cached_provider.write().await; // Double-check we can refresh (another thread might have just done it) @@ -213,7 +214,7 @@ impl OidcState { self.config.issuer_url ); - let http_client = make_http_client(app_config)?; + let http_client = make_http_client(&self.app_config)?; let new_metadata = discover_provider_metadata(&http_client, self.config.issuer_url.clone()).await?; let new_client = make_oidc_client(&self.config, new_metadata.clone())?; @@ -244,6 +245,7 @@ pub async fn initialize_oidc_state( let oidc_state = Arc::new(OidcState { config: oidc_cfg, cached_provider: Arc::new(RwLock::new(CachedProvider::new(client, provider_metadata))), + app_config: Arc::new(app_config.clone()), }); Ok(Some(oidc_state)) From 14c379758fd3315e04fe717652c0d69ba012b6e2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 28 Jul 2025 23:36:31 +0000 Subject: [PATCH 5/9] Checkpoint before follow-up message --- src/webserver/oidc.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index f7eec81e..5be16182 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -167,7 +167,6 @@ fn get_app_host(config: &AppConfig) -> String { pub struct OidcState { pub config: Arc, cached_provider: Arc>, - app_config: Arc, } impl OidcState { @@ -199,7 +198,7 @@ impl OidcState { } /// Refresh provider metadata and client from the OIDC provider - async fn refresh_provider(&self) -> anyhow::Result<()> { + async fn refresh_provider(&self, http_client: &awc::Client) -> anyhow::Result<()> { let mut cache = self.cached_provider.write().await; // Double-check we can refresh (another thread might have just done it) @@ -214,9 +213,8 @@ impl OidcState { self.config.issuer_url ); - let http_client = make_http_client(&self.app_config)?; let new_metadata = - discover_provider_metadata(&http_client, self.config.issuer_url.clone()).await?; + discover_provider_metadata(http_client, self.config.issuer_url.clone()).await?; let new_client = make_oidc_client(&self.config, new_metadata.clone())?; cache.update(new_client, new_metadata); From e5444613db8b003c40928c5879f85071196db38f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 28 Jul 2025 23:38:05 +0000 Subject: [PATCH 6/9] Enhance OIDC client refresh with HTTP client injection and error handling Co-authored-by: contact --- src/webserver/oidc.rs | 68 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 5be16182..f3872f2a 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -181,14 +181,14 @@ impl OidcState { } /// Get the current OIDC client, refreshing if stale and possible - pub async fn get_client_with_refresh(&self) -> OidcClient { + pub async fn get_client_with_refresh(&self, http_client: &awc::Client) -> OidcClient { // Try to refresh if cache is stale and we haven't tried recently { let cache = self.cached_provider.read().await; if cache.is_stale() && cache.can_refresh() { // Release read lock before attempting refresh drop(cache); - if let Err(e) = self.refresh_provider().await { + if let Err(e) = self.refresh_provider(http_client).await { log::warn!("Failed to refresh OIDC provider: {}", e); } } @@ -243,7 +243,6 @@ pub async fn initialize_oidc_state( let oidc_state = Arc::new(OidcState { config: oidc_cfg, cached_provider: Arc::new(RwLock::new(CachedProvider::new(client, provider_metadata))), - app_config: Arc::new(app_config.clone()), }); Ok(Some(oidc_state)) @@ -338,7 +337,20 @@ where } log::debug!("Redirecting to OIDC provider"); - let client = oidc_state.get_client(); + + // Get HTTP client from app data for potential cache refresh + let http_client = match get_http_client_from_appdata(&request) { + Ok(client) => client, + Err(e) => { + log::error!("Failed to get HTTP client from app data: {}", e); + // Fall back to cached client without refresh + let client = oidc_state.get_client(); + let response = build_auth_provider_redirect_response(&client, &oidc_state.config, &request); + return Ok(request.into_response(response)); + } + }; + + let client = oidc_state.get_client_with_refresh(http_client).await; let response = build_auth_provider_redirect_response(&client, &oidc_state.config, &request); Ok(request.into_response(response)) } @@ -347,7 +359,19 @@ async fn handle_oidc_callback( oidc_state: Arc, request: ServiceRequest, ) -> Result, Error> { - let oidc_client = oidc_state.get_client(); + // Get HTTP client from app data for potential cache refresh + let http_client = match get_http_client_from_appdata(&request) { + Ok(client) => client, + Err(e) => { + log::error!("Failed to get HTTP client from app data: {}", e); + // Fall back to cached client without refresh + let oidc_client = oidc_state.get_client(); + let resp = build_auth_provider_redirect_response(&oidc_client, &oidc_state.config, &request); + return Ok(request.into_response(resp)); + } + }; + + let oidc_client = oidc_state.get_client_with_refresh(http_client).await; let query_string = request.query_string(); match process_oidc_callback(&oidc_client, query_string, &request).await { Ok(response) => Ok(request.into_response(response)), @@ -378,7 +402,39 @@ where let service = self.service.clone(); Box::pin(async move { - let oidc_client = oidc_state.get_client(); + // Get HTTP client from app data for potential cache refresh + let http_client = match get_http_client_from_appdata(&request) { + Ok(client) => client, + Err(e) => { + log::error!("Failed to get HTTP client from app data: {}", e); + // Fall back to cached client without refresh + let oidc_client = oidc_state.get_client(); + match get_authenticated_user_info(&oidc_client, &request) { + Ok(Some(claims)) => { + log::trace!("Storing authenticated user info in request extensions: {claims:?}"); + request.extensions_mut().insert(claims); + let future = service.call(request); + return future.await; + } + Ok(None) => { + log::trace!("No authenticated user found"); + return handle_unauthenticated_request(oidc_state, request, service).await; + } + Err(e) => { + log::debug!( + "{:?}", + e.context( + "An auth cookie is present but could not be verified. \ + Redirecting to OIDC provider to re-authenticate." + ) + ); + return handle_unauthenticated_request(oidc_state, request, service).await; + } + } + } + }; + + let oidc_client = oidc_state.get_client_with_refresh(http_client).await; match get_authenticated_user_info(&oidc_client, &request) { Ok(Some(claims)) => { log::trace!( From febbe8130402d59f4d6a1876c15a28c5e4a13a3c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 28 Jul 2025 23:44:36 +0000 Subject: [PATCH 7/9] Make get_client async and remove block_on wrapper Co-authored-by: contact --- src/webserver/oidc.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index f3872f2a..a9454293 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -171,13 +171,11 @@ pub struct OidcState { impl OidcState { /// Get the current OIDC client, checking if cache is stale but not attempting refresh - pub fn get_client(&self) -> OidcClient { + pub async fn get_client(&self) -> OidcClient { // For now, we'll use a simple approach - get the current client // In a production system, you might want to check if cache is stale // and trigger an async refresh task - futures_util::executor::block_on(async { - self.cached_provider.read().await.client.clone() - }) + self.cached_provider.read().await.client.clone() } /// Get the current OIDC client, refreshing if stale and possible @@ -344,7 +342,7 @@ where Err(e) => { log::error!("Failed to get HTTP client from app data: {}", e); // Fall back to cached client without refresh - let client = oidc_state.get_client(); + let client = oidc_state.get_client().await; let response = build_auth_provider_redirect_response(&client, &oidc_state.config, &request); return Ok(request.into_response(response)); } @@ -365,7 +363,7 @@ async fn handle_oidc_callback( Err(e) => { log::error!("Failed to get HTTP client from app data: {}", e); // Fall back to cached client without refresh - let oidc_client = oidc_state.get_client(); + let oidc_client = oidc_state.get_client().await; let resp = build_auth_provider_redirect_response(&oidc_client, &oidc_state.config, &request); return Ok(request.into_response(resp)); } @@ -408,7 +406,7 @@ where Err(e) => { log::error!("Failed to get HTTP client from app data: {}", e); // Fall back to cached client without refresh - let oidc_client = oidc_state.get_client(); + let oidc_client = oidc_state.get_client().await; match get_authenticated_user_info(&oidc_client, &request) { Ok(Some(claims)) => { log::trace!("Storing authenticated user info in request extensions: {claims:?}"); From 78e24e1ca008a5b3c184786eea8213e2e7fe64c8 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Thu, 31 Jul 2025 00:34:06 +0200 Subject: [PATCH 8/9] Update oidc.rs --- src/webserver/oidc.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index a9454293..00134f70 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -335,7 +335,7 @@ where } log::debug!("Redirecting to OIDC provider"); - + // Get HTTP client from app data for potential cache refresh let http_client = match get_http_client_from_appdata(&request) { Ok(client) => client, @@ -343,11 +343,12 @@ where log::error!("Failed to get HTTP client from app data: {}", e); // Fall back to cached client without refresh let client = oidc_state.get_client().await; - let response = build_auth_provider_redirect_response(&client, &oidc_state.config, &request); + let response = + build_auth_provider_redirect_response(&client, &oidc_state.config, &request); return Ok(request.into_response(response)); } }; - + let client = oidc_state.get_client_with_refresh(http_client).await; let response = build_auth_provider_redirect_response(&client, &oidc_state.config, &request); Ok(request.into_response(response)) @@ -364,11 +365,12 @@ async fn handle_oidc_callback( log::error!("Failed to get HTTP client from app data: {}", e); // Fall back to cached client without refresh let oidc_client = oidc_state.get_client().await; - let resp = build_auth_provider_redirect_response(&oidc_client, &oidc_state.config, &request); + let resp = + build_auth_provider_redirect_response(&oidc_client, &oidc_state.config, &request); return Ok(request.into_response(resp)); } }; - + let oidc_client = oidc_state.get_client_with_refresh(http_client).await; let query_string = request.query_string(); match process_oidc_callback(&oidc_client, query_string, &request).await { @@ -409,14 +411,17 @@ where let oidc_client = oidc_state.get_client().await; match get_authenticated_user_info(&oidc_client, &request) { Ok(Some(claims)) => { - log::trace!("Storing authenticated user info in request extensions: {claims:?}"); + log::trace!( + "Storing authenticated user info in request extensions: {claims:?}" + ); request.extensions_mut().insert(claims); let future = service.call(request); return future.await; } Ok(None) => { log::trace!("No authenticated user found"); - return handle_unauthenticated_request(oidc_state, request, service).await; + return handle_unauthenticated_request(oidc_state, request, service) + .await; } Err(e) => { log::debug!( @@ -426,12 +431,13 @@ where Redirecting to OIDC provider to re-authenticate." ) ); - return handle_unauthenticated_request(oidc_state, request, service).await; + return handle_unauthenticated_request(oidc_state, request, service) + .await; } } } }; - + let oidc_client = oidc_state.get_client_with_refresh(http_client).await; match get_authenticated_user_info(&oidc_client, &request) { Ok(Some(claims)) => { From d5bbcdc9f1dca6697284f020319832c4857efc9a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 30 Jul 2025 22:37:24 +0000 Subject: [PATCH 9/9] Improve OIDC client retrieval with staleness warning Co-authored-by: contact --- src/webserver/oidc.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 00134f70..d7a120ee 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -170,12 +170,16 @@ pub struct OidcState { } impl OidcState { - /// Get the current OIDC client, checking if cache is stale but not attempting refresh + /// Get the current OIDC client without attempting refresh pub async fn get_client(&self) -> OidcClient { - // For now, we'll use a simple approach - get the current client - // In a production system, you might want to check if cache is stale - // and trigger an async refresh task - self.cached_provider.read().await.client.clone() + let cache = self.cached_provider.read().await; + if cache.is_stale() { + log::warn!( + "OIDC provider metadata is stale (age: {:?}). Consider using get_client_with_refresh() for automatic refresh.", + cache.age() + ); + } + cache.client.clone() } /// Get the current OIDC client, refreshing if stale and possible