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

refactor: make the app policy's address optional instead of derived on read

Browse Source
This commit is contained in:
Ruben Fiszel
2026-08-04 06:31:56 +00:00
parent 3536740d06
commit 9c310cef01
8 changed files with 65 additions and 423 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/.sqlx/query-02c1a4d76fd7edf99c93be8ff57952d3c36312a1f9f97226004466a80a737b23.json
Generated
-17
View File
@@ -1,17 +0,0 @@
{
"db_name": "PostgreSQL",
"query": "INSERT INTO draft (workspace_id, email, path, typ, value)\n VALUES ($1, $2, $3, 'app', $4)",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Varchar",
"Varchar",
"Varchar",
"Json"
]
},
"nullable": []
},
"hash": "02c1a4d76fd7edf99c93be8ff57952d3c36312a1f9f97226004466a80a737b23"
}
backend/.sqlx/query-89fe60a488284d3b0a3162ff0f1660f9d501ac7ae32c2d21e31d8f92cec94fa3.json
Generated
-17
View File
@@ -1,17 +0,0 @@
{
"db_name": "PostgreSQL",
"query": "INSERT INTO draft (workspace_id, email, path, typ, value)\n VALUES ($1, $2, $3, 'raw_app', $4)",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Varchar",
"Varchar",
"Varchar",
"Json"
]
},
"nullable": []
},
"hash": "89fe60a488284d3b0a3162ff0f1660f9d501ac7ae32c2d21e31d8f92cec94fa3"
}
backend/.sqlx/query-dadf78bae0299b24f6798e7a2cc86f0ecfcf63daa0b185994a410eb2fe41fad9.json
Generated
-23
View File
@@ -1,23 +0,0 @@
{
"db_name": "PostgreSQL",
"query": "SELECT email FROM usr WHERE workspace_id = $1 AND username = $2",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "email",
"type_info": "Varchar"
}
],
"parameters": {
"Left": [
"Text",
"Text"
]
},
"nullable": [
false
]
},
"hash": "dadf78bae0299b24f6798e7a2cc86f0ecfcf63daa0b185994a410eb2fe41fad9"
}
backend/tests/preserve_on_behalf_of.rs
-164
View File
@@ -1391,171 +1391,7 @@ async fn test_app_update_preserves_on_behalf_of(db: Pool<Postgres>) -> anyhow::R
Ok(())
}
/// A draft saved while the address was stored independently can name an account its principal
/// no longer resolves to. The editor deploys the draft's policy verbatim, so reading it back
/// has to derive the address too, or that draft can never be deployed again.
#[sqlx::test(fixtures("preserve_on_behalf_of"))]
async fn test_app_draft_policy_derives_on_behalf_of_email(db: Pool<Postgres>) -> anyhow::Result<()>
{
initialize_tracing().await;
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let base = format!("http://localhost:{port}/api/w/test-workspace");
let path = "u/original-user/app_with_drifted_draft";
let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN")
.json(&new_app_with_on_behalf_of(
path,
Some("u/original-user"),
Some("original@windmill.dev"),
true,
))
.send()
.await?;
assert_eq!(
resp.status(),
201,
"Should create app: {}",
resp.text().await?
);
sqlx::query!(
"INSERT INTO draft (workspace_id, email, path, typ, value)
VALUES ($1, $2, $3, 'app', $4)",
"test-workspace",
"test@windmill.dev",
path,
json!({
"path": path,
"summary": "Drifted draft",
"value": { "type": "rawapp", "inline_script": null },
"policy": {
"execution_mode": "anonymous",
"triggerables": {},
"on_behalf_of": "u/original-user",
"on_behalf_of_email": "stale@windmill.dev"
}
})
)
.execute(&db)
.await?;
let resp = authed(
client().get(format!("{base}/apps/get/p/{path}?get_draft=true")),
"SECRET_TOKEN",
)
.send()
.await?;
assert_eq!(resp.status(), 200, "Should get app with draft");
let body: serde_json::Value = resp.json().await?;
let draft_policy = &body["draft"]["policy"];
assert_eq!(
draft_policy.get("on_behalf_of_email").and_then(|v| v.as_str()),
Some("original@windmill.dev"),
"Draft policy address should be derived from its principal, not the stored one"
);
// The AI chat deploys the draft it reads straight from the draft table instead.
let resp = authed(
client().get(format!("{base}/drafts/get_own/app/{path}")),
"SECRET_TOKEN",
)
.send()
.await?;
assert_eq!(resp.status(), 200, "Should get own draft");
let own: serde_json::Value = resp.json().await?;
assert_eq!(
own["value"]["policy"]
.get("on_behalf_of_email")
.and_then(|v| v.as_str()),
Some("original@windmill.dev"),
"Draft read straight from the draft table should derive its address too"
);
// Deploy the draft exactly as the editor received it.
let resp = authed(
client().post(format!("{base}/apps/update/{path}")),
"SECRET_TOKEN",
)
.json(&json!({
"summary": "Drifted draft",
"policy": draft_policy,
"preserve_on_behalf_of": true
}))
.send()
.await?;
assert_eq!(
resp.status(),
200,
"Deploying the draft back should not be rejected as a contradicting identity: {}",
resp.text().await?
);
Ok(())
}
/// A draft is stored unvalidated, so its principal is whatever its author typed. Resolving it
/// through the instance-wide fallback would let any member turn a guessed username into a
/// non-member superadmin's address.
#[sqlx::test(fixtures("preserve_on_behalf_of"))]
async fn test_app_draft_derivation_does_not_disclose_non_member_email(
db: Pool<Postgres>,
) -> anyhow::Result<()> {
initialize_tracing().await;
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let base = format!("http://localhost:{port}/api/w/test-workspace");
let path = "u/test-user-2/leak_probe";
// test-user-2 is an ordinary member; superadmin-external has no `usr` row here.
sqlx::query!(
"INSERT INTO draft (workspace_id, email, path, typ, value)
VALUES ($1, $2, $3, 'raw_app', $4)",
"test-workspace",
"test2@windmill.dev",
path,
json!({
"path": path,
"summary": "Probe",
"value": { "type": "rawapp", "inline_script": null },
"policy": {
"execution_mode": "anonymous",
"triggerables": {},
"on_behalf_of": "u/superadmin-external",
"on_behalf_of_email": "stale@windmill.dev"
}
})
)
.execute(&db)
.await?;
for (url, at) in [
(format!("{base}/drafts/get_own/raw_app/{path}"), "value"),
(
format!("{base}/apps/get/p/{path}?get_draft=true&raw_app=true"),
"draft",
),
] {
let resp = authed(client().get(&url), "SECRET_TOKEN_2").send().await?;
assert_eq!(resp.status(), 200, "Should read own draft at {url}");
let body = resp.text().await?;
assert!(
!body.contains("superadmin-external@windmill.dev"),
"Draft derivation must not disclose a non-member superadmin's address via {url}: {body}"
);
// Dropped, not left stale: the principal is one the deploy path accepts, so a stale
// address beside it would reject the draft forever.
let policy = &serde_json::from_str::<serde_json::Value>(&body)?[at]["policy"];
assert!(
policy.get("on_behalf_of_email").is_none(),
"Unresolvable principal should leave no address behind via {url}: {policy}"
);
}
Ok(())
}
/// Test schedule update preserves email/edited_by correctly
#[sqlx::test(fixtures("preserve_on_behalf_of"))]
backend/windmill-api/src/apps.rs
+21 -181
View File
@@ -256,162 +256,10 @@ pub struct AppWithLastVersionAndStarred {
pub starred: Option<bool>,
}
/// Fill `policy.on_behalf_of_email` from `policy.on_behalf_of` on every read path that returns
/// a policy, so what a client sees is a function of the principal rather than whatever copy the
/// row happens to hold.
///
/// `round_trips` picks the lookup. A read whose policy the client sends back on deploy must use
/// the uncached one: `resolve_on_behalf_of` validates the returned pair against the uncached
/// address, so a cached read would echo a stale address and 400 its own deploy for the minute
/// after an email change. The serving reads keep the cache — `get_public_app_by_secret` is an
/// anonymous page load, and nothing it returns comes back as a write.
///
/// The substring test keeps a policy with no principal off the parse-and-reserialize path
/// entirely, so an app without an on-behalf-of identity pays nothing either way.
async fn derive_policy_on_behalf_of_email(
db: &DB,
w_id: &str,
policy: &mut sqlx::types::Json<Box<RawValue>>,
round_trips: bool,
) -> Result<()> {
if !policy.0.get().contains("on_behalf_of") {
return Ok(());
}
let Ok(mut obj) =
serde_json::from_str::<serde_json::Map<String, serde_json::Value>>(policy.0.get())
else {
return Ok(());
};
if derive_on_behalf_of_email_in_place(db, w_id, &mut obj, round_trips).await? {
policy.0 = to_raw_value(&obj);
}
Ok(())
}
/// The draft variant of the derivation. A draft is stored exactly as its editor sent it, with
/// no `resolve_on_behalf_of` pass, so its principal is caller-controlled: anyone who can save a
/// draft under their own `u/` path chooses it. `u/` is therefore resolved against workspace
/// membership alone, never `resolve_username_to_email`'s instance `password` fallback, which
/// would turn a guessed username into a non-member superadmin's address.
///
/// A principal naming no member has its address key dropped rather than answered or left as it
/// was: an external superadmin is a principal the deploy path accepts, so leaving a stale
/// address beside it would keep 400ing a draft that is otherwise fine, while dropping it lets
/// `resolve_on_behalf_of` derive from the principal alone. Nothing is disclosed either way.
///
/// `g/` and the bare-address form skip the lookup: the first is synthetic and the second is
/// already the address, so neither tells the caller anything their draft did not.
async fn derive_draft_on_behalf_of_email_in_place(
db: &DB,
w_id: &str,
obj: &mut serde_json::Map<String, serde_json::Value>,
) -> Result<bool> {
let Some(permissioned_as) = obj
.get("on_behalf_of")
.and_then(|v| v.as_str())
.map(|s| s.to_string())
else {
return Ok(false);
};
let email = match permissioned_as.strip_prefix(windmill_common::users::PERMISSIONED_AS_USER_PREFIX)
{
Some(username) => {
let member = sqlx::query_scalar!(
"SELECT email FROM usr WHERE workspace_id = $1 AND username = $2",
w_id,
username
)
.fetch_optional(db)
.await?;
match member {
Some(email) => email,
None => return Ok(obj.remove("on_behalf_of_email").is_some()),
}
}
None => {
windmill_common::users::get_email_from_permissioned_as_uncached(
&permissioned_as,
w_id,
db,
)
.await?
}
};
obj.insert("on_behalf_of_email".to_string(), json!(email));
Ok(true)
}
/// Same derivation on an already-parsed policy. Reports whether it rewrote the address, so
/// callers holding the raw form only pay to re-serialize when there was a principal to derive.
async fn derive_on_behalf_of_email_in_place(
db: &DB,
w_id: &str,
obj: &mut serde_json::Map<String, serde_json::Value>,
round_trips: bool,
) -> Result<bool> {
let Some(permissioned_as) = obj
.get("on_behalf_of")
.and_then(|v| v.as_str())
.map(|s| s.to_string())
else {
return Ok(false);
};
let email = if round_trips {
windmill_common::users::get_email_from_permissioned_as_uncached(&permissioned_as, w_id, db)
.await?
} else {
windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db).await?
};
obj.insert("on_behalf_of_email".to_string(), json!(email));
Ok(true)
}
/// A draft carries its own copy of the policy and is deployed from it verbatim, so it needs the
/// same derivation as the deployed one. A pair that drifted while the address was still stored
/// independently would otherwise survive here — the backfill only fills in a missing principal —
/// and deploying the draft unchanged would be rejected as a contradicting identity.
async fn derive_draft_policy_on_behalf_of_email(
db: &DB,
w_id: &str,
draft: Option<&mut serde_json::Value>,
) -> Result<()> {
let Some(policy) = draft
.and_then(|d| d.get_mut("policy"))
.and_then(|p| p.as_object_mut())
else {
return Ok(());
};
derive_draft_on_behalf_of_email_in_place(db, w_id, policy).await?;
Ok(())
}
/// The same repair for a draft served straight out of the draft table rather than through
/// `get_app`, which is how the AI chat and the other-users' -draft banner read app drafts.
///
/// Not an authorization boundary: it resolves an address for whatever workspace and principal
/// it is handed, so a caller acting for a user MUST already have established their read access
/// to that draft.
pub(crate) async fn derive_stored_draft_policy_on_behalf_of_email(
db: &DB,
w_id: &str,
value: &mut sqlx::types::Json<Box<RawValue>>,
) -> Result<()> {
if !value.0.get().contains("on_behalf_of") {
return Ok(());
}
let Ok(mut obj) =
serde_json::from_str::<serde_json::Map<String, serde_json::Value>>(value.0.get())
else {
return Ok(());
};
let Some(policy) = obj.get_mut("policy").and_then(|p| p.as_object_mut()) else {
return Ok(());
};
if derive_draft_on_behalf_of_email_in_place(db, w_id, policy).await? {
value.0 = to_raw_value(&obj);
}
Ok(())
}
#[derive(Serialize)]
pub struct AppHistory {
@@ -471,10 +319,12 @@ pub struct S3Key {
#[derive(Serialize, Deserialize, Debug, Clone, Default)]
pub struct Policy {
pub on_behalf_of: Option<String>,
/// Derived from `on_behalf_of`, never taken from the request: reads fill it from the
/// principal and writes store what the principal resolves to. Accepted on write so a client
/// naming only the address still resolves to a principal, and rejected there when the two
/// disagree.
/// 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 on read: a policy without it executes by deriving from the
/// principal, which is what makes removing it a change of default (`MIN_VERSION_DERIVES_
/// APP_POLICY_EMAIL`, `docs/app-policy-email-removal.md`).
#[serde(skip_serializing_if = "Option::is_none")]
pub on_behalf_of_email: Option<String>,
//paths:
@@ -1077,7 +927,7 @@ async fn get_app(
check_scopes(&authed, || format!("apps:read:{}", path))?;
let mut tx = user_db.begin(&authed).await?;
let mut app_o = if query.with_starred_info.unwrap_or(false) {
let 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,
@@ -1112,10 +962,6 @@ async fn get_app(
};
tx.commit().await?;
if let Some(app) = app_o.as_mut() {
derive_policy_on_behalf_of_email(&db, &w_id, &mut app.app.policy, true).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.
@@ -1125,7 +971,7 @@ async fn get_app(
None if query.raw_app.unwrap_or(false) => UserDraftItemKind::RawApp,
None => UserDraftItemKind::App,
};
let mut overlay = overlay_or_draft_only(
let overlay = overlay_or_draft_only(
&db,
&w_id,
&authed.email,
@@ -1136,14 +982,12 @@ async fn get_app(
|| windmill_common::error::Error::NotFound(format!("App not found at path {path}")),
)
.await?;
derive_draft_policy_on_behalf_of_email(&db, &w_id, overlay.draft.as_mut()).await?;
Ok(Json(overlay))
}
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();
@@ -1165,8 +1009,7 @@ async fn get_app_lite(
tx.commit().await?;
let mut app = not_found_if_none(app_o, "App", path)?;
derive_policy_on_behalf_of_email(&db, &w_id, &mut app.policy, false).await?;
let app = not_found_if_none(app_o, "App", path)?;
Ok(Json(app))
}
@@ -1283,7 +1126,6 @@ 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?;
@@ -1301,14 +1143,10 @@ async fn get_app_by_id(
.await?;
tx.commit().await?;
let mut app = not_found_if_none(app_o, "App", id.to_string())?;
let app = not_found_if_none(app_o, "App", id.to_string())?;
check_scopes(&authed, || format!("apps:read:{}", &app.path))?;
// Round-tripping: the deployment-history drawer redeploys the version it fetched here, so
// an address left stale by another replica's email change would come back as a
// contradicting pair.
derive_policy_on_behalf_of_email(&db, &w_id, &mut app.policy, true).await?;
Ok(Json(app))
}
@@ -1376,7 +1214,6 @@ async fn get_public_app_by_secret(
app.bundle_secret = Some(compute_bundle_secret(&db, &w_id, &app.versions).await?);
}
derive_policy_on_behalf_of_email(&db, &w_id, &mut app.policy, false).await?;
Ok(Json(app))
}
@@ -4860,12 +4697,10 @@ fn audited_on_behalf_of(policy: &Policy, authed: &ApiAuthed) -> Option<String> {
/// The identity an anonymous or publisher execution runs as.
///
/// The address becomes the job's `permissioned_as_email`, from which
/// `fetch_authed_from_permissioned_as` derives the superadmin flag and the instance groups, so it
/// must not be stale. It is read through `EMAIL_CACHE` anyway: the `notify_user_email_change`
/// trigger evicts the key on every change that can move it, on every replica, the same way this
/// route's own `APP_POLICY_CACHE` is kept honest. Resolving per execution instead would put the
/// only uncached query on a path that otherwise serves entirely from cache.
/// `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
@@ -4877,8 +4712,13 @@ async fn get_on_behalf_of(policy: &Policy, w_id: &str, db: &DB) -> Result<(Strin
)
})?
.to_string();
let email =
windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db).await?;
let email = match policy.on_behalf_of_email.as_deref() {
Some(email) => email.to_string(),
None => {
windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db)
.await?
}
};
Ok((permissioned_as, email))
}
backend/windmill-api/src/drafts.rs
-21
View File
@@ -618,10 +618,6 @@ async fn get_draft_for_user(
.fetch_optional(&db)
.await?;
let mut row = row;
if let Some(row) = row.as_mut() {
derive_app_draft_identity(&db, &w_id, kind, &mut row.value).await?;
}
row.map(Json).ok_or_else(|| {
Error::NotFound(format!(
"no draft for {} at {path}",
@@ -658,26 +654,9 @@ async fn get_own_draft(
)
.fetch_optional(&db)
.await?;
let mut row = row;
if let Some(row) = row.as_mut() {
derive_app_draft_identity(&db, &w_id, kind, &mut row.value).await?;
}
Ok(Json(row))
}
/// An app draft is deployed from its own copy of the policy, so the address beside its
/// principal has to be derived here too — see `apps::derive_stored_draft_policy_on_behalf_of_email`.
async fn derive_app_draft_identity(
db: &DB,
w_id: &str,
kind: UserDraftItemKind,
value: &mut sqlx::types::Json<Box<serde_json::value::RawValue>>,
) -> Result<()> {
if matches!(kind, UserDraftItemKind::App | UserDraftItemKind::RawApp) {
crate::apps::derive_stored_draft_policy_on_behalf_of_email(db, w_id, value).await?;
}
Ok(())
}
/// The deployed table RLS resolves item-level `extra_perms` against.
/// Delegates to `UserDraftItemKind::deployed_table()` (the shared single
backend/windmill-common/src/min_version.rs
+8
View File
@@ -8,6 +8,14 @@ use semver::Version;
// reads that column is live, and those runnables run as their deployer with no error anywhere.
pub const MIN_VERSION_SUPPORTS_ON_BEHALF_OF_PRINCIPAL: VC =
vc(1, 776, 0, "On-behalf-of principal");
/// Names the release that stops *requiring* `policy.on_behalf_of_email`: an app policy carrying
/// only a principal executes, deriving the address from it. The key is still written, so a
/// replica below this version keeps finding what it expects. Nothing gates on this at runtime;
/// it exists so `vc()`'s compile-time assert fires once `MIN_KEEP_ALIVE_VERSION` passes it, at
/// which point the key can go — see `docs/app-policy-email-removal.md`.
pub const MIN_VERSION_DERIVES_APP_POLICY_EMAIL: VC =
vc(1, 778, 0, "App policy email derived from on_behalf_of");
pub const MIN_VERSION_SUPPORTS_NODE_DEBOUNCING: VC = vc(1, 658, 0, "Flow node debouncing");
pub const MIN_VERSION_SUPPORTS_TOKEN_HASH: VC = vc(1, 659, 0, "Token hash storage");
pub const MIN_VERSION_SUPPORTS_SYNC_JOBS_DEBOUNCING: VC = vc(1, 602, 0, "Sync jobs debouncing");
docs/app-policy-email-removal.md
+36
View File
@@ -0,0 +1,36 @@
# Removing `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, because a replica below
`MIN_VERSION_DERIVES_APP_POLICY_EMAIL` (`windmill-common/src/min_version.rs`) errors outright
when the key is absent, and a rolling deploy runs both versions at once.
When that constraint stops compiling, no supported replica requires the key.
## Release A — code only, key still written
1. `stored_on_behalf_of_email` in `windmill-api/src/apps.rs`, and the `update_app_internal` /
`create_app` call sites that store what it returns.
2. The CLI and frontend workspace-deploy paths that send an address for a cross-workspace
deploy (`cli/windmill-utils-internal/src/deploy.ts`,
`frontend/src/lib/utils_workspace_deploy.ts`). They send the address *instead of* a
principal, which is the one case where it is the only identity available — so this step is
really "stop sending it once the target can resolve a principal itself", not a deletion.
## Release B — once every replica runs A
3. Drop the key from `Policy` and stop reading it in `get_on_behalf_of`, which then always
derives. A migration can strip it from stored policies, or leave it as an ignored field.
Removing it in release A would break the replicas still on the release before it: their
`get_on_behalf_of` requires the key and 400s every anonymous and publisher app without it.
## 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. The
address a policy carries is written from its principal on every save, so the stored value is
already the derived one for anything this release wrote.