diff --git a/backend/.sqlx/query-e910ae81fb98642e76644e9691157c3d02adce3ba55324375a2035231c640ac0.json b/backend/.sqlx/query-086866581c8f253a611d85cc9464e63767da94af0fcb6d660cd6fb0479b9a99d.json similarity index 53% rename from backend/.sqlx/query-e910ae81fb98642e76644e9691157c3d02adce3ba55324375a2035231c640ac0.json rename to backend/.sqlx/query-086866581c8f253a611d85cc9464e63767da94af0fcb6d660cd6fb0479b9a99d.json index 8805e4ee22..e9183fe2d4 100644 --- a/backend/.sqlx/query-e910ae81fb98642e76644e9691157c3d02adce3ba55324375a2035231c640ac0.json +++ b/backend/.sqlx/query-086866581c8f253a611d85cc9464e63767da94af0fcb6d660cd6fb0479b9a99d.json @@ -1,16 +1,17 @@ { "db_name": "PostgreSQL", - "query": "SELECT EXISTS (SELECT 1 FROM account WHERE workspace_id = $1 AND id = $2) AS \"exists!\"", + "query": "SELECT EXISTS(SELECT 1 FROM variable WHERE workspace_id = $1 AND path = $2 AND account = $3)", "describe": { "columns": [ { "ordinal": 0, - "name": "exists!", + "name": "exists", "type_info": "Bool" } ], "parameters": { "Left": [ + "Text", "Text", "Int4" ] @@ -19,5 +20,5 @@ null ] }, - "hash": "e910ae81fb98642e76644e9691157c3d02adce3ba55324375a2035231c640ac0" + "hash": "086866581c8f253a611d85cc9464e63767da94af0fcb6d660cd6fb0479b9a99d" } diff --git a/backend/.sqlx/query-7aa589db3199d7f727cc69e63e1281b7ed329ff0c9d1617747f4ccd6014720cf.json b/backend/.sqlx/query-7aa589db3199d7f727cc69e63e1281b7ed329ff0c9d1617747f4ccd6014720cf.json deleted file mode 100644 index b2101cc0d4..0000000000 --- a/backend/.sqlx/query-7aa589db3199d7f727cc69e63e1281b7ed329ff0c9d1617747f4ccd6014720cf.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM variable WHERE account = $1 AND workspace_id = $2)", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "exists", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Int4", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "7aa589db3199d7f727cc69e63e1281b7ed329ff0c9d1617747f4ccd6014720cf" -} diff --git a/backend/.sqlx/query-c925264b7b0fd44ea7ab01c9af1514b9a9f2200e5a5db0a741697b28cd8b505f.json b/backend/.sqlx/query-c925264b7b0fd44ea7ab01c9af1514b9a9f2200e5a5db0a741697b28cd8b505f.json deleted file mode 100644 index 37025d99d8..0000000000 --- a/backend/.sqlx/query-c925264b7b0fd44ea7ab01c9af1514b9a9f2200e5a5db0a741697b28cd8b505f.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT account FROM variable WHERE path = $1 AND workspace_id = $2", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "account", - "type_info": "Int4" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - true - ] - }, - "hash": "c925264b7b0fd44ea7ab01c9af1514b9a9f2200e5a5db0a741697b28cd8b505f" -} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 66b973f850..f6c4caccbe 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -c8d562aa9d75a7e99bf0ecd1ac224f532452a07e +da49c12e3006fc77de789277f1462a4d8d7d92ce diff --git a/backend/windmill-api-integration-tests/tests/variables.rs b/backend/windmill-api-integration-tests/tests/variables.rs index e5018f4f97..7332383936 100644 --- a/backend/windmill-api-integration-tests/tests/variables.rs +++ b/backend/windmill-api-integration-tests/tests/variables.rs @@ -338,3 +338,65 @@ async fn test_variables_are_cached(db: Pool) -> anyhow::Result<()> { Ok(()) } + +/// Reading a variable linked to an OAuth account refreshes through it and returns the fresh token, +/// so the variable endpoints must not let a caller choose the link. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_variable_endpoints_ignore_account_link(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/variables"); + + let account: i32 = sqlx::query_scalar( + "INSERT INTO account (workspace_id, client, expires_at, refresh_token) + VALUES ('test-workspace', 'github', now(), 'r') RETURNING id", + ) + .fetch_one(&db) + .await?; + let link = |path: &'static str| { + let db = db.clone(); + async move { + sqlx::query_scalar::<_, Option>( + "SELECT account FROM variable WHERE workspace_id = 'test-workspace' AND path = $1", + ) + .bind(path) + .fetch_one(&db) + .await + .unwrap() + } + }; + + let path = "u/test-user/oauth_probe"; + let resp = authed(client().post(format!("{base}/create"))) + .json( + &json!({ "path": path, "value": "t", "is_secret": true, "description": "", + "is_oauth": true, "account": account }), + ) + .send() + .await?; + assert_eq!(resp.status(), 201); + assert_eq!(link(path).await, None); + + let other: i32 = sqlx::query_scalar( + "INSERT INTO account (workspace_id, client, expires_at, refresh_token) + VALUES ('test-workspace', 'github', now(), 'r') RETURNING id", + ) + .fetch_one(&db) + .await?; + sqlx::query( + "UPDATE variable SET account = $1 WHERE workspace_id = 'test-workspace' AND path = $2", + ) + .bind(account) + .bind(path) + .execute(&db) + .await?; + let resp = authed(client().post(variable_url(port, "update", path))) + .json(&json!({ "description": "d", "account": other })) + .send() + .await?; + assert_eq!(resp.status(), 200); + assert_eq!(link(path).await, Some(account)); + + Ok(()) +} diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index edbd4165d9..0118184af1 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -8894,6 +8894,9 @@ paths: items: type: string description: "OAuth scopes to use for token refresh. Overrides instance-level scopes." + variable: + $ref: "#/components/schemas/CreateVariable" + description: "The variable holding this account's access token. It is created with the account, in one transaction and with the same checks as createVariable, and linked to it so it refreshes through it." required: - refresh_token - expires_in @@ -32523,7 +32526,8 @@ components: description: The description of the variable account: type: integer - description: The account identifier + deprecated: true + description: Ignored. A variable is linked to an OAuth account only when `createAccount` creates both. is_oauth: type: boolean description: Whether the variable is an OAuth variable diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index 39227e49ee..87496fed45 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -968,11 +968,13 @@ pub(crate) async fn tarball_workspace( } if !skip_variables.unwrap_or(false) { + // `account` is left out: the variable endpoints ignore it, so a pushed spec carrying it + // would never match the variable it created and every sync would report it changed. let variables = sqlx::query_as::<_, ExportableListableVariable>(if !skip_secrets.unwrap_or(false) { - "SELECT workspace_id, path, value, is_secret, description, extra_perms, account, is_oauth, expires_at, labels FROM variable WHERE workspace_id = $1 AND expires_at IS NULL" + "SELECT workspace_id, path, value, is_secret, description, extra_perms, NULL::int AS account, is_oauth, expires_at, labels FROM variable WHERE workspace_id = $1 AND expires_at IS NULL" } else { - "SELECT workspace_id, path, value, is_secret, description, extra_perms, account, is_oauth, expires_at, labels FROM variable WHERE workspace_id = $1 AND is_secret = false AND expires_at IS NULL" + "SELECT workspace_id, path, value, is_secret, description, extra_perms, NULL::int AS account, is_oauth, expires_at, labels FROM variable WHERE workspace_id = $1 AND is_secret = false AND expires_at IS NULL" }) .bind(&w_id) .fetch_all(&mut *tx) diff --git a/backend/windmill-store/src/oauth_refresh_oss.rs b/backend/windmill-store/src/oauth_refresh_oss.rs index 17d49aae4f..3d55b05167 100644 --- a/backend/windmill-store/src/oauth_refresh_oss.rs +++ b/backend/windmill-store/src/oauth_refresh_oss.rs @@ -180,23 +180,6 @@ where Ok(token.access_token) } -/// Whether account `id` exists in `w_id`, read through the caller's user transaction. `account` -/// carries no RLS, so this gates nothing beyond the workspace the caller is already authorized -/// for: a refresh asked for by account id is scoped by that membership, not by ownership. -pub async fn account_visible( - user_conn: &mut sqlx::PgConnection, - w_id: &str, - id: i32, -) -> error::Result { - Ok(sqlx::query_scalar!( - "SELECT EXISTS (SELECT 1 FROM account WHERE workspace_id = $1 AND id = $2) AS \"exists!\"", - w_id, - id - ) - .fetch_one(user_conn) - .await?) -} - async fn current_token(db: &DB, w_id: &str, path: &str) -> error::Result { let value = sqlx::query_scalar!( "SELECT value FROM variable WHERE workspace_id = $1 AND path = $2", diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 0b0660f967..6a4c34e7dd 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -594,6 +594,49 @@ async fn create_variable( Query(AlreadyEncrypted { already_encrypted }): Query, Json(variable): Json, ) -> Result<(StatusCode, String)> { + let path = variable.path.clone(); + create_variable_with_account( + authed, + &db, + user_db, + &webhook, + w_id, + variable, + already_encrypted.unwrap_or(false), + None, + ) + .await?; + Ok((StatusCode::CREATED, format!("variable {path} created"))) +} + +/// An OAuth account to create with the variable that holds its access token. +pub struct NewOAuthAccount { + pub client: String, + pub expires_in: i64, + pub refresh_token: Option, + pub grant_type: String, + pub cc_client_id: Option, + pub cc_client_secret: Option, + pub cc_token_url: Option, + pub mcp_server_url: Option, + pub scopes: Option>, +} + +/// Creates a variable and, given `account`, the OAuth account it refreshes through, linked in the +/// same transaction. Reading a linked variable refreshes through its account and hands the reader +/// the fresh token, so this is the only way a variable gets a link: `variable.account` is ignored. +/// Returns the new account's id. +pub async fn create_variable_with_account( + authed: ApiAuthed, + db: &DB, + user_db: UserDB, + webhook: &WebhookShared, + w_id: String, + variable: CreateVariable, + already_encrypted: bool, + account: Option, +) -> Result> { + let db = db.clone(); check_scopes(&authed, || format!("variables:write:{}", variable.path))?; check_proper_path(&variable.path)?; if let RuleCheckResult::Blocked(msg) = check_deploy_rules( @@ -625,7 +668,7 @@ async fn create_variable( let authed = maybe_refresh_folders(&variable.path, &w_id, authed, &db).await; check_path_conflict(&db, &w_id, &variable.path).await?; - let value = if variable.is_secret && !already_encrypted.unwrap_or(false) { + let value = if variable.is_secret && !already_encrypted { // A restored draft sends the `$encrypted:` marker as-is; decrypt it back // (validating against the workspace key) before the secret backend re-stores it. let plain = if variable.value.starts_with(ENCRYPTED_DRAFT_PREFIX) { @@ -646,6 +689,27 @@ async fn create_variable( let mut tx = user_db.begin(&authed).await?; + let account_id = match account { + Some(a) => Some( + sqlx::query_scalar!( + "INSERT INTO account (workspace_id, client, expires_at, refresh_token, grant_type, cc_client_id, cc_client_secret, cc_token_url, mcp_server_url, scopes) VALUES ($1, $2, now() + ($3 || ' seconds')::interval, $4, $5, $6, $7, $8, $9, $10) RETURNING id", + &w_id, + a.client, + a.expires_in.to_string(), + a.refresh_token, + a.grant_type, + a.cc_client_id, + a.cc_client_secret, + a.cc_token_url, + a.mcp_server_url, + a.scopes.as_deref() + ) + .fetch_one(&mut *tx) + .await?, + ), + None => None, + }; + sqlx::query!( "INSERT INTO variable (workspace_id, path, value, is_secret, description, account, is_oauth, expires_at, labels, edited_by) @@ -655,7 +719,7 @@ async fn create_variable( value, variable.is_secret, variable.description, - variable.account, + account_id, variable.is_oauth.unwrap_or(false), variable.expires_at, variable.labels.as_deref() as Option<&[String]>, @@ -705,10 +769,7 @@ async fn create_variable( WebhookMessage::CreateVariable { workspace: w_id, path: variable.path.clone() }, ); - Ok(( - StatusCode::CREATED, - format!("variable {} created", variable.path), - )) + Ok(account_id) } async fn encrypt_value( @@ -1059,7 +1120,6 @@ struct EditVariable { value: Option, is_secret: Option, description: Option, - account: Option, labels: Option>, ws_specific: Option, } @@ -1156,11 +1216,6 @@ async fn update_variable( has_sql_updates = true; } - if let Some(account_id) = ns.account { - sqlb.set_str("account", account_id); - has_sql_updates = true; - } - if let Some(nbool) = ns.is_secret { let old_secret = sqlx::query_scalar!( "SELECT is_secret from variable WHERE path = $1 AND workspace_id = $2", @@ -1179,20 +1234,6 @@ async fn update_variable( has_sql_updates = true; } - // Get old account_id if we're updating the account field - let old_account_id = if ns.account.is_some() { - sqlx::query_scalar!( - "SELECT account FROM variable WHERE path = $1 AND workspace_id = $2", - &path, - &w_id - ) - .fetch_optional(&db) - .await? - .flatten() - } else { - None - }; - let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; if let Some(npath) = ns.path.clone() { @@ -1366,32 +1407,6 @@ async fn update_variable( ) .await?; - // Clean up old account if it's no longer referenced and different from new account - if let Some(old_acc_id) = old_account_id { - if ns.account.is_some() && ns.account != Some(old_acc_id) { - // Check if old account is still referenced by other variables or resources - let account_still_used = sqlx::query_scalar!( - "SELECT EXISTS(SELECT 1 FROM variable WHERE account = $1 AND workspace_id = $2)", - old_acc_id, - &w_id - ) - .fetch_one(&mut *tx) - .await? - .unwrap_or(true); - - if !account_still_used { - // Delete the orphaned account - sqlx::query!( - "DELETE FROM account WHERE id = $1 AND workspace_id = $2", - old_acc_id, - &w_id - ) - .execute(&mut *tx) - .await?; - } - } - } - tx.commit().await?; // Detect if this was a rename operation diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index 4dc05972ca..173de7e12b 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -939,9 +939,9 @@ args['realmId'] = responseExtra['realmId'] } - let account: number | undefined = undefined + let accountData: any = undefined if (valueToken?.expires_in != undefined) { - const accountData: any = { + accountData = { refresh_token: valueToken.refresh_token ?? '', expires_in: valueToken.expires_in, client: connectClient, @@ -970,13 +970,6 @@ accountData.cc_token_url = tokenUrl.trim() } } - - account = Number( - await OauthService.createAccount({ - workspace: effectiveWorkspace, - requestBody: accountData - }) - ) } // A copy: the form is still mounted and bound to `args` across the awaits below, and @@ -995,21 +988,27 @@ // OAuth flow: single secret variable for the token if (typeof value == 'string' && value != '' && !value.startsWith('$var:')) { savedVariableCount++ - await VariableService.createVariable({ - workspace: effectiveWorkspace, - requestBody: { - path, - value: value, - is_secret: true, - description: emptyString(description) - ? `OAuth token for ${resourceType}` - : description, - is_oauth: true, - account: account, - ws_specific: wsSpecific - } - }) - resourceValue['token'] = `$var:${path}` + const tokenVariable = { + path, + value: value, + is_secret: true, + description: emptyString(description) ? `OAuth token for ${resourceType}` : description, + is_oauth: true, + ws_specific: wsSpecific + } + if (accountData) { + // Creates the variable with the account it refreshes through, in one transaction. + await OauthService.createAccount({ + workspace: effectiveWorkspace, + requestBody: { ...accountData, variable: tokenVariable } + }) + } else { + await VariableService.createVariable({ + workspace: effectiveWorkspace, + requestBody: tokenVariable + }) + } + resourceValue['token'] = `$var:${tokenVariable.path}` } } else if (linkedSecrets.length === 1) { // Single secret: use the resource path as variable name (original behavior) diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index c832fbfc1f..03356a51f0 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -2854,7 +2854,7 @@ function getResourceInstructions(): string { - Global mode writes complete draft payloads only; it does not save, deploy, run, scaffold local files, or generate metadata. - A resource draft is a workspace item: \`{ type: 'resource', path, summary?, value, isDraft }\`. \`value\` is a CreateResource body: \`{ path, value, description?, resource_type, labels? }\` where the inner \`value\` is the resource type's data shape. - Reading a variable returns \`{ type: 'variable', path, summary?, isSecret, isDraft }\` — never its value, secret or not. \`isSecret\` tells you whether the value is encrypted. -- \`write_variable\` takes \`{ path, value?, is_secret?, description?, account?, is_oauth?, expires_at?, labels? }\`. Creating a variable needs \`value\` and \`is_secret\`; editing one needs only the fields you are changing. Omitting \`value\` keeps the stored value, which is the only way to edit a secret variable — you cannot read its value, so passing any \`value\` you did not get from the user destroys it. +- \`write_variable\` takes \`{ path, value?, is_secret?, description?, is_oauth?, expires_at?, labels? }\`. Creating a variable needs \`value\` and \`is_secret\`; editing one needs only the fields you are changing. Omitting \`value\` keeps the stored value, which is the only way to edit a secret variable — you cannot read its value, so passing any \`value\` you did not get from the user destroys it. - For secret fields in a resource value, create the variable with \`write_variable\` and \`is_secret: true\`, and deploy it before the resource (see "Secrets" in the reference below). - Reference formats inside resource values: \`$var:g/all/name\` (global), \`$var:u/user/name\` (user), \`$var:f/folder/name\` (folder). Reference another resource with \`$res:path/to/resource\`. The same strings are also how a resource or variable is passed as a run argument (see the run-argument rule in the resource reference below); what they are never valid as is a variable's own value. - Use \`search_resource_types\` to discover valid \`resource_type\` names and their JSON Schemas. Match the resource value to that schema. diff --git a/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts b/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts index c6b03cee68..e4817e190c 100644 --- a/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts +++ b/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts @@ -488,7 +488,7 @@ export const variableRequestSchema = z.object({ "value": z.string().describe("The value of the variable"), "is_secret": z.boolean().describe("Whether the variable is a secret"), "description": z.string().describe("The description of the variable"), - "account": z.number().int().describe("The account identifier").optional(), + "account": z.number().int().describe("Ignored. A variable is linked to an OAuth account only when `createAccount` creates both.").optional(), "is_oauth": z.boolean().describe("Whether the variable is an OAuth variable").optional(), "expires_at": z.string().datetime({ offset: true }).describe("The expiration date of the variable").optional(), "labels": z.array(z.string()).optional(), diff --git a/frontend/src/lib/components/mcp/McpConnect.svelte b/frontend/src/lib/components/mcp/McpConnect.svelte index a8808aa052..5cf286be80 100644 --- a/frontend/src/lib/components/mcp/McpConnect.svelte +++ b/frontend/src/lib/components/mcp/McpConnect.svelte @@ -278,27 +278,21 @@ if (!t) return const { workspace, path } = t try { - let account: number | undefined = undefined - if (res?.expires_in != undefined) { - account = Number( - await OauthService.createAccount({ - workspace, - requestBody: { - refresh_token: res.refresh_token ?? '', - expires_in: res.expires_in, - client: t.client, - scopes: t.scopes - } - }) - ) - } await upsertSecretVariable({ workspace, path, value: res.access_token, resourcePath: path, isOauth: true, - account + account: + res?.expires_in != undefined + ? { + refresh_token: res.refresh_token ?? '', + expires_in: res.expires_in, + client: t.client, + scopes: t.scopes + } + : undefined }) await createMcpResource(t, `$var:${path}`) sendUserToast(`Connected ${t.label}`) diff --git a/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte b/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte index bafbf4b7b4..3d721d6882 100644 --- a/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte +++ b/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte @@ -1,10 +1,5 @@