feat(messaging): add Microsoft Teams channel adapter#607
Conversation
Add a Microsoft Teams channel adapter built on the Azure Bot Service / Bot Framework Connector protocol. Inbound: an axum webhook server (POST /api/messages) validates the Azure-signed JWT (RS256, JWKS from login.botframework.com, iss/aud/exp), parses Activities into InboundMessage, and enforces per-conversation permissions (DM allowlist with a "*" wildcard; channels open on @mention). Outbound: replies and proactive broadcasts go to the Bot Connector with an Azure AD client-credentials token, guarded by a serviceUrl SSRF allowlist (*.botframework.com / *.trafficmanager.net), with a per-conversation serviceUrl sidecar so proactive sends survive restarts. Capabilities: text and @mentions; Adaptive Cards and card buttons (interactive_elements render as Action.Submit; a click returns as MessageContent::Interaction); a typing indicator; inbound file and image attachments (MessageContent::Media, with the download bearer gated to allowlisted hosts). Config: a [messaging.teams] section (shaped like the other adapters), hot-reloadable permissions via the config watcher, and the client secret sourced from the environment. Setup guide in docs/content/docs/(messaging). Adds one dependency, jsonwebtoken (ring-based). One Teams bot per instance.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds end-to-end Microsoft Teams messaging support: TOML schema types, credential loading with secret redaction, ChangesMicrosoft Teams Adapter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| app_id: &str, | ||
| jwks: &JwksCache, | ||
| ) -> anyhow::Result<()> { | ||
| // Strip "Bearer " prefix. |
There was a problem hiding this comment.
Auth scheme is case-insensitive; this avoids rejecting valid authorization: bearer ... headers (and tolerates extra whitespace) without changing the JWT validation logic.
| // Strip "Bearer " prefix. | |
| // Strip "Bearer " prefix (scheme is case-insensitive). | |
| let (scheme, token) = auth_header | |
| .split_once(' ') | |
| .with_context(|| "Authorization header is not a Bearer token")?; | |
| if !scheme.eq_ignore_ascii_case("bearer") { | |
| anyhow::bail!("Authorization header is not a Bearer token"); | |
| } | |
| let token = token.trim(); |
| /// | ||
| /// This guards against attacker-supplied serviceUrl values that could be used | ||
| /// to exfiltrate the Bot Connector bearer token. | ||
| fn is_allowed_service_url(url: &str) -> bool { |
There was a problem hiding this comment.
Allowlisting all of .trafficmanager.net is pretty broad (anyone can create an Azure Traffic Manager profile under their own <name>.trafficmanager.net), so a forged/compromised inbound serviceUrl could potentially turn into token exfil. If Teams is always smba.trafficmanager.net here, tightening the host check reduces the blast radius.
| fn is_allowed_service_url(url: &str) -> bool { | |
| host.ends_with(".botframework.com") || host == "smba.trafficmanager.net" |
| sidecar_path: self.sidecar_path.clone(), | ||
| }; | ||
|
|
||
| let app = Router::new() |
There was a problem hiding this comment.
Since handle_messages reads raw Bytes, it might be worth setting an explicit request body limit (similar to src/api/server.rs) to avoid a trivial memory DoS on the public webhook.
| let app = Router::new() | |
| let app = Router::new() | |
| .route("/api/messages", post(handle_messages)) | |
| .route("/health", get(handle_health)) | |
| .layer(axum::extract::DefaultBodyLimit::max(10 * 1024 * 1024)) // 10 MiB | |
| .with_state(state); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/content/docs/`(messaging)/teams-setup.mdx:
- Around line 67-68: The Teams DM allowlist docs are using the wrong identifier
format; update the guidance around dm_allowed_users in teams-setup.mdx to tell
users to use AAD object IDs or UPN strings, not 29:... MRI values. Make sure the
examples and surrounding explanation in the Teams setup section clearly match
the actual matching logic used by the bot so users configure the allowlist with
the correct identifier.
In `@src/config/watcher.rs`:
- Around line 698-735: The hot-reload Teams config path in
watcher::apply_config_change (the block that calls build_teams_adapter and
manager.register_and_start for "teams") silently skips unsupported named
instances, unlike cold start. Add the same warning used during startup when
config.messaging.teams.instances contains named Teams instances and hot reload
cannot start them, so the user is told they were ignored; keep the warning close
to the existing Teams enabled/adapter-start logic so it triggers on live config
edits too.
- Around line 264-270: The Teams permissions reload path in watcher.rs currently
only updates when messaging.teams still exists, so disabling Teams or removing
its bindings can leave the existing adapter running with open access via
TeamsPermissions::from_config fallback. Update the reload logic around
teams_permissions and TeamsPermissions::from_config to fail closed: when Teams
is disabled or no longer configured, stop/disable the existing Teams listener or
replace its permissions with a restrictive “deny all” state instead of reusing
the permissive fallback, and keep the reload behavior strict when config changes
remove Teams access.
In `@src/messaging/target.rs`:
- Around line 166-216: The Teams target resolution in target::BroadcastTarget
parsing is incorrectly inferring named instances from opaque conversation IDs by
using the first segment after "teams:" and is_valid_instance_name. Update this
logic to derive the adapter only from an unambiguous source such as persisted
platform metadata or a configured instance lookup, and keep the conversation id
parsing separate from adapter identity. Ensure the BroadcastTarget branch for
"teams" no longer promotes arbitrary IDs like teams:a or teams:prod into adapter
keys unless the instance is explicitly known.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a391978f-fb9f-40a3-b180-e3ec1c2c0f57
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomldocs/content/docs/(messaging)/meta.jsonis excluded by!**/*.json
📒 Files selected for processing (15)
docs/content/docs/(messaging)/messaging.mdxdocs/content/docs/(messaging)/teams-setup.mdxsrc/agent/channel.rssrc/config.rssrc/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/conversation/channels.rssrc/main.rssrc/messaging.rssrc/messaging/target.rssrc/messaging/teams.rssrc/secrets/store.rs
| # Teams user IDs allowed to DM the bot. "*" allows everyone. | ||
| dm_allowed_users = ["*"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the actual Teams DM allowlist identifier.
The implementation allows DMs by matching AAD object IDs (or UPN strings), but this guide tells users to paste 29:... MRI values into dm_allowed_users. That will make the setup look correct while DMs still fail closed.
Suggested fix
-# Teams user IDs allowed to DM the bot. "*" allows everyone.
+# Entra/AAD user IDs (or UPN strings) allowed to DM the bot. "*" allows everyone.
dm_allowed_users = ["*"]
@@
-- To allow specific people, list their Teams user IDs (the `29:...` MRI strings).
+- To allow specific people, list the Entra/AAD object IDs (or the exact UPN string) that Spacebot logs as `sender_id`.
- To allow everyone (org-wide deployments), set `dm_allowed_users = ["*"]`.
@@
-You usually do not know a user's `29:...` MRI up front. The simplest path is to **`@mention` the bot in a channel** (channel messages are not gated), or set the `"*"` wildcard. To allowlist one person, send a DM once (it is dropped), read the dropped `sender_id` from the logs, and add that value.
+You usually do not know a user's allowlist value up front. The simplest path is to **`@mention` the bot in a channel** (channel messages are not gated), or set the `"*"` wildcard. To allowlist one person, send a DM once (it is dropped), read the logged `sender_id`, and add that exact value.Also applies to: 83-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/content/docs/`(messaging)/teams-setup.mdx around lines 67 - 68, The
Teams DM allowlist docs are using the wrong identifier format; update the
guidance around dm_allowed_users in teams-setup.mdx to tell users to use AAD
object IDs or UPN strings, not 29:... MRI values. Make sure the examples and
surrounding explanation in the Teams setup section clearly match the actual
matching logic used by the bot so users configure the allowlist with the correct
identifier.
| if let Some(ref perms) = teams_permissions | ||
| && let Some(teams_config) = &config.messaging.teams | ||
| { | ||
| let new_perms = TeamsPermissions::from_config(teams_config, &config.bindings); | ||
| perms.store(Arc::new(new_perms)); | ||
| tracing::info!("teams permissions reloaded"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when Teams is removed or disabled.
TeamsPermissions::from_config(...) falls back to channel_filter = None, and is_allowed() treats that as open channel/group access. Because this watcher never stops an already-running Teams adapter, flipping [messaging.teams].enabled = false or removing Teams bindings can leave the listener up with broader access than before.
Suggested fix
- if let Some(ref perms) = teams_permissions
- && let Some(teams_config) = &config.messaging.teams
- {
- let new_perms = TeamsPermissions::from_config(teams_config, &config.bindings);
- perms.store(Arc::new(new_perms));
- tracing::info!("teams permissions reloaded");
- }
+ if let Some(ref permissions) = teams_permissions {
+ let new_permissions = match &config.messaging.teams {
+ Some(teams_config) if teams_config.enabled => {
+ TeamsPermissions::from_config(teams_config, &config.bindings)
+ }
+ _ => TeamsPermissions {
+ channel_filter: Some(Vec::new()),
+ dm_allowed_users: Vec::new(),
+ },
+ };
+ permissions.store(Arc::new(new_permissions));
+ tracing::info!("teams permissions reloaded");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(ref perms) = teams_permissions | |
| && let Some(teams_config) = &config.messaging.teams | |
| { | |
| let new_perms = TeamsPermissions::from_config(teams_config, &config.bindings); | |
| perms.store(Arc::new(new_perms)); | |
| tracing::info!("teams permissions reloaded"); | |
| } | |
| if let Some(ref permissions) = teams_permissions { | |
| let new_permissions = match &config.messaging.teams { | |
| Some(teams_config) if teams_config.enabled => { | |
| TeamsPermissions::from_config(teams_config, &config.bindings) | |
| } | |
| _ => TeamsPermissions { | |
| channel_filter: Some(Vec::new()), | |
| dm_allowed_users: Vec::new(), | |
| }, | |
| }; | |
| permissions.store(Arc::new(new_permissions)); | |
| tracing::info!("teams permissions reloaded"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/watcher.rs` around lines 264 - 270, The Teams permissions reload
path in watcher.rs currently only updates when messaging.teams still exists, so
disabling Teams or removing its bindings can leave the existing adapter running
with open access via TeamsPermissions::from_config fallback. Update the reload
logic around teams_permissions and TeamsPermissions::from_config to fail closed:
when Teams is disabled or no longer configured, stop/disable the existing Teams
listener or replace its permissions with a restrictive “deny all” state instead
of reusing the permissive fallback, and keep the reload behavior strict when
config changes remove Teams access.
…ing, doc - target.rs: Teams runs a single listener in v1, so broadcast/delivery target resolution always uses the adapter "teams" and keeps the full conversation id (colons preserved). Drop the instance-name heuristic that misread opaque ids like a DM "a:1-..." as a named instance "teams:a" and mis-routed proactive sends to a non-existent adapter. Update tests; add DM-id regressions. - watcher.rs: warn on named [[messaging.teams.instances]] during hot reload, matching cold start (named instances are not started in v1). - permissions.rs: correct the dm_allowed_users doc to match the actual compare target (the inbound activity.from.id MRI), not AAD object IDs. Note: the hot-reload "fail-open on disable" raised in review is the shared behavior of every adapter (removing config never stops a running adapter), not Teams-specific; tightening only Teams would break cross-adapter parity, so it is intentionally out of scope here.
Collapse a nested if in card_to_adaptive and annotate build_teams_adapter with #[allow(clippy::too_many_arguments)] (discrete args mirror TeamsAdapter::new). Keeps the clippy-all gate clean.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/messaging/target.rs (1)
175-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse full
conversation_idnames here.
conv_id/bare_conv_idbreak the repo’s no-abbreviation rule and make this branch less consistent with the surrounding Teams terminology. Rename them toconversation_id/bare_conversation_id. As per coding guidelines, "Don't abbreviate variable names. Usequeuenotq,messagenotmsg,channelnotch. Common abbreviations likeconfigare fine."Also applies to: 673-677
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/messaging/target.rs` around lines 175 - 191, The Teams target branch uses abbreviated local names that violate the repo’s no-abbreviation naming rule. In the logic in target resolution around the `BroadcastTarget` construction, rename `conv_id` to `conversation_id` and `bare_conv_id` to `bare_conversation_id`, and update the corresponding uses so the naming stays consistent with the surrounding Teams terminology and the existing `channel`/`platform_meta` flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/messaging/target.rs`:
- Around line 175-191: The Teams target branch uses abbreviated local names that
violate the repo’s no-abbreviation naming rule. In the logic in target
resolution around the `BroadcastTarget` construction, rename `conv_id` to
`conversation_id` and `bare_conv_id` to `bare_conversation_id`, and update the
corresponding uses so the naming stays consistent with the surrounding Teams
terminology and the existing `channel`/`platform_meta` flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da9968d6-a5e7-4636-a50a-39258f8c2449
📒 Files selected for processing (4)
src/config/permissions.rssrc/config/watcher.rssrc/messaging/target.rssrc/messaging/teams.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config/watcher.rs
- src/config/permissions.rs
Mirror the fixes landed on the upstream PR branches (spacedriveapp#607/spacedriveapp#608): - target.rs: single-instance broadcast/delivery routing — the adapter is always "teams" and the full conversation id is preserved; no instance is inferred from opaque ids (a DM "a:1-..." was misread as "teams:a"). Tests updated + DM-id regressions added. - watcher.rs: warn on named [[messaging.teams.instances]] during hot reload. - permissions.rs: dm_allowed_users doc now matches the actual compare target (the inbound activity.from.id MRI). - teams.rs + api/messaging.rs: clippy cleanups (collapsible-if, too_many_arguments).
|
Thanks for the review @coderabbitai. Addressed in 1. DM allowlist identifier ( 2. Hot-reload named-instance warning ( 3. Fail-open on Teams disable ( 4. Adapter inference from opaque conversation IDs (
|
|
Tip For best results, initiate chat on the files or code changes.
Good catches on the actual fixes, especially the opaque Teams conversation ID routing bug and the hot-reload warning parity. And agreed on Point 3: changing only Teams there would be inconsistent with the current watcher behavior across adapters, so that teardown/fail-closed behavior is better handled as a broader follow-up rather than in this PR. Also appreciate the clarification on DM allowlisting: matching against 🐇👍 ✏️ Learnings added
|
…acklog Roadmap: add an 'Upstream delivery' block (spacedriveapp#605/spacedriveapp#607/spacedriveapp#608 + state) and a single 'Deferred / out-of-scope — tracked backlog' table (outbound/inbound reactions, select menus, streaming, multi-bot, v3 approval cards, the cross-adapter hot-reload teardown follow-up from the spacedriveapp#607 review, richer identity, facilitation). Handoff: refresh TL;DR/NEXT/branch-map for the upstreaming state (HEADs, the three PR branches).
… in broadcast resolve_broadcast_target yields the bare MS conversation id, but the service_urls map (and inbound capture) key on the runtime-prefixed id (teams:<id>). broadcast passed the bare id straight to send_activity, so the lookup missed -> 'no serviceUrl for routing key'. Reconstruct the prefixed key via broadcast_routing_key (idempotent); send_activity strips it again for the Bot Connector URL. + regression test. Compiles clean (release build verified).
1 participant
What
Adds a Microsoft Teams channel adapter, built on the Azure Bot Service / Bot Framework Connector protocol. An agent can now talk to people in Teams DMs, channels, and group chats.
Capabilities
@mentionhandling (channels and group chats are open on mention; DMs use an allowlist).interactive_elementsrender asAction.Submit; a click returns to the agent asMessageContent::Interaction.MessageContent::Media."*"wildcard for org-wide use.Architecture
axumwebhook server (POST /api/messages) validates the Azure-signed JWT (RS256, JWKS fromlogin.botframework.com,iss/aud/exprequired), parses Activities intoInboundMessage, then applies the permission filter before dispatch.serviceUrlSSRF allowlist (*.botframework.com/*.trafficmanager.net) gates every send, and a per-conversationserviceUrlsidecar lets proactive sends survive restarts. The inbound-media download bearer is gated to the same allowlist.[messaging.teams]section shaped like the other adapters, hot-reloadable permissions via the config watcher, and the client secret sourced from the environment (env:TEAMS_CLIENT_SECRET).Adapter parity
send_status; retry and backoff are inherited from the messaging manager; outbound errors are classified permanent vs retryable.Verification
just gate-prpasses (969 lib tests,cargo check --all-targets, clippy, fmt, typegen).Notes
jsonwebtoken(ring-based, no OpenSSL).docs/content/docs/(messaging)/teams-setup.mdx.