From 2b525d28dbcda4a4b0f626ad40b0c4d79cfbcd53 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 1 Aug 2026 00:29:30 +0200 Subject: [PATCH] fix: honor on-behalf-of when a workflow step dispatches a script or flow (#10437) * fix: run on-behalf-of scripts under their own identity from workflow steps Co-Authored-By: Claude Opus 5 (1M context) * fix: correct the on-behalf-of provenance comment in the script draft deploy Co-Authored-By: Claude Opus 5 (1M context) * test: pin preserve_on_behalf_of forwarding in the script and flow draft deploys Co-Authored-By: Claude Opus 5 (1M context) * docs: drop the historical comparison from the draft-deploy test comment Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- backend/windmill-store/src/variables.rs | 40 +++- backend/windmill-worker/src/bun_executor.rs | 197 +++++++++++--------- frontend/src/lib/utils_draft_deploy.test.ts | 90 ++++++++- frontend/src/lib/utils_draft_deploy.ts | 8 +- 4 files changed, 241 insertions(+), 94 deletions(-) diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 55047ec0c2..3b70a3e3a7 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -10,7 +10,7 @@ use windmill_api_auth::{ build_scope_path_predicate, check_scopes, maybe_refresh_folders, require_owner_of_path, ApiAuthed, }; -use windmill_common::db::DB; +use windmill_common::db::{Authable, DB}; use windmill_common::workspaces::{check_deploy_rules, RuleCheckResult}; use crate::secret_backend_ext::{ @@ -367,10 +367,10 @@ async fn get_variable( { return Ok(Json(overlay)); } - explain_variable_perm_error(&path, &w_id, &db).await?; + explain_variable_perm_error(&path, &w_id, &db, Some(&authed)).await?; unreachable!() } else { - explain_variable_perm_error(&path, &w_id, &db).await?; + explain_variable_perm_error(&path, &w_id, &db, Some(&authed)).await?; unreachable!() }; @@ -461,10 +461,27 @@ async fn get_value( .map(Json); } +/// The grants alone can't explain a denial: a job started on behalf of another +/// user is authorized as that user, so the error has to name who was actually +/// asking or it reads as a grant bug. +fn describe_authed(authed: Option<&(impl Authable + Sync)>) -> String { + match authed { + Some(authed) => format!( + "username: {}, email: {}, groups: {:?}, folders: {:?}", + authed.username(), + authed.email(), + authed.groups(), + authed.folders() + ), + None => "unauthenticated".to_string(), + } +} + async fn explain_variable_perm_error( path: &str, w_id: &str, db: &sqlx::Pool, + authed: Option<&(impl Authable + Sync)>, ) -> windmill_common::error::Result<()> { let extra_perms = sqlx::query_scalar!( "SELECT extra_perms from variable WHERE path = $1 AND workspace_id = $2", @@ -489,13 +506,14 @@ async fn explain_variable_perm_error( .fetch_optional(db) .await?; return Err(Error::NotAuthorized(format!( - "Variable exists but you don't have access to it:\nvariable perms: {}\nfolder perms: {}", - serde_json::to_string_pretty(&extra_perms).unwrap_or_default(), serde_json::to_string_pretty(&folder_extra_perms).unwrap_or_default() + "Variable exists but you don't have access to it:\nvariable perms: {}\nfolder perms: {}\nauthed as: {}", + serde_json::to_string_pretty(&extra_perms).unwrap_or_default(), serde_json::to_string_pretty(&folder_extra_perms).unwrap_or_default(), describe_authed(authed) ))); } else { return Err(Error::NotAuthorized(format!( - "Variable exists but you don't have access to it:\nvariable perms: {}", - serde_json::to_string_pretty(&extra_perms).unwrap_or_default() + "Variable exists but you don't have access to it:\nvariable perms: {}\nauthed as: {}", + serde_json::to_string_pretty(&extra_perms).unwrap_or_default(), + describe_authed(authed) ))); } } @@ -1498,7 +1516,13 @@ pub async fn get_value_internal<'a>( let variable = if let Some(variable) = variable_o { variable } else { - explain_variable_perm_error(path, w_id, &db_with_opt_authed.db()).await?; + explain_variable_perm_error( + path, + w_id, + &db_with_opt_authed.db(), + db_with_opt_authed.authed(), + ) + .await?; unreachable!() }; diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 794514ba11..4f4d33b674 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -2512,10 +2512,11 @@ pub async fn handle_wac_v2_output( }; use serde_json::Value; use windmill_common::get_latest_flow_version_info_for_path; - use windmill_common::jobs::{script_path_to_payload, JobKind, JobPayload, RawCode}; + use windmill_common::jobs::{script_path_to_payload, JobKind, JobPayload, OnBehalfOf, RawCode}; use windmill_common::runnable_settings::{ ConcurrencySettings, ConcurrencySettingsWithCustom, DebouncingSettings, }; + use windmill_common::users::username_to_permissioned_as; use windmill_queue::{push, PushArgs, PushIsolationLevel}; let output = parse_wac_output(&result)?; @@ -2803,86 +2804,101 @@ pub async fn handle_wac_v2_output( let push_result: error::Result<()> = async { for (step, (_, child_uuid)) in steps.iter().zip(job_ids.iter()) { // Resolve job payload based on dispatch_type - let (job_payload, child_args, is_external) = match step.dispatch_type.as_str() { - "script" if step.script.starts_with("./") => { - // Module-relative path: resolve from parent script's modules - let module_key = step.script.strip_prefix("./").unwrap(); - let module = resolve_parent_module(modules, module_key)?; - let payload = JobPayload::Code(RawCode { - content: module.content, - path: job.runnable_path.clone(), - hash: None, - language: module.language, - lock: module.lock, - cache_ttl: job.cache_ttl, - cache_ignore_s3_path: job.cache_ignore_s3_path, - dedicated_worker: None, - concurrency_settings: ConcurrencySettingsWithCustom::default(), - debouncing_settings: DebouncingSettings::default(), - modules: None, - tag: None, - }); - let step_args: HashMap> = step - .args - .iter() - .map(|(k, v)| { - let raw = serde_json::value::to_raw_value(v).unwrap(); - (k.clone(), raw) - }) - .collect(); - (payload, step_args, true) - } - "script" => { - // Resolve script path to job payload (handles hash, lang, etc.) - let (payload, _, _, _, _, _) = script_path_to_payload( - &step.script, - None, // no authed db for background workers - db.clone(), - &job.workspace_id, - Some(true), // skip preprocessor - ) - .await?; - let step_args: HashMap> = step - .args - .iter() - .map(|(k, v)| { - let raw = serde_json::value::to_raw_value(v).unwrap(); - (k.clone(), raw) - }) - .collect(); - (payload, step_args, true) - } - "flow" => { - let flow_info = get_latest_flow_version_info_for_path( - None, - db, - &job.workspace_id, - &step.script, - true, - ) - .await?; - let payload = JobPayload::Flow { - path: step.script.clone(), - dedicated_worker: flow_info.dedicated_worker, - apply_preprocessor: false, - version: flow_info.version, - labels: flow_info.labels.clone(), - }; - let step_args: HashMap> = step - .args - .iter() - .map(|(k, v)| { - let raw = serde_json::value::to_raw_value(v).unwrap(); - (k.clone(), raw) - }) - .collect(); - (payload, step_args, true) - } - _ => { - // "inline" — re-run parent with _executing_key - (job_payload_template.clone(), parent_args.clone(), false) - } - }; + let (job_payload, child_args, is_external, on_behalf_of) = + match step.dispatch_type.as_str() { + "script" if step.script.starts_with("./") => { + // Module-relative path: resolve from parent script's modules + let module_key = step.script.strip_prefix("./").unwrap(); + let module = resolve_parent_module(modules, module_key)?; + let payload = JobPayload::Code(RawCode { + content: module.content, + path: job.runnable_path.clone(), + hash: None, + language: module.language, + lock: module.lock, + cache_ttl: job.cache_ttl, + cache_ignore_s3_path: job.cache_ignore_s3_path, + dedicated_worker: None, + concurrency_settings: ConcurrencySettingsWithCustom::default(), + debouncing_settings: DebouncingSettings::default(), + modules: None, + tag: None, + }); + let step_args: HashMap> = step + .args + .iter() + .map(|(k, v)| { + let raw = serde_json::value::to_raw_value(v).unwrap(); + (k.clone(), raw) + }) + .collect(); + // Inline module code, not a separate runnable: it has no + // identity of its own and runs as the parent. + (payload, step_args, true, None) + } + "script" => { + // Resolve script path to job payload (handles hash, lang, etc.) + let (payload, _, _, _, _, on_behalf_of) = script_path_to_payload( + &step.script, + None, // no authed db for background workers + db.clone(), + &job.workspace_id, + Some(true), // skip preprocessor + ) + .await?; + let step_args: HashMap> = step + .args + .iter() + .map(|(k, v)| { + let raw = serde_json::value::to_raw_value(v).unwrap(); + (k.clone(), raw) + }) + .collect(); + (payload, step_args, true, on_behalf_of) + } + "flow" => { + let flow_info = get_latest_flow_version_info_for_path( + None, + db, + &job.workspace_id, + &step.script, + true, + ) + .await?; + let payload = JobPayload::Flow { + path: step.script.clone(), + dedicated_worker: flow_info.dedicated_worker, + apply_preprocessor: false, + version: flow_info.version, + labels: flow_info.labels.clone(), + }; + let on_behalf_of = + flow_info.on_behalf_of_email.map(|email| OnBehalfOf { + email, + permissioned_as: username_to_permissioned_as( + &flow_info.edited_by, + ), + }); + let step_args: HashMap> = step + .args + .iter() + .map(|(k, v)| { + let raw = serde_json::value::to_raw_value(v).unwrap(); + (k.clone(), raw) + }) + .collect(); + (payload, step_args, true, on_behalf_of) + } + _ => { + // "inline" — re-run parent with _executing_key + ( + job_payload_template.clone(), + parent_args.clone(), + false, + None, + ) + } + }; let push_args = PushArgs { args: &child_args, extra: None }; @@ -2930,6 +2946,21 @@ pub async fn handle_wac_v2_output( } } + // A target runnable that opts into on-behalf-of runs under its own + // identity, never the caller's, so a step that reaches it through a + // workflow cannot widen or narrow its permissions. `created_by` still + // credits the caller, matching how the run API pushes these jobs. + let (child_email, child_permissioned_as) = match on_behalf_of.as_ref() { + Some(on_behalf_of) => ( + on_behalf_of.email.as_str(), + on_behalf_of.permissioned_as.clone(), + ), + None => ( + job.permissioned_as_email.as_str(), + job.permissioned_as.clone(), + ), + }; + let (_, mut tx) = push( db, PushIsolationLevel::IsolatedRoot(db.clone()), @@ -2937,8 +2968,8 @@ pub async fn handle_wac_v2_output( job_payload, push_args, &job.created_by, - &job.permissioned_as_email, - job.permissioned_as.clone(), + child_email, + child_permissioned_as, None, None, None, diff --git a/frontend/src/lib/utils_draft_deploy.test.ts b/frontend/src/lib/utils_draft_deploy.test.ts index bae50691cc..9bed95917b 100644 --- a/frontend/src/lib/utils_draft_deploy.test.ts +++ b/frontend/src/lib/utils_draft_deploy.test.ts @@ -1,5 +1,36 @@ -import { describe, it, expect } from 'vitest' -import { draftBaseIsStale } from './utils_draft_deploy' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { draftBaseIsStale, deployDraft } from './utils_draft_deploy' + +vi.mock('$lib/gen', () => ({ + ScriptService: { getScriptByPath: vi.fn(), createScript: vi.fn() }, + FlowService: { getFlowByPath: vi.fn(), createFlow: vi.fn(), updateFlow: vi.fn() }, + DraftService: { deleteDraft: vi.fn() }, + AppService: {}, + VariableService: {}, + ResourceService: {}, + ScheduleService: {}, + HttpTriggerService: {}, + WebsocketTriggerService: {}, + PostgresTriggerService: {}, + KafkaTriggerService: {}, + NatsTriggerService: {}, + MqttTriggerService: {}, + AmqpTriggerService: {}, + SqsTriggerService: {}, + GcpTriggerService: {}, + AzureTriggerService: {}, + EmailTriggerService: {} +})) +vi.mock('$lib/userDraftDbSyncer.svelte', () => ({ UserDraftDbSyncer: { save: vi.fn() } })) +vi.mock('$lib/workspaceDrafts.svelte', () => ({ invalidateWorkspaceDrafts: vi.fn() })) +vi.mock('$lib/workspaceComparison', () => ({ invalidateWorkspaceComparison: vi.fn() })) +vi.mock('$lib/localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) +vi.mock('$lib/rawAppDeploy', () => ({ deployRawAppDraft: vi.fn() })) +vi.mock('$lib/components/raw_apps/utils', () => ({ canonicalRawAppDiffValue: vi.fn() })) +vi.mock('$lib/appDiffSides', () => ({ classicAppDraftParts: vi.fn() })) +vi.mock('$lib/utils_deployable', () => ({ TRIGGER_RUNTIME_IGNORE: [] })) + +import { ScriptService, FlowService } from '$lib/gen' // draftBaseIsStale compares a draft's base pointer against the deployed head // of the item it was fetched with (`get_draft=true`). Shared by CompareDrafts @@ -38,3 +69,58 @@ describe('draftBaseIsStale', () => { expect(draftBaseIsStale('script', undefined)).toBe(false) }) }) + +// Without preserve_on_behalf_of the backend rewrites on_behalf_of_email to the +// deploying user, so deploying a draft silently re-points the runnable's +// identity. + +describe('deployDraft preserves on_behalf_of', () => { + beforeEach(() => vi.clearAllMocks()) + + it('script: forwards the flag when the draft carries an on_behalf_of_email', async () => { + vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({ + hash: 'v1', + draft: { path: 'f/admin/send_email', on_behalf_of_email: 'alice@windmill.dev' } + } as any) + + expect(await deployDraft('script', 'f/admin/send_email', 'ws')).toEqual({ success: true }) + expect(ScriptService.createScript).toHaveBeenCalledWith( + expect.objectContaining({ + requestBody: expect.objectContaining({ + on_behalf_of_email: 'alice@windmill.dev', + preserve_on_behalf_of: true + }) + }) + ) + }) + + it('script: omits the flag when the draft has no on_behalf_of_email', async () => { + vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({ + hash: 'v1', + draft: { path: 'f/admin/send_email' } + } as any) + + await deployDraft('script', 'f/admin/send_email', 'ws') + expect(ScriptService.createScript).toHaveBeenCalledWith( + expect.objectContaining({ + requestBody: expect.objectContaining({ preserve_on_behalf_of: undefined }) + }) + ) + }) + + it('flow: forwards the flag when the draft carries an on_behalf_of_email', async () => { + vi.mocked(FlowService.getFlowByPath).mockResolvedValueOnce({ + draft: { path: 'f/admin/notify', value: {}, on_behalf_of_email: 'alice@windmill.dev' } + } as any) + + expect(await deployDraft('flow', 'f/admin/notify', 'ws')).toEqual({ success: true }) + expect(FlowService.updateFlow).toHaveBeenCalledWith( + expect.objectContaining({ + requestBody: expect.objectContaining({ + on_behalf_of_email: 'alice@windmill.dev', + preserve_on_behalf_of: true + }) + }) + ) + }) +}) diff --git a/frontend/src/lib/utils_draft_deploy.ts b/frontend/src/lib/utils_draft_deploy.ts index 87baf2e5c7..a9e1f3ad99 100644 --- a/frontend/src/lib/utils_draft_deploy.ts +++ b/frontend/src/lib/utils_draft_deploy.ts @@ -459,7 +459,10 @@ export async function deployDraft( ...rest, path: scriptPath, parent_hash: r.hash, - deployment_message: deploymentMessage + deployment_message: deploymentMessage, + // Deploy the draft's on-behalf-of as-is; the backend resets it to the + // deploying user without this flag, gated by can_preserve_on_behalf_of. + preserve_on_behalf_of: rest.on_behalf_of_email ? true : undefined } }) // Then deploy any draft trigger edits, so they aren't dropped with the draft. @@ -482,6 +485,9 @@ export async function deployDraft( ws_error_handler_muted: d.ws_error_handler_muted, visible_to_runner_only: d.visible_to_runner_only, on_behalf_of_email: d.on_behalf_of_email, + // Same as scripts and apps: the backend resets on_behalf_of_email to the + // deploying user without this flag, gated by can_preserve_on_behalf_of. + preserve_on_behalf_of: d.on_behalf_of_email ? true : undefined, labels: d.labels, deployment_message: deploymentMessage }