diff --git a/backend/tests/app_access_mode_only.rs b/backend/tests/app_access_mode_only.rs new file mode 100644 index 0000000000..b113e9de9a --- /dev/null +++ b/backend/tests/app_access_mode_only.rs @@ -0,0 +1,311 @@ +//! `EditApp.execution_mode`: changing who may open an app without sending its policy. +//! +//! The change is applied to the deployed policy under the row lock a deploy takes, so it keeps +//! every other field a concurrent deploy wrote. It is held to the rules a policy write is held +//! to, and it is refused to a caller who could not keep the app's run-as user, since a policy +//! write from them would rebind it. +//! +//! One test: the protection rules and the workspace roles are cached per workspace id across +//! the process, and every test here uses `test-workspace`. +//! +//! Users from the `base` fixture: +//! test-user (admin, token SECRET_TOKEN) +//! test-user-2 (non-admin, token SECRET_TOKEN_2) +//! test-user-3 (made a compose-only operator below, token SECRET_TOKEN_3) + +use reqwest::multipart; +use serde_json::{json, Value}; +use sqlx::{Pool, Postgres}; +use windmill_common::workspaces::invalidate_protection_rules_cache; +use windmill_test_utils::*; + +const ADMIN: &str = "SECRET_TOKEN"; +const USER: &str = "SECRET_TOKEN_2"; +const BUILDER: &str = "SECRET_TOKEN_3"; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +async fn update(ws: &str, path: &str, token: &str, body: Value) -> anyhow::Result<(u16, String)> { + let resp = client() + .post(format!("{ws}/apps/update/{path}")) + .bearer_auth(token) + .json(&body) + .send() + .await?; + Ok((resp.status().as_u16(), resp.text().await?)) +} + +async fn create(ws: &str, token: &str, path: &str, policy: Value) -> anyhow::Result<()> { + let resp = client() + .post(format!("{ws}/apps/create")) + .bearer_auth(token) + .json(&json!({ "path": path, "summary": "", "value": {}, "policy": policy })) + .send() + .await?; + assert_eq!(resp.status(), 201, "create {path}: {}", resp.text().await?); + Ok(()) +} + +/// An anonymous, sandboxed raw app with no runnables, as a builder may deploy one. +async fn create_raw(ws: &str, token: &str, path: &str) -> anyhow::Result<()> { + let app = json!({ + "path": path, + "summary": "", + "value": { "files": {}, "runnables": {} }, + "policy": { "execution_mode": "anonymous", "sandbox": true, "triggerables_v2": {} }, + }); + let form = multipart::Form::new() + .part( + "app", + multipart::Part::text(app.to_string()).mime_str("application/json")?, + ) + .part("js", multipart::Part::text("console.log('app')")); + let resp = client() + .post(format!("{ws}/apps/create_raw")) + .bearer_auth(token) + .multipart(form) + .send() + .await?; + assert_eq!( + resp.status(), + 201, + "create_raw {path}: {}", + resp.text().await? + ); + Ok(()) +} + +async fn policy(db: &Pool, path: &str) -> anyhow::Result { + Ok(sqlx::query_scalar( + "SELECT policy FROM app WHERE path = $1 AND workspace_id = 'test-workspace'", + ) + .bind(path) + .fetch_one(db) + .await?) +} + +fn triggerable(key: &str) -> Value { + json!({ key: { "static_inputs": {}, "one_of_inputs": {} } }) +} + +#[sqlx::test(fixtures("base"))] +async fn test_access_mode_only_update(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + invalidate_protection_rules_cache("test-workspace"); + let server = ApiServer::start(db.clone()).await?; + let ws = format!( + "http://localhost:{}/api/w/test-workspace", + server.addr.port() + ); + + // ======================================== + // 1. The mode alone changes; every other field of the deployed policy stays. + // ======================================== + const SHARED: &str = "u/test-user-2/shared"; + create( + &ws, + ADMIN, + SHARED, + json!({ + "execution_mode": "publisher", + "triggerables_v2": triggerable("c:script/u/test-user/noop"), + "sandbox": true, + "allowed_s3_keys": [{ "s3_path": "a/b.txt" }], + }), + ) + .await?; + let before = policy(&db, SHARED).await?; + assert_eq!(before["on_behalf_of"], "u/test-user"); + + let (status, body) = + update(&ws, SHARED, ADMIN, json!({ "execution_mode": "anonymous" })).await?; + assert_eq!(status, 200, "{body}"); + let mut after = policy(&db, SHARED).await?; + assert_eq!(after["execution_mode"], "anonymous"); + after["execution_mode"] = before["execution_mode"].clone(); + assert_eq!( + after, before, + "a mode-only change must keep every other policy field" + ); + + // Sending a policy too is ambiguous about which mode wins. + let (status, _) = update( + &ws, + SHARED, + ADMIN, + json!({ "execution_mode": "publisher", "policy": { "execution_mode": "anonymous" } }), + ) + .await?; + assert_eq!(status, 400); + + // ======================================== + // 2. Someone who cannot keep the run-as user cannot change the mode of an app that runs + // as someone else, even with write access to it. + // ======================================== + let (status, body) = + update(&ws, SHARED, USER, json!({ "execution_mode": "publisher" })).await?; + assert_eq!(status, 403, "{body}"); + assert!(body.contains("wm_deployers"), "{body}"); + assert_eq!(policy(&db, SHARED).await?["execution_mode"], "anonymous"); + + // ======================================== + // 3. A policy without a mode keeps the deployed one. + // ======================================== + const OWN: &str = "u/test-user-2/own"; + create(&ws, USER, OWN, json!({ "execution_mode": "publisher" })).await?; + sqlx::query( + "INSERT INTO draft (workspace_id, path, typ, value, email) + VALUES ('test-workspace', $1, 'app', '{}', 'test2@windmill.dev')", + ) + .bind(OWN) + .execute(&db) + .await?; + let (status, body) = update(&ws, OWN, USER, json!({ "execution_mode": "anonymous" })).await?; + assert_eq!(status, 200, "{body}"); + let drafts: i64 = sqlx::query_scalar( + "SELECT count(*) FROM draft WHERE workspace_id = 'test-workspace' AND path = $1", + ) + .bind(OWN) + .fetch_one(&db) + .await?; + assert_eq!(drafts, 1, "a mode-only change must keep the caller's draft"); + let (status, body) = update( + &ws, + OWN, + USER, + json!({ "value": { "v": 2 }, "policy": { "triggerables_v2": triggerable("c:script/f/x/new") } }), + ) + .await?; + assert_eq!(status, 200, "{body}"); + let p = policy(&db, OWN).await?; + assert_eq!(p["execution_mode"], "anonymous"); + assert!( + p["triggerables_v2"].get("c:script/f/x/new").is_some(), + "{p}" + ); + + // ======================================== + // 4. A compose-only operator: an omitted mode inherits the deployed one unless it is Viewer. + // ======================================== + sqlx::query("UPDATE usr SET operator = true WHERE workspace_id = 'test-workspace' AND username = 'test-user-3'") + .execute(&db) + .await?; + sqlx::query( + r#"UPDATE workspace_settings SET role_overrides = '{"operator": {"capabilities": {"apps": "compose"}}}' + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + // In wm_deployers, so the refusal in the last step below holds for a builder who may keep + // another user's identity on deploy. + sqlx::query( + "INSERT INTO group_ (workspace_id, name, summary, extra_perms) VALUES ('test-workspace', 'wm_deployers', '', '{}')", + ) + .execute(&db) + .await?; + sqlx::query( + "INSERT INTO usr_to_group (workspace_id, group_, usr) VALUES ('test-workspace', 'wm_deployers', 'test-user-3')", + ) + .execute(&db) + .await?; + windmill_common::roles::invalidate_workspace_roles_cache("test-workspace"); + windmill_common::roles::invalidate_member_role_cache("test-workspace", "test-user-3"); + + const BUILT: &str = "u/test-user-3/built"; + create_raw(&ws, BUILDER, BUILT).await?; + + let settings_only = json!({ "policy": { "sandbox": true, "triggerables_v2": {} } }); + let (status, body) = update(&ws, BUILT, BUILDER, settings_only.clone()).await?; + assert_eq!(status, 200, "{body}"); + assert_eq!( + policy(&db, BUILT).await?["execution_mode"], + "anonymous", + "a builder's policy without a mode must keep a non-Viewer deployed mode" + ); + + // Viewer stays refused to a builder, alone as in a policy. + let (status, body) = update(&ws, BUILT, BUILDER, json!({ "execution_mode": "viewer" })).await?; + assert_eq!(status, 403, "{body}"); + + // An admin may put the builder's app in Viewer; the builder's next policy without a mode + // then cannot inherit it. + let (status, body) = update(&ws, BUILT, ADMIN, json!({ "execution_mode": "viewer" })).await?; + assert_eq!(status, 200, "{body}"); + let (status, body) = update(&ws, BUILT, BUILDER, settings_only).await?; + assert_eq!(status, 200, "{body}"); + assert_eq!(policy(&db, BUILT).await?["execution_mode"], "publisher"); + + let (status, body) = update( + &ws, + BUILT, + BUILDER, + json!({ "execution_mode": "anonymous" }), + ) + .await?; + assert_eq!( + status, 200, + "a builder may change the mode of its own app: {body}" + ); + assert_eq!(policy(&db, BUILT).await?["execution_mode"], "anonymous"); + + // A builder's deploy would sandbox an app whose policy leaves it unset, which a mode-only + // change must not do. + sqlx::query( + "UPDATE app SET policy = policy - 'sandbox' WHERE path = $1 AND workspace_id = 'test-workspace'", + ) + .bind(BUILT) + .execute(&db) + .await?; + let (status, body) = update( + &ws, + BUILT, + BUILDER, + json!({ "execution_mode": "publisher" }), + ) + .await?; + assert_eq!(status, 403, "{body}"); + let p = policy(&db, BUILT).await?; + assert!(p.get("sandbox").is_none(), "{p}"); + assert_eq!(p["execution_mode"], "anonymous"); + + // Changing the mode alone skips the check of the triggerables against the builder's RLS, so + // it stays refused on an app that runs as someone else; deploying it is the way. + const ADMIN_RUN: &str = "u/test-user-3/admin_run"; + create_raw(&ws, ADMIN, ADMIN_RUN).await?; + let (status, body) = update( + &ws, + ADMIN_RUN, + BUILDER, + json!({ "execution_mode": "publisher" }), + ) + .await?; + assert_eq!(status, 403, "{body}"); + assert!(body.contains("by deploying it"), "{body}"); + + // ======================================== + // 5. Widening is gated by the protection rule; narrowing is not. + // ======================================== + let resp = client() + .post(format!("{ws}/workspaces/protection_rules")) + .bearer_auth(ADMIN) + .json(&json!({ + "name": "no-public-apps", + "rules": ["RestrictAnonymousAppDeployment"], + "bypass_users": [], + "bypass_groups": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + let (status, body) = update(&ws, OWN, USER, json!({ "execution_mode": "publisher" })).await?; + assert_eq!(status, 200, "narrowing stays open: {body}"); + let (status, body) = update(&ws, OWN, USER, json!({ "execution_mode": "anonymous" })).await?; + assert_eq!(status, 403, "{body}"); + assert!(body.contains("no-public-apps"), "{body}"); + assert_eq!(policy(&db, OWN).await?["execution_mode"], "publisher"); + + Ok(()) +} diff --git a/backend/tests/dependency_map.rs b/backend/tests/dependency_map.rs index dad6c86aa3..e68828fe24 100644 --- a/backend/tests/dependency_map.rs +++ b/backend/tests/dependency_map.rs @@ -511,6 +511,7 @@ def main(): ) .unwrap(), policy: None, + execution_mode: None, deployment_message: None, custom_path: None, preserve_on_behalf_of: None, diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 777209eda7..2398eb368a 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -15741,6 +15741,10 @@ paths: value: {} policy: $ref: "#/components/schemas/Policy" + execution_mode: + type: string + enum: [viewer, publisher, guest, anonymous] + description: "Changes who may open the app and leaves the rest of the deployed policy, its run-as user included, as it is. Applied under the same lock as a deploy, so neither overwrites the other. Cannot be sent with `policy`, which states its own mode. When the app runs as another user, refused to a caller who is neither an admin nor in 'wm_deployers', and to an operator with builder rights even in 'wm_deployers'; they change the mode by deploying." deployment_message: type: string custom_path: diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index bc0124b685..773e407372 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -602,6 +602,11 @@ pub struct EditApp { pub summary: Option, pub value: Option>>, pub policy: Option, + /// Changes only who may open the app: applied to the deployed policy under the row lock a + /// deploy takes, so it neither reverts a concurrent deploy's policy nor is reverted by it. + /// Exclusive with `policy`, which states its own mode. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub execution_mode: Option, pub deployment_message: Option, pub custom_path: Option, pub preserve_on_behalf_of: Option, @@ -2366,18 +2371,12 @@ fn check_operator_composed_app( // Viewer mode would make the triggerables collected above non-exhaustive (see the docs), and // the deploy panel shows it as members-only, so an admin deploying a draft would not see it. - // Pin an omitted mode on deploy: `update_app_internal` resolves it to the deployed app's, so a - // redeploy over an admin's viewer-mode app would otherwise inherit `Viewer`. - match policy.stated_execution_mode() { - Some(ExecutionMode::Viewer) => { - return Err(Error::PermissionDenied(format!( - "Operators with builder rights cannot {verb} an app that runs as its viewer" - ))) - } - None if write == BuilderAppWrite::Deploy => { - policy.set_execution_mode(ExecutionMode::Publisher) - } - _ => {} + // An omitted mode is resolved by the deploy: create's default, or on update the deployed mode, + // which `update_app_internal` turns into `Publisher` for a builder when it is `Viewer`. + if policy.stated_execution_mode() == Some(ExecutionMode::Viewer) { + return Err(Error::PermissionDenied(format!( + "Operators with builder rights cannot {verb} an app that runs as its viewer" + ))); } referenced.sort(); @@ -2473,15 +2472,7 @@ pub(crate) async fn validate_operator_composed_app( // docs/operator-builder-rights.md); a draft keeps whatever it names, which an admin's // one-click draft deploy preserves without showing it. if let (BuilderAppWrite::Draft, Some(policy)) = (write, policy.as_deref()) { - let names_another = policy - .on_behalf_of - .as_deref() - .is_some_and(|p| p != username_to_permissioned_as(&authed.username)) - || policy - .on_behalf_of_email - .as_deref() - .is_some_and(|e| e != authed.email); - if names_another { + if policy_runs_as_another(policy, authed) { return Err(Error::PermissionDenied( "Operators with builder rights cannot save an app that runs as someone else" .to_string(), @@ -2492,6 +2483,17 @@ pub(crate) async fn validate_operator_composed_app( require_runnables_readable(authed, db, user_db, w_id, referenced).await } +fn policy_runs_as_another(policy: &Policy, authed: &ApiAuthed) -> bool { + policy + .on_behalf_of + .as_deref() + .is_some_and(|p| p != username_to_permissioned_as(&authed.username)) + || policy + .on_behalf_of_email + .as_deref() + .is_some_and(|e| e != authed.email) +} + async fn require_runnables_readable( authed: &ApiAuthed, db: &DB, @@ -3809,10 +3811,22 @@ async fn update_app_internal<'a>( } } - if caller_capabilities(&db, w_id, &authed).await?.apps == AuthorLevel::Compose { + if ns.policy.is_some() && ns.execution_mode.is_some() { + return Err(Error::BadRequest( + "Send execution_mode either inside the policy or alone, not both".to_string(), + )); + } + // The policy a mode-only change writes is only known under the row lock, so a builder's + // check on it runs there. + let mode_only = ns.policy.is_none() && ns.value.is_none() && ns.execution_mode.is_some(); + + let composes_only = caller_capabilities(&db, w_id, &authed).await?.apps == AuthorLevel::Compose; + if composes_only { // Restoring a full-code version always sends allow_kind_change. A builder converts // nothing, so drop it and let the locked kind check below refuse an actual conversion. ns.allow_kind_change = None; + } + if composes_only && !mode_only { validate_operator_composed_app( &authed, &db, @@ -3890,6 +3904,7 @@ async fn update_app_internal<'a>( || ns.summary.is_some() || ns.custom_path.is_some() || ns.labels.is_some() + || ns.execution_mode.is_some() // A source deploy may send nothing but the value, and its runnables are // what the derived grants describe: skipping the UPDATE here would keep // the grants keyed to the sources this deploy just replaced. @@ -3907,7 +3922,12 @@ async fn update_app_internal<'a>( // that carries no policy keeps the deployed mode, read under the row // lock so a policy update landing alongside cannot slip a guest app // onto a path it cannot be scoped to. - let mode = match ns.policy.as_ref().and_then(|p| p.stated_execution_mode()) { + let stated = ns + .policy + .as_ref() + .and_then(|p| p.stated_execution_mode()) + .or(ns.execution_mode); + let mode = match stated { Some(mode) => mode, None => sqlx::query_scalar::<_, Option>( "SELECT policy->>'execution_mode' FROM app @@ -3987,12 +4007,13 @@ async fn update_app_internal<'a>( // no policy at all, in which case the rest of the deployed one carries // over. Either way a policy is about to be written wholesale. let caller_sent_policy = ns.policy.is_some(); - if caller_sent_policy || derived_triggerables.is_some() { + if caller_sent_policy || derived_triggerables.is_some() || ns.execution_mode.is_some() { // One locked read serving everything below: the mode an omitted one // inherits, the already-anonymous check, and the policy a source - // deploy carries over. FOR UPDATE holds the row until this - // transaction's UPDATE commits, so a policy edit landing between the - // read and the write cannot be clobbered by this stale snapshot. + // deploy or a mode-only change carries over. FOR UPDATE holds the row + // until this transaction's UPDATE commits, so a policy edit landing + // between the read and the write cannot be clobbered by this stale + // snapshot. let deployed = sqlx::query_scalar!( "SELECT policy FROM app WHERE path = $1 AND workspace_id = $2 FOR UPDATE", path, @@ -4006,30 +4027,81 @@ async fn update_app_internal<'a>( let mut npolicy = match ns.policy.take() { Some(npolicy) => npolicy, + None if deployed.is_none() => { + return Err(Error::NotFound(format!("App {path} not found"))) + } // Carrying the deployed policy forward is only safe if all of it // survived the round trip; a partial one would silently drop // `on_behalf_of`, sandboxing or S3 rules. None => deployed_policy.clone().ok_or_else(|| { - Error::internal_err(format!( - "app {path} has no readable policy to deploy its new sources under" - )) + Error::internal_err(format!("app {path} has no readable policy to update")) })?, }; if let Some(triggerables) = derived_triggerables { npolicy.triggerables = None; npolicy.triggerables_v2 = Some(triggerables); } + if let Some(mode) = ns.execution_mode { + // A policy write from someone who cannot keep the run-as user rebinds it + // to them, so the mode alone must not let them open an app running as + // someone else to a new audience. A builder who may keep it is refused too: + // its deploy checks the triggerables against its own RLS, which needs a + // second pool connection and so cannot run under this lock. + if policy_runs_as_another(&npolicy, &authed) + && (composes_only || !windmill_common::can_preserve_on_behalf_of(&authed)) + { + let runs_as = npolicy + .on_behalf_of_email + .as_deref() + .or(npolicy.on_behalf_of.as_deref()) + .unwrap_or("another user"); + return Err(Error::PermissionDenied(if composes_only { + format!( + "app {path} runs as {runs_as}: change who can open it by deploying it" + ) + } else { + format!( + "app {path} runs as {runs_as}: only admins and members of \ + wm_deployers can change who can open it without deploying it, and a \ + deploy from you makes it run as you" + ) + })); + } + npolicy.set_execution_mode(mode); + if composes_only { + // The check below sandboxes an app whose policy leaves `sandbox` unset; a + // mode-only change keeps every other field, so that is left to a deploy. + if npolicy.sandbox != Some(true) { + return Err(Error::PermissionDenied(format!( + "app {path} is not sandboxed: change who can open it by deploying it" + ))); + } + check_operator_composed_app( + BuilderAppWrite::Deploy, + raw_app, + None, + Some(&mut npolicy), + )?; + } + } validate_frontend_sdk_scopes(&npolicy)?; // The policy is written wholesale, so one that states no mode would // otherwise re-permission the app to the create-time default: a // `viewer` app would start running on the publisher's identity. Keep - // the deployed mode instead, and make the stored policy state it. + // the deployed mode instead, and make the stored policy state it. A + // builder may not deploy `Viewer` (see `check_operator_composed_app`), + // so a builder's redeploy over a viewer-mode app makes it `Publisher`. if npolicy.stated_execution_mode().is_none() { + let deployed_mode = deployed_policy + .as_ref() + .map(|d| d.execution_mode()) + .unwrap_or_default(); npolicy.set_execution_mode( - deployed_policy - .as_ref() - .map(|d| d.execution_mode()) - .unwrap_or_default(), + if composes_only && deployed_mode == ExecutionMode::Viewer { + ExecutionMode::Publisher + } else { + deployed_mode + }, ); } refuse_unscopable_guest_app( @@ -4155,7 +4227,8 @@ async fn update_app_internal<'a>( // CLI / git-sync deploys ask us to preserve any existing user draft at this // path instead of wiping it as part of the deploy. Only wipe the deployer's // own draft (plus the legacy NULL-email row) — see create_app_internal. - if !ns.skip_draft_deletion.unwrap_or(false) { + // A mode-only change publishes nothing from the draft, so it keeps it. + if !ns.skip_draft_deletion.unwrap_or(false) && !mode_only { sqlx::query!( "DELETE FROM draft WHERE path = $1 AND workspace_id = $2 AND typ IN ('app', 'raw_app') \ AND (email = $3 OR email IS NULL)", diff --git a/docs/operator-builder-rights.md b/docs/operator-builder-rights.md index 8445fa0dde..8b99401d32 100644 --- a/docs/operator-builder-rights.md +++ b/docs/operator-builder-rights.md @@ -104,12 +104,19 @@ ignored into `inlineScript`. `update_app` takes a builder too, but settings only: the deploy panel saves a deployed full-code app's access and frontend scopes through it with no value. It reaches the same check, so a builder -can no more switch the sandbox off there than on deploy. +can no more switch the sandbox off there than on deploy. A change of `execution_mode` alone runs +`check_operator_composed_app` on the deployed policy under the row lock, but not the readability +check, which resolves script versions on a second pool connection. It is therefore refused when +the app runs as anyone but the builder, `wm_deployers` or not: what an app running as its builder +invokes runs as that builder, who can already run it as themselves by opening the app. It is also +refused on an app whose stored policy is not sandboxed: the check would fill in the sandbox, and a +change of the mode alone keeps every other field of the policy. A builder's **draft** goes through the same `check_operator_composed_app` as its deploy, with `BuilderAppWrite::Draft`: an admin may open it in the editor or deploy it as it stands, so any rule added for deploys must hold for drafts too. Where the two differ, `write` decides: a deploy -fills in an omitted sandbox or mode, since it stores the policy it checked, while a draft skips +fills in an omitted sandbox, since it stores the policy it checked, and an omitted mode inherits the +deployed one, except that `Viewer` becomes `Publisher`, while a draft skips `triggerables_v2` and must not name another user's run identity. Every frontend writer of a builder draft must conform the policy first with `conformBuilderAppPolicy`, since a policy loaded from someone else's app breaks these rules: the raw-app editor component does, which covers both