diff --git a/lib/crates/fabro-slack/src/blocks.rs b/lib/crates/fabro-slack/src/blocks.rs index 7ebc92be9..db143890f 100644 --- a/lib/crates/fabro-slack/src/blocks.rs +++ b/lib/crates/fabro-slack/src/blocks.rs @@ -4,11 +4,23 @@ use serde_json::{Value, json}; use crate::payload::{SlackActionPayload, encode_action_value}; -const ANSWER_ACTION_ID: &str = "interview.answer"; +pub(crate) const ANSWER_ACTION_ID_PREFIX: &str = "interview.answer"; const MULTI_SELECT_BLOCK_ID: &str = "interview.checkboxes"; const MULTI_SELECT_ACTION_ID: &str = "interview.select"; const MULTI_SELECT_SUBMIT_ACTION_ID: &str = "interview.submit"; +/// Build a Slack-unique `action_id` for an interview button. +/// +/// Slack requires `action_id`s to be unique within a single message and caps +/// them at 255 characters. The selected option is carried in the button +/// `value` payload, so the `action_id` only needs to be unique — it doesn't +/// have to encode the selection. Suffixes are short, fixed-shape tokens +/// (`yes`, `no`, or the option index) to avoid any character-set or length +/// concerns when option keys are author-supplied. +fn answer_action_id(suffix: &str) -> String { + format!("{ANSWER_ACTION_ID_PREFIX}.{suffix}") +} + fn text_block(text: &str) -> Value { json!({ "type": "section", @@ -48,11 +60,11 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) button("Yes", &encode_action_value(&SlackActionPayload::Yes { run_id: run_id.to_string(), qid: question_id.to_string(), - }), ANSWER_ACTION_ID), + }), &answer_action_id("yes")), button("No", &encode_action_value(&SlackActionPayload::No { run_id: run_id.to_string(), qid: question_id.to_string(), - }), ANSWER_ACTION_ID), + }), &answer_action_id("no")), ] }); vec![section, actions] @@ -61,7 +73,8 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) let elements: Vec = question .options .iter() - .map(|opt| { + .enumerate() + .map(|(idx, opt)| { button( &opt.label, &encode_action_value(&SlackActionPayload::Selected { @@ -69,7 +82,7 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) qid: question_id.to_string(), key: opt.key.clone(), }), - ANSWER_ACTION_ID, + &answer_action_id(&idx.to_string()), ) }) .collect(); @@ -185,7 +198,23 @@ mod tests { let elements = actions["elements"].as_array().unwrap(); assert_eq!(elements.len(), 3); assert_eq!(elements[0]["text"]["text"], "Rust"); - assert_eq!(elements[0]["action_id"], ANSWER_ACTION_ID); + assert_eq!(elements[0]["action_id"], "interview.answer.0"); + assert_eq!(elements[1]["action_id"], "interview.answer.1"); + assert_eq!(elements[2]["action_id"], "interview.answer.2"); + // Slack requires action_id to be unique within a message. + let ids: std::collections::HashSet<&str> = elements + .iter() + .map(|e| e["action_id"].as_str().unwrap()) + .collect(); + assert_eq!(ids.len(), elements.len()); + // The option key remains in the button `value` payload so the server + // can still route the answer regardless of suffix scheme. + assert!( + elements[0]["value"] + .as_str() + .unwrap() + .contains("\"key\":\"rs\"") + ); assert!( elements[0]["value"] .as_str() @@ -217,7 +246,9 @@ mod tests { let actions = &blocks_json[1]; let elements = actions["elements"].as_array().unwrap(); - assert_eq!(elements[0]["action_id"], ANSWER_ACTION_ID); + assert_eq!(elements[0]["action_id"], "interview.answer.yes"); + assert_eq!(elements[1]["action_id"], "interview.answer.no"); + assert_ne!(elements[0]["action_id"], elements[1]["action_id"]); let value = elements[0]["value"].as_str().unwrap(); assert!(value.contains("\"run_id\":\"run-7\"")); assert!(value.contains("\"qid\":\"q-7\"")); diff --git a/lib/crates/fabro-slack/src/connection.rs b/lib/crates/fabro-slack/src/connection.rs index 9a863f2c6..afbf507a3 100644 --- a/lib/crates/fabro-slack/src/connection.rs +++ b/lib/crates/fabro-slack/src/connection.rs @@ -225,7 +225,7 @@ mod tests { "team": { "id": "T123" }, "user": { "id": "U123", "name": "ada" }, "actions": [{ - "action_id": "interview.answer", + "action_id": "interview.answer.yes", "type": "button", "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" }] diff --git a/lib/crates/fabro-slack/src/dispatch.rs b/lib/crates/fabro-slack/src/dispatch.rs index 0c0ada6c9..548de5ea1 100644 --- a/lib/crates/fabro-slack/src/dispatch.rs +++ b/lib/crates/fabro-slack/src/dispatch.rs @@ -77,7 +77,7 @@ mod tests { "team": { "id": "T123" }, "user": { "id": "U123", "name": "ada" }, "actions": [{ - "action_id": "interview.answer", + "action_id": "interview.answer.yes", "type": "button", "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" }] diff --git a/lib/crates/fabro-slack/src/interaction.rs b/lib/crates/fabro-slack/src/interaction.rs index ec8ca0006..4fc7bac1d 100644 --- a/lib/crates/fabro-slack/src/interaction.rs +++ b/lib/crates/fabro-slack/src/interaction.rs @@ -1,13 +1,23 @@ use fabro_interview::Answer; use serde_json::Value; +use crate::blocks::ANSWER_ACTION_ID_PREFIX; use crate::payload::{self, SlackActionPayload, SlackAnswerSubmission}; const MULTI_SELECT_BLOCK_ID: &str = "interview.checkboxes"; const MULTI_SELECT_ACTION_ID: &str = "interview.select"; -const ANSWER_ACTION_ID: &str = "interview.answer"; const MULTI_SELECT_SUBMIT_ACTION_ID: &str = "interview.submit"; +/// Buttons for the same question must each have a unique `action_id`, so the +/// outbound side stamps `interview.answer.` per element. This matches +/// either the exact prefix (legacy compatibility for messages posted before +/// the suffix scheme) or the suffixed form (current). +const ANSWER_ACTION_ID_PREFIX_DOT: &str = "interview.answer."; + +fn is_answer_action(action_id: &str) -> bool { + action_id == ANSWER_ACTION_ID_PREFIX || action_id.starts_with(ANSWER_ACTION_ID_PREFIX_DOT) +} + /// Parses a Slack interaction payload and returns a server-routable answer /// submission. pub fn parse_interaction(payload: &Value) -> Option { @@ -25,7 +35,7 @@ pub fn parse_interaction(payload: &Value) -> Option { let action_type = action["type"].as_str().unwrap_or("button"); let answer = match action_type { - "button" if action_id == ANSWER_ACTION_ID => match routed { + "button" if is_answer_action(action_id) => match routed { SlackActionPayload::Yes { .. } => Answer::yes(), SlackActionPayload::No { .. } => Answer::no(), SlackActionPayload::Selected { key, .. } => Answer { @@ -255,4 +265,88 @@ mod tests { }); assert!(parse_interaction(&payload).is_none()); } + + /// Suffixed `action_id`s (per-button uniqueness for Slack) must still + /// route to the correct answer. + #[test] + fn parse_suffixed_yes_action_id() { + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answer.yes", + "type": "button", + "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" + }] + }); + let submission = parse_interaction(&payload).unwrap(); + assert_eq!(submission.answer.value, AnswerValue::Yes); + } + + #[test] + fn parse_suffixed_multiple_choice_action_id() { + // `interview.answer.` is what `question_to_blocks` now produces + // for multiple_choice questions. + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answer.2", + "type": "button", + "value": "{\"kind\":\"selected\",\"run_id\":\"run-1\",\"qid\":\"q-1\",\"key\":\"py\"}" + }] + }); + let submission = parse_interaction(&payload).unwrap(); + assert_eq!( + submission.answer.value, + AnswerValue::Selected("py".to_string()) + ); + } + + /// Legacy `action_id` without a suffix must still parse so messages + /// posted by older Fabro builds remain clickable after upgrade. + #[test] + fn parse_legacy_unsuffixed_action_id() { + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answer", + "type": "button", + "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" + }] + }); + let submission = parse_interaction(&payload).unwrap(); + assert_eq!(submission.answer.value, AnswerValue::Yes); + } + + /// Action ids that merely share a prefix but are not the answer family + /// must not be misrouted (no false-positive prefix match). + #[test] + fn rejects_lookalike_action_id() { + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answers.yes", + "type": "button", + "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" + }] + }); + assert!(parse_interaction(&payload).is_none()); + } + + /// The dotted prefix constant must stay in sync with the canonical + /// prefix so outbound and inbound never drift. + #[test] + fn dotted_prefix_constant_matches_canonical_prefix() { + assert_eq!( + ANSWER_ACTION_ID_PREFIX_DOT, + format!("{ANSWER_ACTION_ID_PREFIX}.") + ); + } }