Skip to content

feat(trust): [wecom]/[googlechat]/[teams] first-class trust sections (Phase 1)#1366

Open
chaodu-agent wants to merge 2 commits into
mainfrom
feat/1358-1360-gateway-platform-trust
Open

feat(trust): [wecom]/[googlechat]/[teams] first-class trust sections (Phase 1)#1366
chaodu-agent wants to merge 2 commits into
mainfrom
feat/1358-1360-gateway-platform-trust

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Note

Was stacked on #1365; now rebased onto main (ddc9e5d) after #1363/#1365 merged. Diff is standalone.

What problem does this solve?

WeCom, Google Chat, and MS Teams are the last gateway platforms whose identity trust is controlled only by the uniform GATEWAY_ALLOW_ALL_USERS / GATEWAY_ALLOWED_USERS env vars — no per-platform granularity. This clones the [line] pattern (#1365) to all three, completing the "first-class [section] per platform" row for the simple cases in umbrella #1356.

Implements the config-section + registry-wiring + deprecation tasks of #1358 (WeCom), #1359 (Google Chat), #1360 (Teams). Platform-specific is_bot derivation and deny-echo tasks on those issues remain follow-ups, as with LINE.

At a Glance

┌─ Trust sources ───────────────────┐  ┌─ PlatformTrustConfigs ────────────┐
│                                   │  │                                   │
│  ┌─────────────────────────────┐  │  │  ┌─────────────────────────────┐  │
│  │ [wecom]      / WECOM_*  NEW │──┼──┼─▶│ wecom      ← override   NEW │  │
│  ├─────────────────────────────┤  │  │  ├─────────────────────────────┤  │
│  │ [googlechat] / GOOGLE_  NEW │──┼──┼─▶│ googlechat ← override   NEW │  │
│  │                CHAT_*       │  │  │  ├─────────────────────────────┤  │
│  ├─────────────────────────────┤  │  │  │ teams      ← override   NEW │  │
│  │ [teams]      / TEAMS_*  NEW │──┼──┼─▶├─────────────────────────────┤  │
│  └─────────────────────────────┘  │  │  │ line       ← [line] (#1365) │  │
│  ┌─────────────────────────────┐  │  │  ├─────────────────────────────┤  │
│  │ [line] (#1365)              │  │  │  │ telegram   ← [telegram]     │  │
│  │ [telegram] (#1297)          │  │  │  ├─────────────────────────────┤  │
│  └─────────────────────────────┘  │  │  │ feishu     ← uniform seed   │  │
│  ┌─────────────────────────────┐  │  │  │ (last one — #1357, needs    │  │
│  │ GATEWAY_* env (legacy)      │  │  │  │  double-gate elimination)   │  │
│  │ ⚠ warns per platform when   │  │  │  └─────────────────────────────┘  │
│  │ it still drives trust       │  │  └───────────────────────────────────┘
│  └─────────────────────────────┘  │
└───────────────────────────────────┘

Proposed Solution

  • config.rs — one shared PlatformTrustConfig { allow_all_users, allowed_users } + resolve_with_env(prefix) / env_trust_present(prefix), instead of three copy-pasted structs (the duplication lesson from feat(trust): Phase 1 (slack) — L3 identity via shared gate #1363's self-review, applied proactively). Env prefixes follow each adapter's existing convention: WECOM, GOOGLE_CHAT, TEAMS. Platforms that later need richer fields (e.g. trusted_bot_ids for WeCom/Teams) graduate to their own struct, as LINE will for group policy.
  • main.rs — a platform_trust_override helper applies the section (or its env) over the uniform GATEWAY_* seed and logs the shared Phase 1 deprecation warning when an active platform is still trust-driven by legacy env. Activity signals reuse has_unified_platform_env's exact conditions: WECOM_CORP_ID, GOOGLE_CHAT_ENABLED, TEAMS_APP_ID.
  • Docsconfig.toml.example, config-reference.md (combined [wecom]/[googlechat]/[teams] section with per-platform sender-ID formats), wecom.md, google-chat.md, msteams-selfhosted.md User Trust sections with deprecation callouts.

Why this approach?

Backward-compatible by construction (warning only; overrides are opt-in), and identical semantics to the accepted [telegram]/[line] shape so operators learn one pattern. The shared struct + helper means #1357 (Feishu, the remaining platform) only needs to handle its extra complexity (double-gate elimination, trusted_bot_ids), not re-implement resolution.

Self-review

Reviewed with the standard SOP after opening:

  • Registry/override audit: overrides run after the uniform seed; HashMap insert semantics replace correctly; activity signals verified identical to has_unified_platform_env.
  • Prefix-independence: covered by test scenario 7 (WECOM env must not bleed into TEAMS).
  • 🟡→fixed Mode-scoping docs gap (c058cbf): gate_incoming runs only on the embedded/unified adapter path; the standalone-gateway WebSocket path enforces trust via [gateway].allow_all_users/allowed_users and never consults the registry. The User Trust doc sections were placed in standalone-gateway setup guides without saying so. Added mode-scoping callouts to wecom.md, google-chat.md, msteams-selfhosted.md, config-reference.md — and to line.md, since the merged feat(line): first-class [line] trust section (Phase 1) #1365 docs had the same gap.
  • ℹ️ LINE block stays bespoke (documented in-code): LineConfig grows group-policy fields next, so routing it through platform_trust_override would be undone by the roadmap.

Validation

At ddc9e5d (macOS arm64, rebased onto main 6cf869d; full suite re-run post-rebase — 660 passed, 1 known pre-existing failure):

  • cargo clippy --workspace --all-features -- -D warnings — clean
  • cargo clippy --workspace -- -D warnings (default features) — clean
  • cargo check --features unified (wecom/googlechat/teams features on) — pass
  • cargo check -p openab-core --no-default-features — pass
  • cargo test -p openab-core --all-features — 658 passed, 1 failed: secrets::tests::resolve_exec_nonzero_exit (known pre-existing macOS-only failure, also fails on unmodified main)
  • New tests — 2/2 (platform_trust_sections_parse_from_toml, platform_trust_resolve_all_scenarios)
  • rustfmt --check — no new drift (config.rs 0→0 after fixing one introduced diff, main.rs 4→4)

Tests

  • platform_trust_sections_parse_from_toml — all three sections parse; absent sections → None
  • platform_trust_resolve_all_scenarios — seven env-resolution scenarios in one env-race-safe test fn (same pattern as line_resolve_all_scenarios), including a prefix-independence scenario (WECOM env must not bleed into TEAMS)

Refs #1358 #1359 #1360 (config-section/registry/deprecation tasks — is_bot derivation and deny-echo remain), umbrella #1356, ADR #1291.

…(Phase 1)

Clone the [line] pattern to the three remaining gateway platforms via a
shared PlatformTrustConfig (allow_all_users/allowed_users with
{PREFIX}_ALLOW_ALL_USERS / {PREFIX}_ALLOWED_USERS env fallbacks and
deny-all default). Env prefixes follow the adapters' existing
conventions: WECOM, GOOGLE_CHAT, TEAMS.

main.rs gains a platform_trust_override helper that applies the
first-class section (or its env) over the uniform GATEWAY_* seed, and
logs the shared Phase 1 deprecation warning when an active platform is
still trust-driven by the legacy env (activity signals: WECOM_CORP_ID,
GOOGLE_CHAT_ENABLED, TEAMS_APP_ID — same as has_unified_platform_env).

Trust-only by design: platform credentials stay on the gateway env
vars. Platforms needing richer trust fields later (trusted_bot_ids for
wecom/teams per their issues) graduate to their own struct, as LINE
will for group policy.

Docs: config.toml.example, config-reference.md (combined section with
per-platform sender-ID formats), wecom.md, google-chat.md,
msteams-selfhosted.md.

Refs #1358 #1359 #1360, umbrella #1356, ADR #1291. Stacked on #1365.
@chaodu-agent chaodu-agent marked this pull request as ready for review July 11, 2026 16:18
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 11, 2026 16:18
@chaodu-agent chaodu-agent force-pushed the feat/1358-1360-gateway-platform-trust branch from 7d5502f to ddc9e5d Compare July 11, 2026 16:18
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 11, 2026 16:18
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Clean, well-structured Phase 1 trust sections for all remaining simple-case gateway platforms.

What This PR Does

WeCom, Google Chat, and MS Teams were the last gateway platforms relying solely on the uniform GATEWAY_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERS env vars for identity trust — no per-platform granularity. This PR adds first-class [wecom]/[googlechat]/[teams] config sections following the same shape as the accepted [line]/[telegram] pattern, completing the Phase 1 umbrella (#1356).

How It Works

  • Shared PlatformTrustConfig struct — a single generic struct with resolve_with_env(prefix) eliminates the duplication that plagued feat(trust): Phase 1 (slack) — L3 identity via shared gate #1363. Each platform passes its env prefix (WECOM, GOOGLE_CHAT, TEAMS).
  • platform_trust_override helper in main.rs — centralizes the override-or-warn logic for all three platforms. Activity detection reuses existing env-var conventions (WECOM_CORP_ID, GOOGLE_CHAT_ENABLED, TEAMS_APP_ID).
  • Resolution chain: [section].field{PREFIX}_* env → deny-all default. Backward-compatible (deprecated path just warns).

Findings

# Severity Finding Location
1 🟢 Shared struct eliminates repetition proactively — lesson learned from #1363 applied config.rs:838
2 🟢 Seven env-resolution scenarios with prefix-independence check prevents cross-contamination regressions config.rs:2084
3 🟢 Consistent deprecation warning text with concrete migration path and Phase 2 reference main.rs:157
4 🟢 Documentation covers sender-ID format per platform — operators know what to put in allowed_users config-reference.md
Finding Details

🟢 F1: Shared PlatformTrustConfig eliminates duplication

Instead of three copy-pasted structs (one per platform), a single generic struct with a prefix parameter handles all three platforms. This is the direct application of the lesson from #1363's self-review where LineTrustConfig was just a clone of TelegramConfig. Well done.

🟢 F2: Comprehensive test coverage with prefix-independence

The platform_trust_resolve_all_scenarios test covers: defaults (deny-all), config-wins-over-env, env fallback with comma parsing, empty-string-as-unset, "0"/"false" falsy values, explicit empty list = deny-all, and prefix independence (WECOM env does not bleed into TEAMS). This is thorough.

🟢 F3: Clear deprecation path

The warning message explicitly names the deprecated env vars, the migration target (section or new env prefix), and the future consequence (Phase 2 startup error with issue reference). Operators get everything they need from one log line.

🟢 F4: Platform-specific sender-ID documentation

Each platform doc and the config reference specify the exact ID format operators need ("zhangsan" for WeCom UserIDs, "users/123456789" for Google Chat resource names, "29:1abc..." for Bot Framework IDs). This eliminates guesswork.

Baseline Check

No overlap with main — this is genuinely new code building on the merged [line] pattern.

What's Good (🟢)
  • Proactive deduplication: shared struct + helper instead of three copy-pasted blocks
  • Identical semantics to existing [telegram]/[line] — operators learn one pattern
  • Clear graduation path documented: platforms needing extra fields (e.g. trusted_bot_ids) can evolve from PlatformTrustConfig to their own struct
  • Self-contained — no changes to existing platform behavior, purely additive with opt-in override
  • Honest validation section in PR body (includes known pre-existing failure rather than hiding it)

5️⃣ Three Reasons We Might Not Need This PR

  1. Phase 1 is config-only plumbing with no enforcement change — until adapters actually call the per-platform trust config for request filtering (Phase 2), the sections are dead config that only produces a registry entry. Counter: the registry wiring is the prerequisite, and the deprecation warnings start driving migration now.

  2. Three platforms at once increases review surface — could have been three smaller PRs (one per platform). Counter: because PlatformTrustConfig is shared, splitting would create artificial sequencing; the combined PR is actually smaller than three would be.

  3. Uniform GATEWAY_* still works (just warns) — operators have no pressing reason to migrate today. Counter: starting the deprecation clock now gives operators the full Phase 1→2 window to migrate at their pace, avoiding a surprise breaking change later.

Self-review finding: gate_incoming runs only on the embedded/unified
adapter path (process_gateway_event); the standalone-gateway WebSocket
path enforces trust via [gateway].allow_all_users/allowed_users and
never consults the registry. The User Trust doc sections were placed in
standalone-gateway setup guides without that scoping, overselling where
[wecom]/[googlechat]/[teams] (and the merged [line]) take effect.

Add a mode-scoping callout to each affected doc (wecom.md,
google-chat.md, msteams-selfhosted.md, line.md, config-reference.md)
pointing standalone deployments at [gateway] trust until Phase 1c
consolidates the two paths. Also document why the LINE override stays
bespoke instead of using platform_trust_override (group-policy fields
land on LineConfig next).
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Clean, well-tested per-platform trust graduation with no gaps.

What This PR Does

Adds first-class [wecom], [googlechat], and [teams] trust sections to replace the uniform GATEWAY_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERS env vars for these platforms, completing the per-platform trust pattern established by [telegram] and [line].

How It Works

A shared PlatformTrustConfig struct (two fields: allow_all_users, allowed_users) with a generic resolve_with_env(prefix) method resolves config → env → deny-all default. A platform_trust_override helper in main.rs applies each section over the uniform seed in the PlatformTrustConfigs registry and logs a deprecation warning when an active platform still relies on the legacy GATEWAY_* env. Activity signals match has_unified_platform_env() exactly.

Findings

# Severity Finding Location
1 🟢 Shared struct eliminates duplication while keeping extension path open (platforms can graduate to bespoke structs later) config.rs:840-905
2 🟢 Thorough test coverage — 7 scenarios incl. prefix isolation, empty-string edge cases, config-over-env precedence config.rs:2063-2185
3 🟢 Mode-scoping docs callout retroactively added to line.md (filling a gap from #1365) docs/line.md
4 🟢 Backward-compatible by construction — warnings only, no breaking changes src/main.rs
5 🟢 LINE block intentionally excluded from platform_trust_override with clear in-code rationale (group-policy fields coming next) src/main.rs:457-462
What's Good (🟢)
  • No code duplication — the shared struct + generic prefix approach means adding Feishu (feat(feishu): migrate Feishu trust config to first-class [feishu] section (Phase 1) #1357) later only needs to handle its unique double-gate complexity, not re-implement resolution.
  • Env edge cases handled — empty string = unset, "0"/"false" = false, comma-separated parsing trims and drops empties.
  • Prefix independence verified — test scenario 7 explicitly asserts WECOM env vars don't bleed into TEAMS resolution.
  • Self-review proactively fixed the mode-scoping docs gap before this review (commit c058cbf), and extended the fix to the already-merged LINE docs.
  • Consistent activity signalsWECOM_CORP_ID, GOOGLE_CHAT_ENABLED (with value check), TEAMS_APP_ID match the existing has_unified_platform_env() conditions exactly.
Baseline Check
  • PR opened: 2026-07-11
  • Main already has: uniform GATEWAY_* trust seed applied to all platforms in the registry loop; [telegram] and [line] override blocks; feature flags for wecom/googlechat/teams adapters
  • Net-new value: per-platform [wecom]/[googlechat]/[teams] trust sections, shared PlatformTrustConfig struct, platform_trust_override helper, deprecation warning for legacy env, comprehensive docs with mode-scoping callouts

5️⃣ Three Reasons We Might Not Need This PR

  1. Phase 2 could do it all at once — the deprecation warning is informational only; operators could wait for the breaking change and migrate then. Counter: early migration path reduces Phase 2 blast radius and gives operators a full release cycle to migrate.

  2. Three platforms, zero functional change — no user-visible behavior changes unless someone actively migrates their config; this is "just" opt-in plumbing. Counter: the registry override is the prerequisite for platform-specific is_bot derivation and deny-echo in the follow-up tasks on feat(wecom): migrate WeCom trust config to first-class [wecom] section (Phase 1) #1358/feat(googlechat): migrate Google Chat trust config to first-class [googlechat] section (Phase 1) #1359/feat(teams): migrate MS Teams trust config to first-class [teams] section (Phase 1) #1360.

  3. Shared struct might be premature — what if platforms diverge sooner than expected? — If WeCom needs trusted_bot_ids before Feishu ships, the shared struct becomes a migration burden. Counter: graduation to a bespoke struct is a one-liner change (swap type in Config); the shared struct explicitly documents this exit path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant