From 7cc348874ad0ea6b6c6fc6fa29716b91b3d25b05 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:48:06 +0200 Subject: [PATCH] feat: back the variable expiration handler's built-in Slack and Teams destinations Backfills the Slack bot token grant for g/variable_expiration_handler, mirroring 20231016142659 for g/error_handler: the slack_bot folder's extra_perms is written once at Slack connect and never refreshed, and disconnecting leaves the folder behind, so workspaces that already connected Slack cannot pick the grant up from the creation-time list alone. Adds run_variable_expiration_test_job so an admin can exercise the handler on demand rather than waiting for a real expiry, which may be months away. It goes through push_variable_expiration_handler so the test run proves what matters: the handler resolves, and the group can read what it needs, under the same identity and argument shape the sweep uses. Also covers extra_args on the edit endpoint, which the settings tab now writes for the built-ins' channel. The endpoint rebuilds the setting from the request, so an omitted extra_args erases it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Do7wJTnL6MG6ngFpCGW4DG --- backend/ee-repo-ref.txt | 2 +- ...le_expiration_handler_slack_perms.down.sql | 1 + ...able_expiration_handler_slack_perms.up.sql | 5 ++ backend/tests/variable_expiration.rs | 32 +++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 54 +++++++++++++++++++ backend/windmill-api/openapi.yaml | 34 ++++++++++++ backend/windmill-queue/src/jobs.rs | 21 ++++++++ 7 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 backend/migrations/20260903161430_variable_expiration_handler_slack_perms.down.sql create mode 100644 backend/migrations/20260903161430_variable_expiration_handler_slack_perms.up.sql diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index f552cae039..d09a8a6d56 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -a4934a3213b598f77a35cdb3d774c45ad125e453 +a9102dfd61e598fa77c157f940ffa69613a2f98f diff --git a/backend/migrations/20260903161430_variable_expiration_handler_slack_perms.down.sql b/backend/migrations/20260903161430_variable_expiration_handler_slack_perms.down.sql new file mode 100644 index 0000000000..40443fc999 --- /dev/null +++ b/backend/migrations/20260903161430_variable_expiration_handler_slack_perms.down.sql @@ -0,0 +1 @@ +UPDATE folder SET extra_perms = extra_perms - 'g/variable_expiration_handler' WHERE name = 'slack_bot'; diff --git a/backend/migrations/20260903161430_variable_expiration_handler_slack_perms.up.sql b/backend/migrations/20260903161430_variable_expiration_handler_slack_perms.up.sql new file mode 100644 index 0000000000..540e2ba5f7 --- /dev/null +++ b/backend/migrations/20260903161430_variable_expiration_handler_slack_perms.up.sql @@ -0,0 +1,5 @@ +-- Grant the variable expiration handler group read access to the Slack bot token, so the +-- built-in Slack handler can post. Mirrors 20231016142659 for g/error_handler: the folder's +-- extra_perms is written once at Slack connect (ON CONFLICT DO NOTHING) and never refreshed, +-- so workspaces that connected Slack before this feature need the backfill to get the grant. +UPDATE folder SET extra_perms = JSONB_SET(extra_perms, '{g/variable_expiration_handler}', 'false', true) WHERE name = 'slack_bot'; diff --git a/backend/tests/variable_expiration.rs b/backend/tests/variable_expiration.rs index aa58f71085..b0bbdeb5b7 100644 --- a/backend/tests/variable_expiration.rs +++ b/backend/tests/variable_expiration.rs @@ -356,6 +356,38 @@ async fn test_edit_handler_endpoint(db: Pool) -> anyhow::Result<()> { Some("script/u/test-user/expiry_handler".to_string()) ); + // `extra_args` carries the built-in destinations' channel, so it has to survive a save + // that keeps the handler. The endpoint rebuilds the setting from the request rather than + // merging into the stored one, so an omitted `extra_args` erases it — the settings tab + // compensates by sending back what it loaded. + assert_eq!( + edit( + "SECRET_TOKEN", + serde_json::json!({ + "path": "script/u/test-user/expiry_handler", + "extra_args": { "channel": "windmill-alerts" } + }) + ) + .await?, + 200 + ); + assert_eq!( + stored().await?.map(|h| h["extra_args"].clone()), + Some(serde_json::json!({ "channel": "windmill-alerts" })) + ); + + assert_eq!( + edit( + "SECRET_TOKEN", + serde_json::json!({ + "path": "script/u/test-user/expiry_handler", + "extra_args": "not-an-object" + }) + ) + .await?, + 400 + ); + assert_eq!(edit("SECRET_TOKEN", serde_json::json!({})).await?, 200); assert!( stored().await?.is_none(), diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 544a0c11b9..bd51abb5c8 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -123,6 +123,10 @@ pub fn workspaced_service() -> Router { "/edit_variable_expiration_handler", post(edit_variable_expiration_handler), ) + .route( + "/run_variable_expiration_test_job", + post(run_variable_expiration_test_job), + ) .route( "/edit_large_file_storage_config", post(edit_large_file_storage_config), @@ -405,6 +409,13 @@ struct RunSlackMessageTestJobResponse { job_uuid: String, } +#[derive(Deserialize)] +struct RunVariableExpirationTestJobRequest { + /// Prefixed with `script/` or `flow/`, like the stored setting. + handler_path: String, + extra_args: Option, +} + #[allow(dead_code)] #[derive(Deserialize)] pub struct EditAutoInvite { @@ -1570,6 +1581,49 @@ async fn run_slack_message_test_job( })) } +/// Run the workspace's variable expiration handler now, against a placeholder variable. +/// +/// Admin-only, like the setting it tests. The placeholder path is deliberately not a real +/// variable: the handler is handed a path and never a value, so nothing has to exist for the +/// dispatch to be representative, and naming a real one would invite a handler to rotate it. +async fn run_variable_expiration_test_job( + authed: ApiAuthed, + Extension(db): Extension, + Path(w_id): Path, + Json(req): Json, +) -> JsonResult { + require_admin(authed.is_admin, &authed.username)?; + + if let Some(extra_args) = req.extra_args.as_ref() { + if !extra_args.is_object() { + return Err(Error::BadRequest( + "Field `extra_args` expected to be JSON object".to_string(), + )); + } + } + + let expiring = windmill_queue::ExpiringVariable { + workspace_id: w_id.clone(), + variable_path: "f/test/variable_expiration_test".to_string(), + description: "Test run from the variable expiration settings tab".to_string(), + value_expires_at: Utc::now() + chrono::Duration::hours(1), + is_secret: true, + }; + + let uuid = windmill_queue::push_variable_expiration_test_handler( + &db, + w_id.as_str(), + req.handler_path.as_str(), + &expiring, + req.extra_args.map(|v| sqlx::types::Json(to_raw_value(&v))), + ) + .await?; + + Ok(Json(RunSlackMessageTestJobResponse { + job_uuid: uuid.to_string(), + })) +} + #[derive(Deserialize)] struct SetSlackOAuthConfigRequest { slack_oauth_client_id: String, diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 044f7b7cc3..04f5f1d5ea 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -4531,6 +4531,40 @@ paths: schema: type: string + /w/{workspace}/workspaces/run_variable_expiration_test_job: + post: + summary: run the variable expiration handler against a placeholder variable + operationId: runVariableExpirationTestJob + tags: + - workspace + parameters: + - $ref: "#/components/parameters/WorkspaceId" + requestBody: + description: handler to run and the extra args to pass it + required: true + content: + application/json: + schema: + type: object + required: + - handler_path + properties: + handler_path: + type: string + description: prefixed with `script/` or `flow/`, like the stored setting + extra_args: + $ref: "#/components/schemas/ScriptArgs" + responses: + "200": + description: status + content: + text/json: + schema: + type: object + properties: + job_uuid: + type: string + /w/{workspace}/workspaces/edit_success_handler: post: summary: edit success handler diff --git a/backend/windmill-queue/src/jobs.rs b/backend/windmill-queue/src/jobs.rs index 5c433ef0cb..a360ae1461 100644 --- a/backend/windmill-queue/src/jobs.rs +++ b/backend/windmill-queue/src/jobs.rs @@ -3023,6 +3023,27 @@ pub(crate) async fn push_variable_expiration_handler<'c>( .await } +/// Dispatch the variable expiration handler once, on demand, for a variable the caller +/// names rather than one the sweep found due. +/// +/// Goes through `push_variable_expiration_handler` rather than reimplementing the push so a +/// test run proves the thing that actually matters: that the handler resolves and that +/// `g/variable_expiration_handler` can read what it needs, under the same identity and the +/// same argument shape a real expiry would use. +pub async fn push_variable_expiration_test_handler( + db: &Pool, + w_id: &str, + handler_path: &str, + expiring: &ExpiringVariable, + extra_args: Option>>, +) -> windmill_common::error::Result { + let tx = db.begin().await?; + let (uuid, tx) = + push_variable_expiration_handler(db, tx, w_id, handler_path, expiring, extra_args).await?; + tx.commit().await?; + Ok(uuid) +} + pub async fn push_success_handler<'a, 'c, T: Serialize + Send + Sync>( db: &Pool, job_id: Uuid,