mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-17 08:03:06 +00:00
fix: keep pre-connection triggers working, and guard reconnects and service removal
- The migration recovers a service's connection when the workspace row lost its path, which saving the OAuth app or an abandoned instance connect used to do. - A trigger created without a connection uses the shared account, as older clients and trigger specs expect. - Only whoever connected a path, or an admin, may reconnect over it. - Removing a service or sharing an account checks a path-scoped token against what it touches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
9e0bd22a88
commit
39ac1d8491
+29
@@ -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"
|
||||
}
|
||||
+34
@@ -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"
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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<Postgres>) -> 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(())
|
||||
}
|
||||
|
||||
|
||||
@@ -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!(
|
||||
|
||||
@@ -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<String> {
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user