From e2a91ca2b141a0bee64ddfafe3cbf3ff86b6fb35 Mon Sep 17 00:00:00 2001 From: Guilhem Date: Fri, 18 Sep 2026 11:31:00 +0200 Subject: [PATCH 01/34] feat: flow chat job-backed detail, smooth streaming and answer chrome (#11186) * feat: flow chat job-backed detail, smooth streaming and answer chrome * chore: document setFlowPath, drop unused chatIdentity, tighten comments * fix: read a nested agent tool's call from its own job * fix: show only the model's arguments on a tool card read from its job * feat: add attachments to the chat sdk message type * refactor: read a flow chat message's files from the message, not its run * fix: keep every job-backed answer when reads finish out of order * refactor: build flow chat tool cards from the conversation row alone * feat: keep the model picker's current choice when a flow chat retry replays its run * fix: export ChatAttachment from the chat sdk entry point * fix: drop a conversation list for the flow path before setFlowPath * fix: return the current listing, not old-path rows, after setFlowPath * fix: treat a listing stale on kind or path as one stale listing * revert: drop setFlowPath and rebuild the chat when the flow path changes * fix: refuse a flow chat retry once any turn ran while it read the run * docs: say where a display message's createdAt comes from * style: drop the rule between New chat and the conversation list --- .../lib/components/FlowPreviewContent.svelte | 10 +- frontend/src/lib/components/ScrollFade.svelte | 66 ++++ .../copilot/chat/AIChatDisplay.svelte | 27 +- .../copilot/chat/AIChatManager.svelte.ts | 10 +- .../copilot/chat/AIChatMessage.svelte | 4 +- .../copilot/chat/AssistantMessage.svelte | 55 ++- .../copilot/chat/ToolExecutionDisplay.svelte | 19 +- .../src/lib/components/copilot/chat/shared.ts | 6 +- .../copilot/chat/typewriterReveal.ts | 7 + .../components/flows/content/FlowInput.svelte | 1 + .../flows/conversations/FlowChat.svelte | 50 ++- .../conversations/FlowChatInterface.svelte | 11 +- .../FlowConversationsSidebar.svelte | 2 +- .../flows/conversations/flowChatProps.ts | 19 + .../conversations/flowChatViewHost.svelte.ts | 292 +++++++++++---- .../conversations/flowChatViewHost.test.ts | 343 +++++++++++++++--- .../conversations/messageAttachments.test.ts | 25 ++ .../flows/conversations/messageAttachments.ts | 48 +++ .../(logged)/flows/get/[...path]/+page.svelte | 112 +++--- 19 files changed, 893 insertions(+), 214 deletions(-) create mode 100644 frontend/src/lib/components/ScrollFade.svelte create mode 100644 frontend/src/lib/components/flows/conversations/flowChatProps.ts create mode 100644 frontend/src/lib/components/flows/conversations/messageAttachments.test.ts create mode 100644 frontend/src/lib/components/flows/conversations/messageAttachments.ts diff --git a/frontend/src/lib/components/FlowPreviewContent.svelte b/frontend/src/lib/components/FlowPreviewContent.svelte index 32b46cb2be..5d543ae24d 100644 --- a/frontend/src/lib/components/FlowPreviewContent.svelte +++ b/frontend/src/lib/components/FlowPreviewContent.svelte @@ -471,7 +471,9 @@ return jobId ?? '' }} conversationKind="test" + frame="boxed" path={$pathStore} + identity={$initialPathStore || fakeInitialPath} inputSchema={flowStore.val.schema} flowModules={flowStore.val.value?.modules} /> @@ -558,7 +560,13 @@ {/if} {/if} -
+ +
{#if flowHasChanged()}
+ /** + * A soft edge on a scroller, so content scrolling out of view fades instead of being + * cut against whatever borders it. + * + * Rendered as an overlay in the scroller's positioned ancestor rather than inside the + * scroller: `sticky` would resolve against the scroller's padding box and leave the + * first few pixels unfaded. It shows only when there is something hidden in that + * direction, so a transcript that fits shows no edge at all. + */ + import { twMerge } from 'tailwind-merge' + + interface Props { + /** The scrolling element this masks. */ + scroller: HTMLElement | undefined + edge?: 'top' | 'bottom' + /** Tailwind colour stop to fade from — the surface the scroller sits on. */ + from?: string + /** Tailwind height of the fade band. */ + height?: string + class?: string + } + + let { + scroller, + edge = 'top', + from = 'from-surface', + height = 'h-4', + class: className = '' + }: Props = $props() + + let hidden = $state(true) + + $effect(() => { + const el = scroller + if (!el) return + const update = () => { + // A pixel of slack: fractional scroll offsets otherwise leave the bottom edge + // showing on a scroller that is already at its end. + hidden = + edge === 'top' ? el.scrollTop <= 1 : el.scrollTop + el.clientHeight >= el.scrollHeight - 1 + } + update() + el.addEventListener('scroll', update, { passive: true }) + // Content arriving or the pane resizing changes what is hidden without a scroll. + const observer = new ResizeObserver(update) + observer.observe(el) + if (el.firstElementChild) observer.observe(el.firstElementChild) + return () => { + el.removeEventListener('scroll', update) + observer.disconnect() + } + }) + + + diff --git a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte index 0085ca701f..b7f662b148 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte @@ -35,6 +35,7 @@ import ChatQuickActions from './ChatQuickActions.svelte' import ContextUsageIndicator from './ContextUsageIndicator.svelte' import AIChatModelSettings from './AIChatModelSettings.svelte' + import ScrollFade from '$lib/components/ScrollFade.svelte' import AssistantSettingsModal from './AssistantSettingsModal.svelte' import { SkillsMenu } from './skills/skillsMenu.svelte' import { McpMenu } from '$lib/components/mcp/mcpMenu.svelte' @@ -591,6 +592,15 @@ // The typing-dots indicator implies the AI is busy, which is misleading while // the loop is parked on the user; surface a text pill instead so users know to // act on the tool above. + // A step name hangs its icon in the column's left padding (see AssistantMessage), so a + // transcript carrying one widens the padding, on both sides to keep the column centred. + const agentGutter = $derived(messages.some((m) => m.role === 'assistant' && m.stepName)) + const columnClass = $derived( + wideLayout + ? `w-full max-w-3xl mx-auto ${agentGutter ? 'px-8' : 'px-7'}` + : `w-full max-w-2xl mx-auto ${agentGutter ? 'px-8' : 'px-3'}` + ) + const waitingForUserAction = $derived(chatHost.loading && !!pendingUserAction(messages)) // Gated on `loading` because a card restored from history still looks parked: @@ -823,12 +833,7 @@ the panel, or the Escape-to-stop focus check would wrongly reject them. --> bind:this={scrollElement} onscroll={onScroll} > -
+
{#each messages as message, messageIndex (messageIndex)} {/if}
+ + {#if showScrollToLatest}
{/if} -
+ +
{#if showFlowPendingActionControls}
+ {/if} +
+ {#if editable && row.id !== ADMIN_ROLE} + removeRole(row.id)} /> + {/if} +
+ +
+ {/each} +
+ + {#if editable && unusedRoles.length > 0} +
+ + dataTable.database.resource_type, - (resource_type) => { - dataTable.database = { - resource_type, - resource_path: - resource_type === 'instance' ? defaultInstanceDbName() : undefined + {#if dataTable.reference} +
+ Governed by + {dataTable.reference.workspace_id} + / + {dataTable.reference.datatable} + + This fork uses its parent's data table rather than a copy of it, so the database and + its roles are decided in that workspace. + +
+ {:else} +
+
+ {#if dataTable.database.resource_type === 'instance'} + + Use Windmill's PostgreSQL instance + + {/if} + does. // - // In "large mode" (viewport ≥ 1760px, where app.css bumps :root to 18px → + // In "large mode" (screen ≥ 1760px, where app.css bumps :root to 18px → // font 13.5px) headless-Chromium ink measurement showed the text sitting // ~1px low with the base leading. The residual is a fixed ~2px of line box, // so the exact centered value there is (content-box height − 2px). Scoped to - // the same 1760px breakpoint as the font-size bump; small mode is unchanged. + // the same query as the font-size bump; small mode is unchanged. export const inputLeadingClasses: Record = { - '2xs': 'leading-4 min-[1760px]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem - xs: 'leading-4 min-[1760px]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem - sm: 'leading-6 min-[1760px]:leading-[calc(1.5rem_-_2px)]', // h-7 − py-0.5 → 1.5rem - md: 'leading-8 min-[1760px]:leading-[calc(2rem_-_2px)]', // h-8, no py → 2rem - lg: 'leading-10 min-[1760px]:leading-[calc(2.5rem_-_2px)]' // h-10, no py → 2.5rem + '2xs': 'leading-4 [@media(min-device-width:1760px)]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem + xs: 'leading-4 [@media(min-device-width:1760px)]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem + sm: 'leading-6 [@media(min-device-width:1760px)]:leading-[calc(1.5rem_-_2px)]', // h-7 − py-0.5 → 1.5rem + md: 'leading-8 [@media(min-device-width:1760px)]:leading-[calc(2rem_-_2px)]', // h-8, no py → 2rem + lg: 'leading-10 [@media(min-device-width:1760px)]:leading-[calc(2.5rem_-_2px)]' // h-10, no py → 2.5rem } diff --git a/frontend/src/lib/editorFontSize.svelte.ts b/frontend/src/lib/editorFontSize.svelte.ts index 13d99915e3..96519b0aac 100644 --- a/frontend/src/lib/editorFontSize.svelte.ts +++ b/frontend/src/lib/editorFontSize.svelte.ts @@ -1,25 +1,25 @@ // Reactive Monaco font sizes that follow the global `:root` font-size -// breakpoint set in `frontend/src/lib/assets/app.css` (18px at ≥1760px). +// breakpoint set in `frontend/src/lib/assets/app.css` (18px on screens ≥1760px). // The values mirror Tailwind's `text-xs` (0.75rem) computed pixel size so // editors visually match the surrounding UI. -const LARGE_VIEWPORT_QUERY = '(min-width: 1760px)' +const LARGE_SCREEN_QUERY = '(min-device-width: 1760px)' -let isLargeViewport = $state(false) +let isLargeScreen = $state(false) if (typeof window !== 'undefined') { - const mq = window.matchMedia(LARGE_VIEWPORT_QUERY) - isLargeViewport = mq.matches + const mq = window.matchMedia(LARGE_SCREEN_QUERY) + isLargeScreen = mq.matches mq.addEventListener('change', (e) => { - isLargeViewport = e.matches + isLargeScreen = e.matches }) } export const editorFontSize = { get regular(): number { - return isLargeViewport ? 13.5 : 12 + return isLargeScreen ? 13.5 : 12 }, get small(): number { - return isLargeViewport ? 12 : 11 + return isLargeScreen ? 12 : 11 } } diff --git a/frontend/src/routes/(root)/(logged)/+layout.svelte b/frontend/src/routes/(root)/(logged)/+layout.svelte index d14cd2c2dc..58d852a2d5 100644 --- a/frontend/src/routes/(root)/(logged)/+layout.svelte +++ b/frontend/src/routes/(root)/(logged)/+layout.svelte @@ -139,8 +139,8 @@ let isCollapsed = $state(collapsePref.val) // Resizable desktop rail, sized in REM so it scales with the root font-size the - // same way the old `w-52`/`w-12` classes did — `:root` jumps to 18px past 1760px - // wide (app.css), which grows the rem-based button content; a fixed-px rail would + // same way the old `w-52`/`w-12` classes did — `:root` jumps to 18px on screens + // ≥1760px (app.css), which grows the rem-based button content; a fixed-px rail would // not grow with it and the content would overflow. SIDEBAR_MIN_REM is the default // expanded width (the old w-52); the handle only resizes when expanded and only // widens from there — collapsing is the toggle button's job, not the drag's. From 53afecd4588247bc1812d3e68a30db1f3c3b2724 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 18 Sep 2026 15:15:02 +0200 Subject: [PATCH 20/34] fix: register the job token with the sensitive log masking system (#10943) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(worker): register the job token with the log masking system The masking system covered secrets fetched through `get_value_internal` and `$encrypted:` args, but not the job's own token, so a script that echoed `$WM_TOKEN` wrote it verbatim into logs that are persisted to the database and, when configured, to object storage. `run_worker` now registers the token for the job it just pulled, alongside the existing `register_running_job` call, so it is redacted like any other registered secret. That makes every job carry at least one registered value, where before the per-batch mask snapshot was skipped entirely for the majority of jobs that touched no secret. Cache the compiled Aho-Corasick automaton per job and invalidate it when a new secret is registered, so a chatty job no longer rebuilds it once per log batch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(nativets): mask secrets in the in-process log path NativeTS hands `console.log` output to a task that drains a channel into `append_logs`, so it never reaches the masking in `handle_child::write_lines` and a script logging `$WM_TOKEN` persisted the raw JWT. That drain can still be flushing after the job is unregistered, so a plain per-line `snapshot` would leave the tail unmasked. `JobMasker` keeps the last masks it saw for exactly that window, and refreshes while the job is alive so secrets fetched mid-run are covered too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(nativets): seed the job masker at construction A `JobMasker` that only looked up its masks on the first `mask` call had the same hole at the head of the log that its retention closes at the tail: if the drain task's first productive poll landed after the job was unregistered, the registry was already gone and every line was written raw. `new` now takes the snapshot, and its callers construct it from the job's own execution while the job is still registered. Also cover the nativets sink with an integration test, gated on `deno_core` the way the CI test build is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(nativets): mask on the producing side of the log channel Masking as the drain task wrote to `append_logs` left two holes, because that task is detached and outlives the job: a secret registered mid-run could still be queued when the job was unregistered and would then be written raw, and the `windmill:job_log` tracing emission that EE forwards job logs on never went through the mask at all. Mask where the line is produced instead. That loop is joined before the job completes, so the job's secrets are always still registered, and one call now covers both the tracing mirror and the channel. The result stream keeps reading the raw text, the way `handle_child` keeps its raw `line` for results. `JobMasker` is no longer load-bearing for the post-unregistration window, so it is documented for what it now does: keep the security notice to once per set of secrets for a sink that masks line by line. Also drop the nativets test's tag override — `DEFAULT_TAGS` does advertise `nativets`, so the comment justifying it was wrong — and pin the automaton cache invalidation, whose failure mode is an unmasked secret. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(worker): hold the log-masking lifecycle at the job boundary Registering the job around the poller's call left every other way of running a job uncovered: the interactive worker shell and inline AI agent tools both call `handle_queued_job` directly, and a script logging `$WM_TOKEN` from either persisted the live credential. Register from inside `handle_queued_job` instead, under a drop guard, so each path is covered by construction rather than by remembering to add a call. Nothing is lost by unregistering earlier: the writes that follow go through `append_logs`, which never consulted the registry. In nativets, decide the stream/log routing before masking. `MaskSnapshot`'s notice is one-shot, so a secret-bearing `WM_STREAM:` chunk used to spend it on text that is then discarded, leaving later redactions in `job_logs` unexplained. Restore the masker's post-unregistration test: the memory-limit path never joins the producing loop, so that fallback is still load-bearing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * docs: correct the nativets masking comments The producer loop is not joined on the memory-limit path, so it does not "always" run while the job is registered — say normally, which is what `JobMasker`'s fallback is there for. Name the reason a stream chunk stays raw everywhere it goes, including the tracing mirror: it is result data that no log sink persists, so masking it would be masking a result. State the masker test's invariant without asserting a mechanism behind it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(masking): keep the security notice on a line of its own `mask` appended the notice as a newline plus the notice text, which assumes the caller hands it a bare log line. nativets hands it a chunk that already ends in a newline, and its sink concatenates chunks verbatim, so the notice arrived after a blank line and the next log line was welded onto the end of it. Emit the notice as its own line for either shape. `handle_child` is unaffected: its input never ends in a newline, so it keeps the original path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 --------- Co-authored-by: Claude Opus 5 (1M context) --- backend/tests/job_token_log_masking.rs | 108 +++++++++ .../src/sensitive_log_masks.rs | 221 ++++++++++++++++-- backend/windmill-runtime-nativets/src/lib.rs | 23 +- backend/windmill-worker/src/worker.rs | 30 ++- 4 files changed, 350 insertions(+), 32 deletions(-) create mode 100644 backend/tests/job_token_log_masking.rs diff --git a/backend/tests/job_token_log_masking.rs b/backend/tests/job_token_log_masking.rs new file mode 100644 index 0000000000..5d7de27218 --- /dev/null +++ b/backend/tests/job_token_log_masking.rs @@ -0,0 +1,108 @@ +/* + * The job's own token (`$WM_TOKEN`) stays valid well past the job it was minted + * for, and job logs are persisted to `job_logs` and optionally to object storage, + * so a script that echoes the token would otherwise park a live credential in + * durable storage. `run_worker` registers the token with `sensitive_log_masks` + * for the job it pulled; this pins that the persisted log carries the masked form. + */ + +use sqlx::{Pool, Postgres}; +use windmill_common::{ + jobs::{JobPayload, RawCode}, + scripts::ScriptLang, +}; +use windmill_test_utils::*; + +/// Prefix of a serialized job token: `jwt_` plus the base64 of a JWT header. +/// The masked form keeps only `jwt` + the last three characters, so it never matches. +const RAW_TOKEN_PREFIX: &str = "jwt_ey"; + +#[sqlx::test(fixtures("base"))] +async fn test_job_token_masked_in_persisted_logs(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let job = RunJob::from(JobPayload::Code(RawCode { + hash: None, + content: "echo \"running with --token $WM_TOKEN\"".to_string(), + path: None, + lock: None, + language: ScriptLang::Bash, + cache_ttl: None, + cache_ignore_s3_path: None, + dedicated_worker: None, + concurrency_settings: windmill_common::runnable_settings::ConcurrencySettings::default() + .into(), + debouncing_settings: windmill_common::runnable_settings::DebouncingSettings::default(), + modules: None, + tag: None, + })) + .run_until_complete(&db, false, port) + .await; + assert!(job.success, "job should have succeeded"); + + let logs = + sqlx::query_scalar::<_, Option>("SELECT logs FROM job_logs WHERE job_id = $1") + .bind(job.id) + .fetch_one(&db) + .await? + .unwrap_or_default(); + + assert!( + !logs.contains(RAW_TOKEN_PREFIX), + "an unmasked job token reached the persisted logs: {logs}" + ); + assert!( + logs.contains("secret value was masked"), + "expected the masking notice in logs: {logs}" + ); + Ok(()) +} + +/// nativets runs V8 in-process and persists `console.log` output through its own +/// channel, so it is masked by a different mechanism than the bash case above and +/// needs its own guard. +#[cfg(feature = "deno_core")] +#[sqlx::test(fixtures("base"))] +async fn test_job_token_masked_in_nativets_logs(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let job = RunJob::from(JobPayload::Code(RawCode { + hash: None, + content: "export async function main() {\n console.log('running with --token ' + process.env.WM_TOKEN);\n return 'ok';\n}".to_string(), + path: None, + lock: None, + language: ScriptLang::Nativets, + cache_ttl: None, + cache_ignore_s3_path: None, + dedicated_worker: None, + concurrency_settings: windmill_common::runnable_settings::ConcurrencySettings::default() + .into(), + debouncing_settings: windmill_common::runnable_settings::DebouncingSettings::default(), + modules: None, + tag: None, + })) + .run_until_complete(&db, false, port) + .await; + assert!(job.success, "job should have succeeded"); + + let logs = + sqlx::query_scalar::<_, Option>("SELECT logs FROM job_logs WHERE job_id = $1") + .bind(job.id) + .fetch_one(&db) + .await? + .unwrap_or_default(); + + assert!( + !logs.contains(RAW_TOKEN_PREFIX), + "an unmasked job token reached the persisted logs: {logs}" + ); + assert!( + logs.contains("secret value was masked"), + "expected the masking notice in logs: {logs}" + ); + Ok(()) +} diff --git a/backend/windmill-common/src/sensitive_log_masks.rs b/backend/windmill-common/src/sensitive_log_masks.rs index b6f6262b77..c999297ec0 100644 --- a/backend/windmill-common/src/sensitive_log_masks.rs +++ b/backend/windmill-common/src/sensitive_log_masks.rs @@ -10,7 +10,7 @@ use std::borrow::Cow; use std::collections::{HashMap, HashSet}; -use std::sync::RwLock; +use std::sync::{Arc, RwLock}; use uuid::Uuid; /// Minimum length for a secret to be registered for masking. @@ -20,9 +20,27 @@ const MIN_SECRET_LENGTH: usize = 8; const MASKED_NOTICE: &str = "[windmill] secret value was masked for security reasons, use string transformations to display full value"; +/// The secrets registered for one job, plus the automaton compiled from them. +#[derive(Default)] +struct JobMasks { + secrets: HashSet, + /// Built on the first `snapshot` after a change and shared by every later + /// snapshot. Every job registers at least its own token, so without this + /// cache each log batch of each job would rebuild the automaton. + compiled: Option>, +} + +/// Aho-Corasick automaton for O(m) multi-pattern matching in a single pass, +/// regardless of the number of secrets registered, with the replacement +/// strings indexed to match the automaton's pattern order. +struct CompiledMasks { + ac: aho_corasick::AhoCorasick, + replacements: Vec, +} + lazy_static::lazy_static! { - /// Map of job_id -> set of secret values that should be masked in that job's logs. - static ref SENSITIVE_MASKS: RwLock>> = + /// Map of job_id -> secret values that should be masked in that job's logs. + static ref SENSITIVE_MASKS: RwLock> = RwLock::new(HashMap::new()); /// Set of currently running job IDs on this worker process. @@ -32,13 +50,8 @@ lazy_static::lazy_static! { } /// A lock-free snapshot of secrets for a job, taken once per log batch. -/// Uses Aho-Corasick for O(m) multi-pattern matching in a single pass, -/// regardless of the number of secrets registered. pub struct MaskSnapshot { - /// Aho-Corasick automaton for fast matching. - ac: aho_corasick::AhoCorasick, - /// Replacement strings, indexed to match the automaton's pattern order. - replacements: Vec, + compiled: Arc, /// Whether the security notice has already been appended for this snapshot. /// Tracked locally to avoid a global write lock on every masked line. notice_shown: std::cell::Cell, @@ -53,34 +66,104 @@ impl MaskSnapshot { } // Single-pass check + replace using the pre-built automaton - if !self.ac.is_match(text) { + if !self.compiled.ac.is_match(text) { return Cow::Borrowed(text); } - let mut result = self.ac.replace_all(text, &self.replacements); + let mut result = self + .compiled + .ac + .replace_all(text, &self.compiled.replacements); - // Append the notice only once per snapshot (i.e. per batch) + // Append the notice only once per snapshot (i.e. per batch), as its own line. + // Callers pass either a bare line (`handle_child`) or a chunk that already ends + // in a newline (nativets), and the sinks concatenate what they get verbatim: + // assuming either shape welds the notice onto a neighbouring line. if !self.notice_shown.get() { self.notice_shown.set(true); - result.push('\n'); - result.push_str(MASKED_NOTICE); + if result.ends_with('\n') { + result.push_str(MASKED_NOTICE); + result.push('\n'); + } else { + result.push('\n'); + result.push_str(MASKED_NOTICE); + } } Cow::Owned(result) } } +/// A masker for sinks that mask line by line rather than in batches, like nativets +/// masking each `console.log` chunk as V8 produces it. `snapshot` per line would +/// re-arm the security notice on every one; this keeps it to once per distinct set +/// of secrets while still picking up secrets registered mid-run. +/// +/// Masks by job id alone — the caller is the one that knows the text it passes +/// belongs to that job. +pub struct JobMasker { + job_id: Uuid, + snapshot: Option, +} + +impl JobMasker { + pub fn new(job_id: Uuid) -> Self { + JobMasker { job_id, snapshot: snapshot(&job_id) } + } + + /// Mask every secret registered for the job. Returns `Cow::Borrowed` when no match. + /// Falls back to the masks it last saw once the job is unregistered, so a sink + /// still draining past the end of a run does not start emitting secrets. + pub fn mask<'a>(&mut self, text: &'a str) -> Cow<'a, str> { + if let Some(fresh) = snapshot(&self.job_id) { + // Replacing an equivalent snapshot would re-arm the notice, so only take + // one built from a secret set we have not seen. + let unchanged = self + .snapshot + .as_ref() + .is_some_and(|cur| Arc::ptr_eq(&cur.compiled, &fresh.compiled)); + if !unchanged { + self.snapshot = Some(fresh); + } + } + match self.snapshot.as_ref() { + Some(snapshot) => snapshot.mask(text), + None => Cow::Borrowed(text), + } + } +} + /// Take a snapshot of the current secrets for a job. Returns `None` if no secrets /// are registered (the caller can then skip masking entirely for the whole batch). /// /// Call this once per log batch in `write_lines`, not per line. pub fn snapshot(job_id: &Uuid) -> Option { - let masks = SENSITIVE_MASKS.read().unwrap_or_else(|e| e.into_inner()); - let secrets = masks.get(job_id)?; - if secrets.is_empty() { - return None; + { + let masks = SENSITIVE_MASKS.read().unwrap_or_else(|e| e.into_inner()); + let job = masks.get(job_id)?; + if job.secrets.is_empty() { + return None; + } + if let Some(compiled) = job.compiled.as_ref() { + return Some(MaskSnapshot { + compiled: compiled.clone(), + notice_shown: std::cell::Cell::new(false), + }); + } } + let mut masks = SENSITIVE_MASKS.write().unwrap_or_else(|e| e.into_inner()); + let job = masks.get_mut(job_id)?; + if job.secrets.is_empty() { + return None; + } + let compiled = job + .compiled + .get_or_insert_with(|| Arc::new(compile(&job.secrets))); + Some(MaskSnapshot { compiled: compiled.clone(), notice_shown: std::cell::Cell::new(false) }) +} + +fn compile(secrets: &HashSet) -> CompiledMasks { // Sort longest-first so longer secrets are matched before shorter substrings let mut sorted: Vec<&String> = secrets.iter().collect(); sorted.sort_by(|a, b| b.len().cmp(&a.len())); @@ -106,7 +189,7 @@ pub fn snapshot(job_id: &Uuid) -> Option { .build(sorted.iter().map(|s| s.as_str())) .expect("failed to build aho-corasick automaton"); - Some(MaskSnapshot { ac, replacements, notice_shown: std::cell::Cell::new(false) }) + CompiledMasks { ac, replacements } } /// Register a job as currently running. Call this before `handle_queued_job`. @@ -148,20 +231,110 @@ pub fn register_secret_for_all_running_jobs(secret: &str) { let mut masks = SENSITIVE_MASKS.write().unwrap_or_else(|e| e.into_inner()); for job_id in job_ids { - if let Some(set) = masks.get_mut(&job_id) { - set.insert(secret.to_string()); + if let Some(job) = masks.get_mut(&job_id) { + if job.secrets.insert(secret.to_string()) { + job.compiled = None; + } } } } /// Register a secret value for a specific job. -/// Used for `$encrypted:` args where we know the job ID. +/// Used for the job's own token and for `$encrypted:` args, where we know the job ID. pub fn register_secret_for_job(job_id: Uuid, secret: &str) { if secret.len() < MIN_SECRET_LENGTH { return; } let mut masks = SENSITIVE_MASKS.write().unwrap_or_else(|e| e.into_inner()); - if let Some(set) = masks.get_mut(&job_id) { - set.insert(secret.to_string()); + if let Some(job) = masks.get_mut(&job_id) { + if job.secrets.insert(secret.to_string()) { + job.compiled = None; + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The compiled automaton is cached per job, so a secret registered after the + /// first snapshot only gets masked if the cache is invalidated. + #[test] + fn snapshot_rebuilds_after_a_new_secret_is_registered() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "firstsecretvalue"); + let _ = snapshot(&job_id) + .expect("secret registered") + .mask("firstsecretvalue"); + + register_secret_for_job(job_id, "secondsecretvalue"); + + let snap = snapshot(&job_id).expect("secrets registered"); + let masked = snap.mask("firstsecretvalue then secondsecretvalue"); + assert!(!masked.contains("firstsecretvalue"), "{masked}"); + assert!(!masked.contains("secondsecretvalue"), "{masked}"); + unregister_running_job(job_id); + } + + /// A line-by-line sink must not repeat the notice on every line, and must still + /// pick up a secret registered after the masker was built. + #[test] + fn job_masker_notices_once_per_secret_set() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "firstsecretvalue"); + let mut masker = JobMasker::new(job_id); + + let first = masker.mask("saw firstsecretvalue").into_owned(); + assert!(!first.contains("firstsecretvalue"), "{first}"); + assert!(first.contains(MASKED_NOTICE), "{first}"); + + let second = masker.mask("saw firstsecretvalue again").into_owned(); + assert!(!second.contains("firstsecretvalue"), "{second}"); + assert!(!second.contains(MASKED_NOTICE), "{second}"); + + register_secret_for_job(job_id, "secondsecretvalue"); + let third = masker.mask("saw secondsecretvalue").into_owned(); + assert!(!third.contains("secondsecretvalue"), "{third}"); + unregister_running_job(job_id); + } + + /// Unregistration must not turn masking off under a sink that is still emitting: + /// the masker keeps working off the masks it last saw rather than going quiet. + #[test] + fn job_masker_masks_after_the_job_is_unregistered() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "supersecretvalue"); + let mut masker = JobMasker::new(job_id); + + unregister_running_job(job_id); + + let masked = masker.mask("logged supersecretvalue here"); + assert!(!masked.contains("supersecretvalue"), "{masked}"); + } + + /// The notice has to end up on a line of its own for both shapes callers pass: + /// a bare line (`handle_child`) and a newline-terminated chunk (nativets). The + /// sinks concatenate what they are given verbatim, so getting this wrong welds + /// the notice onto whichever line follows it. + #[test] + fn notice_lands_on_its_own_line_for_both_caller_shapes() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "supersecretvalue"); + + let line = snapshot(&job_id) + .expect("secret registered") + .mask("tok supersecretvalue"); + assert_eq!(line, format!("tok s*****e\n{MASKED_NOTICE}")); + + let chunk = snapshot(&job_id) + .expect("secret registered") + .mask("tok supersecretvalue\n"); + assert_eq!(chunk, format!("tok s*****e\n{MASKED_NOTICE}\n")); + + unregister_running_job(job_id); } } diff --git a/backend/windmill-runtime-nativets/src/lib.rs b/backend/windmill-runtime-nativets/src/lib.rs index d44213d3bc..750bf7ebac 100644 --- a/backend/windmill-runtime-nativets/src/lib.rs +++ b/backend/windmill-runtime-nativets/src/lib.rs @@ -838,6 +838,11 @@ pub async fn eval_fetch_timeout( } } let w_id_for_tracing = w_id_for_tracing; + // nativets delivers logs in-process, so they never reach the masking in + // `handle_child::write_lines` and a `console.log` of `$WM_TOKEN` would be + // persisted verbatim. Mask here rather than in the detached task draining into + // `append_logs`: this loop normally runs while the job is still registered. + let mut masker = windmill_common::sensitive_log_masks::JobMasker::new(job_id); let handle = tokio::spawn(async move { let mut result_stream = String::new(); let mut is_stream = false; @@ -845,10 +850,20 @@ pub async fn eval_fetch_timeout( use windmill_common::result_stream::extract_stream_from_logs; use windmill_common::tracing_init::{OTEL_JOB_LOGS, OTEL_PREFIX}; + let stream = extract_stream_from_logs(&log.trim_end_matches("\n")); + + // A stream chunk is result data, not a log line — it never reaches + // `job_logs`, and `merge_result_stream` can make it the job's result — + // so it stays raw wherever it goes, here and in the mirror below. + // Deliberately unlike `handle_child`, which streams the masked text. + // Routed before masking because the notice is one-shot: spent on a chunk + // no sink persists, a later redaction in `job_logs` would go unexplained. + let logged = stream.is_none().then(|| masker.mask(&log).into_owned()); + // Mirror `process_streaming_log_lines` (EE) + the OTEL_JOB_LOGS // hook from handle_child.rs, neither of which runs for nativets // since nativets delivers logs in-process via the log channel. - for line in log.lines() { + for line in logged.as_deref().unwrap_or(&log).lines() { tracing::info!( target: "windmill:job_log", job_id = ?job_id, @@ -862,7 +877,7 @@ pub async fn eval_fetch_timeout( } } - if let Some(stream) = extract_stream_from_logs(&log.trim_end_matches("\n")) { + if let Some(stream) = stream { if !is_stream { is_stream = true; if let Some(ref f) = stream_notifier_update { @@ -874,8 +889,8 @@ pub async fn eval_fetch_timeout( if let Err(e) = result_stream_sender.send(stream) { tracing::error!("failed to send result stream: {e}"); } - } else { - if let Err(e) = append_logs_sender.send(log) { + } else if let Some(logged) = logged { + if let Err(e) = append_logs_sender.send(logged) { tracing::error!("failed to send log: {e}"); } } diff --git a/backend/windmill-worker/src/worker.rs b/backend/windmill-worker/src/worker.rs index 56b4b26fb0..55a698a9dd 100644 --- a/backend/windmill-worker/src/worker.rs +++ b/backend/windmill-worker/src/worker.rs @@ -3859,8 +3859,6 @@ pub async fn run_worker( let arc_job = Arc::new(job); - windmill_common::sensitive_log_masks::register_running_job(arc_job.id); - let span = create_span_with_name(&arc_job, &worker_name, Some(hostname), "job"); let log_ctx = log_context_for_job(&arc_job, &worker_name, Some(hostname)); @@ -3976,8 +3974,6 @@ pub async fn run_worker( _ => {} } - windmill_common::sensitive_log_masks::unregister_running_job(job_id); - #[cfg(feature = "prometheus")] if let Some(duration) = _timer.map(|x| x.stop_and_record()) { register_metric( @@ -4512,6 +4508,30 @@ async fn detect_and_store_runtime_assets_from_job_args( } } +/// Holds a job's entry in the log-masking registry for as long as it executes, so +/// that secrets it fetches can be registered against it, and masks the job's own +/// token from the start: `$WM_TOKEN` stays valid well past the run, and a script +/// that echoes it would otherwise leave a live credential in the persisted logs. +/// +/// Lives here rather than at the call sites so that every way of running a job — +/// the poller, the interactive worker shell, an inline AI agent tool — is covered +/// by construction. +struct RunningJobMasks(Uuid); + +impl RunningJobMasks { + fn register(job_id: Uuid, token: &str) -> Self { + windmill_common::sensitive_log_masks::register_running_job(job_id); + windmill_common::sensitive_log_masks::register_secret_for_job(job_id, token); + RunningJobMasks(job_id) + } +} + +impl Drop for RunningJobMasks { + fn drop(&mut self) { + windmill_common::sensitive_log_masks::unregister_running_job(self.0); + } +} + pub async fn handle_queued_job( job: Arc, raw_code: Option, @@ -4533,6 +4553,8 @@ pub async fn handle_queued_job( flow_runners: Option>, #[cfg(feature = "benchmark")] _bench: &mut BenchmarkIter, ) -> windmill_common::error::Result { + let _masks = RunningJobMasks::register(job.id, &client.token); + if job.canceled_by.is_some() { return Err(Error::JsonErr(canceled_job_to_result(&job))); } From 9d335de87a4dbaa51038d55afe8d980761dcdfaf Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 18 Sep 2026 15:15:35 +0200 Subject: [PATCH 21/34] feat: add a workspace toggle that adds its admins and developers to new forks (#11215) * feat: add a workspace toggle that adds its admins and developers to new forks Co-Authored-By: Claude Opus 5 * fix: add members copied into a fork as manual members, not instance-group ones Co-Authored-By: Claude Opus 5 * style: keep the fork members copy comment at the query and drop the raw spacer Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- ...83dce0c9986f3847227c2f66daa84d4109d7d.json | 15 ---- ...ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json | 16 ++++ ...56ee979e4c392970278304e80368b770bb7b4.json | 22 ++++++ ...cc1011a49fc8d8ad046ac572e81cf8938995.json} | 4 +- ...d21d12fa1207b58bfc46249c250cdcdb5363.json} | 12 ++- ...78f7e6dfa9185056f7731547ff05b8176b271.json | 15 ++++ ...bf1ebed0192be267983e8fbd18f79997e6142.json | 15 ---- ...1b93db0406aaf278bccba07be00e9f937e2a.json} | 14 +++- ...52fb2b4cf7983049d2c490940df360c7e2b30.json | 15 ++++ ...dd_admins_and_developers_to_forks.down.sql | 1 + ..._add_admins_and_developers_to_forks.up.sql | 1 + backend/summarized_schema.txt | 2 +- .../tests/fork_members.rs | 75 ++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 77 ++++++++++++++++++- .../src/workspaces_extra.rs | 2 +- backend/windmill-api/openapi.yaml | 40 ++++++++++ .../settings/WorkspaceUserSettings.svelte | 36 +++++++++ .../CreateWorkspaceInner.svelte | 19 +++++ 18 files changed, 336 insertions(+), 45 deletions(-) delete mode 100644 backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json create mode 100644 backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json create mode 100644 backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json rename backend/.sqlx/{query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json => query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json} (63%) rename backend/.sqlx/{query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json => query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json} (93%) create mode 100644 backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json delete mode 100644 backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json rename backend/.sqlx/{query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json => query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json} (78%) create mode 100644 backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json create mode 100644 backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql create mode 100644 backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql create mode 100644 backend/windmill-api-integration-tests/tests/fork_members.rs diff --git a/backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json b/backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json deleted file mode 100644 index b9cbc6c382..0000000000 --- a/backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $2", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Varchar", - "Text" - ] - }, - "nullable": [] - }, - "hash": "1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d" -} diff --git a/backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json b/backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json new file mode 100644 index 0000000000..4b96519a06 --- /dev/null +++ b/backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO usr (workspace_id, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via)\n SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account,\n CASE WHEN $3 THEN NULL ELSE added_via END\n FROM usr WHERE workspace_id = $2\n AND (NOT $3 OR (NOT operator AND NOT disabled AND NOT is_service_account))\n ON CONFLICT DO NOTHING", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text", + "Bool" + ] + }, + "nullable": [] + }, + "hash": "2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f" +} diff --git a/backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json b/backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json new file mode 100644 index 0000000000..99b1a9472d --- /dev/null +++ b/backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "add_admins_and_developers_to_forks", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4" +} diff --git a/backend/.sqlx/query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json b/backend/.sqlx/query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json similarity index 63% rename from backend/.sqlx/query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json rename to backend/.sqlx/query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json index 0532a5d3a0..7cd917d623 100644 --- a/backend/.sqlx/query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json +++ b/backend/.sqlx/query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n UPDATE workspace_settings\n SET\n ai_config = source_ws.ai_config,\n large_file_storage = source_ws.large_file_storage,\n ducklake = source_ws.ducklake,\n dbt_warehouses = source_ws.dbt_warehouses,\n datatable = source_ws.datatable,\n git_app_installations = source_ws.git_app_installations\n FROM workspace_settings source_ws\n WHERE source_ws.workspace_id = $1\n AND workspace_settings.workspace_id = $2\n ", + "query": "\n UPDATE workspace_settings\n SET\n ai_config = source_ws.ai_config,\n large_file_storage = source_ws.large_file_storage,\n ducklake = source_ws.ducklake,\n dbt_warehouses = source_ws.dbt_warehouses,\n datatable = source_ws.datatable,\n git_app_installations = source_ws.git_app_installations,\n add_admins_and_developers_to_forks = source_ws.add_admins_and_developers_to_forks\n FROM workspace_settings source_ws\n WHERE source_ws.workspace_id = $1\n AND workspace_settings.workspace_id = $2\n ", "describe": { "columns": [], "parameters": { @@ -11,5 +11,5 @@ }, "nullable": [] }, - "hash": "a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a" + "hash": "5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995" } diff --git a/backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json b/backend/.sqlx/query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json similarity index 93% rename from backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json rename to backend/.sqlx/query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json index 01c0fd19af..7a822a8d6a 100644 --- a/backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json +++ b/backend/.sqlx/query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n workspace_id,\n slack_team_id,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n slack_name,\n slack_command_script,\n teams_command_script,\n slack_email,\n slack_oauth_client_id,\n slack_oauth_client_secret,\n customer_id,\n plan,\n webhook,\n ai_config,\n dbt_warehouses,\n large_file_storage,\n datatable,\n ducklake,\n git_sync,\n deploy_ui,\n default_app,\n default_scripts,\n mute_critical_alerts,\n color,\n operator_settings,\n git_app_installations,\n auto_invite,\n error_handler,\n success_handler,\n public_app_execution_limit_per_minute,\n error_handler_fallback_to_instance_alerts,\n guest_access_enabled,\n guest_jwt_public_key,\n guest_jwt_jwks_url\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", + "query": "\n SELECT\n workspace_id,\n slack_team_id,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n slack_name,\n slack_command_script,\n teams_command_script,\n slack_email,\n slack_oauth_client_id,\n slack_oauth_client_secret,\n customer_id,\n plan,\n webhook,\n ai_config,\n dbt_warehouses,\n large_file_storage,\n datatable,\n ducklake,\n git_sync,\n deploy_ui,\n default_app,\n default_scripts,\n mute_critical_alerts,\n color,\n operator_settings,\n git_app_installations,\n auto_invite,\n error_handler,\n success_handler,\n public_app_execution_limit_per_minute,\n error_handler_fallback_to_instance_alerts,\n guest_access_enabled,\n guest_jwt_public_key,\n guest_jwt_jwks_url,\n add_admins_and_developers_to_forks\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", "describe": { "columns": [ { @@ -177,6 +177,11 @@ "ordinal": 34, "name": "guest_jwt_jwks_url", "type_info": "Text" + }, + { + "ordinal": 35, + "name": "add_admins_and_developers_to_forks", + "type_info": "Bool" } ], "parameters": { @@ -219,8 +224,9 @@ false, false, true, - true + true, + false ] }, - "hash": "dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99" + "hash": "8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363" } diff --git a/backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json b/backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json new file mode 100644 index 0000000000..9c4d587c01 --- /dev/null +++ b/backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings SET add_admins_and_developers_to_forks = $1 WHERE workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Bool", + "Text" + ] + }, + "nullable": [] + }, + "hash": "9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271" +} diff --git a/backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json b/backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json deleted file mode 100644 index 94cf77ebc5..0000000000 --- a/backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "INSERT INTO usr (workspace_id, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via)\n SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via\n FROM usr WHERE workspace_id = $2\n ON CONFLICT DO NOTHING", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Varchar", - "Text" - ] - }, - "nullable": [] - }, - "hash": "b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142" -} diff --git a/backend/.sqlx/query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json b/backend/.sqlx/query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json similarity index 78% rename from backend/.sqlx/query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json rename to backend/.sqlx/query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json index 1c247ad5b5..7462224850 100644 --- a/backend/.sqlx/query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json +++ b/backend/.sqlx/query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n workspace_id,\n slack_team_id,\n slack_name,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n mute_critical_alerts,\n guest_access_enabled,\n deploy_ui,\n large_file_storage,\n datatable\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", + "query": "\n SELECT\n workspace_id,\n slack_team_id,\n slack_name,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n mute_critical_alerts,\n guest_access_enabled,\n add_admins_and_developers_to_forks,\n deploy_ui,\n large_file_storage,\n datatable\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", "describe": { "columns": [ { @@ -45,16 +45,21 @@ }, { "ordinal": 8, + "name": "add_admins_and_developers_to_forks", + "type_info": "Bool" + }, + { + "ordinal": 9, "name": "deploy_ui", "type_info": "Jsonb" }, { - "ordinal": 9, + "ordinal": 10, "name": "large_file_storage", "type_info": "Jsonb" }, { - "ordinal": 10, + "ordinal": 11, "name": "datatable", "type_info": "Jsonb" } @@ -73,10 +78,11 @@ true, true, false, + false, true, true, true ] }, - "hash": "ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447" + "hash": "e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a" } diff --git a/backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json b/backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json new file mode 100644 index 0000000000..ec641bf32d --- /dev/null +++ b/backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text" + ] + }, + "nullable": [] + }, + "hash": "eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30" +} diff --git a/backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql new file mode 100644 index 0000000000..4e8ac47c69 --- /dev/null +++ b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql @@ -0,0 +1 @@ +ALTER TABLE workspace_settings DROP COLUMN add_admins_and_developers_to_forks; diff --git a/backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql new file mode 100644 index 0000000000..bb3502d362 --- /dev/null +++ b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql @@ -0,0 +1 @@ +ALTER TABLE workspace_settings ADD COLUMN add_admins_and_developers_to_forks BOOLEAN NOT NULL DEFAULT false; diff --git a/backend/summarized_schema.txt b/backend/summarized_schema.txt index e27cb2893c..47f9faca68 100644 --- a/backend/summarized_schema.txt +++ b/backend/summarized_schema.txt @@ -234,7 +234,7 @@ workspace_protection_rule: workspace_id(char), name(char), rules(int), bypass_gr FK: (workspace_id) -> workspace(id) workspace_runnable_dependencies: flow_path(char), runnable_path(char), script_hash(bigint), runnable_is_flow(bool), workspace_id(char), app_path(char), id(bigint), runnable_is_agent(bool) FK: (app_path, workspace_id) -> app(path, workspace_id) | (flow_path, workspace_id) -> flow(path, workspace_id) -workspace_settings: workspace_id(char), slack_team_id(char), slack_name(char), slack_command_script(char), slack_email(char), customer_id(char), plan(char), webhook(text), ai_config(jsonb), large_file_storage(jsonb), git_sync(jsonb), default_app(char), default_scripts(jsonb), deploy_ui(jsonb), mute_critical_alerts(bool), color(char), operator_settings(jsonb), teams_command_script(text), teams_team_id(text), teams_team_name(text), git_app_installations(jsonb), ducklake(jsonb), slack_oauth_client_id(char), slack_oauth_client_secret(char), datatable(jsonb), teams_team_guid(text), auto_invite(jsonb), error_handler(jsonb), success_handler(jsonb), public_app_execution_limit_per_minute(int), dbt_warehouses(jsonb), guest_access_enabled(bool), guest_jwt_public_key(text), guest_jwt_jwks_url(text), ai_sessions_backup_generation(int) +workspace_settings: workspace_id(char), slack_team_id(char), slack_name(char), slack_command_script(char), slack_email(char), customer_id(char), plan(char), webhook(text), ai_config(jsonb), large_file_storage(jsonb), git_sync(jsonb), default_app(char), default_scripts(jsonb), deploy_ui(jsonb), mute_critical_alerts(bool), color(char), operator_settings(jsonb), teams_command_script(text), teams_team_id(text), teams_team_name(text), git_app_installations(jsonb), ducklake(jsonb), slack_oauth_client_id(char), slack_oauth_client_secret(char), datatable(jsonb), teams_team_guid(text), auto_invite(jsonb), error_handler(jsonb), success_handler(jsonb), public_app_execution_limit_per_minute(int), dbt_warehouses(jsonb), guest_access_enabled(bool), guest_jwt_public_key(text), guest_jwt_jwks_url(text), ai_sessions_backup_generation(int), add_admins_and_developers_to_forks(bool) FK: (workspace_id) -> workspace(id) zombie_job_counter: job_id(uuid), counter(int) FK: (job_id) -> v2_job(id) diff --git a/backend/windmill-api-integration-tests/tests/fork_members.rs b/backend/windmill-api-integration-tests/tests/fork_members.rs new file mode 100644 index 0000000000..5f4baced1f --- /dev/null +++ b/backend/windmill-api-integration-tests/tests/fork_members.rs @@ -0,0 +1,75 @@ +use serde_json::json; +use sqlx::{Pool, Postgres}; + +use windmill_test_utils::*; + +/// With `add_admins_and_developers_to_forks` on, a fork starts with the parent's admins and +/// developers at their parent role, even when a developer forks it; operators are left out. The +/// copies are manual members: a parent membership that came from an instance group must not carry +/// that provenance into a fork that does not configure the group. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_fork_adds_parent_admins_and_developers(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let base_url = format!( + "http://localhost:{}/api/w/test-workspace/workspaces", + server.addr.port() + ); + let client = reqwest::Client::new(); + + sqlx::query( + "UPDATE usr SET operator = true WHERE workspace_id = 'test-workspace' AND username = 'test-user-3'", + ) + .execute(&db) + .await?; + sqlx::query( + "INSERT INTO usr (workspace_id, email, username, is_admin, added_via) + VALUES ('test-workspace', 'test4@windmill.dev', 'test-user-4', false, + '{\"source\": \"instance_group\", \"group\": \"devs\"}')", + ) + .execute(&db) + .await?; + + let resp = client + .post(format!( + "{base_url}/edit_add_admins_and_developers_to_forks" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ "add_admins_and_developers_to_forks": true })) + .send() + .await?; + assert!( + resp.status().is_success(), + "enabling the setting: {}", + resp.text().await? + ); + + let resp = client + .post(format!("{base_url}/create_fork")) + .header("Authorization", "Bearer SECRET_TOKEN_2") + .json(&json!({ "id": "wm-fork-team", "name": "Team fork" })) + .send() + .await?; + assert!( + resp.status().is_success(), + "creating the fork: {}", + resp.text().await? + ); + + let members: Vec<(String, bool, bool)> = sqlx::query_as( + "SELECT username, is_admin, added_via IS NULL FROM usr + WHERE workspace_id = 'wm-fork-team' ORDER BY username", + ) + .fetch_all(&db) + .await?; + assert_eq!( + members, + vec![ + ("test-user".to_string(), true, true), + ("test-user-2".to_string(), false, true), + ("test-user-4".to_string(), false, true), + ] + ); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index aafba577b6..eb709c49c2 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -155,6 +155,10 @@ pub fn workspaced_service() -> Router { .route("/edit_deploy_ui_config", post(edit_deploy_ui_config)) .route("/edit_default_app", post(edit_default_app)) .route("/edit_guest_access", post(edit_guest_access)) + .route( + "/edit_add_admins_and_developers_to_forks", + post(edit_add_admins_and_developers_to_forks), + ) .route("/edit_guest_jwt_key", post(edit_guest_jwt_key)) .route("/guest_usage", get(get_guest_usage)) .route("/default_app", get(get_default_app)) @@ -338,6 +342,7 @@ pub struct WorkspaceSettings { pub guest_jwt_public_key: Option, #[serde(skip_serializing_if = "Option::is_none")] pub guest_jwt_jwks_url: Option, + pub add_admins_and_developers_to_forks: bool, } /// Subset of `WorkspaceSettings` that is safe to return to any workspace @@ -363,6 +368,8 @@ pub struct WorkspacePublicSettings { /// Not sensitive, and the app editor needs it to say whether the guest rung is /// live -- an app can be set to `guest` while the workspace has guests off. pub guest_access_enabled: bool, + /// Read by the fork dialog, which tells the forker who else the fork will include. + pub add_admins_and_developers_to_forks: bool, #[serde(skip_serializing_if = "Option::is_none")] pub deploy_ui: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -1126,7 +1133,8 @@ async fn get_settings( error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, - guest_jwt_jwks_url + guest_jwt_jwks_url, + add_admins_and_developers_to_forks FROM workspace_settings WHERE @@ -1168,6 +1176,7 @@ async fn get_public_settings( teams_team_guid, mute_critical_alerts, guest_access_enabled, + add_admins_and_developers_to_forks, deploy_ui, large_file_storage, datatable @@ -5052,6 +5061,47 @@ async fn edit_guest_access( )) } +#[derive(Deserialize)] +struct EditAddAdminsAndDevelopersToForks { + add_admins_and_developers_to_forks: bool, +} + +async fn edit_add_admins_and_developers_to_forks( + authed: ApiAuthed, + Extension(db): Extension, + Path(w_id): Path, + Json(EditAddAdminsAndDevelopersToForks { add_admins_and_developers_to_forks }): Json< + EditAddAdminsAndDevelopersToForks, + >, +) -> Result { + require_admin(authed.is_admin, &authed.username)?; + + let mut tx = db.begin().await?; + sqlx::query!( + "UPDATE workspace_settings SET add_admins_and_developers_to_forks = $1 WHERE workspace_id = $2", + add_admins_and_developers_to_forks, + &w_id + ) + .execute(&mut *tx) + .await?; + + audit_log( + &mut *tx, + &authed, + "workspaces.edit_add_admins_and_developers_to_forks", + ActionKind::Update, + &w_id, + Some(&add_admins_and_developers_to_forks.to_string()), + None, + ) + .await?; + tx.commit().await?; + + Ok(format!( + "Adding admins and developers to new forks set to {add_admins_and_developers_to_forks} for workspace {w_id}" + )) +} + #[derive(Deserialize)] struct EditGuestJwtKey { /// A PEM public key (RS or ES family), or a JWKS URL, at most one. Both empty clears the @@ -6712,7 +6762,8 @@ async fn update_workspace_settings( ducklake = source_ws.ducklake, dbt_warehouses = source_ws.dbt_warehouses, datatable = source_ws.datatable, - git_app_installations = source_ws.git_app_installations + git_app_installations = source_ws.git_app_installations, + add_admins_and_developers_to_forks = source_ws.add_admins_and_developers_to_forks FROM workspace_settings source_ws WHERE source_ws.workspace_id = $1 AND workspace_settings.workspace_id = $2 @@ -6853,14 +6904,21 @@ async fn copy_workspace_members( tx: &mut Transaction<'_, Postgres>, source_workspace_id: &str, target_workspace_id: &str, + admins_and_developers_only: bool, ) -> Result<()> { + // Admins and developers join as manual members: the fork does not inherit the source's + // instance-group config, so a copied `instance_group` provenance would let the fork's + // reconciliation delete them and their data. sqlx::query!( "INSERT INTO usr (workspace_id, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via) - SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via + SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account, + CASE WHEN $3 THEN NULL ELSE added_via END FROM usr WHERE workspace_id = $2 + AND (NOT $3 OR (NOT operator AND NOT disabled AND NOT is_service_account)) ON CONFLICT DO NOTHING", target_workspace_id, source_workspace_id, + admins_and_developers_only, ) .execute(&mut **tx) .await?; @@ -8651,8 +8709,19 @@ async fn create_workspace_fork( // intended. Dev creation is already admin-gated, so this is transitively admin-only too. Done before // the explicit creator insert below so the creator (a parent member) is copied with full metadata // (operator/role/is_service_account/added_via), not the bare row the insert alone would leave. + // Independently, the parent's admins can have every fork of it start with its admins and + // developers; the forker cannot opt out, since the point is that those admins can review it. if nw.copy_members && nw.is_dev_workspace { - copy_workspace_members(&mut tx, &parent_workspace_id, &forked_id).await?; + copy_workspace_members(&mut tx, &parent_workspace_id, &forked_id, false).await?; + } else if sqlx::query_scalar!( + "SELECT add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $1", + parent_workspace_id + ) + .fetch_optional(&mut *tx) + .await? + .unwrap_or(false) + { + copy_workspace_members(&mut tx, &parent_workspace_id, &forked_id, true).await?; } // Ensure the creator is a member of the fork even without copy_members (or if they aren't a parent diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 1cb5188e5b..45f82227f5 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -113,7 +113,7 @@ pub(crate) async fn change_workspace_id( // Duplicate workspace settings (keep copy in old workspace for reference) info!("Duplicating workspace_settings table"); sqlx::query!( - "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $2", + "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $2", &rw.new_id, &old_id ) diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index cba085a7e3..23390e0c67 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -4090,9 +4090,13 @@ paths: guest_access_enabled: type: boolean description: Whether this workspace admits guest sessions. An app's own `guest` execution mode is inert while this is false. + add_admins_and_developers_to_forks: + type: boolean + description: Whether every new fork of this workspace starts with its admins and developers as members, keeping their role. required: - workspace_id - guest_access_enabled + - add_admins_and_developers_to_forks /w/{workspace}/workspaces/get_settings: get: @@ -4183,6 +4187,9 @@ paths: guest_jwt_jwks_url: type: string description: JWKS URL a guest JWT (`jwt_guest_`) is verified against for this workspace. Mutually exclusive with `guest_jwt_public_key`. + add_admins_and_developers_to_forks: + type: boolean + description: Whether every new fork of this workspace starts with its admins and developers as members, keeping their role. /w/{workspace}/workspaces/get_deploy_to: get: @@ -6313,6 +6320,39 @@ paths: schema: type: string + /w/{workspace}/workspaces/edit_add_admins_and_developers_to_forks: + post: + summary: choose whether new forks of this workspace start with its admins and developers + description: >- + When on, every fork created from this workspace gets the workspace's admins and + developers as members, with the role they hold here; operators, disabled users and + service accounts are left out. The setting is copied into each fork, so forks of a + fork follow it too. Off by default. Workspace-admin gated. + operationId: editAddAdminsAndDevelopersToForks + tags: + - workspace + parameters: + - $ref: "#/components/parameters/WorkspaceId" + requestBody: + description: Whether new forks start with this workspace's admins and developers + required: true + content: + application/json: + schema: + type: object + properties: + add_admins_and_developers_to_forks: + type: boolean + required: + - add_admins_and_developers_to_forks + responses: + "200": + description: status + content: + text/plain: + schema: + type: string + /w/{workspace}/workspaces/edit_guest_jwt_key: post: summary: set the key guest JWTs are verified against for this workspace diff --git a/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte b/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte index 5c8af52683..449e9c7d31 100644 --- a/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte +++ b/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte @@ -46,6 +46,7 @@ let auto_invite_domain: string | undefined = $state() let operatorOnly: boolean | undefined = $state(undefined) let autoAdd: boolean | undefined = $state(false) + let addAdminsAndDevelopersToForks = $state(false) let nbDisplayed = $state(30) // Instance group auto-add settings @@ -122,6 +123,25 @@ autoAdd = autoInvite?.mode === 'add' autoAddInstanceGroups = autoInvite?.instance_groups || [] autoAddInstanceGroupsRoles = autoInvite?.instance_groups_roles || {} + addAdminsAndDevelopersToForks = settings.add_admins_and_developers_to_forks ?? false + } + + async function updateAddAdminsAndDevelopersToForks(enabled: boolean): Promise { + try { + await WorkspaceService.editAddAdminsAndDevelopersToForks({ + workspace: $workspaceStore!, + requestBody: { add_admins_and_developers_to_forks: enabled } + }) + sendUserToast( + enabled + ? 'New forks will start with the admins and developers of this workspace' + : 'New forks will start with their creator only' + ) + } catch (e) { + console.error('Failed to update the fork members setting:', e) + addAdminsAndDevelopersToForks = !enabled + sendUserToast(`Failed to update the fork members setting: ${e}`, true) + } } let getUsagePromise: CancelablePromise | undefined = undefined @@ -1067,6 +1087,22 @@
+
+ updateAddAdminsAndDevelopersToForks(e.detail)} + options={{ + right: 'Add admins and developers to new forks', + rightTooltip: + 'Admins and developers of this workspace join every new fork with the role they have here, so they can follow and review the work done in it. Forks of those forks follow the same setting.' + }} + /> +
+ {#if invites?.length > 0}
(isFork ? baseWorkspaceId : undefined), + async (ws, _prev, { signal }) => { + if (!ws) return undefined + const settings = await WorkspaceService.getPublicSettings({ workspace: ws }) + if (signal.aborted) throw new DOMException('superseded', 'AbortError') + return { ws, adds: settings.add_admins_and_developers_to_forks } + } + ) + let baseAddsAdminsAndDevelopers = $derived( + baseForkMembersResource.current?.ws === baseWorkspaceId && + !!baseForkMembersResource.current?.adds + ) // Ask the server whether a dev already exists: the caller may not be a member of this prod's dev, // so the client workspace list can't see it and would offer an invalid "create dev" action. const devWorkspaceResource = resource( @@ -929,6 +942,12 @@ disabled={createAsDevWorkspace} /> + {#if baseAddsAdminsAndDevelopers && !(createAsDevWorkspace && copyMembers)} + + {baseWorkspaceId} adds its admins and developers to every new fork, with the role they have + there. + + {/if} {/if} {#if isFork} Date: Fri, 18 Sep 2026 15:21:07 +0200 Subject: [PATCH 22/34] fix: re-encrypt git sync secrets on workspace key rotation (#11218) * fix: re-encrypt git sync credentials and webhook secrets on workspace key rotation Co-Authored-By: Claude Opus 5 * chore: point ee-repo-ref at the git sync key rotation companion Co-Authored-By: Claude Opus 5 * chore: update ee-repo-ref to bc3ef08c8e4233508c023e6ee847a3cd0b8be43b This commit updates the EE repository reference after PR #814 was merged in windmill-ee-private. Previous ee-repo-ref: 8121eac421c5d026f36e2edec140f3c79b23d2cd New ee-repo-ref: bc3ef08c8e4233508c023e6ee847a3cd0b8be43b Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 Co-authored-by: windmill-internal-app[bot] --- backend/ee-repo-ref.txt | 2 +- .../workspace_encryption_key_git_sync.rs | 61 +++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 86 ++++++++++++++++--- 3 files changed, 135 insertions(+), 14 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 3d9d49412a..3dfd92f8c2 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -f2fced19fcae81de7f6dac545010ce404c052e1b +bc3ef08c8e4233508c023e6ee847a3cd0b8be43b diff --git a/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs b/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs index 8edfd39d61..a22ad974c3 100644 --- a/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs +++ b/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs @@ -275,6 +275,67 @@ async fn test_encryption_key_rotation_dispatches_batched_git_sync( Ok(()) } +/// Stored repository tokens and webhook secrets are encrypted under the +/// workspace key but never synced, so a rotation has to carry them over even +/// when the caller skips re-encrypting variables. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_encryption_key_rotation_reencrypts_git_sync_secrets( + db: Pool, +) -> anyhow::Result<()> { + use windmill_common::variables::{build_crypt, crypt_from_key_with_suffix, decrypt, encrypt}; + initialize_tracing().await; + + create_folder(&db, "28103").await?; + create_git_repo_resource(&db).await?; + let sync_script_path = "f/28103/test_sync_script_git_secrets"; + create_sync_script(&db, sync_script_path).await?; + setup_git_sync_config(&db, sync_script_path).await?; + + let mc = build_crypt(&db, "test-workspace").await?; + sqlx::query( + r#" + UPDATE workspace_settings SET + git_credentials = jsonb_build_array(jsonb_build_object( + 'token', $1::text, 'repo_identity', 'https://gitlab.example.com/grp/proj')), + git_sync = jsonb_set(git_sync, '{repositories,0,auto_pull}', jsonb_build_object( + 'enabled', true, 'mode', 'webhook', 'webhook_id', 1, 'webhook_secret', $2::text)) + WHERE workspace_id = 'test-workspace' + "#, + ) + .bind(encrypt(&mc, "stored-token")) + .bind(encrypt(&mc, "hook-secret")) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + let new_key = "c".repeat(64); + let resp = authed(client().post(format!("{base}/encryption_key"))) + .json(&json!({"new_key": new_key, "skip_reencrypt": true})) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "set_encryption_key failed: {}", + resp.text().await? + ); + + let (token, secret): (String, String) = sqlx::query_as( + "SELECT git_credentials->0->>'token', git_sync#>>'{repositories,0,auto_pull,webhook_secret}' + FROM workspace_settings WHERE workspace_id = 'test-workspace'", + ) + .fetch_one(&db) + .await?; + let new_mc = crypt_from_key_with_suffix(&new_key, ""); + assert_eq!(decrypt(&new_mc, token)?, "stored-token"); + assert_eq!(decrypt(&new_mc, secret)?, "hook-secret"); + + Ok(()) +} + /// Regression test for the non-debouncing fallback: a workspace whose sync /// script predates hub version 28103 must still receive git-sync jobs for the /// encryption_key entry and every re-encrypted secret. Before the fallback was diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index eb709c49c2..879babff4f 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -38,7 +38,7 @@ use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE; use windmill_common::query_builders::{render_db_quoted_identifier, DbType}; use windmill_common::users::username_to_permissioned_as; use windmill_common::variables::{ - build_crypt, decrypt, encrypt, SECRET_SALT, WORKSPACE_CRYPT_CACHE, + crypt_from_key_with_suffix, decrypt, encrypt, WORKSPACE_CRYPT_CACHE, }; use windmill_common::worker::{to_raw_value, CLOUD_HOSTED}; use windmill_common::workspaces::GitRepositorySettings; @@ -5690,9 +5690,6 @@ async fn set_encryption_key( )); } - // Build the previous cipher before the transaction (reads from cache/pool) - let previous_encryption_key = build_crypt(&db, w_id.as_str()).await?; - let mut tx = db.begin().await?; // Under the row's lock, so two rotations racing serialize and each sees the key the @@ -5726,17 +5723,14 @@ async fn set_encryption_key( None }; + // From the keys read and written under the lock, never from `build_crypt`: its + // cache can still hold a key an earlier rotation replaced, and the git-sync + // secrets below are skipped rather than failed when they do not decrypt. + let previous_encryption_key = crypt_from_key_with_suffix(&previous_key, ""); + let new_encryption_key = crypt_from_key_with_suffix(&request.new_key, ""); + let mut reencrypted_secret_paths: Vec = Vec::new(); if !request.skip_reencrypt.unwrap_or(false) { - // Build the new cipher directly from the key string, since the transaction - // hasn't committed yet and build_crypt() would read the old key from the pool. - let crypt_key = if let Some(ref salt) = SECRET_SALT.as_ref() { - format!("{}{}", request.new_key, salt) - } else { - request.new_key.clone() - }; - let new_encryption_key = magic_crypt::new_magic_crypt!(crypt_key, 256); - let mut truncated_new_key = request.new_key.clone(); truncated_new_key.truncate(8); tracing::warn!( @@ -5776,6 +5770,14 @@ async fn set_encryption_key( } } + reencrypt_git_sync_secrets( + &mut tx, + &w_id, + &previous_encryption_key, + &new_encryption_key, + ) + .await?; + tx.commit().await?; // Invalidate the cache only after the transaction has committed @@ -5813,6 +5815,64 @@ async fn set_encryption_key( return Ok(()); } +/// Move the git-sync secrets the server keeps under the workspace key (stored +/// repository tokens, webhook secrets) to the new key. They are never synced, so +/// unlike variables they are still under the old key when the caller skips +/// re-encryption. +async fn reencrypt_git_sync_secrets( + conn: &mut sqlx::PgConnection, + w_id: &str, + old: &magic_crypt::MagicCrypt256, + new: &magic_crypt::MagicCrypt256, +) -> Result<()> { + let Some((mut credentials, mut git_sync)) = + sqlx::query_as::<_, (serde_json::Value, Option)>( + "SELECT git_credentials, git_sync FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + ) + .bind(w_id) + .fetch_optional(&mut *conn) + .await? + else { + return Ok(()); + }; + let reencrypt = |value: &mut serde_json::Value| { + let Some(ciphertext) = value.as_str() else { + return; + }; + match decrypt(old, ciphertext.to_string()) { + Ok(plain) => *value = serde_json::Value::String(encrypt(new, &plain)), + // Left by an earlier rotation and unrecoverable either way; failing here + // would block every later rotation of the workspace. + Err(e) => tracing::warn!( + "a git-sync secret of workspace {w_id} does not decrypt under its current key, leaving it as is: {e}" + ), + } + }; + for entry in credentials.as_array_mut().into_iter().flatten() { + if let Some(token) = entry.get_mut("token") { + reencrypt(token); + } + } + let repositories = git_sync + .as_mut() + .and_then(|g| g.get_mut("repositories")) + .and_then(|r| r.as_array_mut()); + for repo in repositories.into_iter().flatten() { + if let Some(secret) = repo.pointer_mut("/auto_pull/webhook_secret") { + reencrypt(secret); + } + } + sqlx::query( + "UPDATE workspace_settings SET git_credentials = $2, git_sync = $3 WHERE workspace_id = $1", + ) + .bind(w_id) + .bind(credentials) + .bind(git_sync) + .execute(&mut *conn) + .await?; + Ok(()) +} + #[derive(Serialize)] struct UsedTriggers { pub websocket_used: bool, From 37e493ae66ed5c000ecac492d60fc0fdf4bda71f Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 18 Sep 2026 15:24:00 +0200 Subject: [PATCH 23/34] feat: add an instance setting to refuse a token in MCP URLs (#11162) * feat: add an instance setting to refuse a token in MCP URLs MCP clients are commonly configured with the token in the URL (`/api/mcp/w/{workspace}/mcp?token=...`). A URL-borne credential ends up in browser history, proxy logs and referrers, so an instance can now turn that channel off with the `mcp_disable_token_query_param` global setting and leave the Authorization header as the only way in, which sends MCP clients through the OAuth flow the endpoints already advertise. The rejection is a middleware on both the workspaced and the gateway MCP mounts, layered outside everything that reads a token and inside the WWW-Authenticate layer, so the 401 carries the resource pointer a client needs to start OAuth discovery. Off by default. With it on, the token drawer and the home connect drawer stop offering to mint a token for an MCP URL and hand over the bare URL instead. Co-Authored-By: Claude Opus 5 * fix: read the MCP URL policy when a URL is asked for, and drop the all-workspaces option Two review findings on the token drawer: The policy was read once per page load and cached for the browser session, so a superadmin turning the setting on left every open tab handing out `?token=` URLs the server now refuses. Both entry points now read it when the user actually asks for an MCP URL: when MCP mode is entered, and when the connect drawer opens. The workspace picker offered "All workspaces / Multi-workspace", but the gateway's consent screen binds the token it issues to the one workspace picked there, so OAuth has no multi-workspace grant to hand out. That entry is now token-only. Co-Authored-By: Claude Opus 5 * fix: don't guess the MCP URL policy, and say where the switch lands on restart Review findings: The comment on the settings load claimed `MODE=mcp` as the target deployment, but that mode joins no monitor loop, so the startup pass is its only read and a change lands on restart. That is true of every global setting there, `base_url` included; the comment now says so, and the setting description tells an operator running dedicated MCP servers what to expect. A failed settings probe resolved to "tokens allowed", so with the switch on the drawer would mint a non-expiring token and hand over a URL the server refuses for as long as it exists. The probe now propagates its error and the panel reports it with a retry, creating nothing until the answer is known. The test passed a valid token, so it could not tell a rejection before authentication from one after it. It now also sends a token that was never valid and asserts the middleware's own message, which fails if the layer moves inward. Co-Authored-By: Claude Opus 5 * fix: withhold the MCP URL until a workspace is picked With no persisted workspace the store starts undefined, so opening the drawer from /user/workspaces before choosing one rendered a copyable `/api/mcp/w/undefined/mcp`. It reads like a real URL and a client pointed at it would never connect. The panel now asks for a workspace instead, matching the guard the token branch already has on its generate button. Co-Authored-By: Claude Opus 5 * docs: drop the coverage-status note from the MCP switch test It documented what the test does not reach rather than a constraint the next reader could break; that belongs in the PR, not the module doc. The layer-order rationale, which is what a future edit would break, stays. Co-Authored-By: Claude Opus 5 * refactor: fall back to the bare MCP URL instead of alerting on a failed read When the setting read fails, show the bare URL rather than an error with a retry. It works whichever way the setting is, so no alert is needed, and it still never mints a token for a URL the server may refuse. The connect drawer's wording falls back the same way so the blurb matches the panel. Co-Authored-By: Claude Opus 5 * refactor: move the MCP URL token setting to Core It sat in the Auth/OAuth/SAML list, which the settings sidebar shows under SSO, suggesting a dependency on SSO that does not exist: MCP OAuth has Windmill act as the authorization server, and any login method, password included, completes it. It is an instance-wide credential policy, so it now lives with the other ones in Core, kept out of quick setup like its neighbours. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- backend/src/main.rs | 9 +- backend/src/monitor.rs | 27 ++ .../tests/mcp_token_query_param.rs | 102 +++++++ backend/windmill-api-settings/src/lib.rs | 13 +- backend/windmill-api/src/lib.rs | 8 +- backend/windmill-api/src/mcp/core.rs | 30 ++- backend/windmill-api/src/mcp/mod.rs | 2 +- .../windmill-common/src/global_settings.rs | 5 + .../components/home/HomeConnectDrawer.svelte | 15 +- .../src/lib/components/instanceSettings.ts | 9 + .../components/settings/CreateToken.svelte | 255 +++++++++++------- frontend/src/lib/mcpAuth.ts | 20 ++ 12 files changed, 385 insertions(+), 110 deletions(-) create mode 100644 backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs create mode 100644 frontend/src/lib/mcpAuth.ts diff --git a/backend/src/main.rs b/backend/src/main.rs index 316ed099f2..9e7a93cfcd 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -52,7 +52,8 @@ use windmill_common::{ INSTANCE_EVENTS_WEBHOOK_SETTING, INSTANCE_PYTHON_VERSION_SETTING, JOB_DEFAULT_TIMEOUT_SECS_SETTING, JOB_ISOLATION_SETTING, JWT_SECRET_SETTING, KEEP_JOB_DIR_SETTING, LICENSE_KEY_SETTING, MAVEN_REPOS_SETTING, MAVEN_SETTINGS_XML_SETTING, - MONITOR_LOGS_ON_OBJECT_STORE_SETTING, NO_DEFAULT_MAVEN_SETTING, + MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, MONITOR_LOGS_ON_OBJECT_STORE_SETTING, + NO_DEFAULT_MAVEN_SETTING, NPM_CONFIG_REGISTRY_SETTING, NSJAIL_TMPFS_SIZE_MB_SETTING, NSJAIL_TMP_BACKING_SETTING, NUGET_CONFIG_SETTING, OAUTH_SETTING, OTEL_SETTING, OTEL_TRACES_RETENTION_SECS_SETTING, OTEL_TRACING_PROXY_SETTING, PIP_INDEX_URL_SETTING, POWERSHELL_REPO_PAT_SETTING, @@ -126,7 +127,8 @@ use windmill_worker::{ use crate::monitor::{ initial_load, load_concurrency_key_max_queued, load_disable_password_login, - load_fork_workspace_tag_append_fork_suffix, load_keep_job_dir, load_metrics_debug_enabled, + load_fork_workspace_tag_append_fork_suffix, load_keep_job_dir, + load_mcp_disable_token_query_param, load_metrics_debug_enabled, load_preview_tags_override, load_require_preexisting_user, load_retention_period_overrides, load_tag_per_workspace_enabled, load_tag_per_workspace_workspaces, load_workspace_fairness_duration_secs, load_workspace_fairness_enabled, @@ -2164,6 +2166,9 @@ async fn process_notify_event( DISABLE_PASSWORD_LOGIN_SETTING => { load_disable_password_login(db).await; } + MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING => { + load_mcp_disable_token_query_param(db).await; + } EXPOSE_METRICS_SETTING => { tracing::info!("Metrics setting changed, restarting"); spawn_graceful_killpill(tx, db, 30, "metrics setting change", server_mode) diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index 6b752c0ab4..750bc40560 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -65,6 +65,7 @@ use windmill_common::{ FORK_WORKSPACE_TAG_APPEND_FORK_SUFFIX_SETTING, HUB_API_SECRET_SETTING, HUB_BASE_URL_SETTING, INSTANCE_PYTHON_VERSION_SETTING, JOB_DEFAULT_TIMEOUT_SECS_SETTING, JOB_ISOLATION_SETTING, JWT_SECRET_SETTING, KEEP_JOB_DIR_SETTING, LICENSE_KEY_SETTING, + MCP_DISABLE_TOKEN_QUERY_PARAM, MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, MONITOR_LOGS_ON_OBJECT_STORE_SETTING, NPMRC_SETTING, NPM_CONFIG_REGISTRY_SETTING, NSJAIL_TMPFS_SIZE_MB_SETTING, NSJAIL_TMP_BACKING_SETTING, NUGET_CONFIG_SETTING, OTEL_SETTING, OTEL_TRACES_RETENTION_SECS_SETTING, OTEL_TRACING_PROXY_SETTING, @@ -288,6 +289,15 @@ pub async fn initial_load( ); if let Some(db) = conn.as_sql() { + // Outside the `server_mode` block below: a `MODE=mcp` process serves the MCP routes + // with `server_mode` false and would otherwise never read this at all. That mode + // joins no monitor loop, so there — as for every global setting, `base_url` + // included — this pass is the only read, and a change lands on restart. + pass.setting( + MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, + false, + |v| async move { apply_mcp_disable_token_query_param(v) }, + ); pass.setting(DEFAULT_TAGS_PER_WORKSPACE_SETTING, false, |v| async move { apply_tag_per_workspace_enabled(v) }); @@ -1617,6 +1627,23 @@ pub fn apply_disable_password_login(value: Option) { }; } +pub async fn load_mcp_disable_token_query_param(db: &DB) { + match load_value_from_global_settings(db, MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING).await { + Ok(v) => apply_mcp_disable_token_query_param(v), + Err(e) => tracing::error!("Error loading mcp_disable_token_query_param setting: {e:#}"), + }; +} + +pub fn apply_mcp_disable_token_query_param(value: Option) { + match value { + Some(serde_json::Value::Bool(t)) => { + MCP_DISABLE_TOKEN_QUERY_PARAM.store(t, Ordering::Relaxed) + } + None => MCP_DISABLE_TOKEN_QUERY_PARAM.store(false, Ordering::Relaxed), + _ => (), + }; +} + struct LogFile { file_path: String, hostname: String, diff --git a/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs b/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs new file mode 100644 index 0000000000..d0d6e070f6 --- /dev/null +++ b/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs @@ -0,0 +1,102 @@ +//! The `mcp_disable_token_query_param` switch closes the URL-borne credential path. +//! +//! The rejection is a middleware layered between the `WWW-Authenticate` decorator and +//! everything that reads a token, on both the workspaced and the gateway mount. Each half of +//! that sandwich is pinned: the `WWW-Authenticate` header on the refusal catches the layer +//! being moved outward (a client would lose the pointer that starts OAuth discovery), and +//! refusing a token that was never valid catches it being moved inward past authentication +//! (the URL-borne token would be hashed and looked up before anything refused it). +#![cfg(feature = "mcp")] + +use std::sync::atomic::Ordering; + +use serde_json::json; +use sqlx::{Pool, Postgres}; + +use windmill_common::global_settings::MCP_DISABLE_TOKEN_QUERY_PARAM; +use windmill_test_utils::*; + +/// Workspace-less with an `mcp:` scope, which is what the gateway mount requires; the +/// workspaced mount takes its workspace from the path, so one token reaches both. +async fn insert_mcp_token(db: &Pool) -> anyhow::Result<()> { + sqlx::query( + "INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin, scopes) + VALUES (encode(sha256('MCP_TOKEN'::bytea), 'hex'), 'MCP_TOK', 'MCP_TOKEN', 'test@windmill.dev', 'mcp token', true, ARRAY['mcp:all'])", + ) + .execute(db) + .await?; + Ok(()) +} + +/// A token that is not in `token` at all. Authentication would refuse it on its own, so a +/// refusal carrying the middleware's own wording is evidence nothing looked it up first. +const BOGUS_TOKEN: &str = "NOT_A_REAL_TOKEN"; + +async fn tools_list(url: &str) -> anyhow::Result { + Ok(reqwest::Client::new() + .post(url) + .header("Accept", "application/json, text/event-stream") + .json(&json!({ "jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {} })) + .send() + .await?) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_mcp_token_query_param_switch(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + set_jwt_secret().await; + insert_mcp_token(&db).await?; + let server = ApiServer::start_mcp(db.clone()).await?; + let port = server.addr.port(); + let workspaced = + format!("http://localhost:{port}/api/mcp/w/test-workspace/mcp?token=MCP_TOKEN"); + let gateway = format!("http://localhost:{port}/api/mcp/gateway?token=MCP_TOKEN"); + + assert_eq!( + tools_list(&workspaced).await?.status(), + 200, + "a URL-borne token is the documented default and must keep working while the switch is off" + ); + assert_eq!(tools_list(&gateway).await?.status(), 200); + + MCP_DISABLE_TOKEN_QUERY_PARAM.store(true, Ordering::Relaxed); + + for url in [&workspaced, &gateway] { + let resp = tools_list(url).await?; + assert_eq!( + resp.status(), + 401, + "{url} still admitted a token in the URL" + ); + // What sends the client into the OAuth flow rather than leaving it stuck on a 401. + assert!( + resp.headers().contains_key("www-authenticate"), + "{url} rejected without pointing at the authorization server" + ); + } + + // Refused before authentication, not after: an invalid token gets the middleware's own + // message rather than the generic 401 that looking it up would produce. + let resp = tools_list(&format!( + "http://localhost:{port}/api/mcp/w/test-workspace/mcp?token={BOGUS_TOKEN}" + )) + .await?; + assert_eq!(resp.status(), 401); + assert!( + resp.text().await?.contains("does not accept a token in the MCP URL"), + "an invalid URL token was answered by authentication, so the token was read before \ + the switch refused it" + ); + + // The header stays open: it is the channel the OAuth flow itself hands tokens over on. + let resp = reqwest::Client::new() + .post(format!("http://localhost:{port}/api/mcp/gateway")) + .header("Accept", "application/json, text/event-stream") + .header("Authorization", "Bearer MCP_TOKEN") + .json(&json!({ "jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {} })) + .send() + .await?; + assert_eq!(resp.status(), 200); + + Ok(()) +} diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 55e22d912f..3bbaf888e2 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -64,10 +64,11 @@ use windmill_common::{ GITHUB_APP_WEBHOOK_BASE_URL_SETTING, HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, HUB_ACCESSIBLE_URL_SETTING, HUB_BASE_URL_SETTING, INSTANCE_BANNER_SETTING, MAX_RETENTION_OVERRIDE_WORKSPACES, - MAX_TOKEN_EXPIRATION_DAYS_SETTING, RETENTION_PERIOD_SECS_OVERRIDES_SETTING, - RUFF_CONFIG_SETTING, UNIQUE_ID_SETTING, WORKSPACE_FAIRNESS_DURATION_SECS_SETTING, - WORKSPACE_FAIRNESS_ENABLED_SETTING, WORKSPACE_FAIRNESS_MAX_PERCENT_SETTING, - WORKSPACE_FAIRNESS_MIN_TOTAL_SETTING, WS_BASE_URL_SETTING, + MAX_TOKEN_EXPIRATION_DAYS_SETTING, MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, + RETENTION_PERIOD_SECS_OVERRIDES_SETTING, RUFF_CONFIG_SETTING, UNIQUE_ID_SETTING, + WORKSPACE_FAIRNESS_DURATION_SECS_SETTING, WORKSPACE_FAIRNESS_ENABLED_SETTING, + WORKSPACE_FAIRNESS_MAX_PERCENT_SETTING, WORKSPACE_FAIRNESS_MIN_TOTAL_SETTING, + WS_BASE_URL_SETTING, }, instance_config::{self, ApplyMode, InstanceConfig}, server::Smtp, @@ -1364,6 +1365,10 @@ pub async fn get_global_setting( && key != INSTANCE_BANNER_SETTING // The token form reads it to stop offering expirations the server would shorten. && key != MAX_TOKEN_EXPIRATION_DAYS_SETTING + // Whoever is wiring up an MCP client reads it to know whether a URL-borne token + // would be refused, and they are usually not a superadmin. Not a secret: pointing + // any MCP client at the instance discovers the same answer. + && key != MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING { require_super_admin(&db, &authed).await?; } diff --git a/backend/windmill-api/src/lib.rs b/backend/windmill-api/src/lib.rs index b70d80140a..0fd5ac31b5 100644 --- a/backend/windmill-api/src/lib.rs +++ b/backend/windmill-api/src/lib.rs @@ -560,7 +560,7 @@ pub async fn run_server( if server_mode || mcp_mode { use mcp::{ add_www_authenticate_header, add_www_authenticate_header_gateway, - extract_workspace_from_token, + extract_workspace_from_token, reject_token_query_param, }; let (mcp_router, mcp_cancellation_token) = setup_mcp_server( db.clone(), @@ -573,15 +573,17 @@ pub async fn run_server( let workspaced_mcp_router = mcp_router .clone() .route_layer(from_extractor::()) + .layer(axum::middleware::from_fn(reject_token_query_param)) .layer(axum::middleware::from_fn(add_www_authenticate_header)) .layer(axum::middleware::from_fn(extract_and_store_workspace_id)); // Gateway MCP router — resolves workspace from token let gateway_mcp_router = mcp_router .route_layer(from_extractor::()) + .layer(axum::middleware::from_fn(extract_workspace_from_token)) + .layer(axum::middleware::from_fn(reject_token_query_param)) .layer(axum::middleware::from_fn( add_www_authenticate_header_gateway, - )) - .layer(axum::middleware::from_fn(extract_workspace_from_token)); + )); ( workspaced_mcp_router, gateway_mcp_router, diff --git a/backend/windmill-api/src/mcp/core.rs b/backend/windmill-api/src/mcp/core.rs index 480e3c0841..86fbec1806 100644 --- a/backend/windmill-api/src/mcp/core.rs +++ b/backend/windmill-api/src/mcp/core.rs @@ -43,11 +43,14 @@ use axum::{ extract::{Extension, Path}, http::Request, middleware::Next, - response::Response, + response::{IntoResponse, Response}, routing::get, Json, Router, }; -use windmill_common::{auth::hash_token, db::GatewayWorkspaceId, error::JsonResult}; +use windmill_common::{ + auth::hash_token, db::GatewayWorkspaceId, error::JsonResult, + global_settings::MCP_DISABLE_TOKEN_QUERY_PARAM, +}; // McpAuth impl for ApiAuthed is in windmill-api-auth (same crate as the type) @@ -446,6 +449,29 @@ pub async fn add_www_authenticate_header( } } +/// Middleware refusing a credential carried in the MCP URL once the instance sets +/// `mcp_disable_token_query_param`. Sits outside everything that reads the token, so neither +/// the gateway lookup nor `ApiAuthed` ever sees it, and inside the `WWW-Authenticate` layer, +/// whose header is what sends the client into the OAuth flow instead. Refused rather than +/// ignored: the URL leaked the token whether or not the request used it. +pub async fn reject_token_query_param(request: Request, next: Next) -> Response { + let carries_token = MCP_DISABLE_TOKEN_QUERY_PARAM.load(std::sync::atomic::Ordering::Relaxed) + && request + .uri() + .query() + .is_some_and(|q| url::form_urlencoded::parse(q.as_bytes()).any(|(k, _)| k == "token")); + if carries_token { + return ( + axum::http::StatusCode::UNAUTHORIZED, + "This instance does not accept a token in the MCP URL. Remove the token query \ + parameter and let your client sign in through OAuth, or send the token in an \ + Authorization header.", + ) + .into_response(); + } + next.run(request).await +} + /// Extract the bearer token from either the `Authorization` header or the /// `?token=` query parameter (MCP clients commonly pass it in the URL). fn extract_gateway_token(request: &Request) -> Option { diff --git a/backend/windmill-api/src/mcp/mod.rs b/backend/windmill-api/src/mcp/mod.rs index 5f6bd5edb5..5545d59a9f 100644 --- a/backend/windmill-api/src/mcp/mod.rs +++ b/backend/windmill-api/src/mcp/mod.rs @@ -12,5 +12,5 @@ pub mod oauth_server; pub use core::{ add_www_authenticate_header, add_www_authenticate_header_gateway, extract_and_store_workspace_id, extract_workspace_from_token, list_tools_service, - setup_mcp_server, + reject_token_query_param, setup_mcp_server, }; diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index b022f9195e..6b9aae519d 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -102,6 +102,10 @@ pub const HUB_API_SECRET_SETTING: &str = "hub_api_secret"; pub const AUTOMATE_USERNAME_CREATION_SETTING: &str = "automate_username_creation"; pub const DISABLE_WORKSPACE_INVITE_EMAILS_SETTING: &str = "disable_workspace_invite_emails"; pub const DISABLE_PASSWORD_LOGIN_SETTING: &str = "disable_password_login"; +/// Refuse `?token=` on the MCP endpoints, leaving the `Authorization` header as the only way +/// in. A URL-borne credential ends up in browser history, proxy logs and referrers, so an +/// instance that cares sends MCP clients through the OAuth flow instead. +pub const MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING: &str = "mcp_disable_token_query_param"; /// Ceiling, in days, on how far ahead a token minted through `POST /users/tokens/create` or /// `POST /users/tokens/impersonate` may expire; a request asking for more, or for no /// expiration at all, is shortened to it rather than refused. On those routes only: server-side @@ -407,6 +411,7 @@ use std::sync::atomic::AtomicBool; lazy_static::lazy_static! { pub static ref HTTP_ROUTE_WORKSPACED_ROUTE: AtomicBool = AtomicBool::new(false); pub static ref DISABLE_PASSWORD_LOGIN: AtomicBool = AtomicBool::new(false); + pub static ref MCP_DISABLE_TOKEN_QUERY_PARAM: AtomicBool = AtomicBool::new(false); /// Origins HTTP routes allow cross-origin when they configure none of their /// own. Empty means unset, which keeps the historical `*`. pub static ref HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS: arc_swap::ArcSwap> = diff --git a/frontend/src/lib/components/home/HomeConnectDrawer.svelte b/frontend/src/lib/components/home/HomeConnectDrawer.svelte index e7e4a306d9..e08ac2ddea 100644 --- a/frontend/src/lib/components/home/HomeConnectDrawer.svelte +++ b/frontend/src/lib/components/home/HomeConnectDrawer.svelte @@ -5,10 +5,12 @@ import CopyableCodeBlock from '$lib/components/details/CopyableCodeBlock.svelte' import { Bot, ExternalLink, Terminal } from 'lucide-svelte' import { shell } from 'svelte-highlight/languages' + import { mcpTokenUrlDisabled } from '$lib/mcpAuth' type ConnectTab = 'cli' | 'mcp' let drawer: Drawer | undefined = $state() + let tokenUrlDisabled = $state(false) let selectedTab: ConnectTab = $state('cli') let openVersion = $state(0) @@ -24,6 +26,10 @@ wmill sync pull`) export function openDrawer(tab: ConnectTab = 'cli') { selectedTab = tab openVersion += 1 + // Falls back like CreateToken below, which shows the bare URL when the read fails. + void mcpTokenUrlDisabled() + .then((v) => (tokenUrlDisabled = v)) + .catch(() => (tokenUrlDisabled = true)) drawer?.openDrawer() } @@ -96,8 +102,13 @@ wmill sync pull`)

MCP URL

- Generate an MCP server URL for the current workspace and choose which - scripts, flows, and endpoints the client can access. + {#if tokenUrlDisabled} + The MCP server URL for the current workspace. Your client signs in to + Windmill to use it. + {:else} + Generate an MCP server URL for the current workspace and choose which + scripts, flows, and endpoints the client can access. + {/if}

diff --git a/frontend/src/lib/components/instanceSettings.ts b/frontend/src/lib/components/instanceSettings.ts index 06e720a846..7335b33848 100644 --- a/frontend/src/lib/components/instanceSettings.ts +++ b/frontend/src/lib/components/instanceSettings.ts @@ -325,6 +325,15 @@ export const settings: Record = { value === null || value === '' || parseMaxTokenExpirationDays(value) !== undefined + }, + { + label: 'Disable token in MCP URLs', + description: + 'Reject the ?token= query parameter on the MCP endpoints, so MCP clients authenticate with an Authorization header or through the OAuth flow. A token in a URL is a credential that ends up in browser history, proxy logs and referrers. Existing MCP URLs carrying a token stop working. Servers and workers pick this up within a minute; dedicated MCP servers (MODE=mcp) apply it when they next restart.', + key: 'mcp_disable_token_query_param', + fieldType: 'boolean', + storage: 'setting', + hideInQuickSetup: true } ], Jobs: [ diff --git a/frontend/src/lib/components/settings/CreateToken.svelte b/frontend/src/lib/components/settings/CreateToken.svelte index acde0a557a..146f844b3f 100644 --- a/frontend/src/lib/components/settings/CreateToken.svelte +++ b/frontend/src/lib/components/settings/CreateToken.svelte @@ -6,12 +6,15 @@ workspaceStore, type UserWorkspace } from '$lib/stores' - import { Button } from '../common' + import { Alert, Button, Skeleton } from '../common' import { triggerableByAI } from '$lib/actions/triggerableByAI.svelte' import Toggle from '../Toggle.svelte' import { SettingService, UserService, type NewToken } from '$lib/gen' + import { mcpTokenUrlDisabled } from '$lib/mcpAuth' import TokenDisplay from './TokenDisplay.svelte' import ScopesPicker from './ScopesPicker.svelte' + import CopyableCodeBlock from '../details/CopyableCodeBlock.svelte' + import { shell } from 'svelte-highlight/languages' import { parseMaxTokenExpirationDays } from '$lib/tokenExpiration' import TextInput from '../text_input/TextInput.svelte' @@ -59,6 +62,22 @@ let pickedScopes = $state(null) let readOnly = $state(false) + // How this instance lets an MCP client in. `oauth` means it refuses `?token=`, so a + // generated token would not get a client in and the URL is handed over bare instead. + // A failed read lands on `oauth`: the bare URL works whichever way the setting is, + // whereas guessing `token` mints a non-expiring credential the server may refuse. + type McpUrlPolicy = 'loading' | 'token' | 'oauth' + let mcpUrlPolicy = $state('loading') + + async function loadMcpUrlPolicy() { + mcpUrlPolicy = 'loading' + try { + mcpUrlPolicy = (await mcpTokenUrlDisabled()) ? 'oauth' : 'token' + } catch (err) { + console.error('Failed to load the MCP token setting:', err) + mcpUrlPolicy = 'oauth' + } + } const DAY_SECS = 24 * 60 * 60 const EXPIRATION_CHOICES = [ @@ -117,6 +136,7 @@ function enterMcpMode() { mcpCreationMode = true + void loadMcpUrlPolicy() resetExpirationOnModeChange() newTokenWorkspace = defaultNewTokenWorkspace ?? $workspaceStore newToken = undefined @@ -224,11 +244,17 @@ const scopeWorkspaceId = $derived( isAllWorkspaces ? $workspaceStore || '' : newTokenWorkspace || $workspaceStore || '' ) - const mcpBaseUrl = $derived( + // Undefined wherever the workspace is: `/api/mcp/w/undefined/mcp` reads like a real URL + // and is copyable, so the OAuth panel withholds it rather than showing a broken one. The + // token branch guards the same case by disabling its generate button. + const mcpUrl = $derived( isAllWorkspaces - ? `${window.location.origin}/api/mcp/gateway?token=` - : `${window.location.origin}/api/mcp/w/${newTokenWorkspace}/mcp?token=` + ? `${window.location.origin}/api/mcp/gateway` + : newTokenWorkspace + ? `${window.location.origin}/api/mcp/w/${newTokenWorkspace}/mcp` + : undefined ) + const mcpBaseUrl = $derived(`${mcpUrl ?? ''}?token=`) $effect(() => { const requestedMcpMode = mcpOnly || openWithMcpMode @@ -256,7 +282,9 @@
-

{title}

+

+ {mcpCreationMode && mcpUrlPolicy !== 'token' ? 'MCP URL' : title} +

{#if showMcpMode && !mcpOnly}
{/if} - {#if scopes != undefined} -
- Scope - {#each scopes as scope (scope)} - - {/each} -
- + {:else if mcpCreationMode && mcpUrlPolicy === 'oauth'} + {#if !lockWorkspace} +
+ Workspace + + ({ label: w.name, value: w.id, subtitle: w.id })) - ]} +
+ + Paste this URL into your client. It opens a Windmill page where you approve the access + it asks for, and no token needs to be copied around. + +
+ {:else} +

Pick a workspace to get its MCP URL.

+ {/if} + + {#if !mcpOnly} +
+ +
+ {/if} + {:else} + {#if scopes != undefined} +
+ Scope + {#each scopes as scope (scope)} + + {/each} +
+ - {#if isAllWorkspaces} +
+
+ {/if} + + {#if !scopes || scopes.length === 0} + + {/if} + +
+ {#if mcpCreationMode} + {#if !lockWorkspace} +
+ Workspace + newTokenExpiration, (v) => (pickedExpiration = v)} + placeholder={maxExpirationSecs == undefined ? 'No expiration' : 'Pick an expiration'} + inputClass="w-full" + items={expirationItems} + /> + {#if maxExpirationSecs != undefined}

- This token works across every workspace you can access. Tools take a - workspace_id argument; call list_workspaces to discover them. + This instance limits tokens to {maxExpirationLabel}.

{/if}
{/if} - {/if} +
- {#if !mcpOnly} -
- Label (optional) + {#if !mcpOnly} +
- {/if} - - {#if !mcpCreationMode || maxExpirationSecs != undefined} -
- - Expires In - {#if maxExpirationSecs == undefined} - (optional) - {/if} - - ({ value: s, label: s }))} + bind:value={ + () => schema, + (s) => { + schema = s + table = undefined + } + } + placeholder="The database itself" + clearable + loading={schemasLoading} + size="sm" + class="w-56" + /> + {#if schema} + info.owner, + (role) => { + // The select shows what the database says; a pick is a request, and only the + // applied change moves it. + if (role && role !== info.owner) { + confirm({ type: 'set_owner', role }, `Ownership transferred to ${role}`) + } + } + } + /> + {:else} + {info.owner} + {/if} +
+ {/if} + +
+
+ Grants + + {target.kind === 'database' + ? 'What each role may do on the database itself — CREATE is the right to create schemas in it — and what default privileges set database-wide give it on what is created later, in every schema. No schema can take those back.' + : 'What each role may do here, beyond what it owns.'} + +
+ {#if info.editable} + + confirm( + { type: 'grant', role, privileges, scope }, + `Granted ${privileges.join(', ')} to ${role}` + )} + /> + {/if} + {#if grantRows.length === 0} + No grants yet. + {:else} + + + + Role + Privileges + On + + + + + {#each grantRows as grant (grantKey(grant))} + {@const revokeScope = revokeScopeOf(grant)} + {@const revocable = revocablePrivileges(grant, target)} + {@const blocked = blockingSources(grant, revocable)} + {@const uncovered = uncoveredCreators(grant, info.roles)} + + {grant.grantee} + {grant.privileges.join(', ')} + + {grantScopeLabel(grant)} + {#if blocked.length > 0} + + from {blocked.join(', ')} + + {/if} + {#if uncovered.length > 0} + + · not for what {uncovered.join(', ')} + {uncovered.length === 1 ? 'creates' : 'create'} + + {/if} + + + + {#if info.editable && revokeScope && revocable.length > 0 && blocked.length === 0 && info.roles.includes(grant.grantee) && grant.grantee !== ADMIN_ROLE} +
+
+{/if} + + (pending = undefined)} +> +
+ {#if pendingCoversObjects} + + The same privileges on several objects read as one row, and are revoked together. + + {/if} + {#each pending?.warnings ?? [] as warning (warning)} + {warning} + {/each} + + Runs against {datatable} in a single transaction: + + 
{(pending?.statements ?? []).join(';\n')};
+
+ diff --git a/frontend/src/lib/components/datatableAcl/PgGrantBuilder.svelte b/frontend/src/lib/components/datatableAcl/PgGrantBuilder.svelte new file mode 100644 index 0000000000..5e5119abb6 --- /dev/null +++ b/frontend/src/lib/components/datatableAcl/PgGrantBuilder.svelte @@ -0,0 +1,95 @@ + + +
+
+ GRANT + ({ value: p, label: p }))} + placeholder="privileges" + {disabled} + size="sm" + class="min-w-48" + /> + ON + ({ value: r, label: r }))} + placeholder="role" + {disabled} + size="sm" + class="w-40" + /> + +
+ {#if statement} + 
{statement}
+ {/if} +
diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts new file mode 100644 index 0000000000..7c01836e89 --- /dev/null +++ b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, it } from 'vitest' +import type { AclGrant } from '$lib/gen' +import { + blockingSources, + grantKey, + groupGrants, + revocablePrivileges, + revokeScopeOf, + uncoveredCreators +} from './aclScopes' + +const table = (name: string) => ({ name, kind: 'TABLE' }) +const by = (role: string, privileges: string[], reachable = true) => ({ + role, + privileges, + reachable +}) +const byAdmin = (grant: Omit): AclGrant => ({ + ...grant, + sources: [by('admin', grant.privileges)] +}) + +describe('grantKey', () => { + it('tells apart a table and a function of the same name', () => { + const row = (object: { name: string; kind: string; args?: string }) => ({ + grantee: 'analytics', + privileges: ['SELECT'], + objects: [object], + sources: [by('admin', ['SELECT'])] + }) + expect(grantKey(row(table('orders')))).not.toBe( + grantKey(row({ name: 'orders', kind: 'FUNCTION', args: '' })) + ) + }) +}) + +describe('groupGrants', () => { + // A row's revoke names every object in it, so a row must only hold what one revoke may take. + it('folds the same privileges on objects of one kind, and nothing else', () => { + const grants: AclGrant[] = [ + { grantee: 'analytics', privileges: ['SELECT'], object: table('orders') }, + { grantee: 'analytics', privileges: ['SELECT'], object: table('salaries') }, + { grantee: 'operator', privileges: ['SELECT'], object: table('orders') }, + { grantee: 'analytics', privileges: ['INSERT', 'SELECT'], object: table('events') }, + { grantee: 'analytics', privileges: ['SELECT'], object: { name: 's', kind: 'SEQUENCE' } }, + { grantee: 'analytics', privileges: ['SELECT'], future: 'TABLES' }, + { grantee: 'analytics', privileges: ['USAGE'] } + ].map(byAdmin) + const rows = groupGrants(grants) + expect(rows.map((r) => [r.grantee, r.privileges, r.objects, r.future])).toEqual([ + ['analytics', ['SELECT'], [table('orders'), table('salaries')], undefined], + ['operator', ['SELECT'], [table('orders')], undefined], + ['analytics', ['INSERT', 'SELECT'], [table('events')], undefined], + ['analytics', ['SELECT'], [{ name: 's', kind: 'SEQUENCE' }], undefined], + ['analytics', ['SELECT'], [], 'TABLES'], + ['analytics', ['USAGE'], [], undefined] + ]) + }) + + // A revoke takes the row back from every source, so the row must name them all, with what each + // gave. + it('keeps every source of the grants it folds', () => { + const grants: AclGrant[] = [ + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('orders'), + sources: [by('admin', ['SELECT'])] + }, + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('salaries'), + sources: [by('admin', ['SELECT']), by('operator', ['SELECT'])] + } + ] + expect(groupGrants(grants)[0].sources).toEqual([ + by('admin', ['SELECT']), + by('operator', ['SELECT']) + ]) + }) +}) + +describe('revoke of a row', () => { + const row = (future?: string) => ({ + grantee: 'analytics', + privileges: ['SELECT'], + objects: [], + future, + sources: [by('admin', ['SELECT'])] + }) + + it('takes back only what the editor may revoke on the database', () => { + const database = { ...row(), privileges: ['CONNECT', 'CREATE'] } + expect(revocablePrivileges(database, { kind: 'database' })).toEqual(['CREATE']) + expect(revocablePrivileges(database, { kind: 'schema', schema: 'public' })).toEqual([ + 'CONNECT', + 'CREATE' + ]) + // Set database-wide, so not one a schema's revoke could take back either. + const schemasLater = { ...row('SCHEMAS'), privileges: ['CREATE'] } + expect(revocablePrivileges(schemasLater, { kind: 'database' })).toEqual([]) + }) + + it('maps default privileges to their scope, and refuses the ones it has none for', () => { + expect(revokeScopeOf(row())).toBe('target') + expect(revokeScopeOf(row('TABLES'))).toBe('future_tables') + expect(revokeScopeOf(row('TYPES'))).toBeUndefined() + expect(revokeScopeOf({ ...row(), objects: [{ name: 'mood', kind: 'TYPE' }] })).toBeUndefined() + }) + + // Postgres takes a grant back only through its source: offering the revoke would promise what + // the plan then refuses. But only the sources of what is revoked count: the catalog's CONNECT + // on the database comes from its owner, out of reach, and must not hold back a CREATE the + // editor granted. + it('is held back only by a source out of reach for what it takes', () => { + const database = { + ...row(), + privileges: ['CONNECT', 'CREATE'], + sources: [by('postgres', ['CONNECT'], false), by('admin', ['CREATE'])] + } + const revocable = revocablePrivileges(database, { kind: 'database' }) + expect(blockingSources(database, revocable)).toEqual([]) + expect(blockingSources(database, ['CONNECT'])).toEqual(['postgres']) + const partly = { + ...row('TABLES'), + sources: [by('admin', ['SELECT']), by('postgres', ['SELECT'], false)] + } + expect(blockingSources(partly, ['SELECT'])).toEqual(['postgres']) + }) + + // Whether a grant can be taken back depends on its object, so a row folding several objects is + // only revocable if each of its grants is. + it('is held back by a source out of reach on any of the objects it folds', () => { + const grants: AclGrant[] = [ + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('orders'), + sources: [by('admin', ['SELECT'])] + }, + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('salaries'), + sources: [by('admin', ['SELECT'], false)] + } + ] + const [folded] = groupGrants(grants) + expect(folded.objects).toHaveLength(2) + expect(blockingSources(folded, ['SELECT'])).toEqual(['admin']) + // Folding reads the grants, never rewrites them. + expect(grants[0].sources[0].reachable).toBe(true) + }) +}) + +describe('uncoveredCreators', () => { + // A default privilege binds only the creating roles it was granted for: a role added since is + // left out until the grant is made again. + it('names the roles a created-later row leaves out', () => { + const future = { + grantee: 'analytics', + privileges: ['SELECT'], + objects: [], + future: 'TABLES', + sources: [by('admin', ['SELECT']), by('analytics', ['SELECT'])] + } + expect(uncoveredCreators(future, ['admin', 'analytics', 'late'])).toEqual(['late']) + expect(uncoveredCreators({ ...future, future: undefined }, ['late'])).toEqual([]) + // Set by a role outside the catalog, it was never meant to cover the catalog's roles. + expect( + uncoveredCreators({ ...future, sources: [by('postgres', ['SELECT'], false)] }, [ + 'admin', + 'late' + ]) + ).toEqual([]) + }) +}) diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.ts b/frontend/src/lib/components/datatableAcl/aclScopes.ts new file mode 100644 index 0000000000..d5b8ad9d20 --- /dev/null +++ b/frontend/src/lib/components/datatableAcl/aclScopes.ts @@ -0,0 +1,211 @@ +import type { AclGrant, AclSource, AclTarget } from '$lib/gen' + +/** The role a data table connects as without roles — `custom_instance_user` in Postgres. */ +export const ADMIN_ROLE = 'admin' + +/** Privileges Postgres accepts per kind of object. Mirrors the whitelist the backend validates + * against — a privilege missing here just cannot be built. */ +/** `CREATE` on a database is the right to create schemas in it, and the only database privilege + * handed out here: `CONNECT` is managed with the instance's role catalog. */ +export const DATABASE_PRIVILEGES = ['CREATE'] +export const SCHEMA_PRIVILEGES = ['USAGE', 'CREATE'] +export const TABLE_PRIVILEGES = [ + 'SELECT', + 'INSERT', + 'UPDATE', + 'DELETE', + 'TRUNCATE', + 'REFERENCES', + 'TRIGGER' +] +/** Postgres 17 and later only, so it is offered from what the server reports. */ +export const MAINTAIN_PRIVILEGE = 'MAINTAIN' +export const SEQUENCE_PRIVILEGES = ['USAGE', 'SELECT', 'UPDATE'] +export const FUNCTION_PRIVILEGES = ['EXECUTE'] + +export type AclScope = + | 'target' + | 'all_tables' + | 'all_sequences' + | 'all_functions' + | 'future_tables' + | 'future_sequences' + | 'future_functions' + +export type AclTargetKind = AclTarget['kind'] + +/** The scopes a target can grant on, in the order the builder offers them. */ +export function scopesOf(kind: AclTargetKind): { value: AclScope; label: string }[] { + if (kind === 'database') return [{ value: 'target', label: 'the database itself' }] + if (kind === 'table') return [{ value: 'target', label: 'this table' }] + return [ + { value: 'target', label: 'the schema itself' }, + { value: 'all_tables', label: 'all tables in it' }, + { value: 'all_sequences', label: 'all sequences in it' }, + { value: 'all_functions', label: 'all functions in it' }, + { value: 'future_tables', label: 'tables created later' }, + { value: 'future_sequences', label: 'sequences created later' }, + { value: 'future_functions', label: 'functions created later' } + ] +} + +export function privilegesOf( + scope: AclScope, + kind: AclTargetKind, + supportsMaintain = false +): string[] { + const tablePrivileges = supportsMaintain + ? [...TABLE_PRIVILEGES, MAINTAIN_PRIVILEGE] + : TABLE_PRIVILEGES + switch (scope) { + case 'target': + if (kind === 'database') return DATABASE_PRIVILEGES + return kind === 'schema' ? SCHEMA_PRIVILEGES : tablePrivileges + case 'all_tables': + case 'future_tables': + return tablePrivileges + case 'all_sequences': + case 'future_sequences': + return SEQUENCE_PRIVILEGES + case 'all_functions': + case 'future_functions': + return FUNCTION_PRIVILEGES + } +} + +/** What a statement built at this scope reads as, for the builder's own preview. */ +export function scopeSql(scope: AclScope, target: AclTarget, dbname?: string): string { + if (target.kind === 'database') return `DATABASE ${dbname ?? ''}`.trim() + const schema = target.schema + switch (scope) { + case 'target': + return target.kind === 'schema' ? `SCHEMA ${schema}` : `TABLE ${schema}.${target.table}` + case 'all_tables': + return `ALL TABLES IN SCHEMA ${schema}` + case 'all_sequences': + return `ALL SEQUENCES IN SCHEMA ${schema}` + case 'all_functions': + return `ALL FUNCTIONS IN SCHEMA ${schema}` + case 'future_tables': + return `TABLES (default privileges in ${schema})` + case 'future_sequences': + return `SEQUENCES (default privileges in ${schema})` + case 'future_functions': + return `FUNCTIONS (default privileges in ${schema})` + } +} + +/** One row of the grants table: the same privileges on several objects read as one line, since + * granting them per object is what `ON ALL TABLES` does. */ +export type GroupedGrant = { + grantee: string + privileges: string[] + objects: NonNullable[] + future?: string + /** Every role the row's grants come from, each once, with what it gave. */ + sources: AclSource[] +} + +export function groupGrants(grants: AclGrant[]): GroupedGrant[] { + const rows: GroupedGrant[] = [] + for (const grant of grants) { + const existing = grant.object + ? rows.find( + (r) => + r.grantee === grant.grantee && + r.future === grant.future && + r.objects[0]?.kind === grant.object?.kind && + r.privileges.join() === grant.privileges.join() + ) + : undefined + if (existing) { + existing.objects.push(grant.object!) + for (const source of grant.sources) { + const known = existing.sources.find((s) => s.role === source.role) + // Whether a role's grant can be taken back depends on the object it is on, so a row + // holds a source as reachable only if it is on every object the row folds. + if (known) { + known.reachable &&= source.reachable + known.privileges = [...new Set([...known.privileges, ...source.privileges])].sort() + } else { + existing.sources.push({ ...source, privileges: [...source.privileges] }) + } + } + } else { + rows.push({ + grantee: grant.grantee, + privileges: grant.privileges, + objects: grant.object ? [grant.object] : [], + future: grant.future, + sources: grant.sources.map((s) => ({ ...s, privileges: [...s.privileges] })) + }) + } + } + return rows +} + +/** The roles that gave some of `privileges` and that this data table's connection cannot act for. + * Only they can take those grants back, so a revoke of `privileges` is not offered. */ +export function blockingSources(grant: GroupedGrant, privileges: string[]): string[] { + return grant.sources + .filter((s) => !s.reachable && s.privileges.some((p) => privileges.includes(p))) + .map((s) => s.role) +} + +/** Which of `roles` a "created later" row granted for some of them does not cover. A default + * privilege binds only the creating roles it was granted for, so what the others create stays out + * of it. A row none of `roles` set — the instance's own, say — was never meant to cover them, and + * names none. */ +export function uncoveredCreators(grant: GroupedGrant, roles: string[]): string[] { + if (!grant.future || !grant.sources.some((s) => roles.includes(s.role))) return [] + return roles.filter((r) => !grant.sources.some((s) => s.role === r)) +} + +/** A row's identity. Two rows may share a grantee and an object name — a table `orders` and a + * function `orders()` — so the kind and the privileges are part of it too. */ +export function grantKey(grant: GroupedGrant): string { + return [ + grant.grantee, + grant.future ?? '', + grant.privileges.join(','), + ...grant.objects.map((o) => `${o.kind}:${o.name}(${o.args ?? ''})`) + ].join('|') +} + +/** The scope a revoke of this row takes, or `undefined` when the builder cannot express it — + * Postgres also records privileges on types, present and default, which nothing here grants and + * the API has no scope for. */ +export function revokeScopeOf(grant: GroupedGrant): AclScope | undefined { + if (!grant.future) return grant.objects.some((o) => o.kind === 'TYPE') ? undefined : 'target' + const scope = `future_${grant.future.toLowerCase()}` + return (['future_tables', 'future_sequences', 'future_functions'] as const).find( + (s) => s === scope + ) +} + +/** The privileges of a row a revoke may take back. On the database that is `CREATE` alone: + * `CONNECT` belongs to the role catalog, which would grant it again, and `TEMPORARY` is not one + * the editor hands out — a row holding only those has nothing to revoke here. A database's rows + * "created later" are default privileges set database-wide, which nothing here revokes. */ +export function revocablePrivileges(grant: GroupedGrant, target: AclTarget): string[] { + if (target.kind === 'database' && grant.objects.length === 0) { + if (grant.future) return [] + return grant.privileges.filter((p) => DATABASE_PRIVILEGES.includes(p)) + } + return grant.privileges +} + +/** How a row reads back: what it covers, in one phrase. */ +export function grantScopeLabel(grant: GroupedGrant): string { + if (grant.future) return `${grant.future.toLowerCase()} created later` + if (grant.objects.length === 1) { + const object = grant.objects[0] + // A routine's arguments are part of what it is, so two of the same name would otherwise + // read as one row twice. + const args = object.args !== undefined ? `(${object.args})` : '' + return `${object.kind.toLowerCase()} ${object.name}${args}` + } + if (grant.objects.length > 1) + return `${grant.objects.length} ${grant.objects[0].kind.toLowerCase()}s` + return 'itself' +} diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index 17ba0edd09..c5c4171150 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -11,10 +11,14 @@ GroupService, UserService, WorkspaceService, + type AclTarget, + type DatatableAclInfo, type DatatablePermissions, type InstanceDatatableRole } from '$lib/gen' import { sendUserToast } from '$lib/toast' + import AclTargetPicker from '../datatableAcl/AclTargetPicker.svelte' + import PgAclEditor from '../datatableAcl/PgAclEditor.svelte' const ADMIN_ROLE = 'admin' @@ -49,11 +53,38 @@ const availableRoles: InstanceDatatableRole[] = $derived(info?.available_roles ?? []) const unusedRoles = $derived(availableRoles.filter((r) => !rows.some((row) => row.id === r.id))) + let aclSchema = $state(undefined) + let aclTable = $state(undefined) + const aclTarget: AclTarget = $derived( + aclSchema + ? aclTable + ? { kind: 'table', schema: aclSchema, table: aclTable } + : { kind: 'schema', schema: aclSchema } + : { kind: 'database' } + ) + let aclSchemas = $state([]) + let aclSchemasLoaded = $state(false) + let aclTables = $state([]) + + // The editor's read of a database lists its schemas, and of a schema its tables — which is what + // the picker offers, so the picker reads nothing of its own. A read for a target since left + // behind is dropped. + function onAclLoaded(target: AclTarget, loaded: DatatableAclInfo) { + if (JSON.stringify(target) !== JSON.stringify(aclTarget)) return + if (target.kind === 'database') { + aclSchemas = loaded.children + aclSchemasLoaded = true + } else if (target.kind === 'schema') aclTables = loaded.children + } + async function load() { loading = true loadError = undefined try { - const res = await WorkspaceService.getDatatablePermissions({ workspace, datatableName: datatable }) + const res = await WorkspaceService.getDatatablePermissions({ + workspace, + datatableName: datatable + }) info = res permissioned = res.permissioned defaultRole = res.default_role @@ -124,6 +155,11 @@ } export function open() { + aclSchema = undefined + aclTable = undefined + aclSchemas = [] + aclSchemasLoaded = false + aclTables = [] drawer?.openDrawer() load() } @@ -143,8 +179,11 @@ drawer?.closeDrawer()} - tooltip="A data table role is a Postgres login. A job that names one connects as it, and Postgres decides what it may touch — grant privileges with SQL. Roles are defined for the whole instance; here you say who may use each one on this data table." + tooltip="A data table role is a Postgres login. A job that names one connects as it, and Postgres decides what it may touch — grant it privileges under Access. Roles are defined for the whole instance; here you say who may use each one on this data table." > + {#snippet titleExtra()} + Beta + {/snippet} {#if loading}

Loading…

{:else if loadError} @@ -173,7 +212,7 @@ These data tables point at the same database with their own entry, so what you set here does not reach them:
    - {#each info.ungoverned_reachers as reacher} + {#each info.ungoverned_reachers as reacher (`${reacher.workspace_id}/${reacher.datatable}`)}
  • {reacher.workspace_id} / {reacher.datatable}
    • {/each}
@@ -194,7 +233,7 @@ {#if availableRoles.length === 0} Only admin can be used until a superadmin adds a data table - role in the data table settings page. + role, from Instance roles at the top of the data tables settings page. {/if} @@ -271,6 +310,34 @@
{/if} + + {#if info?.supported} +
+
+ Access + + What each role may do in Postgres, on the database, a schema or a table. Every + change shows the SQL it runs before running it. + +
+ aclSchema, + (s) => { + aclSchema = s + aclTables = [] + } + } + bind:table={aclTable} + /> + {#key JSON.stringify(aclTarget)} + + {/key} +
+ {/if}
{/if} diff --git a/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte b/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte index e46b578597..d100b1681a 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte @@ -3,7 +3,6 @@ import CloseButton from '../common/CloseButton.svelte' import TextInput from '../text_input/TextInput.svelte' import Toggle from '../Toggle.svelte' - import Tooltip from '../Tooltip.svelte' import ConfirmationModal from '../common/confirmationModal/ConfirmationModal.svelte' import { createAsyncConfirmationModal } from '../common/confirmationModal/asyncConfirmationModal.svelte' import Cell from '../table/Cell.svelte' @@ -83,16 +82,6 @@
-
-

Instance roles

- - A data table role is a real Postgres login on this instance, shared by every instance - database. A job that names one connects as it, and Postgres decides what it may touch — grant - it privileges with SQL. Which people may use a role on a given data table is set per data - table, in its roles drawer. - -
- {#if loadError} {loadError} {:else} diff --git a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte index 867cc58d87..53b4819689 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte @@ -92,8 +92,7 @@ type GetSettingsResponse, type TestDataTableConnectionResponse } from '$lib/gen' - // `superadmin` gates the commented-out roles section at the bottom; restore it there. - import { workspaceStore } from '$lib/stores' + import { enterpriseLicense, superadmin, workspaceStore } from '$lib/stores' import { createAsyncConfirmationModal } from '../common/confirmationModal/asyncConfirmationModal.svelte' import ConfirmationModal from '../common/confirmationModal/ConfirmationModal.svelte' import { resource } from 'runed' @@ -101,10 +100,8 @@ import { Popover } from '../meltComponents' import ExploreAssetButton from '../ExploreAssetButton.svelte' import DataTableMigrationsButton from './DataTableMigrationsButton.svelte' - // Both components are complete and reviewed; their call sites in this file are commented - // out until the ACL editor lands. Uncomment these with them. - // import DataTablePermissionsButton from './DataTablePermissionsButton.svelte' - // import DataTableRolesSection from './DataTableRolesSection.svelte' + import DataTablePermissionsButton from './DataTablePermissionsButton.svelte' + import InstanceRolesButton from './InstanceRolesButton.svelte' import { deepEqual } from 'fast-equals' import { clone } from '$lib/utils' import SettingsFooter from './SettingsFooter.svelte' @@ -318,7 +315,13 @@ title="Data tables" description="Relational storage the whole workspace shares under one name. Scripts, flows and apps address it as datatable://main instead of picking a PostgreSQL resource, so nobody needs access to the credentials to query it, and you can point that name at another database without touching a line of code. Browse and edit tables, and version schema changes as migrations, from here." link="https://www.windmill.dev/docs/core_concepts/persistent_storage/data_tables" -/> +> + {#snippet actions()} + {#if $superadmin && $enterpriseLicense && !isCloudHosted()} + + {/if} + {/snippet} + {#if isCloudHosted()} @@ -472,15 +475,6 @@ datatable={dataTable.name} disabled={!!dirtyMap[dataTable.name]} /> -
-{/if} ---> - + import { Badge, Button, Drawer, DrawerContent } from '../common' + import { Users } from 'lucide-svelte' + import DataTableRolesSection from './DataTableRolesSection.svelte' + + let drawer: Drawer | undefined = $state(undefined) + + + + + + drawer?.closeDrawer()} + tooltip="A data table role is a real Postgres login on this instance, shared by every instance database. A job that names one connects as it, and Postgres decides what it may touch. Which people may use a role on a given data table, and what it may do there, is set per data table, in its roles drawer." + > + {#snippet titleExtra()} + Beta + {/snippet} + + + From 9e0e835c6876305d3a44902fb1432fd9c2348a7c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 19 Sep 2026 23:26:28 +0200 Subject: [PATCH 31/34] fix: bump git sync hub scripts to cli 1.815.1 (#11239) Co-authored-by: Claude Opus 5 (1M context) --- backend/windmill-common/src/workspaces.rs | 4 ++-- frontend/src/lib/hubPaths.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 711b4dac29..df741a9394 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -184,7 +184,7 @@ pub enum ObjectType { DatatableMigration, } -pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28969/sync-script-to-git-repo-windmill"; +pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28971/sync-script-to-git-repo-windmill"; /// Hub script that applies a repository's state back into a workspace /// (the repo → Windmill / "pull" direction). Same script the UI runs from @@ -192,7 +192,7 @@ pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28969/sync-script-to-git-repo /// ignores the slug, so the slug is kept free of characters that would be /// percent-encoded into the run URL (a `:` becomes `%3A`, which some hardened /// reverse proxies reject as double-encoding when the client re-encodes it). -pub const GIT_SYNC_PULL_SCRIPT_PATH: &str = "hub/28957/git-sync-init-repository-windmill"; +pub const GIT_SYNC_PULL_SCRIPT_PATH: &str = "hub/28970/git-sync-init-repository-windmill"; /// Prefix used to identify fork workspaces. A workspace whose id starts with this string is a /// fork of another workspace. diff --git a/frontend/src/lib/hubPaths.json b/frontend/src/lib/hubPaths.json index dfa0be57c1..831380748e 100644 --- a/frontend/src/lib/hubPaths.json +++ b/frontend/src/lib/hubPaths.json @@ -1,6 +1,6 @@ { "gitSyncTest": "hub/28950/git-repo-test-read-write-windmill", - "gitInitRepo": "hub/28957/git-sync-init-repository-windmill", + "gitInitRepo": "hub/28970/git-sync-init-repository-windmill", "slackErrorHandler": "hub/28794/workspace-or-schedule-error-handler-slack", "emailErrorHandler": "hub/19795/workspace-or-error-handler-email", "slackRecoveryHandler": "hub/28791/slack/schedule-recovery-handler-slack", From 6186a0645d3f3e92e06b666a6b5774b8af2b488f Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Sat, 19 Sep 2026 23:29:14 +0200 Subject: [PATCH 32/34] feat: edit variables, resources and triggers in their own session tab (#11206) * feat: edit variables, resources and triggers in their own session tab Co-Authored-By: Claude Opus 5 (1M context) * fix: take a trigger's flow kind from its config and name the written trigger Co-Authored-By: Claude Opus 5 (1M context) * refactor: one operating-workspace context for editors acting on a session's workspace Co-Authored-By: Claude Opus 5 (1M context) * fix: reload page item tabs only in sessions acting on the tool's workspace Co-Authored-By: Claude Opus 5 (1M context) * fix: resolve open in workspace for every preview tab kind in one place Co-Authored-By: Claude Opus 5 (1M context) * fix: guard every component under a session editor against the navigation workspace Co-Authored-By: Claude Opus 5 (1M context) * fix: show a page item tab whose item is gone instead of mounting its editor Co-Authored-By: Claude Opus 5 (1M context) * fix: follow a saved page item in the tab's own workspace and restore in-frame rows Co-Authored-By: Claude Opus 5 (1M context) * feat: close a page item tab from its editor header Co-Authored-By: Claude Opus 5 (1M context) * fix: judge trigger permissions by the user acting in the session workspace Co-Authored-By: Claude Opus 5 (1M context) * fix: judge every session-editor permission by the user acting in that workspace Co-Authored-By: Claude Opus 5 (1M context) * fix: keep session permissions reactive to the acting role and restore tabs as last seen Co-Authored-By: Claude Opus 5 (1M context) * fix: default Path's owner controls to the user acting in its workspace Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Ruben Fiszel --- .../lib/components/AIProviderPicker.svelte | 6 +- frontend/src/lib/components/AgentTrace.svelte | 8 +- .../src/lib/components/ApiConnectForm.svelte | 6 +- .../src/lib/components/AppConnectInner.svelte | 7 +- frontend/src/lib/components/ArgInfo.svelte | 8 +- frontend/src/lib/components/ArgInput.svelte | 6 +- .../components/BedrockCredentialsCheck.svelte | 12 +- .../src/lib/components/ChannelSelector.svelte | 10 +- frontend/src/lib/components/CliHelpBox.svelte | 6 +- frontend/src/lib/components/DBManager.svelte | 8 +- .../lib/components/DBManagerContent.svelte | 12 +- frontend/src/lib/components/DBTable.svelte | 14 +- .../src/lib/components/DatatablePicker.svelte | 6 +- .../src/lib/components/DefaultScripts.svelte | 7 +- .../lib/components/DefaultScriptsInner.svelte | 7 +- .../src/lib/components/DucklakePicker.svelte | 12 +- .../lib/components/EditableSchemaForm.svelte | 6 +- frontend/src/lib/components/Editor.svelte | 22 +- frontend/src/lib/components/EditorBar.svelte | 11 +- .../src/lib/components/EditorHeader.svelte | 17 +- .../components/ErrorOrRecoveryHandler.svelte | 21 +- .../lib/components/ExploreAssetButton.svelte | 19 +- .../src/lib/components/FlowBuilder.svelte | 26 +- .../src/lib/components/FlowGraphViewer.svelte | 6 +- .../lib/components/FlowGraphViewerStep.svelte | 7 +- .../components/FlowHistoryJobPicker.svelte | 8 +- .../src/lib/components/FlowLogViewer.svelte | 6 +- .../FlowLoopIterationPreview.svelte | 6 +- .../lib/components/FlowPreviewContent.svelte | 6 +- .../lib/components/FlowRestartButton.svelte | 8 +- .../lib/components/FlowStatusViewer.svelte | 7 +- .../components/FlowStatusViewerInner.svelte | 20 +- frontend/src/lib/components/FlowViewer.svelte | 6 +- .../src/lib/components/FolderEditor.svelte | 34 ++- .../src/lib/components/FolderPicker.svelte | 10 +- .../components/GitHubAppIntegration.svelte | 54 ++-- .../lib/components/GitLabIntegration.svelte | 14 +- .../components/GitRepoPopoverPicker.svelte | 10 +- .../components/GitRepoResourcePicker.svelte | 6 +- .../src/lib/components/GitRepoViewer.svelte | 6 +- .../src/lib/components/GroupEditor.svelte | 43 +++- .../src/lib/components/HistoricInputs.svelte | 6 +- .../src/lib/components/HistoricList.svelte | 14 +- .../components/InputTransformPickers.svelte | 6 +- .../InputTransformSchemaForm.svelte | 6 +- frontend/src/lib/components/JobLoader.svelte | 6 +- .../src/lib/components/JobOtelTraces.svelte | 43 ++-- .../src/lib/components/LabelsInput.svelte | 6 +- .../LightweightResourcePicker.svelte | 8 +- frontend/src/lib/components/LogViewer.svelte | 10 +- .../lib/components/ModulePreviewForm.svelte | 6 +- frontend/src/lib/components/ModuleTest.svelte | 6 +- .../components/ParqetCsvTableRenderer.svelte | 9 +- .../lib/components/PasswordArgInput.svelte | 7 +- frontend/src/lib/components/Path.svelte | 56 +++-- .../components/PathNameAutocomplete.svelte | 11 +- .../lib/components/PermissionHistory.svelte | 14 +- .../src/lib/components/QueuePosition.svelte | 6 +- .../src/lib/components/ResourceEditor.svelte | 12 +- .../components/ResourceEditorDrawer.svelte | 71 ++++-- .../src/lib/components/ResourceForm.svelte | 7 +- .../src/lib/components/ResourcePicker.svelte | 6 +- .../lib/components/ResourceTypePicker.svelte | 8 +- .../components/ResourceVersionHistory.svelte | 6 +- .../src/lib/components/S3FilePicker.svelte | 6 +- .../lib/components/S3FilePickerInner.svelte | 6 +- .../src/lib/components/S3FilePreview.svelte | 6 +- .../lib/components/SaveInputsButton.svelte | 6 +- .../lib/components/SavedInputsPicker.svelte | 16 +- frontend/src/lib/components/SchemaForm.svelte | 6 +- .../src/lib/components/ScriptBuilder.svelte | 17 +- .../src/lib/components/ScriptEditor.svelte | 13 +- .../src/lib/components/ScriptPicker.svelte | 18 +- .../components/ScriptVersionHistory.svelte | 8 +- frontend/src/lib/components/SqlRepl.svelte | 6 +- .../lib/components/SummaryPathDisplay.svelte | 24 +- .../src/lib/components/TestConnection.svelte | 6 +- .../src/lib/components/VariableEditor.svelte | 43 +++- .../src/lib/components/VariableForm.svelte | 7 +- .../src/lib/components/WorkerTagPicker.svelte | 12 +- .../src/lib/components/WorkerTagSelect.svelte | 14 +- .../lib/components/WorkflowTimeline.svelte | 18 +- .../WorkspaceItemDrillPicker.svelte | 11 +- .../lib/components/aiEvals/EvalsPane.svelte | 6 +- .../apps/editor/AppEditorHeaderDeploy.svelte | 36 +-- .../apps/editor/AppJobsDrawer.svelte | 12 +- .../apps/editor/DeploymentHistory.svelte | 14 +- .../InlineScriptRunnableByPath.svelte | 12 +- .../mainInput/RunnableSelector.svelte | 13 +- .../mainInput/WorkspaceFlowList.svelte | 6 +- .../mainInput/WorkspaceScriptList.svelte | 6 +- .../AssetGraph/AssetGraphDetailsPane.svelte | 7 +- .../assets/AssetGraph/AssetNode.svelte | 15 +- .../assets/AssetGraph/AssetRunsPanel.svelte | 18 +- .../assets/AssetGraph/DataTablePreview.svelte | 14 +- .../AssetGraph/DucklakeResultPreview.svelte | 12 +- .../AssetGraph/DucklakeVersionPreview.svelte | 13 +- .../AssetGraph/PipelineScriptView.svelte | 6 +- .../AssetGraph/PipelineTriggerEditors.svelte | 13 +- .../assets/AssetGraph/RunnableNode.svelte | 6 +- .../assets/AssetsDropdownButton.svelte | 10 +- .../components/assets/JobAssetsViewer.svelte | 16 +- .../DraftChangesConfirmationModal.svelte | 9 +- .../common/drawer/DrawerContent.svelte | 34 ++- .../common/fileDownload/FileDownload.svelte | 6 +- .../common/fileUpload/FileUpload.svelte | 16 +- .../components/copilot/StepGenQuick.svelte | 6 +- .../copilot/chat/AIChatMessage.svelte | 8 +- .../copilot/chat/AIChatModelSettings.svelte | 18 +- .../chat/AssistantInstructionsSection.svelte | 10 +- .../chat/AssistantSettingsModal.svelte | 6 +- .../copilot/chat/ChatContextPicker.svelte | 6 +- .../chat/DatatableCreationPolicy.svelte | 6 +- .../copilot/chat/LinkRenderer.svelte | 8 +- .../copilot/chat/ToolMessageActions.svelte | 5 +- .../chat/createdResourceActions.svelte.ts | 15 +- .../components/copilot/chat/global/core.ts | 12 +- .../src/lib/components/copilot/chat/shared.ts | 10 +- .../src/lib/components/dbt/DbtEditor.svelte | 6 +- .../details/ErrorHandlerToggleButtonV2.svelte | 10 +- .../components/flows/FlowAssetsHandler.svelte | 6 +- .../lib/components/flows/FlowEditor.svelte | 15 +- .../components/flows/FlowHistoryInner.svelte | 6 +- .../flows/common/FlowCardHeader.svelte | 7 +- .../flows/content/AgentEditorModal.svelte | 6 +- .../flows/content/AgentResourceBar.svelte | 8 +- .../flows/content/AiAgentStepInputs.svelte | 6 +- .../flows/content/ExpandedSubflowStep.svelte | 6 +- .../flows/content/FlowEditorDrawer.svelte | 6 +- .../content/FlowEnvironmentVariables.svelte | 6 +- .../components/flows/content/FlowInput.svelte | 6 +- .../flows/content/FlowInputsFlow.svelte | 6 +- .../flows/content/FlowInputsQuick.svelte | 24 +- .../flows/content/FlowModuleComponent.svelte | 6 +- .../flows/content/FlowModuleScript.svelte | 6 +- .../flows/content/FlowModuleSuspend.svelte | 9 +- .../content/FlowModuleWorkerTagSelect.svelte | 4 +- .../flows/content/FlowPathViewer.svelte | 8 +- .../flows/content/FlowSettings.svelte | 16 +- .../flows/content/McpToolEditor.svelte | 5 +- .../flows/content/ScriptEditorDrawer.svelte | 6 +- .../WorkspaceScriptSettingsDrawer.svelte | 6 +- .../flows/conversations/FlowChat.svelte | 7 +- .../flows/map/FlowModuleSchemaMap.svelte | 6 +- .../flows/map/InsertModuleInner.svelte | 6 +- .../flows/pickers/PickHubScript.svelte | 7 +- .../flows/pickers/PickHubScriptQuick.svelte | 7 +- .../pickers/WorkspaceScriptPicker.svelte | 10 +- .../pickers/WorkspaceScriptPickerQuick.svelte | 11 +- .../flows/propPicker/StepHistory.svelte | 6 +- .../lib/components/graph/FlowGraphV2.svelte | 6 +- .../graph/renderers/nodes/AssetNode.svelte | 7 +- .../jobs/MissingWorkerTagAlert.svelte | 6 +- .../components/metrics/MetricsDrawer.svelte | 6 +- .../components/operatingWorkspace.svelte.ts | 48 ++++ .../lib/components/operatingWorkspace.test.ts | 119 +++++++++ .../propertyPicker/ObjectViewer.svelte | 14 +- .../propertyPicker/PropPicker.svelte | 8 +- .../raw_apps/DefaultDatabaseSelector.svelte | 7 +- .../raw_apps/RawAppDataTableDrawer.svelte | 7 +- .../components/raw_apps/RawAppEditor.svelte | 15 +- .../raw_apps/RawAppEditorHeader.svelte | 26 +- .../raw_apps/RawAppInlineScriptEditor.svelte | 7 +- .../RawAppInlineScriptRunnable.svelte | 15 +- .../raw_apps/RawAppInlineScriptsPanel.svelte | 7 +- .../raw_apps/RawAppInputsSpecEditor.svelte | 15 +- .../raw_apps/RawAppSharedUiDrawer.svelte | 7 +- .../raw_apps/RawAppTemplatePicker.svelte | 8 +- .../components/raw_apps/rawAppWorkspace.ts | 22 -- .../components/runs/JobDetailHeader.svelte | 24 +- .../runs/NoWorkerWithTagWarning.svelte | 6 +- .../src/lib/components/runs/RunBadges.svelte | 6 +- .../components/schema/JobSchemaPicker.svelte | 6 +- .../schema/RunningJobSchemaPicker.svelte | 62 +++-- .../components/scriptEditor/LogPanel.svelte | 12 +- .../sessions/OpenInSessionButton.svelte | 7 +- .../sessions/PageItemEditorView.svelte | 174 +++++++++++++ .../components/sessions/PreviewTabHost.svelte | 23 ++ .../sessions/SessionChangesBar.svelte | 4 +- .../sessions/SessionEditorTarget.svelte | 2 + .../components/sessions/pageDrawerSession.ts | 35 ++- .../lib/components/sessions/pageItemLookup.ts | 62 +++++ .../lib/components/sessions/previewPaths.ts | 81 ++++++ .../components/sessions/previewReload.test.ts | 21 ++ .../lib/components/sessions/previewReload.ts | 74 ++++-- .../components/sessions/previewRouter.test.ts | 21 +- .../lib/components/sessions/previewRouter.ts | 70 +++++- .../components/sessions/sessionMode.svelte.ts | 11 + .../sessions/sessionPreviewTabs.svelte.ts | 136 ++++++---- .../sessions/sessionPreviewTabs.test.ts | 236 +++++++----------- .../sessions/sessionRuntime.svelte.ts | 20 +- .../components/settings/CreateToken.svelte | 18 +- .../settings/EditTokenScopesModal.svelte | 6 +- .../settings/UserAIPromptsSettings.svelte | 7 +- .../triggers/AddTriggersButton.svelte | 10 +- .../components/triggers/CaptureSection.svelte | 10 +- .../components/triggers/CaptureTable.svelte | 11 +- .../components/triggers/CaptureWrapper.svelte | 10 +- .../triggers/PermissionedAsLine.svelte | 15 +- .../triggers/TestTriggerConnection.svelte | 7 +- .../triggers/TriggerEditorToolbar.svelte | 6 +- .../triggers/TriggerHistoryButton.svelte | 7 +- .../triggers/TriggerSuspendedJobsModal.svelte | 7 +- .../components/triggers/TriggerTokens.svelte | 8 +- .../components/triggers/TriggersEditor.svelte | 39 +-- .../amqp/AmqpEditorConfigSection.svelte | 7 +- .../amqp/AmqpTriggerEditorInner.svelte | 96 ++++--- .../AzureTriggerEditorConfigSection.svelte | 18 +- .../azure/AzureTriggerEditorInner.svelte | 113 ++++++--- .../triggers/email/DefaultEmailCapture.svelte | 6 +- .../email/DefaultEmailConfigSection.svelte | 9 +- .../triggers/email/DefaultEmailPanel.svelte | 16 +- .../triggers/email/EmailCapture.svelte | 6 +- .../EmailTriggerEditorConfigSection.svelte | 14 +- .../email/EmailTriggerEditorInner.svelte | 102 +++++--- .../triggers/email/EmailTriggerPanel.svelte | 8 +- .../components/triggers/gcp/GcpCapture.svelte | 6 +- .../gcp/GcpTriggerEditorConfigSection.svelte | 21 +- .../triggers/gcp/GcpTriggerEditorInner.svelte | 102 +++++--- .../triggers/http/OpenAPISpecGenerator.svelte | 9 +- .../triggers/http/RouteCapture.svelte | 6 +- .../triggers/http/RouteCorsOption.svelte | 4 +- .../http/RouteEditorConfigSection.svelte | 24 +- .../triggers/http/RouteEditorInner.svelte | 121 +++++---- .../triggers/http/RoutesGenerator.svelte | 9 +- .../triggers/http/RoutesPanel.svelte | 7 +- .../kafka/KafkaTriggerEditorInner.svelte | 102 +++++--- .../kafka/KafkaTriggersConfigSection.svelte | 8 +- .../mqtt/MqttEditorConfigSection.svelte | 7 +- .../mqtt/MqttTriggerEditorInner.svelte | 102 +++++--- .../native/NativeTriggerEditor.svelte | 41 ++- .../triggers/native/NativeTriggerTable.svelte | 9 +- .../services/github/GitHubTriggerForm.svelte | 10 +- .../google/GoogleCalendarPicker.svelte | 16 +- .../services/google/GoogleDrivePicker.svelte | 74 +++--- .../nextcloud/NextcloudTriggerForm.svelte | 10 +- .../nats/NatsTriggerEditorInner.svelte | 102 +++++--- .../nats/NatsTriggersConfigSection.svelte | 7 +- .../postgres/CheckPostgresRequirement.svelte | 15 +- .../PostgresTriggerEditorInner.svelte | 96 ++++--- .../postgres/PublicationPicker.svelte | 27 +- .../triggers/postgres/SlotPicker.svelte | 17 +- .../schedules/ScheduleEditorInner.svelte | 80 +++--- .../sqs/SqsTriggerEditorConfigSection.svelte | 7 +- .../triggers/sqs/SqsTriggerEditorInner.svelte | 102 +++++--- .../components/triggers/triggerWorkspace.ts | 29 --- .../triggers/webhook/WebhooksCapture.svelte | 6 +- .../webhook/WebhooksConfigSection.svelte | 8 +- .../websocket/WebsocketCapture.svelte | 26 +- .../WebsocketEditorConfigSection.svelte | 8 +- .../WebsocketTriggerEditorInner.svelte | 111 +++++--- .../lib/components/wizards/AppPicker.svelte | 33 ++- .../DataTableMigrationsButton.svelte | 8 +- .../src/routes/(root)/(logged)/+layout.svelte | 14 +- .../(root)/(logged)/resources/+page.svelte | 6 +- .../(root)/(logged)/sessions/+page.svelte | 93 ++++--- 256 files changed, 3455 insertions(+), 1774 deletions(-) create mode 100644 frontend/src/lib/components/operatingWorkspace.svelte.ts create mode 100644 frontend/src/lib/components/operatingWorkspace.test.ts delete mode 100644 frontend/src/lib/components/raw_apps/rawAppWorkspace.ts create mode 100644 frontend/src/lib/components/sessions/PageItemEditorView.svelte create mode 100644 frontend/src/lib/components/sessions/pageItemLookup.ts delete mode 100644 frontend/src/lib/components/triggers/triggerWorkspace.ts diff --git a/frontend/src/lib/components/AIProviderPicker.svelte b/frontend/src/lib/components/AIProviderPicker.svelte index 19b1346b84..f69bb6e8f4 100644 --- a/frontend/src/lib/components/AIProviderPicker.svelte +++ b/frontend/src/lib/components/AIProviderPicker.svelte @@ -3,11 +3,13 @@ import Select from './select/Select.svelte' import { fetchAvailableModels, AI_PROVIDERS } from './copilot/lib' import type { AIProvider, ProviderConfig } from '$lib/gen' - import { workspaceStore } from '$lib/stores' import ResourcePicker from './ResourcePicker.svelte' import Toggle from './Toggle.svelte' import { saveConfig, removeConfig, isSameAsStoredConfig } from './aiProviderStorage' import AIReasoningEffortPicker from './AIReasoningEffortPicker.svelte' + import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' + + const operatingWorkspace = useOperatingWorkspace() interface Props { value: ProviderConfig | undefined @@ -26,7 +28,7 @@ workspace = undefined }: Props = $props() - let effectiveWorkspace = $derived(workspace ?? $workspaceStore ?? '') + let effectiveWorkspace = $derived(workspace ?? $operatingWorkspace ?? '') let value = $derived.by(() => { if (!_uncheckedValue || typeof _uncheckedValue !== 'object') return undefined diff --git a/frontend/src/lib/components/AgentTrace.svelte b/frontend/src/lib/components/AgentTrace.svelte index 8c151e192c..91b87fea67 100644 --- a/frontend/src/lib/components/AgentTrace.svelte +++ b/frontend/src/lib/components/AgentTrace.svelte @@ -2,7 +2,6 @@ import { ExternalLink, Globe } from 'lucide-svelte' import { JobService, type Job } from '$lib/gen' import { base } from '$lib/base' - import { workspaceStore } from '$lib/stores' import { msToReadableTimeShort } from '$lib/utils' import ChatCollapsibleCard from './copilot/chat/ChatCollapsibleCard.svelte' import ToolContentDisplay from './copilot/chat/ToolContentDisplay.svelte' @@ -10,6 +9,9 @@ import GfmMarkdown from './GfmMarkdown.svelte' import type { AgentTraceEntry } from './agentTrace' import { SvelteMap, SvelteSet } from 'svelte/reactivity' + import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' + + const operatingWorkspace = useOperatingWorkspace() interface Props { entries: AgentTraceEntry[] @@ -40,7 +42,7 @@ try { jobs.set( jobId, - await JobService.getJob({ id: jobId, workspace: workspaceId ?? $workspaceStore! }) + await JobService.getJob({ id: jobId, workspace: workspaceId ?? $operatingWorkspace! }) ) } catch { // A tool job can be gone (retention) or unreadable. The row still has its @@ -121,7 +123,7 @@ {:else if entry.jobId} diff --git a/frontend/src/lib/components/ApiConnectForm.svelte b/frontend/src/lib/components/ApiConnectForm.svelte index dedeee90e2..2255817c5e 100644 --- a/frontend/src/lib/components/ApiConnectForm.svelte +++ b/frontend/src/lib/components/ApiConnectForm.svelte @@ -1,7 +1,6 @@ @@ -20,7 +22,7 @@ Setup the wmill cli for this workspace & remote:
diff --git a/frontend/src/lib/components/DBManager.svelte b/frontend/src/lib/components/DBManager.svelte index 7520cc5013..fc1bbb9752 100644 --- a/frontend/src/lib/components/DBManager.svelte +++ b/frontend/src/lib/components/DBManager.svelte @@ -1,5 +1,5 @@ -{#if $userStore?.is_admin || $userStore?.is_super_admin} +{#if actingUser?.is_admin || actingUser?.is_super_admin} diff --git a/frontend/src/lib/components/DefaultScriptsInner.svelte b/frontend/src/lib/components/DefaultScriptsInner.svelte index eaa3d989a0..b062fb4c49 100644 --- a/frontend/src/lib/components/DefaultScriptsInner.svelte +++ b/frontend/src/lib/components/DefaultScriptsInner.svelte @@ -1,10 +1,13 @@
@@ -46,9 +47,6 @@ {onClear} /> {#if showSchemaExplorer && value && assetCanBeExplored({ kind: 'ducklake', path: value })} - + {/if}
diff --git a/frontend/src/lib/components/EditableSchemaForm.svelte b/frontend/src/lib/components/EditableSchemaForm.svelte index bd3399d9d2..79173424d9 100644 --- a/frontend/src/lib/components/EditableSchemaForm.svelte +++ b/frontend/src/lib/components/EditableSchemaForm.svelte @@ -4,7 +4,6 @@ const bubble = createBubbler() import type { Schema } from '$lib/common' import { VariableService, type ScriptLang } from '$lib/gen' - import { workspaceStore } from '$lib/stores' import { Button } from './common' import ItemPicker from './ItemPicker.svelte' import VariableEditor from './VariableEditor.svelte' @@ -36,6 +35,9 @@ import Section from '$lib/components/Section.svelte' import Editor from './Editor.svelte' import AddPropertyV2 from './schema/AddPropertyV2.svelte' + import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' + + const operatingWorkspace = useOperatingWorkspace() // export let openEditTab: () => void = () => {} const dispatch = createEventDispatcher() @@ -128,7 +130,7 @@ workspace = undefined }: Props = $props() - let ws = $derived(workspace ?? $workspaceStore) + let ws = $derived(workspace ?? $operatingWorkspace) $effect.pre(() => { if (args == undefined) { diff --git a/frontend/src/lib/components/Editor.svelte b/frontend/src/lib/components/Editor.svelte index 987aa937eb..6aabc30c2b 100644 --- a/frontend/src/lib/components/Editor.svelte +++ b/frontend/src/lib/components/Editor.svelte @@ -42,7 +42,6 @@ import { editorConfig, registerWebviewPaste, updateOptions } from '$lib/editorUtils' import { editorFontSize } from '$lib/editorFontSize.svelte' import { createHash as randomHash } from '$lib/editorLangUtils' - import { workspaceStore } from '$lib/stores' import DdlMigrationGuard from './DdlMigrationGuard.svelte' import { type Preview, @@ -120,6 +119,9 @@ import { rawAppLintStore, type MonacoLintError } from './raw_apps/lintStore' import { MarkerSeverity } from 'monaco-editor' import { resource, useDebounce, watch } from 'runed' + import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' + + const operatingWorkspace = useOperatingWorkspace() // import EditorTheme from './EditorTheme.svelte' let divEl: HTMLDivElement | null = $state(null) @@ -684,7 +686,7 @@ // via a short-TTL cache — macros are late-bound, so mild staleness is fine. async function addWorkspaceMacroCompletions() { workspaceMacroCompletor?.dispose() - const workspace = $workspaceStore + const workspace = $operatingWorkspace if (!workspace) return let macros: Awaited> = [] try { @@ -739,7 +741,7 @@ provideCompletionItems: async function (model, position) { // Read the store per request, not at registration — the provider // outlives a workspace switch. - const workspace = $workspaceStore + const workspace = $operatingWorkspace if (!workspace) return { suggestions: [] } const before = model.getLineContent(position.lineNumber).slice(0, position.column - 1) if (!/^\s*(\/\/|--|#)\s*(column|data_test|on|materialize)\b/.test(before)) { @@ -780,7 +782,7 @@ $dbSchemas[resourcePath] = await getDbSchemas( lang === 'graphql' ? 'graphql' : (scriptLang ?? ''), resourcePath, - $workspaceStore, + $operatingWorkspace, (e) => console.error(`error getting ${lang} (${scriptLang}) db schema`, e), { customTag } ) @@ -1778,9 +1780,9 @@ let customTsTypesData = resource([() => lang], async () => { if (lang !== 'typescript') return undefined let datatables = ( - await WorkspaceService.listDataTables({ workspace: $workspaceStore ?? '' }) + await WorkspaceService.listDataTables({ workspace: $operatingWorkspace ?? '' }) ).map((d) => d.name) - let ducklakes = await WorkspaceService.listDucklakes({ workspace: $workspaceStore ?? '' }) + let ducklakes = await WorkspaceService.listDucklakes({ workspace: $operatingWorkspace ?? '' }) return { datatables, ducklakes } }) function setTypescriptCustomTypes() { @@ -1822,7 +1824,7 @@ scriptLang === 'nativets') ) { const resourceTypes = await ResourceService.listResourceType({ - workspace: $workspaceStore ?? '' + workspace: $operatingWorkspace ?? '' }) const namespace = formatResourceTypes( @@ -2023,7 +2025,7 @@ $lspTokenStore = newToken token = newToken } - let root = hostname + '/api/scripts_u/tokened_raw/' + $workspaceStore + '/' + token + let root = hostname + '/api/scripts_u/tokened_raw/' + $operatingWorkspace + '/' + token return root } @@ -2274,10 +2276,10 @@ -{#if datatableForMigrations && $workspaceStore} +{#if datatableForMigrations && $operatingWorkspace} {/if} diff --git a/frontend/src/lib/components/EditorBar.svelte b/frontend/src/lib/components/EditorBar.svelte index 91516843ab..81ef6c1c96 100644 --- a/frontend/src/lib/components/EditorBar.svelte +++ b/frontend/src/lib/components/EditorBar.svelte @@ -22,7 +22,6 @@ {/snippet} - +{/snippet} diff --git a/frontend/src/lib/components/ResourceForm.svelte b/frontend/src/lib/components/ResourceForm.svelte index df878280a7..1d4f091236 100644 --- a/frontend/src/lib/components/ResourceForm.svelte +++ b/frontend/src/lib/components/ResourceForm.svelte @@ -8,7 +8,7 @@ import { Alert, Skeleton } from './common' import Path from './Path.svelte' import LabelsInput from './LabelsInput.svelte' - import { workspaceStore, type UserExt } from '$lib/stores' + import { type UserExt } from '$lib/stores' import SchemaForm from './SchemaForm.svelte' import SimpleEditor from './SimpleEditor.svelte' import FilesetEditor from './FilesetEditor.svelte' @@ -22,6 +22,9 @@ import SyncResourceTypes from './SyncResourceTypes.svelte' import Label from './Label.svelte' import ResourcePathHint from './ResourcePathHint.svelte' + import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' + + const operatingWorkspace = useOperatingWorkspace() interface Props { path: string @@ -81,7 +84,7 @@ onCredentialStored }: Props = $props() - let ws = $derived(workspace ?? $workspaceStore) + let ws = $derived(workspace ?? $operatingWorkspace) let rawCode: string | undefined = $state(undefined) let textFileContent: string = $state('') diff --git a/frontend/src/lib/components/ResourcePicker.svelte b/frontend/src/lib/components/ResourcePicker.svelte index 06023b1bb4..778c1d2614 100644 --- a/frontend/src/lib/components/ResourcePicker.svelte +++ b/frontend/src/lib/components/ResourcePicker.svelte @@ -1,6 +1,5 @@ - clearPageDrawerAnchor(VARIABLES_PATH)}> +{#if inline} + {@render content()} +{:else} + clearPageDrawerAnchor(VARIABLES_PATH)}> + {@render content()} + +{/if} + +{#snippet content()} (inline ? onClose?.() : drawer?.closeDrawer())} > {#snippet banner()} {/snippet} - +{/snippet} diff --git a/frontend/src/lib/components/VariableForm.svelte b/frontend/src/lib/components/VariableForm.svelte index 61945eef08..63fdbf7425 100644 --- a/frontend/src/lib/components/VariableForm.svelte +++ b/frontend/src/lib/components/VariableForm.svelte @@ -10,10 +10,13 @@ import ToggleButton from './common/toggleButton-v2/ToggleButton.svelte' import { Loader2, RotateCcw } from 'lucide-svelte' import autosize from '$lib/autosize' - import { workspaceStore, type UserExt } from '$lib/stores' + import { type UserExt } from '$lib/stores' import { isOwner } from '$lib/utils' import { isEncryptedDraftValue } from '$lib/encryptedDraft' import EncryptedDraftField from './EncryptedDraftField.svelte' + import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' + + const operatingWorkspace = useOperatingWorkspace() interface Variable { value: string @@ -56,7 +59,7 @@ actingUser }: Props = $props() - let ws = $derived(workspace ?? $workspaceStore) + let ws = $derived(workspace ?? $operatingWorkspace) // Loading the deployed secret overwrites the draft row this form shares with the AI // chat, so every path that would trigger it has to be blocked while that row stages a diff --git a/frontend/src/lib/components/WorkerTagPicker.svelte b/frontend/src/lib/components/WorkerTagPicker.svelte index ccc3266ced..dae4967e05 100644 --- a/frontend/src/lib/components/WorkerTagPicker.svelte +++ b/frontend/src/lib/components/WorkerTagPicker.svelte @@ -2,6 +2,7 @@ import { Button } from '$lib/components/common' import { ExternalLink, RotateCw, Loader2 } from 'lucide-svelte' import { workerTags, workspaceStore } from '$lib/stores' + import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' import AssignableTags from './AssignableTags.svelte' import { WorkerService } from '$lib/gen' import WorkerTagSelect from './WorkerTagSelect.svelte' @@ -11,8 +12,8 @@ popupPlacement?: 'bottom-end' | 'top-end' disabled?: boolean placeholder?: string - // Workspace to read tags from; defaults to $workspaceStore. A fork-scoped - // session passes its effective workspace so the picker matches the deploy target. + // Workspace to read tags from; defaults to the operating workspace (see + // `useOperatingWorkspace`). workspaceId?: string } @@ -26,8 +27,11 @@ // See WorkerTagSelect: the shared `workerTags` cache is navigation-scoped, so a // different target workspace reads/writes a local list to avoid clobbering it. - let effectiveWorkspace = $derived(workspaceId ?? $workspaceStore) - let usesLocal = $derived(workspaceId != undefined && workspaceId !== $workspaceStore) + const operatingWorkspace = useOperatingWorkspace() + let effectiveWorkspace = $derived(workspaceId ?? $operatingWorkspace) + let usesLocal = $derived( + effectiveWorkspace != undefined && effectiveWorkspace !== $workspaceStore + ) let localWorkerTags = $state(undefined) let currentTags = $derived(usesLocal ? localWorkerTags : $workerTags) diff --git a/frontend/src/lib/components/WorkerTagSelect.svelte b/frontend/src/lib/components/WorkerTagSelect.svelte index fe00f07f9d..624aeb1bcd 100644 --- a/frontend/src/lib/components/WorkerTagSelect.svelte +++ b/frontend/src/lib/components/WorkerTagSelect.svelte @@ -1,5 +1,6 @@ + +{#snippet loading()} +
+ +
+{/snippet} + +
+ {#if eeLocked} +
This trigger requires an enterprise license.
+ {:else if lookup.loading || !lookup.current} + {@render loading()} + {:else if !lookup.current.exists} +
+

+ {pageItemKindLabel(item)} {item.path} no longer exists in this + workspace. +

+ +
+ {:else} + {#key `${item.kind}:${triggerKey}:${item.path}:${workspaceId}:${reloadNonce}:${savedNonce}`} + {#if item.kind === 'variable'} + + {:else if item.kind === 'resource'} + { + if (path !== undefined) onSaved(path) + }} + onRestored={() => savedNonce++} + /> + {:else if triggerKey} + {#await TRIGGER_EDITORS[triggerKey]()} + {@render loading()} + {:then Module} + onSaved(path)} + /> + {/await} + {/if} + {/key} + {/if} +
diff --git a/frontend/src/lib/components/sessions/PreviewTabHost.svelte b/frontend/src/lib/components/sessions/PreviewTabHost.svelte index 3dee7dd1b3..b88907bf02 100644 --- a/frontend/src/lib/components/sessions/PreviewTabHost.svelte +++ b/frontend/src/lib/components/sessions/PreviewTabHost.svelte @@ -103,12 +103,19 @@ applyPageIframeTheme(darkMode) }) + // A page item's editor reads its draft only when it loads, so a reload remounts it. + let pageItemReloadNonce = $state(0) + export function reload() { // A live editor shares the runtime store the chat mutates, so generic chat // edits are already reflected — no reload needed. Deploys refresh it via // each editor view's onDeploy → runtime.syncPreviewWithDeployed. So only the // iframe fallback (a separate page) has to be told to refresh. if (slot.kind === 'editor') return + if (slot.kind === 'pageitem') { + pageItemReloadNonce++ + return + } try { const win = frame?.contentWindow if (!win) return @@ -311,6 +318,22 @@ {/await} {/if}
+{:else if slot.kind === 'pageitem' && mounted && runtime} +
+ + {#if overlayHostEl} + {#await import('./PageItemEditorView.svelte')} + {@render editorLoading()} + {:then Module} + + {/await} + {/if} +
{:else if slot.kind === 'artifact' && mounted}
workspaceId) // This tab's storage path, for the editor below: several tabs are mounted at // once and only this one knows which item each is open on. diff --git a/frontend/src/lib/components/sessions/pageDrawerSession.ts b/frontend/src/lib/components/sessions/pageDrawerSession.ts index ca7379be20..16d8d9204a 100644 --- a/frontend/src/lib/components/sessions/pageDrawerSession.ts +++ b/frontend/src/lib/components/sessions/pageDrawerSession.ts @@ -11,6 +11,7 @@ import type { UserDraftItemKind } from '$lib/gen' // flow editors, where pulling the filter schemas that module reads views from would make // every trigger's save utils eager. import { + drawerHashFor, pageHref, stripBase, TRIGGER_PAGES, @@ -20,6 +21,7 @@ import { type TriggerKind } from './previewPaths' import type { OpenInSessionSource } from './OpenInSessionButton.svelte' +import { isSessionPreviewFrame } from './sessionMode.svelte' // The draft each page's drawer edits. The preview loads the page in its own // document and reads the draft back from the server, so opening a session has to @@ -63,11 +65,6 @@ async function flushOrRefuse(query: Parameters[0 } } -// How each page addresses a row in its hash. Resources route theirs through an extra -// segment; every other page names the path directly. -const drawerHashFor = (pagePath: string, itemPath: string) => - pagePath === RESOURCES_PATH ? `/resource/${itemPath}` : itemPath - /** * Deep-link the row whose drawer just opened, so the location says what is on screen — a * drawer opened from a row's Edit button is as open as one reached by link, and the chat @@ -87,6 +84,34 @@ export function setPageDrawerAnchor(pagePath: string, itemPath: string | undefin history.replaceState(history.state, '', `${pathname}${search}${anchor}`) } +/** + * Inside a session preview frame, hand a list page row up to the session, which edits it in + * a tab of its own. True when handed off: the caller must then not open its drawer. False + * off that page, and outside a preview frame, where the drawer is how the row is edited. + */ +export function handOffPageDrawer(pagePath: string, itemPath: string | undefined): boolean { + if (!itemPath || !isSessionPreviewFrame()) return false + if (stripBase(window.location.pathname) !== pagePath) return false + try { + window.parent.postMessage( + { type: 'wm.session.openPageItem', pagePath, path: itemPath }, + window.location.origin + ) + } catch { + return false + } + // A frame left on a row's hash claims a row nobody has open here, and reopens its tab on + // every reload. Not through the router: these pages open their drawer from the hash. Once + // more after the event: a row link's `href="#"` lands after its click handler. + const dropAnchor = () => { + const { pathname, search, hash } = window.location + if (hash) history.replaceState(history.state, '', `${pathname}${search}`) + } + dropAnchor() + setTimeout(dropAnchor, 0) + return true +} + /** * Drop the row a list page deep-links, once its drawer closes. The hash is how the row was * requested; leaving it behind makes the location claim a drawer that is no longer open — diff --git a/frontend/src/lib/components/sessions/pageItemLookup.ts b/frontend/src/lib/components/sessions/pageItemLookup.ts new file mode 100644 index 0000000000..bee3fc41c1 --- /dev/null +++ b/frontend/src/lib/components/sessions/pageItemLookup.ts @@ -0,0 +1,62 @@ +import { + AmqpTriggerService, + ApiError, + AzureTriggerService, + EmailTriggerService, + GcpTriggerService, + HttpTriggerService, + KafkaTriggerService, + MqttTriggerService, + NatsTriggerService, + PostgresTriggerService, + ResourceService, + ScheduleService, + SqsTriggerService, + VariableService, + WebsocketTriggerService +} from '$lib/gen' +import type { PageItemRef, TriggerKind } from './previewPaths' + +type Get = (args: { workspace: string; path: string; getDraft: boolean }) => Promise + +const TRIGGER_GETS: Record = { + http: (a) => HttpTriggerService.getHttpTrigger(a), + websocket: (a) => WebsocketTriggerService.getWebsocketTrigger(a), + postgres: (a) => PostgresTriggerService.getPostgresTrigger(a), + kafka: (a) => KafkaTriggerService.getKafkaTrigger(a), + nats: (a) => NatsTriggerService.getNatsTrigger(a), + mqtt: (a) => MqttTriggerService.getMqttTrigger(a), + amqp: (a) => AmqpTriggerService.getAmqpTrigger(a), + sqs: (a) => SqsTriggerService.getSqsTrigger(a), + gcp: (a) => GcpTriggerService.getGcpTrigger(a), + azure: (a) => AzureTriggerService.getAzureTrigger(a), + email: (a) => EmailTriggerService.getEmailTrigger(a) +} + +function getFor(ref: PageItemRef): Get { + switch (ref.kind) { + case 'variable': + return (a) => VariableService.getVariable(a) + case 'resource': + return (a) => ResourceService.getResource(a) + case 'schedule': + return (a) => ScheduleService.getSchedule(a) + case 'trigger': + return TRIGGER_GETS[ref.triggerKind] + } +} + +/** A page item as its editor would load it — deployed, or only a draft — or undefined when it + * is neither: deleted, or a draft that was discarded. Any other failure is thrown, for the + * caller to leave to the editor rather than report as a missing item. */ +export async function lookupPageItem( + ref: PageItemRef, + workspace: string +): Promise | undefined> { + try { + return (await getFor(ref)({ workspace, path: ref.path, getDraft: true })) as Record + } catch (e) { + if (e instanceof ApiError && e.status === 404) return undefined + throw e + } +} diff --git a/frontend/src/lib/components/sessions/previewPaths.ts b/frontend/src/lib/components/sessions/previewPaths.ts index 8d10dc1a32..13d5abca33 100644 --- a/frontend/src/lib/components/sessions/previewPaths.ts +++ b/frontend/src/lib/components/sessions/previewPaths.ts @@ -48,6 +48,87 @@ export const TRIGGER_PAGES: Record + pagePath === RESOURCES_PATH ? `/resource/${itemPath}` : itemPath + +/** The full page a page item is edited on: its list page, with the row's drawer open. */ +export function pageItemPageHref(ref: PageItemRef): string { + const listPath = pageItemListPath(ref) + return `${pageHref(listPath)}#${drawerHashFor(listPath, ref.path)}` +} + +/** The list page a page item is edited from. */ +export function pageItemListPath(ref: PageItemRef): string { + switch (ref.kind) { + case 'variable': + return VARIABLES_PATH + case 'resource': + return RESOURCES_PATH + case 'schedule': + return SCHEDULES_PATH + case 'trigger': + return TRIGGER_PAGES[ref.triggerKind].path + } +} + +/** The page item a list page's row names, or undefined for a page that lists none. */ +export function pageItemForListPath(pagePath: string, path: string): PageItemRef | undefined { + const clean = stripBase(pagePath) + if (clean === VARIABLES_PATH) return { kind: 'variable', path } + if (clean === RESOURCES_PATH) return { kind: 'resource', path } + if (clean === SCHEDULES_PATH) return { kind: 'schedule', path } + const trigger = Object.entries(TRIGGER_PAGES).find(([, p]) => p.path === clean) + return trigger ? { kind: 'trigger', triggerKind: trigger[0] as TriggerKind, path } : undefined +} + +const PAGE_ITEM_ROUTE = /^pageitem:(variable|resource|schedule|trigger\.([a-z]+))\/([^?#]+)$/ + +// A scheme rather than a path, like artifacts: the tab mounts the item's editor in process, +// so there is no page a frame could load. The path is encoded whole, so its slashes cannot +// be read as part of the scheme. +export function pageItemUrl(ref: PageItemRef): string { + const kind = ref.kind === 'trigger' ? `trigger.${ref.triggerKind}` : ref.kind + return `pageitem:${kind}/${encodeURIComponent(ref.path)}` +} + +export function parsePageItemRoute(url: string): PageItemRef | null { + const m = url.match(PAGE_ITEM_ROUTE) + if (!m) return null + let path: string + try { + path = decodeURIComponent(m[3]) + } catch { + return null + } + if (m[2] !== undefined) { + if (!(m[2] in TRIGGER_PAGES)) return null + return { kind: 'trigger', triggerKind: m[2] as TriggerKind, path } + } + return { kind: m[1] as 'variable' | 'resource' | 'schedule', path } +} + +/** Singular human name of a page item's kind, e.g. "Kafka trigger". */ +export function pageItemKindLabel(ref: PageItemRef): string { + switch (ref.kind) { + case 'variable': + return 'Variable' + case 'resource': + return 'Resource' + case 'schedule': + return 'Schedule' + case 'trigger': + return TRIGGER_PAGES[ref.triggerKind].label.replace(/s$/, '') + } +} + /** Label a trigger list page from its (base-stripped) pathname, or undefined. */ export function triggerLabelForPath(path: string): string | undefined { const clean = stripBase(path) diff --git a/frontend/src/lib/components/sessions/previewReload.test.ts b/frontend/src/lib/components/sessions/previewReload.test.ts index de11805b9a..ce9491d334 100644 --- a/frontend/src/lib/components/sessions/previewReload.test.ts +++ b/frontend/src/lib/components/sessions/previewReload.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest' import { toolReloadEffect, tabsToReload } from './previewReload' import type { SessionPreviewTab } from './sessionState.svelte' +import { pageItemUrl } from './previewPaths' describe('toolReloadEffect', () => { it('maps a non-item mutation to its own list page only', () => { @@ -52,6 +53,26 @@ describe('toolReloadEffect', () => { }) }) +describe('page item tabs', () => { + const tab = (url: string): SessionPreviewTab => ({ id: url, url, loc: url }) + const kafkaA = tab('pageitem:trigger.kafka/u%2Fme%2Fa') + const kafkaB = tab('pageitem:trigger.kafka/u%2Fme%2Fb') + const list = tab('/kafka_triggers') + + it('reloads only the trigger a write names, and its list page', () => { + const { pages, items } = toolReloadEffect('write_trigger', { + kind: 'kafka', + config: { path: 'u/me/a' } + }) + const named = new Set(items.map((i) => pageItemUrl(i))) + expect(tabsToReload([kafkaA, kafkaB, list], new Set(pages), named)).toEqual([kafkaA, list]) + }) + + it('reloads every tab of the kind when the tool names no item', () => { + expect(tabsToReload([kafkaA, kafkaB], new Set(['/kafka_triggers']))).toEqual([kafkaA, kafkaB]) + }) +}) + describe('tabsToReload', () => { const scheduleTab: SessionPreviewTab = { id: 's', url: '/schedules', loc: '/schedules' } const resourceTab: SessionPreviewTab = { id: 'r', url: '/resources', loc: '/resources' } diff --git a/frontend/src/lib/components/sessions/previewReload.ts b/frontend/src/lib/components/sessions/previewReload.ts index 6e9d750a1f..bdfbeaa3ff 100644 --- a/frontend/src/lib/components/sessions/previewReload.ts +++ b/frontend/src/lib/components/sessions/previewReload.ts @@ -1,6 +1,14 @@ import type { SessionPreviewTab } from './sessionState.svelte' import { whereIs } from './sessionPreviewTabs.svelte' -import { stripBase, TRIGGER_PAGES, type TriggerKind } from './previewPaths' +import { + pageItemListPath, + pageItemUrl, + parsePageItemRoute, + stripBase, + TRIGGER_PAGES, + type PageItemRef, + type TriggerKind +} from './previewPaths' // Which list pages a completed chat tool can change, as base-stripped paths // (e.g. `/schedules`). This allowlist is the single source of truth for "does @@ -13,23 +21,27 @@ import { stripBase, TRIGGER_PAGES, type TriggerKind } from './previewPaths' // deliberately absent: every editable item is a live in-process editor that // self-syncs from the store the chat mutates, so its tab needs no reload — and // no list page we preview lists open drafts. They fall through to NO_RELOAD. -// This "live editors self-sync, only list pages reload" invariant is the reason -// the callers below and in the sessions page reload nothing for item tabs. -export type ToolReloadEffect = { pages: string[] } -const NO_RELOAD: ToolReloadEffect = { pages: [] } +// +// Page items (variables, resources, schedules, triggers) are the exception among +// in-process tabs: their editors read a draft only when they open, so a write to +// one reloads its tab too. `items` names it when the tool's args do; without a +// path, every tab of that kind reloads. +export type ToolReloadEffect = { pages: string[]; items: PageItemRef[] } +const NO_RELOAD: ToolReloadEffect = { pages: [], items: [] } export function toolReloadEffect(name: string, args: any): ToolReloadEffect { switch (name) { case 'write_schedule': - return { pages: ['/schedules'] } + return withItem(['/schedules'], itemRef('schedule', args)) case 'write_trigger': - return { pages: triggerPages(args?.kind) } + // Its path sits in the trigger's own config, not beside `kind`. + return withItem(triggerPages(args?.kind), itemRef('trigger', args?.config, args?.kind)) case 'write_resource': - return { pages: ['/resources'] } + return withItem(['/resources'], itemRef('resource', args)) case 'write_variable': - return { pages: ['/variables'] } + return withItem(['/variables'], itemRef('variable', args)) case 'create_folder': - return { pages: ['/folders'] } + return { pages: ['/folders'], items: [] } // Generic item tools carry a workspace-item `type`; refresh its list page // when it lives on one (schedule/resource/variable/trigger). script/flow/app // have their own live editor tab and no previewed list page → nothing. @@ -37,12 +49,31 @@ export function toolReloadEffect(name: string, args: any): ToolReloadEffect { case 'discard_local_draft': case 'deploy_workspace_item': case 'rebase_draft': - return { pages: pagesForItemType(args?.type, args) } + return withItem( + pagesForItemType(args?.type, args), + itemRef(args?.type, args, args?.trigger_kind) + ) default: return NO_RELOAD } } +function withItem(pages: string[], item: PageItemRef | undefined): ToolReloadEffect { + return { pages, items: item && pages.length ? [item] : [] } +} + +function itemRef(type: unknown, args: any, triggerKind?: unknown): PageItemRef | undefined { + const path = args?.path + if (typeof path !== 'string' || !path) return undefined + if (type === 'variable' || type === 'resource' || type === 'schedule') { + return { kind: type, path } + } + if (type === 'trigger' && (triggerKind as string) in TRIGGER_PAGES) { + return { kind: 'trigger', triggerKind: triggerKind as TriggerKind, path } + } + return undefined +} + function pagesForItemType(type: unknown, args: any): string[] { switch (type) { case 'schedule': @@ -63,14 +94,23 @@ function triggerPages(kind: unknown): string[] { return page ? [page.path] : [] } -// The open tabs a page-reload should refresh: those whose observed page path is -// in `pages`. Item-editor and pipeline tab routes are never list pages, so they -// never match (see the self-sync invariant above). Pure over a tab snapshot so -// the sessions page can reload by id and this stays unit-testable. +// The open tabs a reload should refresh: list-page tabs whose observed page path is +// in `pages`, and page item tabs on those pages — only the named ones when a tool +// named its item. Item-editor and pipeline tab routes are never list pages, so they +// never match (see the self-sync invariant above). Pure over a tab snapshot so the +// sessions page can reload by id and this stays unit-testable. export function tabsToReload( tabs: SessionPreviewTab[], - pages: ReadonlySet + pages: ReadonlySet, + items: ReadonlySet = new Set() ): SessionPreviewTab[] { if (pages.size === 0) return [] - return tabs.filter((t) => pages.has(stripBase(whereIs(t)))) + return tabs.filter((t) => { + const pageItem = parsePageItemRoute(t.url) + if (!pageItem) return pages.has(stripBase(whereIs(t))) + const listPath = pageItemListPath(pageItem) + if (!pages.has(listPath)) return false + const named = [...items].some((u) => pageItemListPath(parsePageItemRoute(u)!) === listPath) + return !named || items.has(pageItemUrl(pageItem)) + }) } diff --git a/frontend/src/lib/components/sessions/previewRouter.test.ts b/frontend/src/lib/components/sessions/previewRouter.test.ts index 7d56f9e717..3bc76882e5 100644 --- a/frontend/src/lib/components/sessions/previewRouter.test.ts +++ b/frontend/src/lib/components/sessions/previewRouter.test.ts @@ -14,8 +14,27 @@ import { previewLocationContext, previewLocationLabel, resolvePreviewTab, - runFormUrl + runFormUrl, + workspacePageHref } from './previewRouter' +import { pageItemUrl } from './previewPaths' + +describe('workspacePageHref', () => { + it('sends a page item tab to its list page with the row open, never to its scheme', () => { + expect(workspacePageHref(pageItemUrl({ kind: 'resource', path: 'u/me/db' }))).toBe( + '/resources#/resource/u/me/db' + ) + expect( + workspacePageHref(pageItemUrl({ kind: 'trigger', triggerKind: 'kafka', path: 'f/a/b' })) + ).toBe('/kafka_triggers#f/a/b') + }) + + it('has no page for a tab that belongs to the chat', () => { + expect(workspacePageHref(artifactUrl('a1', 'Plan'))).toBeUndefined() + expect(workspacePageHref(runFormUrl('call_1', 'Run'))).toBeUndefined() + expect(workspacePageHref('/runs?path=u/me/x')).toBe('/runs?path=u/me/x') + }) +}) describe('drawerAnchorFor', () => { it('reads the anchored row on the pages that deep-link one', () => { diff --git a/frontend/src/lib/components/sessions/previewRouter.ts b/frontend/src/lib/components/sessions/previewRouter.ts index 9f312ea389..13e7626e70 100644 --- a/frontend/src/lib/components/sessions/previewRouter.ts +++ b/frontend/src/lib/components/sessions/previewRouter.ts @@ -3,8 +3,13 @@ import { AUDIT_LOGS_PATH, FOLDERS_PATH, GROUPS_PATH, + pageItemForListPath, + pageItemListPath, + pageItemPageHref, + pageItemUrl, pageKey, pageHref, + parsePageItemRoute, parsePreviewItemRoute, RESOURCES_PATH, RUNS_PATH, @@ -14,17 +19,22 @@ import { WORKSPACE_SETTINGS_PATH, triggerLabelForPath, TRIGGER_PAGES, + type PageItemRef, type PreviewItemRoute, type TriggerKind } from './previewPaths' // Re-exported so the preview code that already reads locations through this module keeps // one import, while a caller needing only a path can reach for the leaf instead. export { + pageItemListPath, + pageItemUrl, pageKey, pageHref, + parsePageItemRoute, parsePreviewItemRoute, stripBase, TRIGGER_PAGES, + type PageItemRef, type PreviewItemRoute, type TriggerKind } @@ -68,6 +78,7 @@ export type PreviewTarget = | { type: 'item'; item: WorkspaceItem } | { type: 'artifact'; id: string; name: string; version?: ArtifactVersionTarget } | { type: 'runform'; toolCallId: string; label: string } + | { type: 'pageitem'; ref: PageItemRef } export type PreviewPage = { label: string; path: string; icon: DrillIcon } @@ -117,6 +128,26 @@ export function drawerAnchorFor(location: string): string | undefined { return location.slice(hashAt + 1).replace(/^\/resource\//, '') || undefined } +/** The item a list-page location deep-links, as a tab of its own: a session edits these + * in process, so the list page's drawer is never where one belongs. */ +export function pageItemForLocation(location: string): PageItemRef | undefined { + const anchor = drawerAnchorFor(location) + if (!anchor) return undefined + let path: string + try { + path = decodeURIComponent(anchor) + } catch { + return undefined + } + return pageItemForListPath(location, path) +} + +/** A location with a deep-linked row replaced by that row's own tab; any other unchanged. */ +export function pageItemLocation(location: string): string { + const ref = pageItemForLocation(location) + return ref ? pageItemUrl(ref) : location +} + // Query params the preview host injects into an iframe URL (`nomenubar` hides the nav, // `workspace` scopes the page). Never part of what a location means. const INJECTED_PARAMS = ['nomenubar', 'workspace'] as const @@ -126,7 +157,7 @@ const INJECTED_PARAMS = ['nomenubar', 'workspace'] as const export function canonicalizeObservedLoc(loc: string): string { // An artifact or a run form is a scheme, not a path — `new URL` would happily parse it // and hand back a pathname with the scheme gone. - if (parseArtifactRoute(loc) || parseRunFormRoute(loc)) return loc + if (parseArtifactRoute(loc) || parseRunFormRoute(loc) || parsePageItemRoute(loc)) return loc try { const u = new URL(loc, 'http://_') for (const p of INJECTED_PARAMS) u.searchParams.delete(p) @@ -201,6 +232,8 @@ export function describeLocation(loc: string): PreviewLocation { // Identity is the call, never the label: that carries the script's summary, so folding it // in would open a second tab for the same form whenever the summary differed. if (runForm) return { identity: `runform:${runForm.toolCallId}`, view: '', anchor: '' } + const pageItem = parsePageItemRoute(loc) + if (pageItem) return { identity: pageItemUrl(pageItem), view: '', anchor: '' } const canonical = canonicalizeObservedLoc(loc) const path = stripBase(canonical) const bare = canonical.split('#')[0] @@ -322,6 +355,15 @@ export function previewLocationContext(loc: string): { location: string open?: string } { + // Told as its list page with the item open, the shape the model already reads for a row + // whose drawer is open — which is all a page item tab is to it. + const pageItem = parsePageItemRoute(loc) + if (pageItem) { + return { + ...previewLocationContext(pageItemListPath(pageItem)), + open: promptSafe(pageItem.path) + } + } const { identity, anchor } = describeLocation(loc) const bare = canonicalizeObservedLoc(loc).split('#')[0] const query = bare.includes('?') ? bare.slice(bare.indexOf('?') + 1) : '' @@ -370,6 +412,8 @@ export function previewLocationLabel(url: string): string { if (artifact) return artifact.name || 'Artifact' const runForm = parseRunFormRoute(url) if (runForm) return runForm.label || 'Run form' + const pageItem = parsePageItemRoute(url) + if (pageItem) return pageItem.path.split('/').pop() || pageItem.path const page = matchReusablePage(url) if (page) return page.label const trigger = triggerLabelForPath(url) @@ -492,6 +536,7 @@ export type PreviewSlot = | { kind: 'editor'; editorKind: SessionTargetKind | 'pipeline'; path: string } | { kind: 'artifact'; id: string; version?: number } | { kind: 'runform'; toolCallId: string } + | { kind: 'pageitem'; ref: PageItemRef } | { kind: 'iframe' } export function resolvePreviewTab(url: string): PreviewSlot { @@ -499,6 +544,8 @@ export function resolvePreviewTab(url: string): PreviewSlot { if (artifact) return { kind: 'artifact', id: artifact.id, version: artifact.version } const runForm = parseRunFormRoute(url) if (runForm) return { kind: 'runform', toolCallId: runForm.toolCallId } + const pageItem = parsePageItemRoute(url) + if (pageItem) return { kind: 'pageitem', ref: pageItem } const pipelineFolder = parsePipelineRoute(url) if (pipelineFolder) { return { kind: 'editor', editorKind: 'pipeline', path: pipelineFolder } @@ -516,3 +563,24 @@ export function resolvePreviewTab(url: string): PreviewSlot { if (!editorKind) return { kind: 'iframe' } return { kind: 'editor', editorKind, path: route.itemPath } } + +/** The full workspace page showing what a tab shows ("Open in workspace"), or undefined when + * there is none. Every tab kind answers here, so a new one cannot fall through to its url being + * navigated as a path — an artifact or page item url is a scheme, not a route. */ +export function workspacePageHref(location: string): string | undefined { + const slot = resolvePreviewTab(location) + switch (slot.kind) { + case 'artifact': + case 'runform': + return undefined + case 'pageitem': + return pageItemPageHref(slot.ref) + case 'editor': + case 'iframe': + return location + default: { + const unhandled: never = slot + return unhandled + } + } +} diff --git a/frontend/src/lib/components/sessions/sessionMode.svelte.ts b/frontend/src/lib/components/sessions/sessionMode.svelte.ts index a4972a9afb..a2c20e08a8 100644 --- a/frontend/src/lib/components/sessions/sessionMode.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionMode.svelte.ts @@ -47,6 +47,17 @@ export function withMenuHidden(url: string, workspaceId?: string): string { } } +// True when this window is a sessions-preview iframe: embedded, with the `nomenubar` flag +// the preview always sets and the logged layout stickies into sessionStorage. +export function isSessionPreviewFrame(): boolean { + if (typeof window === 'undefined' || window.self === window.top) return false + try { + return sessionStorage.getItem('nomenubar_embedded') === 'true' + } catch { + return false + } +} + // Append `?workspace=` to a canonical route so a full-page navigation (e.g. // "Open in workspace") lands on the session's effective workspace instead of // the navigation workspace. Unlike withMenuHidden, the menu is kept visible — diff --git a/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts b/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts index a9db01e0fa..a90eb83cc4 100644 --- a/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts @@ -8,7 +8,10 @@ import { describeLocation, matchPreviewPage, showsView, + pageItemLocation, + pageItemUrl, parseArtifactRoute, + parsePageItemRoute, parsePipelineRoute, previewLocationContext, promptSafe, @@ -24,6 +27,12 @@ import { import type { SessionPreviewTab, SessionTarget } from './sessionState.svelte' import type { Kind } from '$lib/utils_deployable' import { pipelineFolderFromBundlePath } from '$lib/pipelinePaths' +import { + pageItemKindLabel, + TRIGGER_PAGES, + type PageItemRef, + type TriggerKind +} from './previewPaths' // The single live owner of a session's preview tabs. Runs behind a small // interface both the sessions page (renderer) and the `open_preview` tool cross, @@ -78,7 +87,10 @@ function keptVersion( // scheme. `onto` is the tab about to be written, passed wherever one is being re-pointed so // that every such path keeps its pin. function targetUrl(target: PreviewTarget, onto?: SessionPreviewTab): string { - if (target.type === 'page') return target.href + // A list page asked for with a row anchored is that row's own tab: its drawer would only + // open the editor a page item tab already hosts, inside a frame of its own. + if (target.type === 'page') return pageItemLocation(target.href) + if (target.type === 'pageitem') return pageItemUrl(target.ref) if (target.type === 'artifact') { return artifactUrl(target.id, target.name, keptVersion(target, onto)) } @@ -152,10 +164,9 @@ export function previewTargetForSessionTarget( // Adapt a deployable item's layout kind (the session review dock speaks `Kind`, // not SessionTarget) to a preview destination: the three live editors, data -// pipelines, plus legacy drag-and-drop apps, which the panel hosts as an iframe -// over their edit route. Every other kind maps to undefined — not for lack of any -// route (a variable or trigger has a list page the panel can host) but because -// there is no item editor to preview, so their row falls back to the diff. The +// pipelines, page items (variables, resources, schedules, triggers), plus legacy +// drag-and-drop apps, which the panel hosts as an iframe over their edit route. +// Every other kind maps to undefined, and its row falls back to the diff. The // undefined is also the caller's test for "can this row be previewed?". export function previewTargetForDeployKind(kind: Kind, path: string): PreviewTarget | undefined { if (kind === 'app') { @@ -164,6 +175,16 @@ export function previewTargetForDeployKind(kind: Kind, path: string): PreviewTar if (kind === 'script' || kind === 'flow' || kind === 'raw_app') { return previewTargetForSessionTarget(kind, path) } + if (kind === 'variable' || kind === 'resource' || kind === 'schedule') { + return { type: 'pageitem', ref: { kind, path } } + } + const triggerKind = kind.endsWith('_trigger') ? kind.slice(0, -'_trigger'.length) : undefined + if (triggerKind && triggerKind in TRIGGER_PAGES) { + return { + type: 'pageitem', + ref: { kind: 'trigger', triggerKind: triggerKind as TriggerKind, path } + } + } // A pipeline's editor is its folder's graph view, not its bundle path. if (kind === 'data_pipeline') { const folder = pipelineFolderFromBundlePath(path) @@ -189,7 +210,14 @@ export function hydratePreviewTabs(session: { seen.add(t.id) // Rebuilt field-by-field so stray properties on old saved records (e.g. the // retired `pinned` flag) don't survive hydration and get persisted back. - tabs.push({ id: t.id, url: t.url, loc: t.loc || t.url }) + // A list page saved with a row's drawer open comes back as that row's own tab. Read from + // where the frame was, not from what was last commanded: the user may have moved to + // another row since, or closed the drawer, and the observation is what they saw. + const loc = t.loc || t.url + const item = pageItemLocation(t.loc ? loc : t.url) + tabs.push( + parsePageItemRoute(item) ? { id: t.id, url: item, loc: item } : { id: t.id, url: t.url, loc } + ) } if (tabs.length > 0) { const wantActive = session.activePreviewTabId @@ -290,22 +318,14 @@ export class SessionPreviewTabs { // Drift is a change of what the frame *shows*, not of its URL string: a page // writing its own filter defaults back is not the user navigating away. const drifted = !showsView(tab.loc, url) - // Both cases the browser will not act on, decided here because this is where the - // old and new commands are both in hand: re-commanding the URL a drifted frame - // already carries moves nothing, and moving to another fragment resolves within the - // same document — so a list page never re-runs the `#` read that opens a row. - // Dropping the fragment is not one of them: the same-document path applies only to a - // target that has one, so the browser loads the page — closing the drawer by itself — - // and forcing a second load races that one back onto the row. - const fragmentOnly = - !commandUnchanged && url.includes('#') && tab.url.split('#')[0] === url.split('#')[0] + // Decided here because this is where the old and new commands are both in hand: + // re-commanding the URL a drifted frame already carries moves nothing. retargetTab(tab, url) - if ((commandUnchanged && drifted) || fragmentOnly) this.pulseReload(tab.id) + if (commandUnchanged && drifted) this.pulseReload(tab.id) } - // Force the host to reload the iframe. A navigation onto the tab's exact current URL - // changes nothing, so URL-driven behavior — a `#` opening a drawer the user has - // since closed — would never re-fire. + // Force the host to reload the tab. A navigation onto the tab's exact current URL + // changes nothing, so URL-driven behavior would never re-fire. pulseReload(id: string): void { this.#reloadPulse = { id, nonce: this.#reloadPulse.nonce + 1 } } @@ -435,21 +455,21 @@ export class SessionPreviewTabs { // shows this. The tab on this exact view wins over any other on the page — // `new_tab` puts two views side by side, and retargeting whichever sits first // would overwrite the other and leave both on the same row. - const shown = opts?.forceNewTab - ? undefined - : (this.#tabs.find((t) => showsView(t.loc, url)) ?? - this.#tabs.find((t) => describeLocation(t.loc).identity === describeLocation(url).identity)) + // A page item stays one tab whatever the opener asks: two would hold two drafts of it. + const shown = + opts?.forceNewTab && !parsePageItemRoute(url) + ? undefined + : (this.#tabs.find((t) => showsView(t.loc, url)) ?? + this.#tabs.find( + (t) => describeLocation(t.loc).identity === describeLocation(url).identity + )) if (shown) { const same = showsView(shown.loc, url) if (same) { // The frame is already here, but record what was asked for: `url` is what the // tab persists and remounts from, so leaving it on where the frame started - // sends a refresh back to the row the user has since moved off. + // sends a refresh back to the view the user has since moved off. recordCommand(shown, url) - // Nothing to navigate to, so nothing would re-run: the list pages read their - // `#` once per document, and the drawer it opens may since have been - // closed. Only a forced load can bring it back. - if (describeLocation(url).anchor) this.pulseReload(shown.id) } else { this.#retarget(shown, url) } @@ -512,7 +532,17 @@ export class SessionPreviewTabs { return } } - this.#retarget(t, targetUrl(target, t)) + // One tab per page item, as for editors: two would hold two drafts of one item. + const url = targetUrl(target, t) + if (parsePageItemRoute(url)) { + const existing = this.#tabs.find((x) => x.url === url) + if (existing && existing.id !== t.id) { + this.#activeId = existing.id + this.#flush() + return + } + } + this.#retarget(t, url) this.#flush() } @@ -574,6 +604,17 @@ export class SessionPreviewTabs { this.#flush() } + /** Follow a page item its editor saved under a new path, in place. */ + retargetPageItem(from: PageItemRef, to: PageItemRef): void { + const fromUrl = pageItemUrl(from) + const toUrl = pageItemUrl(to) + if (fromUrl === toUrl) return + const tab = this.#tabs.find((t) => t.url === fromUrl) + if (!tab) return + retargetTab(tab, toUrl) + this.#flush() + } + closeArtifact(artifactId: string): void { const tab = this.#tabs.find((t) => parseArtifactRoute(t.url)?.id === artifactId) if (tab) this.close(tab.id) @@ -606,23 +647,14 @@ export class SessionPreviewTabs { } // Feed back the location an iframe reported on load (only the page can read - // contentWindow.location). Updates the observed `loc`; `url` follows only when a - // drawer closed (below), and the host navigates on a command it isn't already at, - // so that write does not move the frame. + // contentWindow.location). Updates the observed `loc` only: the host navigates on a + // command it isn't already at, and an in-frame move is the user browsing. observeLocation(id: string, loc: string): void { const t = this.#tabs.find((x) => x.id === id) if (!t) return const canonical = canonicalizeObservedLoc(loc) if (t.loc === canonical) return t.loc = canonical - // Closing a drawer drops the row from the frame's URL. The command has to follow, or - // the tab reopens it on the next mount — the iframe loads `url`, not `loc`. Only the - // anchor: any other in-frame move is the user browsing, which must not re-command. - const commanded = describeLocation(t.url) - const observed = describeLocation(canonical) - if (commanded.anchor && !observed.anchor && commanded.identity === observed.identity) { - t.url = t.url.split('#')[0] - } this.#flush() } @@ -730,6 +762,7 @@ export function describePreview( const lines = tabs.map((t) => { const where = whereIs(t) const artifact = parseArtifactRoute(where) + const pageItem = parsePageItemRoute(where) const page = matchPreviewPage(where) const pipelineFolder = parsePipelineRoute(where) const route = parsePreviewItemRoute(where) @@ -737,16 +770,19 @@ export function describePreview( ? // A pinned tab is not showing what the assistant last wrote, and nothing else in this // summary would tell it so. `artifact "${artifact.name || 'Artifact'}"${artifact.version ? ` (pinned to v${artifact.version})` : ''}` - : page - ? `page "${page.label}"${previewLocationDetail(where)}` - : pipelineFolder - ? `pipeline "${pipelineFolder}"` - : route - ? `${route.raw_app ? 'raw_app' : route.kind} "${route.itemPath}"` - : // Trigger list pages land here (they're outside PREVIEW_PAGES), and - // their `#` is the trigger the drawer has open. - `${stripBase(where)}${previewLocationDetail(where)}` - const live = resolvePreviewTab(t.url).kind === 'editor' ? ', live editor' : '' + : pageItem + ? `${pageItemKindLabel(pageItem).toLowerCase()} "${pageItem.path}"` + : page + ? `page "${page.label}"${previewLocationDetail(where)}` + : pipelineFolder + ? `pipeline "${pipelineFolder}"` + : route + ? `${route.raw_app ? 'raw_app' : route.kind} "${route.itemPath}"` + : // Trigger list pages land here (they're outside PREVIEW_PAGES), and + // their `#` is the trigger the drawer has open. + `${stripBase(where)}${previewLocationDetail(where)}` + const slotKind = resolvePreviewTab(t.url).kind + const live = slotKind === 'editor' || slotKind === 'pageitem' ? ', live editor' : '' const active = t.id === activeId ? ', active' : '' // One list entry per tab: an artifact's name, a pipeline folder and an item path // all arrive decoded from a URL, so any of them could otherwise write a line here. diff --git a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts index b71de62c23..8b1ba27ea0 100644 --- a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts +++ b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts @@ -177,10 +177,88 @@ describe('previewTargetForDeployKind', () => { pipelineTarget ) }) + it('routes variables, resources, schedules and triggers to their own tab', () => { + expect(previewTargetForDeployKind('schedule', 'u/me/s')).toEqual({ + type: 'pageitem', + ref: { kind: 'schedule', path: 'u/me/s' } + }) + expect(previewTargetForDeployKind('http_trigger', 'u/me/t')).toEqual({ + type: 'pageitem', + ref: { kind: 'trigger', triggerKind: 'http', path: 'u/me/t' } + }) + expect(previewTargetForDeployKind('variable', 'u/me/v')).toEqual({ + type: 'pageitem', + ref: { kind: 'variable', path: 'u/me/v' } + }) + }) + it('has no destination for kinds the preview panel cannot host', () => { - expect(previewTargetForDeployKind('schedule', 'u/me/s')).toBeUndefined() - expect(previewTargetForDeployKind('http_trigger', 'u/me/t')).toBeUndefined() - expect(previewTargetForDeployKind('variable', 'u/me/v')).toBeUndefined() + expect(previewTargetForDeployKind('folder', 'f/x')).toBeUndefined() + expect(previewTargetForDeployKind('resource_type', 'x')).toBeUndefined() + }) +}) + +describe('page item tabs', () => { + const variable: PreviewTarget = { + type: 'pageitem', + ref: { kind: 'variable', path: 'u/me/token' } + } + + it('opens a list page anchored at a row as that row’s own tab, beside the list', () => { + const o = owner() + o.open({ type: 'page', href: '/routes', label: 'HTTP routes' }) + o.open({ type: 'page', href: '/routes#u/me/a', label: 'HTTP routes' }) + expect(o.tabs.map((t) => t.url)).toEqual(['/routes', 'pageitem:trigger.http/u%2Fme%2Fa']) + + // Resources address their row through an extra segment. + o.open({ type: 'page', href: '/resources?owner=u#/resource/u/me/db', label: 'Resources' }) + expect(o.tabs.at(-1)!.url).toBe('pageitem:resource/u%2Fme%2Fdb') + }) + + it('keeps one tab per item, whatever the opener asks', () => { + const o = owner() + o.open(variable) + o.open({ type: 'page', href: '/runs', label: 'Runs' }) + expect(o.open({ type: 'page', href: '/variables#u/me/token', label: 'V' }).status).toBe( + 'focused' + ) + expect(o.open(variable, { forceNewTab: true }).status).toBe('focused') + o.navigate(variable) + expect(o.tabs).toHaveLength(2) + expect(o.activeId).toBe(o.tabs[0].id) + }) + + it('follows an item saved under a new path in place', () => { + const o = owner() + o.open(variable) + const id = o.tabs[0].id + o.retargetPageItem( + { kind: 'variable', path: 'u/me/token' }, + { kind: 'variable', path: 'f/x/token' } + ) + expect(o.tabs).toEqual([ + { id, url: 'pageitem:variable/f%2Fx%2Ftoken', loc: 'pageitem:variable/f%2Fx%2Ftoken' } + ]) + }) + + it('restores a tab saved on a row’s drawer as the row it was last on', () => { + const snap = hydratePreviewTabs({ + previewTabs: [ + { id: 'a', url: '/schedules#u/me/daily', loc: '/schedules?path=u#u/me/daily' }, + // A drawer opened inside the frame: the command is still the bare list. + { id: 'b', url: '/variables', loc: '/variables#u/me/token' }, + // The user moved on to another row inside the frame… + { id: 'c', url: '/variables#u/me/token', loc: '/variables#u/me/other' }, + // …or closed the drawer, leaving no row to restore. + { id: 'd', url: '/variables#u/me/token', loc: '/variables' } + ] + }) + expect(snap.tabs).toEqual([ + { id: 'a', url: 'pageitem:schedule/u%2Fme%2Fdaily', loc: 'pageitem:schedule/u%2Fme%2Fdaily' }, + { id: 'b', url: 'pageitem:variable/u%2Fme%2Ftoken', loc: 'pageitem:variable/u%2Fme%2Ftoken' }, + { id: 'c', url: 'pageitem:variable/u%2Fme%2Fother', loc: 'pageitem:variable/u%2Fme%2Fother' }, + { id: 'd', url: '/variables#u/me/token', loc: '/variables' } + ]) }) }) @@ -232,93 +310,34 @@ describe('SessionPreviewTabs.open', () => { expect(o.activeId).toBe(firstId) }) - // A trigger list page is not a `matchReusablePage`, so the runtime's - // navigate-in-place path doesn't cover it: re-pointing the tab has to happen - // here or the panel keeps showing the previously opened row. - it('re-points a page tab whose hash target changed instead of only focusing it', () => { - const o = owner() - const routes = (href: string) => ({ type: 'page' as const, href, label: 'HTTP routes' }) - o.open(routes('/routes#u/me/a')) - const firstId = o.activeId - - // 'retargeted', not 'opened': the tab count is unchanged, and the caller - // reports that to the model. - const res = o.open(routes('/routes#u/me/b')) - expect(res.status).toBe('retargeted') - expect(o.tabs).toHaveLength(1) - expect(o.activeId).toBe(firstId) - expect(o.tabs[0].url).toBe('/routes#u/me/b') - - // Back to the bare list: still the same tab, no longer anchored at a row. - expect(o.open(routes('/routes')).status).toBe('retargeted') - expect(o.tabs).toHaveLength(1) - expect(o.tabs[0].url).toBe('/routes') - - // ...and asking for the view it already shows is a plain focus. - expect(o.open(routes('/routes')).status).toBe('focused') - }) - // The list pages rewrite their own filter defaults into the URL after mount, // and `loc` follows that rewrite. Matching on anything but the path made a tab // stop recognizing itself, so every later open spawned a duplicate. it('still recognizes a tab after the page rewrote its own filter params', () => { const o = owner() const routes = (href: string) => ({ type: 'page' as const, href, label: 'HTTP routes' }) - o.open(routes('/routes#u/me/a')) - const id = o.tabs[0].id - o.observeLocation(id, '/routes?filter_path_of=trigger#u/me/a') + o.open(routes('/routes')) + o.observeLocation(o.tabs[0].id, '/routes?filter_path_of=trigger') - const res = o.open(routes('/routes#u/me/b')) - expect(res.status).toBe('retargeted') + expect(o.open(routes('/routes')).status).toBe('focused') expect(o.tabs).toHaveLength(1) - expect(o.tabs[0].url).toBe('/routes#u/me/b') }) // `new_tab` deliberately keeps two views of one page side by side. Reopening one of // them must focus the tab already showing it, not retarget whichever tab happens to - // sit first in the strip — that would overwrite the other view and leave two tabs - // on the same row. + // sit first in the strip — that would overwrite the other view. it('focuses the tab already showing the exact location before retargeting by path', () => { const o = owner() - const routes = (href: string) => ({ type: 'page' as const, href, label: 'HTTP routes' }) - o.open(routes('/routes#u/me/a')) + const runs = (href: string) => ({ type: 'page' as const, href, label: 'Runs' }) + o.open(runs('/runs?path=u/me/a')) const first = o.tabs[0].id - o.open(routes('/routes#u/me/b'), { forceNewTab: true }) + o.open(runs('/runs?path=u/me/b'), { forceNewTab: true }) const second = o.tabs[1].id - expect(o.open(routes('/routes#u/me/b')).status).toBe('focused') + expect(o.open(runs('/runs?path=u/me/b')).status).toBe('focused') expect(o.activeId).toBe(second) expect(o.tabs).toHaveLength(2) - expect(o.tabs.find((t) => t.id === first)?.url).toBe('/routes#u/me/a') - }) - - // The list pages read their `#` once per document, so a drawer the user closed - // inside the frame only comes back on a forced load — and re-commanding the location - // the tab already shows produces no navigation the host could act on. - it('forces a load when the requested row is the one the tab already shows', () => { - const o = owner() - const routes = (href: string) => ({ type: 'page' as const, href, label: 'HTTP routes' }) - o.open(routes('/routes#u/me/a')) - const id = o.tabs[0].id - o.observeLocation(id, '/routes?filter_path_of=trigger#u/me/a') - const before = o.reloadPulse.nonce - - expect(o.open(routes('/routes#u/me/a')).status).toBe('focused') - expect(o.reloadPulse).toEqual({ id, nonce: before + 1 }) - }) - - // Dropping the fragment is a load in itself, so the forced one lands on top of a - // navigation still in flight — and reloads the row the command asked to leave. - it('does not force a load when the requested location drops the row', () => { - const o = owner() - const routes = (href: string) => ({ type: 'page' as const, href, label: 'HTTP routes' }) - o.open(routes('/routes#u/me/a')) - const id = o.tabs[0].id - const before = o.reloadPulse.nonce - - o.navigate(routes('/routes')) - expect(o.tabs.find((t) => t.id === id)?.url).toBe('/routes') - expect(o.reloadPulse.nonce).toBe(before) + expect(o.tabs.find((t) => t.id === first)?.url).toBe('/runs?path=u/me/a') }) // Runs restores the user's "hide schedules" preference into the URL whenever a load @@ -365,29 +384,11 @@ describe('SessionPreviewTabs.open', () => { expect(o.tabs[0].url).toBe('/apps/edit/u/me/dash') }) - // Re-commanding the URL a tab is already pointed at changes nothing the host can - // see, so the frame would stay wherever the user navigated it inside the page. - it('forces a reload when the request matches the command but the frame drifted', () => { - const o = owner() - const routes = (href: string) => ({ type: 'page' as const, href, label: 'HTTP routes' }) - o.open(routes('/routes#u/me/a')) - const id = o.tabs[0].id - // The user clicked another trigger inside the iframe. - o.observeLocation(id, '/routes#u/me/b') - const before = o.reloadPulse.nonce - - const res = o.open(routes('/routes#u/me/a')) - expect(res.status).toBe('retargeted') - expect(o.tabs).toHaveLength(1) - expect(o.tabs[0].loc).toBe('/routes#u/me/a') - expect(o.reloadPulse.nonce).toBe(before + 1) - }) - it('forceNewTab opts a page out of the location dedupe', () => { const o = owner() const routes = (href: string) => ({ type: 'page' as const, href, label: 'HTTP routes' }) - o.open(routes('/routes#u/me/a')) - const res = o.open(routes('/routes#u/me/b'), { forceNewTab: true }) + o.open(routes('/routes')) + const res = o.open(routes('/routes'), { forceNewTab: true }) expect(res.status).toBe('opened') expect(o.tabs).toHaveLength(2) }) @@ -512,59 +513,6 @@ describe('SessionPreviewTabs.open', () => { }) }) -describe('SessionPreviewTabs.open — commanded url', () => { - it('records the requested row even when the frame is already showing it', () => { - const o = owner() - o.open({ type: 'page', href: '/routes#u/me/a', label: 'R' }) - // The user moves to another row inside the frame. - o.observeLocation(o.tabs[0].id, '/routes#u/me/b') - o.open({ type: 'page', href: '/routes#u/me/b', label: 'R' }) - // `url` is what a refresh and a remount reload from, so it has to follow. - expect(o.tabs[0].url).toBe('/routes#u/me/b') - expect(o.tabs).toHaveLength(1) - }) -}) - -describe('SessionPreviewTabs.observeLocation', () => { - it('drops the row from the command when the frame closes its drawer', () => { - const o = owner() - o.open({ type: 'page', href: '/routes#u/me/a', label: 'R' }) - // The page clears its own hash when the drawer closes. - o.observeLocation(o.tabs[0].id, '/routes?filter_path_of=trigger') - // The iframe mounts from `url`, so a remount would otherwise reopen the drawer. - expect(o.tabs[0].url).toBe('/routes') - }) - - it('leaves the command alone when the user just browses inside the frame', () => { - const o = owner() - o.open({ type: 'page', href: '/routes#u/me/a', label: 'R' }) - o.observeLocation(o.tabs[0].id, '/routes#u/me/b') - expect(o.tabs[0].url).toBe('/routes#u/me/a') - }) -}) - -describe('SessionPreviewTabs.open — forced loads', () => { - it('pulses when only the fragment changes, since the browser would not load', () => { - const o = owner() - o.open({ type: 'page', href: '/routes#u/me/a', label: 'R' }) - const before = o.reloadPulse.nonce - o.open({ type: 'page', href: '/routes#u/me/b', label: 'R' }) - // Same document: the browser resolves the new fragment without a load, so the - // list page never re-runs the `#` read that opens the row. - expect(o.reloadPulse.nonce).toBeGreaterThan(before) - expect(o.tabs).toHaveLength(1) - }) - - it('does not pulse when the document itself changes', () => { - const o = owner() - o.open({ type: 'page', href: '/routes#u/me/a', label: 'R' }) - const before = o.reloadPulse.nonce - o.open({ type: 'page', href: '/schedules#u/me/a', label: 'S' }) - // Different page: src changes, the browser loads it, nothing to force. - expect(o.reloadPulse.nonce).toBe(before) - }) -}) - describe('SessionPreviewTabs.navigate', () => { it('retargets the active tab to an editor item', () => { const o = owner() diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index bd899c8208..640b484478 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -56,7 +56,10 @@ import { selectPreviewTabsToClose, whereIs } from './sessionPreviewTabs.svelte' +import { pageItemKindLabel } from './previewPaths' import { + pageItemForLocation, + pageItemLocation, parsePreviewItemRoute, previewLocationContext, previewLocationLabel, @@ -387,7 +390,8 @@ function createRuntime(session: Session): SessionRuntime { // What the side panel is showing, stamped on each user message so the chat // knows the page (and the row whose drawer is open) without spending a // get_preview_status round-trip. Live editors are skipped: they register - // themselves as the ACTIVE EDITOR through UserDraft's live-draft registry. + // themselves as the ACTIVE EDITOR through UserDraft's live-draft registry. A page + // item tab is not one of them, and reads as its list page with the item open. manager.activePreviewResolver = () => { const owner = getRuntime(session.id)?.previewTabs // What is on screen, not merely which tab is selected: the rule tells the model @@ -395,7 +399,8 @@ function createRuntime(session: Session): SessionRuntime { // point those at a page the user cannot see. const tab = owner?.displayedTab if (!tab) return undefined - if (resolvePreviewTab(tab.url).kind !== 'iframe') return undefined + const slotKind = resolvePreviewTab(tab.url).kind + if (slotKind !== 'iframe' && slotKind !== 'pageitem') return undefined return previewLocationContext(whereIs(tab)) } // Pre-flight: materialise the (still-transient) session, then commit @@ -1101,7 +1106,9 @@ async function applyRemoteTurnEnd(sessionId: string, chatId: string): Promise { // open_page dispatches here to show a workspace page (Runs/Schedules) as a page // tab in the calling session's preview panel. Returns undefined when there is no // session so open_page can fall back to browser navigation. -setOpenPagePreviewHandler(({ sessionId: callerSessionId, href, label, newTab }) => { +setOpenPagePreviewHandler(({ sessionId: callerSessionId, href, label: pageLabel, newTab }) => { const sessionId = callerSessionId ?? sessionState.currentSessionId if (!sessionId) return undefined const session = sessionState.sessions.find((s) => s.id === sessionId) if (!session) return undefined const owner = getOrCreateRuntime(session).previewTabs + // A page opened on one item is that item's tab, and the report has to name what opened. + const pageItem = pageItemForLocation(href) + const label = pageItem + ? `the ${pageItemKindLabel(pageItem).toLowerCase()} ${promptSafe(pageItem.path)}` + : pageLabel // open() owns the whole decision — which tab already shows this page, whether the // requested view differs from what it shows, and whether a forced load is needed to // re-fire a drawer. Deciding any of that again here means two predicates for one diff --git a/frontend/src/lib/components/settings/CreateToken.svelte b/frontend/src/lib/components/settings/CreateToken.svelte index 146f844b3f..8ee5391443 100644 --- a/frontend/src/lib/components/settings/CreateToken.svelte +++ b/frontend/src/lib/components/settings/CreateToken.svelte @@ -1,11 +1,6 @@ {#if captureInfo} diff --git a/frontend/src/lib/components/triggers/email/EmailTriggerEditorConfigSection.svelte b/frontend/src/lib/components/triggers/email/EmailTriggerEditorConfigSection.svelte index 3a60d3be71..4a44e01b30 100644 --- a/frontend/src/lib/components/triggers/email/EmailTriggerEditorConfigSection.svelte +++ b/frontend/src/lib/components/triggers/email/EmailTriggerEditorConfigSection.svelte @@ -2,8 +2,6 @@ import { Alert } from '$lib/components/common' import Required from '$lib/components/Required.svelte' import Section from '$lib/components/Section.svelte' - import { userStore, workspaceStore } from '$lib/stores' - import { getTriggerWorkspace } from '$lib/components/triggers/triggerWorkspace' // import { page } from '$app/state' import { getEmailAddress, getEmailDomain } from './utils' import { isCloudHosted } from '$lib/cloud' @@ -12,6 +10,10 @@ import { untrack } from 'svelte' import { EmailTriggerService } from '$lib/gen' import ClipboardPanel from '$lib/components/details/ClipboardPanel.svelte' + import { + useOperatingUser, + useOperatingWorkspace + } from '$lib/components/operatingWorkspace.svelte' interface Props { initialTriggerPath?: string | undefined dirtyLocalPart?: boolean @@ -35,8 +37,10 @@ isDraftOnly = true, showTestingBadge = false }: Props = $props() - const triggerWs = getTriggerWorkspace() - const wsId = $derived(triggerWs?.() ?? $workspaceStore) + const operatingWorkspace = useOperatingWorkspace() + const operatingUser = useOperatingUser() + const actingUser = $derived(operatingUser.current) + const wsId = $derived($operatingWorkspace) let validateTimeout: number | undefined = undefined @@ -95,7 +99,7 @@ local_part === undefined && (local_part = '') }) - let userIsAdmin = $derived($userStore?.is_admin || $userStore?.is_super_admin) + let userIsAdmin = $derived(actingUser?.is_admin || actingUser?.is_super_admin) let userCanEditConfig = $derived(userIsAdmin || isDraftOnly) // User can edit config if they are admin or if the trigger is a draft which will not be saved diff --git a/frontend/src/lib/components/triggers/email/EmailTriggerEditorInner.svelte b/frontend/src/lib/components/triggers/email/EmailTriggerEditorInner.svelte index d2133cf865..30f848004f 100644 --- a/frontend/src/lib/components/triggers/email/EmailTriggerEditorInner.svelte +++ b/frontend/src/lib/components/triggers/email/EmailTriggerEditorInner.svelte @@ -2,6 +2,7 @@ import { Button } from '$lib/components/common' import { clearPageDrawerAnchor, + handOffPageDrawer, setPageDrawerAnchor } from '$lib/components/sessions/pageDrawerSession' import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' @@ -17,8 +18,7 @@ type Retry, type TriggerMode } from '$lib/gen' - import { usedTriggerKinds, userStore, workspaceStore } from '$lib/stores' - import { getTriggerWorkspace } from '$lib/components/triggers/triggerWorkspace' + import { usedTriggerKinds } from '$lib/stores' import { canWrite, capitalize, emptyString, sendUserToast } from '$lib/utils' import Section from '$lib/components/Section.svelte' import { Loader2 } from 'lucide-svelte' @@ -38,9 +38,16 @@ import LocalDraftBanner from '$lib/components/LocalDraftBanner.svelte' import TriggerSuspendedJobsAlert from '../TriggerSuspendedJobsAlert.svelte' import TriggerSuspendedJobsModal from '../TriggerSuspendedJobsModal.svelte' + import { + useOperatingUser, + useOperatingWorkspace, + useOperatingWorkspaceHref + } from '$lib/components/operatingWorkspace.svelte' let { useDrawer = true, + inline = false, + onClose = undefined, hideTarget = false, description = undefined, isEditor = false, @@ -55,8 +62,11 @@ trigger = undefined, customSaveBehavior = undefined } = $props() - const triggerWs = getTriggerWorkspace() - const wsId = $derived(triggerWs?.() ?? $workspaceStore) + const operatingWorkspace = useOperatingWorkspace() + const operatingUser = useOperatingUser() + const actingUser = $derived(operatingUser.current) + const operatingHref = useOperatingWorkspaceHref() + const wsId = $derived($operatingWorkspace) // Form data state let initialPath = $state('') @@ -75,7 +85,13 @@ let workspaced_local_part = $state(false) let drawerLoading = $state(true) let showLoader = $state(false) - let can_write = $state(true) + let permsPath = $state(undefined) + let permsForWrite = $state | undefined>(undefined) + // The acting user in the operating workspace arrives asynchronously, and an unknown user + // refuses — so the editor stays read-only until the lookup lands, which is the safe answer. + const can_write = $derived( + permsPath === undefined ? true : canWrite(permsPath, permsForWrite ?? {}, actingUser) + ) let extraPerms = $state | undefined>(undefined) let error_handler_path: string | undefined = $state() let error_handler_args: Record = $state({}) @@ -95,7 +111,7 @@ let originalConfig = $state(undefined) let hasChanged = $derived(!deepEqual(getEmailTriggerConfig(), originalConfig ?? {})) - const isAdmin = $derived($userStore?.is_admin || $userStore?.is_super_admin) + const isAdmin = $derived(actingUser?.is_admin || actingUser?.is_super_admin) const emailConfig = $derived.by(getEmailTriggerConfig) const draftSync = useTriggerDraftSync({ @@ -127,6 +143,9 @@ defaultConfig?: Partial, fixedScriptPath_?: string ) { + if (handOffPageDrawer(TRIGGER_PAGES.email.path, ePath)) return + // A `whoami` that failed earlier would otherwise pin this workspace to "unknown user". + operatingUser.forgetFailures() drawerLoading = true let loader = setTimeout(() => { showLoader = true @@ -206,6 +225,8 @@ } function loadTriggerConfig(cfg?: Partial): void { + // The loaded trigger says what it runs; an opener's `isFlow` is only its guess. + if (cfg?.is_flow !== undefined) itemKind = cfg.is_flow ? 'flow' : 'script' script_path = cfg?.script_path ?? '' initialScriptPath = cfg?.script_path ?? '' is_flow = cfg?.is_flow ?? false @@ -213,7 +234,8 @@ local_part = cfg?.local_part ?? '' workspaced_local_part = cfg?.workspaced_local_part ?? false extraPerms = cfg?.extra_perms ?? undefined - can_write = canWrite(path, cfg?.extra_perms ?? {}, $userStore) + permsPath = path + permsForWrite = cfg?.extra_perms ?? {} error_handler_path = cfg?.error_handler_path error_handler_args = cfg?.error_handler_args ?? {} retry = cfg?.retry @@ -265,7 +287,7 @@ saveCfg, edit, wsId!, - !!$userStore?.is_admin || !!$userStore?.is_super_admin, + !!actingUser?.is_admin || !!actingUser?.is_super_admin, usedTriggerKinds ) if (isSaved) { @@ -405,7 +427,7 @@ bind:scriptPath={script_path} {initialScriptPath} canWrite={can_write} - isOperator={!!$userStore?.operator} + isOperator={!!actingUser?.operator} promptClass="text-xs mt-3 mb-1 text-primary" > {#snippet createButton()} @@ -414,7 +436,9 @@ btnClasses="ml-4" variant="accent" size="xs" - href={itemKind === 'flow' ? '/flows/add?hub=72' : '/scripts/add?hub=hub%2F19813'} + href={operatingHref( + itemKind === 'flow' ? '/flows/add?hub=72' : '/scripts/add?hub=hub%2F19813' + )} target="_blank">Create from template {/if} @@ -486,36 +510,44 @@ {/if} {/snippet} -{#if useDrawer} +{#snippet drawerBody()} + (inline ? onClose?.() : drawer?.closeDrawer())} + > + {#snippet actions()} + {@render saveButton()} + {/snippet} + {#snippet banner()} + draftSync.deployed} + reserveSpace={draftSync.hasBaseline} + getCurrent={() => draftSync.current} + onDiscard={() => draftSync.resetToDeployed(initialPath)} + disabled={!can_write} + /> + {/snippet} + {@render config()} + +{/snippet} + +{#if useDrawer && inline} + {@render drawerBody()} +{:else if useDrawer} clearPageDrawerAnchor(TRIGGER_PAGES.email.path)} > - drawer?.closeDrawer()} - > - {#snippet actions()} - {@render saveButton()} - {/snippet} - {#snippet banner()} - draftSync.deployed} - reserveSpace={draftSync.hasBaseline} - getCurrent={() => draftSync.current} - onDiscard={() => draftSync.resetToDeployed(initialPath)} - disabled={!can_write} - /> - {/snippet} - {@render config()} - + {@render drawerBody()} {:else}
diff --git a/frontend/src/lib/components/triggers/email/EmailTriggerPanel.svelte b/frontend/src/lib/components/triggers/email/EmailTriggerPanel.svelte index 12ec403aeb..7eac6a8971 100644 --- a/frontend/src/lib/components/triggers/email/EmailTriggerPanel.svelte +++ b/frontend/src/lib/components/triggers/email/EmailTriggerPanel.svelte @@ -1,11 +1,15 @@
{#if resourceId} -
+
Selected: {resourceName || resourceId}
{:else} -
+
No file selected
@@ -198,21 +196,27 @@