diff --git a/backend/.sqlx/query-5af4e97b30d968e2b55f9560c6bb1c3f543964d2c05d2f44f0a7716f890a3fce.json b/backend/.sqlx/query-5af4e97b30d968e2b55f9560c6bb1c3f543964d2c05d2f44f0a7716f890a3fce.json new file mode 100644 index 0000000000..113d1d6ce0 --- /dev/null +++ b/backend/.sqlx/query-5af4e97b30d968e2b55f9560c6bb1c3f543964d2c05d2f44f0a7716f890a3fce.json @@ -0,0 +1,29 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT\n (SELECT created_by FROM resource WHERE workspace_id = $1 AND path = $2) AS \"owner?\",\n EXISTS(SELECT 1 FROM resource WHERE workspace_id = $1 AND path = $2)\n OR EXISTS(SELECT 1 FROM variable WHERE workspace_id = $1 AND path = $2) AS \"taken!\"", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "owner?", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "taken!", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + null, + null + ] + }, + "hash": "5af4e97b30d968e2b55f9560c6bb1c3f543964d2c05d2f44f0a7716f890a3fce" +} diff --git a/backend/.sqlx/query-e6d49c3186419c726cc6ed771548c72164b36ccb5434bcc4babefa7f3363c675.json b/backend/.sqlx/query-e6d49c3186419c726cc6ed771548c72164b36ccb5434bcc4babefa7f3363c675.json new file mode 100644 index 0000000000..99ec12fd85 --- /dev/null +++ b/backend/.sqlx/query-e6d49c3186419c726cc6ed771548c72164b36ccb5434bcc4babefa7f3363c675.json @@ -0,0 +1,34 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT script_path FROM native_trigger WHERE workspace_id = $1 AND service_name = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "script_path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + { + "Custom": { + "name": "native_trigger_service", + "kind": { + "Enum": [ + "nextcloud", + "google", + "github" + ] + } + } + } + ] + }, + "nullable": [ + false + ] + }, + "hash": "e6d49c3186419c726cc6ed771548c72164b36ccb5434bcc4babefa7f3363c675" +} diff --git a/backend/migrations/20260925162004_native_trigger_connection.up.sql b/backend/migrations/20260925162004_native_trigger_connection.up.sql index c5a2ae4695..3b235a8d4d 100644 --- a/backend/migrations/20260925162004_native_trigger_connection.up.sql +++ b/backend/migrations/20260925162004_native_trigger_connection.up.sql @@ -2,6 +2,20 @@ -- trigger of a service sharing the single workspace connection. ALTER TABLE native_trigger ADD COLUMN connection_path VARCHAR(255); +-- Saving the OAuth app, or starting a connect with the instance app, used to clear +-- resource_path while the connection itself stayed. A service then had at most one connection, +-- so when exactly one is found it is the one its triggers used. +UPDATE workspace_integrations wi +SET resource_path = ( + SELECT CASE WHEN count(*) = 1 THEN min(v.path) END + FROM variable v + JOIN account a ON a.workspace_id = v.workspace_id AND a.id = v.account + WHERE v.workspace_id = wi.workspace_id + AND a.client = wi.service_name::text + AND a.is_workspace_integration = true +) +WHERE wi.resource_path IS NULL; + UPDATE native_trigger nt SET connection_path = wi.resource_path FROM workspace_integrations wi diff --git a/backend/windmill-api-integration-tests/tests/native_triggers.rs b/backend/windmill-api-integration-tests/tests/native_triggers.rs index 66d07f53da..f2689d34c3 100644 --- a/backend/windmill-api-integration-tests/tests/native_triggers.rs +++ b/backend/windmill-api-integration-tests/tests/native_triggers.rs @@ -26,9 +26,10 @@ use windmill_native_triggers::{ grant_refused, http_error_status, list_native_triggers, list_usable_connections, map_external_error, native_trigger_is_enabled, nextcloud::NextCloud, - require_native_integration_use, set_native_trigger_enabled, store_native_trigger, - store_workspace_integration, update_native_trigger, update_oauth_token_resource, External, - ExternalReadFailure, HttpRequestError, NativeTriggerConfig, OAuthConfig, ServiceName, + require_native_integration_use, resolve_usable_connection, set_native_trigger_enabled, + store_native_trigger, store_workspace_integration, update_native_trigger, + update_oauth_token_resource, External, ExternalReadFailure, HttpRequestError, + NativeTriggerConfig, OAuthConfig, ServiceName, }; // ============================================================================ @@ -257,6 +258,18 @@ async fn test_workspace_connection_usable_by_members(db: Pool) -> anyh .await?; assert_eq!(usable_paths().await?, vec![(path.to_string(), true)]); + // Clients and specs that predate connections name none: they keep getting the shared + // account once the member also has their own. + let own = "u/test-user-2/native_nextcloud"; + setup_oauth_integration(&db, ServiceName::Nextcloud, own, "b", "s", None).await?; + let mut tx = UserDB::new(db.clone()).begin(&member).await?; + let default = + resolve_usable_connection(&db, &mut tx, "test-workspace", ServiceName::Nextcloud, None) + .await?; + tx.commit().await?; + assert_eq!(usable_paths().await?.len(), 2); + assert_eq!(default, path); + Ok(()) } diff --git a/backend/windmill-native-triggers/src/lib.rs b/backend/windmill-native-triggers/src/lib.rs index 0b4577ae3f..bceb569c9a 100644 --- a/backend/windmill-native-triggers/src/lib.rs +++ b/backend/windmill-native-triggers/src/lib.rs @@ -1821,7 +1821,9 @@ pub async fn workspace_connection( } /// Pick the connection a new trigger acts through: the one asked for if the caller may use it, -/// otherwise the caller's only usable connection. `tx` must be a user transaction. +/// otherwise the shared account, otherwise the caller's only usable connection. Clients and +/// trigger specs that predate connections send none and expect the shared account, so it wins +/// over a lone personal one. `tx` must be a user transaction. pub async fn resolve_usable_connection( db: &DB, tx: &mut PgConnection, @@ -1833,9 +1835,10 @@ pub async fn resolve_usable_connection( match requested { Some(path) if usable.iter().any(|c| c.path == path) => Ok(path.to_string()), Some(path) => Err(Error::NotAuthorized(format!( - "No {service_name} connection you can use at {path}. Connect your own account or ask \ - for read access to that connection." + "No {service_name} connection you can use at {path}. Connect an account at that \ + path, or ask for read access to the connection there." ))), + None if usable.first().is_some_and(|c| c.workspace) => Ok(usable[0].path.clone()), None => match usable.as_slice() { [only] => Ok(only.path.clone()), [] => Err(Error::BadRequest(format!( diff --git a/backend/windmill-native-triggers/src/workspace_integrations.rs b/backend/windmill-native-triggers/src/workspace_integrations.rs index 01ae351615..624bb9dfd2 100644 --- a/backend/windmill-native-triggers/src/workspace_integrations.rs +++ b/backend/windmill-native-triggers/src/workspace_integrations.rs @@ -230,6 +230,38 @@ fn check_connection_scopes(authed: &ApiAuthed, path: &str) -> Result<()> { check_scopes(authed, || format!("resources:write:{path}")) } +/// Reconnecting at a path swaps the account every trigger using it acts as, including the shared +/// account's, so only whoever connected it or an admin may replace what is there. Writing the +/// path is not enough: a folder writer would otherwise turn other members' triggers into their own. +#[cfg(feature = "native_trigger")] +async fn check_can_replace( + db: &DB, + authed: &ApiAuthed, + workspace_id: &str, + path: &str, +) -> Result<()> { + if authed.is_admin { + return Ok(()); + } + let existing = sqlx::query!( + r#"SELECT + (SELECT created_by FROM resource WHERE workspace_id = $1 AND path = $2) AS "owner?", + EXISTS(SELECT 1 FROM resource WHERE workspace_id = $1 AND path = $2) + OR EXISTS(SELECT 1 FROM variable WHERE workspace_id = $1 AND path = $2) AS "taken!""#, + workspace_id, + path, + ) + .fetch_one(db) + .await?; + if existing.taken && existing.owner.as_deref() != Some(authed.username.as_str()) { + return Err(Error::NotAuthorized(format!( + "{path} holds a connection someone else created. Connect at another path, or ask an \ + admin to reconnect it." + ))); + } + Ok(()) +} + #[cfg(feature = "native_trigger")] fn default_connection_path(authed: &ApiAuthed, service_name: ServiceName) -> String { format!( @@ -256,6 +288,7 @@ async fn generate_connect_url( .filter(|p| !p.is_empty()) .unwrap_or_else(|| default_connection_path(&authed, service_name)); check_connection_scopes(&authed, &connection_path)?; + check_can_replace(&db, &authed, &workspace_id, &connection_path).await?; // A GitHub OAuth app only redirects under its one registered callback, and the instance app's // is the resource connect callback, so its connections land on a page below that one. @@ -470,13 +503,18 @@ async fn delete_integration( ) -> JsonResult { require_admin(authed.is_admin, &workspace_id)?; - // Delete triggers first: removing their registrations needs the connections cleaned up below - delete_triggers_for_service(&db, &workspace_id, service_name, None).await; - - let mut tx = user_db.begin(&authed).await?; - - // Same serialization as a single disconnect: a trigger created meanwhile either committed - // before these locks, and is caught below, or waits and then finds its connection gone. + // This deletes every trigger and connection of the service, so a path-scoped token must + // cover each of them, as for a single disconnect. + let script_paths = sqlx::query_scalar!( + "SELECT script_path FROM native_trigger WHERE workspace_id = $1 AND service_name = $2", + workspace_id, + service_name as ServiceName, + ) + .fetch_all(&db) + .await?; + for script_path in &script_paths { + check_scopes(&authed, || format!("native_triggers:write:{script_path}"))?; + } let mut paths = sqlx::query_scalar!( "SELECT v.path FROM variable v JOIN account a ON a.workspace_id = v.workspace_id AND a.id = v.account @@ -486,6 +524,17 @@ async fn delete_integration( ) .fetch_all(&db) .await?; + for path in &paths { + check_connection_scopes(&authed, path)?; + } + + // Delete triggers first: removing their registrations needs the connections cleaned up below + delete_triggers_for_service(&db, &workspace_id, service_name, None).await; + + let mut tx = user_db.begin(&authed).await?; + + // Same serialization as a single disconnect: a trigger created meanwhile either committed + // before these locks, and is caught below, or waits and then finds its connection gone. paths.sort(); for path in &paths { lock_connection(&mut tx, &workspace_id, path).await?; @@ -661,6 +710,7 @@ async fn oauth_callback( ) .await?; check_connection_scopes(&authed, &resource_path)?; + check_can_replace(&db, &authed, &workspace_id, &resource_path).await?; let (oauth_config, is_instance_shared) = resolve_oauth_client(&db, &workspace_id, service_name).await?; @@ -1121,6 +1171,8 @@ async fn set_workspace_connection( require_admin(authed.is_admin, &workspace_id)?; if let Some(path) = path.as_deref() { + // Sharing lets every member act as the account, so a path-scoped token must cover it. + check_connection_scopes(&authed, path)?; let exists = sqlx::query_scalar!( "SELECT EXISTS(SELECT 1 FROM variable v JOIN account a ON a.workspace_id = v.workspace_id AND a.id = v.account