diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 23a20f48af..8adbba84ec 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -543cfe53a6627514b7e557218fb483b2b896d722 +163a335a4f4e9ae179ffb96fc92ee1681cb44e60 diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index 4f3b429df5..9c8f6f8823 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -36,6 +36,17 @@ fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuil builder.header("Authorization", format!("Bearer {}", token)) } +async fn enable_guests(port: u16, ws: &str) -> anyhow::Result<()> { + authed( + client().post(format!("http://localhost:{port}/api/w/{ws}/workspaces/edit_guest_access")), + ADMIN_TOKEN, + ) + .json(&json!({ "guest_access_enabled": true })) + .send() + .await?; + Ok(()) +} + fn guest_scopes() -> Vec { vec![ "guest".to_string(), @@ -74,6 +85,7 @@ async fn guest_session_is_confined_to_its_app(db: Pool) -> anyhow::Res let port = server.addr.port(); let ws = format!("http://localhost:{port}/api/w/test-workspace"); + enable_guests(port, "test-workspace").await?; insert_guest_token(&db, "test-workspace").await?; // Its own identity resolves, and reports the role rather than falling through to @@ -346,11 +358,12 @@ fn execute(port: u16, ws: &str, app: &str, token: &str) -> reqwest::RequestBuild })) } -/// The run path re-reads the workspace switch instead of trusting mint time. This is -/// the only thing standing between a `guest` policy pushed by git-sync and execution -/// once an admin has turned guests off. +/// The workspace switch is enforced at the auth door for every guest request, not +/// remembered per handler. This is what stands between a `guest` policy pushed by +/// git-sync and execution once an admin has turned guests off — and it closes the +/// app to sessions already issued. #[sqlx::test(fixtures("base"))] -async fn execute_component_re_checks_the_workspace_switch( +async fn the_door_re_checks_the_workspace_switch( db: Pool, ) -> anyhow::Result<()> { initialize_tracing().await; @@ -365,34 +378,35 @@ async fn execute_component_re_checks_the_workspace_switch( assert_eq!(resp.status(), 201, "{}", resp.text().await?); insert_guest_token(&db, "test-workspace").await?; - // Switch off: refused at the gate, even though the app's policy says guest and - // the session was (in this fixture) issued regardless. + // Switch off: the session does not authenticate at all, even though the app's + // policy says guest and the session was (in this fixture) issued regardless. On + // the authed route that is a 401; on the optional-auth run route the rejected + // token reads as no token, and a guest-mode app then refuses the anonymous + // caller — a denial either way. + let resp = authed(client().get(format!("{ws}/users/whoami")), GUEST_TOKEN) + .send() + .await?; + assert_eq!(resp.status(), 401, "a guest must not authenticate while guests are off"); let resp = execute(port, "test-workspace", APP_PATH, GUEST_TOKEN) .send() .await?; - assert_eq!( - resp.status(), - 403, - "a guest must not run components while the workspace has guests off" + assert!( + resp.status().is_client_error() && resp.status() != 404, + "a guest must not run while guests are off, got {}", + resp.status() ); - // Switch on: past the gate. What follows is the runnable lookup, which fails on - // the nonexistent script — the point is that it is no longer a 403. - authed( - client().post(format!("{ws}/workspaces/edit_guest_access")), - ADMIN_TOKEN, - ) - .json(&json!({ "guest_access_enabled": true })) - .send() - .await?; + // Switch on: through the door. What follows the run is the runnable lookup, + // which fails on the nonexistent script — the point is that it is no longer a + // denial. + enable_guests(port, "test-workspace").await?; let resp = execute(port, "test-workspace", APP_PATH, GUEST_TOKEN) .send() .await?; - assert_ne!( - resp.status(), - 403, - "with guests on, the guest gate must let the run through: {}", - resp.text().await? + assert!( + resp.status() != 401 && resp.status() != 403, + "with guests on, the door must let the run through: {}", + resp.status() ); Ok(()) @@ -445,13 +459,7 @@ async fn a_guest_minted_embed_token_stays_a_guest(db: Pool) -> anyhow: let port = server.addr.port(); let ws = format!("http://localhost:{port}/api/w/test-workspace"); - authed( - client().post(format!("{ws}/workspaces/edit_guest_access")), - ADMIN_TOKEN, - ) - .json(&json!({ "guest_access_enabled": true })) - .send() - .await?; + enable_guests(port, "test-workspace").await?; let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) .json(&guest_app_with_runnable(APP_PATH, true)) .send() @@ -506,7 +514,7 @@ async fn a_guest_minted_embed_token_stays_a_guest(db: Pool) -> anyhow: let me: serde_json::Value = resp.json().await?; assert_eq!(me["role"], json!("guest")); - // Governed: the workspace switch closes it, iframe or not. + // Governed: the workspace switch closes it at the door, iframe or not. authed( client().post(format!("{ws}/workspaces/edit_guest_access")), ADMIN_TOKEN, @@ -514,14 +522,23 @@ async fn a_guest_minted_embed_token_stays_a_guest(db: Pool) -> anyhow: .json(&json!({ "guest_access_enabled": false })) .send() .await?; - let resp = execute(port, "test-workspace", APP_PATH, &embed) + let resp = authed(client().get(format!("{ws}/users/whoami")), &embed) .send() .await?; assert_eq!( resp.status(), - 403, - "turning guests off must stop a guest's embed token running components" + 401, + "turning guests off must stop a guest's embed token authenticating" ); + let resp = execute(port, "test-workspace", APP_PATH, &embed) + .send() + .await?; + assert!( + resp.status().is_client_error() && resp.status() != 404, + "and running components, got {}", + resp.status() + ); + enable_guests(port, "test-workspace").await?; // And its scopes are not something the guest's email can later rewrite. The // guest session itself cannot reach `/users/*` (workspace pin), so model the real @@ -566,6 +583,7 @@ async fn a_guest_label_is_governed_without_the_sentinel(db: Pool) -> a let port = server.addr.port(); let ws = format!("http://localhost:{port}/api/w/test-workspace"); + enable_guests(port, "test-workspace").await?; let scopes: Vec = guest_scopes().into_iter().filter(|s| s != "guest").collect(); sqlx::query( "INSERT INTO token (token_hash, token_prefix, token, email, label, scopes, workspace_id, expiration) diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index b37557d21b..3fb8852d16 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -138,12 +138,29 @@ impl AuthCache { w_id: Option, token: &str, ) -> Option { - let mut opt_job_authed = self.get_opt_job_authed_inner(w_id, token).await?; + let mut opt_job_authed = self.get_opt_job_authed_inner(w_id.clone(), token).await?; // Single source of truth: mirror the resolved job_id onto the authed so // every consumer (require_super_admin, ...) sees that this identity came // from a job's WM_TOKEN, even on an AUTH_CACHE hit whose cached authed // predates this field. opt_job_authed.authed.job_id = opt_job_authed.job_id; + // The workspace's guest switch is enforced here, at the door, for every + // request a guest makes — not in the handlers, where each guest-reachable + // route would have to remember to re-check it. Uncached and per request, so + // turning guests off takes effect on the next request of every guest + // session and every token derived from one. Guests are a small share of + // traffic; the read is one primary-key lookup. + if crate::scopes::has_guest_sentinel(opt_job_authed.authed.scopes.as_deref()) { + let Some(w_id) = w_id else { return None }; + match windmill_common::workspaces::is_guest_access_enabled(&self.db, &w_id).await { + Ok(true) => {} + Ok(false) => return None, + Err(e) => { + tracing::error!("guest access check failed for {w_id}: {e:#}"); + return None; + } + } + } Some(opt_job_authed) } diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 84fbb3e803..fcc1593d57 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -335,12 +335,9 @@ fn deployment_rule_for_mode(mode: ExecutionMode) -> Option { /// A guest is authorized by its token's scope and never by an ACL probe: it holds no /// `usr` row, so RLS finds nothing for it and every guest would read as having no /// access. That scope is also what keeps a guest session to the one app it was minted -/// for, even though the mode itself admits anyone signed in. The workspace switch is -/// re-read for a guest here as it is on the run path, so turning guests off closes -/// the app to sessions already issued rather than waiting out their expiry. -pub async fn authorize_non_member_viewer( - db: &DB, - w_id: &str, +/// for, even though the mode itself admits anyone signed in. The workspace's guest +/// switch is not checked here: `AuthCache` enforces it for every guest request. +pub fn authorize_non_member_viewer( mode: ExecutionMode, app_path: &str, opt_authed: &Option, @@ -356,11 +353,6 @@ pub async fn authorize_non_member_viewer( let is_guest = windmill_api_auth::scopes::has_guest_sentinel(authed.scopes.as_deref()); if matches!(mode, ExecutionMode::Guest) { if is_guest { - if !windmill_common::workspaces::is_guest_access_enabled(db, w_id).await? { - return Err(Error::PermissionDenied(format!( - "app {app_path} is not open to guests" - ))); - } check_scopes(authed, || format!("apps:read:{}", app_path))?; } return Ok(true); @@ -376,7 +368,6 @@ pub async fn authorize_non_member_viewer( /// [`authorize_non_member_viewer`] plus the member read-access probe, for the /// entry points that address an app by id. async fn authorize_app_viewer( - db: &DB, mode: ExecutionMode, app_path: &str, app_id: i64, @@ -384,7 +375,7 @@ async fn authorize_app_viewer( user_db: &UserDB, opt_authed: &Option, ) -> Result<()> { - if authorize_non_member_viewer(db, w_id, mode, app_path, opt_authed).await? { + if authorize_non_member_viewer(mode, app_path, opt_authed)? { return Ok(()); } let authed = opt_authed @@ -1330,7 +1321,6 @@ async fn get_public_app_by_secret( let policy = serde_json::from_str::(app.policy.0.get()).map_err(to_anyhow)?; authorize_app_viewer( - &db, policy.execution_mode(), &app.path, id, @@ -1859,7 +1849,7 @@ async fn get_app_embed_token( } else { ExecutionMode::Publisher }; - authorize_app_viewer(&db, mode, &app.path, id, &w_id, &user_db, &opt_authed).await?; + authorize_app_viewer(mode, &app.path, id, &w_id, &user_db, &opt_authed).await?; opt_authed }; @@ -4092,14 +4082,9 @@ async fn execute_component( // A guest session holds no ACL of its own, so the read-permit probe below would // deny every guest. What confines it is the scope the session was minted with, // naming the one app it may run — and the app has to be open to guests at all. - // - // The workspace switch is re-read here rather than trusted from mint time, so - // turning guests off stops them running code within the request, not within the - // session's remaining lifetime. One indexed lookup, and only on the guest path. + // The workspace's guest switch is enforced by `AuthCache` on every guest request. if let Some(authed) = opt_authed.as_ref().filter(|_| is_guest_caller) { - if !matches!(policy.execution_mode(), ExecutionMode::Guest) - || !windmill_common::workspaces::is_guest_access_enabled(&db, &w_id).await? - { + if !matches!(policy.execution_mode(), ExecutionMode::Guest) { return Err(Error::PermissionDenied(format!( "app {path} is not open to guests" ))); diff --git a/frontend/src/lib/components/Login.svelte b/frontend/src/lib/components/Login.svelte index acd0389a31..8629f6496a 100644 --- a/frontend/src/lib/components/Login.svelte +++ b/frontend/src/lib/components/Login.svelte @@ -644,10 +644,12 @@ markLoginMethodPending({ kind: 'saml' }) // SAML goes straight to the IdP and never passes through // `/api/oauth/login/`, where the OAuth path has the server write the - // guest-entry cookie; ask for the same write here. It must have landed before - // we leave: without it the callback provisions an account, so a failed write - // is a stop, not a fall-through. - if (!(await setGuestAppCookie(guestApp))) { + // guest-entry cookie; ask for the same write here. With a guest target it must + // have landed before we leave — without it the callback provisions an account — + // so that failure is a stop. Without one this is only a clear, and the callback + // clears on consume anyway: an ordinary SAML sign-in must not depend on it. + const wrote = await setGuestAppCookie(guestApp) + if (guestApp && !wrote) { clearPendingLoginMethod() sendUserToast('Could not start sign-in, please try again.', true) return false diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte index 10a8d2d440..eb9c5e2020 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte @@ -488,6 +488,8 @@ {#if !$enterpriseLicense} Guest sign-in is a Windmill Enterprise Edition feature, so this app still admits members only. + {:else if guestAccessEnabled === undefined} + Checking whether this workspace allows guests… {:else if guestAccessEnabled === false} Guests are turned off for this workspace, so this app still admits members only. A workspace admin can turn them on in the workspace settings. diff --git a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte index 11ee72f464..4b0a93ff90 100644 --- a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte +++ b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte @@ -213,7 +213,14 @@ * account is never given a guest session, so an account holder who is not a * member of this workspace lands here. */ let signedInHere = $state(false) - let signInDidNotHelp = $derived(status === 'noPermission' && signedInHere) + let deniedStatus: number | undefined = $state(undefined) + /** The sign-in card belongs on a 401, and on a 403 against an app that admits + * guests (a session for another app of the same workspace; signing in again + * replaces it). */ + let offerSignIn = $derived( + status === 'noPermission' || (status === 'notExists' && deniedStatus === 403 && guestEntry === 'guest') + ) + let signInDidNotHelp = $derived(offerSignIn && signedInHere) let embedToken: string | null = $state(null) let iframeEl: HTMLIFrameElement | undefined = $state(undefined) @@ -312,6 +319,10 @@ } finishReady() } catch (e: any) { + // 401: no session. 403 on an app that admits guests: a guest session for a + // different app of this workspace, which a fresh sign-in replaces. Either + // way the sign-in card is the answer; anything else is not found. + deniedStatus = e?.status status = e?.status === 401 ? 'noPermission' : 'notExists' } } @@ -533,7 +544,7 @@ {/if} {:else if status === 'loading'} -{:else if status === 'notExists'} +{:else if status === 'notExists' && !offerSignIn}
There was an error loading the app, is the url correct? @@ -548,15 +559,15 @@ onContinue={onSdkConsentContinue} onDecline={onSdkConsentDecline} /> -{:else if status === 'noPermission' && guestEntry === 'pending'} +{:else if offerSignIn && guestEntry === 'pending'} -{:else if status === 'noPermission' && guestEntry === 'error'} +{:else if offerSignIn && guestEntry === 'error'}
The app could not be reached to find out who may open it. Reload to try again.
-{:else if status === 'noPermission'} +{:else if offerSignIn} {#if signInDidNotHelp} diff --git a/frontend/src/routes/a/[...path]/+page.svelte b/frontend/src/routes/a/[...path]/+page.svelte index a874d585a5..268bf4059b 100644 --- a/frontend/src/routes/a/[...path]/+page.svelte +++ b/frontend/src/routes/a/[...path]/+page.svelte @@ -69,6 +69,9 @@ let guestEntry: 'pending' | 'none' | 'guest' | 'error' = $state('pending') async function loadGuestEntry() { + // Settled once: `loadApp` calls this again on failure, and a later transient + // fault must not overwrite an answer already in hand. + if (guestEntry === 'guest' || guestEntry === 'none') return for (let attempt = 0; attempt < 3; attempt++) { try { const entry = await AppService.getGuestEntryByCustomPath({ diff --git a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte index e63c890f32..7c1596d564 100644 --- a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte +++ b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte @@ -104,6 +104,9 @@ } async function loadGuestEntry() { + // Settled once: `loadApp` calls this again on failure, and a later transient + // fault must not overwrite an answer already in hand. + if (guestEntry === 'guest' || guestEntry === 'none') return for (let attempt = 0; attempt < 3; attempt++) { try { const entry = await AppService.getGuestEntry({ workspace, path: parsedSecret.secret })