diff --git a/backend/windmill-api-integration-tests/tests/native_triggers.rs b/backend/windmill-api-integration-tests/tests/native_triggers.rs index 17c6521309..c86ba3154f 100644 --- a/backend/windmill-api-integration-tests/tests/native_triggers.rs +++ b/backend/windmill-api-integration-tests/tests/native_triggers.rs @@ -11,14 +11,22 @@ use serde_json::json; use sqlx::{Pool, Postgres}; +use axum::http::StatusCode; use windmill_api_auth::ApiAuthed; -use windmill_common::variables::{build_crypt, encrypt}; +use windmill_common::{ + error::Error, + variables::{build_crypt, encrypt}, +}; use windmill_native_triggers::{ - decrypt_oauth_data, delete_native_trigger, delete_workspace_integration, - get_workspace_integration, + classify_read_failure, decrypt_oauth_data, delete_native_trigger, + delete_workspace_integration, get_workspace_integration, + github::GitHub, google::{parse_stop_channel_params, should_renew_channel}, - list_native_triggers, require_native_integration_use, store_native_trigger, - store_workspace_integration, NativeTriggerConfig, OAuthConfig, ServiceName, + http_error_status, list_native_triggers, map_external_error, + nextcloud::NextCloud, + grant_refused, require_native_integration_use, store_native_trigger, + store_workspace_integration, External, ExternalReadFailure, HttpRequestError, + NativeTriggerConfig, OAuthConfig, ServiceName, }; // ============================================================================ @@ -710,3 +718,133 @@ fn test_parse_stop_channel_params_missing_resource_id() { assert!(channel_id.is_none()); assert_eq!(resource_id, ""); } + +// --- provider error reporting --- + +fn nextcloud_error(status: StatusCode, body: &str) -> Error { + NextCloud.external_api_error(HttpRequestError::ApiError { status, body: body.to_string() }) +} + +/// A rejection has to reach the user as the service's own sentence plus what to do about it, +/// never as an internal error carrying the raw envelope. +#[test] +fn test_provider_rejection_is_readable_and_not_internal() { + let err = nextcloud_error( + StatusCode::FORBIDDEN, + r#"{"ocs":{"meta":{"status":"failure","statuscode":403,"message":"Logged in account must be an admin, a sub admin or gotten special right to access this setting"},"data":[]}}"#, + ); + + let message = map_external_error(err).to_string(); + assert!( + message.contains("Logged in account must be an admin"), + "message={message}" + ); + assert!( + !message.contains("\"ocs\""), + "the envelope should not reach the user: {message}" + ); + assert!( + message.contains("Workspace settings > Integrations"), + "the hint should say what to do: {message}" + ); +} + +/// Both the "trigger is gone on the service" path and the delete that tolerates an +/// already-removed webhook branch on this status. +#[test] +fn test_provider_404_is_recognized() { + let err = nextcloud_error(StatusCode::NOT_FOUND, "{}"); + assert_eq!(http_error_status(&err), Some(StatusCode::NOT_FOUND)); + assert!( + matches!(map_external_error(err), Error::NotFound(_)), + "a missing external trigger must map to NotFound" + ); + assert!(matches!( + classify_read_failure(nextcloud_error(StatusCode::NOT_FOUND, "{}")), + ExternalReadFailure::Missing + )); +} + +/// Sending a user to reconnect their integration is only right when the token endpoint refused +/// the grant; a busy or broken endpoint has them fix credentials that are fine. +#[test] +fn test_refresh_failures_blame_only_the_grant_they_refuse() { + let refused = |code: u16| grant_refused(Some(StatusCode::from_u16(code).unwrap()), ""); + for code in [400, 401, 403] { + assert!(refused(code), "{code} refuses the grant"); + } + for code in [404, 408, 429, 500, 503] { + assert!(!refused(code), "{code} says nothing about the grant"); + } + assert!(!grant_refused(None, "")); + + // GitHub answers `bad_refresh_token` with HTTP 200, so the body is the only tell. + let ok = Some(StatusCode::OK); + assert!(grant_refused(ok, r#"{"error":"bad_refresh_token"}"#)); + assert!(grant_refused(ok, r#"{"error":"invalid_grant"}"#)); + assert!(!grant_refused(ok, r#"{"access_token":"t","token_type":"bearer"}"#)); +} + +/// A service that is busy or broken has not refused anything, and callers react differently to +/// the two. GitHub and Google spend a 403 on throttling, where advice about permissions sends +/// the reader after a problem they do not have. +#[test] +fn test_transient_service_failures_are_not_refusals() { + for transient in [408, 429, 503] { + let err = nextcloud_error(StatusCode::from_u16(transient).unwrap(), "{}"); + assert!( + matches!(map_external_error(err), Error::BadGateway(_)), + "{transient} should read as the service failing to serve, not refusing" + ); + } + + // GitHub words its throttle two ways, and neither is a permission problem. + for wording in [ + "API rate limit exceeded for user ID 1.", + "You have exceeded a secondary rate limit.", + "You have triggered an abuse detection mechanism.", + ] { + let throttled = GitHub.external_api_error(HttpRequestError::ApiError { + status: StatusCode::FORBIDDEN, + body: format!(r#"{{"message":"{wording}"}}"#), + }); + let throttled = map_external_error(throttled); + assert!( + matches!(throttled, Error::BadGateway(_)), + "a throttled 403 is the service failing to serve: {throttled:?}" + ); + assert!( + !throttled.to_string().contains("admin rights"), + "a throttled 403 must not advise about permissions: {throttled}" + ); + } + + let refused = GitHub.external_api_error(HttpRequestError::ApiError { + status: StatusCode::FORBIDDEN, + body: r#"{"message":"Must have admin rights to Repository."}"#.to_string(), + }); + assert!( + map_external_error(refused).to_string().contains("admin rights"), + "a real 403 keeps its guidance" + ); +} + +/// A service read degrades to the stored configuration, but only for the service's own +/// failures: `External::get` also runs queries, and reporting one of those as the service's +/// word would hide a Windmill outage behind a 200. +#[test] +fn test_only_service_failures_degrade_the_read() { + assert!(matches!( + classify_read_failure(nextcloud_error(StatusCode::FORBIDDEN, "{}")), + ExternalReadFailure::Unreadable(_) + )); + assert!(matches!( + classify_read_failure(Error::internal_err("connection pool timed out")), + ExternalReadFailure::Internal(_) + )); + let internal = Error::internal_err("connection pool timed out"); + assert!( + matches!(map_external_error(internal), Error::InternalErrLoc { .. }), + "a non-provider error must pass through unmapped" + ); +} diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 78b224cce9..532b765375 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -33202,8 +33202,17 @@ components: description: Short summary to be displayed when listed external_data: type: object - description: Configuration data from the external service + nullable: true + description: >- + Configuration data from the external service. Null when the service has no + such API, or when it could not be read — see external_error. additionalProperties: true + external_error: + type: string + nullable: true + description: >- + Why the external service configuration could not be read. When set, + external_data is null and the configuration Windmill stored is returned instead. required: - external_id - workspace_id diff --git a/backend/windmill-native-triggers/src/github/external.rs b/backend/windmill-native-triggers/src/github/external.rs index d31b2ae207..2c23e02bef 100644 --- a/backend/windmill-native-triggers/src/github/external.rs +++ b/backend/windmill-native-triggers/src/github/external.rs @@ -232,7 +232,10 @@ impl External for GitHub { Err(e) => { errors.push(crate::sync::SyncError { resource_path: trigger.script_path.clone(), - error_message: format!("Failed to verify GitHub webhook: {}", e), + error_message: format!( + "Failed to verify GitHub webhook: {}", + crate::external_error_message(&e) + ), error_type: "api_error".to_string(), }); } @@ -288,6 +291,31 @@ impl External for GitHub { fn additional_routes(&self) -> axum::Router { routes::github_routes(self.clone()) } + + fn error_hint(&self, status: StatusCode) -> Option<&'static str> { + match status { + StatusCode::UNAUTHORIZED => { + Some("reconnect the GitHub integration from Workspace settings > Integrations.") + } + StatusCode::FORBIDDEN => Some( + "check that the OAuth app is authorized for the organization and that the \ + connected GitHub account has the access this needs — managing webhooks requires \ + admin rights on the repository.", + ), + _ => None, + } + } + + /// GitHub answers 403 to both throttling and a permission failure, and words the throttle + /// two ways: the primary and secondary limits say "rate limit", the older secondary + /// response says "abuse detection mechanism". + fn is_transient_response(&self, status: StatusCode, body: &str) -> bool { + if status != StatusCode::FORBIDDEN { + return false; + } + let body = body.to_ascii_lowercase(); + body.contains("rate limit") || body.contains("abuse detection") + } } impl GitHub { diff --git a/backend/windmill-native-triggers/src/github/routes.rs b/backend/windmill-native-triggers/src/github/routes.rs index b2196f2249..68de93b6a9 100644 --- a/backend/windmill-native-triggers/src/github/routes.rs +++ b/backend/windmill-native-triggers/src/github/routes.rs @@ -5,7 +5,10 @@ use http::Method; use windmill_api_auth::ApiAuthed; use windmill_common::{error::JsonResult, DB}; -use crate::{get_workspace_integration, require_native_integration_use, External, ServiceName}; +use crate::{ + get_workspace_integration, map_external_error, require_native_integration_use, External, + ServiceName, +}; use super::{GitHub, GithubApiRepoResponse, GithubRepoEntry}; @@ -33,7 +36,8 @@ async fn list_repos( let repos: Vec = handler .http_client_request::<_, ()>(&url, Method::GET, &workspace_id, &db, None, None) - .await?; + .await + .map_err(map_external_error)?; let count = repos.len(); all_entries.extend(repos.into_iter().map(|r| GithubRepoEntry { diff --git a/backend/windmill-native-triggers/src/google/external.rs b/backend/windmill-native-triggers/src/google/external.rs index b8728e2f3f..1c5a2dd9ec 100644 --- a/backend/windmill-native-triggers/src/google/external.rs +++ b/backend/windmill-native-triggers/src/google/external.rs @@ -199,6 +199,26 @@ impl External for Google { fn additional_routes(&self) -> axum::Router { routes::google_routes(self.clone()) } + + fn error_hint(&self, status: http::StatusCode) -> Option<&'static str> { + match status { + http::StatusCode::UNAUTHORIZED => { + Some("reconnect the Google integration from Workspace settings > Integrations.") + } + http::StatusCode::FORBIDDEN => Some( + "the connected Google account is missing access to this resource or the scope the \ + integration was granted does not cover it.", + ), + _ => None, + } + } + + /// Google answers 403 to a quota being spent as well as to a permission failure. Every + /// throttling reason it defines sits in the `usageLimits` domain, so matching the domain + /// covers the group without an allowlist to extend each time one is added. + fn is_transient_response(&self, status: http::StatusCode, body: &str) -> bool { + status == http::StatusCode::FORBIDDEN && body.contains("usageLimits") + } } // Helper methods for creating trigger type-specific watches @@ -656,7 +676,10 @@ async fn renew_expiring_channels( workspace_id, ServiceName::Google, &trigger.external_id, - Some(&format!("Channel renewal failed: {}", e)), + Some(&format!( + "Channel renewal failed: {}", + crate::external_error_message(&e) + )), ) .await; diff --git a/backend/windmill-native-triggers/src/google/routes.rs b/backend/windmill-native-triggers/src/google/routes.rs index c5f7611f10..306a7e54e2 100644 --- a/backend/windmill-native-triggers/src/google/routes.rs +++ b/backend/windmill-native-triggers/src/google/routes.rs @@ -10,7 +10,10 @@ use serde::{Deserialize, Serialize}; use windmill_api_auth::ApiAuthed; use windmill_common::{error::JsonResult, DB}; -use crate::{get_workspace_integration, require_native_integration_use, External, ServiceName}; +use crate::{ + get_workspace_integration, map_external_error, require_native_integration_use, External, + ServiceName, +}; use super::Google; @@ -100,7 +103,8 @@ async fn list_calendars( let response: GoogleCalendarListResponse = handler .http_client_request::<_, ()>(&url, Method::GET, &workspace_id, &db, None, None) - .await?; + .await + .map_err(map_external_error)?; let calendars = response .items @@ -153,7 +157,8 @@ async fn list_drive_files( let response: DriveApiResponse = handler .http_client_request::<_, ()>(&url, Method::GET, &workspace_id, &db, None, None) - .await?; + .await + .map_err(map_external_error)?; let files = response .files @@ -206,7 +211,8 @@ async fn list_shared_drives( let response: SharedDrivesApiResponse = handler .http_client_request::<_, ()>(&url, Method::GET, &workspace_id, &db, None, None) - .await?; + .await + .map_err(map_external_error)?; let drives = response .drives diff --git a/backend/windmill-native-triggers/src/handler.rs b/backend/windmill-native-triggers/src/handler.rs index 45d6368fc9..cc6c61be81 100644 --- a/backend/windmill-native-triggers/src/handler.rs +++ b/backend/windmill-native-triggers/src/handler.rs @@ -1,9 +1,11 @@ use crate::{ - decrypt_oauth_data, delete_native_trigger, delete_token_by_hash, get_native_trigger, - list_native_triggers, lock::TriggerLock, rotate_webhook_token, store_native_trigger, + classify_read_failure, decrypt_oauth_data, delete_native_trigger, delete_token_by_hash, + get_native_trigger, list_native_triggers, lock::TriggerLock, map_external_error, + map_external_error_with, rotate_webhook_token, store_native_trigger, sync::EXTERNAL_TRIGGER_MISSING_ERROR, update_native_trigger_error, update_native_trigger_if_runnable_unchanged, webhook_token_label, webhook_token_scopes, - External, NativeTrigger, NativeTriggerConfig, NativeTriggerData, ServiceName, + External, ExternalReadFailure, NativeTrigger, NativeTriggerConfig, NativeTriggerData, + ServiceName, }; use axum::{ extract::{Path, Query}, @@ -111,6 +113,10 @@ pub struct FullTriggerResponse { #[serde(flatten)] pub windmill_data: NativeTrigger, pub external_data: Option, + /// Why `external_data` is missing, when the service could not be read. The stored + /// configuration is still returned so the trigger stays viewable and editable. + #[serde(skip_serializing_if = "Option::is_none")] + pub external_error: Option, } #[derive(Debug, Serialize)] @@ -193,7 +199,8 @@ async fn create_native_trigger( &db, &mut tx, ) - .await?; + .await + .map_err(map_external_error)?; let (external_id, _) = handler.external_id_and_metadata_from_response(&resp); @@ -214,7 +221,8 @@ async fn create_native_trigger( &db, &mut tx, ) - .await? + .await + .map_err(map_external_error)? }; let config = NativeTriggerConfig { @@ -349,7 +357,8 @@ async fn update_native_trigger_handler( &db, &mut tx, ) - .await?; + .await + .map_err(map_external_error)?; let config = NativeTriggerConfig { script_path: data.script_path.clone(), @@ -443,6 +452,8 @@ async fn get_native_trigger_handler( .get(&workspace_id, &oauth_data, &external_id, &db, &mut tx) .await; + let mut external_error = None; + let external_data = match native_trigger { Ok(Some(native_cfg)) => { // Only the "no longer exists" error is disproven by the trigger being there; other @@ -461,35 +472,54 @@ async fn get_native_trigger_handler( Some(native_cfg) } Ok(None) => None, - Err(Error::NotFound(_)) => { - let error_msg = EXTERNAL_TRIGGER_MISSING_ERROR.to_string(); - tracing::warn!( - "Native trigger no longer exists on external service {}, setting error", - service_name - ); + Err(e) => match classify_read_failure(e) { + ExternalReadFailure::Missing => { + let error_msg = EXTERNAL_TRIGGER_MISSING_ERROR.to_string(); + tracing::warn!( + "Native trigger no longer exists on external service {}, setting error", + service_name + ); - update_native_trigger_error( - &mut *tx, - &workspace_id, - service_name, - &external_id, - Some(&error_msg), - ) - .await?; + update_native_trigger_error( + &mut *tx, + &workspace_id, + service_name, + &external_id, + Some(&error_msg), + ) + .await?; - tx.commit().await?; + tx.commit().await?; - return Err(Error::NotFound(format!( - "Trigger '{}' no longer exists on external service {}", - external_id, service_name - ))); - } - Err(e) => return Err(e), + return Err(Error::NotFound(format!( + "Trigger '{}' no longer exists on external service {}", + external_id, service_name + ))); + } + // The service being unreadable says nothing about the trigger Windmill stores, and + // failing here would leave the editor with no configuration to show at all. Report + // what the service said alongside the stored configuration instead. + ExternalReadFailure::Unreadable(message) => { + tracing::warn!( + "Could not read trigger '{}' from {}, returning the stored configuration: {}", + external_id, + service_name, + message + ); + external_error = Some(message); + None + } + ExternalReadFailure::Internal(e) => return Err(e), + }, }; tx.commit().await?; - let full_resp = Json(FullTriggerResponse { windmill_data: windmill_trigger, external_data }); + let full_resp = Json(FullTriggerResponse { + windmill_data: windmill_trigger, + external_data, + external_error, + }); Ok(full_resp) } @@ -528,7 +558,17 @@ async fn delete_native_trigger_handler( handler .delete(&workspace_id, &oauth_data, &external_id, &db, &mut tx) - .await?; + .await + .map_err(|e| { + map_external_error_with(e, |m| { + let end = if m.ends_with(['.', '!', '?']) { + "" + } else { + "." + }; + format!("{m}{end} The trigger was kept in Windmill, so it can still fire.") + }) + })?; let deleted = delete_native_trigger(&mut *tx, &workspace_id, service_name, &external_id).await?; diff --git a/backend/windmill-native-triggers/src/lib.rs b/backend/windmill-native-triggers/src/lib.rs index df92dcbc17..4400091ed6 100644 --- a/backend/windmill-native-triggers/src/lib.rs +++ b/backend/windmill-native-triggers/src/lib.rs @@ -54,7 +54,7 @@ use windmill_common::{ use windmill_queue::PushArgsOwned; #[cfg(feature = "native_trigger")] -use windmill_oauth::{OClient, RefreshToken, Url, OAUTH_HTTP_CLIENT}; +use windmill_oauth::{ErrorField, ExecuteError, OClient, RefreshToken, Url, OAUTH_HTTP_CLIENT}; use windmill_api_auth::ApiAuthed; pub mod handler; @@ -365,6 +365,23 @@ pub trait External: Send + Sync + 'static { axum::Router::new() } + /// Pull the human-readable message out of an error body, when the service wraps it in an + /// envelope. Returning `None` (default) shows the body as-is. + fn describe_error_body(&self, _body: &str) -> Option { + None + } + + /// What the user has to do about a rejection, appended to the service's own message. + fn error_hint(&self, _status: StatusCode) -> Option<&'static str> { + None + } + + /// Whether the service is telling us it cannot serve the request right now, when the + /// status alone would read as a refusal. GitHub and Google both answer 403 to throttling. + fn is_transient_response(&self, _status: StatusCode, _body: &str) -> bool { + false + } + async fn http_client_request( &self, url: &str, @@ -388,13 +405,14 @@ pub trait External: Send + Sync + 'static { match result { Ok(response) => Ok(response), - Err(err) - if err.status() == Some(StatusCode::UNAUTHORIZED) - || err.status() == Some(StatusCode::FORBIDDEN) => - { + // Only an expired or revoked token is worth a refresh. A 403 means the account + // behind the token is authenticated and still not allowed, which minting a new + // token for that same account cannot change; retrying would only burn a refresh + // rotation per request and bury the service's own explanation. + Err(err) if err.status() == Some(StatusCode::UNAUTHORIZED) => { tracing::info!( - "HTTP auth error ({}), attempting token refresh", - err.status().unwrap() + "HTTP 401 from {}, attempting token refresh", + Self::DISPLAY_NAME ); let refreshed_oauth_config = refresh_oauth_tokens( @@ -402,7 +420,16 @@ pub trait External: Send + Sync + 'static { Self::REFRESH_ENDPOINT, Self::AUTH_ENDPOINT, ) - .await?; + .await + .map_err(|f| { + self.external_error( + f.rejected.then_some(StatusCode::UNAUTHORIZED), + format!( + "the stored credentials could not be refreshed: {}", + f.message + ), + ) + })?; task::spawn({ let db_clone = db.clone(); @@ -422,7 +449,7 @@ pub trait External: Send + Sync + 'static { } }); - let response = make_http_request( + make_http_request( url, method, headers, @@ -430,12 +457,122 @@ pub trait External: Send + Sync + 'static { &refreshed_oauth_config.access_token, ) .await - .map_err(to_anyhow)?; - Ok(response) + .map_err(|e| self.external_api_error(e)) } - Err(e) => Err(to_anyhow(e).into()), + Err(e) => Err(self.external_api_error(e)), } } + + /// Wrap a failed provider call so the status stays inspectable by internal callers (a 404 + /// means the trigger is gone, not that the call broke) and the message stays readable by + /// the time it reaches a user. + fn external_api_error(&self, e: HttpRequestError) -> Error { + let (detail, transient) = match &e { + HttpRequestError::ApiError { status, body } => ( + self.describe_error_body(body) + .unwrap_or_else(|| body.to_string()), + self.is_transient_response(*status, body), + ), + // A reqwest error names the request it was making, never why it failed: the reason + // (refused, DNS, TLS) lives one link down the source chain. + other => (error_source_chain(other), false), + }; + self.external_error_inner(e.status(), detail, transient) + } + + fn external_error(&self, status: Option, detail: String) -> Error { + self.external_error_inner(status, detail, false) + } + + fn external_error_inner( + &self, + status: Option, + detail: String, + transient: bool, + ) -> Error { + to_anyhow(ExternalApiError { + service: Self::DISPLAY_NAME, + status, + detail: truncate_detail(&detail), + // Guidance about permissions on a throttled request sends the reader after a + // problem they do not have. + hint: if transient { + None + } else { + status.and_then(|s| self.error_hint(s)) + }, + transient, + }) + .into() + } +} + +/// A native trigger provider refused or could not serve a request. +#[derive(Debug)] +pub struct ExternalApiError { + pub service: &'static str, + pub status: Option, + pub detail: String, + pub hint: Option<&'static str>, + /// The service could not serve the request now, whatever the status suggests: providers + /// overload 403 for throttling, and a refusal and an outage want opposite reactions. + pub transient: bool, +} + +impl ExternalApiError { + fn is_transient(&self) -> bool { + self.transient || self.status.is_some_and(is_transient_status) + } +} + +impl std::fmt::Display for ExternalApiError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.status { + Some(status) if self.is_transient() => write!( + f, + "{} failed to serve the request ({}): {}", + self.service, status, self.detail + )?, + Some(status) => write!( + f, + "{} rejected the request ({}): {}", + self.service, status, self.detail + )?, + // No status covers both never reaching the service and not being able to read what + // it answered, so the wording may not commit to either. + None => write!(f, "Request to {} failed: {}", self.service, self.detail)?, + } + if let Some(hint) = self.hint { + write!(f, " — {}", hint)?; + } + Ok(()) + } +} + +impl std::error::Error for ExternalApiError {} + +fn error_source_chain(e: &dyn std::error::Error) -> String { + let mut msg = e.to_string(); + let mut source = e.source(); + while let Some(cause) = source { + let cause = cause.to_string(); + // reqwest repeats "error sending request for url (…)" at two levels of the chain. + if !msg.contains(&cause) { + msg.push_str(&format!(": {cause}")); + } + source = source.and_then(|c| c.source()); + } + msg +} + +/// Provider bodies are unbounded and end up in toasts and `native_trigger.error`. +fn truncate_detail(detail: &str) -> String { + const MAX: usize = 400; + let detail = detail.trim(); + match detail.char_indices().nth(MAX) { + Some((idx, _)) => format!("{}…", &detail[..idx]), + None => detail.to_string(), + } } #[derive(Debug, Serialize, Deserialize)] @@ -537,17 +674,83 @@ impl HttpRequestError { } } -/// Extract the HTTP status code from an error returned by `http_client_request`. -/// Returns `None` if the error didn't originate from an HTTP call. -pub fn http_error_status(e: &windmill_common::error::Error) -> Option { +/// `Some` only for a failure the service itself produced. Everything else — a query, a +/// decryption, a serialization — is Windmill's own and must not be reported as the service's. +pub fn as_external_error(e: &Error) -> Option<&ExternalApiError> { match e { - windmill_common::error::Error::Anyhow { error, .. } => error - .downcast_ref::() - .and_then(|e| e.status()), + Error::Anyhow { error, .. } => error.downcast_ref::(), _ => None, } } +/// Extract the HTTP status code from an error returned by `http_client_request`. +/// Returns `None` if the error didn't originate from an HTTP call. +pub fn http_error_status(e: &Error) -> Option { + as_external_error(e).and_then(|e| e.status) +} + +/// The message to show a user for a failed provider call, without the internal decoration +/// `Error`'s own `Display` adds. Non-provider errors are rendered as-is. +pub fn external_error_message(e: &Error) -> String { + as_external_error(e).map_or_else(|| e.to_string(), |ext| ext.to_string()) +} + +/// What a failed read of a trigger from its service means for the response. +#[derive(Debug)] +pub enum ExternalReadFailure { + /// The service answered that the trigger is gone. + Missing, + /// The service could not be read, which says nothing about the stored trigger. + Unreadable(String), + /// Windmill's own failure, wearing no service's name. + Internal(Error), +} + +pub fn classify_read_failure(e: Error) -> ExternalReadFailure { + match as_external_error(&e) { + Some(ext) if ext.status == Some(StatusCode::NOT_FOUND) => ExternalReadFailure::Missing, + Some(ext) => ExternalReadFailure::Unreadable(ext.to_string()), + None => ExternalReadFailure::Internal(e), + } +} + +/// Turn a provider failure into something a client can read and act on. +/// +/// Without this every rejection reaches the browser as a 500 carrying a raw upstream body. +/// The provider's status is deliberately not mirrored: a 401/403 answered by Windmill reads as +/// a Windmill permission problem, when the account that lacks rights is the one connected to +/// the *provider*. Errors that did not come from a provider call pass through untouched. +pub fn map_external_error(e: Error) -> Error { + map_external_error_with(e, |message| message) +} + +/// `map_external_error` with a chance to add what the failure means for Windmill's own state. +pub fn map_external_error_with(e: Error, decorate: impl FnOnce(String) -> String) -> Error { + let Some((status, transient, message)) = + as_external_error(&e).map(|ext| (ext.status, ext.is_transient(), ext.to_string())) + else { + return e; + }; + let message = decorate(message); + match status { + // A refusal is the caller's to fix; being unable to serve the request now is not, and + // the two want different reactions from whoever reads it. + _ if transient => Error::BadGateway(message), + Some(StatusCode::NOT_FOUND) => Error::NotFound(message), + Some(_) => Error::BadRequest(message), + // No status at all: the provider was unreachable or answered something undecodable. + None => Error::BadGateway(message), + } +} + +fn is_transient_status(status: StatusCode) -> bool { + status.is_server_error() + || matches!( + status, + StatusCode::REQUEST_TIMEOUT | StatusCode::TOO_MANY_REQUESTS + ) +} + /// Read OAuth client_id and client_secret from instance-level global settings. /// Used when a workspace integration has `instance_shared: true`. async fn get_instance_oauth_credentials( @@ -652,24 +855,98 @@ struct RefreshTokenResponse { refresh_token: Option, } +/// Why an OAuth token refresh did not produce a new token. +#[derive(Debug)] +pub struct RefreshFailure { + /// The token endpoint refused the grant, so the stored credentials are what to fix. + /// + /// Nothing else about the answer is carried further. The status a caller reads off an + /// `ExternalApiError` says what the call for the *trigger* answered, and 404 there means + /// the trigger is gone — it records that on the row and lets a delete drop it. A wrong + /// base URL 404s the token endpoint while the webhook is perfectly alive. + pub rejected: bool, + pub message: String, +} + +#[cfg(feature = "native_trigger")] +impl RefreshFailure { + fn rejected(message: String) -> Self { + RefreshFailure { rejected: true, message } + } + + fn outage(message: String) -> Self { + RefreshFailure { rejected: false, message } + } +} + +/// Whether a token endpoint's answer means reconnecting the integration is the fix, rather +/// than the endpoint failing to serve a grant that may well be valid. +/// +/// Used when the answer carried no OAuth error code to read. The body is checked because no +/// status reliably reports a refusal: GitHub answers `bad_refresh_token` with HTTP 200. The +/// status is the last resort — on a token endpoint these three most often mean the stored +/// grant is spent, and there is nothing better to go on. +pub fn grant_refused(status: Option, body: &str) -> bool { + body.contains("invalid_grant") + || body.contains("bad_refresh_token") + || matches!( + status, + Some(StatusCode::BAD_REQUEST | StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN) + ) +} + +/// Whether an OAuth error code (RFC 6749 §5.2) means the credentials Windmill stored are what +/// needs replacing. The rest — a malformed request, an unsupported grant type, a scope the +/// server will not grant — are faults in how the integration asks, which reconnecting as the +/// same user will reproduce exactly. +#[cfg(feature = "native_trigger")] +fn code_means_reconnect(code: &ErrorField) -> bool { + matches!(code, ErrorField::InvalidGrant | ErrorField::InvalidClient) +} + +#[cfg(feature = "native_trigger")] +fn refresh_failure_from(e: ExecuteError) -> RefreshFailure { + let rejected = match &e { + // A code says exactly what was refused, so nothing else needs guessing at. + ExecuteError::ErrorResponse { error, .. } => code_means_reconnect(&error.error), + // A body that would not deserialize is where a refusal hides when the status says + // nothing, and it is the only place the reason is written down. + ExecuteError::BadResponse { status, body, .. } => { + grant_refused(Some(*status), &String::from_utf8_lossy(body)) + } + _ => grant_refused(e.status(), ""), + }; + let message = match &e { + ExecuteError::BadResponse { body, .. } => { + format!("{e}: {}", String::from_utf8_lossy(body)) + } + _ => error_source_chain(&e), + }; + if rejected { + RefreshFailure::rejected(message) + } else { + RefreshFailure::outage(message) + } +} + /// Refresh OAuth tokens using windmill-oauth. #[cfg(feature = "native_trigger")] pub async fn refresh_oauth_tokens( oauth_config: &OAuthConfig, refresh_endpoint: &str, auth_endpoint: &str, -) -> Result { +) -> std::result::Result { let refresh_token_str = oauth_config .refresh_token .as_ref() - .ok_or_else(|| Error::InternalErr("No refresh token available".to_string()))?; + .ok_or_else(|| RefreshFailure::rejected("no refresh token is stored".to_string()))?; // Build OAuth client for token refresh // Auth URL is not used for refresh, but required by the client constructor let auth_url = Url::parse(&resolve_endpoint(&oauth_config.base_url, auth_endpoint)) - .map_err(|e| Error::InternalErr(format!("Invalid auth URL: {}", e)))?; + .map_err(|e| RefreshFailure::outage(format!("invalid auth URL: {e}")))?; let token_url = Url::parse(&resolve_endpoint(&oauth_config.base_url, refresh_endpoint)) - .map_err(|e| Error::InternalErr(format!("Invalid token URL: {}", e)))?; + .map_err(|e| RefreshFailure::outage(format!("invalid token URL: {e}")))?; let mut client = OClient::new(oauth_config.client_id.clone(), auth_url, token_url); client.set_client_secret(oauth_config.client_secret.clone()); @@ -679,7 +956,7 @@ pub async fn refresh_oauth_tokens( .with_client(&*OAUTH_HTTP_CLIENT) .execute() .await - .map_err(|e| Error::InternalErr(format!("Failed to refresh token: {:?}", e)))?; + .map_err(refresh_failure_from)?; Ok(OAuthConfig { base_url: oauth_config.base_url.clone(), @@ -698,10 +975,11 @@ pub async fn refresh_oauth_tokens( _oauth_config: &OAuthConfig, _refresh_endpoint: &str, _auth_endpoint: &str, -) -> Result { - Err(Error::InternalErr( - "Native triggers feature is not enabled".to_string(), - )) +) -> std::result::Result { + Err(RefreshFailure { + rejected: false, + message: "the native_trigger feature is not enabled".to_string(), + }) } async fn update_oauth_token_resource( diff --git a/backend/windmill-native-triggers/src/nextcloud/external.rs b/backend/windmill-native-triggers/src/nextcloud/external.rs index d46c0d4a21..763ad6fbb6 100644 --- a/backend/windmill-native-triggers/src/nextcloud/external.rs +++ b/backend/windmill-native-triggers/src/nextcloud/external.rs @@ -9,13 +9,14 @@ use windmill_common::{ }; use crate::{ - generate_webhook_service_url, + generate_webhook_service_url, http_error_status, nextcloud::{ routes, NextCloud, NextCloudOAuthData, NextCloudTriggerData, NextcloudServiceConfig, OcsResponse, }, External, NativeTriggerData, ServiceName, }; +use http::StatusCode; lazy_static::lazy_static! { pub static ref TOKEN_NEEDED: Box = to_raw_value(&serde_json::json!({ @@ -235,11 +236,13 @@ impl External for NextCloud { let mut headers = HashMap::new(); headers.insert("OCS-APIRequest".to_string(), "true".to_string()); + // A webhook already removed in Nextcloud is the outcome this call wants, so only 404 is + // swallowed; anything else would leave a live webhook behind while Windmill forgets it. let _: serde_json::Value = self .http_client_request::<_, ()>(&url, Method::DELETE, w_id, db, Some(headers), None) .await - .or_else(|e| match &e { - Error::InternalErr(msg) if msg.contains("404") => Ok(serde_json::Value::Null), + .or_else(|e| match http_error_status(&e) { + Some(StatusCode::NOT_FOUND) => Ok(serde_json::Value::Null), _ => Err(e), })?; @@ -265,7 +268,10 @@ impl External for NextCloud { ); errors.push(crate::sync::SyncError { resource_path: format!("workspace:{}", workspace_id), - error_message: format!("Failed to fetch external triggers: {}", e), + error_message: format!( + "Failed to fetch external triggers: {}", + crate::external_error_message(&e) + ), error_type: "external_service_error".to_string(), }); return; @@ -303,6 +309,28 @@ impl External for NextCloud { fn additional_routes(&self) -> axum::Router { routes::nextcloud_routes(self.clone()) } + + fn describe_error_body(&self, body: &str) -> Option { + let ocs: OcsResponse = serde_json::from_str(body).ok()?; + Some(ocs.ocs.meta.message).filter(|m| !m.is_empty()) + } + + /// Appended to every failed call of this service, so a hint may not assert what the caller + /// was doing — only name the requirement that most often explains the status. + fn error_hint(&self, status: StatusCode) -> Option<&'static str> { + match status { + StatusCode::UNAUTHORIZED => { + Some("reconnect the Nextcloud integration from Workspace settings > Integrations.") + } + StatusCode::FORBIDDEN => Some( + "Nextcloud grants webhook management to administrators only, and a Windmill \ + admin is a different thing. If the connected Nextcloud account is not an admin \ + there and holds no delegated admin rights for the Webhooks setting, reconnect \ + the integration from Workspace settings > Integrations with one that does.", + ), + _ => None, + } + } } impl NextCloud { diff --git a/backend/windmill-native-triggers/src/nextcloud/routes.rs b/backend/windmill-native-triggers/src/nextcloud/routes.rs index 2f54124d64..a1c679602c 100644 --- a/backend/windmill-native-triggers/src/nextcloud/routes.rs +++ b/backend/windmill-native-triggers/src/nextcloud/routes.rs @@ -10,7 +10,7 @@ use windmill_common::{ use windmill_api_auth::ApiAuthed; use crate::{ - get_workspace_integration, + get_workspace_integration, map_external_error, nextcloud::{NextCloudEventType, OcsResponse}, require_native_integration_use, External, ServiceName, }; @@ -47,7 +47,8 @@ async fn list_available_events( Some(headers), None, ) - .await?; + .await + .map_err(map_external_error)?; let events = serde_json::from_str(&ocs_response.ocs.data) .map_err(|e| Error::InternalErr(format!("Failed to parse NextCloud events data: {}", e)))?; diff --git a/backend/windmill-oauth/src/lib.rs b/backend/windmill-oauth/src/lib.rs index e314f13d75..32648a2115 100644 --- a/backend/windmill-oauth/src/lib.rs +++ b/backend/windmill-oauth/src/lib.rs @@ -32,7 +32,8 @@ pub type DB = sqlx::Pool; // Re-export oauth2 types that consumers need (also used internally) pub use oauth2::{ - helpers, AccessToken, AuthType, Client as OClient, RefreshToken, Scope, State, Url, + helpers, AccessToken, AuthType, Client as OClient, ErrorField, ExecuteError, RefreshToken, + Scope, State, Url, }; // Re-export reqwest Client (version 0.12 compatible with async-oauth2) diff --git a/frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte b/frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte index 7c14d1c099..66192f184d 100644 --- a/frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte +++ b/frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte @@ -14,7 +14,7 @@ import TextInput from '$lib/components/text_input/TextInput.svelte' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' - import { Loader2, Save } from 'lucide-svelte' + import { Loader2, RefreshCw, Save } from 'lucide-svelte' import ScriptPicker from '$lib/components/ScriptPicker.svelte' import Section from '$lib/components/Section.svelte' import Required from '$lib/components/Required.svelte' @@ -25,6 +25,7 @@ import { handleConfigChange, type Trigger } from '$lib/components/triggers/utils' import { deepEqual } from 'fast-equals' import type { Snippet } from 'svelte' + import Alert from '$lib/components/common/alert/Alert.svelte' interface Props { service: NativeServiceName @@ -103,6 +104,9 @@ let can_write = $state(true) let originalConfig = $state | undefined>(undefined) let initialConfig = $state | undefined>(undefined) + let loadError = $state(undefined) + let externalError = $state(undefined) + let retryEdit = $state<(() => void) | undefined>(undefined) export function openNew( nis_flow?: boolean, @@ -129,6 +133,9 @@ originalConfig = undefined initialConfig = undefined summary = '' + loadError = undefined + externalError = undefined + retryEdit = undefined } export function openRecreate(nativeTrigger: ExtendedNativeTrigger) { @@ -153,6 +160,9 @@ originalConfig = undefined initialConfig = undefined summary = nativeTrigger.summary ?? '' + loadError = undefined + externalError = undefined + retryEdit = undefined } export async function openEdit( @@ -177,6 +187,15 @@ originalConfig = undefined initialConfig = undefined itemKind = nis_flow ? 'flow' : 'script' + loadError = undefined + externalError = undefined + retryEdit = undefined + // A failed load must not leave the previously edited trigger's target and config in the + // form, where saving would silently repoint this trigger at them. + serviceConfig = {} + scriptPath = '' + initialScriptPath = '' + summary = '' try { const fullTrigger = await NativeTriggerService.getNativeTrigger({ @@ -191,6 +210,7 @@ can_write = canWrite(fullTrigger.script_path, {}, $userStore) summary = fullTrigger.summary ?? '' externalData = fullTrigger.external_data + externalError = fullTrigger.external_error ?? undefined // Apply default values if provided (for draft triggers) if (defaultValues) { @@ -198,8 +218,14 @@ externalData = { ...externalData, ...defaultValues } } } catch (err: any) { - sendUserToast(`Failed to load trigger configuration: ${err}`, true) + loadError = err.body ?? err.message ?? String(err) + sendUserToast(`Failed to load trigger configuration: ${loadError}`, true) externalData = null + // The service form is not rendered in the error state, so nothing else will ever + // clear its loading flag or narrow the permission left over from the last trigger. + loadingForm = false + can_write = false + retryEdit = () => openEdit(externalIdOrPath, nis_flow, defaultValues) } finally { clearTimeout(loadingTimeout) loadingConfig = false @@ -256,7 +282,8 @@ loadingConfig || loadingForm || !can_write || - !hasChanged + !hasChanged || + loadError !== undefined ) const saveCfg = $derived.by(getSaveCfg) @@ -388,12 +415,41 @@ {#snippet content()} {#if loadingConfig && showLoading} + {:else if loadError} + +
+ {loadError} + +
+ {:else}
{#if description} {@render description()} {/if}
+ {#if externalError} +
+ + {externalError} The configuration below is the one Windmill last saved. + +
+ {/if}
diff --git a/frontend/src/lib/components/triggers/native/services/nextcloud/NextcloudTriggerForm.svelte b/frontend/src/lib/components/triggers/native/services/nextcloud/NextcloudTriggerForm.svelte index cee2980b27..c282797b9b 100644 --- a/frontend/src/lib/components/triggers/native/services/nextcloud/NextcloudTriggerForm.svelte +++ b/frontend/src/lib/components/triggers/native/services/nextcloud/NextcloudTriggerForm.svelte @@ -35,6 +35,7 @@ let availableEvents = $state([]) let serviceSchema = $state(null) + let eventsError = $state(undefined) async function loadAvailableEvents() { if (!$workspaceStore) { @@ -43,6 +44,7 @@ } loading = true + eventsError = undefined try { const events = await NativeTriggerService.listNextCloudEvents({ workspace: $workspaceStore! @@ -51,7 +53,8 @@ serviceSchema = getNextcloudSchema(events) } catch (err: any) { console.error('Failed to load NextCloud events:', err) - sendUserToast(`Failed to load available events: ${err.body || err.message}`, true) + eventsError = err.body || err.message || String(err) + sendUserToast(`Failed to load available events: ${eventsError}`, true) availableEvents = [] } finally { loading = false @@ -108,9 +111,13 @@
{:else if availableEvents.length === 0}
-
No events available. Please ensure your workspace has a connected Nextcloud integration.
+ {#if eventsError} +
Could not load the available events: {eventsError}
+ {:else} +
No events available. Please ensure your workspace has a connected Nextcloud integration.
+ {/if}