From e03ff932805e9c816aae598db341e2e51a8f6e7e Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 16 Oct 2026 13:08:47 +0200 Subject: [PATCH] refactor: create an OAuth token variable and its account in one request createAccount takes the variable and the store creates both in one transaction, so a connect is either fully saved or not at all. The connect flows make that single call instead of creating the variable and linking it afterwards. Co-Authored-By: Claude Opus 5.5 --- ...14ae645076a85940cef890083a45301a7cd62.json | 24 ------ ...6595e3786e4489ceff5ad2e301635c4606f2e.json | 24 ------ backend/ee-repo-ref.txt | 2 +- backend/windmill-api/openapi.yaml | 8 +- backend/windmill-store/src/variables.rs | 75 +++++++++++++++++-- .../src/lib/components/AppConnectInner.svelte | 54 +++++-------- .../copilot/chat/workspaceToolsZod.gen.ts | 2 +- .../src/lib/components/mcp/McpConnect.svelte | 23 +++--- .../mcp/McpServerOAuthConnect.svelte | 30 +++----- .../src/lib/components/mcp/secretVariable.ts | 22 ++++-- 10 files changed, 130 insertions(+), 134 deletions(-) delete mode 100644 backend/.sqlx/query-92d2202c8752ab24777daa5c46b14ae645076a85940cef890083a45301a7cd62.json delete mode 100644 backend/.sqlx/query-fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e.json diff --git a/backend/.sqlx/query-92d2202c8752ab24777daa5c46b14ae645076a85940cef890083a45301a7cd62.json b/backend/.sqlx/query-92d2202c8752ab24777daa5c46b14ae645076a85940cef890083a45301a7cd62.json deleted file mode 100644 index ac011459b9..0000000000 --- a/backend/.sqlx/query-92d2202c8752ab24777daa5c46b14ae645076a85940cef890083a45301a7cd62.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM variable WHERE account = $1 AND workspace_id = $2 AND path <> $3)", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "exists", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Int4", - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "92d2202c8752ab24777daa5c46b14ae645076a85940cef890083a45301a7cd62" -} diff --git a/backend/.sqlx/query-fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e.json b/backend/.sqlx/query-fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e.json deleted file mode 100644 index de3d1d2e90..0000000000 --- a/backend/.sqlx/query-fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "WITH previous AS (SELECT account FROM variable WHERE workspace_id = $1 AND path = $2)\n UPDATE variable SET account = $3 WHERE workspace_id = $1 AND path = $2\n RETURNING (SELECT account FROM previous)", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "account", - "type_info": "Int4" - } - ], - "parameters": { - "Left": [ - "Text", - "Text", - "Int4" - ] - }, - "nullable": [ - null - ] - }, - "hash": "fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e" -} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index e970dc0a06..42e955293d 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -fa84be43efe17ec2f55a6026d48c48c064d97254 \ No newline at end of file +8ded9be3d5c9b19cc84dd5d2b3106b2ac00d2ac5 \ No newline at end of file diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index dab5fe82f7..bbf8155443 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -8811,9 +8811,9 @@ paths: items: type: string description: "OAuth scopes to use for token refresh. Overrides instance-level scopes." - variable_path: - type: string - description: "Path of an existing variable, writable by the caller, to link to this account so it refreshes through it. Its previous account is deleted when nothing else uses it." + 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 @@ -31994,7 +31994,7 @@ components: account: type: integer deprecated: true - description: Ignored. A variable is linked to an OAuth account only by `createAccount`'s `variable_path`. + 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-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 332c86ec89..83a2789d9e 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -596,6 +596,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( @@ -627,7 +670,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) { @@ -648,6 +691,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) @@ -657,9 +721,7 @@ async fn create_variable( value, variable.is_secret, variable.description, - // Reading a variable linked to an OAuth account refreshes through it and hands the reader - // the fresh token, so a caller never chooses the link: only the connect flow sets it. - None::, + account_id, variable.is_oauth.unwrap_or(false), variable.expires_at, variable.labels.as_deref() as Option<&[String]>, @@ -709,10 +771,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( diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index 279165ffbd..173de7e12b 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -988,41 +988,27 @@ // OAuth flow: single secret variable for the token if (typeof value == 'string' && value != '' && !value.startsWith('$var:')) { savedVariableCount++ - // Read once: the path field stays editable while this awaits, and the account - // must link the variable this call created. - const tokenPath = path - await VariableService.createVariable({ - workspace: effectiveWorkspace, - requestBody: { - path: tokenPath, - value: value, - is_secret: true, - description: emptyString(description) - ? `OAuth token for ${resourceType}` - : description, - is_oauth: true, - ws_specific: wsSpecific - } - }) - if (accountData) { - try { - await OauthService.createAccount({ - workspace: effectiveWorkspace, - requestBody: { ...accountData, variable_path: tokenPath } - }) - } catch (e) { - // An unlinked token never refreshes, and its path would block a retry. Not when - // filling: deleting a variable also deletes the resource at its path. - if (!filling) { - await VariableService.deleteVariable({ - workspace: effectiveWorkspace, - path: tokenPath - }).catch(() => {}) - } - throw e - } + const tokenVariable = { + path, + value: value, + is_secret: true, + description: emptyString(description) ? `OAuth token for ${resourceType}` : description, + is_oauth: true, + ws_specific: wsSpecific } - resourceValue['token'] = `$var:${tokenPath}` + 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/workspaceToolsZod.gen.ts b/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts index bdcf9848aa..77f1c8dba7 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("Ignored. A variable is linked to an OAuth account only by `createAccount`'s `variable_path`.").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 9079f1e4dc..5cf286be80 100644 --- a/frontend/src/lib/components/mcp/McpConnect.svelte +++ b/frontend/src/lib/components/mcp/McpConnect.svelte @@ -283,20 +283,17 @@ path, value: res.access_token, resourcePath: path, - isOauth: true + isOauth: true, + account: + res?.expires_in != undefined + ? { + refresh_token: res.refresh_token ?? '', + expires_in: res.expires_in, + client: t.client, + scopes: t.scopes + } + : undefined }) - if (res?.expires_in != undefined) { - await OauthService.createAccount({ - workspace, - requestBody: { - refresh_token: res.refresh_token ?? '', - expires_in: res.expires_in, - client: t.client, - scopes: t.scopes, - variable_path: path - } - }) - } await createMcpResource(t, `$var:${path}`) sendUserToast(`Connected ${t.label}`) } catch (e) { diff --git a/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte b/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte index 606739e9c2..3d721d6882 100644 --- a/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte +++ b/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte @@ -1,10 +1,5 @@