OIDC: non-blocking background refresh and body read timeout

- OIDC provider metadata refreshes now run in a background task via
  spawn_local, never blocking incoming HTTP requests.
- Multiple concurrent refresh triggers are deduplicated via an AtomicBool.
- The write lock on the OIDC client is only held briefly to swap data,
  not during the upstream HTTP call.
- Add a body-read timeout to OIDC HTTP requests to prevent hangs when
  the provider stalls after sending headers.
- Add tests for both scenarios: slow discovery and slow token endpoint.

Fixes #1231

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: lovasoa

src/webserver/oidc.rs

@@ -198,6 +198,7 @@ pub struct ClientWithTime {
 pub struct OidcState {
     pub config: OidcConfig,
     client: RwLock<ClientWithTime>,
+    refreshing: std::sync::atomic::AtomicBool,
 }
 
 impl OidcState {
@@ -212,36 +213,51 @@ impl OidcState {
                 end_session_endpoint,
                 last_update: Instant::now(),
             }),
+            refreshing: std::sync::atomic::AtomicBool::new(false),
         })
     }
 
-    async fn refresh(&self, service_request: &ServiceRequest) {
-        let mut write_guard = self.client.write().await;
-        match build_oidc_client_from_appdata(&self.config, service_request).await {
-            Ok((http_client, end_session_endpoint)) => {
-                *write_guard = ClientWithTime {
-                    client: http_client,
-                    end_session_endpoint,
-                    last_update: Instant::now(),
+    /// Spawns a background task to refresh the OIDC client from the provider
+    /// metadata URL if it hasn't been refreshed within `max_age`.
+    /// Returns immediately without blocking the caller.
+    /// Multiple concurrent calls are deduplicated via an atomic flag.
+    fn refresh_in_background(self: &Arc<Self>, http_client: &Client, max_age: Duration) {
+        use std::sync::atomic::Ordering;
+        let Ok(last_update) = self.client.try_read().map(|g| g.last_update) else {
+            return; // write lock held → a refresh is already in progress
+        };
+        if last_update.elapsed() <= max_age {
+            return;
+        }
+        if self.refreshing.swap(true, Ordering::AcqRel) {
+            return; // another refresh is already running
+        }
+        let state = Arc::clone(self);
+        let http_client = http_client.clone();
+        tokio::task::spawn_local(async move {
+            match build_oidc_client(&state.config, &http_client).await {
+                Ok((client, end_session_endpoint)) => {
+                    *state.client.write().await = ClientWithTime {
+                        client,
+                        end_session_endpoint,
+                        last_update: Instant::now(),
+                    };
                 }
+                Err(e) => log::error!("Failed to refresh OIDC client: {e:#}"),
             }
-            Err(e) => log::error!("Failed to refresh OIDC client: {e:#}"),
-        }
+            state.refreshing.store(false, Ordering::Release);
+        });
     }
 
     /// 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_MAX_REFRESH_INTERVAL {
-            self.refresh(service_request).await;
-        }
+    pub fn refresh_if_expired(self: &Arc<Self>, http_client: &Client) {
+        self.refresh_in_background(http_client, OIDC_CLIENT_MAX_REFRESH_INTERVAL);
     }
 
     /// 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;
-        }
+    pub fn refresh_on_error(self: &Arc<Self>, http_client: &Client) {
+        self.refresh_in_background(http_client, OIDC_CLIENT_MIN_REFRESH_INTERVAL);
     }
 
     /// Gets a reference to the oidc client, potentially generating a new one if needed
@@ -252,6 +268,15 @@ impl OidcState {
         )
     }
 
+    /// Forces the OIDC client to appear stale so that the next request triggers a refresh.
+    #[doc(hidden)]
+    pub async fn force_expire(&self) {
+        self.client.write().await.last_update =
+            Instant::now()
+                .checked_sub(OIDC_CLIENT_MAX_REFRESH_INTERVAL + Duration::from_secs(1))
+                .unwrap_or(Instant::now());
+    }
+
     pub async fn get_end_session_endpoint(&self) -> Option<EndSessionUrl> {
         self.client.read().await.end_session_endpoint.clone()
     }
@@ -309,14 +334,6 @@ pub async fn initialize_oidc_state(
     )))
 }
 
