fix: report why a native trigger service refused instead of a 500 (#10463)

* fix: report why a native trigger service refused instead of a 500

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: keep the cause of an unreachable trigger service in the message

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: degrade a trigger read only for the service's own failures

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: tell a refresh outage apart from a rejected refresh grant

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: treat a rate-limited or timed-out service as an outage, not a refusal

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: keep a token endpoint's status out of the trigger's

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: read a refused refresh grant off the body, not only the status

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: let a throttled 403 read as an outage, not a permission refusal

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: classify a refresh refusal by its OAuth code, and Google quotas by domain

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: recognize GitHub's other wording for a throttled request

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-08-03 17:10:52 +00:00
committed by GitHub
parent 42543240e8
commit 4f03aa91a7
13 changed files with 703 additions and 84 deletions
@@ -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"
);
}
+10 -1
View File
@@ -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
@@ -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 {
@@ -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<GithubApiRepoResponse> = 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 {
@@ -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;
@@ -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
+69 -29
View File
@@ -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<T: Serialize> {
#[serde(flatten)]
pub windmill_data: NativeTrigger,
pub external_data: Option<T>,
/// 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<String>,
}
#[derive(Debug, Serialize)]
@@ -193,7 +199,8 @@ async fn create_native_trigger<T: External>(
&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<T: External>(
&db,
&mut tx,
)
.await?
.await
.map_err(map_external_error)?
};
let config = NativeTriggerConfig {
@@ -349,7 +357,8 @@ async fn update_native_trigger_handler<T: External>(
&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<T: External>(
.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<T: External>(
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<T: External>(
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?;
+305 -27
View File
@@ -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<String> {
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<T: DeserializeOwned + Send, B: Serialize + Send + Sync>(
&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<StatusCode>, detail: String) -> Error {
self.external_error_inner(status, detail, false)
}
fn external_error_inner(
&self,
status: Option<StatusCode>,
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<StatusCode>,
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<StatusCode> {
/// `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::<HttpRequestError>()
.and_then(|e| e.status()),
Error::Anyhow { error, .. } => error.downcast_ref::<ExternalApiError>(),
_ => 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<StatusCode> {
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<String>,
}
/// 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<StatusCode>, 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<OAuthConfig> {
) -> std::result::Result<OAuthConfig, RefreshFailure> {
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<OAuthConfig> {
Err(Error::InternalErr(
"Native triggers feature is not enabled".to_string(),
))
) -> std::result::Result<OAuthConfig, RefreshFailure> {
Err(RefreshFailure {
rejected: false,
message: "the native_trigger feature is not enabled".to_string(),
})
}
async fn update_oauth_token_resource(
@@ -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<serde_json::value::RawValue> = 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<String> {
let ocs: OcsResponse<serde_json::Value> = 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 {
@@ -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<T: External>(
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)))?;
+2 -1
View File
@@ -32,7 +32,8 @@ pub type DB = sqlx::Pool<sqlx::Postgres>;
// 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)
@@ -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<Record<string, any> | undefined>(undefined)
let initialConfig = $state<Record<string, any> | undefined>(undefined)
let loadError = $state<string | undefined>(undefined)
let externalError = $state<string | undefined>(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}
<Loader2 class="animate-spin" />
{:else if loadError}
<Alert
type="error"
title="Could not load this {serviceInfo?.serviceDisplayName} trigger"
descriptionClass="break-words"
>
<div class="flex flex-col gap-2 items-start">
<span>{loadError}</span>
<Button
size="xs"
variant="subtle"
startIcon={{ icon: RefreshCw }}
on:click={() => retryEdit?.()}
>
Retry
</Button>
</div>
</Alert>
{:else}
<div class="flex flex-col gap-4">
{#if description}
{@render description()}
{/if}
</div>
{#if externalError}
<div class="mt-4">
<Alert
type="warning"
title="Could not read this trigger from {serviceInfo?.serviceDisplayName}"
descriptionClass="break-words"
>
{externalError} The configuration below is the one Windmill last saved.
</Alert>
</div>
{/if}
<div class="flex flex-col gap-12 mt-6">
<Section headless>
<div class="flex flex-col gap-6">
@@ -35,6 +35,7 @@
let availableEvents = $state<NextCloudEventType[]>([])
let serviceSchema = $state<any>(null)
let eventsError = $state<string | undefined>(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 @@
</div>
{:else if availableEvents.length === 0}
<div class="text-red-500 text-xs space-y-2">
<div
>No events available. Please ensure your workspace has a connected Nextcloud integration.</div
>
{#if eventsError}
<div class="break-words">Could not load the available events: {eventsError}</div>
{:else}
<div
>No events available. Please ensure your workspace has a connected Nextcloud integration.</div
>
{/if}
<div class="flex gap-2">
<Button variant="default" on:click={loadAvailableEvents} {disabled}>
Retry loading events