diff --git a/.agents/skills/native-trigger/SKILL.md b/.agents/skills/native-trigger/SKILL.md index 6342ec3e48..a4ff8e121a 100644 --- a/.agents/skills/native-trigger/SKILL.md +++ b/.agents/skills/native-trigger/SKILL.md @@ -131,7 +131,7 @@ accounts) are many per service. Each trigger acts through one connection, named A user may use a connection when they can read its variable (checked with a user transaction, so the variable's row-level security decides): `resolve_usable_connection` / `list_usable_connections` in `lib.rs`. The exception is the **workspace connection**: an admin -may share one connection they made per service (`workspace_integrations.resource_path`, set by +may share one connection per service (`workspace_integrations.resource_path`, set by `POST .../workspace_connection`), and every member may then use it without reading its token. Renames carry the marker along; disconnecting clears it. Background work (sync, renewal, rename re-registration, webhook handling) uses the stored `connection_path` directly. diff --git a/backend/.sqlx/query-72c123eb6a0d66f4e57d86694581772154b09f4a89903462875e57c9760a19e8.json b/backend/.sqlx/query-1966a1c4776492599de66d6d189d399c37957f832485156f703f59bd39cdc2bb.json similarity index 55% rename from backend/.sqlx/query-72c123eb6a0d66f4e57d86694581772154b09f4a89903462875e57c9760a19e8.json rename to backend/.sqlx/query-1966a1c4776492599de66d6d189d399c37957f832485156f703f59bd39cdc2bb.json index b143a4f1b1..7f1ebcf9dc 100644 --- a/backend/.sqlx/query-72c123eb6a0d66f4e57d86694581772154b09f4a89903462875e57c9760a19e8.json +++ b/backend/.sqlx/query-1966a1c4776492599de66d6d189d399c37957f832485156f703f59bd39cdc2bb.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM variable v\n JOIN account a ON a.workspace_id = v.workspace_id AND a.id = v.account\n JOIN resource r ON r.workspace_id = v.workspace_id AND r.path = v.path\n WHERE v.workspace_id = $1 AND v.path = $2 AND a.client = $3\n AND a.is_workspace_integration = true AND r.created_by = $4)", + "query": "SELECT EXISTS(SELECT 1 FROM variable v\n JOIN account a ON a.workspace_id = v.workspace_id AND a.id = v.account\n WHERE v.workspace_id = $1 AND v.path = $2 AND a.client = $3\n AND a.is_workspace_integration = true)", "describe": { "columns": [ { @@ -11,7 +11,6 @@ ], "parameters": { "Left": [ - "Text", "Text", "Text", "Text" @@ -21,5 +20,5 @@ null ] }, - "hash": "72c123eb6a0d66f4e57d86694581772154b09f4a89903462875e57c9760a19e8" + "hash": "1966a1c4776492599de66d6d189d399c37957f832485156f703f59bd39cdc2bb" } diff --git a/backend/.sqlx/query-78954c7a6d38ae9d094f77bb255ae235c9b3083b4c179f819c927342bc58651f.json b/backend/.sqlx/query-78954c7a6d38ae9d094f77bb255ae235c9b3083b4c179f819c927342bc58651f.json new file mode 100644 index 0000000000..a8e33b105a --- /dev/null +++ b/backend/.sqlx/query-78954c7a6d38ae9d094f77bb255ae235c9b3083b4c179f819c927342bc58651f.json @@ -0,0 +1,34 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT EXISTS(SELECT 1 FROM native_trigger WHERE workspace_id = $1 AND service_name = $2)", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "exists", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text", + { + "Custom": { + "name": "native_trigger_service", + "kind": { + "Enum": [ + "nextcloud", + "google", + "github" + ] + } + } + } + ] + }, + "nullable": [ + null + ] + }, + "hash": "78954c7a6d38ae9d094f77bb255ae235c9b3083b4c179f819c927342bc58651f" +} diff --git a/backend/.sqlx/query-d79ba22a53c9f96c96fcd4b4cf0e6cc17f767055de943709192ef1ff5106eac5.json b/backend/.sqlx/query-d79ba22a53c9f96c96fcd4b4cf0e6cc17f767055de943709192ef1ff5106eac5.json new file mode 100644 index 0000000000..3f3a3f20e4 --- /dev/null +++ b/backend/.sqlx/query-d79ba22a53c9f96c96fcd4b4cf0e6cc17f767055de943709192ef1ff5106eac5.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT v.path FROM variable v\n JOIN account a ON a.workspace_id = v.workspace_id AND a.id = v.account\n WHERE v.workspace_id = $1 AND a.client = $2 AND a.is_workspace_integration = true", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "d79ba22a53c9f96c96fcd4b4cf0e6cc17f767055de943709192ef1ff5106eac5" +} diff --git a/backend/windmill-native-triggers/src/lib.rs b/backend/windmill-native-triggers/src/lib.rs index b58ec2d629..0b4577ae3f 100644 --- a/backend/windmill-native-triggers/src/lib.rs +++ b/backend/windmill-native-triggers/src/lib.rs @@ -1189,6 +1189,8 @@ pub async fn delete_token_by_hash<'c, E: sqlx::Executor<'c, Database = Postgres> Ok(deleted > 0) } +/// Callers must hold [`lock_connection`] on `connection_path` in the same transaction and check +/// the connection still exists, or a concurrent disconnect can leave this trigger stranded. pub async fn store_native_trigger<'c, E: sqlx::Executor<'c, Database = Postgres>, C: Serialize>( db: E, workspace_id: &str, diff --git a/backend/windmill-native-triggers/src/workspace_integrations.rs b/backend/windmill-native-triggers/src/workspace_integrations.rs index 884e1a4c7f..cef6feaebc 100644 --- a/backend/windmill-native-triggers/src/workspace_integrations.rs +++ b/backend/windmill-native-triggers/src/workspace_integrations.rs @@ -475,8 +475,40 @@ async fn delete_integration( 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. + 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 + WHERE v.workspace_id = $1 AND a.client = $2 AND a.is_workspace_integration = true", + workspace_id, + service_name.as_str(), + ) + .fetch_all(&db) + .await?; + paths.sort(); + for path in &paths { + lock_connection(&mut tx, &workspace_id, path).await?; + } + cleanup_service_connections(&mut *tx, &workspace_id, service_name).await?; + let created_meanwhile = sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM native_trigger WHERE workspace_id = $1 AND service_name = $2)", + workspace_id, + service_name as ServiceName, + ) + .fetch_one(&mut *tx) + .await? + .unwrap_or(false); + if created_meanwhile { + tx.rollback().await?; + return Err(Error::BadRequest(format!( + "A {service_name} trigger was created while removing the integration, so it was kept. \ + Remove it again." + ))); + } + let deleted = delete_workspace_integration(&mut *tx, &workspace_id, service_name).await?; if !deleted { @@ -1088,26 +1120,22 @@ async fn set_workspace_connection( ) -> JsonResult { require_admin(authed.is_admin, &workspace_id)?; - // Sharing lets everyone act as the account behind it, so only the one who connected it may. if let Some(path) = path.as_deref() { - let own = sqlx::query_scalar!( + 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 - JOIN resource r ON r.workspace_id = v.workspace_id AND r.path = v.path WHERE v.workspace_id = $1 AND v.path = $2 AND a.client = $3 - AND a.is_workspace_integration = true AND r.created_by = $4)", + AND a.is_workspace_integration = true)", workspace_id, path, service_name.as_str(), - authed.username, ) .fetch_one(&db) .await? .unwrap_or(false); - if !own { + if !exists { return Err(Error::BadRequest(format!( - "No {service_name} connection you made at {path}. You can only share an account \ - you connected yourself." + "No {service_name} connection at {path}" ))); } } @@ -1213,9 +1241,10 @@ async fn delete_connection( let account = cleanup_connection(&mut *tx, &workspace_id, &path).await?; // A trigger moved or created meanwhile was skipped above and still acts through this - // connection, so removing the connection would strand it. Checked after the delete: a create - // holds the connection's variable until it commits, so the delete waits for it and this - // check then sees its trigger. + // connection, so removing the connection would strand it. `lock_connection` is what makes + // this check sound: a create stores its trigger under the same lock, so it either committed + // before this transaction took the lock, and is seen here, or it waits and finds the + // connection gone. let still_used = sqlx::query_scalar!( "SELECT EXISTS(SELECT 1 FROM native_trigger WHERE workspace_id = $1 AND service_name = $2 AND connection_path = $3)", diff --git a/frontend/src/lib/components/triggers/native/NativeConnectionPicker.svelte b/frontend/src/lib/components/triggers/native/NativeConnectionPicker.svelte index 0b15ebf5c0..3582214f60 100644 --- a/frontend/src/lib/components/triggers/native/NativeConnectionPicker.svelte +++ b/frontend/src/lib/components/triggers/native/NativeConnectionPicker.svelte @@ -18,8 +18,6 @@ /** An existing trigger keeps the connection its webhook was registered under. */ locked?: boolean disabled?: boolean - /** Only offer the caller's own connections. */ - ownOnly?: boolean /** Select the first connection when none is chosen yet. */ autoSelect?: boolean clearable?: boolean @@ -35,7 +33,6 @@ value = $bindable(), locked = false, disabled = false, - ownOnly = false, autoSelect = true, clearable = false, placeholder, @@ -60,8 +57,8 @@ connections = [] sendUserToast(`Failed to load ${displayName} connections: ${err.body ?? err.message}`, true) } - if (autoSelect && !locked && value === undefined && offered.length > 0) { - select(offered[0].path) + if (autoSelect && !locked && value === undefined && connections.length > 0) { + select(connections[0].path) } } @@ -119,12 +116,9 @@ return !c.workspace && !!c.owner && c.owner === $userStore?.username } - const offered = $derived( - (connections ?? []).filter((c) => !ownOnly || c.owner === $userStore?.username) - ) const items = $derived.by(() => { - const list = offered.map((c) => ({ - label: c.workspace && !ownOnly ? `${c.path} (workspace connection)` : c.path, + const list = (connections ?? []).map((c) => ({ + label: c.workspace ? `${c.path} (workspace connection)` : c.path, value: c.path })) if (value && !list.some((i) => i.value === value)) { @@ -133,7 +127,7 @@ return list }) const unusable = $derived( - !ownOnly && connections !== undefined && !!value && !connections.some((c) => c.path === value) + connections !== undefined && !!value && !connections.some((c) => c.path === value) ) diff --git a/frontend/src/lib/components/workspaceSettings/WorkspaceIntegrations.svelte b/frontend/src/lib/components/workspaceSettings/WorkspaceIntegrations.svelte index d10477d779..06eed08ee3 100644 --- a/frontend/src/lib/components/workspaceSettings/WorkspaceIntegrations.svelte +++ b/frontend/src/lib/components/workspaceSettings/WorkspaceIntegrations.svelte @@ -158,10 +158,12 @@ const displayName = supportedServices[serviceName]?.displayName ?? serviceName if (path) { + const owner = connections[serviceName]?.find((c) => c.path === path)?.owner + const account = owner ? `this account, connected by ${owner}` : 'this account' const confirmed = await confirmationModal.ask({ title: `Share ${path} with the workspace?`, confirmationText: 'Share', - children: `Every member will be able to create and edit ${displayName} triggers that act as this account. They will not see its token. It replaces any connection shared before.` + children: `Every member will be able to create and edit ${displayName} triggers that act as ${account}. They will not see its token. It replaces any connection shared before.` }) if (!confirmed) return } else { @@ -356,7 +358,6 @@