diff --git a/backend/.sqlx/query-1f35a009e0740cbd847631ecfdeb88de508c463d11b4dce75f9a0b62c88e7a52.json b/backend/.sqlx/query-1f35a009e0740cbd847631ecfdeb88de508c463d11b4dce75f9a0b62c88e7a52.json deleted file mode 100644 index d9adeff46f..0000000000 --- a/backend/.sqlx/query-1f35a009e0740cbd847631ecfdeb88de508c463d11b4dce75f9a0b62c88e7a52.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM variable WHERE workspace_id = $1 AND account = $2)", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "exists", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Text", - "Int4" - ] - }, - "nullable": [ - null - ] - }, - "hash": "1f35a009e0740cbd847631ecfdeb88de508c463d11b4dce75f9a0b62c88e7a52" -} diff --git a/backend/.sqlx/query-3883333dd076b66a2329afbfb510339c2b3b6f39f872e0315e3e3f74908d2e4b.json b/backend/.sqlx/query-26beff5e94b68703ad81ef9dd2d08869eb3bb7659efd9bac04cdf98ae963063d.json similarity index 60% rename from backend/.sqlx/query-3883333dd076b66a2329afbfb510339c2b3b6f39f872e0315e3e3f74908d2e4b.json rename to backend/.sqlx/query-26beff5e94b68703ad81ef9dd2d08869eb3bb7659efd9bac04cdf98ae963063d.json index 15daed7cdf..b8c8f832d3 100644 --- a/backend/.sqlx/query-3883333dd076b66a2329afbfb510339c2b3b6f39f872e0315e3e3f74908d2e4b.json +++ b/backend/.sqlx/query-26beff5e94b68703ad81ef9dd2d08869eb3bb7659efd9bac04cdf98ae963063d.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "INSERT INTO account (workspace_id, client, expires_at, refresh_token, is_workspace_integration, created_by)\n VALUES ($1, $2, now() + ($3 || ' seconds')::interval, $4, true, $5)\n RETURNING id", + "query": "INSERT INTO account (workspace_id, client, expires_at, refresh_token, is_workspace_integration)\n VALUES ($1, $2, now() + ($3 || ' seconds')::interval, $4, true)\n RETURNING id", "describe": { "columns": [ { @@ -14,7 +14,6 @@ "Varchar", "Varchar", "Text", - "Varchar", "Varchar" ] }, @@ -22,5 +21,5 @@ false ] }, - "hash": "3883333dd076b66a2329afbfb510339c2b3b6f39f872e0315e3e3f74908d2e4b" + "hash": "26beff5e94b68703ad81ef9dd2d08869eb3bb7659efd9bac04cdf98ae963063d" } diff --git a/backend/.sqlx/query-61fb1a18aaec9ca699fa17245cc190eb69010b1dd732a8ae848b705e2f565bd8.json b/backend/.sqlx/query-61fb1a18aaec9ca699fa17245cc190eb69010b1dd732a8ae848b705e2f565bd8.json deleted file mode 100644 index a7b99b9157..0000000000 --- a/backend/.sqlx/query-61fb1a18aaec9ca699fa17245cc190eb69010b1dd732a8ae848b705e2f565bd8.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT created_by FROM account WHERE workspace_id = $1 AND id = $2", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "created_by", - "type_info": "Varchar" - } - ], - "parameters": { - "Left": [ - "Text", - "Int4" - ] - }, - "nullable": [ - true - ] - }, - "hash": "61fb1a18aaec9ca699fa17245cc190eb69010b1dd732a8ae848b705e2f565bd8" -} diff --git a/backend/.sqlx/query-d5b3132a3bea036abbbf806cc1d1a70e79c624371c25208ba31e23dbd5d48ed6.json b/backend/.sqlx/query-870e1c3f0dc1aaa07ac74a2e37721ce352ad4fb67d36c19dce09d841e36f85dd.json similarity index 67% rename from backend/.sqlx/query-d5b3132a3bea036abbbf806cc1d1a70e79c624371c25208ba31e23dbd5d48ed6.json rename to backend/.sqlx/query-870e1c3f0dc1aaa07ac74a2e37721ce352ad4fb67d36c19dce09d841e36f85dd.json index 8d53e86626..2b2fe3c8e6 100644 --- a/backend/.sqlx/query-d5b3132a3bea036abbbf806cc1d1a70e79c624371c25208ba31e23dbd5d48ed6.json +++ b/backend/.sqlx/query-870e1c3f0dc1aaa07ac74a2e37721ce352ad4fb67d36c19dce09d841e36f85dd.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "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, created_by) VALUES ($1, $2, now() + ($3 || ' seconds')::interval, $4, $5, $6, $7, $8, $9, $10, $11) RETURNING id", + "query": "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", "describe": { "columns": [ { @@ -20,13 +20,12 @@ "Varchar", "Varchar", "Text", - "TextArray", - "Varchar" + "TextArray" ] }, "nullable": [ false ] }, - "hash": "d5b3132a3bea036abbbf806cc1d1a70e79c624371c25208ba31e23dbd5d48ed6" + "hash": "870e1c3f0dc1aaa07ac74a2e37721ce352ad4fb67d36c19dce09d841e36f85dd" } 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/.sqlx/query-fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e.json b/backend/.sqlx/query-fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e.json new file mode 100644 index 0000000000..de3d1d2e90 --- /dev/null +++ b/backend/.sqlx/query-fca09e2895680f5eee6e89bcecf6595e3786e4489ceff5ad2e301635c4606f2e.json @@ -0,0 +1,24 @@ +{ + "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 f943cc8bab..94b3b55eaa 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -c2cf2e450e7808b6bd5d20c4ef08bd77f0018d32 +b2f116e93b5658dcc47bcb84c130701701abcf7f \ No newline at end of file diff --git a/backend/migrations/20260925161502_account_created_by.down.sql b/backend/migrations/20260925161502_account_created_by.down.sql deleted file mode 100644 index 4485b2b43d..0000000000 --- a/backend/migrations/20260925161502_account_created_by.down.sql +++ /dev/null @@ -1 +0,0 @@ -ALTER TABLE account DROP COLUMN IF EXISTS created_by; diff --git a/backend/migrations/20260925161502_account_created_by.up.sql b/backend/migrations/20260925161502_account_created_by.up.sql deleted file mode 100644 index 14eba39c61..0000000000 --- a/backend/migrations/20260925161502_account_created_by.up.sql +++ /dev/null @@ -1,4 +0,0 @@ --- Who connected an OAuth account. A variable may link an account only if its caller created it --- or can already read a variable linking it. Existing accounts stay NULL: they can still be --- linked through a readable variable, never claimed as unlinked. -ALTER TABLE account ADD COLUMN IF NOT EXISTS created_by VARCHAR(255); diff --git a/backend/migrations/20261015160053_account_created_by_follows_user.down.sql b/backend/migrations/20261015160053_account_created_by_follows_user.down.sql deleted file mode 100644 index dde8dc2a9c..0000000000 --- a/backend/migrations/20261015160053_account_created_by_follows_user.down.sql +++ /dev/null @@ -1,4 +0,0 @@ -DROP TRIGGER IF EXISTS account_created_by_follows_instance_user_trigger ON password; -DROP FUNCTION IF EXISTS account_created_by_follows_instance_user(); -DROP TRIGGER IF EXISTS account_created_by_follows_user_trigger ON usr; -DROP FUNCTION IF EXISTS account_created_by_follows_user(); diff --git a/backend/migrations/20261015160053_account_created_by_follows_user.up.sql b/backend/migrations/20261015160053_account_created_by_follows_user.up.sql deleted file mode 100644 index ad7bd0a94e..0000000000 --- a/backend/migrations/20261015160053_account_created_by_follows_user.up.sql +++ /dev/null @@ -1,46 +0,0 @@ --- `account.created_by` grants the right to link the account to a variable, so it must name the --- same person for as long as it is set: follow the user through a rename, and clear it when the --- user leaves the workspace, so a later member given the same username does not inherit it. --- A trigger rather than application code, because users are removed from several code paths. -CREATE OR REPLACE FUNCTION account_created_by_follows_user() RETURNS trigger AS $$ -BEGIN - IF TG_OP = 'DELETE' THEN - UPDATE account SET created_by = NULL - WHERE workspace_id = OLD.workspace_id AND created_by = OLD.username; - ELSIF NEW.username IS DISTINCT FROM OLD.username THEN - UPDATE account SET created_by = NEW.username - WHERE workspace_id = OLD.workspace_id AND created_by = OLD.username; - END IF; - RETURN NULL; -END; -$$ LANGUAGE plpgsql; - -CREATE TRIGGER account_created_by_follows_user_trigger -AFTER UPDATE OF username OR DELETE ON usr -FOR EACH ROW -EXECUTE FUNCTION account_created_by_follows_user(); - --- A superadmin can connect an account in a workspace they are not a member of, which records --- their instance username with no `usr` row for the trigger above to follow. Follow --- `password.username` there instead, skipping workspaces where they are a member (handled above) --- and those where a member holds that username (the name is theirs). -CREATE OR REPLACE FUNCTION account_created_by_follows_instance_user() RETURNS trigger AS $$ -BEGIN - IF TG_OP = 'UPDATE' AND NEW.username IS NOT DISTINCT FROM OLD.username THEN - RETURN NULL; - END IF; - UPDATE account a SET created_by = CASE WHEN TG_OP = 'DELETE' THEN NULL ELSE NEW.username END - WHERE a.created_by = OLD.username - AND NOT EXISTS ( - SELECT 1 FROM usr u - WHERE u.workspace_id = a.workspace_id - AND (u.email = OLD.email OR u.username = OLD.username) - ); - RETURN NULL; -END; -$$ LANGUAGE plpgsql; - -CREATE TRIGGER account_created_by_follows_instance_user_trigger -AFTER UPDATE OF username OR DELETE ON password -FOR EACH ROW -EXECUTE FUNCTION account_created_by_follows_instance_user(); diff --git a/backend/windmill-api-integration-tests/tests/variables.rs b/backend/windmill-api-integration-tests/tests/variables.rs index 887672e37c..7332383936 100644 --- a/backend/windmill-api-integration-tests/tests/variables.rs +++ b/backend/windmill-api-integration-tests/tests/variables.rs @@ -339,41 +339,22 @@ async fn test_variables_are_cached(db: Pool) -> anyhow::Result<()> { Ok(()) } -/// Reading a variable linked to an OAuth account refreshes through that account, so a variable -/// may only link an account its caller connected or can already reach through a readable -/// variable. Any other requested link is dropped, and the variable is still created. -#[sqlx::test(migrations = "../migrations", fixtures("base", "permissions_test"))] -async fn test_oauth_account_link_requires_ownership(db: Pool) -> anyhow::Result<()> { +/// 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 base = format!( - "http://localhost:{}/api/w/test-workspace/variables", - server.addr.port() - ); + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/variables"); - let alice_account: i32 = sqlx::query_scalar( - "INSERT INTO account (workspace_id, client, expires_at, refresh_token, created_by) - VALUES ('test-workspace', 'github', now(), 'alice-refresh', 'alice') RETURNING id", + 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 missing_account = alice_account + 1000; - - let create = |token: &'static str, path: &'static str, account: i32| { - let url = format!("{base}/create"); - async move { - client() - .post(url) - .header("Authorization", format!("Bearer {token}")) - .json(&json!({ "path": path, "value": "x", "is_secret": false, - "description": "", "account": account })) - .send() - .await - .unwrap() - .status() - } - }; - let linked = |path: &'static str| { + let link = |path: &'static str| { let db = db.clone(); async move { sqlx::query_scalar::<_, Option>( @@ -386,123 +367,36 @@ async fn test_oauth_account_link_requires_ownership(db: Pool) -> anyho } }; - assert_eq!( - create("ALICE_TOKEN_TEST", "u/alice/conn", alice_account).await, - 201 - ); - assert_eq!(linked("u/alice/conn").await, Some(alice_account)); - - assert_eq!( - create("BOB_TOKEN_TEST12", "u/bob/alias", alice_account).await, - 201 - ); - assert_eq!(linked("u/bob/alias").await, None, "not bob's account"); - - let status = client() - .post(format!("{base}/update/u/bob/alias")) - .header("Authorization", "Bearer BOB_TOKEN_TEST12") - .json(&json!({ "account": alice_account })) + 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? - .status(); - assert_eq!(status, 200); - assert_eq!( - linked("u/bob/alias").await, - None, - "nor by relinking an existing variable" - ); + .await?; + assert_eq!(resp.status(), 201); + assert_eq!(link(path).await, None); - assert_eq!( - create("BOB_TOKEN_TEST12", "u/bob/preclaim", missing_account).await, - 201 - ); - assert_eq!( - linked("u/bob/preclaim").await, - None, - "an id nobody connected yet" - ); - - sqlx::query( - "UPDATE variable SET extra_perms = '{\"u/bob\": false}' - WHERE workspace_id = 'test-workspace' AND path = 'u/alice/conn'", - ) - .execute(&db) - .await?; - assert_eq!( - create("BOB_TOKEN_TEST12", "u/bob/shared", alice_account).await, - 201 - ); - assert_eq!( - linked("u/bob/shared").await, - Some(alice_account), - "bob can read a variable linking it" - ); - - let status = client() - .post(format!("{base}/update/u/bob/alias")) - .header("Authorization", "Bearer BOB_TOKEN_TEST12") - .json(&json!({ "account": alice_account })) - .send() - .await? - .status(); - assert_eq!(status, 200); - assert_eq!( - linked("u/bob/alias").await, - Some(alice_account), - "and a permitted relink through an update lands" - ); - - Ok(()) -} - -/// The creator recorded on an OAuth account follows the user through a rename and is cleared -/// when they leave, so a later member reusing the username cannot link the account. -#[sqlx::test(migrations = "../migrations", fixtures("base", "permissions_test"))] -async fn test_oauth_account_creator_follows_user(db: Pool) -> anyhow::Result<()> { - let account: i32 = sqlx::query_scalar( - "INSERT INTO account (workspace_id, client, expires_at, refresh_token, created_by) - VALUES ('test-workspace', 'github', now(), 'r', 'alice') RETURNING id", + 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?; - let creator = || async { - sqlx::query_scalar::<_, Option>("SELECT created_by FROM account WHERE id = $1") - .bind(account) - .fetch_one(&db) - .await - }; - - sqlx::query("UPDATE usr SET username = 'alice2' WHERE username = 'alice'") - .execute(&db) - .await?; - assert_eq!(creator().await?.as_deref(), Some("alice2")); - - sqlx::query("DELETE FROM usr WHERE username = 'alice2'") - .execute(&db) - .await?; - assert_eq!(creator().await?, None); - - // A superadmin who is not a member of the workspace is recorded by their instance username. sqlx::query( - "INSERT INTO password (email, password_hash, login_type, super_admin, verified, username) - VALUES ('sa@windmill.dev', 'x', 'password', true, true, 'sa')", + "UPDATE variable SET account = $1 WHERE workspace_id = 'test-workspace' AND path = $2", ) + .bind(account) + .bind(path) .execute(&db) .await?; - sqlx::query("UPDATE account SET created_by = 'sa' WHERE id = $1") - .bind(account) - .execute(&db) + let resp = authed(client().post(variable_url(port, "update", path))) + .json(&json!({ "description": "d", "account": other })) + .send() .await?; - - sqlx::query("UPDATE password SET username = 'sa2' WHERE email = 'sa@windmill.dev'") - .execute(&db) - .await?; - assert_eq!(creator().await?.as_deref(), Some("sa2")); - - sqlx::query("DELETE FROM password WHERE email = 'sa@windmill.dev'") - .execute(&db) - .await?; - assert_eq!(creator().await?, None); + 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 e9b5732f0a..dab5fe82f7 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -8811,6 +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." required: - refresh_token - expires_in @@ -31990,7 +31993,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 by `createAccount`'s `variable_path`. is_oauth: type: boolean description: Whether the variable is an OAuth variable diff --git a/backend/windmill-native-triggers/src/workspace_integrations.rs b/backend/windmill-native-triggers/src/workspace_integrations.rs index 9bb8c6aa7a..eccfa56f19 100644 --- a/backend/windmill-native-triggers/src/workspace_integrations.rs +++ b/backend/windmill-native-triggers/src/workspace_integrations.rs @@ -588,14 +588,13 @@ async fn oauth_callback( // 1. Create account record for token refresh let account_id = sqlx::query_scalar!( - "INSERT INTO account (workspace_id, client, expires_at, refresh_token, is_workspace_integration, created_by) - VALUES ($1, $2, now() + ($3 || ' seconds')::interval, $4, true, $5) + "INSERT INTO account (workspace_id, client, expires_at, refresh_token, is_workspace_integration) + VALUES ($1, $2, now() + ($3 || ' seconds')::interval, $4, true) RETURNING id", workspace_id, service_name.as_str(), expires_in.to_string(), token_response.refresh_token.as_deref().unwrap_or(""), - authed.username, ) .fetch_one(&mut *tx) .await diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 3e7196392e..332c86ec89 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -587,55 +587,6 @@ fn validate_already_encrypted_secret(path: &str, value: &str) -> Result<()> { Ok(()) } -/// The OAuth account a variable may link, of the one requested. Reading a variable linked to an -/// account refreshes through it and hands the fresh token to the reader, so only the account's -/// creator may link it, or a caller who can already read a variable that does (checked in the -/// caller's own transaction, so RLS decides). -/// -/// Anything else is dropped rather than refused: account ids are local to a workspace, so a -/// variable synced from another one carries an id that means nothing here, and must still land. -async fn permitted_account_link( - db: &DB, - user_db: &UserDB, - authed: &ApiAuthed, - w_id: &str, - account: Option, -) -> Result> { - let Some(account) = account else { - return Ok(None); - }; - let created_by = sqlx::query_scalar!( - "SELECT created_by FROM account WHERE workspace_id = $1 AND id = $2", - w_id, - account - ) - .fetch_optional(db) - .await? - .flatten(); - if created_by.as_deref() == Some(authed.username.as_str()) { - return Ok(Some(account)); - } - let mut tx = user_db.clone().begin(authed).await?; - let readable = sqlx::query_scalar!( - "SELECT EXISTS(SELECT 1 FROM variable WHERE workspace_id = $1 AND account = $2)", - w_id, - account - ) - .fetch_one(&mut *tx) - .await? - .unwrap_or(false); - tx.commit().await?; - if readable { - Ok(Some(account)) - } else { - tracing::warn!( - "Not linking OAuth account {account} in {w_id}: {} neither connected it nor can read a variable linking it", - authed.username - ); - Ok(None) - } -} - async fn create_variable( authed: ApiAuthed, Extension(db): Extension, @@ -695,8 +646,6 @@ async fn create_variable( variable.value }; - let account = permitted_account_link(&db, &user_db, &authed, &w_id, variable.account).await?; - let mut tx = user_db.begin(&authed).await?; sqlx::query!( @@ -708,7 +657,9 @@ async fn create_variable( value, variable.is_secret, variable.description, - account, + // 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::, variable.is_oauth.unwrap_or(false), variable.expires_at, variable.labels.as_deref() as Option<&[String]>, @@ -1112,7 +1063,6 @@ struct EditVariable { value: Option, is_secret: Option, description: Option, - account: Option, labels: Option>, ws_specific: Option, } @@ -1227,30 +1177,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 account = if ns.account.is_some() && ns.account != old_account_id { - permitted_account_link(&db, &user_db, &authed, &w_id, ns.account).await? - } else { - ns.account - }; - if let Some(account_id) = account { - sqlb.set_str("account", account_id); - has_sql_updates = true; - } - let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; if let Some(npath) = ns.path.clone() { @@ -1424,32 +1350,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 account.is_some() && 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..e69b3dc667 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 @@ -1005,10 +998,16 @@ ? `OAuth token for ${resourceType}` : description, is_oauth: true, - account: account, ws_specific: wsSpecific } }) + if (accountData) { + // Links the variable just created, so it refreshes through the account. + await OauthService.createAccount({ + workspace: effectiveWorkspace, + requestBody: { ...accountData, variable_path: path } + }) + } resourceValue['token'] = `$var:${path}` } } else if (linkedSecrets.length === 1) { diff --git a/frontend/src/lib/components/mcp/McpConnect.svelte b/frontend/src/lib/components/mcp/McpConnect.svelte index a8808aa052..9079f1e4dc 100644 --- a/frontend/src/lib/components/mcp/McpConnect.svelte +++ b/frontend/src/lib/components/mcp/McpConnect.svelte @@ -278,28 +278,25 @@ 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 + isOauth: true }) + 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 bafbf4b7b4..606739e9c2 100644 --- a/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte +++ b/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte @@ -136,28 +136,25 @@ if (!target) return const { workspace, path } = target try { - let accountId: number | undefined - if (data.expires_in && data.refresh_token) { - const accountIdStr = await OauthService.createAccount({ - workspace, - requestBody: { - refresh_token: data.refresh_token, - expires_in: data.expires_in, - client: 'mcp', - mcp_server_url: data.mcp_server_url - } - }) - accountId = Number(accountIdStr) - } - await upsertSecretVariable({ workspace, path, value: data.access_token, resourcePath: path, - isOauth: true, - account: accountId + isOauth: true }) + if (data.expires_in && data.refresh_token) { + await OauthService.createAccount({ + workspace, + requestBody: { + refresh_token: data.refresh_token, + expires_in: data.expires_in, + client: 'mcp', + mcp_server_url: data.mcp_server_url, + variable_path: path + } + }) + } await ResourceService.createResource({ workspace, diff --git a/frontend/src/lib/components/mcp/secretVariable.test.ts b/frontend/src/lib/components/mcp/secretVariable.test.ts index ced0f8e611..3f5b45165a 100644 --- a/frontend/src/lib/components/mcp/secretVariable.test.ts +++ b/frontend/src/lib/components/mcp/secretVariable.test.ts @@ -1,14 +1,20 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { existsVariable, getVariable, createVariable, updateVariable, deleteVariable, existsResource } = - vi.hoisted(() => ({ - existsVariable: vi.fn(), - getVariable: vi.fn(), - createVariable: vi.fn(), - updateVariable: vi.fn(), - deleteVariable: vi.fn(), - existsResource: vi.fn() - })) +const { + existsVariable, + getVariable, + createVariable, + updateVariable, + deleteVariable, + existsResource +} = vi.hoisted(() => ({ + existsVariable: vi.fn(), + getVariable: vi.fn(), + createVariable: vi.fn(), + updateVariable: vi.fn(), + deleteVariable: vi.fn(), + existsResource: vi.fn() +})) vi.mock('$lib/gen', () => ({ VariableService: { existsVariable, getVariable, createVariable, updateVariable, deleteVariable }, @@ -71,7 +77,7 @@ describe('upsertSecretVariable', () => { existsVariable.mockResolvedValue(true) getVariable.mockResolvedValue({ description: OURS }) - await upsertSecretVariable({ ...ARGS, isOauth: true, account: 7 }) + await upsertSecretVariable({ ...ARGS, isOauth: true }) expect(deleteVariable).toHaveBeenCalled() expect(createVariable).toHaveBeenCalledWith({ @@ -81,7 +87,6 @@ describe('upsertSecretVariable', () => { value: 'token', is_secret: true, is_oauth: true, - account: 7, description: OURS } }) diff --git a/frontend/src/lib/components/mcp/secretVariable.ts b/frontend/src/lib/components/mcp/secretVariable.ts index e84a5ac641..c8e63b67d4 100644 --- a/frontend/src/lib/components/mcp/secretVariable.ts +++ b/frontend/src/lib/components/mcp/secretVariable.ts @@ -33,9 +33,8 @@ export async function upsertSecretVariable(args: { /** The MCP resource this token belongs to; stamped into the description. */ resourcePath: string isOauth?: boolean - account?: number }): Promise { - const { workspace, path, value, resourcePath, isOauth, account } = args + const { workspace, path, value, resourcePath, isOauth } = args const description = mcpTokenDescription(resourcePath) // The path picker rejects an occupied path, but it validates on a debounce and @@ -66,6 +65,6 @@ export async function upsertSecretVariable(args: { await VariableService.createVariable({ workspace, - requestBody: { path, value, is_secret: true, is_oauth: isOauth, account, description } + requestBody: { path, value, is_secret: true, is_oauth: isOauth, description } }) }