diff --git a/backend/windmill-api-integration-tests/tests/protection_rules.rs b/backend/windmill-api-integration-tests/tests/protection_rules.rs index 9469554999..e032acef7b 100644 --- a/backend/windmill-api-integration-tests/tests/protection_rules.rs +++ b/backend/windmill-api-integration-tests/tests/protection_rules.rs @@ -123,6 +123,72 @@ async fn test_protection_rules(db: Pool) -> anyhow::Result<()> { .await?; assert!(!resp.status().is_success(), "Non-admin should be blocked from flows: {}", resp.status()); + // ======================================== + // 4b. ...but a draft-only resource stays deletable: nothing is deployed at + // its path, so its DELETE is a draft discard rather than a deployment. + // ======================================== + + let draft_only_path = "u/test-user-2/draft_only_resource"; + let resp = authed( + client().post(format!("{base}/drafts/update/resource/{draft_only_path}")), + "SECRET_TOKEN_2", + ) + .json(&json!({ "value": { + "path": draft_only_path, + "value": { "a": 1 }, + "resource_type": "c_test", + "description": "" + }})) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "Non-admin should save a draft: {}", + resp.text().await? + ); + + let resp = authed( + client().delete(format!("{base}/resources/delete/{draft_only_path}")), + "SECRET_TOKEN_2", + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "Draft-only delete should not be gated by the deploy rules: {}", + resp.text().await? + ); + + let remaining: i64 = sqlx::query_scalar( + "SELECT count(*) FROM draft WHERE workspace_id = 'test-workspace' AND path = $1 \ + AND typ = 'resource'::DRAFT_KIND", + ) + .bind(draft_only_path) + .fetch_one(&db) + .await?; + assert_eq!(remaining, 0, "the draft should be gone"); + + // The gate itself is still there for a DEPLOYED resource at the same path. + let resp = authed( + client().post(format!("{base}/resources/create")), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "path": draft_only_path, + "value": { "a": 1 }, + "resource_type": "c_test", + "description": "" + })) + .send() + .await?; + assert!( + !resp.status().is_success(), + "Non-admin should still be blocked from creating a resource: {}", + resp.status() + ); + // ======================================== // 5. Admin bypasses protection rule // ======================================== diff --git a/backend/windmill-api-integration-tests/tests/schedules.rs b/backend/windmill-api-integration-tests/tests/schedules.rs index 89ba0ec2cc..881f11ff43 100644 --- a/backend/windmill-api-integration-tests/tests/schedules.rs +++ b/backend/windmill-api-integration-tests/tests/schedules.rs @@ -113,7 +113,9 @@ async fn test_schedule_endpoints(db: Pool) -> anyhow::Result<()> { "expected at least 2 schedules, got {}", list.len() ); - assert!(list.iter().any(|s| s["path"] == "u/test-user/test_schedule")); + assert!(list + .iter() + .any(|s| s["path"] == "u/test-user/test_schedule")); // --- list_with_jobs --- let resp = authed(client().get(format!("{base}/list_with_jobs"))) @@ -125,18 +127,14 @@ async fn test_schedule_endpoints(db: Pool) -> anyhow::Result<()> { assert!(!list.is_empty()); // --- update --- - let resp = authed(client().post(schedule_url( - port, - "update", - "u/test-user/test_schedule", - ))) - .json(&json!({ - "schedule": "0 0 */12 * * *", - "timezone": "Europe/Paris" - })) - .send() - .await - .unwrap(); + let resp = authed(client().post(schedule_url(port, "update", "u/test-user/test_schedule"))) + .json(&json!({ + "schedule": "0 0 */12 * * *", + "timezone": "Europe/Paris" + })) + .send() + .await + .unwrap(); assert_eq!(resp.status(), 200, "update: {}", resp.text().await?); // verify update @@ -204,14 +202,11 @@ async fn test_schedule_endpoints(db: Pool) -> anyhow::Result<()> { assert_eq!(resp.status(), 200); // --- delete --- - let resp = authed(client().delete(schedule_url( - port, - "delete", - "u/test-user/another_schedule", - ))) - .send() - .await - .unwrap(); + let resp = + authed(client().delete(schedule_url(port, "delete", "u/test-user/another_schedule"))) + .send() + .await + .unwrap(); assert_eq!(resp.status(), 200); let resp = authed_get(port, "exists", "u/test-user/another_schedule").await; @@ -220,17 +215,108 @@ async fn test_schedule_endpoints(db: Pool) -> anyhow::Result<()> { // ===== Global endpoints ===== // --- preview --- - let resp = authed(client().post(format!( - "http://localhost:{port}/api/schedules/preview" - ))) - .json(&json!({ - "schedule": "0 0 */6 * * *", - "timezone": "UTC" - })) - .send() - .await - .unwrap(); + let resp = authed(client().post(format!("http://localhost:{port}/api/schedules/preview"))) + .json(&json!({ + "schedule": "0 0 */6 * * *", + "timezone": "UTC" + })) + .send() + .await + .unwrap(); assert_eq!(resp.status(), 200, "preview: {}", resp.text().await?); Ok(()) } + +/// A schedule with no `schedule` row is listed from the `draft` table, so its +/// DELETE drops that draft, then 404s once nothing is left at the path. A legacy +/// (`email IS NULL`) draft is owned by nobody and stays put. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_delete_draft_only_schedule(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let path = "u/test-user/draft_only_schedule"; + + let resp = authed(client().post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/update/trigger_schedule/{path}" + ))) + .json(&json!({ "value": { + "path": path, + "schedule": "0 0 */6 * * *", + "timezone": "UTC", + "script_path": "u/test-user/never_deployed", + "is_flow": false, + }})) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200, "save draft: {}", resp.text().await?); + + let resp = authed(client().get(format!( + "http://localhost:{port}/api/w/test-workspace/schedules/list?include_draft_only=true" + ))) + .send() + .await + .unwrap(); + let listed: Vec = resp.json().await?; + assert!( + listed + .iter() + .any(|s| s["path"] == path && s["draft_only"] == json!(true)), + "draft-only schedule should be listed: {listed:?}" + ); + + let resp = authed(client().delete(schedule_url(port, "delete", path))) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200, "delete: {}", resp.text().await?); + + let remaining: i64 = sqlx::query_scalar( + "SELECT count(*) FROM draft WHERE workspace_id = 'test-workspace' AND path = $1 \ + AND typ = 'trigger_schedule'::DRAFT_KIND", + ) + .bind(path) + .fetch_one(&db) + .await?; + assert_eq!(remaining, 0, "the draft should be gone"); + + let resp = authed(client().delete(schedule_url(port, "delete", path))) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 404, "nothing left at the path"); + + // A legacy (email IS NULL) draft is owned by nobody and keeps the write gate + // on the drafts routes, so this one must not become a second door to it. + let legacy_path = "u/test-user/legacy_draft_only_schedule"; + sqlx::query( + "INSERT INTO draft (workspace_id, email, path, typ, value) \ + VALUES ('test-workspace', NULL, $1, 'trigger_schedule'::DRAFT_KIND, '{}'::json)", + ) + .bind(legacy_path) + .execute(&db) + .await?; + + let resp = authed(client().delete(schedule_url(port, "delete", legacy_path))) + .send() + .await + .unwrap(); + assert_eq!( + resp.status(), + 404, + "legacy draft is not this route's to delete" + ); + + let legacy_remaining: i64 = sqlx::query_scalar( + "SELECT count(*) FROM draft WHERE workspace_id = 'test-workspace' AND path = $1 \ + AND typ = 'trigger_schedule'::DRAFT_KIND", + ) + .bind(legacy_path) + .fetch_one(&db) + .await?; + assert_eq!(legacy_remaining, 1, "the legacy draft should survive"); + + Ok(()) +} diff --git a/backend/windmill-api-schedule/src/lib.rs b/backend/windmill-api-schedule/src/lib.rs index 6ae902e984..1b6b49a136 100644 --- a/backend/windmill-api-schedule/src/lib.rs +++ b/backend/windmill-api-schedule/src/lib.rs @@ -31,8 +31,8 @@ use windmill_common::{ self, TriggerHistoryEvent, TriggerOperation, TriggerSource, SCHEDULE_TRIGGER_KIND, }, user_drafts::{ - delete_all_drafts_for_path, fetch_draft_only_list_rows, overlay_or_draft_only, - UserDraftItemKind, WithDraftOverlay, WithDraftQuery, + delete_all_drafts_for_path, delete_draft_only_for_path, fetch_draft_only_list_rows, + overlay_or_draft_only, UserDraftItemKind, WithDraftOverlay, WithDraftQuery, }, utils::{ escape_ilike_pattern, not_found_if_none, paginate, Pagination, ScheduleType, StripPath, @@ -1331,6 +1331,18 @@ async fn delete_schedule( .flatten(); if exists.is_none() { + drop(tx); + if delete_draft_only_for_path( + &db, + &w_id, + UserDraftItemKind::TriggerSchedule, + path, + &authed.email, + ) + .await? + { + return Ok(format!("Draft-only schedule {} deleted", path)); + } return Err(windmill_common::error::Error::NotFound(format!( "Schedule {} not found", path diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index 115dae34e7..aed9b5c5de 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -401,6 +401,45 @@ pub async fn fetch_draft_only_list_rows( Ok(rows) } +/// Delete the caller's OWN draft at a path with no deployed row, for the DELETE +/// route of a kind whose list synthesizes such rows via +/// `fetch_draft_only_list_rows`. The `NOT EXISTS` leaves a deployed row's draft +/// alone, so a route may call this on its not-found branch whatever the reason +/// for the miss. `Ok(false)` means nothing matched: the caller reports its own error. +/// +/// Takes no permission check and callers must not add one: an email-scoped row +/// belongs to the caller, who can always discard it, as `update_draft`'s +/// own-discard does. Legacy (`email IS NULL`) rows are owned by nobody and keep +/// their write gate, so discarding one stays on the `update_draft` route. +pub async fn delete_draft_only_for_path( + db: &DB, + w_id: &str, + kind: UserDraftItemKind, + path: &str, + email: &str, +) -> Result { + let Some(table) = kind.deployed_table() else { + return Ok(false); + }; + // `table` is from the closed `deployed_table()` enum, never user input. + let sql = format!( + "DELETE FROM draft \ + WHERE workspace_id = $1 AND typ = $2::text::DRAFT_KIND AND path = $3 \ + AND email = $4 \ + AND NOT EXISTS (SELECT 1 FROM {table} t \ + WHERE t.workspace_id = draft.workspace_id AND t.path = draft.path)" + ); + let deleted = sqlx::query(&sql) + .bind(w_id) + .bind(kind.as_str()) + .bind(path) + .bind(email) + .execute(db) + .await? + .rows_affected(); + Ok(deleted > 0) +} + /// The get-by-path draft choreography, shared by every entity's "get by path" /// route. Given the deployed entity as an `Option` (caller maps its own "not /// found" to `None`): diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index f9bfe2f4df..1c2d52bdd8 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -50,9 +50,9 @@ use windmill_common::{ error::{self, Error, JsonResult, Result}, get_database_url, user_drafts::{ - delete_all_drafts_for_path, delete_own_draft_for_path, fetch_draft_only, - fetch_draft_only_list_rows, maybe_overlay_draft, UserDraftItemKind, WithDraftOverlay, - WithDraftQuery, + delete_all_drafts_for_path, delete_draft_only_for_path, delete_own_draft_for_path, + fetch_draft_only, fetch_draft_only_list_rows, maybe_overlay_draft, UserDraftItemKind, + WithDraftOverlay, WithDraftQuery, }, utils::{not_found_if_none, paginate, require_admin, Pagination, StripPath}, variables, @@ -134,7 +134,10 @@ pub struct EditResourceType { /// `Option` conflates: an absent field leaves the extension alone, while an /// explicit `null` clears it. A hub pull relies on both — a type that stops /// being a file type has to stop being one locally too. - #[serde(default, deserialize_with = "windmill_common::more_serde::double_option")] + #[serde( + default, + deserialize_with = "windmill_common::more_serde::double_option" + )] pub format_extension: Option>, } @@ -1309,6 +1312,17 @@ async fn delete_resource( let path = path.to_path(); check_scopes(&authed, || format!("resources:write:{}", path))?; + + // Ahead of the deploy rules: nothing is deployed at a draft-only path, so + // gating this discard on them would strand the row in a protected workspace. + // Ahead of the transaction too — the not-found branch other kinds hang this + // off is the `not_found_if_none` below, past the linked-variable cascade. + if delete_draft_only_for_path(&db, &w_id, UserDraftItemKind::Resource, path, &authed.email) + .await? + { + return Ok(format!("draft-only resource {} deleted", path)); + } + if let RuleCheckResult::Blocked(msg) = check_deploy_rules( &w_id, AuditAuthorable::username(&authed), @@ -1320,6 +1334,7 @@ async fn delete_resource( { return Err(Error::PermissionDenied(msg)); } + let mut tx = user_db.begin(&authed).await?; // Capture resource data for trashbin before deleting diff --git a/backend/windmill-trigger/src/handler.rs b/backend/windmill-trigger/src/handler.rs index 8ef7c1ee39..953f3a571b 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -18,8 +18,9 @@ use windmill_common::{ error::{Error, JsonResult, Result}, trigger_history::{self, TriggerHistoryEvent, TriggerOperation, TriggerSource}, user_drafts::{ - delete_all_drafts_for_path, delete_own_draft_for_path, fetch_draft_only_list_rows, - overlay_or_draft_only, UserDraftItemKind, WithDraftOverlay, WithDraftQuery, + delete_all_drafts_for_path, delete_draft_only_for_path, delete_own_draft_for_path, + fetch_draft_only_list_rows, overlay_or_draft_only, UserDraftItemKind, WithDraftOverlay, + WithDraftQuery, }, utils::{paginate, Pagination, StripPath}, worker::CLOUD_HOSTED, @@ -990,6 +991,18 @@ async fn delete_trigger( .await?; if !deleted { + drop(tx); + if delete_draft_only_for_path( + &db, + &workspace_id, + T::user_draft_item_kind(), + path, + &authed.email, + ) + .await? + { + return Ok(format!("Draft-only trigger '{}' deleted", path)); + } return Err(Error::NotFound(format!( "Trigger not found at path: {}", path