mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 16:02:59 +00:00
refactor: derive an app's address from its principal instead of storing it
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EYWxvwmteVyzcWenJfbnzs
This commit is contained in:
co-authored by
Claude Opus 5
parent
49f8a253fd
commit
673e3f26e7
@@ -628,10 +628,6 @@ async fn test_folder_default_permissioned_as(db: Pool<Postgres>) -> anyhow::Resu
|
||||
policy["on_behalf_of"], "g/wm_deployers",
|
||||
"app policy.on_behalf_of gets folder default"
|
||||
);
|
||||
assert_eq!(
|
||||
policy["on_behalf_of_email"], "group-wm_deployers@windmill.dev",
|
||||
"the stored address is derived from that principal"
|
||||
);
|
||||
|
||||
// 7b. Admin, non-matching path — acting user
|
||||
let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN")
|
||||
|
||||
@@ -405,7 +405,6 @@ async fn test_preserve_on_behalf_of(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
// 7. App: Admin preserves on_behalf_of
|
||||
// ========================================
|
||||
|
||||
// Principal only, so the stored address can only have come from deriving it.
|
||||
let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN")
|
||||
.json(&new_app_with_on_behalf_of(
|
||||
"u/test-user/app_admin_preserve",
|
||||
@@ -435,12 +434,10 @@ async fn test_preserve_on_behalf_of(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
Some("u/original-user"),
|
||||
"Admin should preserve app on_behalf_of"
|
||||
);
|
||||
// The address is written through from the principal, never taken from the request, so the
|
||||
// stored copy can only agree with it.
|
||||
assert_eq!(
|
||||
policy.get("on_behalf_of_email").and_then(|v| v.as_str()),
|
||||
Some("original@windmill.dev"),
|
||||
"the stored address is derived from the principal, not the one the client sent"
|
||||
policy.get("on_behalf_of_email"),
|
||||
None,
|
||||
"the address is not stored beside the principal"
|
||||
);
|
||||
let resp = authed(
|
||||
client().get(format!("{base}/apps/get/p/u/test-user/app_admin_preserve")),
|
||||
@@ -452,7 +449,7 @@ async fn test_preserve_on_behalf_of(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
assert_eq!(
|
||||
returned["policy"]["on_behalf_of_email"].as_str(),
|
||||
Some("original@windmill.dev"),
|
||||
"the response returns the address written through from the principal"
|
||||
"the response derives the address from the principal"
|
||||
);
|
||||
|
||||
// ========================================
|
||||
@@ -530,24 +527,33 @@ async fn test_preserve_on_behalf_of(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
);
|
||||
|
||||
// ========================================
|
||||
// 9b. App: a policy naming two different accounts is rejected
|
||||
// 9b. App: an address in the request is ignored, the principal decides
|
||||
// ========================================
|
||||
|
||||
// This is the shape a workspace deploy produces when it carries the source
|
||||
// workspace's principal beside the target's address.
|
||||
// The shape a client written before the principal existed still sends. It names another
|
||||
// account than the principal does, and the principal is the one that counts.
|
||||
let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN")
|
||||
.json(&new_app_with_on_behalf_of(
|
||||
"u/test-user/app_mismatched_pair",
|
||||
"u/test-user/app_address_ignored",
|
||||
Some("u/original-user"),
|
||||
Some("test2@windmill.dev"),
|
||||
true,
|
||||
))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 201, "{}", resp.text().await?);
|
||||
|
||||
let app = sqlx::query!(
|
||||
"SELECT policy FROM app WHERE path = $1 AND workspace_id = $2",
|
||||
"u/test-user/app_address_ignored",
|
||||
"test-workspace"
|
||||
)
|
||||
.fetch_one(&db)
|
||||
.await?;
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
400,
|
||||
"a policy whose two halves name different accounts must be rejected"
|
||||
app.policy.get("on_behalf_of").and_then(|v| v.as_str()),
|
||||
Some("u/original-user"),
|
||||
"the principal is stored, not the account the address named"
|
||||
);
|
||||
|
||||
// ========================================
|
||||
@@ -2885,9 +2891,9 @@ async fn test_schedule_permissions_superadmin_not_in_workspace(
|
||||
|
||||
/// Reserved internal sentinel identities are rejected by name at deploy time on
|
||||
/// every entity that stores a preserved on_behalf_of. Real identities stay
|
||||
/// deployable by a `wm_deployers` member — including a real superadmin, and even
|
||||
/// their email pinned onto an unrelated principal — because that escalation is
|
||||
/// closed at execution by the job-token cap, not by restricting what is stored.
|
||||
/// deployable by a `wm_deployers` member — a real superadmin included — because
|
||||
/// that escalation is closed at execution by the job-token cap, not by
|
||||
/// restricting what is stored.
|
||||
#[sqlx::test(fixtures("preserve_on_behalf_of"))]
|
||||
async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
@@ -2900,18 +2906,17 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool<Postgres>) -> anyho
|
||||
const SENTINEL: &str = "superadmin_secret@windmill.dev";
|
||||
// Reserved sentinel matched on permissioned_as.
|
||||
const SYNC_SENTINEL: &str = "superadmin_sync@windmill.dev";
|
||||
// Real instance superadmin, present only in `password` (not a workspace member).
|
||||
const REAL_SA: &str = "superadmin-external@windmill.dev";
|
||||
|
||||
// App: deployer cannot pin the sentinel email.
|
||||
// App: deployer cannot pin the sentinel. The policy stores only a principal, and the
|
||||
// sentinels are address-shaped, so this is the bare-address form of one.
|
||||
let resp = authed(
|
||||
client().post(format!("{base}/apps/create")),
|
||||
"DEPLOYER_TOKEN",
|
||||
)
|
||||
.json(&new_app_with_on_behalf_of(
|
||||
"u/deployer-user/app_sentinel",
|
||||
Some("u/original-user"),
|
||||
Some(SENTINEL),
|
||||
None,
|
||||
true,
|
||||
))
|
||||
.send()
|
||||
@@ -2919,34 +2924,13 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool<Postgres>) -> anyho
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
400,
|
||||
"deployer must not pin the sentinel as an app on_behalf_of_email: {}",
|
||||
"deployer must not pin the sentinel as an app on_behalf_of: {}",
|
||||
resp.text().await?
|
||||
);
|
||||
|
||||
// App: a real superadmin's address pinned onto an unrelated principal is a pair naming two
|
||||
// accounts, which the principal-authoritative policy refuses.
|
||||
let resp = authed(
|
||||
client().post(format!("{base}/apps/create")),
|
||||
"DEPLOYER_TOKEN",
|
||||
)
|
||||
.json(&new_app_with_on_behalf_of(
|
||||
"u/deployer-user/app_real_sa",
|
||||
Some("u/original-user"),
|
||||
Some(REAL_SA),
|
||||
true,
|
||||
))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
400,
|
||||
"a superadmin address beside an unrelated principal must be refused: {}",
|
||||
resp.text().await?
|
||||
);
|
||||
|
||||
// App: a real superadmin on_behalf_of is allowed at deploy when consistently named (deployers
|
||||
// may deploy on behalf of any real user); the escalation is closed at execution by the
|
||||
// job-token cap, not by restricting what can be stored.
|
||||
// App: a real superadmin on_behalf_of is allowed at deploy (deployers may deploy on behalf
|
||||
// of any real user); the escalation is closed at execution by the job-token cap, not by
|
||||
// restricting what can be stored.
|
||||
let resp = authed(
|
||||
client().post(format!("{base}/apps/create")),
|
||||
"DEPLOYER_TOKEN",
|
||||
@@ -2954,7 +2938,7 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool<Postgres>) -> anyho
|
||||
.json(&new_app_with_on_behalf_of(
|
||||
"u/deployer-user/app_consistent_sa",
|
||||
Some("u/superadmin-external"),
|
||||
Some(REAL_SA),
|
||||
None,
|
||||
true,
|
||||
))
|
||||
.send()
|
||||
@@ -2962,7 +2946,7 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool<Postgres>) -> anyho
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
201,
|
||||
"deployer may preserve a consistently named superadmin identity: {}",
|
||||
"deployer may preserve a superadmin identity: {}",
|
||||
resp.text().await?
|
||||
);
|
||||
|
||||
|
||||
@@ -1054,9 +1054,10 @@ pub async fn fetch_api_authed_from_permissioned_as(
|
||||
username_override: Option<String>,
|
||||
) -> error::Result<ApiAuthed> {
|
||||
// Keyed by the supplied address, so an entry built for a principal's previous holder is reused
|
||||
// while that address is still supplied, until its 120s expiry: a cached dispatch address is
|
||||
// evicted sooner, an app's stored one (a username deleted then reused) may not be. Accepted;
|
||||
// the rebuild after expiry is the current holder's.
|
||||
// while that address is still supplied. Every caller now derives that address rather than
|
||||
// reading a stored one, so it goes stale only for as long as `get_email_from_permissioned_as`
|
||||
// serves the old answer, and the 120s expiry here caps whatever survives that. Accepted; the
|
||||
// rebuild after expiry is the current holder's.
|
||||
let key = (w_id.to_string(), permissioned_as.clone(), email.clone());
|
||||
|
||||
let mut api_authed = match API_AUTHED_CACHE.get(&key) {
|
||||
|
||||
@@ -117,7 +117,10 @@ async fn test_offboard_to_user(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
.fetch_one(&db)
|
||||
.await?;
|
||||
assert_eq!(
|
||||
(obo.on_behalf_of.as_deref(), obo.on_behalf_of_email.as_deref()),
|
||||
(
|
||||
obo.on_behalf_of.as_deref(),
|
||||
obo.on_behalf_of_email.as_deref()
|
||||
),
|
||||
(Some("u/test-user"), Some("test@windmill.dev")),
|
||||
"the reassignment moves the whole identity, not just the half this release reads"
|
||||
);
|
||||
@@ -618,8 +621,7 @@ async fn test_offboard_group_prefixed_member_keeps_group_identities(
|
||||
INSERT INTO group_(workspace_id, name, summary) VALUES ('test-workspace', 'ops', '');
|
||||
INSERT INTO app(workspace_id, path, summary, policy, versions, extra_perms)
|
||||
VALUES ('test-workspace', 'f/shared/ops_app', '',
|
||||
'{\"execution_mode\": \"publisher\", \"on_behalf_of\": \"g/ops\",
|
||||
\"on_behalf_of_email\": \"group-ops@windmill.dev\"}', '{}', '{}');",
|
||||
'{\"execution_mode\": \"publisher\", \"on_behalf_of\": \"g/ops\"}', '{}', '{}');",
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
@@ -33949,7 +33949,8 @@ components:
|
||||
description: The user or group the app runs as in anonymous or publisher mode (e.g. 'u/admin' or 'g/mygroup'). The authority for the app's identity.
|
||||
on_behalf_of_email:
|
||||
type: string
|
||||
description: Address of `on_behalf_of`, written through from it on every save and returned as stored. Optional; when absent it is derived from `on_behalf_of`. Sending it is optional too; it must name the same account as `on_behalf_of`, and a pair that disagrees is rejected.
|
||||
readOnly: true
|
||||
description: Address of `on_behalf_of`, derived from it on every read and never stored. Absent when the policy names no identity. Read-only - sending it is ignored, so a deploy that means to keep an identity has to name it in `on_behalf_of`.
|
||||
sandbox:
|
||||
type: boolean
|
||||
description: >
|
||||
|
||||
@@ -491,13 +491,9 @@ pub struct S3Key {
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default)]
|
||||
pub struct Policy {
|
||||
pub on_behalf_of: Option<String>,
|
||||
/// The address `on_behalf_of` resolves to. Every write stores what the principal resolves
|
||||
/// to, so it is not taken from the request except when a client names only the address —
|
||||
/// which is how a cross-workspace deploy carries an identity — and it is rejected when the
|
||||
/// two disagree. Optional: a policy without it executes by deriving from the principal, so
|
||||
/// removing it is a change of default rather than of behavior — see
|
||||
/// `docs/app-policy-email-removal.md`.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
/// The address `on_behalf_of` resolves to. Response-only: never stored and never read from
|
||||
/// the request. [`attach_on_behalf_of_email`] puts it here, and states which reads may.
|
||||
#[serde(skip_deserializing, skip_serializing_if = "Option::is_none")]
|
||||
pub on_behalf_of_email: Option<String>,
|
||||
//paths:
|
||||
// - script/<path>
|
||||
@@ -1130,7 +1126,7 @@ async fn get_app(
|
||||
check_scopes(&authed, || format!("apps:read:{}", path))?;
|
||||
let mut tx = user_db.begin(&authed).await?;
|
||||
|
||||
let app_o = if query.with_starred_info.unwrap_or(false) {
|
||||
let mut app_o = if query.with_starred_info.unwrap_or(false) {
|
||||
sqlx::query_as::<_, AppWithLastVersionAndStarred>(
|
||||
"SELECT app.id, app.path, app.summary, app.versions, app.policy, app.custom_path,
|
||||
app.extra_perms, app_version.value,
|
||||
@@ -1165,6 +1161,11 @@ async fn get_app(
|
||||
};
|
||||
tx.commit().await?;
|
||||
|
||||
// The deployed policy only. A draft overlaid below keeps whatever its editor last saved.
|
||||
if let Some(app) = app_o.as_mut() {
|
||||
attach_on_behalf_of_email(&mut app.app.policy, &w_id, &db).await?;
|
||||
}
|
||||
|
||||
// No deployed row + `get_draft`: fall back to the draft table; see scripts.rs.
|
||||
// Draft kind comes from the deployed row's `raw_app` flag, or for a draft-only
|
||||
// path from the caller's `raw_app` query param.
|
||||
@@ -1191,6 +1192,7 @@ async fn get_app(
|
||||
async fn get_app_lite(
|
||||
authed: ApiAuthed,
|
||||
Extension(user_db): Extension<UserDB>,
|
||||
Extension(db): Extension<DB>,
|
||||
Path((w_id, path)): Path<(String, StripPath)>,
|
||||
) -> JsonResult<AppWithLastVersion> {
|
||||
let path = path.to_path();
|
||||
@@ -1212,7 +1214,8 @@ async fn get_app_lite(
|
||||
|
||||
tx.commit().await?;
|
||||
|
||||
let app = not_found_if_none(app_o, "App", path)?;
|
||||
let mut app = not_found_if_none(app_o, "App", path)?;
|
||||
attach_on_behalf_of_email(&mut app.policy, &w_id, &db).await?;
|
||||
Ok(Json(app))
|
||||
}
|
||||
|
||||
@@ -1329,6 +1332,7 @@ async fn custom_path_exists(
|
||||
async fn get_app_by_id(
|
||||
authed: ApiAuthed,
|
||||
Extension(user_db): Extension<UserDB>,
|
||||
Extension(db): Extension<DB>,
|
||||
Path((w_id, id)): Path<(String, i64)>,
|
||||
) -> JsonResult<AppWithLastVersion> {
|
||||
let mut tx = user_db.begin(&authed).await?;
|
||||
@@ -1346,10 +1350,11 @@ async fn get_app_by_id(
|
||||
.await?;
|
||||
tx.commit().await?;
|
||||
|
||||
let app = not_found_if_none(app_o, "App", id.to_string())?;
|
||||
let mut app = not_found_if_none(app_o, "App", id.to_string())?;
|
||||
|
||||
check_scopes(&authed, || format!("apps:read:{}", &app.path))?;
|
||||
|
||||
attach_on_behalf_of_email(&mut app.policy, &w_id, &db).await?;
|
||||
Ok(Json(app))
|
||||
}
|
||||
|
||||
@@ -1403,6 +1408,7 @@ async fn get_public_app_by_secret(
|
||||
app.bundle_secret = Some(compute_bundle_secret(&db, &w_id, &app.versions).await?);
|
||||
}
|
||||
|
||||
attach_on_behalf_of_email(&mut app.policy, &w_id, &db).await?;
|
||||
Ok(Json(app))
|
||||
}
|
||||
|
||||
@@ -2473,12 +2479,12 @@ async fn create_app_internal<'a>(
|
||||
// rebinding the stored principal, is known and accepted: see `resolve_on_behalf_of`.
|
||||
let should_preserve = app.preserve_on_behalf_of.unwrap_or(false)
|
||||
&& windmill_common::can_preserve_on_behalf_of(&authed)
|
||||
&& (app.policy.on_behalf_of.is_some() || app.policy.on_behalf_of_email.is_some());
|
||||
&& app.policy.on_behalf_of.is_some();
|
||||
|
||||
let mut preserved_on_behalf_of: Option<String> = None;
|
||||
if should_preserve {
|
||||
app.policy.on_behalf_of = windmill_common::resolve_on_behalf_of(
|
||||
app.policy.on_behalf_of_email.as_deref(),
|
||||
None,
|
||||
app.policy.on_behalf_of.as_deref(),
|
||||
true,
|
||||
&authed,
|
||||
@@ -2496,18 +2502,14 @@ async fn create_app_internal<'a>(
|
||||
app.policy.on_behalf_of =
|
||||
Some(folder_default.unwrap_or_else(|| username_to_permissioned_as(&authed.username)));
|
||||
}
|
||||
app.policy.on_behalf_of_email = stored_on_behalf_of_email(&app.policy, w_id, &db).await?;
|
||||
if should_preserve {
|
||||
preserved_on_behalf_of = audited_on_behalf_of(&app.policy, &authed);
|
||||
preserved_on_behalf_of = audited_on_behalf_of(&app.policy, &authed, w_id, &db).await?;
|
||||
}
|
||||
|
||||
// Reject a forged superadmin run identity in the (possibly preserved) policy.
|
||||
// Done on the non-RLS pool before the transaction below, like the resolution
|
||||
// above, to avoid holding a second connection while `tx` is checked out.
|
||||
windmill_common::auth::validate_on_behalf_of(
|
||||
app.policy.on_behalf_of.as_deref(),
|
||||
app.policy.on_behalf_of_email.as_deref(),
|
||||
)?;
|
||||
windmill_common::auth::validate_on_behalf_of(app.policy.on_behalf_of.as_deref(), None)?;
|
||||
|
||||
let mut tx = user_db.clone().begin(&authed).await?;
|
||||
let path = app.path.clone();
|
||||
@@ -3410,11 +3412,11 @@ async fn update_app_internal<'a>(
|
||||
if let Some(npolicy) = ns.policy.as_mut() {
|
||||
let should_preserve = ns.preserve_on_behalf_of.unwrap_or(false)
|
||||
&& windmill_common::can_preserve_on_behalf_of(&authed)
|
||||
&& (npolicy.on_behalf_of.is_some() || npolicy.on_behalf_of_email.is_some());
|
||||
&& npolicy.on_behalf_of.is_some();
|
||||
|
||||
if should_preserve {
|
||||
npolicy.on_behalf_of = windmill_common::resolve_on_behalf_of(
|
||||
npolicy.on_behalf_of_email.as_deref(),
|
||||
None,
|
||||
npolicy.on_behalf_of.as_deref(),
|
||||
true,
|
||||
&authed,
|
||||
@@ -3425,9 +3427,8 @@ async fn update_app_internal<'a>(
|
||||
} else {
|
||||
npolicy.on_behalf_of = Some(username_to_permissioned_as(&authed.username));
|
||||
}
|
||||
npolicy.on_behalf_of_email = stored_on_behalf_of_email(npolicy, w_id, &db).await?;
|
||||
if should_preserve {
|
||||
preserved_on_behalf_of = audited_on_behalf_of(npolicy, &authed);
|
||||
preserved_on_behalf_of = audited_on_behalf_of(npolicy, &authed, w_id, &db).await?;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5542,44 +5543,65 @@ async fn app_load_csv_preview() -> Result<()> {
|
||||
))
|
||||
}
|
||||
|
||||
/// The address to store beside the principal. Derived from it, never taken from the request, so
|
||||
/// the stored copy can only ever agree with the principal — the drift it used to allow is what
|
||||
/// this replaces.
|
||||
/// Attach to a policy on its way out the address its principal resolves to.
|
||||
///
|
||||
/// Written unconditionally, including for the versions that could derive it instead: a replica
|
||||
/// predating that fallback fails outright when the key is absent, which would 400 every
|
||||
/// anonymous, publisher and guest app for the length of a rolling deploy. The write is what
|
||||
/// holds the key in place — see `docs/app-policy-email-removal.md`.
|
||||
async fn stored_on_behalf_of_email(policy: &Policy, w_id: &str, db: &DB) -> Result<Option<String>> {
|
||||
let Some(permissioned_as) = policy.on_behalf_of.as_deref() else {
|
||||
return Ok(None);
|
||||
/// Derived here rather than left to the caller because the app bundle reads it as `ctx.author`
|
||||
/// in the browser, where an anonymous viewer has no session to resolve `u/alice` with. Cached,
|
||||
/// like every other derivation of a run identity that is not being persisted: nothing reads this
|
||||
/// back as authority, so the accepted dispatch staleness `get_email_from_permissioned_as`
|
||||
/// documents applies.
|
||||
///
|
||||
/// Only ever for a policy the server itself wrote. A draft's principal is whatever its editor
|
||||
/// last typed, so resolving one would answer "which address is `u/x`?" for any principal a
|
||||
/// caller cares to name — including a superadmin outside their own workspaces, whom
|
||||
/// `resolve_username_to_email` reaches instance-wide.
|
||||
pub async fn attach_on_behalf_of_email(
|
||||
policy: &mut sqlx::types::Json<Box<RawValue>>,
|
||||
w_id: &str,
|
||||
db: &DB,
|
||||
) -> Result<()> {
|
||||
// Field-level, not a `Policy` round-trip: a legacy policy carries keys this struct has
|
||||
// never had, and returning it shorn of them is not this function's job.
|
||||
let mut value = serde_json::from_str::<serde_json::Value>(policy.0.get()).map_err(to_anyhow)?;
|
||||
let Some(object) = value.as_object_mut() else {
|
||||
return Ok(());
|
||||
};
|
||||
Ok(Some(
|
||||
windmill_common::users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db)
|
||||
.await?,
|
||||
))
|
||||
let Some(permissioned_as) = object
|
||||
.get("on_behalf_of")
|
||||
.and_then(|v| v.as_str())
|
||||
.map(str::to_string)
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
let email =
|
||||
windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db).await?;
|
||||
object.insert("on_behalf_of_email".to_string(), json!(email));
|
||||
*policy = sqlx::types::Json(serde_json::value::to_raw_value(&value).map_err(to_anyhow)?);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// The address to record in the `apps.on_behalf_of` audit entry: the one the app will run as,
|
||||
/// when it is not the deployer's own. `None` when they match — a deployer handing an app their
|
||||
/// own identity is not an on-behalf-of deploy.
|
||||
///
|
||||
/// Reads the address `stored_on_behalf_of_email` just resolved rather than looking it up again,
|
||||
/// so the audit row and the policy row can only ever name the same account.
|
||||
fn audited_on_behalf_of(policy: &Policy, authed: &ApiAuthed) -> Option<String> {
|
||||
policy
|
||||
.on_behalf_of_email
|
||||
.as_deref()
|
||||
.filter(|email| *email != authed.email)
|
||||
.map(str::to_string)
|
||||
/// Uncached, unlike the read path above: this one is written to a row that is never revisited,
|
||||
/// so a stale answer here is a permanent misattribution of the deploy.
|
||||
async fn audited_on_behalf_of(
|
||||
policy: &Policy,
|
||||
authed: &ApiAuthed,
|
||||
w_id: &str,
|
||||
db: &DB,
|
||||
) -> Result<Option<String>> {
|
||||
let Some(permissioned_as) = policy.on_behalf_of.as_deref() else {
|
||||
return Ok(None);
|
||||
};
|
||||
let email =
|
||||
windmill_common::users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db)
|
||||
.await?;
|
||||
Ok((email != authed.email).then_some(email))
|
||||
}
|
||||
|
||||
/// The identity an anonymous, publisher or guest execution runs as.
|
||||
///
|
||||
/// `on_behalf_of_email` is optional: every write stores it, so it is present on anything this
|
||||
/// release deployed, and it is only derived for a policy that predates that. Deriving is the
|
||||
/// fallback rather than the rule so that removing the key later is a change of default, not a
|
||||
/// change of behavior — see `docs/app-policy-email-removal.md`.
|
||||
async fn get_on_behalf_of(policy: &Policy, w_id: &str, db: &DB) -> Result<(String, String)> {
|
||||
let permissioned_as = policy
|
||||
.on_behalf_of
|
||||
@@ -5591,15 +5613,10 @@ async fn get_on_behalf_of(policy: &Policy, w_id: &str, db: &DB) -> Result<(Strin
|
||||
)
|
||||
})?
|
||||
.to_string();
|
||||
let email = match policy.on_behalf_of_email.as_deref() {
|
||||
Some(email) => email.to_string(),
|
||||
// Cached on purpose, up to one notify poll stale: the accepted dispatch case
|
||||
// `get_email_from_permissioned_as` documents.
|
||||
None => {
|
||||
windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db)
|
||||
.await?
|
||||
}
|
||||
};
|
||||
// Cached on purpose, up to one notify poll stale: the accepted dispatch case
|
||||
// `get_email_from_permissioned_as` documents.
|
||||
let email =
|
||||
windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db).await?;
|
||||
// Defence in depth against a policy that already carries a forged superadmin
|
||||
// sentinel (deployed before validation existed, or copied verbatim by a
|
||||
// workspace fork): the sentinels are internal-only and never a legitimate app
|
||||
@@ -5746,14 +5763,11 @@ async fn build_args(
|
||||
"email" => authed.as_ref().map(|a| serde_json::to_value(&a.email)),
|
||||
"workspace" => Some(serde_json::to_value(&w_id)),
|
||||
"groups" => authed.as_ref().map(|a| serde_json::to_value(&a.groups)),
|
||||
// Same rule as `get_on_behalf_of`: the stored address, derived only when absent.
|
||||
// Derived from the principal, cached, exactly as `get_on_behalf_of` does: this
|
||||
// is the same address the run it is an argument to executes under.
|
||||
"author" => {
|
||||
let author = match (
|
||||
policy.on_behalf_of_email.as_deref(),
|
||||
policy.on_behalf_of.as_deref(),
|
||||
) {
|
||||
(Some(email), _) => Some(email.to_string()),
|
||||
(None, Some(permissioned_as)) => Some(
|
||||
let author = match policy.on_behalf_of.as_deref() {
|
||||
Some(permissioned_as) => Some(
|
||||
windmill_common::users::get_email_from_permissioned_as(
|
||||
permissioned_as,
|
||||
w_id,
|
||||
@@ -5761,7 +5775,7 @@ async fn build_args(
|
||||
)
|
||||
.await?,
|
||||
),
|
||||
(None, None) => None,
|
||||
None => None,
|
||||
};
|
||||
Some(serde_json::to_value(&author))
|
||||
}
|
||||
|
||||
@@ -285,13 +285,11 @@ pub fn check_on_behalf_of_preservation(
|
||||
/// Resolves the identity to store when creating/updating a flow, script or app.
|
||||
///
|
||||
/// The permissioned_as is the identity: it decides what the job may access, and the address is
|
||||
/// a function of it, so the two can never name different accounts. For a script or flow the
|
||||
/// address is derived at read time; an app still stores it, as a compatibility copy written
|
||||
/// through from the principal on every save and returned verbatim by the app reads (see
|
||||
/// `docs/app-policy-email-removal.md`). Callers may supply either: a bare email (every client
|
||||
/// written before the principal existed) is resolved to the principal it names, and an email
|
||||
/// that names nobody is rejected rather than recorded, since it could only produce a runnable
|
||||
/// that cannot authenticate.
|
||||
/// a function of it, derived at read time rather than stored, so the two can never name
|
||||
/// different accounts. Callers may still supply either for a script or flow: a bare email
|
||||
/// (every client written before the principal existed) is resolved to the principal it names,
|
||||
/// and an email that names nobody is rejected rather than recorded, since it could only produce
|
||||
/// a runnable that cannot authenticate. An app policy has no address to supply.
|
||||
///
|
||||
/// Returns `None` when the runnable has no on-behalf-of identity, and the caller's own
|
||||
/// identity when they are not allowed to preserve someone else's.
|
||||
@@ -303,9 +301,8 @@ pub fn check_on_behalf_of_preservation(
|
||||
/// Known, accepted race. The lookup runs on the pool, outside the caller's write transaction, so
|
||||
/// an account renamed or removed between the two has its sweep run before the write is visible,
|
||||
/// and the write stores the old principal. The runnable then fails to authenticate until it is
|
||||
/// deployed with a current identity, with two exceptions: an app naming an external superadmin
|
||||
/// keeps running as that account through its stored address, and if the freed username is later
|
||||
/// given to another account, the stale principal binds to that account and runs as it. Every
|
||||
/// deployed with a current identity, with one exception: if the freed username is later given to
|
||||
/// another account, the stale principal binds to that account and runs as it. Every
|
||||
/// caller shares this (scripts, flows and apps, address-only inputs included), and it needs a
|
||||
/// rename or removal of the exact account inside the lookup-to-commit gap. Closing it means
|
||||
/// serializing every identity write against every identity mutation, across all runnable kinds
|
||||
|
||||
@@ -572,7 +572,7 @@ const command = new Command()
|
||||
})
|
||||
.command(
|
||||
"set-permissioned-as",
|
||||
"Set the on_behalf_of_email for an app (requires admin or wm_deployers group)"
|
||||
"Set the user an app runs on behalf of (requires admin or wm_deployers group)"
|
||||
)
|
||||
.arguments("<path:string> <email:string>")
|
||||
.action((async (opts: any, appPath: string, email: string) => {
|
||||
@@ -592,15 +592,15 @@ const command = new Command()
|
||||
// Only spread remote.policy — spreading the full remote object would include
|
||||
// `value` and trigger a new app version on every call. EditApp has all-Option
|
||||
// fields, so a minimal body only updates the policy column.
|
||||
// The read carries a derived `on_behalf_of_email`; a server old enough to still read it
|
||||
// would see the previous holder's address beside the new principal and reject the pair.
|
||||
const policy = { ...(remote.policy as any), on_behalf_of: `u/${username}` };
|
||||
delete policy.on_behalf_of_email;
|
||||
await wmill.updateApp({
|
||||
workspace: workspace.workspaceId,
|
||||
path: appPath,
|
||||
requestBody: {
|
||||
policy: {
|
||||
...(remote.policy as any),
|
||||
on_behalf_of: `u/${username}`,
|
||||
on_behalf_of_email: email,
|
||||
} as any,
|
||||
policy: policy as any,
|
||||
preserve_on_behalf_of: true,
|
||||
// Preserve any user draft at this path (see backend skip_draft_deletion).
|
||||
skip_draft_deletion: true,
|
||||
|
||||
@@ -1,47 +1,31 @@
|
||||
# Removing `policy.on_behalf_of_email`
|
||||
# `policy.on_behalf_of_email`
|
||||
|
||||
An app's identity is `policy.on_behalf_of`; the address beside it is a function of that
|
||||
principal. `on_behalf_of_email` is no longer required — a policy carrying only a principal
|
||||
executes, deriving the address — but it is still written on every save, and that is the only
|
||||
thing holding it in place.
|
||||
principal. The policy stores only the principal. The address is derived wherever it is needed:
|
||||
at execution (`get_on_behalf_of`, `$ctx:author`) and on the reads that return a deployed policy
|
||||
(`attach_on_behalf_of_email`), where it is a response-only field the write path ignores.
|
||||
|
||||
## The gate
|
||||
|
||||
**This cannot ship until `MIN_KEEP_ALIVE_VERSION` (`windmill-common/src/min_version.rs`) has
|
||||
passed 1.810.**
|
||||
|
||||
`get_on_behalf_of` gained its derive-when-absent fallback in **1.810**. Every replica before that
|
||||
*requires* the key and errors outright without it, so it would 400 every anonymous, publisher and
|
||||
guest app saved by a newer one. A rolling deploy runs both versions at once, which is why the
|
||||
write stays until no replica older than 1.810 can be live — in practice, once
|
||||
`MIN_KEEP_ALIVE_VERSION` (`windmill-common/src/min_version.rs`) has passed it.
|
||||
*requires* the key and errors outright without it, so a rolling deploy that runs both would 400
|
||||
every anonymous, publisher and guest app the moment one of them saves. The write is what holds
|
||||
the key in place, so stopping it is only safe once no replica older than 1.810 can be live.
|
||||
|
||||
There is no `MIN_VERSION_*` constant for this and it does not need one: those exist to gate
|
||||
behavior at runtime or to trip the build when a constraint expires, and nothing here does either.
|
||||
The key is written unconditionally, so no replica ever meets its absence until someone follows
|
||||
the steps below.
|
||||
`MIN_KEEP_ALIVE_VERSION` is 1.420.0, and a compile-time rule keeps it at least 50 minor versions
|
||||
behind current, so it cannot reach 1.810 until releases pass ~1.860.
|
||||
|
||||
## Step 1 — stop writing the key
|
||||
There is no `MIN_VERSION_*` constant for this and it does not need one: those gate behavior at
|
||||
runtime or trip the build when a constraint expires, and nothing here does either.
|
||||
|
||||
- `stored_on_behalf_of_email` in `windmill-api/src/apps.rs`, and the `create_app` /
|
||||
`update_app_internal` call sites that store what it returns.
|
||||
- The CLI and frontend workspace-deploy paths (`cli/windmill-utils-internal/src/deploy.ts`,
|
||||
`frontend/src/lib/utils_workspace_deploy.ts`). These send the address *instead of* a principal
|
||||
for a cross-workspace deploy, which is the one case where it is the only identity available —
|
||||
so this is "stop sending it once the target resolves a principal itself", not a deletion.
|
||||
## Why the address is not derived on a draft read
|
||||
|
||||
Policies written before this keep their key and keep being read from it; they agree with their
|
||||
principal, so nothing has to strip them.
|
||||
|
||||
## Step 2 — drop the field
|
||||
|
||||
Remove `on_behalf_of_email` from `Policy` and the fallback in `get_on_behalf_of`, which then
|
||||
always derives. Optionally strip the key from stored policies.
|
||||
|
||||
This can ship with step 1. It is written separately because step 1 alone is revertible without
|
||||
touching stored data or the response schema, and because the gate above is what makes either
|
||||
step safe — nothing about step 2 needs its own waiting period.
|
||||
|
||||
## Why the address is not derived on read
|
||||
|
||||
Read paths return the stored address verbatim rather than recomputing it. Deriving on read means
|
||||
resolving a principal that, for a draft, is caller-controlled — which turns the read into an
|
||||
oracle for addresses the caller cannot otherwise see, and leaves a principal that resolves to
|
||||
nobody with no address at all. Both were live defects while the read paths did derive.
|
||||
A deployed policy's principal was written by the server, from an identity `resolve_on_behalf_of`
|
||||
validated. A draft's is whatever its editor last typed. Deriving one would answer "which address
|
||||
is `u/x`?" for any principal a caller cares to name — including a superadmin outside their own
|
||||
workspaces, whom `resolve_username_to_email` reaches instance-wide. That was a live defect while
|
||||
every read derived, and is why the derivation is attached per read path rather than to `Policy`
|
||||
itself.
|
||||
|
||||
@@ -144,7 +144,6 @@
|
||||
}
|
||||
}
|
||||
let canPreserve = $derived(!!$userStore?.is_admin || !!$userStore?.is_super_admin || isDeployer)
|
||||
let savedOnBehalfOfEmail = $derived(savedApp?.policy?.on_behalf_of_email)
|
||||
let savedOnBehalfOf = $derived(savedApp?.policy?.on_behalf_of)
|
||||
let onBehalfOfChoice: OnBehalfOfChoice = $state(undefined)
|
||||
let customOnBehalfOfEmail: string = $state('')
|
||||
@@ -315,22 +314,19 @@
|
||||
user to run this app on behalf of. Once deployed the app will be run on behalf of
|
||||
<OnBehalfOfSelector
|
||||
targetWorkspace={opWs ?? ''}
|
||||
targetValue={savedOnBehalfOfEmail}
|
||||
targetValue={savedOnBehalfOf}
|
||||
selected={onBehalfOfChoice}
|
||||
onSelect={(choice, details) => {
|
||||
onBehalfOfChoice = choice
|
||||
if (choice === 'me') {
|
||||
policy.on_behalf_of_email = $userStore?.email
|
||||
policy.on_behalf_of = `u/${$userStore?.username}`
|
||||
customOnBehalfOfEmail = ''
|
||||
preserveOnBehalfOf = false
|
||||
} else if (choice === 'target') {
|
||||
policy.on_behalf_of_email = savedOnBehalfOfEmail
|
||||
policy.on_behalf_of = savedOnBehalfOf
|
||||
customOnBehalfOfEmail = ''
|
||||
preserveOnBehalfOf = true
|
||||
} else if (choice === 'custom' && details) {
|
||||
policy.on_behalf_of_email = details.email
|
||||
policy.on_behalf_of = details.permissionedAs
|
||||
customOnBehalfOfEmail = details.email
|
||||
preserveOnBehalfOf = true
|
||||
|
||||
@@ -172,7 +172,6 @@
|
||||
on_behalf_of: $userStore?.username.includes('@')
|
||||
? $userStore?.username
|
||||
: `u/${$userStore?.username}`,
|
||||
on_behalf_of_email: $userStore?.email,
|
||||
execution_mode: 'publisher'
|
||||
}
|
||||
|
||||
|
||||
@@ -211,7 +211,6 @@
|
||||
on_behalf_of: $userStore?.username.includes('@')
|
||||
? $userStore?.username
|
||||
: `u/${$userStore?.username}`,
|
||||
on_behalf_of_email: $userStore?.email,
|
||||
execution_mode: 'publisher'
|
||||
} as any
|
||||
// Explicit path seed: the fork-a-draft handoff re-homes the source
|
||||
|
||||
Reference in New Issue
Block a user