From 71e7c5883816a3658fd8957fcbbe273ced3656ac Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 28 May 2026 12:00:13 +0200 Subject: [PATCH] refactor: route draft permission check through authed.folders + RLS, drop client-supplied email --- backend/windmill-api/src/drafts.rs | 99 ++++++++++++------- .../UserDraftConflictModal.svelte | 1 - frontend/src/lib/userDraft.svelte.ts | 8 +- .../src/lib/userDraftConflictStore.svelte.ts | 1 - frontend/src/lib/userDraftDbSyncer.svelte.ts | 34 +++---- 5 files changed, 81 insertions(+), 62 deletions(-) diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index 786b5f02e1..661ea8ce41 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -211,7 +211,7 @@ async fn list_users_with_draft_on_path( Path((w_id, kind, path)): Path<(String, String, windmill_common::utils::StripPath)>, ) -> Result>> { let path = path.to_path(); - require_access_to_path(&authed, &user_db, &w_id, &kind, path).await?; + require_can_read_path(&authed, &user_db, &w_id, &kind, path).await?; let rows = sqlx::query_as!( UserWithDraft, @@ -231,13 +231,47 @@ async fn list_users_with_draft_on_path( Ok(Json(rows)) } -/// Resolves to `Ok(())` only if `authed` can read the underlying item at -/// `path`. Implemented by issuing a `SELECT 1` through `user_db` (which -/// applies row-level extra_perms via `set_session_user`), so a user who -/// can't see the script/flow/app gets back zero rows. We don't leak the -/// set of paths that exist either way — both "not readable" and "doesn't -/// exist" return the same 404. -async fn require_access_to_path( +/// Each `UserDraftItemKind` maps to either its own table (where RLS can +/// resolve item-level extra_perms grants that bypass folder/owner checks) +/// or `None` (kinds without a backing table fall through to the path-only +/// access check below). +fn table_for_kind(kind: &str) -> Option<&'static str> { + match kind { + "script" => Some("script"), + "flow" => Some("flow"), + "app" | "raw_app" => Some("app"), + "resource" => Some("resource"), + "variable" => Some("variable"), + "trigger_schedule" => Some("schedule"), + "trigger_http" => Some("http_trigger"), + "trigger_websocket" => Some("websocket_trigger"), + "trigger_postgres" => Some("postgres_trigger"), + "trigger_kafka" => Some("kafka_trigger"), + "trigger_nats" => Some("nats_trigger"), + "trigger_mqtt" => Some("mqtt_trigger"), + "trigger_sqs" => Some("sqs_trigger"), + "trigger_gcp" => Some("gcp_trigger"), + "trigger_azure" => Some("azure_trigger"), + "trigger_email" | "trigger_default_email" => Some("email_trigger"), + "trigger_poll" | "trigger_cli" | "trigger_nextcloud" | "trigger_google" + | "trigger_github" => Some("native_trigger"), + // trigger_webhook is a property of script/flow rows, not its own row. + _ => None, + } +} + +/// Resolves to `Ok(())` if `authed` can read at `path`. Three layers, in +/// order of cheapness: +/// 1. admin → always. +/// 2. Path-prefix match against the user's own namespace (`u/{username}`) +/// or any folder in `authed.folders` (which is the precomputed read +/// set used to seed UserDB's session context, so groups + direct +/// grants on the folder are already factored in). +/// 3. RLS-aware `SELECT 1` against the kind's backing table — covers +/// item-level extra_perms grants that bypass folder/owner checks. +/// Both "not readable" and "doesn't exist" return a 404 — we don't leak +/// path existence to non-readers. +async fn require_can_read_path( authed: &ApiAuthed, user_db: &UserDB, w_id: &str, @@ -247,32 +281,31 @@ async fn require_access_to_path( if authed.is_admin { return Ok(()); } - let table = match kind { - "script" => "script", - "flow" => "flow", - "app" => "app", - // Item kinds without a backing path-permission table (raw_app, - // resource, variable, trigger_*, ...) fall back to the user's own - // `u/{authed.username}/...` path namespace. - _ => { - let prefix = format!("u/{}/", authed.username); - if path.starts_with(&prefix) { - return Ok(()); + let parts: Vec<&str> = path.splitn(3, '/').collect(); + if parts.len() >= 2 { + match parts[0] { + "u" if parts[1] == authed.username => return Ok(()), + "f" => { + let folder = parts[1]; + if authed.folders.iter().any(|(name, _, _)| name == folder) { + return Ok(()); + } } - return Err(Error::NotFound(format!("no draft visible at {path}"))); + _ => {} } - }; - let mut tx = user_db.clone().begin(authed).await?; - let query = - format!("SELECT 1 AS exists FROM {table} WHERE path = $1 AND workspace_id = $2 LIMIT 1"); - let row = sqlx::query_scalar::<_, i32>(&query) - .bind(path) - .bind(w_id) - .fetch_optional(&mut *tx) - .await?; - tx.commit().await?; - if row.is_none() { - return Err(Error::NotFound(format!("no {kind} visible at {path}"))); } - Ok(()) + if let Some(table) = table_for_kind(kind) { + let mut tx = user_db.clone().begin(authed).await?; + let query = format!("SELECT 1 FROM {table} WHERE path = $1 AND workspace_id = $2 LIMIT 1"); + let row = sqlx::query_scalar::<_, i32>(&query) + .bind(path) + .bind(w_id) + .fetch_optional(&mut *tx) + .await?; + tx.commit().await?; + if row.is_some() { + return Ok(()); + } + } + Err(Error::NotFound(format!("no draft visible at {path}"))) } diff --git a/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte b/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte index 93137f89d7..cc0efa725f 100644 --- a/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte +++ b/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte @@ -27,7 +27,6 @@ try { await syncDrafts({ workspace: conflict.workspace, - email: conflict.email, drafts: [ { itemKind: conflict.itemKind, diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index f39a7890f6..3c1efde0c5 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -361,11 +361,12 @@ function pushToSyncer( value: unknown, workspace: string ): void { - const email = get(userStore)?.email - if (!email) return + // Skip sync until a user is signed in — without a session the request + // would 401 and the queue would discard the entry. The next save after + // login will pick the draft up from localStorage and re-push. + if (get(userStore) === undefined) return UserDraftDbSyncer.pushDrafts({ workspace, - email, drafts: [{ itemKind, path, value }], onMissedDrafts: (drafts) => { for (const d of drafts) { @@ -379,7 +380,6 @@ function pushToSyncer( UserDraftConflictStore.enqueue( rejected.map((r) => ({ workspace, - email, itemKind: r.typ as UserDraftItemKind, rejected: r })) diff --git a/frontend/src/lib/userDraftConflictStore.svelte.ts b/frontend/src/lib/userDraftConflictStore.svelte.ts index bd0c4751dc..1c058da009 100644 --- a/frontend/src/lib/userDraftConflictStore.svelte.ts +++ b/frontend/src/lib/userDraftConflictStore.svelte.ts @@ -11,7 +11,6 @@ import type { RejectedDraft } from './userDraftDbSyncer.svelte' export type ConflictEntry = { workspace: string - email: string itemKind: UserDraftItemKind rejected: RejectedDraft } diff --git a/frontend/src/lib/userDraftDbSyncer.svelte.ts b/frontend/src/lib/userDraftDbSyncer.svelte.ts index c4fc23a64d..25d10d445f 100644 --- a/frontend/src/lib/userDraftDbSyncer.svelte.ts +++ b/frontend/src/lib/userDraftDbSyncer.svelte.ts @@ -37,25 +37,18 @@ export type RejectedDraftsCallback = (rejected: RejectedDraft[]) => void export type SyncOptions = { workspace: string - email: string drafts: PendingDraft[] onMissedDrafts?: MissedDraftCallback onDraftsRejected?: RejectedDraftsCallback } type QueueKey = string -function queueKey( - workspace: string, - email: string, - kind: UserDraftItemKind, - path: string -): QueueKey { - return `${workspace}|${email}|${kind}|${path}` +function queueKey(workspace: string, kind: UserDraftItemKind, path: string): QueueKey { + return `${workspace}|${kind}|${path}` } type QueuedEntry = { workspace: string - email: string itemKind: UserDraftItemKind path: string value: unknown @@ -105,20 +98,18 @@ async function flushQueue(): Promise { const entries = Array.from(queue.values()) queue.clear() - // Group entries by (workspace, email) — every sync call is scoped to a - // single email, so we issue one request per distinct group. In practice - // the queue is dominated by the active session's workspace+email, so - // there's almost always exactly one group. + // One request per workspace. The server scopes every row by the + // session's authed email, so we don't track user identity on the + // client side — a tab can only be logged in as one user at a time. const groups = new Map() for (const entry of entries) { - const key = `${entry.workspace}|${entry.email}` - const list = groups.get(key) + const list = groups.get(entry.workspace) if (list) list.push(entry) - else groups.set(key, [entry]) + else groups.set(entry.workspace, [entry]) } - for (const group of groups.values()) { - await runSync(group[0].workspace, group) + for (const [workspace, group] of groups.entries()) { + await runSync(workspace, group) } } @@ -133,7 +124,6 @@ async function runSync(workspace: string, group: QueuedEntry[]): Promise { })) await syncDrafts({ workspace, - email: group[0].email, drafts, onMissedDrafts, onDraftsRejected @@ -186,17 +176,15 @@ export const UserDraftDbSyncer = { /** * Enqueue drafts for a batched sync. Repeated pushes for the same - * (workspace, email, itemKind, path) coalesce — only the latest value / + * (workspace, itemKind, path) coalesce — only the latest value / * callbacks survive. Returns the same Promise as the eventual * `syncDrafts` so callers can `await` flush completion. */ pushDrafts(opts: SyncOptions): void { const ws = opts.workspace - const email = opts.email for (const d of opts.drafts) { - queue.set(queueKey(ws, email, d.itemKind, d.path), { + queue.set(queueKey(ws, d.itemKind, d.path), { workspace: ws, - email, itemKind: d.itemKind, path: d.path, value: d.value,