Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions crates/openab-core/src/discord.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
64 changes: 64 additions & 0 deletions crates/openab-core/src/slack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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<SlackAdapter>,
router: Arc<crate::adapter::AdapterRouter>,
app_token: String,
allow_all_channels: bool,
allow_all_users: bool,
Expand Down Expand Up @@ -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();
Expand All @@ -969,6 +972,7 @@ pub async fn run_slack_adapter(
&event,
&team_id,
&adapter,
&router,
&bot_token,
allow_all_channels,
allow_all_users,
Expand Down Expand Up @@ -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();
Expand All @@ -1220,6 +1225,7 @@ pub async fn run_slack_adapter(
&event,
&team_id,
&adapter,
&router,
&bot_token,
allow_all_channels,
allow_all_users,
Expand Down Expand Up @@ -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<SlackAdapter>,
router: &Arc<crate::adapter::AdapterRouter>,
bot_token: &str,
allow_all_channels: bool,
allow_all_users: bool,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down
11 changes: 11 additions & 0 deletions crates/openab-core/src/trust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
8 changes: 4 additions & 4 deletions docs/platforms/schema/slack.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand Down Expand Up @@ -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"

Expand Down
26 changes: 26 additions & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<String>::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
Expand Down Expand Up @@ -547,11 +571,13 @@ async fn main() -> anyhow::Result<()> {
dispatchers.lock().unwrap().push(slack_dispatcher.clone());
let slack_allowed_users: std::collections::HashSet<String> =
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,
Expand Down
Loading