starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-08-18 16:02:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: honor on-behalf-of when a workflow step dispatches a script or flow (#10437)

Browse Source 
* fix: run on-behalf-of scripts under their own identity from workflow steps

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: correct the on-behalf-of provenance comment in the script draft deploy

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: pin preserve_on_behalf_of forwarding in the script and flow draft deploys

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: drop the historical comparison from the draft-deploy test comment

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-08-01 00:29:30 +02:00
committed by GitHub
parent bd7156682d
commit 2b525d28db
4 changed files with 241 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/windmill-store/src/variables.rs
+32 -8
View File
@@ -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<Postgres>,
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!()
};
backend/windmill-worker/src/bun_executor.rs
+114 -83
View File
@@ -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<String, Box<RawValue>> = 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<String, Box<RawValue>> = 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<String, Box<RawValue>> = 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<String, Box<RawValue>> = 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<String, Box<RawValue>> = 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<String, Box<RawValue>> = 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,
frontend/src/lib/utils_draft_deploy.test.ts
+88 -2
View File
@@ -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
})
})
)
})
})
frontend/src/lib/utils_draft_deploy.ts
+7 -1
View File
@@ -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
}