-async fn build_oidc_client_from_appdata(
-    cfg: &OidcConfig,
-    req: &ServiceRequest,
-) -> anyhow::Result<(OidcClient, Option<EndSessionUrl>)> {
-    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,
@@ -405,9 +422,14 @@ enum MiddlewareResponse {
     Respond(ServiceResponse),
 }
 
-async fn handle_request(oidc_state: &OidcState, request: ServiceRequest) -> MiddlewareResponse {
+async fn handle_request(
+    oidc_state: &Arc<OidcState>,
+    request: ServiceRequest,
+) -> MiddlewareResponse {
     log::trace!("Started OIDC middleware request handling");
-    oidc_state.refresh_if_expired(&request).await;
+    if let Ok(http_client) = get_http_client_from_appdata(&request) {
+        oidc_state.refresh_if_expired(http_client);
+    }
 
     if request.path() == oidc_state.config.redirect_uri {
         let response = handle_oidc_callback(oidc_state, request).await;
@@ -431,7 +453,9 @@ async fn handle_request(oidc_state: &OidcState, request: ServiceRequest) -> Midd
         }
         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;
+            if let Ok(http_client) = get_http_client_from_appdata(&request) {
+                oidc_state.refresh_on_error(http_client);
+            }
             handle_unauthenticated_request(oidc_state, request).await
         }
     }
@@ -456,7 +480,10 @@ async fn handle_unauthenticated_request(
     MiddlewareResponse::Respond(request.into_response(response))
 }
 
-async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) -> ServiceResponse {
+async fn handle_oidc_callback(
+    oidc_state: &Arc<OidcState>,
+    request: ServiceRequest,
+) -> ServiceResponse {
     match process_oidc_callback(oidc_state, &request).await {
         Ok(mut response) => {
             clear_redirect_count_cookie(&mut response);
@@ -467,7 +494,7 @@ async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) -
 }
 
 async fn handle_oidc_callback_error(
-    oidc_state: &OidcState,
+    oidc_state: &Arc<OidcState>,
     request: ServiceRequest,
     e: anyhow::Error,
 ) -> ServiceResponse {
@@ -478,7 +505,9 @@ async fn handle_oidc_callback_error(
     log::error!(
         "Failed to process OIDC callback (attempt {redirect_count}). Refreshing oidc provider metadata, then redirecting to home page: {e:#}"
     );
-    oidc_state.refresh_on_error(&request).await;
+    if let Ok(http_client) = get_http_client_from_appdata(&request) {
+        oidc_state.refresh_on_error(http_client);
+    }
     let resp = build_auth_provider_redirect_response(oidc_state, "/", redirect_count).await;
     request.into_response(resp)
 }

tests/oidc/mod.rs

@@ -1,5 +1,6 @@
 use actix_web::{
     cookie::Cookie,
+    dev::Service,
     http::{header, StatusCode},
     test,
     web::{self, Data},
@@ -83,17 +84,21 @@ struct TokenResponse {
 }
 
 async fn discovery_endpoint(state: Data<SharedProviderState>) -> impl Responder {
-    let state = state.lock().unwrap();
-    let discovery = DiscoveryResponse {
-        issuer: state.issuer_url.clone(),
-        authorization_endpoint: format!("{}/auth", state.issuer_url),
-        token_endpoint: format!("{}/token", state.issuer_url),
-        jwks_uri: format!("{}/jwks", state.issuer_url),
-        response_types_supported: vec!["code".to_string()],
-        subject_types_supported: vec!["public".to_string()],
-        id_token_signing_alg_values_supported: vec!["HS256".to_string()],
-        end_session_endpoint: format!("{}/logout", state.issuer_url),
+    let (discovery, delay) = {
+        let state = state.lock().unwrap();
+        let discovery = DiscoveryResponse {
+            issuer: state.issuer_url.clone(),
+            authorization_endpoint: format!("{}/auth", state.issuer_url),
+            token_endpoint: format!("{}/token", state.issuer_url),
+            jwks_uri: format!("{}/jwks", state.issuer_url),
+            response_types_supported: vec!["code".to_string()],
+            subject_types_supported: vec!["public".to_string()],
+            id_token_signing_alg_values_supported: vec!["HS256".to_string()],
+            end_session_endpoint: format!("{}/logout", state.issuer_url),
+        };
+        (discovery, state.discovery_delay)
     };
+    tokio::time::sleep(delay).await;
     HttpResponse::Ok()
         .insert_header((header::CONTENT_TYPE, "application/json"))
         .json(discovery)
@@ -282,6 +287,7 @@ async fn setup_oidc_test(
         Error = actix_web::Error,
     >,
     FakeOidcProvider,
+    Data<sqlpage::AppState>,
 ) {
     use sqlpage::{
         app_config::{test_database_url, AppConfig},
@@ -313,13 +319,14 @@ async fn setup_oidc_test(
 
     let config: AppConfig = serde_json::from_str(&config_json).unwrap();
     let app_state = AppState::init(&config).await.unwrap();
-    let app = test::init_service(create_app(Data::new(app_state))).await;
-    (app, provider)
+    let app_data = Data::new(app_state);
+    let app = test::init_service(create_app(app_data.clone())).await;
+    (app, provider, app_data)
 }
 
 #[actix_web::test]
 async fn test_oidc_happy_path() {
-    let (app, provider) = setup_oidc_test(|_| {}).await;
+    let (app, provider, _) = setup_oidc_test(|_| {}).await;
     let mut cookies: Vec<Cookie<'static>> = Vec::new();
 
     let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies);
@@ -348,7 +355,7 @@ async fn assert_oidc_login_fails(
     provider_mutator: impl FnOnce(&mut ProviderState),
     state_override: Option<String>,
 ) {
-    let (app, provider) = setup_oidc_test(provider_mutator).await;
+    let (app, provider, _) = setup_oidc_test(provider_mutator).await;
     let mut cookies: Vec<Cookie<'static>> = Vec::new();
 
     let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies);
@@ -556,8 +563,52 @@ async fn test_oidc_logout_uses_correct_scheme() {
     assert_eq!(post_logout, "https://example.com/logged_out");
 }
 
-/// A slow OIDC provider must not freeze the server.
-/// See https://github.com/sqlpage/SQLPage/issues/1231
+/// An OIDC provider metadata refresh must not block authenticated requests.
+/// The refresh should happen in the background while existing requests are
+/// served using the current (possibly stale) OIDC client.
+#[actix_web::test]
+async fn test_slow_discovery_does_not_block_authenticated_requests() {
+    let (app, provider) = setup_oidc_test(|_| {}).await;
+    let mut cookies: Vec<Cookie<'static>> = Vec::new();
+
+    // Complete a full login to get auth cookies
+    let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies);
+    assert_eq!(resp.status(), StatusCode::SEE_OTHER);
+    let auth_url = Url::parse(resp.headers().get("location").unwrap().to_str().unwrap()).unwrap();
+    let state_param = get_query_param(&auth_url, "state");
+    let nonce = get_query_param(&auth_url, "nonce");
+    let redirect_uri = get_query_param(&auth_url, "redirect_uri");
+    provider.store_auth_code("test_auth_code".to_string(), nonce);
+    let callback_uri = format!(
+        "{}?code=test_auth_code&state={}",
+        Url::parse(&redirect_uri).unwrap().path(),
+        state_param
+    );
+    let callback_resp =
+        request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies);
+    assert_eq!(callback_resp.status(), StatusCode::SEE_OTHER);
+
+    // Advance time so the OIDC snapshot appears stale.
+    // The next request triggers a background refresh.
+    let count_before = provider.discovery_count();
+    tokio::time::pause();
+    tokio::time::advance(Duration::from_secs(3601)).await;
+
+    // An authenticated request must succeed immediately, even though
+    // it triggers a background refresh.
+    let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies);
+    assert_eq!(resp.status(), StatusCode::OK);
+
+    // Let the background refresh task complete.
+    tokio::task::yield_now().await;
+    assert!(
+        provider.discovery_count() > count_before,
+        "OIDC provider metadata was not refreshed"
+    );
+}
+
+/// A slow OIDC token endpoint must not freeze the server.
+/// The body-read timeout fires and the request completes with a redirect.
 #[actix_web::test]
 async fn test_slow_token_endpoint_does_not_freeze_server() {
     let (app, provider) = setup_oidc_test(|_| {}).await;