From 2bf11dcb15540c538ea2ac3cf70dcbe589060b4e Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 29 May 2026 00:33:44 +0200 Subject: [PATCH] feat(oauth): support per-provider sandbox URLs (#9358) * feat(oauth): support per-provider sandbox URLs in registry + instance settings * fix(oauth): polish sandbox review nits (cc lookup, header label, ee ref) * refactor(oauth): drop dead build_oauth_clients duplicate in windmill-oauth * refactor(oauth): derive sandbox-capable provider list from registry * chore(docker): copy oauth_connect.json into frontend build stage * test(oauth): cover sandbox helpers (as_sandbox, canonical_name, resolve) * chore: update ee-repo-ref to 9297d8f790346e6a6ad540c7bca1a67f91ec11a2 This commit updates the EE repository reference after PR #595 was merged in windmill-ee-private. Previous ee-repo-ref: 3ab3eca9ac15ebab6db991e7964bc5e48ce21f42 New ee-repo-ref: 9297d8f790346e6a6ad540c7bca1a67f91ec11a2 Automated by sync-ee-ref workflow. --------- Co-authored-by: windmill-internal-app[bot] --- Dockerfile | 1 + backend/ee-repo-ref.txt | 2 +- backend/oauth_connect.json | 6 +- .../windmill-common/src/instance_config.rs | 15 + backend/windmill-oauth/src/lib.rs | 393 +++++++++--------- docker/RHEL8/Dockerfile | 1 + docker/RHEL9/Dockerfile | 1 + .../src/lib/components/AppConnectInner.svelte | 35 +- .../src/lib/components/AuthSettings.svelte | 50 ++- frontend/svelte.config.js | 3 +- 10 files changed, 282 insertions(+), 225 deletions(-) diff --git a/Dockerfile b/Dockerfile index e11cf9cecd..9062a4d9d8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -66,6 +66,7 @@ RUN npm ci COPY frontend . RUN mkdir /backend COPY /backend/windmill-api/openapi.yaml /backend/windmill-api/openapi.yaml +COPY /backend/oauth_connect.json /backend/oauth_connect.json COPY /openflow.openapi.yaml /openflow.openapi.yaml COPY /backend/windmill-api/build_openapi.sh /backend/windmill-api/build_openapi.sh COPY /system_prompts/auto-generated /system_prompts/auto-generated diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index f457a34fb7..d4a79d49d1 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -a30079e75dc5b7d7413aa8ee20e40e80bfea9cbd +9297d8f790346e6a6ad540c7bca1a67f91ec11a2 diff --git a/backend/oauth_connect.json b/backend/oauth_connect.json index c9693b2311..d18c8c8d24 100644 --- a/backend/oauth_connect.json +++ b/backend/oauth_connect.json @@ -176,6 +176,10 @@ "token_url": "https://account.docusign.com/oauth/token", "scopes": [ "signature" - ] + ], + "sandbox": { + "auth_url": "https://account-d.docusign.com/oauth/auth", + "token_url": "https://account-d.docusign.com/oauth/token" + } } } diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index 1872b52140..2239868982 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -586,6 +586,21 @@ pub struct OAuthConfig { pub req_body_auth: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub grant_types: Vec, + /// Optional URL overrides for the provider's sandbox environment. + #[serde(skip_serializing_if = "Option::is_none")] + pub sandbox: Option, +} + +/// URL overrides for an OAuth provider's sandbox environment. +#[derive(Deserialize, Serialize, Clone, Debug, Default)] +#[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))] +pub struct OAuthSandboxOverride { + #[serde(skip_serializing_if = "Option::is_none")] + pub auth_url: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub token_url: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub userinfo_url: Option, } // --------------------------------------------------------------------------- diff --git a/backend/windmill-oauth/src/lib.rs b/backend/windmill-oauth/src/lib.rs index ea65a83ba4..874a859200 100644 --- a/backend/windmill-oauth/src/lib.rs +++ b/backend/windmill-oauth/src/lib.rs @@ -18,9 +18,7 @@ use std::collections::HashMap; use std::fmt::Debug; use anyhow::anyhow; -use base64::Engine; use hmac::Mac; -use itertools::Itertools; use serde::{de::DeserializeOwned, Deserialize, Serialize}; use sqlx::{Postgres, Transaction}; use tower_cookies::{Cookie, Cookies}; @@ -89,6 +87,76 @@ pub struct OAuthConfig { pub req_body_auth: Option, #[serde(default = "default_grant_types")] pub grant_types: Vec, + /// Optional URL overrides for the provider's sandbox environment. When + /// present and the admin has configured a `_sandbox` credentials + /// entry, `build_oauth_clients` registers a second client under that key. + #[serde(skip_serializing_if = "Option::is_none")] + pub sandbox: Option, +} + +/// URL overrides for an OAuth provider's sandbox environment. Inherits +/// scopes, extra_params, etc. from the parent [`OAuthConfig`]. +#[derive(Clone, Debug, Default, Serialize, Deserialize)] +pub struct OAuthSandboxOverride { + #[serde(skip_serializing_if = "Option::is_none")] + pub auth_url: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub token_url: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub userinfo_url: Option, +} + +impl OAuthConfig { + /// Returns a copy of this config with sandbox URL overrides applied and + /// the nested `sandbox` field cleared. Returns `None` if no overrides are + /// set. + pub fn as_sandbox(&self) -> Option { + let sb = self.sandbox.as_ref()?; + let mut out = self.clone(); + out.sandbox = None; + if let Some(u) = &sb.auth_url { + out.auth_url = u.clone(); + } + if let Some(u) = &sb.token_url { + out.token_url = u.clone(); + } + if sb.userinfo_url.is_some() { + out.userinfo_url = sb.userinfo_url.clone(); + } + Some(out) + } +} + +/// Suffix appended to a provider name to identify its sandbox variant in the +/// instance credentials map and in `account.client`. +pub const SANDBOX_SUFFIX: &str = "_sandbox"; + +/// Strips [`SANDBOX_SUFFIX`] from a client name, returning the canonical +/// provider name. Returns the input unchanged if no suffix is present. +pub fn canonical_provider_name(client_name: &str) -> &str { + client_name + .strip_suffix(SANDBOX_SUFFIX) + .unwrap_or(client_name) +} + +/// Resolves a registry [`OAuthConfig`] for `client_name`, transparently +/// applying the `sandbox` override block when the name carries the sandbox +/// suffix (e.g. `docusign_sandbox` resolves to `docusign` with sandbox URLs +/// applied). Used so callers don't need to know whether a name is a sandbox +/// variant before looking it up. +pub fn resolve_registry_config( + static_configs: &HashMap, + client_name: &str, +) -> Option { + if let Some(cfg) = static_configs.get(client_name) { + return Some(cfg.clone()); + } + if client_name.ends_with(SANDBOX_SUFFIX) { + return static_configs + .get(canonical_provider_name(client_name)) + .and_then(|cfg| cfg.as_sandbox()); + } + None } /// OAuth client credentials @@ -181,181 +249,6 @@ pub struct OAuthCallback { pub state: String, } -/// Build all OAuth clients from configuration -pub async fn build_oauth_clients( - base_url: &str, - oauths_from_config: Option>, - connect_configs_json: &str, - login_configs_json: &str, -) -> anyhow::Result { - let connect_configs = - serde_json::from_str::>(connect_configs_json)?; - let login_configs = serde_json::from_str::>(login_configs_json)?; - - let oauths = if let Some(oauths) = oauths_from_config { - tracing::info!("Using OAuth clients from config: {oauths:?}"); - oauths - } else { - let path = "./oauth.json"; - let content: String = if let Ok(e) = std::env::var("OAUTH_JSON_AS_BASE64") { - std::str::from_utf8( - &base64::engine::general_purpose::STANDARD - .decode(e) - .map_err(to_anyhow)?, - )? - .to_string() - } else if std::path::Path::new(path).exists() { - std::fs::read_to_string(path).map_err(to_anyhow)? - } else { - tracing::warn!("oauth.json not found, no OAuth clients loaded"); - return Ok(AllClients { - logins: HashMap::new(), - connects: HashMap::new(), - slack: None, - }); - }; - - if content.is_empty() { - tracing::warn!("oauth.json is empty, no OAuth clients loaded"); - return Ok(AllClients { - logins: HashMap::new(), - connects: HashMap::new(), - slack: None, - }); - }; - match serde_json::from_str::>(&content) { - Ok(clients) => clients, - Err(e) => { - tracing::error!("deserializing oauth.json: {e}"); - HashMap::new() - } - } - .into_iter() - .collect() - }; - - tracing::info!("OAuth loaded clients: {}", oauths.keys().join(", ")); - - let logins = login_configs - .into_iter() - .filter_map(|x| oauths.get(&x.0).map(|c| (x.0, (c, x.1)))) - .chain(oauths.iter().filter_map(|x| { - x.1.login_config - .as_ref() - .map(|c| (x.0.clone(), (x.1, c.clone()))) - })) - .filter_map(|(k, (client_params, config))| { - let named_client = build_basic_client( - k.clone(), - config.clone(), - client_params.clone(), - true, - base_url, - None, - ); - named_client - .map(|named_client| { - ( - named_client.0, - ClientWithScopes { - client: named_client.1, - scopes: config.scopes.unwrap_or(vec![]), - extra_params: config.extra_params, - extra_params_callback: config.extra_params_callback, - allowed_domains: client_params.allowed_domains.clone(), - userinfo_url: config.userinfo_url, - display_name: client_params.display_name.clone(), - grant_types: client_params.grant_types.clone(), - }, - ) - }) - .map_err(|e| { - tracing::error!("Error building oauth client {k}: {e}"); - e - }) - .ok() - }) - .collect(); - - let connects = connect_configs - .into_iter() - .filter_map(|x| oauths.get(&x.0).map(|c| (x.0, (c, x.1)))) - .chain(oauths.iter().filter_map(|x| { - x.1.connect_config - .as_ref() - .map(|c| (x.0.clone(), (x.1, c.clone()))) - })) - .filter_map(|(k, (client_params, config))| { - let named_client = build_basic_client( - k.clone(), - config.clone(), - client_params.clone(), - false, - base_url, - if k == "supabase_wizard" { - Some(format!("{base_url}/oauth/callback_supabase")) - } else { - None - }, - ); - named_client - .map(|named_client| { - ( - named_client.0, - ClientWithScopes { - client: named_client.1, - scopes: config.scopes.unwrap_or(vec![]), - extra_params: config.extra_params, - extra_params_callback: config.extra_params_callback, - allowed_domains: None, - userinfo_url: None, - display_name: client_params.display_name.clone(), - grant_types: client_params.grant_types.clone(), - }, - ) - }) - .map_err(|e| { - tracing::error!("Error building oauth client {k}: {e}"); - e - }) - .ok() - }) - .collect(); - - let slack = oauths - .get("slack") - .map(|v| { - build_basic_client( - "slack".to_string(), - OAuthConfig { - auth_url: "https://slack.com/oauth/v2/authorize".to_string(), - token_url: "https://slack.com/api/oauth.v2.access".to_string(), - userinfo_url: None, - scopes: None, - extra_params: None, - extra_params_callback: None, - req_body_auth: None, - grant_types: vec!["authorization_code".to_string()], - }, - v.clone(), - false, - base_url, - Some(format!("{base_url}/oauth/callback_slack")), - ) - .map(|x| x.1) - .map_err(|e| { - tracing::error!("Error building oauth slack client: {e}"); - e - }) - .ok() - }) - .flatten(); - - let all_clients = AllClients { logins, connects, slack }; - tracing::debug!("Final oauth config: {all_clients:#?}"); - Ok(all_clients) -} - /// Build a basic OAuth client from configuration pub fn build_basic_client( name: String, @@ -433,38 +326,29 @@ pub async fn build_client_credentials_oauth_client( let oauth_client_config: OAuthClient = serde_json::from_value(oauth_config.clone()) .map_err(|e| error::Error::BadRequest(format!("Invalid OAuth config: {}", e)))?; - let mut connect_config = if let Some(ref config) = oauth_client_config.connect_config { - if !config.auth_url.is_empty() && !config.token_url.is_empty() { - config.clone() - } else { - let static_configs = - serde_json::from_str::>(connect_configs_json) - .map_err(|e| { - error::Error::InternalErr(format!( - "Failed to parse oauth_connect.json: {}", - e - )) - })?; - - static_configs.get(client_name).cloned().ok_or_else(|| { - error::Error::BadRequest(format!( - "OAuth configuration not found for '{}' in either global settings or static config", - client_name - )) - })? - } - } else { - let static_configs = - serde_json::from_str::>(connect_configs_json).map_err( - |e| error::Error::InternalErr(format!("Failed to parse oauth_connect.json: {}", e)), - )?; - - static_configs.get(client_name).cloned().ok_or_else(|| { + let parse_static_configs = || { + serde_json::from_str::>(connect_configs_json).map_err(|e| { + error::Error::InternalErr(format!("Failed to parse oauth_connect.json: {}", e)) + }) + }; + let resolve_from_registry = |client_name: &str| -> error::Result { + let static_configs = parse_static_configs()?; + resolve_registry_config(&static_configs, client_name).ok_or_else(|| { error::Error::BadRequest(format!( "OAuth configuration not found for '{}' in either global settings or static config", client_name )) - })? + }) + }; + + let mut connect_config = if let Some(ref config) = oauth_client_config.connect_config { + if !config.auth_url.is_empty() && !config.token_url.is_empty() { + config.clone() + } else { + resolve_from_registry(client_name)? + } + } else { + resolve_from_registry(client_name)? }; if let Some(override_url) = cc_token_url_override { @@ -905,4 +789,103 @@ mod tests { let verifier = SlackVerifier::new("test_secret").unwrap(); assert!(verifier.verify("123", "body", "wrong_sig").is_err()); } + + #[test] + fn canonical_provider_name_strips_sandbox_suffix() { + assert_eq!(canonical_provider_name("docusign_sandbox"), "docusign"); + assert_eq!(canonical_provider_name("docusign"), "docusign"); + assert_eq!(canonical_provider_name(""), ""); + // Only strips the suffix once; trailing suffix on already-canonical name. + assert_eq!( + canonical_provider_name("foo_sandbox_sandbox"), + "foo_sandbox" + ); + } + + fn sample_oauth_config(with_sandbox: bool) -> OAuthConfig { + OAuthConfig { + auth_url: "https://account.example.com/oauth/auth".to_string(), + token_url: "https://account.example.com/oauth/token".to_string(), + userinfo_url: Some("https://account.example.com/userinfo".to_string()), + scopes: Some(vec!["signature".to_string()]), + extra_params: None, + extra_params_callback: None, + req_body_auth: None, + grant_types: default_grant_types(), + sandbox: with_sandbox.then(|| OAuthSandboxOverride { + auth_url: Some("https://account-d.example.com/oauth/auth".to_string()), + token_url: Some("https://account-d.example.com/oauth/token".to_string()), + userinfo_url: None, + }), + } + } + + #[test] + fn as_sandbox_returns_none_when_no_override() { + assert!(sample_oauth_config(false).as_sandbox().is_none()); + } + + #[test] + fn as_sandbox_overlays_urls_and_inherits_rest() { + let resolved = sample_oauth_config(true).as_sandbox().unwrap(); + // URLs overridden by sandbox block + assert_eq!( + resolved.auth_url, + "https://account-d.example.com/oauth/auth" + ); + assert_eq!( + resolved.token_url, + "https://account-d.example.com/oauth/token" + ); + // userinfo_url not in override → inherits from parent + assert_eq!( + resolved.userinfo_url, + Some("https://account.example.com/userinfo".to_string()) + ); + // Scopes/grant_types inherited from parent + assert_eq!(resolved.scopes, Some(vec!["signature".to_string()])); + assert_eq!(resolved.grant_types, default_grant_types()); + // Nested sandbox field cleared on the resolved config + assert!(resolved.sandbox.is_none()); + } + + #[test] + fn resolve_registry_config_direct_lookup() { + let mut registry = HashMap::new(); + registry.insert("docusign".to_string(), sample_oauth_config(true)); + + let resolved = resolve_registry_config(®istry, "docusign").unwrap(); + assert_eq!(resolved.auth_url, "https://account.example.com/oauth/auth"); + // Direct lookup returns the entry as-is (sandbox block still attached). + assert!(resolved.sandbox.is_some()); + } + + #[test] + fn resolve_registry_config_sandbox_fallback() { + let mut registry = HashMap::new(); + registry.insert("docusign".to_string(), sample_oauth_config(true)); + + let resolved = resolve_registry_config(®istry, "docusign_sandbox").unwrap(); + // Sandbox-suffixed lookup resolves to parent's sandbox-overlaid config. + assert_eq!( + resolved.auth_url, + "https://account-d.example.com/oauth/auth" + ); + assert!(resolved.sandbox.is_none()); + } + + #[test] + fn resolve_registry_config_missing_returns_none() { + let registry: HashMap = HashMap::new(); + assert!(resolve_registry_config(®istry, "docusign").is_none()); + assert!(resolve_registry_config(®istry, "docusign_sandbox").is_none()); + } + + #[test] + fn resolve_registry_config_sandbox_without_block_returns_none() { + let mut registry = HashMap::new(); + // Parent exists but has no sandbox override. + registry.insert("docusign".to_string(), sample_oauth_config(false)); + assert!(resolve_registry_config(®istry, "docusign_sandbox").is_none()); + } } diff --git a/docker/RHEL8/Dockerfile b/docker/RHEL8/Dockerfile index cb5f36cef5..500050de67 100644 --- a/docker/RHEL8/Dockerfile +++ b/docker/RHEL8/Dockerfile @@ -30,6 +30,7 @@ RUN npm ci COPY frontend . RUN mkdir /backend COPY /backend/windmill-api/openapi.yaml /backend/windmill-api/openapi.yaml +COPY /backend/oauth_connect.json /backend/oauth_connect.json COPY /openflow.openapi.yaml /openflow.openapi.yaml COPY /backend/windmill-api/build_openapi.sh /backend/windmill-api/build_openapi.sh COPY /system_prompts/auto-generated /system_prompts/auto-generated diff --git a/docker/RHEL9/Dockerfile b/docker/RHEL9/Dockerfile index 6d96804381..a0fff8dd91 100644 --- a/docker/RHEL9/Dockerfile +++ b/docker/RHEL9/Dockerfile @@ -30,6 +30,7 @@ RUN npm ci COPY frontend . RUN mkdir /backend COPY /backend/windmill-api/openapi.yaml /backend/windmill-api/openapi.yaml +COPY /backend/oauth_connect.json /backend/oauth_connect.json COPY /openflow.openapi.yaml /openflow.openapi.yaml COPY /backend/windmill-api/build_openapi.sh /backend/windmill-api/build_openapi.sh COPY /system_prompts/auto-generated /system_prompts/auto-generated diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index 4f0a5a76b2..7b6ed37ecc 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -74,6 +74,16 @@ let value: string = $state('') let valueToken: TokenResponse | undefined = undefined let connects: string[] | undefined = $state(undefined) + + const SANDBOX_SUFFIX = '_sandbox' + function stripSandboxSuffix(name: string): string { + return name.endsWith(SANDBOX_SUFFIX) ? name.slice(0, -SANDBOX_SUFFIX.length) : name + } + // `resourceType` is always the canonical type (e.g. `docusign`) so resource + // rows are uniform. `connectClient` carries the suffixed OAuth client name + // (e.g. `docusign_sandbox`) used to look up credentials/URLs at runtime + // and stored on `account.client` so token refresh hits the right endpoint. + let connectClient: string = $state('') let connectsManual: { key: string; img?: string; instructions: string[] }[] | undefined = $state(undefined) let args: any = $state({}) @@ -152,7 +162,9 @@ description = '' labels = undefined wsSpecific = false - resourceType = rt ?? '' + const rawRt = rt ?? '' + connectClient = rawRt + resourceType = stripSandboxSuffix(rawRt) valueToken = undefined // Reset client credentials state @@ -163,7 +175,7 @@ tokenUrl = '' await loadConnects() - manual = !connects?.includes(resourceType) + manual = !connects?.includes(connectClient) if (manual && express) { dispatch('error', 'Express OAuth setup is not available for non OAuth resource types') return @@ -312,7 +324,8 @@ sendUserToast(data.error, true) step = 2 } else if (data.type === 'success') { - resourceType = data.resource_type + connectClient = data.resource_type + resourceType = stripSandboxSuffix(connectClient) value = data.res.access_token! valueToken = data.res responseExtra = data.extra ?? {} @@ -325,7 +338,7 @@ } async function getScopesAndParams() { - const connect = await OauthService.getOauthConnect({ client: resourceType }) + const connect = await OauthService.getOauthConnect({ client: connectClient }) scopes = connect.scopes ?? [] extra_params = Object.entries(connect.extra_params ?? {}) as [string, string][] @@ -401,7 +414,7 @@ } const tokenResponse = await OauthService.connectClientCredentials({ - client: resourceType, + client: connectClient, requestBody }) @@ -428,7 +441,7 @@ * Requires user interaction and consent * Opens popup for user to authenticate with OAuth provider */ - const url = new URL(`/api/oauth/connect/${resourceType}`, window.location.origin) + const url = new URL(`/api/oauth/connect/${connectClient}`, window.location.origin) url.searchParams.append('scopes', scopes.join('+')) if (extra_params.length > 0) { extra_params.forEach(([key, value]) => url.searchParams.append(key, value)) @@ -490,7 +503,7 @@ const accountData: any = { refresh_token: valueToken.refresh_token ?? '', expires_in: valueToken.expires_in, - client: resourceType, + client: connectClient, grant_type: valueToken.grant_type || 'authorization_code' } @@ -602,6 +615,7 @@ ) step = 1 resourceType = '' + connectClient = '' } } @@ -660,10 +674,11 @@