diff --git a/backend/.sqlx/query-cb135f028eb579ee912c618f48f38d99f84311ac3297f04bc19425a4bddceb33.json b/backend/.sqlx/query-0d9a83d77632bc0de19a48eddb7e6f80b2a41db4f1d352920d0c295ea3a018b1.json similarity index 83% rename from backend/.sqlx/query-cb135f028eb579ee912c618f48f38d99f84311ac3297f04bc19425a4bddceb33.json rename to backend/.sqlx/query-0d9a83d77632bc0de19a48eddb7e6f80b2a41db4f1d352920d0c295ea3a018b1.json index 335ed9035d..93251596cf 100644 --- a/backend/.sqlx/query-cb135f028eb579ee912c618f48f38d99f84311ac3297f04bc19425a4bddceb33.json +++ b/backend/.sqlx/query-0d9a83d77632bc0de19a48eddb7e6f80b2a41db4f1d352920d0c295ea3a018b1.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "UPDATE draft\n SET path = $3,\n -- Both path keys, not just the typed one: the editors mirror the\n -- typed path into the other while it differs from the row's path,\n -- and the loaders prefer the mirror — left naming the old location\n -- it un-does this move on the next save. `create_missing = false`\n -- on both, so a draft carrying only one keeps only one.\n value = to_json(\n jsonb_set(\n jsonb_set(\n CASE WHEN $7::text IS NULL THEN to_jsonb(value)\n ELSE jsonb_set(to_jsonb(value), ARRAY['summary'], to_jsonb($7::text))\n END,\n ARRAY[$5::text], to_jsonb($3::text), false\n ),\n ARRAY[$8::text], to_jsonb($3::text), false\n )\n )\n WHERE workspace_id = $1\n AND path = $2\n AND typ = $4\n AND email = $6\n -- A pre-sanitizer NUL escape makes `to_jsonb` raise 22P05. Excluded\n -- here so the statement can't 500; reported below instead. Unlike the\n -- passive carry, rewriting the value IS this operation, so skipping it\n -- silently would move the row and leave its typed path stale.\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) = 0\n -- Skipped on a summary-only edit, where the \"target\" row is this\n -- row and the guard would refuse the update against itself.\n AND ($2 = $3 OR NOT EXISTS (\n SELECT 1 FROM draft o\n WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[])\n -- The legacy row counts: a deploy at that path wipes it together with\n -- the caller's, so parking a second draft there discards edits the\n -- caller never saw.\n AND (o.email = $6 OR o.email IS NULL)\n ))\n RETURNING id", + "query": "UPDATE draft\n SET path = $3,\n -- Both path keys, not just the typed one: the editors mirror the\n -- typed path into the other while it differs from the row's path,\n -- and the loaders prefer the mirror — left naming the old location\n -- it un-does this move on the next save. `create_missing = false`\n -- on both, so a draft carrying only one keeps only one.\n value = to_json(\n jsonb_set(\n jsonb_set(\n CASE WHEN $7::text IS NULL THEN to_jsonb(value)\n ELSE jsonb_set(to_jsonb(value), ARRAY['summary'], to_jsonb($7::text))\n END,\n ARRAY[$5::text], to_jsonb($3::text), false\n ),\n ARRAY[$8::text], to_jsonb($3::text), false\n )\n )\n WHERE workspace_id = $1\n AND path = $2\n AND typ = $4\n AND email = $6\n -- A pre-sanitizer NUL escape makes `to_jsonb` raise 22P05. Excluded\n -- here so the statement can't 500; reported below instead. Unlike the\n -- passive carry, rewriting the value IS this operation, so skipping it\n -- silently would move the row and leave its typed path stale.\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) = 0\n -- Skipped on a summary-only edit, where the \"target\" row is this\n -- row and the guard would refuse the update against itself.\n AND ($2 = $3 OR NOT EXISTS (\n SELECT 1 FROM draft o\n WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[])\n -- Of this kind only the caller's own row and the legacy one collide:\n -- teammates' drafts of one item share its path by design, but a deploy\n -- there wipes those two together, so a second would discard edits the\n -- caller never saw. The other app kind is a different item on the same\n -- deployed path, so it collides whoever owns it.\n AND (o.typ <> $4 OR o.email = $6 OR o.email IS NULL)\n ))\n RETURNING id", "describe": { "columns": [ { @@ -60,5 +60,5 @@ false ] }, - "hash": "cb135f028eb579ee912c618f48f38d99f84311ac3297f04bc19425a4bddceb33" + "hash": "0d9a83d77632bc0de19a48eddb7e6f80b2a41db4f1d352920d0c295ea3a018b1" } diff --git a/backend/.sqlx/query-07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8.json b/backend/.sqlx/query-a1ed94ef0a6d4f7bc89ac4a2341bc06dc95d60ec92469523935f5a18a4d3f907.json similarity index 50% rename from backend/.sqlx/query-07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8.json rename to backend/.sqlx/query-a1ed94ef0a6d4f7bc89ac4a2341bc06dc95d60ec92469523935f5a18a4d3f907.json index a8b1eaa95c..c536425f1d 100644 --- a/backend/.sqlx/query-07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8.json +++ b/backend/.sqlx/query-a1ed94ef0a6d4f7bc89ac4a2341bc06dc95d60ec92469523935f5a18a4d3f907.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT\n -- Own row first: with both an own and a legacy row at the destination,\n -- a bare LIMIT 1 would name an arbitrary one and the two need different\n -- remedies (discard your own vs. ask an admin).\n (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND (email = $4 OR email IS NULL)\n ORDER BY email NULLS LAST LIMIT 1) as \"at_target\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email = $4) as \"at_target_own!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email IS NULL) as \"legacy!\" ", + "query": "SELECT\n -- The guard's own predicate, ordered own row, then legacy, then another\n -- user's other-kind row: each needs a different remedy, and a bare\n -- LIMIT 1 would name an arbitrary one.\n (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[])\n AND (typ <> $2 OR email = $4 OR email IS NULL)\n ORDER BY CASE WHEN email = $4 THEN 0 WHEN email IS NULL THEN 1 ELSE 2 END\n LIMIT 1) as \"at_target\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email = $4) as \"at_target_own!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email IS NULL) as \"at_target_legacy!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email IS NULL) as \"legacy!\" ", "describe": { "columns": [ { @@ -15,11 +15,16 @@ }, { "ordinal": 2, - "name": "poisoned!", + "name": "at_target_legacy!", "type_info": "Bool" }, { "ordinal": 3, + "name": "poisoned!", + "type_info": "Bool" + }, + { + "ordinal": 4, "name": "legacy!", "type_info": "Bool" } @@ -72,8 +77,9 @@ null, null, null, + null, null ] }, - "hash": "07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8" + "hash": "a1ed94ef0a6d4f7bc89ac4a2341bc06dc95d60ec92469523935f5a18a4d3f907" } diff --git a/backend/tests/drafts_move_taken.rs b/backend/tests/drafts_move_taken.rs index fc2790e3dc..c1d713c8a0 100644 --- a/backend/tests/drafts_move_taken.rs +++ b/backend/tests/drafts_move_taken.rs @@ -181,3 +181,54 @@ async fn test_draft_move_refuses_the_other_app_kind(db: Pool) -> anyho ); Ok(()) } + +/// Teammates' drafts of one item share its path by design, so another user's row is no +/// obstacle — except across the app pair, where the two kinds are different items on one +/// deployed path: deploying either strands the other, and deleting the app takes both. +#[sqlx::test(fixtures("base", "drafts_move_taken"))] +async fn test_draft_move_refuses_another_users_other_app_kind( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let move_to = |kind: &'static str, from: &'static str, to: &'static str| async move { + let resp = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/move/{kind}/{from}" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ "new_path": to })) + .send() + .await?; + Ok::<_, anyhow::Error>((resp.status(), resp.text().await?)) + }; + + let (status, body) = move_to( + "app", + "u/test-user/mvtaken_app", + "u/test-user/mvtaken_theirs", + ) + .await?; + assert_eq!( + status, 400, + "a classic app was moved onto another user's raw app: {body}" + ); + assert!( + body.contains("Another user has a raw app draft"), + "the refusal did not name the occupant: {body}" + ); + + // The same-kind case is the ordinary one: two users' drafts of one raw app. + let (status, body) = move_to( + "raw_app", + "u/test-user/mvtaken_raw", + "u/test-user/mvtaken_theirs", + ) + .await?; + assert!( + status.is_success(), + "a raw app was refused beside another user's raw-app draft: {body}" + ); + Ok(()) +} diff --git a/backend/tests/fixtures/drafts_move_taken.sql b/backend/tests/fixtures/drafts_move_taken.sql index 85b2543b26..13d1fa3e90 100644 --- a/backend/tests/fixtures/drafts_move_taken.sql +++ b/backend/tests/fixtures/drafts_move_taken.sql @@ -24,9 +24,12 @@ VALUES ('test-workspace', 'u/test-user/mvtaken_legacy', 'script', NULL); -- A draft-only classic app and a draft-only raw app of the same owner. They share --- the `app` table, so one occupies the other's path. +-- the `app` table, so one occupies the other's path. The third is another user's raw +-- app: a different item at its own path, not a second copy of anyone's. INSERT INTO draft (workspace_id, path, typ, value, email) VALUES ('test-workspace', 'u/test-user/mvtaken_app', 'app', '{"summary": "classic", "value": {}}', 'test@windmill.dev'), ('test-workspace', 'u/test-user/mvtaken_raw', 'raw_app', - '{"summary": "raw", "files": {}}', 'test@windmill.dev'); + '{"summary": "raw", "files": {}}', 'test@windmill.dev'), + ('test-workspace', 'u/test-user/mvtaken_theirs', 'raw_app', + '{"summary": "theirs", "files": {}}', 'test2@windmill.dev'); diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index a1e6dc6b7d..17ded64c45 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -762,10 +762,12 @@ async fn move_draft( AND ($2 = $3 OR NOT EXISTS ( SELECT 1 FROM draft o WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[]) - -- The legacy row counts: a deploy at that path wipes it together with - -- the caller's, so parking a second draft there discards edits the - -- caller never saw. - AND (o.email = $6 OR o.email IS NULL) + -- Of this kind only the caller's own row and the legacy one collide: + -- teammates' drafts of one item share its path by design, but a deploy + -- there wipes those two together, so a second would discard edits the + -- caller never saw. The other app kind is a different item on the same + -- deployed path, so it collides whoever owns it. + AND (o.typ <> $4 OR o.email = $6 OR o.email IS NULL) )) RETURNING id"#, &w_id, @@ -796,14 +798,18 @@ async fn move_draft( if moved.is_none() { let row = sqlx::query!( r#"SELECT - -- Own row first: with both an own and a legacy row at the destination, - -- a bare LIMIT 1 would name an arbitrary one and the two need different - -- remedies (discard your own vs. ask an admin). + -- The guard's own predicate, ordered own row, then legacy, then another + -- user's other-kind row: each needs a different remedy, and a bare + -- LIMIT 1 would name an arbitrary one. (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3 - AND typ::text = ANY($6::text[]) AND (email = $4 OR email IS NULL) - ORDER BY email NULLS LAST LIMIT 1) as "at_target", + AND typ::text = ANY($6::text[]) + AND (typ <> $2 OR email = $4 OR email IS NULL) + ORDER BY CASE WHEN email = $4 THEN 0 WHEN email IS NULL THEN 1 ELSE 2 END + LIMIT 1) as "at_target", EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3 AND typ::text = ANY($6::text[]) AND email = $4) as "at_target_own!", + EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3 + AND typ::text = ANY($6::text[]) AND email IS NULL) as "at_target_legacy!", EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5 AND typ = $2 AND email = $4 AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0 @@ -841,7 +847,7 @@ async fn move_draft( let occupant = occupant.replace('_', " "); if row.at_target_own { format!("You already have a draft at '{new_path}' ({occupant})") - } else { + } else if row.at_target_legacy { // An ownerless row the caller cannot clear themselves, so send them to // the one place it can be resolved rather than to "discard your draft". format!( @@ -849,6 +855,13 @@ async fn move_draft( ({occupant}). A workspace admin can claim or discard it on the Review & \ deploy drafts page." ) + } else { + // The other app kind, owned by someone else: one deployed path cannot hold + // both, so this is the other item's path, not a teammate's copy of this one. + format!( + "Another user has a {occupant} draft at '{new_path}', and an app and a \ + raw app cannot share a path." + ) } } else { format!("You have no draft at '{path}'") diff --git a/frontend/src/lib/components/sessions/FlowEditorView.svelte b/frontend/src/lib/components/sessions/FlowEditorView.svelte index c00ed28b0d..5ab1aae5b2 100644 --- a/frontend/src/lib/components/sessions/FlowEditorView.svelte +++ b/frontend/src/lib/components/sessions/FlowEditorView.svelte @@ -104,9 +104,9 @@ cell.store), reactive state owned by the SessionRuntime class (via flowCell), not by a component ancestor — so Svelte's ownership check flags a false positive here. --> - +