diff --git a/backend/tests/protection_rules.rs b/backend/tests/protection_rules.rs index 6714b24801..6a96a66f8f 100644 --- a/backend/tests/protection_rules.rs +++ b/backend/tests/protection_rules.rs @@ -686,3 +686,112 @@ async fn test_protection_rule_rename(db: Pool) -> anyhow::Result<()> { Ok(()) } + +/// `RequireIsolatedForks` refuses a non-bypassing forker any data shared with the workspace, on +/// both fork entry points, and lets an admin through. +#[sqlx::test(fixtures("base"))] +async fn test_require_isolated_forks(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + invalidate_protection_rules_cache("test-workspace"); + // `dt2` is a clone governed from `gov`, but its database is this workspace's; `dt3` points + // into `gov`'s database. + let pg = r#"{"resource_type": "postgresql", "resource_path": "u/test-user/pg"}"#; + let gov = r#"{"workspace_id": "gov", "datatable": "src"}"#; + sqlx::raw_sql( + "INSERT INTO workspace (id, name, owner) VALUES ('gov', 'gov', 'test-user'); + INSERT INTO workspace_settings (workspace_id) VALUES ('gov')", + ) + .execute(&db) + .await?; + for (w_id, datatables) in [ + ("gov", format!(r#"{{"src": {{"database": {pg}}}}}"#)), + ( + "test-workspace", + format!( + r#"{{"dt1": {{"database": {pg}}}, "dt2": {{"database": {pg}, "governed_by": {gov}}}, + "dt3": {{"reference": {gov}}}}}"# + ), + ), + ] { + sqlx::query( + "UPDATE workspace_settings SET datatable = jsonb_build_object('datatables', $2::jsonb) \ + WHERE workspace_id = $1", + ) + .bind(w_id) + .bind(datatables) + .execute(&db) + .await?; + } + + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace", + server.addr.port() + ); + let resp = authed( + client().post(format!("{base}/workspaces/protection_rules")), + "SECRET_TOKEN", + ) + .json(&json!({ + "name": "isolate", + "rules": ["RequireIsolatedForks"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + let cloned = + json!([{"name": "dt1", "new_dbname": "wm_fork_iso__dt1", "fork_behavior": "schema_only"}]); + for (route, body, shared) in [ + ( + "create_fork", + json!({"shared_storages": ["_default_"], "forked_datatables": cloned}), + "storage", + ), + ( + "create_workspace_fork_branch", + json!({"shared_ducklakes": ["main"], "forked_datatables": cloned}), + "ducklake", + ), + ( + "create_fork", + json!({"forked_datatables": cloned}), + "data table `dt2`", + ), + ] { + let mut body = body; + body["id"] = json!("wm-fork-iso"); + body["name"] = json!("iso"); + let resp = authed( + client().post(format!("{base}/workspaces/{route}")), + "SECRET_TOKEN_2", + ) + .json(&body) + .send() + .await?; + let status = resp.status(); + let text = resp.text().await?; + assert!( + status == 403 && text.contains(shared) && !text.contains("dt3"), + "{route} sharing {shared} should be refused: {status} {text}" + ); + } + + // An admin bypasses the rule: what stops this request is the unknown storage name. + let resp = authed( + client().post(format!("{base}/workspaces/create_fork")), + "SECRET_TOKEN", + ) + .json(&json!({"id": "wm-fork-iso", "name": "iso", "shared_storages": ["_default_"]})) + .send() + .await?; + let text = resp.text().await?; + assert!( + !text.contains("isolate"), + "admin should bypass the rule: {text}" + ); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 17db877834..eb00c4106d 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -2332,6 +2332,9 @@ struct DataTableListItem { /// at its parent. Its permissions apply here, and only its admins may edit them. #[serde(skip_serializing_if = "Option::is_none")] governing_workspace_id: Option, + /// The workspace owning the database this entry points at, when it is not this workspace. + #[serde(skip_serializing_if = "Option::is_none")] + database_workspace_id: Option, /// Whether the governing entry is under roles. permissioned: bool, } @@ -2361,6 +2364,8 @@ async fn list_datatables( resource_path: database.resource_path.clone(), governing_workspace_id: (governing.governing_workspace_id() != w_id) .then(|| governing.governing_workspace_id().to_string()), + database_workspace_id: (governing.workspace_id != w_id) + .then(|| governing.workspace_id.clone()), permissioned: governing.datatable.permissions.is_some(), }); } @@ -8897,6 +8902,7 @@ async fn create_workspace_fork_branch( { return Err(Error::PermissionDenied(msg)); } + enforce_isolated_forks(&db, &authed, &w_id, &nw).await?; // Two-phase create for git-synced workspaces: this endpoint only creates the git branch(es) and // validates up front; it does NOT create the workspace row. The caller follows up with @@ -9647,6 +9653,7 @@ async fn create_workspace_fork( { return Err(Error::PermissionDenied(msg)); } + enforce_isolated_forks(&db, &authed, &parent_workspace_id, &nw).await?; if nw.is_dev_workspace { ensure_dev_parent_can_host_dev(&db, &parent_workspace_id).await?; @@ -10021,6 +10028,94 @@ fn copy_requests( .collect() } +/// Under `RequireIsolatedForks` on `parent`, refuse a fork that would leave any of its data +/// shared: a shared storage or lake, or a data table not cloned (`Keep original` points the fork +/// at the parent's database). +async fn enforce_isolated_forks( + db: &DB, + authed: &ApiAuthed, + parent: &str, + nw: &CreateWorkspaceFork, +) -> Result<()> { + let Some(msg) = isolation_required(db, authed, parent).await? else { + return Ok(()); + }; + let mut shared: Vec = nw + .shared_storages + .iter() + .map(|s| format!("storage `{s}`")) + .chain( + nw.shared_ducklakes + .iter() + .map(|l| format!("ducklake `{l}`")), + ) + .collect(); + shared.extend( + kept_parent_datatables(db, parent, &nw.forked_datatables) + .await? + .into_iter() + .map(|name| format!("data table `{name}`")), + ); + refuse_shared(msg, shared) +} + +/// The rule's message when `RequireIsolatedForks` on `parent` binds `authed`. +async fn isolation_required(db: &DB, authed: &ApiAuthed, parent: &str) -> Result> { + match check_user_against_rule( + parent, + &ProtectionRuleKind::RequireIsolatedForks, + AuditAuthorable::username(authed), + &authed.groups, + authed.is_admin, + db, + ) + .await? + { + RuleCheckResult::Blocked(msg) => Ok(Some(msg)), + RuleCheckResult::Allowed => Ok(None), + } +} + +/// `parent`'s own data tables that `forked` leaves as pointers at its database. An entry whose +/// database another workspace owns is not `parent`'s data, so the rule does not cover it. +async fn kept_parent_datatables( + db: &DB, + parent: &str, + forked: &[ForkedDatatableInfo], +) -> Result> { + Ok( + windmill_common::workspaces::resolve_workspace_governing_datatables(db, parent) + .await? + .into_iter() + .filter(|(_, governing)| governing.workspace_id == parent) + .filter(|(name, _)| { + !forked.iter().any(|d| { + &d.name == name + && matches!( + d.fork_behavior, + Some( + DataTableForkBehavior::SchemaOnly + | DataTableForkBehavior::SchemaAndData + ) + ) + }) + }) + .map(|(name, _)| name) + .collect(), + ) +} + +fn refuse_shared(msg: String, shared: Vec) -> Result<()> { + if shared.is_empty() { + return Ok(()); + } + Err(Error::PermissionDenied(format!( + "{msg}. Shared in this request: {}. Only a bypasser of the rule can fork while one of \ + them cannot be isolated", + shared.join(", ") + ))) +} + /// Record every storage of the fork's cloned settings as isolated, except `shared` ones. /// Isolation is a per-fork-creation choice, never inherited: a fork of a shared fork is /// isolated unless it asks otherwise. @@ -10134,6 +10229,17 @@ async fn write_workspace_fork( // Also before the external cluster's lifecycle lock, which finalizing an external copy takes: // fork cleanup takes the two in this order. windmill_common::workspaces::lock_fork_datatables(&mut tx, &parent_workspace_id).await?; + // Data table edits take the lock above: one committed since the creation check would reach + // this fork as a pointer at the parent's database. + if let Some(msg) = isolation_required(&db, &authed, &parent_workspace_id).await? { + let kept = kept_parent_datatables(&db, &parent_workspace_id, &nw.forked_datatables).await?; + refuse_shared( + msg, + kept.into_iter() + .map(|name| format!("data table `{name}`")) + .collect(), + )?; + } if nw.is_dev_workspace { // The checks above ran outside a transaction, so the parent's eligibility and the chain's diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 806d91e8b8..6dd49e0385 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -5640,6 +5640,9 @@ paths: type: string governing_workspace_id: type: string + database_workspace_id: + type: string + description: The workspace owning the database this entry points at, when it is not this workspace. permissioned: type: boolean @@ -39184,6 +39187,7 @@ components: - RestrictAnonymousAppDeployment - RestrictPublicRunSharing - RestrictGuestAppDeployment + - RequireIsolatedForks RuleBypasserGroups: type: array description: Groups that can bypass this ruleset diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 958c62a3d4..1b9412eb5a 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -73,6 +73,7 @@ bitflags::bitflags! { const RESTRICT_ANONYMOUS_APP_DEPLOYMENT = 1 << 3; const RESTRICT_PUBLIC_RUN_SHARING = 1 << 4; const RESTRICT_GUEST_APP_DEPLOYMENT = 1 << 5; + const REQUIRE_ISOLATED_FORKS = 1 << 6; } } @@ -86,6 +87,7 @@ pub enum ProtectionRuleKind { RestrictAnonymousAppDeployment, RestrictPublicRunSharing, RestrictGuestAppDeployment, + RequireIsolatedForks, } impl ProtectionRuleKind { @@ -109,6 +111,7 @@ impl ProtectionRuleKind { ProtectionRuleKind::RestrictGuestAppDeployment => { ProtectionRules::RESTRICT_GUEST_APP_DEPLOYMENT } + ProtectionRuleKind::RequireIsolatedForks => ProtectionRules::REQUIRE_ISOLATED_FORKS, } } @@ -130,6 +133,9 @@ impl ProtectionRuleKind { ProtectionRuleKind::RestrictGuestAppDeployment => { "Opening an app to guests (anyone who can sign in) is restricted in this workspace" } + ProtectionRuleKind::RequireIsolatedForks => { + "Forks of this workspace must isolate its data: no shared object storage or ducklake, and every data table cloned" + } } } } diff --git a/cli/src/guidance/skills.gen.ts b/cli/src/guidance/skills.gen.ts index d1cf9672a8..5cf07a243b 100644 --- a/cli/src/guidance/skills.gen.ts +++ b/cli/src/guidance/skills.gen.ts @@ -8343,7 +8343,7 @@ write a secret you were not given: keep the placeholder. | \`git_sync\` | \`{ repositories: [...] }\`, each with \`git_repo_resource_path\`, \`script_path\` and \`settings\` (include/exclude paths and types). Server-owned fields (webhook secrets, sync state, credentials) are never read nor written. | | \`datatable\` | \`{ datatables: { : { database: { resource_type, resource_path? } } } }\`. Role permissions are \`datatable_permissions\`. | | \`datatable_permissions\` | \`{ : { default_role, roles: { : [tenants] } } }\`: who may connect to each data table as which role. Tenants are \`u/\`, \`g/\`, \`f/\` or \`*\`; \`admin\` is always a role, and \`default_role: null\` means no default. A data table the value omits is no longer under roles. Only a data table on a cluster Windmill manages can be. | -| \`protection_rules\` | \`{ : { rules: [...], bypass_groups: [...], bypass_users: [...] } }\`, rules among \`DisableDirectDeployment\`, \`DisableWorkspaceForking\`, \`RestrictDeployToDeployers\`, \`RestrictAnonymousAppDeployment\`, \`RestrictPublicRunSharing\`, \`RestrictGuestAppDeployment\`. A rule the value omits is deleted. The dev workspace lock is not listed and cannot be set here. | +| \`protection_rules\` | \`{ : { rules: [...], bypass_groups: [...], bypass_users: [...] } }\`, rules among \`DisableDirectDeployment\`, \`DisableWorkspaceForking\`, \`RestrictDeployToDeployers\`, \`RestrictAnonymousAppDeployment\`, \`RestrictPublicRunSharing\`, \`RestrictGuestAppDeployment\`, \`RequireIsolatedForks\`. A rule the value omits is deleted. The dev workspace lock is not listed and cannot be set here. | | \`ducklake\` | \`{ ducklakes: { : { catalog, storage, ... } } }\`. | | \`dbt_warehouses\` | dbt warehouse connections. | | \`default_app\` | Path of the app shown as the workspace home. | diff --git a/docs/fork-object-storage.md b/docs/fork-object-storage.md index 6544edf902..7428119f97 100644 --- a/docs/fork-object-storage.md +++ b/docs/fork-object-storage.md @@ -54,3 +54,15 @@ the fork does not own costs one HEAD per chain level before the GET. them and refuses while they cannot be purged, since a same-id fork would read them. - Isolation is a per-fork-creation choice (`shared_storages` on create-fork), never inherited. Forks created before the registry existed have no rows and keep sharing. + +## Requiring isolation + +The `RequireIsolatedForks` protection rule on a workspace makes its forks leave nothing shared +with it, for every user the ruleset does not exempt (admins always are): no `shared_storages`, +no `shared_ducklakes`, and every data table whose database it owns cloned rather than kept as a +pointer. A data table whose database another workspace owns is left alone: that is not its data. +A clone governed from another workspace still has its own database, so it is not exempt. It is +checked against the workspace being forked, in +`create_workspace_fork` and in the git-branch pre-check (so a refused fork leaves no branch +behind), and the data table part again under the parent's `lock_fork_datatables`, which data table +edits also take: a table added between the two would otherwise reach the fork as a pointer. diff --git a/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte b/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte index 33543efa82..2afcbce3c8 100644 --- a/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte +++ b/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte @@ -36,10 +36,13 @@ import { useForkableWorkspaces } from '$lib/utils/useForkableWorkspaces.svelte' import { isOperatorInWorkspace } from '$lib/components/sidebar/operatorRoutes' import { + canUserBypassRuleKindInRulesets, fetchProtectionRulesForWorkspace, + isRuleActiveInRulesets, isRuleUnconditionallyActiveInRulesets } from '$lib/workspaceProtectionRules.svelte' import { resource } from 'runed' + import { getUserExt } from '$lib/user' import { Button } from '$lib/components/common' import { DEV_WORKSPACE_LABELS, @@ -279,6 +282,34 @@ let forkDatatableSection: ReturnType | undefined = $state(undefined) let forkDucklakeSection: ReturnType | undefined = $state(undefined) let forkStorageSection: ReturnType | undefined = $state(undefined) + let datatableCount = $state(0) + let ducklakeCount = $state(0) + let storageCount = $state(0) + let forkDataCount = $derived(datatableCount + ducklakeCount + storageCount) + + // The base's `RequireIsolatedForks` rule leaves no data shared with it: the sections below drop + // their sharing options. The server enforces it; this only keeps the form from offering a + // choice it would refuse. Bypass is judged with the user's identity in the BASE, like + // ParentWorkspaceProtectionAlert. + const baseRuleContext = resource( + () => (isFork ? baseWorkspaceId : undefined), + async (ws) => { + if (!ws) return undefined + const [rules, user] = await Promise.all([ + fetchProtectionRulesForWorkspace(ws), + getUserExt(ws) + ]) + return { rules, user } + } + ) + let requireIsolation = $derived.by(() => { + const ctx = baseRuleContext.current + return ( + !!ctx && + isRuleActiveInRulesets(ctx.rules, 'RequireIsolatedForks') && + !canUserBypassRuleKindInRulesets(ctx.rules, 'RequireIsolatedForks', ctx.user) + ) + }) let workspaceColor: string | undefined = $state(undefined) let colorEnabled = $state(false) @@ -435,9 +466,11 @@ id: prefixed_id, name, color: colorEnabled && workspaceColor ? workspaceColor : undefined, - // Sent in this first phase too, so a clone the fork would refuse is refused before - // any branch is created. + // Sent in this first phase too, so a clone or a sharing choice the fork would refuse + // is refused before any branch is created. forked_datatables: forkedDatatables, + shared_ducklakes: forkDucklakeSection?.getSharedDucklakes() ?? [], + shared_storages: forkStorageSection?.getSharedStorages() ?? [], is_dev_workspace: createAsDevWorkspace, dev_workspace_label: createAsDevWorkspace ? devWorkspaceLabel : undefined, // Send the lock intent in this first phase too so the backend can reject a non-admin's @@ -1013,18 +1046,42 @@ {/if} {/if} {#if isFork} - { - completeFork(effectiveForkId) - }} - onCanceled={() => { - forkCreationLoading = false - }} - /> - - + +
0 ? 'flex flex-col gap-3' : 'hidden'}> + + { + completeFork(effectiveForkId) + }} + onCanceled={() => { + forkCreationLoading = false + }} + /> + + +
{/if} {#if !automateUsernameCreation}