mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-08 08:04:25 +00:00
fix: let a draft-only schedule, trigger or resource be deleted (#11010)
* fix: let a draft-only schedule, trigger or resource be deleted Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012SV5kjTis3AFtTx2nW2VRi * fix: keep the legacy-draft write gate out of the draft-only delete Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012SV5kjTis3AFtTx2nW2VRi * fix: don't gate a draft-only resource discard on the deployment rules Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012SV5kjTis3AFtTx2nW2VRi * docs: condense the draft-only delete comments per the comment policy Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012SV5kjTis3AFtTx2nW2VRi --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
1be390aa87
commit
8d0f4754e4
@@ -123,6 +123,72 @@ async fn test_protection_rules(db: Pool<Postgres>) -> 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
|
||||
// ========================================
|
||||
|
||||
@@ -113,7 +113,9 @@ async fn test_schedule_endpoints(db: Pool<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<serde_json::Value> = 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(())
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<bool> {
|
||||
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`):
|
||||
|
||||
@@ -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<Option<String>>,
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<T: TriggerCrud>(
|
||||
.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
|
||||
|
||||
Reference in New Issue
Block a user