feat(trust): Phase 1 (slack) — L3 identity via shared gate - #1363
Conversation
Wire Slack into the PlatformTrustConfigs registry and call the shared ingress trust gate from the Slack message path, mirroring the Discord wiring from #1270. Slack was the only configured platform absent from the registry — the gate would have fallen back to the deny-all default had it ever run for slack events. - main.rs: insert "slack" TrustConfig — L2 open (Slack's own channel allowlist stays authoritative in the adapter), L3 mirrors the resolved [slack].allow_all_users/allowed_users, so the gate agrees with Slack's existing user check (behavior-preserving). - slack.rs: thread Arc<AdapterRouter> through run_slack_adapter into handle_message; evaluate gate_incoming after the existing user check (redundant-but-matching, non-regressive). Bots bypass L3 — same rationale as Discord (#1270 review F1): bot admission is allow_bot_messages + trusted_bot_ids, and L3 is human-identity only. - is_dm passed truthfully via Slack conversation-ID prefix (D… = DM), cf. #1270 review F2; decision is identical either way today since the entry is L2-open with allow_dm=true. - tests: pin the L3 bot-bypass and the DM prefix classification. Refs #1361 (first task), umbrella #1356, ADR #1291.
This comment has been minimized.
This comment has been minimized.
Self-review finding: the Slack wiring copy-pasted l3_gate_applies from discord.rs (identical 3-line fn + doc). Move the single definition next to Decision in trust.rs and import it from both gate call sites. Each adapter keeps its own pinning test against the shared fn.
This comment has been minimized.
This comment has been minimized.
The platform-facts KB (#1295) landed on main after this branch was cut and records 'Slack does NOT use the shared gate' — which this PR makes stale. Update the trust_gate feature entry (partial → implemented, mirroring discord.toml's phrasing) and the Native-trust-divergence quirk. Conformance suite passes against this branch's code refs.
|
Note LGTM ✅ — Slack joins the shared L3 identity gate with a clean, behavior-preserving wiring that mirrors the accepted Discord Phase 1 pattern. What This PR DoesSlack was the only configured platform absent from the How It Works
Findings
Finding Details🟢 F1: Registry agrees with inline check by constructionThe registry entry uses 🟢 F2: Shared
|
Since the last revision, main gained: Slack L3 gate (#1363), per-platform [section] trust for all 8 platforms (#1365/#1366/#1385), full config-first parity with conformance guard (#1375/#1387), and the L1 unenforceable-auth startup warning (#1373). The Phase 0.5 row now lists the actual inventory and names the one outstanding Phase 1 prerequisite (standalone WS path still on should_skip_event, tracked on #1356).
* docs(adr): revise identity-trust-none to three-layer architecture Receiver → Trust Gate → Handler replaces the previous 'gate at handle_message()' design. Addresses all findings from the PR #1263 mob review (howie + 3 LLM reviewers). Key changes: - §4.2: Trust Gate is a dedicated ingress layer upstream of Handler - §5: New architecture diagram showing three-layer separation - §7: Implementation plan starts with Receiver/Handler split - Address #1: gate at actual convergence point (not handle_message) - Address #2: trust lookup keys off per-event platform (not adapter) - Address #3: slash commands gated (Handler is downstream of gate) - Address #4: exhaustive scattered-checks inventory - Address #5: explicit empty-vs-missing semantics - Address #6: phased rollout (Phase 0-3) - Address #7: echo rate-limit + bot exclusion + DM-preferred - Address #8: gateway vs first-class section precedence - Address #9: no static HashSet (runtime construction) - Address #10: structured logging on allow + deny - Address #11-#15: minor fixes (Teams ID, bot semantics, etc.) * docs(adr): address team review findings - Add type-level guarantee (GatedEvent vs InboundEvent) — compile-time enforcement, not just convention (#4) - Clarify Gateway Receiver is one receiver that demuxes by platform (#11) - Fix layer numbering inconsistency — use names, not numbers (#21) - Add sender ID format table with per-platform gotchas (#22, #23, #24) - Clarify is_bot bypass is caller-side, not inside decide() (擺渡-1) - Change echo group fallback to silent drop (avoid UID leakage) (#6) * docs(adr): add event loop binding design + fix is_bot L2 bypass - Add §5 'Event loop binding' section: run_platform generic pipeline, EventReceiver/EventHandler traits, main.rs startup wiring - Gateway platforms: one shared WS, demux by event.platform, fan-out to per-platform Handlers - Fix is_bot bypass: bots skip L3 but STILL enforce L2 scope (擺渡-1 🔴) - Add cross-crate boundary note for Gateway Receiver (擺渡-2 🟡) - Include binding topology summary diagram * docs(adr): address round 3 findings — tighten pseudocode precision - GatedEvent: private field in narrow module (not pub(crate)), with read-only accessors and module layout diagram (諸葛村夫-1) - gate_event: use configs.get().surface_allowed() to match real API (擺渡-3) - Phase table: add Phase 0.5 for current partially-wired state on main, clarify Phase 2 means 'refuse to start' (諸葛村夫-2) * docs(adr): fix seal() visibility — private fn, not pub(super) seal() lives in the same module as gate_event(), so it should be a plain private fn. pub(super) would unnecessarily expose it to the parent module. * docs(adr): fix stale module layout comment — constructor is private, not pub(super) * docs(adr): address LINE/Slack/Feishu review feedback - Replace line-number refs with symbol+semantic descriptions (drift-proof) - Rewrite echo section: platform-specific echo trait (LINE=Reply only, Slack=chat.postEphemeral, Discord=DM); leak-safe content by scope - Add is_bot per-platform derivation table (pinned canonical rules) - Document trusted_bot_ids as shared config (resolves Feishu circular dep) - Clarify slash commands scope (Slack doesn't consume them) - Update Slack sender ID: Enterprise Grid composite key (team_id, sender_id) - Add non-message events section (assistant_thread_started must gate) - Add Slack scope notes (Socket Mode only, MPIM=channel) - Add LINE group policy: open/members dual-mode in decide() - Add LINE @mention pre-filter as documented Receiver exception - Feishu: gateway=L1 only, eliminate double-gating, empty list=deny-all Addresses feedback from: - @luffy-aiagent (LINE platform review) - @antigenius0910 (Slack platform review) - @wangyuyan-agent (Feishu platform review) * docs(adr): address 9 review findings — API contract gaps + hardening Fixes identified during group review: F1+F6: Add workspace_id to InboundEvent; define Slack Enterprise Grid canonical sender_id format and config examples for Grid deployments F2: Replace HashMap<String, TrustConfig> with enum PlatformTrustConfig (Base/Line/Slack) — LINE group policy and Slack workspace-scoped trust now have proper type representations F3: Add cron bypass in gate_event() — system-initiated events skip L2/L3 (platform='cron' or sender_id='openab-cron') F4: Add #[cfg(test)] assume_trusted_for_test() constructor for GatedEvent — enables Handler unit testing without full pipeline F5+F9: Change into_inner() to pub(crate); adjust safety claim wording from 'bypass impossible' to 'accidental bypass compile error' F7: Change gate_event() signature to take InboundEvent by value — zero-copy hot path (no .clone() on RawPlatformEvent) F8: Specify bounded LRU cache (max_capacity + TTL) for rate-limit state — prevents OOM from random sender_id flooding * fix(adr): address review findings F1-F3 on identity-trust-none F1 (critical): Remove sender_id spoofing hole in cron bypass — only check platform == "cron" since WeCom allows freeform UserIDs that could match any synthetic value. Update rationale accordingly. F2: Add WeCom, Google Chat, MS Teams to pinned is_bot derivation and echo-delivery tables (all 8 platforms now covered). F3: Fix pseudocode precision — unwrap_or no longer borrows a temporary; add PlatformTrustConfigs::get() and PlatformTrustConfig::surface_allowed() delegating method used by gate_event. * fix(adr): address group review round 2 — 9 findings from B1/B4/B5/B8/B11/B12/B15 Fixes: - WeCom is_bot: remove enter_agent (user-initiated, not bot); keep only trusted_bot_ids (B5 F1, B12 F1) - SlackTrustConfig::decide(): workspace_users is now strict override (ignore allow_all_users); base fallback supports team_id:user_id composite key for Enterprise Grid (B4 F1, B5 F2, B12 F3) - LINE group policy: fix prose vs code contradiction — unconfigured groups use default_group_policy, not DenyScope (B12 F2) - Reserved platform validation: MUST-level requirement for all external Receivers to reject reserved platform names; cron bypass invariant documented; Phase 1 SHOULD for typed InboundSource enum (B1) - decide() simplified to use self.get() — remove duplicate default logic (B5 F3) - Echo rate-limit key updated to (platform, workspace_id, sender_id) (B12 F4, B15 F2) - Module layout: InboundEvent in mod.rs (public), gate.rs narrow (B12 F5) - into_inner() trust boundary doc: module-level vs crate-level explained, Phase 1 SHOULD for lint/annotation (B11, B15 F1) - Slack Enterprise Grid gotcha: split dense table cell into footnote (B8 F2) * fix(adr): add platform lowercase invariant to InboundEvent (B14 F2) InboundEvent.platform MUST be lowercase — Receivers normalize before constructing. This ensures consistency between gate_event's == "cron" check and PlatformTrustConfigs::get()'s to_lowercase() lookup. * docs(adr): clarify gw_event.platform source in run_gateway_platforms Add comment noting platform field is assigned by gateway routing config, not from webhook payload body — satisfies reserved platform invariant. * docs(adr): WeCom corrections from external review (canyugs#18) Three verified adjustments to the v2 trust architecture text: - WeCom callback mode is DM-only: channel_type always "direct" with a per-user channel id, so "group routing" does not exist for WeCom and its L2 scope is effectively allow_dm only (verified wecom.rs:1059-71) - Sender-id table: plain UserIDs are self-built-app only; external contact / ISV callbacks carry wm/wo-prefixed external_userid or encrypted OpenUserID (verified against official WeCom API docs) - Bot-bypass wording: is_bot is hardcoded false in the WeCom Receiver today, so the L3 bot-bypass is a no-op for WeCom — the bypass is available uniformly but effective only where the Receiver can derive is_bot Credit: canyugs. * docs(adr): refresh Phase 0.5 snapshot to shipped state Since the last revision, main gained: Slack L3 gate (#1363), per-platform [section] trust for all 8 platforms (#1365/#1366/#1385), full config-first parity with conformance guard (#1375/#1387), and the L1 unenforceable-auth startup warning (#1373). The Phase 0.5 row now lists the actual inventory and names the one outstanding Phase 1 prerequisite (standalone WS path still on should_skip_event, tracked on #1356). --------- Co-authored-by: chaodu-agent <chaodu-agent@openab.dev> Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com>
What problem does this solve?
Slack is the only configured platform absent from the
PlatformTrustConfigsregistry:src/main.rsinserts the six gateway platforms, Discord (#1270), and Telegram — but never Slack. Slack trust relies entirely on the adapter's inline checks, and the registry's fallback for unregistered platforms is the deny-all default, so the shared ingress gate could never be made authoritative for Slack (Phase 1c) without this wiring.This implements the first task of #1361 (umbrella #1356), mirroring the Discord Phase 1 pattern from #1270.
At a Glance
Phase 1c (future) flips the gate from redundant-but-matching to authoritative and removes the scattered inline checks — this PR makes that possible for Slack.
Proposed Solution
src/main.rs— insert a"slack"entry into the registry: L2 open (Slack's own channel allowlist stays authoritative in the adapter), L3 mirrors the resolved[slack].allow_all_users/allowed_usersviaconfig::resolve_allow_all, so the gate agrees exactly with Slack's existing user check.crates/openab-core/src/slack.rs— threadArc<AdapterRouter>throughrun_slack_adapterintohandle_message, and evaluategate_incoming("slack", …)after the existing user check. The gate is redundant-but-matching: it cannot deny anything the inline check already admitted (non-regressive), and the existing 🚫-reaction deny UX is unchanged.l3_gate_applies(is_bot_msg)skips the gate for bot senders, mirroring the inline check's!is_bot_msgbypass and Discord's rationale (feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge] #1270 review F1): bot admission isallow_bot_messages+trusted_bot_ids; L3 is a human-identity allowlist. Running it on bots would wrongly drop trusted bot-to-bot messages whenallow_all_users = false(multi-agent).is_dm— passed via Slack conversation-ID prefix (D…= DM,C…/G…= channel/group), cf. feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge] #1270 review F2. Informational today: the registry entry is L2-open withallow_dm = true, so the decision is identical either way.crates/openab-core/src/trust.rs(added in7908cb7after self-review) —l3_gate_appliesnow has a single definition next toDecision, imported by both the Discord and Slack gate call sites, instead of being copy-pasted per adapter.Why this approach?
It is the exact shape of the accepted Discord Phase 1 change (#1270: registry insert + redundant-but-matching gate call + bot bypass + tests), keeping the two core platforms symmetrical ahead of Phase 1c (gate becomes authoritative, scattered checks removed). Behavior-preserving by construction — no config field changes, no default changes.
Self-review
Reviewed with the standard SOP after opening:
dispatcher.submitsite in slack.rs is insidehandle_message, and both event paths (app_mention,message) route through it — no Slack dispatch path bypasses the new gate.identity_allowed's empty-sender fail-closed cannot fire (handle_messagereturns early when the event has nouser/bot_id).l3_gate_appliesduplication: the original commit copy-pasted the helper from discord.rs. Consolidated intotrust.rsin7908cb7; both adapters now import the shared definition, each keeping its own pinning test.Platform-facts KB sync
maingained the schema-driven platform facts KB (#1295) after this branch was cut;docs/platforms/schema/slack.tomlrecorded "Slack does NOT use the shared gate," which this PR makes stale. Commitf6606e1updates thetrust_gateentry (partial → implemented, phrasing mirrors discord.toml) and the Native-trust-divergence quirk. TheconformanceCI check now runs on this PR and passes.Validation
At
7908cb7(macOS arm64):cargo clippy --workspace --all-features -- -D warnings— cleancargo clippy --workspace -- -D warnings(default features) — cleancargo check -p openab-core --no-default-features— passcargo check --no-default-features --features discord(slack feature off) — pass (1 pre-existingsession_ttl_durwarning, present on unmodifiedmain)cargo test -p openab-core --all-features— 656 passed, 1 failed:secrets::tests::resolve_exec_nonzero_exit, the known pre-existing macOS-only failure (also fails on unmodifiedmain)l3_gate*) — 2/2 across both adaptersrustfmt --check— no new drift introduced (per-file diff counts identical tomainbaseline: trust.rs 8→8, slack.rs 25→25, discord.rs 55→55, main.rs 4→4)Tests
slack::tests::l3_gate_skips_bots_applies_to_humans— pins the L3 bot-bypass (mirrors Discord's feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge] #1270 F4 test)slack::tests::is_dm_channel_by_prefix— pins theD…/C…/G…conversation-ID classificationdiscord.rskeeps its existingl3_gate_skips_bots_admits_humanspin, now exercising the sharedtrust::l3_gate_appliesRefs #1361 (first task — Enterprise Grid
workspace_usersremains), umbrella #1356, ADR #1291.