From ecae9320d0f752d01af12da1834e15d741b443d0 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 4 Aug 2026 16:25:59 +0000 Subject: [PATCH] fix: let admins edit the dev workspace lock ruleset (#10512) * fix: let admins edit the dev workspace lock ruleset Co-Authored-By: Claude Opus 5 (1M context) * fix: route the empty protections panel through the owning workspace Co-Authored-By: Claude Opus 5 (1M context) * fix: make protection rule rename actually apply Co-Authored-By: Claude Opus 5 (1M context) * chore: cache the renamed protection rule query for sqlx offline Co-Authored-By: Claude Opus 5 (1M context) * fix: keep verbatim rule names and scope parent-admin lookup to its workspace Co-Authored-By: Claude Opus 5 (1M context) * fix: store renamed protection rule names verbatim Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- ...f60c179e5bccc54a36c8d5dcf04fd84445309.json | 19 ++ backend/tests/protection_rules.rs | 265 ++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 71 ++++- backend/windmill-api/openapi.yaml | 6 + .../lib/components/DevWorkspaceSetting.svelte | 195 ++++++++++--- .../workspaceSettings/RulesetEditor.svelte | 15 +- .../WorkspaceRulesets.svelte | 58 +++- .../lib/workspaceProtectionRules.svelte.ts | 5 + 8 files changed, 567 insertions(+), 67 deletions(-) create mode 100644 backend/.sqlx/query-adbe6ce27d46ba0d2d3ce87d875f60c179e5bccc54a36c8d5dcf04fd84445309.json diff --git a/backend/.sqlx/query-adbe6ce27d46ba0d2d3ce87d875f60c179e5bccc54a36c8d5dcf04fd84445309.json b/backend/.sqlx/query-adbe6ce27d46ba0d2d3ce87d875f60c179e5bccc54a36c8d5dcf04fd84445309.json new file mode 100644 index 0000000000..04ef8a7d24 --- /dev/null +++ b/backend/.sqlx/query-adbe6ce27d46ba0d2d3ce87d875f60c179e5bccc54a36c8d5dcf04fd84445309.json @@ -0,0 +1,19 @@ +{ + "db_name": "PostgreSQL", + "query": "\n UPDATE workspace_protection_rule\n SET name = $1, rules = $2, bypass_groups = $3, bypass_users = $4\n WHERE workspace_id = $5 AND name = $6\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Int4", + "TextArray", + "TextArray", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "adbe6ce27d46ba0d2d3ce87d875f60c179e5bccc54a36c8d5dcf04fd84445309" +} diff --git a/backend/tests/protection_rules.rs b/backend/tests/protection_rules.rs index 69873bf158..6714b24801 100644 --- a/backend/tests/protection_rules.rs +++ b/backend/tests/protection_rules.rs @@ -421,3 +421,268 @@ async fn test_restrict_deploy_to_deployers(db: Pool) -> anyhow::Result Ok(()) } + +/// The dev-workspace pairing owns `dev_workspace_lock` by name: attaching creates it, detaching +/// deletes it. Only those two ends of the name are reserved. Updating the rule has to stay open, +/// since relaxing a restriction from the rulesets UI is the only way an admin can loosen a pairing's +/// lock without detaching the dev workspace outright. +/// +/// Reads the row directly rather than through `list_protection_rules`, which goes via the +/// process-wide PROTECTION_RULES_CACHE that other tests in this file share. +#[sqlx::test(fixtures("base"))] +async fn test_dev_workspace_lock_rule_reservation(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + let name = windmill_common::workspaces::DEV_WORKSPACE_LOCK_RULE_NAME; + + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": name, + "rules": ["DisableDirectDeployment"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "creating the reserved rule by hand should be refused" + ); + + // Stands in for an attach, which is what really creates the rule. 3 = DisableDirectDeployment | + // DisableWorkspaceForking, the pairing's default lock. + sqlx::query( + "INSERT INTO workspace_protection_rule (workspace_id, name, rules, bypass_groups, bypass_users) + VALUES ('test-workspace', $1, 3, '{}', '{}')", + ) + .bind(name) + .execute(&db) + .await?; + + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules/{name}")), + "SECRET_TOKEN", + ) + .json(&json!({ + "rules": ["DisableWorkspaceForking"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "an admin must be able to drop a restriction from the reserved rule: {}", + resp.text().await? + ); + + let rules: i32 = sqlx::query_scalar( + "SELECT rules FROM workspace_protection_rule WHERE workspace_id = 'test-workspace' AND name = $1", + ) + .bind(name) + .fetch_one(&db) + .await?; + assert_eq!(rules, 2, "only DisableWorkspaceForking should remain"); + + // Renaming it away would strand the pairing's lock, which is located by name. + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules/{name}")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": "renamed-lock", + "rules": ["DisableWorkspaceForking"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "renaming the reserved rule should be refused" + ); + + let resp = authed( + client().delete(format!("{base}/workspaces/protection_rules/{name}")), + "SECRET_TOKEN", + ) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "deleting the reserved rule should stay refused: detaching the dev workspace removes it" + ); + + Ok(()) +} + +/// The name is half the row's primary key, so a rename has to move the row: the update applies the +/// name from the body, not just the one in the path. Collisions and the reserved name are refused, +/// and a name submitted unchanged is left exactly as stored. +#[sqlx::test(fixtures("base"))] +async fn test_protection_rule_rename(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + + for name in ["before-rename", "occupied"] { + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": name, + "rules": ["DisableDirectDeployment"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "setup: create '{}'", name); + } + + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules/before-rename")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": "after-rename", + "rules": ["DisableWorkspaceForking"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "rename should succeed: {}", + resp.text().await? + ); + + let names: Vec = sqlx::query_scalar( + "SELECT name FROM workspace_protection_rule WHERE workspace_id = 'test-workspace' ORDER BY name", + ) + .fetch_all(&db) + .await?; + assert_eq!( + names, + vec!["after-rename".to_string(), "occupied".to_string()], + "the row should have moved to the new name, not been duplicated" + ); + + for (target, why) in [ + ( + "occupied", + "renaming onto an existing rule should be refused", + ), + ( + windmill_common::workspaces::DEV_WORKSPACE_LOCK_RULE_NAME, + "renaming onto the reserved name should be refused", + ), + ] { + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules/after-rename")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": target, + "rules": ["DisableWorkspaceForking"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 400, "{}", why); + } + + // Names are stored verbatim and the editor submits the current name on every save, so a padded + // name has to survive a restrictions-only edit rather than being trimmed into a rename. + let padded = " padded-name "; + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": padded, + "rules": ["DisableDirectDeployment"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "setup: create the padded rule"); + + let resp = authed( + client().post(format!( + "{base}/workspaces/protection_rules/%20padded-name%20" + )), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": padded, + "rules": ["DisableWorkspaceForking"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "restrictions-only edit of a padded name should succeed: {}", + resp.text().await? + ); + + let still_there: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM workspace_protection_rule WHERE workspace_id = 'test-workspace' AND name = $1)", + ) + .bind(padded) + .fetch_one(&db) + .await?; + assert!( + still_there, + "the padded name must not have been trimmed into a rename" + ); + + // The mirror case: a name differing only in surrounding whitespace is a real rename, applied + // verbatim rather than collapsing into a success that changed nothing. + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules/after-rename")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": " after-rename ", + "rules": ["DisableWorkspaceForking"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "whitespace-only rename should apply: {}", + resp.text().await? + ); + let renamed: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM workspace_protection_rule WHERE workspace_id = 'test-workspace' AND name = ' after-rename ')", + ) + .fetch_one(&db) + .await?; + assert!(renamed, "the padded form should now be the stored name"); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 605846de94..2ce3737dc9 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -8770,6 +8770,9 @@ struct CreateProtectionRuleRequest { #[derive(Deserialize)] struct UpdateProtectionRuleRequest { + /// Renames the rule when it differs from the path name. Absent means "leave the name alone", + /// which is what a client that never sends the field gets. + name: Option, rules: Vec, bypass_groups: Vec, bypass_users: Vec, @@ -8917,10 +8920,11 @@ async fn ensure_dev_parent_is_root(db: &DB, parent_w_id: &str) -> Result<()> { Ok(()) } -/// `dev_workspace_lock` is owned by the dev-workspace feature (attach/detach/archive/delete create and -/// remove it by name). Reserve it from the public protection-rule API so a user-managed rule can't -/// collide: otherwise the feature's name-based cleanup would clobber the user's rule, or a manual edit -/// could weaken the feature's lock. +/// `dev_workspace_lock` is owned by the dev-workspace feature, which creates and removes it by name +/// (attach/detach/archive/delete). Reserve the name against creation and deletion so a user-managed +/// rule can't collide with the feature's name-based cleanup, and so detach stays the way a pairing's +/// lock is lifted. Updating it is deliberately allowed: relaxing the lock is an admin's call, and the +/// name is immutable on update so no collision can arise. fn reject_reserved_rule_name(name: &str) -> Result<()> { if name == DEV_WORKSPACE_LOCK_RULE_NAME { return Err(Error::BadRequest(format!( @@ -9014,7 +9018,28 @@ async fn update_protection_rule( Json(req): Json, ) -> Result { require_admin(authed.is_admin, &authed.username)?; - reject_reserved_rule_name(&rule_name)?; + + // A rename moves the row's primary key, so it needs the same name checks a create does. The + // reserved rule can be neither end of one: the dev-workspace feature finds it by name, so + // renaming it away would strand the lock and renaming onto it would collide with the feature. + // Names are stored verbatim, as creation stores them, so this comparison is raw: a rule called + // " prod-lock " survives an edit that submits its current name back untouched, and a name that + // differs only in surrounding whitespace is a real rename rather than a silent no-op. + let new_name = req.name.as_deref().filter(|n| *n != rule_name); + if let Some(new_name) = new_name { + if new_name.trim().is_empty() { + return Err(Error::BadRequest( + "Protection rule name cannot be empty".to_string(), + )); + } + if rule_name == DEV_WORKSPACE_LOCK_RULE_NAME { + return Err(Error::BadRequest(format!( + "'{}' cannot be renamed: the dev workspace feature locates it by name", + DEV_WORKSPACE_LOCK_RULE_NAME + ))); + } + reject_reserved_rule_name(new_name)?; + } let mut tx = db.begin().await?; @@ -9035,13 +9060,33 @@ async fn update_protection_rule( ))); } + if let Some(new_name) = new_name { + let taken = sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM workspace_protection_rule WHERE workspace_id = $1 AND name = $2)", + &w_id, + new_name + ) + .fetch_one(&mut *tx) + .await? + .unwrap_or(false); + if taken { + return Err(Error::BadRequest(format!( + "Protection rule with name '{}' already exists", + new_name + ))); + } + } + + let final_name = new_name.unwrap_or(&rule_name); + // Update the rule sqlx::query!( r#" UPDATE workspace_protection_rule - SET rules = $1, bypass_groups = $2, bypass_users = $3 - WHERE workspace_id = $4 AND name = $5 + SET name = $1, rules = $2, bypass_groups = $3, bypass_users = $4 + WHERE workspace_id = $5 AND name = $6 "#, + final_name, ProtectionRules::from(&req.rules).bits(), &req.bypass_groups, &req.bypass_users, @@ -9051,14 +9096,18 @@ async fn update_protection_rule( .execute(&mut *tx) .await?; + let mut audit_args = std::collections::HashMap::from([("name", final_name)]); + if new_name.is_some() { + audit_args.insert("previous_name", &rule_name[..]); + } audit_log( &mut *tx, &authed, "workspaces.update_protection_rule", ActionKind::Update, &w_id, - Some(&rule_name), - Some([("name", &rule_name[..])].into()), + Some(final_name), + Some(audit_args), ) .await?; @@ -9072,14 +9121,14 @@ async fn update_protection_rule( &authed.username, &db, &w_id, - DeployedObject::Settings { setting_type: format!("protection_rule_{}", rule_name) }, + DeployedObject::Settings { setting_type: format!("protection_rule_{}", final_name) }, None, false, None, ) .await?; - Ok(format!("Updated protection rule '{}'", rule_name)) + Ok(format!("Updated protection rule '{}'", final_name)) } /// Delete a protection rule diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 8c0df3bb9a..9e188cade4 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -6657,6 +6657,12 @@ paths: - bypass_groups - bypass_users properties: + name: + type: string + description: >- + New name for the rule. Omit, or pass the current name, to leave it unchanged. + The reserved `dev_workspace_lock` rule cannot be renamed, nor can another rule + be renamed onto it. rules: $ref: "#/components/schemas/ProtectionRules" bypass_groups: diff --git a/frontend/src/lib/components/DevWorkspaceSetting.svelte b/frontend/src/lib/components/DevWorkspaceSetting.svelte index d0339287ba..8df0931ae3 100644 --- a/frontend/src/lib/components/DevWorkspaceSetting.svelte +++ b/frontend/src/lib/components/DevWorkspaceSetting.svelte @@ -9,13 +9,15 @@ import { goto } from '$app/navigation' import { base } from '$lib/base' import { findCanonicalDevWorkspace } from '$lib/utils/workspaceHierarchy' + import { getUserExt } from '$lib/user' import { devBadgeText, devLabelKey, devLabelNoun } from '$lib/utils/devWorkspaceLabel' import { loadProtectionRules, isRuleActiveInRulesets, - isRuleUnconditionallyActiveInRulesets + isRuleUnconditionallyActiveInRulesets, + DEV_WORKSPACE_LOCK_RULE_NAME } from '$lib/workspaceProtectionRules.svelte' - import { GitFork, ExternalLink, Check, Minus } from 'lucide-svelte' + import { GitFork, ExternalLink, Check, Minus, Pen } from 'lucide-svelte' import { resource } from 'runed' let currentWs = $derived($userWorkspaces.find((w) => w.id === $workspaceStore)) @@ -59,13 +61,18 @@ let attachLabel = $state<'dev' | 'staging'>('dev') let busy = $state(false) + // The pairing's locks always sit on the root (prod) workspace: this one when viewed from prod, the + // parent's when viewed from inside the dev workspace. A plain fork has no pairing of its own, so it + // reads nothing. + let lockWorkspace = $derived(parentId ? (isDev ? parentId : undefined) : $workspaceStore) + // If this workspace already blocks direct deploy / forking through an existing protection rule, keep // the matching lock toggle on but locked: attaching only manages its own reserved dev-workspace rule, // so turning it "off" here couldn't lift a separately-defined block. A failed fetch falls back to the // editable default-on toggle (real rules still enforce). Once paired, the same rules report which // locks are actually in force. const rootProtectionRules = resource( - () => (!parentId ? $workspaceStore : undefined), + () => lockWorkspace, async (ws, _prev, { signal }) => { if (!ws) return undefined // `fetchProtectionRulesForWorkspace` fails open with an empty list, which the toggles below @@ -82,11 +89,12 @@ return { ws, rules: rules ?? [], failed: rules === undefined } } ) - // Only trust a result that belongs to the current workspace (guards the in-flight window and any - // out-of-order response); undefined means "not known yet" and is treated as locked below. + // Only trust a result that belongs to the workspace we're currently reading rules for (guards the + // in-flight window and any out-of-order response); undefined means "not known yet" and is treated as + // locked below. let rootResult = $derived.by(() => { const current = rootProtectionRules.current - return current && current.ws === $workspaceStore ? current : undefined + return current && current.ws === lockWorkspace ? current : undefined }) let rootRules = $derived(rootResult?.rules) // Only a rule with no bypass users/groups matches the empty-bypass reserved lock we would create; a @@ -123,6 +131,51 @@ let enforcementReadFailed = $derived(rootResult?.failed ?? false) let enforcementUnknown = $derived(rulesUnknown || enforcementReadFailed) + // The named rulesets actually carrying either lock. Reporting only "blocked / allowed" left no way + // to reach the rule that decides it, which is the one thing a reader here wants to change. + let enforcingRulesets = $derived( + (rootRules ?? []).filter( + (r) => + r.rules.includes('DisableDirectDeployment') || r.rules.includes('DisableWorkspaceForking') + ) + ) + // Editing prod's rules from the dev side needs admin IN PROD, which membership does not imply and + // this workspace's own admin rights say nothing about: the rulesets tab is admin-only, so a link + // offered to anyone else lands them on a tab they cannot open. Asked of the parent directly, as + // `is_admin` is per-workspace. A superadmin is admin everywhere and has no `usr` row to find. + // Tagged with its workspace and guarded against a superseded response, like the rules resource + // above: runed keeps the previous `current` while a new source loads, so switching between dev + // workspaces would otherwise offer Edit based on the previous parent's role. + const parentUser = resource( + () => (isDev && parentId ? parentId : undefined), + async (ws, _prev, { signal }) => { + if (!ws) return undefined + const user = await getUserExt(ws) + if (signal.aborted) throw new DOMException('superseded', 'AbortError') + return { ws, isAdmin: user?.is_admin === true } + } + ) + let canEditParentRules = $derived( + $superadmin || (parentUser.current?.ws === parentId && parentUser.current?.isAdmin === true) + ) + + // With a name, deep-links into that rule's drawer; without one, the rulesets list. Built from + // scratch rather than from the current query so no stale `?workspace=` survives a switch. + function rulesetsHref(name?: string): string { + const rule = name ? `&rule=${encodeURIComponent(name)}` : '' + return `${base}/workspace_settings?tab=rulesets${rule}` + } + + function openRulesets(name?: string) { + goto(rulesetsHref(name)) + } + + function openRulesetsInParent(name?: string) { + if (!parentId) return + switchWorkspace(parentId) + goto(rulesetsHref(name)) + } + // A standalone root workspace, or an existing fork of this prod (same family), can be attached. // A fork parented to a different workspace can't (the backend rejects a parent that isn't this // prod), so it's excluded here. @@ -205,6 +258,80 @@ } + +{#snippet protectionsPanel(opts: { + title: string + onOpen?: (name?: string) => void + editLabel: string + manageLabel: string +})} + {@const onOpen = opts.onOpen} +
+ {opts.title} + {#if enforcementUnknown} + + {enforcementReadFailed + ? 'Could not read the protection rules' + : 'Checking protection rules…'} + + {:else} + + {#if enforcesDeployBlock}{:else}{/if} + Direct edits {enforcesDeployBlock ? 'are blocked' : 'are allowed'} + + + {#if enforcesForkingBlock}{:else}{/if} + Forking {enforcesForkingBlock ? 'is blocked' : 'is allowed'} + + {#if enforcesDeployBlock || enforcesForkingBlock} + + Workspace admins always bypass these rules. + {/if} + {#if enforcingRulesets.length > 0} +
+ Enforced by + {#each enforcingRulesets as ruleset (ruleset.name)} +
+
+ {ruleset.name} + {#if ruleset.name === DEV_WORKSPACE_LOCK_RULE_NAME} + Applied by this pairing + {/if} +
+ {#if onOpen} + + {/if} +
+ {/each} +
+ {/if} + {/if} + {#if onOpen && enforcingRulesets.length === 0} +
+ +
+ {/if} +
+{/snippet} + {#if isDev && parentId}

@@ -218,6 +345,16 @@ {currentLabel} branch.

+ + {#if !enforcementReadFailed} + {@render protectionsPanel({ + title: `Protections in force on ${parentId}`, + onOpen: canEditParentRules ? openRulesetsInParent : undefined, + editLabel: `Edit in ${parentId}`, + manageLabel: `Manage in ${parentId}` + })} + {/if}
-
- + {@render protectionsPanel({ + title: 'Protections in force on this workspace', + onOpen: openRulesets, + editLabel: 'Edit', + manageLabel: 'Manage in Rulesets' + })}
{#if pairedDev.isMember || $superadmin}