mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-17 00:02:59 +00:00
refactor: link a variable to its OAuth account only in the connect flow
The variable endpoints no longer take an account link from the caller. The connect flows create the token variable first and pass its path to create_account, which links it server-side. This replaces the creator check and the created_by bookkeeping, along with both migrations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
ae87cff76f
commit
ad8bc5d4df
-23
@@ -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"
|
||||
}
|
||||
+2
-3
@@ -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"
|
||||
}
|
||||
-23
@@ -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"
|
||||
}
|
||||
+3
-4
@@ -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"
|
||||
}
|
||||
-23
@@ -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"
|
||||
}
|
||||
+24
@@ -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"
|
||||
}
|
||||
@@ -1 +1 @@
|
||||
c2cf2e450e7808b6bd5d20c4ef08bd77f0018d32
|
||||
b2f116e93b5658dcc47bcb84c130701701abcf7f
|
||||
@@ -1 +0,0 @@
|
||||
ALTER TABLE account DROP COLUMN IF EXISTS created_by;
|
||||
@@ -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);
|
||||
@@ -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();
|
||||
@@ -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();
|
||||
@@ -339,41 +339,22 @@ async fn test_variables_are_cached(db: Pool<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<i32>>(
|
||||
@@ -386,123 +367,36 @@ async fn test_oauth_account_link_requires_ownership(db: Pool<Postgres>) -> 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<Postgres>) -> 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<String>>("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(())
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<i32>,
|
||||
) -> Result<Option<i32>> {
|
||||
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<DB>,
|
||||
@@ -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::<i32>,
|
||||
variable.is_oauth.unwrap_or(false),
|
||||
variable.expires_at,
|
||||
variable.labels.as_deref() as Option<&[String]>,
|
||||
@@ -1112,7 +1063,6 @@ struct EditVariable {
|
||||
value: Option<String>,
|
||||
is_secret: Option<bool>,
|
||||
description: Option<String>,
|
||||
account: Option<i32>,
|
||||
labels: Option<Vec<String>>,
|
||||
ws_specific: Option<bool>,
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
}
|
||||
})
|
||||
|
||||
@@ -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<void> {
|
||||
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 }
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user