diff --git a/crates/openab-core/src/discord.rs b/crates/openab-core/src/discord.rs index 6cdee8ee2..be0b0a44e 100644 --- a/crates/openab-core/src/discord.rs +++ b/crates/openab-core/src/discord.rs @@ -7,6 +7,7 @@ use crate::dispatch::DispatchTarget; use crate::format; use crate::media; use crate::remind::{self, ReminderStore}; +use crate::trust::l3_gate_applies; use async_trait::async_trait; use serenity::builder::{ CreateActionRow, CreateAttachment, CreateButton, CreateCommand, CreateCommandOption, @@ -2968,16 +2969,6 @@ fn is_denied_user( !is_bot && !allow_all_users && !allowed_users.contains(&user_id) } -/// Whether the shared L3 identity gate (`AdapterRouter::gate_incoming`) should run -/// for this sender. Bots bypass L3 — mirroring [`is_denied_user`]'s `!is_bot` -/// bypass — because bot admission is a separate concern (`allow_bot_messages` + -/// `trusted_bot_ids`), and L3 (`allowed_users`) is a human-identity allowlist. -/// Running L3 on bots would wrongly deny mode-admitted/trusted bots when -/// `allow_all_users=false` (multi-agent). See PR #1270 review. -fn l3_gate_applies(is_bot: bool) -> bool { - !is_bot -} - /// Returns `true` if a bot message should bypass the `allow_bot_messages` mode check. /// A trusted bot that @mentions this bot is treated the same as a human @mention — /// it can pull the bot into a thread regardless of the `allow_bot_messages` setting. diff --git a/crates/openab-core/src/slack.rs b/crates/openab-core/src/slack.rs index 8e37d5df2..04af2d9bc 100644 --- a/crates/openab-core/src/slack.rs +++ b/crates/openab-core/src/slack.rs @@ -3,6 +3,7 @@ use crate::adapter::{ChannelRef, ChatAdapter, MessageRef, SenderContext}; use crate::bot_turns::{BotTurnTracker, TurnAction, TurnSeverity}; use crate::config::{AllowBots, AllowUsers, SttConfig}; use crate::media; +use crate::trust::l3_gate_applies; use anyhow::{anyhow, Result}; use async_trait::async_trait; use futures_util::{SinkExt, StreamExt}; @@ -803,6 +804,7 @@ fn socket_idle(since_last_inbound: std::time::Duration, timeout: std::time::Dura #[allow(clippy::too_many_arguments)] pub async fn run_slack_adapter( adapter: Arc, + router: Arc, app_token: String, allow_all_channels: bool, allow_all_users: bool, @@ -953,6 +955,7 @@ pub async fn run_slack_adapter( } let event = event.clone(); let adapter = adapter.clone(); + let router = router.clone(); let bot_token = bot_token.clone(); let allowed_channels = allowed_channels.clone(); let allowed_users = allowed_users.clone(); @@ -969,6 +972,7 @@ pub async fn run_slack_adapter( &event, &team_id, &adapter, + &router, &bot_token, allow_all_channels, allow_all_users, @@ -1208,6 +1212,7 @@ pub async fn run_slack_adapter( .to_string(); let event = event.clone(); let adapter = adapter.clone(); + let router = router.clone(); let bot_token = bot_token.clone(); let allowed_channels = allowed_channels.clone(); let allowed_users = allowed_users.clone(); @@ -1220,6 +1225,7 @@ pub async fn run_slack_adapter( &event, &team_id, &adapter, + &router, &bot_token, allow_all_channels, allow_all_users, @@ -1365,11 +1371,21 @@ async fn get_socket_mode_url_with_timeout( .await } +/// Whether a Slack conversation ID denotes a DM. Slack conversation IDs are +/// prefix-typed: `C…` public channel, `G…` private channel/group, `D…` DM. +/// Only informational for the gate today — Slack's registry entry is L2-open +/// with `allow_dm=true`, so the decision is identical either way — but passing +/// the real surface keeps the gate's inputs truthful (cf. #1270 review F2). +fn is_dm_channel(channel_id: &str) -> bool { + channel_id.starts_with('D') +} + #[allow(clippy::too_many_arguments)] async fn handle_message( event: &serde_json::Value, team_id: &str, adapter: &Arc, + router: &Arc, bot_token: &str, allow_all_channels: bool, allow_all_users: bool, @@ -1422,6 +1438,32 @@ async fn handle_message( return; } + // Shared ingress trust gate (L3 identity). Redundant-but-matching with + // Slack's own user check that already ran above, so it cannot deny anything + // already admitted (non-regressive). L2 (channel allowlist) stays in the + // adapter for Slack — its registry entry is L2-open. + // + // Bots are skipped here: the user check above has the same `!is_bot_msg` + // bypass (bot admission is handled separately by allow_bot_messages + + // trusted_bot_ids), and the shared L3 gate is human-identity only. Running + // it on bots would wrongly drop trusted bot-to-bot messages when + // allow_all_users=false (multi-agent). Same rationale as Discord (#1270 + // review F1). Phase 1c makes this authoritative and removes the scattered + // check (#1361). + if l3_gate_applies(is_bot_msg) { + let decision = + router.gate_incoming("slack", &channel_id, is_dm_channel(&channel_id), &user_id); + if !decision.is_allowed() { + tracing::info!( + user_id, + channel = %channel_id, + ?decision, + "slack message denied by trust gate" + ); + return; + } + } + // Capture the native-streaming recipient for THIS turn, now that the sender has // passed the channel + user allow-list checks above (so denied/unauthorized // senders are never recorded). It rides on the per-turn BufferedMessage to @@ -2048,6 +2090,28 @@ fn build_set_status_body(channel_id: &str, thread_ts: &str, status: &str) -> ser mod tests { use super::*; + // --- trust gate tests --- + + /// Pins the L3 bot-bypass: the shared identity gate must not run for bot + /// senders (bot admission = allow_bot_messages + trusted_bot_ids), matching + /// the inline user check's `!is_bot_msg` bypass. Regression for #1361 + /// (mirrors the Discord test for PR #1270 review F4). + #[test] + fn l3_gate_skips_bots_applies_to_humans() { + assert!(!l3_gate_applies(true)); // bot → gate skipped + assert!(l3_gate_applies(false)); // human → gate applies + } + + /// Pins Slack conversation-ID prefix typing for the gate's `is_dm` input: + /// `D…` = DM; `C…` (public) and `G…` (private/group) are not DMs. + #[test] + fn is_dm_channel_by_prefix() { + assert!(is_dm_channel("D0123456789")); + assert!(!is_dm_channel("C0123456789")); + assert!(!is_dm_channel("G0123456789")); + assert!(!is_dm_channel("")); + } + // --- builder tests --- #[test] diff --git a/crates/openab-core/src/trust.rs b/crates/openab-core/src/trust.rs index 603b564fc..5d3ab343b 100644 --- a/crates/openab-core/src/trust.rs +++ b/crates/openab-core/src/trust.rs @@ -50,6 +50,17 @@ impl Decision { } } +/// Whether the shared L3 identity gate (`AdapterRouter::gate_incoming`) should +/// run for this sender. Bots bypass L3 — mirroring the adapters' inline user +/// checks' `!is_bot` bypass — because bot admission is a separate concern +/// (`allow_bot_messages` + `trusted_bot_ids`), and L3 (`allowed_users`) is a +/// human-identity allowlist. Running L3 on bots would wrongly deny +/// mode-admitted/trusted bots when `allow_all_users=false` (multi-agent). +/// See PR #1270 review F1; shared by the Discord and Slack gate call sites. +pub fn l3_gate_applies(is_bot: bool) -> bool { + !is_bot +} + /// Per-platform trust configuration (L2 scope + L3 identity). /// /// Construct via [`TrustConfig::new`], which applies the ADR defaults: diff --git a/docs/platforms/schema/slack.toml b/docs/platforms/schema/slack.toml index 8e34c46eb..cbcc06e5c 100644 --- a/docs/platforms/schema/slack.toml +++ b/docs/platforms/schema/slack.toml @@ -184,9 +184,9 @@ pr = "" [[openab_features]] feature = "trust_gate" -status = "partial" -note = "Native adapter does NOT use the shared AdapterRouter::gate_incoming (L3 identity trust) — that's wired only for gateway and discord. Slack enforces inline allowlists: allowed_channels and allowed_users, plus trusted_bot_ids resolution (B…→U… via bots.info) for bot senders." -source = ["crates/openab-core/src/slack.rs#allowed_channels", "crates/openab-core/src/slack.rs#trusted_bot_ids_contains"] +status = "implemented" +note = "Two layers, mirroring Discord Phase 1 (#1270): adapter-level inline allowlists (allowed_channels, allowed_users, plus trusted_bot_ids resolution B…→U… via bots.info for bot senders) + shared L3 identity gate router.gate_incoming after the inline check (redundant-but-matching; humans only — bots bypass via trust::l3_gate_applies; is_dm from the D… conversation-ID prefix). Registry entry seeded from resolved [slack] config (L2 open, L3 mirrors allow_all_users/allowed_users)." +source = ["crates/openab-core/src/slack.rs#l3_gate_applies", "crates/openab-core/src/slack.rs#is_dm_channel", "crates/openab-core/src/adapter.rs#gate_incoming", "crates/openab-core/src/trust.rs#l3_gate_applies"] pr = "" [[openab_features]] @@ -273,7 +273,7 @@ source = "crates/openab-core/src/slack.rs#bot_participated_in_thread" [[quirks]] date = "2026-07-04" title = "Native trust divergence" -note = "Unlike the gateway/Discord paths (which call AdapterRouter::gate_incoming), the Slack adapter predates the unified trust gate and enforces access with inline allowed_channels/allowed_users/trusted_bot_ids checks. Deny UX is a 🚫 reaction, not the gateway's DenyIdentity identity-echo. A future consolidation onto AdapterRouter::with_trust would unify this." +note = "The Slack adapter predates the unified trust gate and enforced access only with inline allowed_channels/allowed_users/trusted_bot_ids checks. Since the Slack Phase 1 wiring (#1361/#1363), the shared AdapterRouter::gate_incoming also runs after the inline user check (redundant-but-matching, non-regressive). Deny UX remains the adapter's 🚫 reaction, not the gateway's DenyIdentity identity-echo. Phase 1c makes the shared gate authoritative and removes the inline checks." kind = "openab_decision" source = "crates/openab-core/src/adapter.rs#with_trust" diff --git a/src/main.rs b/src/main.rs index 43a7a32f4..e5eaaf62c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -336,6 +336,30 @@ async fn main() -> anyhow::Result<()> { ); } + // Slack: gate L3 (identity) only via the shared gate, mirroring the + // Discord entry above. Slack's L2 (channel allowlist) stays in the + // adapter — its registry entry is L2-open — and L3 mirrors the resolved + // [slack].allow_all_users/allowed_users, so the gate agrees with + // Slack's existing user check (behavior-preserving). Without this + // entry, Slack was the only configured platform absent from the + // registry, falling back to the deny-all default if the gate ever ran + // for it (#1361). + if let Some(s) = &cfg.slack { + reg.insert( + "slack", + TrustConfig::new( + Some(true), // L2 open — Slack's own channel check still applies + Vec::::new(), + Some(true), + Some(config::resolve_allow_all( + s.allow_all_users, + &s.allowed_users, + )), + s.allowed_users.clone(), + ), + ); + } + // Telegram: L3 (identity) mirrors the resolved // [telegram].allow_all_users/allowed_users, so config.toml can // restrict who can message the bot without needing @@ -547,11 +571,13 @@ async fn main() -> anyhow::Result<()> { dispatchers.lock().unwrap().push(slack_dispatcher.clone()); let slack_allowed_users: std::collections::HashSet = slack_cfg.allowed_users.into_iter().collect(); + let slack_router = router.clone(); #[cfg(feature = "filestore")] let slack_filestore = filestore.clone(); Some(tokio::spawn(async move { if let Err(e) = slack::run_slack_adapter( adapter, + slack_router, slack_cfg.app_token, allow_all_channels, allow_all_users,