mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-07 00:01:49 +00:00
fix: the label alone governs a guest; refuse guests with accounts; unserialize discovery
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5
This commit is contained in:
Ruben Fiszel
co-authored by
Claude Opus 5
Claude Opus 5
parent
27cb199e0a
commit
b317002dd6
Generated
+2
@@ -14790,6 +14790,7 @@ dependencies = [
|
||||
"tikv-jemallocator",
|
||||
"tokio",
|
||||
"tokio-stream",
|
||||
"tower-cookies",
|
||||
"tracing",
|
||||
"tracing-subscriber",
|
||||
"url",
|
||||
@@ -14801,6 +14802,7 @@ dependencies = [
|
||||
"windmill-api-client",
|
||||
"windmill-api-scripts",
|
||||
"windmill-api-settings",
|
||||
"windmill-api-users",
|
||||
"windmill-autoscaling",
|
||||
"windmill-common",
|
||||
"windmill-dep-map",
|
||||
|
||||
@@ -351,6 +351,8 @@ windmill-trigger-sqs.workspace = true
|
||||
windmill-trigger-gcp.workspace = true
|
||||
windmill-trigger-azure.workspace = true
|
||||
windmill-api-auth.workspace = true
|
||||
tower-cookies.workspace = true
|
||||
windmill-api-users.workspace = true
|
||||
axum.workspace = true
|
||||
serde.workspace = true
|
||||
windmill-api-client.workspace = true
|
||||
|
||||
@@ -1 +1 @@
|
||||
60ea364b42bd6e099d46522f3399274d25925ba7
|
||||
fdb2f5b5df33776bc440a12e39bcc6b65981bd37
|
||||
|
||||
@@ -554,3 +554,92 @@ async fn a_guest_minted_embed_token_stays_a_guest(db: Pool<Postgres>) -> anyhow:
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// The label is the single source of truth: a guest-labelled credential is governed
|
||||
/// as a guest even if its scopes carry no sentinel. Otherwise every mint that derives
|
||||
/// a token from a guest session is one forgotten `push` away from an ungoverned
|
||||
/// non-member credential.
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn a_guest_label_is_governed_without_the_sentinel(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
let ws = format!("http://localhost:{port}/api/w/test-workspace");
|
||||
|
||||
let scopes: Vec<String> = 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)
|
||||
VALUES (encode(sha256($1::bytea), 'hex'), 'NOSENTINE_', $2, 'guest@example.com',
|
||||
'guest_session', $3, 'test-workspace', now() + interval '8 hours')",
|
||||
)
|
||||
.bind(b"NOSENTINEL".as_slice())
|
||||
.bind("NOSENTINEL")
|
||||
.bind(scopes)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
let resp = authed(client().get(format!("{ws}/users/whoami")), "NOSENTINEL")
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 200);
|
||||
let me: serde_json::Value = resp.json().await?;
|
||||
assert_eq!(
|
||||
me["role"],
|
||||
json!("guest"),
|
||||
"the label alone must make a credential a guest"
|
||||
);
|
||||
let resp = authed(client().get(format!("{ws}/jobs/list")), "NOSENTINEL")
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 403, "and confine it like one");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A guest is someone with no account at all — including a deactivated one. The
|
||||
/// sign-in path's own account lookup filters on `disabled = false`, so a disabled
|
||||
/// account reads as absent there; the mint has to refuse on its own or deactivation
|
||||
/// (manual or SCIM, whose revocation is "delete the tokens") walks straight back in.
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn a_disabled_account_cannot_become_a_guest(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
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?;
|
||||
let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN)
|
||||
.json(&guest_app_with_runnable(APP_PATH, false))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 201, "{}", resp.text().await?);
|
||||
sqlx::query(
|
||||
"INSERT INTO password (email, password_hash, login_type, super_admin, verified, disabled)
|
||||
VALUES ('gone@example.com', 'x', 'password', false, true, true)",
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
let mut tx = db.begin().await?;
|
||||
let cookies = tower_cookies::Cookies::default();
|
||||
let minted = windmill_api_users::users::create_guest_session_token(
|
||||
"gone@example.com",
|
||||
"test-workspace",
|
||||
APP_PATH,
|
||||
&mut tx,
|
||||
cookies,
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
matches!(minted, Err(windmill_common::error::Error::NotAuthorized(_))),
|
||||
"a deactivated account must be refused a guest session, got {minted:?}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -527,6 +527,15 @@ impl AuthCache {
|
||||
// scopes on a user-minted token are whatever
|
||||
// the caller typed.
|
||||
None if is_guest_session => {
|
||||
// The label is the grant; every guest
|
||||
// control downstream keys on the `guest`
|
||||
// sentinel. Pin the two together here so
|
||||
// a credential that carries the label is
|
||||
// governed as a guest whatever its scopes
|
||||
// say — nothing else may decide that.
|
||||
let scopes = Some(crate::scopes::with_guest_sentinel(
|
||||
scopes.unwrap_or_default(),
|
||||
));
|
||||
Some(ApiAuthed {
|
||||
username: email.clone(),
|
||||
email,
|
||||
|
||||
@@ -785,6 +785,14 @@ pub fn has_guest_sentinel(scopes: Option<&[String]>) -> bool {
|
||||
scopes.is_some_and(|s| s.iter().any(|x| x == GUEST_SENTINEL))
|
||||
}
|
||||
|
||||
/// `scopes` with the guest sentinel present exactly once.
|
||||
pub fn with_guest_sentinel(mut scopes: Vec<String>) -> Vec<String> {
|
||||
if !scopes.iter().any(|x| x == GUEST_SENTINEL) {
|
||||
scopes.push(GUEST_SENTINEL.to_string());
|
||||
}
|
||||
scopes
|
||||
}
|
||||
|
||||
/// Sentinel in raw-app SDK tokens. Grants nothing; `check_route_access` uses it
|
||||
/// to narrow the declared scopes to what the viewer's prompt promised.
|
||||
pub const RAW_APP_SDK_SENTINEL: &str = "raw_app_sdk";
|
||||
|
||||
@@ -2994,6 +2994,19 @@ pub async fn create_guest_session_token<'c>(
|
||||
};
|
||||
let scopes = guest_session_scopes(app_path);
|
||||
|
||||
// A guest is someone with no account at all. Checked here, not only by the caller,
|
||||
// because the sign-in path's own account lookup filters on `disabled = false` —
|
||||
// a deactivated account (manual or SCIM, whose revocation is "delete the tokens")
|
||||
// would otherwise read as absent and walk straight back in as a guest.
|
||||
let has_account: bool = sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM password WHERE email = $1)")
|
||||
.bind(email)
|
||||
.fetch_one(&mut **tx)
|
||||
.await?;
|
||||
if has_account {
|
||||
return Err(Error::NotAuthorized(
|
||||
"an existing account cannot hold a guest session".to_string(),
|
||||
));
|
||||
}
|
||||
if !windmill_common::workspaces::guest_app_admits(&mut **tx, w_id, app_path).await? {
|
||||
return Err(Error::NotAuthorized(format!(
|
||||
"app {app_path} is not open to guests"
|
||||
|
||||
@@ -1540,7 +1540,22 @@ pub async fn build_embed_token_response(
|
||||
&& opt_authed.is_some()
|
||||
&& !policy.frontend_sdk_scopes.is_empty()
|
||||
{
|
||||
Some(policy.frontend_sdk_scopes.clone())
|
||||
// An SDK token runs as the viewer, and a guest's session is the ceiling on what
|
||||
// it may delegate — the mint enforces that. Advertise only what a guest can
|
||||
// actually be granted, so the consent prompt never promises a scope the mint
|
||||
// would then refuse.
|
||||
let declared = policy.frontend_sdk_scopes.clone();
|
||||
let offered = match opt_authed {
|
||||
Some(a) if windmill_api_auth::scopes::has_guest_sentinel(a.scopes.as_deref()) => {
|
||||
let held = a.scopes.as_deref().unwrap_or_default();
|
||||
declared
|
||||
.into_iter()
|
||||
.filter(|sc| held.iter().any(|h| h == sc))
|
||||
.collect::<Vec<_>>()
|
||||
}
|
||||
_ => declared,
|
||||
};
|
||||
(!offered.is_empty()).then_some(offered)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
@@ -404,7 +404,9 @@
|
||||
if (autoRedirect && autoLogin && !error && !shouldSkipAutoRedirect()) {
|
||||
if (autoLogin === 'saml' && saml) {
|
||||
autoRedirecting = true
|
||||
if (!redirectSaml()) autoRedirecting = false
|
||||
redirectSaml().then((ok) => {
|
||||
if (!ok) autoRedirecting = false
|
||||
})
|
||||
} else if (logins?.some((l) => l.type === autoLogin)) {
|
||||
autoRedirecting = true
|
||||
if (!storeRedirect(autoLogin)) {
|
||||
@@ -596,16 +598,14 @@
|
||||
console.log('oauth: popup closed before login completed')
|
||||
return
|
||||
}
|
||||
// A guest session is pinned to its workspace and cannot answer the global
|
||||
// probe; an ordinary session for a non-member cannot answer the workspace
|
||||
// one. Either answering means the popup signed someone in.
|
||||
const guestWorkspace = guestApp?.split('/')[0]
|
||||
const probes: Promise<unknown>[] = [UserService.getCurrentEmail()]
|
||||
if (guestWorkspace) probes.push(UserService.whoami({ workspace: guestWorkspace }))
|
||||
try {
|
||||
// A guest session is pinned to its workspace and cannot authenticate on
|
||||
// any workspace-less route, so the global probe would 401 forever and this
|
||||
// fallback would never complete a guest sign-in.
|
||||
const guestWorkspace = guestApp?.split('/')[0]
|
||||
if (guestWorkspace) {
|
||||
await UserService.whoami({ workspace: guestWorkspace })
|
||||
} else {
|
||||
await UserService.getCurrentEmail()
|
||||
}
|
||||
await Promise.any(probes)
|
||||
} catch {
|
||||
return
|
||||
}
|
||||
@@ -614,24 +614,26 @@
|
||||
}, 1500)
|
||||
}
|
||||
|
||||
/** The SAML counterpart of the cookie the OAuth `login` handler writes server-side
|
||||
* (`set_unsensitive_cookie`), including clearing it when this sign-in is not a
|
||||
* guest entry. `login_externally` consumes it. `SameSite=None` is required: the
|
||||
* SAML ACS is a cross-site POST from the IdP, and a Lax cookie is not sent on
|
||||
* those. `None` needs `Secure`, so this only survives the round trip over https;
|
||||
* over plain http the browser drops it and a guest sign-in falls through to
|
||||
* ordinary provisioning. Host-only: a `COOKIE_DOMAIN` deployment that serves the
|
||||
* ACS from a different host than this page would need the domain set here too. */
|
||||
function setGuestAppCookie(value: string | undefined) {
|
||||
/** Have the server write the guest-entry cookie, as the OAuth `login` handler does
|
||||
* on its own path. SAML goes straight to the IdP and never passes through `login`,
|
||||
* and a browser-set cookie would be host-only — on a `COOKIE_DOMAIN` deployment
|
||||
* whose ACS answers on a sibling host it would never arrive. Server-set, it carries
|
||||
* the same attributes as every other session cookie. Cleared (empty) when this
|
||||
* sign-in is not a guest entry. `login_externally` consumes it. */
|
||||
async function setGuestAppCookie(value: string | undefined) {
|
||||
try {
|
||||
const secure = window.location.protocol === 'https:' ? '; Secure' : ''
|
||||
document.cookie = `guest_app=${encodeURIComponent(value ?? '')}; path=/; SameSite=None${secure}`
|
||||
await fetch(`${base}/api/oauth/guest_app`, {
|
||||
method: 'POST',
|
||||
credentials: 'include',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ guest_app: value ?? '' })
|
||||
})
|
||||
} catch (e) {
|
||||
console.error('Could not set the guest app cookie', e)
|
||||
}
|
||||
}
|
||||
|
||||
function redirectSaml(): boolean {
|
||||
async function redirectSaml(): Promise<boolean> {
|
||||
if (!saml) {
|
||||
sendUserToast('No SAML login available', true)
|
||||
return false
|
||||
@@ -644,7 +646,7 @@
|
||||
// too. Client-set is safe: the callback still checks that the named app is in
|
||||
// guest mode and that the workspace allows guests, so the worst a forged value
|
||||
// can do is give its own author a narrower session than they'd otherwise get.
|
||||
setGuestAppCookie(guestApp)
|
||||
await setGuestAppCookie(guestApp)
|
||||
let target = saml
|
||||
let relayStateSet = false
|
||||
// Carry the SP-initiated deep link through the IdP round-trip via SAML
|
||||
|
||||
@@ -50,7 +50,8 @@
|
||||
onViewerReady,
|
||||
viewer,
|
||||
viewerUrl,
|
||||
guestAppPath = undefined
|
||||
guestAppPath = undefined,
|
||||
guestEntryResolved = true
|
||||
}: {
|
||||
/** Embedder-side: validate access + mint the scoped token. Throws with a
|
||||
* `.status` of 401 (login required) or 404 (not found). Pass
|
||||
@@ -74,6 +75,10 @@
|
||||
* up front and pass it down — otherwise a signed-out visitor is offered an
|
||||
* ordinary sign-in that creates an account and still cannot open the app. */
|
||||
guestAppPath?: string | undefined
|
||||
/** False while the page is still finding out whether the app admits guests. The
|
||||
* sign-in card waits for it (a configured auto-login would otherwise start an
|
||||
* ordinary sign-in); nothing else does. */
|
||||
guestEntryResolved?: boolean
|
||||
} = $props()
|
||||
|
||||
const EMBED_PARAM = 'wm_embed'
|
||||
@@ -542,6 +547,8 @@
|
||||
onContinue={onSdkConsentContinue}
|
||||
onDecline={onSdkConsentDecline}
|
||||
/>
|
||||
{:else if status === 'noPermission' && !guestEntryResolved}
|
||||
<Skeleton layout={[[4], 0.5, [50]]} />
|
||||
{:else if status === 'noPermission'}
|
||||
<!-- Login happens here, on the embedder (main) window, so the session cookie
|
||||
is set on the main origin only and never reaches the opaque iframe. -->
|
||||
|
||||
@@ -61,8 +61,9 @@
|
||||
/** `<workspace>/<app_path>` when this app is open to guests. Resolved eagerly:
|
||||
* PublicAppFrame renders its sign-in gate before `onViewerReady` fires. */
|
||||
let guestAppPath: string | undefined = $state(undefined)
|
||||
/** The frame's sign-in gate must not mount before this is known: a configured
|
||||
* auto-login would otherwise fire an ordinary sign-in and provision an account. */
|
||||
/** The frame's sign-in card must not mount before this is known: a configured
|
||||
* auto-login would otherwise fire an ordinary sign-in and provision an account.
|
||||
* Only the card waits — the app load itself runs in parallel with discovery. */
|
||||
let guestEntryResolved = $state(false)
|
||||
|
||||
async function loadGuestEntry() {
|
||||
@@ -148,11 +149,11 @@
|
||||
}
|
||||
</script>
|
||||
|
||||
{#if guestEntryResolved}
|
||||
<PublicAppFrame
|
||||
{fetchEmbedToken}
|
||||
{viewerUrl}
|
||||
{guestAppPath}
|
||||
{guestEntryResolved}
|
||||
onViewerReady={(_token, requestTokenRefresh) => {
|
||||
refresh = requestTokenRefresh
|
||||
loadApp()
|
||||
@@ -170,4 +171,3 @@
|
||||
></PublicApp>
|
||||
{/snippet}
|
||||
</PublicAppFrame>
|
||||
{/if}
|
||||
|
||||
@@ -20,8 +20,9 @@
|
||||
* offer a guest session rather than a dead end. 404 (the common case) leaves it
|
||||
* undefined. */
|
||||
let guestAppPath: string | undefined = $state(undefined)
|
||||
/** The frame's sign-in gate must not mount before this is known: a configured
|
||||
* auto-login would otherwise fire an ordinary sign-in and provision an account. */
|
||||
/** The frame's sign-in card must not mount before this is known: a configured
|
||||
* auto-login would otherwise fire an ordinary sign-in and provision an account.
|
||||
* Only the card waits — the app load itself runs in parallel with discovery. */
|
||||
let guestEntryResolved = $state(false)
|
||||
|
||||
function parseSecret(secret: string): { secret: string; jwt: string | undefined } {
|
||||
@@ -124,11 +125,11 @@
|
||||
}
|
||||
</script>
|
||||
|
||||
{#if guestEntryResolved}
|
||||
<PublicAppFrame
|
||||
{fetchEmbedToken}
|
||||
{viewerUrl}
|
||||
{guestAppPath}
|
||||
{guestEntryResolved}
|
||||
onViewerReady={(_token, requestTokenRefresh) => {
|
||||
refresh = requestTokenRefresh
|
||||
loadApp()
|
||||
@@ -146,4 +147,3 @@
|
||||
></PublicApp>
|
||||
{/snippet}
|
||||
</PublicAppFrame>
|
||||
{/if}
|
||||
|
||||
Reference in New Issue
Block a user