feat(gateway): add MS Teams adapter + self-hosted docs#667
feat(gateway): add MS Teams adapter + self-hosted docs#667thepagent merged 4 commits intoopenabdev:mainfrom
Conversation
- Add Teams adapter (JWT validation, OAuth2, Bot Framework API) - Add Teams routing to gateway main.rs - Add jsonwebtoken dependency - Include 16 unit tests (tenant check, JWT errors, activity deserialization) - Add docs/msteams-selfhosted.md (Docker Compose + Cloudflare Tunnel) - service_url cache with TTL cleanup (4 hours) Closes openabdev#548
masami-agent
left a comment
There was a problem hiding this comment.
PR Review: #667
Summary
- Problem: Add MS Teams as a gateway adapter plugin for self-hosted deployments
- Approach: Teams adapter module + self-hosted deployment docs
- Risk level: Medium — JWT/OAuth2 code already reviewed in original #550
Core Assessment
- Problem clearly stated: ✅ — linked to #548, follows Custom Gateway ADR
- Approach appropriate: ✅ — clean adapter plugin, matches modular architecture from #550
- Alternatives considered: ✅ — research comment in #550 covers all Teams integration methods
- Best approach for now: ✅
Code Review
This code was extracted from the original #550 and has already been through:
- masami-agent review (REQUEST_CHANGES → fixes applied)
- obrutjack maintainer review (REQUEST_CHANGES → fixes applied → APPROVE)
- 四法師 joint review (APPROVE with Suggested Changes)
- neilkuan E2E POC verification (screenshot in #550)
All previous blockers were resolved:
- ✅ JWT issuer validation (validates
https://api.botframework.com) - ✅ JWKS cache miss auto-refresh (handles key rotation)
- ✅ Dead code removed (
reply_handlers,GatewayResponseecho block) - ✅ 16 unit tests passing
Integration Verification
Manually verified:
teams.rsimports (crate::schema::*,crate::AppState) match current main branchTeamsConfig,TeamsAdapter,webhook,handle_replyare allpub— matches main.rs usage- AppState fields (
teams,teams_service_urls) correctly added - Reply routing
"teams"match arm correctly dispatches to adapter - service_url cleanup task (4-hour TTL, 5-min sweep) added
Deferred Items (tracked for follow-up)
serviceUrlJWT claim validationRwLockforteams_service_urls(performance optimization)
Verdict
APPROVE — code already thoroughly reviewed in #550. Integration with merged modular architecture is clean.
Note: Cannot submit binding approval on own PR. Requesting @obrutjack approval and @thepagent code owner review.
masami-agent
left a comment
There was a problem hiding this comment.
PR Review: #667
Summary
- Problem: Add MS Teams as a gateway adapter plugin for enterprise users
- Approach: New
adapters/teams.rsmodule + self-hosted docs, building on #550 modular architecture - Risk level: Medium — JWT validation, OAuth2, external API integration (all previously reviewed in original #550)
Core Assessment
- Problem clearly stated: ✅ — linked to #548, follows Custom Gateway ADR
- Approach appropriate: ✅ — clean adapter module, fits the architecture from #550
- Alternatives considered: ✅ — ADR + research from original #550 discussion
- Best approach for now: ✅ — plugin architecture keeps Teams isolated from core
Findings
🟠 IMPORTANT — Duplicate "no adapters configured" warning
main.rs has two "no adapters configured" warnings:
- Line ~219 (existing, not removed):
if telegram_bot_token.is_none() && line_access_token.is_none()— fires before Teams adapter is initialized - Line ~237 (new):
if telegram_bot_token.is_none() && line_access_token.is_none() && teams.is_none()— correct check
If only Teams is configured, the old warning fires incorrectly. Fix: remove the original 2-adapter check (the new 3-adapter check replaces it).
ℹ️ Info
- Code is from the original #550 (reviewed by obrutjack + masami-agent), all previous blockers resolved (JWT issuer validation, JWKS cache miss fallback,
Activity.Fromfield) - 16 unit tests covering tenant resolution, JWT error paths, activity deserialization, config parsing
docs/msteams-selfhosted.mdis thorough — Azure setup, manifest options, Docker Compose, troubleshooting- All sensitive values use
<YOUR_xxx>placeholders ✅ - CI passes ✅
jsonwebtoken = "9"is a justified dependency
⚪ Nits
Instantimport added tomain.rsviause std::time::Instant— already imported on main from #550, so this is fine
Verdict
COMMENT — fix the duplicate warning, then ready for approval.
The old 2-adapter check fired before Teams adapter initialization, producing a false warning when only Teams is configured. Removed in favor of the new 3-adapter check that runs after all adapters init.
|
Hi @neilkuan! 👋 Since you've already done the E2E POC for Teams (verified working in #550), could you help review this PR? The Teams adapter code is extracted from the original #550 and integrated with the new modular gateway architecture. Your review would be especially valuable for:
Thank you! 🙏 |
chaodu-agent
left a comment
There was a problem hiding this comment.
🔍 Four-Monk Triage Review — PR #667
Reviewers: 超渡法師, 普渡法師, 擺渡法師, 覺渡法師(pending)
Great work on the Teams adapter — the architecture follows the existing adapter pattern perfectly, JWKS cache with auto-refresh on kid miss is solid, resolved_tenant_id() three-level fallback is thorough, and the 304-line self-hosted docs are production-grade. 16 unit tests cover the critical paths well.
However, we found three issues that should be addressed before merge:
🔴 B1 — Missing serviceUrl claim validation (Auth contract)
The JWT validation in validate_jwt() checks issuer, audience, expiry, and signature, but does not verify that the token's serviceUrl claim matches the inbound Activity's serviceUrl.
This is a documented requirement in the Bot Framework auth spec. Since the gateway subsequently caches activity.service_url and uses it for outbound replies with an OAuth bearer token, the trust chain is incomplete without this binding.
Suggested fix: After decode(), compare the token's serviceUrl claim against the Activity's serviceUrl. Reject if they don't match.
🔴 B2 — Missing JWK endorsements / channelId validation (Auth contract)
The JWT validation does not verify that the signing key's endorsements include the inbound Activity's channelId (msteams).
This is also a documented requirement in the Bot Framework auth contract. Even though channelId is currently always msteams, omitting this check means the validation accepts tokens that lack the required channel endorsement constraint.
Suggested fix: When looking up the JWK by kid, also verify that the key's endorsements array contains the Activity's channelId. Add an endorsements field to the JwkKey struct.
🔴 B3 — Empty service_url cached without guard (Correctness)
In the webhook handler, service_url defaults to "" via unwrap_or("") and is inserted into the cache unconditionally. When handle_reply later uses this empty string, send_activity constructs a URL like /v3/conversations/{id}/activities (missing host), which will always fail.
Suggested fix:
if service_url.is_empty() {
warn!("teams: activity missing service_url, cannot route replies");
return StatusCode::OK;
}🟡 Suggestions (non-blocking, recommended for follow-up)
validate_nbf = false— Consider enabling nbf validation withvalidation.leeway = 30for clock skew tolerance instead of skipping entirelyalgorithmsincludes RS384 — OpenID metadata only declares RS256; consider restricting tovec![Algorithm::RS256]service_urldomain sanity check — Defense-in-depth (note: Microsoft has markedIsTrustedServiceUrlas obsolete, so this should not be the primary defense)teams_service_urlsunbounded — Consider adding a max size similar to LINE'sREPLY_TOKEN_CACHE_MAXjsonwebtoken = "9"unpinned — Security-critical dependency; consider pinning to exact versionupdate_activity()dead code — Defined but never called; add#[allow(dead_code)]with comment or defer to streaming PR- JWKS
refresh_jwksthundering herd — Multiple concurrent cache misses during key rotation will all hit Microsoft's endpoint; consider documenting this as accepted behavior config_from_envtest usesremove_var— Race condition risk in multi-threaded test runner; consider#[serial_test::serial]ortemp_env
Overall this is a well-structured PR. The three blocking items are all straightforward fixes (~20 lines total) and don't require architectural changes.
…rsements, empty guard - B1: Validate JWT serviceUrl claim matches activity's serviceUrl - B2: Add JWK endorsements field, validate channelId is endorsed - B3: Guard against empty service_url — skip caching, return early - Update unit tests for new validate_jwt(auth, activity) signature
|
Thank you for the thorough review! I've addressed all three blockers in commit 8f911fb:
Also updated the 3 JWT unit tests to pass the The 🟡 suggestions are noted for follow-up. Please take another look when you get a chance. |
obrutjack
left a comment
There was a problem hiding this comment.
All three four-monk blockers addressed:
- B1 ✅ serviceUrl claim validation
- B2 ✅ JWK endorsements/channelId validation
- B3 ✅ Empty service_url guard
CI green. LGTM.
chaodu-agent
left a comment
There was a problem hiding this comment.
🔍 Re-review: commit 8f911fb
Thanks for the quick turnaround on B1/B2/B3. B3 is fixed correctly ✅. B1 and B2 have the right structure but both have silent-skip gaps that need tightening:
✅ B3 — Empty service_url guard — FIXED
Guard is placed before cache insert. Correct.
⚠️ B1 — serviceUrl claim — silent-skip when token lacks claim
Current code:
if let Some(activity_service_url) = activity.service_url.as_deref() {
if let Some(token_service_url) = token_data.claims.get("serviceurl")... {
// only checks here
}
// token missing claim → silently passes
}If the token has no serviceUrl claim, the inner if let falls through and validation passes without checking. The activity-side skip is covered by B3 (empty service_url early return), but the token-side skip has no safety net.
Suggested fix: If the activity has a serviceUrl but the token does not contain the serviceUrl claim, reject:
if let Some(activity_service_url) = activity.service_url.as_deref() {
match token_data.claims.get("serviceurl").and_then(|v| v.as_str()) {
Some(token_service_url) if token_service_url == activity_service_url => {}
Some(token_service_url) => {
anyhow::bail!("serviceUrl mismatch: token={token_service_url}, activity={activity_service_url}");
}
None => {
anyhow::bail!("token missing serviceUrl claim");
}
}
}⚠️ B2 — endorsements — two silent-skip paths
Current code:
if let Some(channel_id) = activity.channel_id.as_deref() {
if !key.endorsements.is_empty() && !key.endorsements.iter().any(|e| e == channel_id) {
anyhow::bail!(...);
}
}Two gaps:
- Activity missing
channelId→ entire check skipped. Should reject — cannot validate without it. key.endorsementsis empty → check skipped. A key with no endorsements has not declared authorization for any channel and should not be trusted.
Suggested fix:
let channel_id = activity.channel_id.as_deref()
.ok_or_else(|| anyhow::anyhow!("activity missing channelId"))?;
if key.endorsements.is_empty() || !key.endorsements.iter().any(|e| e == channel_id) {
anyhow::bail!(
"JWK endorsements {:?} do not include channelId={channel_id}",
key.endorsements
);
}Summary
| Item | Status |
|---|---|
| B1 serviceUrl claim | |
| B2 endorsements | |
| B3 empty service_url | ✅ Fixed |
All fixes are ~3 lines each. The auth contract checks need to fail-closed (reject on missing input), not fail-open (skip on missing input).
- B1: Reject when activity or token lacks serviceUrl (was silent pass) - B2: Reject when activity lacks channelId or JWK has empty endorsements (was silent pass)
chaodu-agent
left a comment
There was a problem hiding this comment.
✅ Four-Monk Re-review: commit d89ed15
All three blocking items are now resolved. Auth validation is fully fail-closed.
B1 — serviceUrl claim ✅
- Activity missing
serviceUrl→ reject - Token missing
serviceurlclaim → reject - Mismatch → reject
- No silent-skip paths remain
B2 — endorsements / channelId ✅
- Activity missing
channelId→ reject - JWK
endorsementsempty → reject endorsementsdoes not containchannelId→ reject- No silent-skip paths remain
B3 — empty service_url guard ✅ (fixed in previous commit)
🟡 Remaining suggestions (non-blocking, recommended for follow-up)
validate_nbf = false→ considervalidation.leeway = 30algorithmsincludes RS384 → consider RS256-onlyservice_urldomain sanity check (defense-in-depth)teams_service_urlsunbounded — consider max sizejsonwebtoken = "9"— consider pinning exact versionupdate_activity()dead code annotation- JWKS
refresh_jwksthundering herd documentation config_from_envtestremove_varrace condition
LGTM — approving. Great work addressing the auth contract gaps. 🙏
Discord Discussion URL: https://discord.com/channels/1488041051187974246/1497258664090931280
Description
Add MS Teams as a gateway adapter plugin, with self-hosted deployment documentation. This is the first of two Teams PRs split from the original #550.
Closes #548
Architecture
Changes
gateway/src/adapters/teams.rsgateway/src/adapters/mod.rsgateway/src/main.rsgateway/Cargo.tomljsonwebtoken = "9"dependencydocs/msteams-selfhosted.mdTeams Adapter Features
POST /webhook/teamsendpoint for Bot Framework activitieshttps://api.botframework.comper Bot Framework auth specPOST /v3/conversations/{id}/activitiesvia per-conversationserviceUrlcaching (4-hour TTL)Test Coverage (16 unit tests)
ensure_trailing_slash()check_tenant()validate_jwt()error pathsTeamsConfig::from_env()Context
This is part of the PR #550 split:
Notes
<YOUR_xxx>placeholders