From 695093445931315810d6b100b9601e8dc9c228ff Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 4 Jun 2026 10:29:38 +0200 Subject: [PATCH 1/3] Spike: Microsoft Teams Adapter (Bot Framework) (ADR 0007) Implements the ADR 0007 Teams adapter as a code spike under adapters/msteams, behind the existing chat.Adapter interface with the runtime and core types unchanged. This is a SPIKE: exercised end to end against fake Bot Framework servers, but NOT yet validated against a real Azure Bot resource / Teams tenant. The ADR Open Questions remain the live-validation checklist. What it does: - Inbound: validates the per-request Bot Connector JWT against the OpenID/JWKS metadata (Bearer, RS256-only, kid, signature, iss, aud==AppID, exp/nbf with 5-min skew, serviceurl claim bound to Activity.serviceUrl), with a JWKS cache (>=24h, refresh on kid miss/rotation) and a strict fail-closed channel endorsement check. No switch disables validation. - Outbound: client_credentials token minted and cached in process memory, lazily refreshed; replies/proactive posts go out as separate authenticated Connector "send to conversation" REST calls. Runtime State stores no credentials. - Opaque Thread ID serializes the minimal conversationReference (serviceUrl, conversation id, tenant id, bot id, channel id) so posting survives restarts; ValidateThreadID round-trips it for Thread Handle reconstruction. - Normalizes message Activities to Event/Message/Actor: bot-mention stripping, Mentioned from entities (never text matching), tenant-safe self-filtering, personal-scope DirectMessage, full Activity preserved via the Platform Escape Hatch. Outbound rate-limit retry (ADR 0005) reuses internal/ratelimit and is surfaced through the Observer (ADR 0010). Key deviation (spike finding, resolves Open Question 9): JWT/JWKS validation is standard-library only, so the zero-dependency core gains no JWT library; msbotbuilder-go is not adopted and golang-jwt is unnecessary. Deferred per ADR: Adaptive Card native content / interaction + command invokes (0004/0003), multi-tenant install (0006), Graph message history (0009), ephemeral. Tested: inbound-auth matrix incl. endorsement + cache/rotation, normalization, self-filter, mention detection, Thread ID round-trip, token mint/cache/refresh, Connector posting (markdown/plain) + proactive error mapping + 429 retry, and a full chat.New runtime integration (OnNewMention -> Connector reply). go build/vet and the root module test suite are green; no new module dependencies. Change-Id: I64a892e6d4056c9b08f527ce33b7ff0ecbe2abb7 Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Thomas Kosiewski --- adapters/msteams/activity.go | 196 +++++++++++++++ adapters/msteams/auth.go | 363 +++++++++++++++++++++++++++ adapters/msteams/auth_test.go | 220 ++++++++++++++++ adapters/msteams/connector.go | 166 ++++++++++++ adapters/msteams/connector_test.go | 199 +++++++++++++++ adapters/msteams/doc.go | 62 +++++ adapters/msteams/export_test.go | 20 ++ adapters/msteams/integration_test.go | 80 ++++++ adapters/msteams/msteams.go | 280 +++++++++++++++++++++ adapters/msteams/normalize_test.go | 225 +++++++++++++++++ adapters/msteams/retry.go | 172 +++++++++++++ adapters/msteams/testutil_test.go | 196 +++++++++++++++ adapters/msteams/threadid.go | 86 +++++++ adapters/msteams/threadid_test.go | 59 +++++ adapters/msteams/token.go | 109 ++++++++ adapters/msteams/token_test.go | 67 +++++ docs/adr/0007-teams-adapter.md | 45 +++- 17 files changed, 2544 insertions(+), 1 deletion(-) create mode 100644 adapters/msteams/activity.go create mode 100644 adapters/msteams/auth.go create mode 100644 adapters/msteams/auth_test.go create mode 100644 adapters/msteams/connector.go create mode 100644 adapters/msteams/connector_test.go create mode 100644 adapters/msteams/doc.go create mode 100644 adapters/msteams/export_test.go create mode 100644 adapters/msteams/integration_test.go create mode 100644 adapters/msteams/msteams.go create mode 100644 adapters/msteams/normalize_test.go create mode 100644 adapters/msteams/retry.go create mode 100644 adapters/msteams/testutil_test.go create mode 100644 adapters/msteams/threadid.go create mode 100644 adapters/msteams/threadid_test.go create mode 100644 adapters/msteams/token.go create mode 100644 adapters/msteams/token_test.go diff --git a/adapters/msteams/activity.go b/adapters/msteams/activity.go new file mode 100644 index 0000000..a2203b0 --- /dev/null +++ b/adapters/msteams/activity.go @@ -0,0 +1,196 @@ +package msteams + +import ( + "encoding/json" + "errors" + "strings" + + "github.com/coder/chat" +) + +// activity is the Supported Platform Shape of a Bot Framework Activity: only the +// fields this slice reads. Decoding is permissive -- unrelated Bot Framework fields +// are tolerated -- and the full raw JSON is preserved as the Platform Escape Hatch +// on both Event.Raw and Message.Raw. +type activity struct { + Type string `json:"type"` + ID string `json:"id"` + ChannelID string `json:"channelId"` + ServiceURL string `json:"serviceUrl"` + Text string `json:"text"` + From channelAccount `json:"from"` + Recipient channelAccount `json:"recipient"` + Conversation conversationAccount `json:"conversation"` + Entities []entity `json:"entities"` + Raw json.RawMessage `json:"-"` +} + +func (a *activity) UnmarshalJSON(data []byte) error { + type alias activity + var decoded alias + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + *a = activity(decoded) + a.Raw = append(json.RawMessage(nil), data...) + return nil +} + +type channelAccount struct { + ID string `json:"id"` + Name string `json:"name"` + AADObjectID string `json:"aadObjectId"` +} + +type conversationAccount struct { + ID string `json:"id"` + TenantID string `json:"tenantId"` + ConversationType string `json:"conversationType"` + IsGroup bool `json:"isGroup"` +} + +type entity struct { + Type string `json:"type"` + Text string `json:"text"` + Mentioned channelAccount `json:"mentioned"` +} + +// normalizeActivity converts a decoded Activity into a runtime Event. It returns +// ok=false for the Ignored Events of this slice: any non-message activity type, and +// the bot's own messages. +// +// Self-filtering is done here (not left solely to the runtime's BotActor() match) +// because a single-install Teams bot can receive activity from more than one +// Platform Tenant, so the runtime's tenant-scoped isSelfActor cannot be relied on +// to catch the bot's own echo -- the same reason the Slack multi-tenant path drops +// self messages in the adapter. +func (a *Adapter) normalizeActivity(act activity) (*chat.Event, bool, error) { + if !strings.EqualFold(act.Type, "message") { + return nil, false, nil + } + if act.ID == "" { + return nil, false, errors.New("msteams: activity id is required") + } + if act.ServiceURL == "" { + return nil, false, errors.New("msteams: activity service url is required") + } + if act.Conversation.ID == "" { + return nil, false, errors.New("msteams: activity conversation id is required") + } + tenantID := act.Conversation.TenantID + if tenantID == "" { + return nil, false, errors.New("msteams: activity conversation tenant id is required") + } + + author := a.actorForActivity(act) + if author.ID == "" { + return nil, false, errors.New("msteams: activity author is required") + } + if author.BotKind == chat.BotBot && a.botID != "" && author.ID == a.botID { + // The bot's own message: an Ignored Event, dropped before dispatch. + return nil, false, nil + } + + threadID, err := encodeThreadID(conversationReference{ + ServiceURL: act.ServiceURL, + ConversationID: act.Conversation.ID, + TenantID: tenantID, + BotID: act.Recipient.ID, + ChannelID: act.ChannelID, + ConversationType: act.Conversation.ConversationType, + IsGroup: act.Conversation.IsGroup, + }) + if err != nil { + return nil, false, err + } + + direct := act.Conversation.ConversationType == "personal" || + (act.Conversation.ConversationType == "" && !act.Conversation.IsGroup) + + return &chat.Event{ + ID: act.ID, + Adapter: adapterName, + Tenant: tenantID, + ThreadID: threadID, + DirectMessage: direct, + Raw: act.Raw, + Message: &chat.Message{ + ID: act.ID, + Text: stripBotMention(act), + Author: author, + Mentioned: botMentioned(act), + Raw: act.Raw, + }, + }, true, nil +} + +// actorForActivity maps the inbound author. The canonical Actor.ID prefers +// from.aadObjectId (tenant-stable across conversations) and falls back to from.id; +// a self-authored message (from.id == the configured bot id) is tagged BotBot with +// from.id so it matches BotActor() for Self Message filtering. The exact canonical +// key is spike-required (ADR 0007 Open Question 10). +func (a *Adapter) actorForActivity(act activity) chat.Actor { + id := firstNonEmpty(act.From.AADObjectID, act.From.ID) + kind := chat.BotHuman + if a.botID != "" && act.From.ID == a.botID { + id = act.From.ID + kind = chat.BotBot + } + return chat.Actor{ + Adapter: adapterName, + Tenant: act.Conversation.TenantID, + ID: id, + Name: act.From.Name, + BotKind: kind, + } +} + +// botMentioned reports whether the bot is @mentioned, derived from the Activity's +// mention entities (mentioned.id == the bot's recipient id), never from substring +// matching on display-name text. The inbound recipient is, by definition, the bot. +func botMentioned(act activity) bool { + botID := act.Recipient.ID + if botID == "" { + return false + } + for _, e := range act.Entities { + if strings.EqualFold(e.Type, "mention") && e.Mentioned.ID == botID { + return true + } + } + return false +} + +// stripBotMention removes the bot mention from the message text so handlers see the +// user's actual words. Teams embeds the mention as "Bot" both in the text +// and as an entity carrying that exact substring; the entity text is the reliable +// thing to strip. The leading- fallback applies only when the bot is mentioned +// but no entity carried text, so it never strips another user's leading mention. +// Markdown subset fidelity beyond this is spike-required (ADR 0007 Open Question 5). +func stripBotMention(act activity) string { + text := act.Text + botID := act.Recipient.ID + stripped := false + for _, e := range act.Entities { + if strings.EqualFold(e.Type, "mention") && e.Mentioned.ID == botID && e.Text != "" { + text = strings.ReplaceAll(text, e.Text, "") + stripped = true + } + } + if !stripped && botMentioned(act) { + text = stripLeadingAtTag(text) + } + return strings.TrimSpace(text) +} + +func stripLeadingAtTag(text string) string { + trimmed := strings.TrimSpace(text) + if !strings.HasPrefix(trimmed, "") { + return text + } + end := strings.Index(trimmed, "") + if end < 0 { + return text + } + return trimmed[end+len(""):] +} diff --git a/adapters/msteams/auth.go b/adapters/msteams/auth.go new file mode 100644 index 0000000..52997cb --- /dev/null +++ b/adapters/msteams/auth.go @@ -0,0 +1,363 @@ +package msteams + +import ( + "context" + "crypto" + "crypto/rsa" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "io" + "math/big" + "net/http" + "strings" + "sync" + "time" +) + +const ( + // botConnectorIssuer is the only accepted iss for Bot Connector -> bot tokens. + botConnectorIssuer = "https://api.botframework.com" + // openIDMetadataURL is the static Bot Connector OpenID metadata document whose + // jwks_uri yields the signing keys (with Bot Framework endorsement annotations). + openIDMetadataURL = "https://login.botframework.com/v1/.well-known/openidconfiguration" + // defaultJWKSCacheTTL keeps fetched keys at least a day, refreshed on a kid miss + // (key rotation) regardless. + defaultJWKSCacheTTL = 24 * time.Hour + // clockSkew is the tolerated exp/nbf skew, per Microsoft guidance. + clockSkew = 5 * time.Minute +) + +// authValidator performs every mandatory inbound Bot Connector JWT check against a +// JWKS resolved from the Bot Framework OpenID metadata, plus the Bot Framework +// channel-endorsement check. It is a deep adapter-internal module behind a single +// validate method; there is deliberately no switch to disable any check. It is +// testable with a fake metadata+JWKS server and in-test RSA keys. +// +// JWT parsing and RS256 verification are stdlib-only (crypto/rsa over a public key +// rebuilt from the JWK n/e), so the otherwise zero-dependency core module gains no +// JWT library. This is a deliberate spike finding for ADR 0007 Open Question 9 +// (msbotbuilder-go is not adopted, and even golang-jwt is unnecessary here). +type authValidator struct { + appID string + openIDMetaURL string + issuer string + client *http.Client + now func() time.Time + cacheTTL time.Duration + + mu sync.Mutex + keys map[string]jwk + fetchedAt time.Time +} + +// jwk is the subset of a JSON Web Key the validator needs, plus the Bot Framework +// per-key endorsements list (a Bot Framework extension to the standard JWKS). +type jwk struct { + Kid string `json:"kid"` + Kty string `json:"kty"` + N string `json:"n"` + E string `json:"e"` + Endorsements []string `json:"endorsements"` + pub *rsa.PublicKey +} + +type jwtHeader struct { + Alg string `json:"alg"` + Kid string `json:"kid"` + Typ string `json:"typ"` +} + +type jwtPayload struct { + Iss string `json:"iss"` + Aud json.RawMessage `json:"aud"` + Exp int64 `json:"exp"` + Nbf int64 `json:"nbf"` + Iat int64 `json:"iat"` + ServiceURL string `json:"serviceurl"` +} + +// validate runs the full inbound check on the Authorization header for an Activity +// with the given serviceUrl and channelId. It returns nil only when every check +// passes; any failure is an error the caller maps to HTTP 403. The serviceUrl claim +// is bound to the Activity's serviceUrl so a token minted for one service URL +// cannot be replayed against a spoofed one. +func (v *authValidator) validate(ctx context.Context, authzHeader, serviceURL, channelID string) error { + raw, err := bearerToken(authzHeader) + if err != nil { + return err + } + header, payload, signingInput, sig, err := splitJWT(raw) + if err != nil { + return err + } + if !strings.EqualFold(header.Alg, "RS256") { + return fmt.Errorf("msteams: unexpected jwt alg %q", header.Alg) + } + if header.Kid == "" { + return errors.New("msteams: jwt missing kid") + } + + key, err := v.keyForKid(ctx, header.Kid) + if err != nil { + return err + } + if err := verifyRS256(key.pub, signingInput, sig); err != nil { + return err + } + + if payload.Iss != v.issuer { + return fmt.Errorf("msteams: unexpected jwt issuer %q", payload.Iss) + } + if !audienceMatches(payload.Aud, v.appID) { + return errors.New("msteams: jwt audience does not match bot app id") + } + if !validAt(payload, v.now(), clockSkew) { + return errors.New("msteams: jwt outside validity window") + } + if payload.ServiceURL == "" { + return errors.New("msteams: jwt missing serviceurl claim") + } + if !serviceURLMatches(payload.ServiceURL, serviceURL) { + return errors.New("msteams: jwt serviceurl claim does not match activity") + } + if err := checkEndorsement(key, channelID); err != nil { + return err + } + return nil +} + +// keyForKid resolves the signing key for a kid, refreshing the JWKS on a miss +// (handling key rotation) or when the cache is stale. A refresh failure is tolerated +// only when an existing key for the kid is still cached, so a transient metadata +// outage does not reject otherwise-valid traffic. +func (v *authValidator) keyForKid(ctx context.Context, kid string) (jwk, error) { + v.mu.Lock() + key, ok := v.keys[kid] + fresh := !v.fetchedAt.IsZero() && v.now().Sub(v.fetchedAt) < v.cacheTTL + v.mu.Unlock() + if ok && fresh { + return key, nil + } + + if err := v.refresh(ctx); err != nil { + if ok { + return key, nil + } + return jwk{}, err + } + + v.mu.Lock() + key, ok = v.keys[kid] + v.mu.Unlock() + if !ok { + return jwk{}, fmt.Errorf("msteams: no signing key for kid %q", kid) + } + return key, nil +} + +// refresh fetches the OpenID metadata, then the JWKS, building an rsa.PublicKey for +// each RSA key. The HTTP fetches run without the cache lock held; only the swap is +// locked. +func (v *authValidator) refresh(ctx context.Context) error { + jwksURI, err := v.discoverJWKSURI(ctx) + if err != nil { + return err + } + keys, err := v.fetchKeys(ctx, jwksURI) + if err != nil { + return err + } + v.mu.Lock() + v.keys = keys + v.fetchedAt = v.now() + v.mu.Unlock() + return nil +} + +func (v *authValidator) discoverJWKSURI(ctx context.Context) (string, error) { + var meta struct { + JWKSURI string `json:"jwks_uri"` + } + if err := v.getJSON(ctx, v.openIDMetaURL, &meta); err != nil { + return "", fmt.Errorf("msteams: fetch openid metadata: %w", err) + } + if meta.JWKSURI == "" { + return "", errors.New("msteams: openid metadata has no jwks_uri") + } + return meta.JWKSURI, nil +} + +func (v *authValidator) fetchKeys(ctx context.Context, jwksURI string) (map[string]jwk, error) { + var doc struct { + Keys []jwk `json:"keys"` + } + if err := v.getJSON(ctx, jwksURI, &doc); err != nil { + return nil, fmt.Errorf("msteams: fetch jwks: %w", err) + } + keys := make(map[string]jwk, len(doc.Keys)) + for _, k := range doc.Keys { + if k.Kid == "" || !strings.EqualFold(k.Kty, "RSA") { + continue + } + pub, err := rsaPublicKeyFromJWK(k) + if err != nil { + // Skip a malformed key rather than failing the whole rotation. + continue + } + k.pub = pub + keys[k.Kid] = k + } + if len(keys) == 0 { + return nil, errors.New("msteams: jwks has no usable RSA keys") + } + return keys, nil +} + +func (v *authValidator) getJSON(ctx context.Context, url string, dest any) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return err + } + resp, err := v.client.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return err + } + if resp.StatusCode < 200 || resp.StatusCode > 299 { + return fmt.Errorf("status %d", resp.StatusCode) + } + return json.Unmarshal(body, dest) +} + +// checkEndorsement enforces that an msteams Activity's signing key endorses the +// msteams channel: the matched key's endorsements must contain the channel id, or +// the request is rejected (HTTP 403 at the caller). This is the strict reading of +// ADR 0007; whether msteams strictly requires the endorsement, and the exact rule, +// is spike-required (Open Question 3). Failing closed is the safer default for a +// security check a human will validate before production. +func checkEndorsement(key jwk, channelID string) error { + if channelID == "" { + return nil + } + for _, e := range key.Endorsements { + if strings.EqualFold(e, channelID) { + return nil + } + } + return fmt.Errorf("msteams: signing key does not endorse channel %q", channelID) +} + +func bearerToken(header string) (string, error) { + if header == "" { + return "", errors.New("msteams: missing authorization header") + } + const prefix = "Bearer " + if len(header) <= len(prefix) || !strings.EqualFold(header[:len(prefix)], prefix) { + return "", errors.New("msteams: authorization is not a bearer token") + } + token := strings.TrimSpace(header[len(prefix):]) + if token == "" { + return "", errors.New("msteams: empty bearer token") + } + return token, nil +} + +func splitJWT(raw string) (jwtHeader, jwtPayload, []byte, []byte, error) { + parts := strings.Split(raw, ".") + if len(parts) != 3 { + return jwtHeader{}, jwtPayload{}, nil, nil, errors.New("msteams: malformed jwt") + } + headerBytes, err := base64.RawURLEncoding.DecodeString(parts[0]) + if err != nil { + return jwtHeader{}, jwtPayload{}, nil, nil, fmt.Errorf("msteams: decode jwt header: %w", err) + } + var header jwtHeader + if err := json.Unmarshal(headerBytes, &header); err != nil { + return jwtHeader{}, jwtPayload{}, nil, nil, fmt.Errorf("msteams: parse jwt header: %w", err) + } + payloadBytes, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + return jwtHeader{}, jwtPayload{}, nil, nil, fmt.Errorf("msteams: decode jwt payload: %w", err) + } + var payload jwtPayload + if err := json.Unmarshal(payloadBytes, &payload); err != nil { + return jwtHeader{}, jwtPayload{}, nil, nil, fmt.Errorf("msteams: parse jwt payload: %w", err) + } + sig, err := base64.RawURLEncoding.DecodeString(parts[2]) + if err != nil { + return jwtHeader{}, jwtPayload{}, nil, nil, fmt.Errorf("msteams: decode jwt signature: %w", err) + } + signingInput := []byte(parts[0] + "." + parts[1]) + return header, payload, signingInput, sig, nil +} + +func verifyRS256(pub *rsa.PublicKey, signingInput, sig []byte) error { + if pub == nil { + return errors.New("msteams: nil signing key") + } + hashed := sha256.Sum256(signingInput) + if err := rsa.VerifyPKCS1v15(pub, crypto.SHA256, hashed[:], sig); err != nil { + return fmt.Errorf("msteams: jwt signature invalid: %w", err) + } + return nil +} + +func rsaPublicKeyFromJWK(k jwk) (*rsa.PublicKey, error) { + nBytes, err := base64.RawURLEncoding.DecodeString(strings.TrimRight(k.N, "=")) + if err != nil { + return nil, fmt.Errorf("msteams: decode jwk modulus: %w", err) + } + eBytes, err := base64.RawURLEncoding.DecodeString(strings.TrimRight(k.E, "=")) + if err != nil { + return nil, fmt.Errorf("msteams: decode jwk exponent: %w", err) + } + if len(nBytes) == 0 || len(eBytes) == 0 { + return nil, errors.New("msteams: jwk modulus or exponent empty") + } + return &rsa.PublicKey{ + N: new(big.Int).SetBytes(nBytes), + E: int(new(big.Int).SetBytes(eBytes).Int64()), + }, nil +} + +// audienceMatches accepts the aud claim as either a JSON string or array of +// strings, matching the bot's Microsoft App ID. +func audienceMatches(aud json.RawMessage, appID string) bool { + if len(aud) == 0 || appID == "" { + return false + } + var single string + if err := json.Unmarshal(aud, &single); err == nil { + return single == appID + } + var many []string + if err := json.Unmarshal(aud, &many); err == nil { + for _, a := range many { + if a == appID { + return true + } + } + } + return false +} + +func validAt(p jwtPayload, now time.Time, skew time.Duration) bool { + if p.Exp != 0 && now.After(time.Unix(p.Exp, 0).Add(skew)) { + return false + } + if p.Nbf != 0 && now.Before(time.Unix(p.Nbf, 0).Add(-skew)) { + return false + } + return true +} + +func serviceURLMatches(a, b string) bool { + return strings.TrimRight(a, "/") == strings.TrimRight(b, "/") +} diff --git a/adapters/msteams/auth_test.go b/adapters/msteams/auth_test.go new file mode 100644 index 0000000..1952038 --- /dev/null +++ b/adapters/msteams/auth_test.go @@ -0,0 +1,220 @@ +package msteams_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/coder/chat" +) + +// recordingHandler returns a Webhook handler plus a pointer that is set when the +// runtime dispatch is invoked, so tests can tell "accepted and dispatched" from +// "rejected before dispatch". +func recordingHandler(a interface { + Webhook(chat.DispatchFunc) http.Handler +}) (http.Handler, *bool) { + dispatched := false + h := a.Webhook(func(context.Context, *chat.Event) error { + dispatched = true + return nil + }) + return h, &dispatched +} + +func TestWebhookInboundAuthMatrix(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + cases := []struct { + name string + auth func() string // Authorization header value + wantStatus int + wantDispatch bool + }{ + { + name: "valid token accepted and dispatched", + auth: func() string { return "Bearer " + bf.sign(t, bf.validClaims()) }, + wantStatus: http.StatusOK, + wantDispatch: true, + }, + { + name: "missing authorization rejected", + auth: func() string { return "" }, + wantStatus: http.StatusForbidden, + }, + { + name: "wrong scheme rejected", + auth: func() string { return "Token " + bf.sign(t, bf.validClaims()) }, + wantStatus: http.StatusForbidden, + }, + { + name: "malformed jwt rejected", + auth: func() string { return "Bearer not.a.jwt" }, + wantStatus: http.StatusForbidden, + }, + { + name: "unknown kid rejected", + auth: func() string { + return "Bearer " + signRS256(t, bf.key, "no-such-kid", bf.validClaims()) + }, + wantStatus: http.StatusForbidden, + }, + { + name: "bad signature rejected", + auth: func() string { return "Bearer " + bf.signForeign(t, bf.validClaims()) }, + wantStatus: http.StatusForbidden, + }, + { + name: "wrong issuer rejected", + auth: func() string { + c := bf.validClaims() + c["iss"] = "https://evil.example.com" + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusForbidden, + }, + { + name: "wrong audience rejected", + auth: func() string { + c := bf.validClaims() + c["aud"] = "some-other-app" + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusForbidden, + }, + { + name: "expired beyond skew rejected", + auth: func() string { + c := bf.validClaims() + c["exp"] = testClock.Add(-10 * time.Minute).Unix() + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusForbidden, + }, + { + name: "not yet valid beyond skew rejected", + auth: func() string { + c := bf.validClaims() + c["nbf"] = testClock.Add(10 * time.Minute).Unix() + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusForbidden, + }, + { + name: "expired within skew accepted", + auth: func() string { + c := bf.validClaims() + // Expired 2 minutes ago, inside the 5-minute skew window. + c["exp"] = testClock.Add(-2 * time.Minute).Unix() + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusOK, + wantDispatch: true, + }, + { + name: "serviceurl claim mismatch rejected", + auth: func() string { + c := bf.validClaims() + c["serviceurl"] = "https://attacker.example.com/" + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusForbidden, + }, + { + name: "missing serviceurl claim rejected", + auth: func() string { + c := bf.validClaims() + delete(c, "serviceurl") + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusForbidden, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h, dispatched := recordingHandler(a) + rec := postActivity(t, h, tc.auth(), messageActivity()) + if rec.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d (body %s)", rec.Code, tc.wantStatus, rec.Body.String()) + } + if *dispatched != tc.wantDispatch { + t.Fatalf("dispatched = %v, want %v", *dispatched, tc.wantDispatch) + } + }) + } +} + +func TestWebhookChannelEndorsement(t *testing.T) { + t.Parallel() + + t.Run("msteams endorsement present accepted", func(t *testing.T) { + bf := newFakeBotConnector(t) + bf.endorsements = []string{"msteams", "directline"} + a := newTestAdapter(t, bf, nil) + h, dispatched := recordingHandler(a) + rec := postActivity(t, h, "Bearer "+bf.sign(t, bf.validClaims()), messageActivity()) + if rec.Code != http.StatusOK || !*dispatched { + t.Fatalf("status = %d dispatched = %v, want 200 + dispatch", rec.Code, *dispatched) + } + }) + + t.Run("msteams endorsement absent rejected", func(t *testing.T) { + bf := newFakeBotConnector(t) + bf.endorsements = []string{"directline"} + a := newTestAdapter(t, bf, nil) + h, dispatched := recordingHandler(a) + rec := postActivity(t, h, "Bearer "+bf.sign(t, bf.validClaims()), messageActivity()) + if rec.Code != http.StatusForbidden || *dispatched { + t.Fatalf("status = %d dispatched = %v, want 403 no dispatch", rec.Code, *dispatched) + } + }) +} + +// TestWebhookValidationAlwaysOn proves there is no construction path that turns +// inbound validation off: an adapter built with every available Option still rejects +// a bad token. Options has no disable-validation field by design. +func TestWebhookValidationAlwaysOn(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + h, dispatched := recordingHandler(a) + rec := postActivity(t, h, "Bearer not.a.jwt", messageActivity()) + if rec.Code != http.StatusForbidden || *dispatched { + t.Fatalf("status = %d dispatched = %v, want 403 no dispatch", rec.Code, *dispatched) + } +} + +func TestJWKSCacheAndRotation(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + post := func() int { + h, _ := recordingHandler(a) + return postActivity(t, h, "Bearer "+bf.sign(t, bf.validClaims()), messageActivity()).Code + } + + if code := post(); code != http.StatusOK { + t.Fatalf("first post status = %d", code) + } + if code := post(); code != http.StatusOK { + t.Fatalf("second post status = %d", code) + } + // The JWKS was fetched once and reused from cache for the second request. + if bf.keyRequests != 1 { + t.Fatalf("jwks fetches = %d, want 1 (cache reuse)", bf.keyRequests) + } + + // Rotate to a new kid: a token signed with the new kid forces a refetch. + bf.kid = "test-key-2" + if code := post(); code != http.StatusOK { + t.Fatalf("post after rotation status = %d", code) + } + if bf.keyRequests != 2 { + t.Fatalf("jwks fetches after rotation = %d, want 2", bf.keyRequests) + } +} diff --git a/adapters/msteams/connector.go b/adapters/msteams/connector.go new file mode 100644 index 0000000..d87147e --- /dev/null +++ b/adapters/msteams/connector.go @@ -0,0 +1,166 @@ +package msteams + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/coder/chat" +) + +// ErrBotNotInstalled is the explicit, unwrappable error for a Connector 403 +// ForbiddenOperationException: the bot is not installed in the target conversation, +// so callers can distinguish "not installed" from a transient failure. +var ErrBotNotInstalled = errors.New("msteams: bot not installed in target conversation") + +// ErrMessageWritesBlocked is the explicit, unwrappable error for a Connector 403 +// MessageWritesBlocked: the user has blocked or uninstalled the bot. +var ErrMessageWritesBlocked = errors.New("msteams: message writes blocked by user") + +// ConnectorError carries a non-throttling Connector failure: the HTTP status and +// the Bot Framework error envelope (code/message) as a Platform Escape Hatch. Known +// 403 codes set Err to one of the sentinels above so callers can errors.Is them. +type ConnectorError struct { + Status int + Code string + Message string + Raw any + Err error +} + +func (e *ConnectorError) Error() string { + msg := fmt.Sprintf("msteams: connector status %d", e.Status) + if e.Code != "" { + msg += " (" + e.Code + ")" + } + if e.Message != "" { + msg += ": " + e.Message + } + return msg +} + +func (e *ConnectorError) Unwrap() error { return e.Err } + +// PostMessage maps a Thread.Post to the Connector "send to conversation" REST call +// (POST {serviceUrl}/v3/conversations/{conversationId}/activities) using the +// serviceUrl from the opaque Thread ID as the base URI. Plain Text renders with +// textFormat=plain and Portable Markdown with textFormat=markdown; the Sent Message +// id is the ResourceResponse id. Within a turn this is the bot's reply; out of band +// it is a proactive post -- the same call, which is why proactive 403s surface here +// as ErrBotNotInstalled / ErrMessageWritesBlocked. +func (a *Adapter) PostMessage(ctx context.Context, thread chat.ThreadRef, msg chat.PostableMessage) (*chat.SentMessage, error) { + ref, err := connRefForThread(thread) + if err != nil { + return nil, err + } + textFormat, err := textFormatFor(msg.Format) + if err != nil { + return nil, err + } + payload := outboundActivity{ + Type: "message", + Text: msg.Text, + TextFormat: textFormat, + From: outboundAccount{ID: a.botID, Name: a.botName}, + Conversation: outboundConversation{ID: ref.ConversationID}, + } + endpoint := fmt.Sprintf("%s/v3/conversations/%s/activities", + strings.TrimRight(ref.ServiceURL, "/"), url.PathEscape(ref.ConversationID)) + + var resp resourceResponse + if err := a.connectorCall(ctx, http.MethodPost, endpoint, payload, &resp); err != nil { + return nil, err + } + return &chat.SentMessage{ID: resp.ID, ThreadID: thread.ID, Raw: resp}, nil +} + +// connRefForThread reuses the conversationReference already attached to a verified +// ThreadRef.Raw and otherwise decodes the opaque Thread ID. +func connRefForThread(thread chat.ThreadRef) (conversationReference, error) { + if ref, ok := thread.Raw.(conversationReference); ok { + return ref, nil + } + return decodeThreadID(thread.ID) +} + +func textFormatFor(format chat.MessageFormat) (string, error) { + switch format { + case chat.MessageFormatText: + return "plain", nil + case chat.MessageFormatMarkdown: + return "markdown", nil + default: + return "", fmt.Errorf("msteams: unsupported message format %d", format) + } +} + +// connectorCall authorizes with the cached outbound token and sends through the +// bounded rate-limit retry loop (ADR 0005). +func (a *Adapter) connectorCall(ctx context.Context, method, endpoint string, payload, dest any) error { + token, err := a.tokens.get(ctx) + if err != nil { + return err + } + body, err := json.Marshal(payload) + if err != nil { + return fmt.Errorf("msteams: encode connector request: %w", err) + } + req, err := http.NewRequestWithContext(ctx, method, endpoint, bytes.NewReader(body)) + if err != nil { + return err + } + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + return a.doWithRetry(ctx, endpoint, req, dest) +} + +// connectorErrorFor builds a ConnectorError from a non-2xx, non-429 response, lifting +// the Bot Framework error envelope and mapping the documented 403 codes to sentinels. +func connectorErrorFor(status int, body []byte) *ConnectorError { + var env struct { + Error struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + _ = json.Unmarshal(body, &env) + ce := &ConnectorError{ + Status: status, + Code: env.Error.Code, + Message: env.Error.Message, + Raw: json.RawMessage(append([]byte(nil), body...)), + } + switch env.Error.Code { + case "ForbiddenOperationException": + ce.Err = ErrBotNotInstalled + case "MessageWritesBlocked": + ce.Err = ErrMessageWritesBlocked + } + return ce +} + +type outboundActivity struct { + Type string `json:"type"` + Text string `json:"text"` + TextFormat string `json:"textFormat,omitempty"` + From outboundAccount `json:"from"` + Conversation outboundConversation `json:"conversation"` +} + +type outboundAccount struct { + ID string `json:"id,omitempty"` + Name string `json:"name,omitempty"` +} + +type outboundConversation struct { + ID string `json:"id"` +} + +type resourceResponse struct { + ID string `json:"id"` +} diff --git a/adapters/msteams/connector_test.go b/adapters/msteams/connector_test.go new file mode 100644 index 0000000..672f37e --- /dev/null +++ b/adapters/msteams/connector_test.go @@ -0,0 +1,199 @@ +package msteams_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/coder/chat" + "github.com/coder/chat/adapters/msteams" +) + +// postTarget builds an adapter wired to a fake token endpoint and returns a +// ThreadRef whose serviceUrl points at the given Connector server, so PostMessage +// hits the fake. +func postTarget(t *testing.T, bf *fakeBotConnector, connURL string, mutate func(*msteams.Options)) (*msteams.Adapter, chat.ThreadRef) { + t.Helper() + tok := fakeTokenServer(t, "out-token", nil) + a := newTestAdapter(t, bf, func(o *msteams.Options) { + o.TokenURL = tok.URL + o.BotName = "Bot" + if mutate != nil { + mutate(o) + } + }) + id := msteams.EncodeThreadIDForTest(connURL, testConvID, testTenant, "msteams", "channel", true) + ref, err := a.ValidateThreadID(id) + if err != nil { + t.Fatalf("validate thread id: %v", err) + } + return a, ref +} + +func TestPostMessageSendToConversation(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + + var gotAuth, gotMethod, gotPath string + var gotBody map[string]any + conn := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth, gotMethod, gotPath = r.Header.Get("Authorization"), r.Method, r.URL.Path + _ = json.NewDecoder(r.Body).Decode(&gotBody) + _ = json.NewEncoder(w).Encode(map[string]any{"id": "posted-99"}) + })) + t.Cleanup(conn.Close) + + a, ref := postTarget(t, bf, conn.URL, nil) + sent, err := a.PostMessage(context.Background(), ref, chat.Markdown("**hi there**")) + if err != nil { + t.Fatalf("post: %v", err) + } + if sent.ID != "posted-99" || sent.ThreadID != ref.ID { + t.Fatalf("sent = %#v", sent) + } + if gotMethod != http.MethodPost || gotAuth != "Bearer out-token" { + t.Fatalf("method = %q auth = %q", gotMethod, gotAuth) + } + if !strings.Contains(gotPath, "/v3/conversations/") || !strings.HasSuffix(gotPath, "/activities") { + t.Fatalf("path = %q", gotPath) + } + if gotBody["type"] != "message" || gotBody["text"] != "**hi there**" || gotBody["textFormat"] != "markdown" { + t.Fatalf("body = %#v", gotBody) + } + if conv, _ := gotBody["conversation"].(map[string]any); conv["id"] != testConvID { + t.Fatalf("conversation = %#v", gotBody["conversation"]) + } +} + +func TestPostMessagePlainTextFormat(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + + var gotBody map[string]any + conn := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewDecoder(r.Body).Decode(&gotBody) + _ = json.NewEncoder(w).Encode(map[string]any{"id": "posted-1"}) + })) + t.Cleanup(conn.Close) + + a, ref := postTarget(t, bf, conn.URL, nil) + if _, err := a.PostMessage(context.Background(), ref, chat.Text("plain words")); err != nil { + t.Fatalf("post: %v", err) + } + if gotBody["textFormat"] != "plain" || gotBody["text"] != "plain words" { + t.Fatalf("body = %#v", gotBody) + } +} + +func TestPostMessageMapsProactiveErrors(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + + cases := []struct { + name string + status int + code string + wantErr error + }{ + {"not installed", http.StatusForbidden, "ForbiddenOperationException", msteams.ErrBotNotInstalled}, + {"writes blocked", http.StatusForbidden, "MessageWritesBlocked", msteams.ErrMessageWritesBlocked}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + conn := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.status) + _ = json.NewEncoder(w).Encode(map[string]any{"error": map[string]any{"code": tc.code, "message": "nope"}}) + })) + t.Cleanup(conn.Close) + + a, ref := postTarget(t, bf, conn.URL, nil) + _, err := a.PostMessage(context.Background(), ref, chat.Text("hi")) + if !errors.Is(err, tc.wantErr) { + t.Fatalf("err = %v, want errors.Is %v", err, tc.wantErr) + } + }) + } +} + +func TestPostMessageGenericErrorIsConnectorError(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + + conn := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(map[string]any{"error": map[string]any{"code": "BadArgument", "message": "bad"}}) + })) + t.Cleanup(conn.Close) + + a, ref := postTarget(t, bf, conn.URL, nil) + _, err := a.PostMessage(context.Background(), ref, chat.Text("hi")) + var ce *msteams.ConnectorError + if !errors.As(err, &ce) { + t.Fatalf("err = %v, want *ConnectorError", err) + } + if ce.Status != http.StatusBadRequest || ce.Code != "BadArgument" { + t.Fatalf("connector error = %#v", ce) + } + if errors.Is(err, msteams.ErrBotNotInstalled) || errors.Is(err, msteams.ErrMessageWritesBlocked) { + t.Fatal("generic error should not match the proactive sentinels") + } +} + +func TestConnectorRetriesOn429ThenSucceeds(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + + var attempts int32 + conn := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if atomic.AddInt32(&attempts, 1) < 3 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"id": "ok-after-retry"}) + })) + t.Cleanup(conn.Close) + + a, ref := postTarget(t, bf, conn.URL, func(o *msteams.Options) { + o.RetryPolicy = msteams.RetryPolicy{MaxAttempts: 5, BaseDelay: time.Millisecond, MaxDelay: 2 * time.Millisecond, MaxElapsed: time.Second} + }) + sent, err := a.PostMessage(context.Background(), ref, chat.Text("hi")) + if err != nil { + t.Fatalf("post: %v", err) + } + if sent.ID != "ok-after-retry" { + t.Fatalf("sent id = %q", sent.ID) + } + if got := atomic.LoadInt32(&attempts); got != 3 { + t.Fatalf("attempts = %d, want 3", got) + } +} + +func TestConnectorRateLimitedExhausts(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + + conn := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + })) + t.Cleanup(conn.Close) + + a, ref := postTarget(t, bf, conn.URL, func(o *msteams.Options) { + o.RetryPolicy = msteams.RetryPolicy{MaxAttempts: 2, BaseDelay: time.Millisecond, MaxDelay: 2 * time.Millisecond, MaxElapsed: time.Second} + }) + _, err := a.PostMessage(context.Background(), ref, chat.Text("hi")) + var rl *msteams.RateLimited + if !errors.As(err, &rl) { + t.Fatalf("err = %v, want *RateLimited", err) + } + if rl.Attempts != 2 || rl.Adapter != "msteams" { + t.Fatalf("rate limited = %#v", rl) + } +} diff --git a/adapters/msteams/doc.go b/adapters/msteams/doc.go new file mode 100644 index 0000000..1be2ea5 --- /dev/null +++ b/adapters/msteams/doc.go @@ -0,0 +1,62 @@ +// Package msteams provides the Microsoft Teams (Bot Framework) adapter for the chat +// runtime, behind the same chat.Adapter interface as Slack and Linear. +// +// # Spike status +// +// This adapter is a SPIKE implementing ADR 0007. Its behavior is exercised end to +// end against fake Bot Framework servers, but it has NOT been validated against a +// real Azure Bot resource and a live Teams tenant. The ADR's Open Questions remain +// the live-validation checklist; the spike encodes the documented behavior and +// marks the unverified assumptions inline. Treat it as ready for a human to wire to +// a real bot and confirm, not as production-proven. +// +// # Boundary shape +// +// Teams does not look like Slack/Linear at the boundary, and the adapter absorbs the +// differences so the runtime and core types stay unchanged: +// +// - Inbound auth is a per-request JWT validated against the Bot Connector JWKS +// (fetch + cache + endorsements), not a shared-secret HMAC. Every documented +// check is enforced and none can be disabled (see auth.go). JWT/JWKS handling is +// stdlib-only (crypto/rsa over a key rebuilt from the JWK n/e), so the +// zero-dependency core gains no JWT library -- a deliberate spike finding for +// Open Question 9 (msbotbuilder-go is not adopted; golang-jwt is not needed). +// - Outbound replies are separate authenticated Connector REST calls +// (client_credentials token, cached in process memory, lazily refreshed), not a +// webhook response body. Runtime State is never expanded to store credentials. +// - The opaque Thread ID serializes the minimal conversationReference +// (serviceUrl, conversation id, tenant id, bot id, channel id) so out-of-webhook +// and proactive posting survive process restarts; serviceUrl is refreshed from +// each inbound Activity. This is heavier than Slack's id but exactly what the +// opaque, adapter-produced Thread ID contract permits. +// +// # Inbound turn / dispatch +// +// The Webhook validates the JWT, normalizes a message Activity into an Event, runs +// Runtime Dispatch synchronously on the request context, then acks with HTTP 200. +// The handler's reply goes out as a separate Connector call via Thread.Post during +// dispatch; there is no body-reply shortcut for message activities. Non-message +// activity types and the bot's own messages are Ignored Events. invoke activities +// (command/card-action transport) are out of this slice. Work exceeding the Teams +// turn budget (~10-15s) is the runtime's Ack-Then-Work concern (ADR 0002), delivered +// later as a proactive Connector post through a stored Thread ID. +// +// # Normalization +// +// Event.Adapter is msteams; Event.Tenant is conversation.tenantId; Event.ID is the +// Activity id; Event.Raw and Message.Raw hold the full Activity (Platform Escape +// Hatch). Message.Text has the leading bot mention stripped; Message.Mentioned is +// derived from Activity.entities mention objects (never substring text matching); +// the inbound Actor comes from Activity.from (Bot Kind human) and BotActor() from +// the configured bot identity (Bot Kind bot). DirectMessage reflects personal scope. +// +// # Capability boundaries (deferred, cross-referenced) +// +// Adaptive Card native content (ADR 0004 Native Content), Teams card actions and +// command-style invokes (ADR 0004 Interaction Event / ADR 0003 Command Event), +// multi-tenant install (ADR 0006), message history via Graph (ADR 0009), and an +// EphemeralPoster (no clean Teams equivalent) are intentionally NOT implemented +// here; this slice is a Single-Install Adapter with Plain Text and Portable Markdown +// as the portable posting surface. Outbound rate-limit retry (ADR 0005) IS wired, +// reusing the shared internal/ratelimit mechanics, surfaced through the Observer. +package msteams diff --git a/adapters/msteams/export_test.go b/adapters/msteams/export_test.go new file mode 100644 index 0000000..5282d06 --- /dev/null +++ b/adapters/msteams/export_test.go @@ -0,0 +1,20 @@ +package msteams + +import "github.com/coder/chat" + +// EncodeThreadIDForTest builds an opaque conversationReference Thread ID for tests +// in the msteams_test package (posting and round-trip tests). +func EncodeThreadIDForTest(serviceURL, conversationID, tenantID, channelID, conversationType string, isGroup bool) chat.ThreadID { + id, err := encodeThreadID(conversationReference{ + ServiceURL: serviceURL, + ConversationID: conversationID, + TenantID: tenantID, + ChannelID: channelID, + ConversationType: conversationType, + IsGroup: isGroup, + }) + if err != nil { + panic(err) + } + return id +} diff --git a/adapters/msteams/integration_test.go b/adapters/msteams/integration_test.go new file mode 100644 index 0000000..5d84b7a --- /dev/null +++ b/adapters/msteams/integration_test.go @@ -0,0 +1,80 @@ +package msteams_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/coder/chat" + "github.com/coder/chat/adapters/msteams" + "github.com/coder/chat/state/memory" +) + +// TestRuntimeNewMentionDispatchesAndReplies wires the adapter behind the real chat +// runtime (memory state) and proves the whole turn: a validly-signed @mention +// Activity reaches the OnNewMention handler, and the handler's Thread.Post goes out +// as a separate authenticated Connector "send to conversation" call. +func TestRuntimeNewMentionDispatchesAndReplies(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + + var posted map[string]any + var postAuth string + conn := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + postAuth = r.Header.Get("Authorization") + _ = json.NewDecoder(r.Body).Decode(&posted) + _ = json.NewEncoder(w).Encode(map[string]any{"id": "reply-1"}) + })) + t.Cleanup(conn.Close) + tok := fakeTokenServer(t, "out-token", nil) + + adapter := newTestAdapter(t, bf, func(o *msteams.Options) { o.TokenURL = tok.URL }) + bot, err := chat.New(context.Background(), + chat.WithState(memory.New()), + chat.WithAdapter(adapter), + ) + if err != nil { + t.Fatalf("chat.New: %v", err) + } + + var handledThread chat.ThreadID + bot.OnNewMention(func(ctx context.Context, ev *chat.MessageEvent) error { + handledThread = ev.Thread.ID() + _, postErr := ev.Thread.Post(ctx, chat.Markdown("hi back")) + return postErr + }) + + handler, err := bot.Webhook("msteams") + if err != nil { + t.Fatalf("webhook: %v", err) + } + + // Point both the Activity serviceUrl and the token's serviceurl claim at the + // fake Connector so the reply is delivered there. + act := messageActivity() + act["serviceUrl"] = conn.URL + claims := bf.validClaims() + claims["serviceurl"] = conn.URL + + rec := postActivity(t, handler, "Bearer "+bf.sign(t, claims), act) + if rec.Code != http.StatusOK { + t.Fatalf("webhook status = %d, body %s", rec.Code, rec.Body.String()) + } + if handledThread == "" { + t.Fatal("OnNewMention handler did not run") + } + if posted == nil { + t.Fatal("handler reply was not sent to the Connector") + } + if posted["text"] != "hi back" || posted["textFormat"] != "markdown" { + t.Fatalf("posted reply = %#v", posted) + } + if postAuth != "Bearer out-token" { + t.Fatalf("reply auth = %q, want bearer outbound token", postAuth) + } + if conv, _ := posted["conversation"].(map[string]any); conv["id"] != testConvID { + t.Fatalf("reply conversation = %#v", posted["conversation"]) + } +} diff --git a/adapters/msteams/msteams.go b/adapters/msteams/msteams.go new file mode 100644 index 0000000..87c85ad --- /dev/null +++ b/adapters/msteams/msteams.go @@ -0,0 +1,280 @@ +package msteams + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "time" + + "github.com/coder/chat" +) + +const defaultMaxWebhookBodyBytes = 1 << 20 + +// Options configures the Teams adapter. The single-install model mirrors Slack's +// MVP: one Azure Bot registration (MicrosoftAppID + MicrosoftAppPassword) serving +// the deployment. Multi-tenant credential resolution is deferred to ADR 0006. +type Options struct { + // MicrosoftAppID is the bot's Microsoft App (client) ID. It is the required + // inbound JWT audience and the outbound client_credentials client id. + MicrosoftAppID string + // MicrosoftAppPassword is the bot's client secret for outbound token minting. + // (Managed identity is a spike-time alternative, ADR 0007 Open Question 4.) + MicrosoftAppPassword string + // BotID is the bot's Teams ChannelAccount id as it appears in Activity.recipient + // and in a self-authored Activity.from -- conventionally "28:"+MicrosoftAppID, + // which is the default when this is empty. It drives tenant-safe Self Message + // filtering and is the outbound message sender id. The exact self id format is + // spike-required (ADR 0007 Open Question 10). + BotID string + // BotName is an optional display name set on outbound messages. + BotName string + // TenantID is the bot's home Azure AD tenant, used only for BotActor().Tenant. + // Inbound Actors always carry the per-Activity conversation tenant regardless. + TenantID string + + // HTTPClient is injected for all inbound-key, token, and Connector calls; nil + // uses http.DefaultClient. + HTTPClient *http.Client + // OpenIDMetadataURL overrides the Bot Connector OpenID metadata document (for + // tests). Empty uses the production Bot Framework URL. + OpenIDMetadataURL string + // TokenURL overrides the outbound client_credentials endpoint (for tests). Empty + // uses the single-tenant Bot Framework URL. + TokenURL string + // Now injects the clock for token expiry, JWKS cache freshness, and the JWT + // validity window; nil uses time.Now. + Now func() time.Time + // Logger receives structured adapter logs; nil discards. + Logger *slog.Logger + // Observer receives adapter-facing observations (ObsAdapterCall, ObsRateLimit); + // nil is a no-op. It is adapter-owned wiring, not on the core Adapter interface. + Observer chat.Observer + // RetryPolicy bounds outbound Connector rate-limit retry (ADR 0005). The zero + // value applies a conservative default that stays under the Teams turn budget. + RetryPolicy RetryPolicy + // MaxWebhookBodyBytes caps the inbound Activity body; zero applies a 1 MiB + // default. + MaxWebhookBodyBytes int64 +} + +// Adapter is the Microsoft Teams Platform Adapter. It satisfies chat.Adapter and +// owns the Bot Framework boundary (inbound JWT/JWKS validation, outbound token +// minting, Activity normalization, the opaque conversationReference Thread ID, and +// Connector posting); the runtime is unchanged. +type Adapter struct { + appID string + botID string + botName string + tenantID string + client *http.Client + now func() time.Time + logger *slog.Logger + observer chat.Observer + retryPolicy RetryPolicy + maxBody int64 + + validator *authValidator + tokens *tokenSource +} + +var _ chat.Adapter = (*Adapter)(nil) + +// New validates configuration and constructs the adapter. Missing App ID or +// password is a fail-fast Runtime Construction error, consistent with fallible +// adapter construction elsewhere. +func New(ctx context.Context, opts Options) (*Adapter, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + if opts.MicrosoftAppID == "" { + return nil, errors.New("msteams: microsoft app id is required") + } + if opts.MicrosoftAppPassword == "" { + return nil, errors.New("msteams: microsoft app password is required") + } + + client := opts.HTTPClient + if client == nil { + client = http.DefaultClient + } + now := opts.Now + if now == nil { + now = time.Now + } + logger := opts.Logger + if logger == nil { + logger = slog.New(slog.NewTextHandler(io.Discard, nil)) + } + observer := opts.Observer + if observer == nil { + observer = noopObserver{} + } + botID := opts.BotID + if botID == "" { + botID = "28:" + opts.MicrosoftAppID + } + metaURL := opts.OpenIDMetadataURL + if metaURL == "" { + metaURL = openIDMetadataURL + } + tokenURL := opts.TokenURL + if tokenURL == "" { + tokenURL = defaultTokenURL + } + maxBody := opts.MaxWebhookBodyBytes + if maxBody <= 0 { + maxBody = defaultMaxWebhookBodyBytes + } + + return &Adapter{ + appID: opts.MicrosoftAppID, + botID: botID, + botName: opts.BotName, + tenantID: opts.TenantID, + client: client, + now: now, + logger: logger, + observer: observer, + retryPolicy: opts.RetryPolicy.withDefaults(), + maxBody: maxBody, + validator: &authValidator{ + appID: opts.MicrosoftAppID, + openIDMetaURL: metaURL, + issuer: botConnectorIssuer, + client: client, + now: now, + cacheTTL: defaultJWKSCacheTTL, + }, + tokens: &tokenSource{ + appID: opts.MicrosoftAppID, + appSecret: opts.MicrosoftAppPassword, + tokenURL: tokenURL, + scope: connectorScope, + client: client, + now: now, + }, + }, nil +} + +func (a *Adapter) Name() string { return adapterName } + +// Init mints the outbound client_credentials token so invalid Bot Framework +// credentials fail fast before any webhook is served, and warms the token cache. +func (a *Adapter) Init(ctx context.Context) error { + if _, err := a.tokens.get(ctx); err != nil { + return fmt.Errorf("msteams: validate credentials: %w", err) + } + return nil +} + +func (a *Adapter) Shutdown(context.Context) error { return nil } + +// Webhook handles inbound Activities at the bot messaging endpoint. It decodes the +// Activity (a Supported Platform Shape), validates the inbound JWT before any +// normalization or dispatch (there is no path that skips validation), normalizes a +// message Activity into a runtime Event, dispatches synchronously, then acks with +// HTTP 200. The reply, if any, was sent by the handler via Thread.Post as a separate +// authenticated Connector call during dispatch -- there is no webhook-body reply +// shortcut for message activities. Work exceeding the turn budget is the runtime's +// Ack-Then-Work concern (ADR 0002), delivered later via a proactive Connector post. +func (a *Adapter) Webhook(dispatch chat.DispatchFunc) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, a.maxBody)) + if err != nil { + var maxBytesErr *http.MaxBytesError + if errors.As(err, &maxBytesErr) { + http.Error(w, "msteams payload too large", http.StatusRequestEntityTooLarge) + return + } + http.Error(w, "read body", http.StatusBadRequest) + return + } + + var act activity + if err := json.Unmarshal(body, &act); err != nil { + http.Error(w, "invalid msteams activity", http.StatusBadRequest) + return + } + + if err := a.validator.validate(r.Context(), r.Header.Get("Authorization"), act.ServiceURL, act.ChannelID); err != nil { + a.logger.Warn("msteams inbound auth rejected", "error", err) + http.Error(w, "invalid msteams authorization", http.StatusForbidden) + return + } + + event, ok, err := a.normalizeActivity(act) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + if ok { + if err := dispatch(r.Context(), event); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + } + w.WriteHeader(http.StatusOK) + }) +} + +// ValidateThreadID decodes the opaque conversationReference Thread ID into a +// ThreadRef so Thread Handle reconstruction (out-of-webhook and proactive posting) +// works as it does for the other adapters. +func (a *Adapter) ValidateThreadID(id chat.ThreadID) (chat.ThreadRef, error) { + ref, err := decodeThreadID(id) + if err != nil { + return chat.ThreadRef{}, err + } + return chat.ThreadRef{ + ID: id, + Adapter: adapterName, + Tenant: ref.TenantID, + Channel: ref.ConversationID, + Direct: ref.direct(), + Raw: ref, + }, nil +} + +// BotActor is the adapter's own identity for runtime Self Message filtering. The +// Tenant is the configured home tenant (single-install); per-tenant self-filtering +// is additionally enforced in normalizeActivity because one bot can span tenants. +func (a *Adapter) BotActor() chat.Actor { + return chat.Actor{ + Adapter: adapterName, + Tenant: a.tenantID, + ID: a.botID, + BotKind: chat.BotBot, + } +} + +func firstNonEmpty(values ...string) string { + for _, value := range values { + if value != "" { + return value + } + } + return "" +} + +// noopObserver is the default Observer: it records nothing. +type noopObserver struct{} + +func (noopObserver) Event(context.Context, chat.ObservationName, ...chat.Attr) {} + +func (noopObserver) Dispatch(ctx context.Context, _ ...chat.Attr) (context.Context, chat.DispatchSpan) { + return ctx, noopSpan{} +} + +type noopSpan struct{} + +func (noopSpan) End(chat.DispatchOutcome, ...chat.Attr) {} diff --git a/adapters/msteams/normalize_test.go b/adapters/msteams/normalize_test.go new file mode 100644 index 0000000..cce12de --- /dev/null +++ b/adapters/msteams/normalize_test.go @@ -0,0 +1,225 @@ +package msteams_test + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/coder/chat" + "github.com/coder/chat/adapters/msteams" +) + +// dispatchActivity posts a validly-signed Activity through the Webhook and returns +// the dispatched Event (nil if the Activity was an Ignored Event) and the HTTP code. +func dispatchActivity(t *testing.T, a *msteams.Adapter, bf *fakeBotConnector, activity map[string]any) (*chat.Event, int) { + t.Helper() + var got *chat.Event + h := a.Webhook(func(_ context.Context, ev *chat.Event) error { + got = ev + return nil + }) + rec := postActivity(t, h, "Bearer "+bf.sign(t, bf.validClaims()), activity) + return got, rec.Code +} + +func TestNormalizeChannelMention(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + got, code := dispatchActivity(t, a, bf, messageActivity()) + if code != http.StatusOK { + t.Fatalf("status = %d", code) + } + if got == nil { + t.Fatal("event was not dispatched") + } + if got.Adapter != "msteams" || got.Tenant != testTenant || got.ID != "activity-1" { + t.Fatalf("event envelope = %#v", got) + } + if got.DirectMessage { + t.Fatal("channel message marked as direct") + } + if got.Message == nil { + t.Fatal("message is nil") + } + if got.Message.Text != "hello there" { + t.Fatalf("text = %q, want %q (bot mention should be stripped)", got.Message.Text, "hello there") + } + if !got.Message.Mentioned { + t.Fatal("bot mention entity should set Mentioned") + } + wantAuthor := chat.Actor{Adapter: "msteams", Tenant: testTenant, ID: "aad-alice", Name: "Alice", BotKind: chat.BotHuman} + if got.Message.Author != wantAuthor { + t.Fatalf("author = %#v, want %#v", got.Message.Author, wantAuthor) + } + + // The opaque Thread ID round-trips through ValidateThreadID to the conversation. + ref, err := a.ValidateThreadID(got.ThreadID) + if err != nil { + t.Fatalf("validate thread id: %v", err) + } + if ref.Channel != testConvID || ref.Tenant != testTenant || ref.Direct { + t.Fatalf("thread ref = %#v", ref) + } +} + +func TestNormalizeDirectMessage(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + act := messageActivity() + act["text"] = "hello in dm" + act["entities"] = []map[string]any{} + act["conversation"] = map[string]any{ + "id": "19:dm-conv", + "tenantId": testTenant, + "conversationType": "personal", + "isGroup": false, + } + + got, code := dispatchActivity(t, a, bf, act) + if code != http.StatusOK || got == nil { + t.Fatalf("status = %d event = %v", code, got) + } + if !got.DirectMessage { + t.Fatal("personal conversation should be a direct message") + } + if got.Message.Mentioned { + t.Fatal("no mention entity, Mentioned should be false") + } +} + +func TestNormalizeMentionRequiresEntity(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + // A channel message whose text contains the bot's display name but carries no + // mention entity must NOT be treated as a mention. + act := messageActivity() + act["text"] = "hey Bot are you there" + act["entities"] = []map[string]any{} + + got, code := dispatchActivity(t, a, bf, act) + if code != http.StatusOK || got == nil { + t.Fatalf("status = %d event = %v", code, got) + } + if got.Message.Mentioned { + t.Fatal("substring of bot name must not set Mentioned") + } + if got.Message.Text != "hey Bot are you there" { + t.Fatalf("text = %q", got.Message.Text) + } +} + +func TestNormalizeActorFallsBackToFromID(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + act := messageActivity() + act["from"] = map[string]any{"id": "29:user-bbb", "name": "Bob"} // no aadObjectId + + got, code := dispatchActivity(t, a, bf, act) + if code != http.StatusOK || got == nil { + t.Fatalf("status = %d event = %v", code, got) + } + if got.Message.Author.ID != "29:user-bbb" { + t.Fatalf("author id = %q, want fallback to from.id", got.Message.Author.ID) + } +} + +func TestNormalizeMentionStripFallbackAndForeignMention(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + t.Run("bot mention with empty entity text uses leading-tag fallback", func(t *testing.T) { + act := messageActivity() + act["text"] = "Bot do the thing" + act["entities"] = []map[string]any{ + {"type": "mention", "mentioned": map[string]any{"id": testBotID, "name": "Bot"}}, // no text + } + got, code := dispatchActivity(t, a, bf, act) + if code != http.StatusOK || got == nil { + t.Fatalf("status = %d event = %v", code, got) + } + if !got.Message.Mentioned { + t.Fatal("bot should be detected as mentioned via entity") + } + if got.Message.Text != "do the thing" { + t.Fatalf("text = %q, want %q", got.Message.Text, "do the thing") + } + }) + + t.Run("foreign leading mention is not stripped", func(t *testing.T) { + act := messageActivity() + act["text"] = "Other look at this bot" + act["entities"] = []map[string]any{ + {"type": "mention", "text": "Other", "mentioned": map[string]any{"id": "29:other-user", "name": "Other"}}, + } + got, code := dispatchActivity(t, a, bf, act) + if code != http.StatusOK || got == nil { + t.Fatalf("status = %d event = %v", code, got) + } + if got.Message.Mentioned { + t.Fatal("bot is not mentioned; Mentioned should be false") + } + if got.Message.Text != "Other look at this bot" { + t.Fatalf("text = %q, foreign mention must be preserved", got.Message.Text) + } + }) +} + +func TestNormalizeSelfMessageIgnored(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + // The bot's own echo: from.id is the bot id. It must be dropped before dispatch + // (acked with 200, never routed) so the bot cannot loop. + act := messageActivity() + act["from"] = map[string]any{"id": testBotID, "name": "Bot"} + + got, code := dispatchActivity(t, a, bf, act) + if code != http.StatusOK { + t.Fatalf("status = %d, want 200 ack", code) + } + if got != nil { + t.Fatalf("self message reached dispatch: %#v", got) + } +} + +func TestNormalizeNonMessageIgnored(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + + act := messageActivity() + act["type"] = "conversationUpdate" + + got, code := dispatchActivity(t, a, bf, act) + if code != http.StatusOK { + t.Fatalf("status = %d, want 200 ack", code) + } + if got != nil { + t.Fatalf("non-message reached dispatch: %#v", got) + } +} + +func TestWebhookRejectsNonPost(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + a := newTestAdapter(t, bf, nil) + h, _ := recordingHandler(a) + + req := httptest.NewRequest(http.MethodGet, "/api/messages", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("GET status = %d, want 405", rec.Code) + } +} diff --git a/adapters/msteams/retry.go b/adapters/msteams/retry.go new file mode 100644 index 0000000..19ca30e --- /dev/null +++ b/adapters/msteams/retry.go @@ -0,0 +1,172 @@ +package msteams + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "strconv" + "strings" + "time" + + "github.com/coder/chat" + "github.com/coder/chat/internal/ratelimit" +) + +// RetryPolicy bounds outbound Connector rate-limit retry/backoff (ADR 0005). Retry +// is bounded by attempt count, cumulative elapsed backoff, and the caller's context +// deadline, honors a Connector Retry-After, and never sleeps past the deadline so +// in-line synchronous retry stays inside the Teams turn budget (~10-15s) rather than +// blowing it. The zero value applies a conservative default; MaxAttempts: 1 disables +// retry. It is per-adapter platform config, never Runtime Options. +type RetryPolicy struct { + MaxAttempts int + MaxElapsed time.Duration + BaseDelay time.Duration + MaxDelay time.Duration +} + +// withDefaults keeps the worst-case backoff well under the Teams turn budget so a +// reply or proactive post cannot retry its way past the window; a longer Retry-After +// exhausts to a typed RateLimited instead of sleeping past the deadline. +func (p RetryPolicy) withDefaults() RetryPolicy { + out := p + if out.MaxAttempts <= 0 { + out.MaxAttempts = 3 + } + if out.MaxElapsed <= 0 { + out.MaxElapsed = 4 * time.Second + } + if out.BaseDelay <= 0 { + out.BaseDelay = 500 * time.Millisecond + } + if out.MaxDelay <= 0 { + out.MaxDelay = 2 * time.Second + } + return out +} + +// RateLimited is returned when bounded retry is exhausted against a Connector 429, +// or when a single Retry-After would exceed the caller's deadline. It carries the +// adapter name, the last Retry-After, the attempt count, and the raw platform +// response as a Platform Escape Hatch, and is unwrappable to any underlying error. +type RateLimited struct { + Adapter string + RetryAfter time.Duration + Attempts int + Raw any + Err error +} + +func (e *RateLimited) Error() string { + msg := fmt.Sprintf("msteams: rate limited after %d attempts", e.Attempts) + if e.RetryAfter > 0 { + msg += fmt.Sprintf(" (retry after %s)", e.RetryAfter) + } + if e.Err != nil { + msg += ": " + e.Err.Error() + } + return msg +} + +func (e *RateLimited) Unwrap() error { return e.Err } + +// doWithRetry sends the request, retrying only on a Connector HTTP 429 within the +// bounded RetryPolicy and the caller's context deadline. Any other non-2xx is a +// ConnectorError returned immediately (never retried); a 2xx decodes into dest. +// Every attempt emits ObsAdapterCall and every throttle emits ObsRateLimit through +// the configured Observer (ADR 0010); exhaustion is additionally logged. +func (a *Adapter) doWithRetry(ctx context.Context, label string, req *http.Request, dest any) error { + policy := a.retryPolicy + bodyBytes, err := ratelimit.BufferRequestBody(req) + if err != nil { + return err + } + + var elapsed time.Duration + for attempt := 1; ; attempt++ { + a.observer.Event(ctx, chat.ObsAdapterCall, chat.AdapterAttr(adapterName)) + + attemptReq := req + if attempt > 1 { + attemptReq = ratelimit.CloneRequest(req, bodyBytes) + } + status, retryAfterHeader, payload, doErr := a.sendOnce(attemptReq) + if doErr != nil { + return fmt.Errorf("msteams: connector request: %w", doErr) + } + + if status != http.StatusTooManyRequests { + if status < 200 || status > 299 { + return connectorErrorFor(status, payload) + } + if dest == nil || len(payload) == 0 { + return nil + } + if err := json.Unmarshal(payload, dest); err != nil { + return fmt.Errorf("msteams: decode connector response: %w", err) + } + return nil + } + + a.observer.Event(ctx, chat.ObsRateLimit, chat.AdapterAttr(adapterName)) + + retryAfter := parseRetryAfter(retryAfterHeader) + rateLimited := &RateLimited{Adapter: adapterName, RetryAfter: retryAfter, Attempts: attempt, Raw: json.RawMessage(payload)} + if attempt >= policy.MaxAttempts { + a.logRetry(label, attempt, retryAfter, "exhausted") + return rateLimited + } + + delay := ratelimit.BackoffDelay(policy.BaseDelay, policy.MaxDelay, attempt, retryAfter) + if elapsed+delay > policy.MaxElapsed { + a.logRetry(label, attempt, retryAfter, "ceiling") + return rateLimited + } + // The load-bearing invariant: never sleep past the caller's context deadline, + // so retry under synchronous dispatch stays inside the Teams turn budget. + if deadline, ok := ctx.Deadline(); ok && a.now().Add(delay).After(deadline) { + a.logRetry(label, attempt, retryAfter, "deadline") + return rateLimited + } + a.logRetry(label, attempt, retryAfter, "retry") + if err := ratelimit.SleepCtx(ctx, delay); err != nil { + return err + } + elapsed += delay + } +} + +func (a *Adapter) sendOnce(req *http.Request) (int, string, []byte, error) { + resp, err := a.client.Do(req) + if err != nil { + return 0, "", nil, err + } + defer resp.Body.Close() + payload, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return 0, "", nil, err + } + return resp.StatusCode, resp.Header.Get("Retry-After"), payload, nil +} + +func (a *Adapter) logRetry(label string, attempt int, retryAfter time.Duration, outcome string) { + a.logger.Warn("msteams rate limited", + "adapter", adapterName, "endpoint", label, "attempt", attempt, "retry_after", retryAfter, "outcome", outcome) +} + +// parseRetryAfter parses the Connector Retry-After header as an integer number of +// seconds. A missing or unparseable header yields zero, and computed backoff takes +// over. (HTTP-date form is not emitted by the Connector for throttling; left for the +// live spike to confirm.) +func parseRetryAfter(value string) time.Duration { + value = strings.TrimSpace(value) + if value == "" { + return 0 + } + if seconds, err := strconv.ParseFloat(value, 64); err == nil && seconds > 0 { + return time.Duration(seconds * float64(time.Second)) + } + return 0 +} diff --git a/adapters/msteams/testutil_test.go b/adapters/msteams/testutil_test.go new file mode 100644 index 0000000..3a36c7b --- /dev/null +++ b/adapters/msteams/testutil_test.go @@ -0,0 +1,196 @@ +package msteams_test + +import ( + "bytes" + "context" + "crypto" + "crypto/rand" + "crypto/rsa" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "math/big" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/coder/chat/adapters/msteams" +) + +const ( + testAppID = "11111111-1111-1111-1111-111111111111" + testBotID = "28:11111111-1111-1111-1111-111111111111" + testServiceURL = "https://smba.trafficmanager.net/teams/" + testIssuer = "https://api.botframework.com" + testKid = "test-key-1" + testTenant = "tenant-aaa" + testConvID = "19:conversation-xyz" +) + +// testClock is a fixed instant so JWT validity windows and JWKS cache freshness are +// deterministic. +var testClock = time.Unix(1_700_000_000, 0).UTC() + +func fixedNow() func() time.Time { return func() time.Time { return testClock } } + +// fakeBotConnector stands in for the Bot Framework inbound-auth surface: it serves +// the OpenID metadata + JWKS (with endorsements) and signs JWTs with a test RSA key. +type fakeBotConnector struct { + key *rsa.PrivateKey + kid string + endorsements []string + server *httptest.Server + keyRequests int +} + +func newFakeBotConnector(t *testing.T) *fakeBotConnector { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generate rsa key: %v", err) + } + bf := &fakeBotConnector{key: key, kid: testKid, endorsements: []string{"msteams"}} + mux := http.NewServeMux() + mux.HandleFunc("/openid", func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"jwks_uri": bf.server.URL + "/keys"}) + }) + mux.HandleFunc("/keys", func(w http.ResponseWriter, _ *http.Request) { + bf.keyRequests++ + _ = json.NewEncoder(w).Encode(map[string]any{"keys": []map[string]any{bf.jwk()}}) + }) + bf.server = httptest.NewServer(mux) + t.Cleanup(bf.server.Close) + return bf +} + +func (bf *fakeBotConnector) metadataURL() string { return bf.server.URL + "/openid" } + +func (bf *fakeBotConnector) jwk() map[string]any { + return map[string]any{ + "kty": "RSA", + "kid": bf.kid, + "n": base64.RawURLEncoding.EncodeToString(bf.key.PublicKey.N.Bytes()), + "e": base64.RawURLEncoding.EncodeToString(big.NewInt(int64(bf.key.PublicKey.E)).Bytes()), + "endorsements": bf.endorsements, + } +} + +// validClaims returns a claim set that passes every check at testClock; tests mutate +// it before signing. +func (bf *fakeBotConnector) validClaims() map[string]any { + return map[string]any{ + "iss": testIssuer, + "aud": testAppID, + "serviceurl": testServiceURL, + "iat": testClock.Add(-time.Minute).Unix(), + "nbf": testClock.Add(-time.Minute).Unix(), + "exp": testClock.Add(time.Hour).Unix(), + } +} + +func (bf *fakeBotConnector) sign(t *testing.T, claims map[string]any) string { + return signRS256(t, bf.key, bf.kid, claims) +} + +// signForeign signs with a different key but advertises the real kid, producing a +// signature that must fail verification. +func (bf *fakeBotConnector) signForeign(t *testing.T, claims map[string]any) string { + t.Helper() + foreign, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generate foreign key: %v", err) + } + return signRS256(t, foreign, bf.kid, claims) +} + +func signRS256(t *testing.T, key *rsa.PrivateKey, kid string, claims map[string]any) string { + t.Helper() + seg := func(v any) string { + b, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshal jwt segment: %v", err) + } + return base64.RawURLEncoding.EncodeToString(b) + } + signingInput := seg(map[string]any{"alg": "RS256", "typ": "JWT", "kid": kid}) + "." + seg(claims) + digest := sha256.Sum256([]byte(signingInput)) + sig, err := rsa.SignPKCS1v15(rand.Reader, key, crypto.SHA256, digest[:]) + if err != nil { + t.Fatalf("sign jwt: %v", err) + } + return signingInput + "." + base64.RawURLEncoding.EncodeToString(sig) +} + +// messageActivity is a well-formed channel message Activity that @mentions the bot. +func messageActivity() map[string]any { + return map[string]any{ + "type": "message", + "id": "activity-1", + "channelId": "msteams", + "serviceUrl": testServiceURL, + "text": "Bot hello there", + "from": map[string]any{"id": "29:user-aaa", "name": "Alice", "aadObjectId": "aad-alice"}, + "recipient": map[string]any{"id": testBotID, "name": "Bot"}, + "conversation": map[string]any{ + "id": testConvID, + "tenantId": testTenant, + "conversationType": "channel", + "isGroup": true, + }, + "entities": []map[string]any{ + {"type": "mention", "text": "Bot", "mentioned": map[string]any{"id": testBotID, "name": "Bot"}}, + }, + } +} + +func newTestAdapter(t *testing.T, bf *fakeBotConnector, mutate func(*msteams.Options)) *msteams.Adapter { + t.Helper() + opts := msteams.Options{ + MicrosoftAppID: testAppID, + MicrosoftAppPassword: "test-secret", + OpenIDMetadataURL: bf.metadataURL(), + Now: fixedNow(), + } + if mutate != nil { + mutate(&opts) + } + a, err := msteams.New(context.Background(), opts) + if err != nil { + t.Fatalf("msteams.New: %v", err) + } + return a +} + +func postActivity(t *testing.T, handler http.Handler, authHeader string, activity map[string]any) *httptest.ResponseRecorder { + t.Helper() + body, err := json.Marshal(activity) + if err != nil { + t.Fatalf("marshal activity: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/api/messages", bytes.NewReader(body)) + if authHeader != "" { + req.Header.Set("Authorization", authHeader) + } + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + return rec +} + +// fakeTokenServer returns a server that mints the given outbound token, recording +// how many times it was called. +func fakeTokenServer(t *testing.T, token string, calls *int) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if calls != nil { + *calls++ + } + _ = r.ParseForm() + if r.FormValue("grant_type") != "client_credentials" { + t.Errorf("token grant_type = %q", r.FormValue("grant_type")) + } + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": token, "expires_in": 3600}) + })) + t.Cleanup(srv.Close) + return srv +} diff --git a/adapters/msteams/threadid.go b/adapters/msteams/threadid.go new file mode 100644 index 0000000..c3eb70a --- /dev/null +++ b/adapters/msteams/threadid.go @@ -0,0 +1,86 @@ +package msteams + +import ( + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "strings" + + "github.com/coder/chat" +) + +const ( + adapterName = "msteams" + threadIDPrefix = "msteams:v1:" +) + +// conversationReference is the minimal, restart-durable subset of the Bot +// Framework conversationReference needed to post back to a Teams conversation +// later -- either a reply within the turn or a proactive post after it. It is the +// payload serialized into the opaque, adapter-produced Thread ID. +// +// serviceUrl is carried (unlike Slack's human-style channel+ts id) because Teams +// proactive posting cannot be reconstructed from a short id, and Microsoft warns +// serviceUrl can change over time, so the adapter refreshes it from each inbound +// Activity. tenantId is carried so the Thread ID is Platform Tenant scoped and +// cross-tenant collisions are impossible. +type conversationReference struct { + ServiceURL string `json:"service_url"` + ConversationID string `json:"conversation_id"` + TenantID string `json:"tenant_id"` + BotID string `json:"bot_id,omitempty"` + ChannelID string `json:"channel_id,omitempty"` + ConversationType string `json:"conversation_type,omitempty"` + IsGroup bool `json:"is_group,omitempty"` +} + +// direct reports personal (DM) scope. Teams conversationType is the authoritative +// signal ("personal" vs "groupChat"/"channel"); isGroup is the fallback when the +// type is absent. +func (r conversationReference) direct() bool { + if r.ConversationType != "" { + return r.ConversationType == "personal" + } + return !r.IsGroup +} + +// encodeThreadID serializes a conversationReference into a versioned, opaque Thread +// ID. The required fields (serviceUrl, conversation id, tenant id) are exactly what +// later posting needs; missing any of them is a programming error surfaced here. +func encodeThreadID(ref conversationReference) (chat.ThreadID, error) { + if ref.ServiceURL == "" { + return "", errors.New("msteams: thread service url is required") + } + if ref.ConversationID == "" { + return "", errors.New("msteams: thread conversation id is required") + } + if ref.TenantID == "" { + return "", errors.New("msteams: thread tenant id is required") + } + body, err := json.Marshal(ref) + if err != nil { + return "", err + } + return chat.ThreadID(threadIDPrefix + base64.RawURLEncoding.EncodeToString(body)), nil +} + +// decodeThreadID reverses encodeThreadID, rejecting a wrong adapter prefix or any +// reference missing a load-bearing field. +func decodeThreadID(id chat.ThreadID) (conversationReference, error) { + if !strings.HasPrefix(string(id), threadIDPrefix) { + return conversationReference{}, fmt.Errorf("msteams: malformed thread id %q", id) + } + body, err := base64.RawURLEncoding.DecodeString(strings.TrimPrefix(string(id), threadIDPrefix)) + if err != nil { + return conversationReference{}, fmt.Errorf("msteams: decode thread id: %w", err) + } + var ref conversationReference + if err := json.Unmarshal(body, &ref); err != nil { + return conversationReference{}, fmt.Errorf("msteams: parse thread id: %w", err) + } + if ref.ServiceURL == "" || ref.ConversationID == "" || ref.TenantID == "" { + return conversationReference{}, fmt.Errorf("msteams: invalid thread id %q", id) + } + return ref, nil +} diff --git a/adapters/msteams/threadid_test.go b/adapters/msteams/threadid_test.go new file mode 100644 index 0000000..5e935d5 --- /dev/null +++ b/adapters/msteams/threadid_test.go @@ -0,0 +1,59 @@ +package msteams_test + +import ( + "testing" + + "github.com/coder/chat" + "github.com/coder/chat/adapters/msteams" +) + +func TestThreadIDRoundTrip(t *testing.T) { + t.Parallel() + a := newTestAdapter(t, newFakeBotConnector(t), nil) + + id := msteams.EncodeThreadIDForTest(testServiceURL, testConvID, testTenant, "msteams", "channel", true) + ref, err := a.ValidateThreadID(id) + if err != nil { + t.Fatalf("validate: %v", err) + } + if ref.ID != id || ref.Adapter != "msteams" || ref.Tenant != testTenant || ref.Channel != testConvID { + t.Fatalf("ref = %#v", ref) + } + if ref.Direct { + t.Fatal("channel thread should not be direct") + } + if _, ok := ref.Raw.(any); !ok || ref.Raw == nil { + t.Fatal("thread ref should carry the conversation reference as Raw") + } +} + +func TestThreadIDDirectFromPersonalScope(t *testing.T) { + t.Parallel() + a := newTestAdapter(t, newFakeBotConnector(t), nil) + + id := msteams.EncodeThreadIDForTest(testServiceURL, "19:dm", testTenant, "msteams", "personal", false) + ref, err := a.ValidateThreadID(id) + if err != nil { + t.Fatalf("validate: %v", err) + } + if !ref.Direct { + t.Fatal("personal scope should decode as direct") + } +} + +func TestThreadIDRejectsInvalid(t *testing.T) { + t.Parallel() + a := newTestAdapter(t, newFakeBotConnector(t), nil) + + bad := []chat.ThreadID{ + "", + "slack:v1:abc", // wrong adapter prefix + "msteams:v1:!!!not-base64!!!", // undecodable + "msteams:v1:e30", // base64 of "{}" -> reference missing required fields + } + for _, id := range bad { + if _, err := a.ValidateThreadID(id); err == nil { + t.Fatalf("ValidateThreadID(%q) succeeded, want error", id) + } + } +} diff --git a/adapters/msteams/token.go b/adapters/msteams/token.go new file mode 100644 index 0000000..5cc8d3a --- /dev/null +++ b/adapters/msteams/token.go @@ -0,0 +1,109 @@ +package msteams + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "sync" + "time" +) + +const ( + // defaultTokenURL is the Bot Framework client_credentials token endpoint for the + // single-tenant deployment model (ADR 0007 Open Question 4). Multi-tenant and + // managed-identity use different URLs and are out of this slice. + defaultTokenURL = "https://login.microsoftonline.com/botframework.com/oauth2/v2.0/token" + // connectorScope is the OAuth2 scope for outbound Connector calls. + connectorScope = "https://api.botframework.com/.default" + // tokenRefreshMargin re-mints slightly before expiry so an in-flight call never + // uses a just-expired token. + tokenRefreshMargin = 60 * time.Second +) + +// tokenSource mints and caches the outbound client_credentials bearer token used to +// authorize Connector REST calls. The token lives only in adapter process memory +// and is refreshed lazily before expiry; it is never written to Runtime State, +// matching the Linear app-actor token-cache decision (ADR 0001). +type tokenSource struct { + appID string + appSecret string + tokenURL string + scope string + client *http.Client + now func() time.Time + + mu sync.Mutex + token string + expiresAt time.Time +} + +// get returns a cached token when one is still comfortably valid, otherwise mints a +// fresh one. The HTTP mint runs without the lock held. +func (t *tokenSource) get(ctx context.Context) (string, error) { + t.mu.Lock() + if t.token != "" && t.now().Before(t.expiresAt.Add(-tokenRefreshMargin)) { + token := t.token + t.mu.Unlock() + return token, nil + } + t.mu.Unlock() + + token, expiresIn, err := t.mint(ctx) + if err != nil { + return "", err + } + t.mu.Lock() + t.token = token + t.expiresAt = t.now().Add(time.Duration(expiresIn) * time.Second) + t.mu.Unlock() + return token, nil +} + +func (t *tokenSource) mint(ctx context.Context) (string, int, error) { + form := url.Values{} + form.Set("grant_type", "client_credentials") + form.Set("client_id", t.appID) + form.Set("client_secret", t.appSecret) + form.Set("scope", t.scope) + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, t.tokenURL, strings.NewReader(form.Encode())) + if err != nil { + return "", 0, err + } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + resp, err := t.client.Do(req) + if err != nil { + return "", 0, fmt.Errorf("msteams: mint token: %w", err) + } + defer resp.Body.Close() + body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return "", 0, err + } + if resp.StatusCode < 200 || resp.StatusCode > 299 { + return "", 0, fmt.Errorf("msteams: mint token status %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) + } + + var out struct { + AccessToken string `json:"access_token"` + ExpiresIn int `json:"expires_in"` + Error string `json:"error"` + ErrorDesc string `json:"error_description"` + } + if err := json.Unmarshal(body, &out); err != nil { + return "", 0, fmt.Errorf("msteams: decode token response: %w", err) + } + if out.AccessToken == "" { + return "", 0, fmt.Errorf("msteams: token response missing access_token (error %q: %s)", out.Error, out.ErrorDesc) + } + expiresIn := out.ExpiresIn + if expiresIn <= 0 { + expiresIn = 3600 + } + return out.AccessToken, expiresIn, nil +} diff --git a/adapters/msteams/token_test.go b/adapters/msteams/token_test.go new file mode 100644 index 0000000..b0a99a2 --- /dev/null +++ b/adapters/msteams/token_test.go @@ -0,0 +1,67 @@ +package msteams_test + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/coder/chat/adapters/msteams" +) + +func TestNewRequiresAppCredentials(t *testing.T) { + t.Parallel() + if _, err := msteams.New(context.Background(), msteams.Options{MicrosoftAppPassword: "secret"}); err == nil { + t.Fatal("missing app id should be a construction error") + } + if _, err := msteams.New(context.Background(), msteams.Options{MicrosoftAppID: "app"}); err == nil { + t.Fatal("missing app password should be a construction error") + } +} + +func TestInitMintsAndCachesOutboundToken(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + now := testClock + calls := 0 + srv := fakeTokenServer(t, "tok-1", &calls) + a := newTestAdapter(t, bf, func(o *msteams.Options) { + o.Now = func() time.Time { return now } + o.TokenURL = srv.URL + }) + + if err := a.Init(context.Background()); err != nil { + t.Fatalf("init: %v", err) + } + if err := a.Init(context.Background()); err != nil { + t.Fatalf("init (cached): %v", err) + } + if calls != 1 { + t.Fatalf("token mints = %d, want 1 (cached on the second Init)", calls) + } + + // Advance past expiry (expires_in 3600s) so the next use re-mints lazily. + now = testClock.Add(2 * time.Hour) + if err := a.Init(context.Background()); err != nil { + t.Fatalf("init (refresh): %v", err) + } + if calls != 2 { + t.Fatalf("token mints after expiry = %d, want 2", calls) + } +} + +func TestInitFailsOnBadCredentials(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"invalid_client","error_description":"bad secret"}`)) + })) + t.Cleanup(srv.Close) + a := newTestAdapter(t, bf, func(o *msteams.Options) { o.TokenURL = srv.URL }) + + if err := a.Init(context.Background()); err == nil { + t.Fatal("Init should fail fast on invalid credentials") + } +} diff --git a/docs/adr/0007-teams-adapter.md b/docs/adr/0007-teams-adapter.md index c0055a1..4b07f86 100644 --- a/docs/adr/0007-teams-adapter.md +++ b/docs/adr/0007-teams-adapter.md @@ -2,7 +2,8 @@ ## Status -Proposed +Proposed — spike implemented (see Spike Findings); pending live validation against a +real Azure Bot resource and Teams tenant before Accepted. ## Context @@ -120,3 +121,45 @@ Rejected for this slice. The Slack precedent is a **Single-Install Adapter**, an ### Implement now and verify behavior during implementation Rejected. The inbound ack/turn contract, endorsement enforcement, Markdown fidelity, `serviceUrl`/`conversation.id` persistence stability, proactive-install prerequisites, and the `msbotbuilder-go` auth implementation are documentation-only or unverified. Committing code before a spike confirms them risks building on wrong assumptions about the `msteams` channel. The decision is to design now and gate implementation on the spike. + +## Spike Findings + +A code spike of the adapter now exists under `adapters/msteams` (behind a draft PR for +live validation). It implements the full designed shape and is exercised end to end +against fake Bot Framework servers (OpenID metadata + JWKS, the `client_credentials` +token endpoint, and the Connector), including a runtime integration test that drives an +`@mention` through `chat.New` and asserts the reply is delivered as a separate Connector +call. What the spike **resolved**: + +- **The shape holds unchanged.** The small **Adapter** interface, opaque + adapter-produced **Thread ID** (a versioned `conversationReference`), **Platform Escape + Hatch**, and direct-HTTP pattern absorbed Teams with no change to the runtime or core + types. +- **Open Question 9 (SDK).** `msbotbuilder-go` is not adopted. Further: the inbound + JWT/JWKS validation is implemented with the **standard library only** (`crypto/rsa` + over a public key rebuilt from the JWK `n`/`e`), so the otherwise zero-dependency core + module gains no JWT library at all — `golang-jwt`/`jwx` proved unnecessary. This is a + deliberate deviation from the ADR's "use a maintained `golang-jwt`" note, chosen to + preserve the repo's zero-dependency, stdlib-direct stance (Slack/Linear precedent). +- Every mandatory inbound check is enforced with no disable switch (Bearer, RS256-only, + `kid`, signature, `iss`, `aud == App ID`, exp/nbf with 5-minute skew, `serviceurl` + claim bound to `Activity.serviceUrl`), plus JWKS cache (≥24h) with refresh on `kid` + miss / rotation, and the strict (fail-closed) channel-endorsement check. + +What still requires **live validation** before this ADR is Accepted (the original Open +Questions, now the test plan for the human spike) — each is marked `spike-required` inline +in the code: + +1. Exact `msteams` inbound ack semantics and the real turn timeout. +2. Confirm every reply is a separate Connector REST call (no body-reply shortcut for + `message` activities). +3. The exact channel-endorsement rule (the spike fails closed when `msteams` is absent; + confirm this does not reject valid production traffic). +4. Single-tenant Azure Bot resource specifics (token URL, `aud`/`iss`). +5. Teams Markdown subset fidelity under `textFormat = markdown`. +6. `serviceUrl` / `conversation.id` persistence stability for proactive posting. +7. Proactive-posting prerequisites (inbound-first vs Graph install). +8. RSC mention behavior (`OnNewMention` only on an explicit bot Mention entity). +9. Canonical `Actor.ID` key (`from.aadObjectId` vs `from.id`; the spike prefers + `aadObjectId`, falling back to `from.id`). +10. `Activity.id` stability as the dedupe key across Connector redelivery. From c907dafcf912e9fc540d8bc9d558c5479318068b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 4 Jun 2026 10:58:08 +0200 Subject: [PATCH 2/3] Address self-review of the Teams spike (security + correctness) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Max-effort code review (9 finder angles + verify + sweep). Applied fixes; refuted/out-of-scope items noted in the PR. Security / correctness: - Endorsement bypass: checkEndorsement was fed the Activity-body channelId and short-circuited on "", so an attacker holding a token whose signing key did not endorse msteams could skip the gate by omitting channelId. Now bound to the adapter's own channel constant, never the body. (+test) - JWT with no exp was accepted as never-expiring; now exp is mandatory. (+test) - Cold-cache JWKS/metadata outage returned 403 (which the Connector treats as permanent, dropping a valid Activity); a transient key-fetch failure now returns a retryable 503 via a typed errKeysUnavailable sentinel. (+test) - RSA public exponent was built via big.Int.Int64() with no bound, risking silent truncation (32-bit int); now range-checked. - PostMessage trusted ThreadRef.Raw via an unchecked type assertion; now always decodes the authoritative opaque Thread ID, so a mismatched Raw cannot misroute a reply. - Webhook no longer leaks internal error strings on a normalize failure (generic 400 + structured log), matching the Slack adapter. Correctness/clarity: - stripBotMention strips only the first mention occurrence (was ReplaceAll), so a token the user legitimately repeats is preserved; mention scan computed once. - Deduplicated the DirectMessage rule via conversationReference.direct(). - Corrected BotActor()/normalizeActivity comments: the adapter-internal self-drop is authoritative (the runtime's tenant-scoped isSelfActor cannot fire for a cross-tenant Teams bot) and depends on BotID being correct (spike-required). - Removed a dead Adapter.appID field. Refuted (no change): the serviceUrl SSRF/token-exfil concern — the serviceurl claim is verified against the signed token before use, so it cannot be steered to an arbitrary host. Noted out of scope: factoring the retry loop / noopObserver / firstNonEmpty / thread-id codec into shared helpers (touches Slack/Linear), and single-flight on the JWKS/token caches. Change-Id: I3be83810514260873154700135d1783351227105 Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Thomas Kosiewski --- adapters/msteams/activity.go | 41 +++++++++++++------------ adapters/msteams/auth.go | 56 +++++++++++++++++++++++++++-------- adapters/msteams/auth_test.go | 49 ++++++++++++++++++++++++++++++ adapters/msteams/connector.go | 9 +++--- adapters/msteams/msteams.go | 26 +++++++++++----- 5 files changed, 137 insertions(+), 44 deletions(-) diff --git a/adapters/msteams/activity.go b/adapters/msteams/activity.go index a2203b0..2c0769d 100644 --- a/adapters/msteams/activity.go +++ b/adapters/msteams/activity.go @@ -59,11 +59,12 @@ type entity struct { // ok=false for the Ignored Events of this slice: any non-message activity type, and // the bot's own messages. // -// Self-filtering is done here (not left solely to the runtime's BotActor() match) -// because a single-install Teams bot can receive activity from more than one -// Platform Tenant, so the runtime's tenant-scoped isSelfActor cannot be relied on -// to catch the bot's own echo -- the same reason the Slack multi-tenant path drops -// self messages in the adapter. +// Self-filtering is authoritative HERE, not at the runtime's BotActor()/isSelfActor +// match: a single-install Teams bot can receive activity from more than one Platform +// Tenant, so the runtime's tenant-scoped isSelfActor cannot catch the bot's own echo +// (its Tenant is the home tenant, the echo carries the conversation tenant). This is +// the same reason the Slack multi-tenant path drops self messages in the adapter. It +// depends on a.botID being the bot's true self id (spike-required, Open Question 10). func (a *Adapter) normalizeActivity(act activity) (*chat.Event, bool, error) { if !strings.EqualFold(act.Type, "message") { return nil, false, nil @@ -91,7 +92,7 @@ func (a *Adapter) normalizeActivity(act activity) (*chat.Event, bool, error) { return nil, false, nil } - threadID, err := encodeThreadID(conversationReference{ + ref := conversationReference{ ServiceURL: act.ServiceURL, ConversationID: act.Conversation.ID, TenantID: tenantID, @@ -99,26 +100,25 @@ func (a *Adapter) normalizeActivity(act activity) (*chat.Event, bool, error) { ChannelID: act.ChannelID, ConversationType: act.Conversation.ConversationType, IsGroup: act.Conversation.IsGroup, - }) + } + threadID, err := encodeThreadID(ref) if err != nil { return nil, false, err } - direct := act.Conversation.ConversationType == "personal" || - (act.Conversation.ConversationType == "" && !act.Conversation.IsGroup) - + mentioned := botMentioned(act) return &chat.Event{ ID: act.ID, Adapter: adapterName, Tenant: tenantID, ThreadID: threadID, - DirectMessage: direct, + DirectMessage: ref.direct(), Raw: act.Raw, Message: &chat.Message{ ID: act.ID, - Text: stripBotMention(act), + Text: stripBotMention(act, mentioned), Author: author, - Mentioned: botMentioned(act), + Mentioned: mentioned, Raw: act.Raw, }, }, true, nil @@ -164,20 +164,23 @@ func botMentioned(act activity) bool { // stripBotMention removes the bot mention from the message text so handlers see the // user's actual words. Teams embeds the mention as "Bot" both in the text // and as an entity carrying that exact substring; the entity text is the reliable -// thing to strip. The leading- fallback applies only when the bot is mentioned -// but no entity carried text, so it never strips another user's leading mention. -// Markdown subset fidelity beyond this is spike-required (ADR 0007 Open Question 5). -func stripBotMention(act activity) string { +// thing to strip. Only the first occurrence is removed, so a token the user +// legitimately repeats later in the message is preserved. The leading- fallback +// applies only when the bot is mentioned but no entity carried text, so it never +// strips another user's leading mention. The caller passes the already-computed +// mentioned flag to avoid re-scanning the entities. Markdown subset fidelity beyond +// this is spike-required (ADR 0007 Open Question 5). +func stripBotMention(act activity, mentioned bool) string { text := act.Text botID := act.Recipient.ID stripped := false for _, e := range act.Entities { if strings.EqualFold(e.Type, "mention") && e.Mentioned.ID == botID && e.Text != "" { - text = strings.ReplaceAll(text, e.Text, "") + text = strings.Replace(text, e.Text, "", 1) stripped = true } } - if !stripped && botMentioned(act) { + if !stripped && mentioned { text = stripLeadingAtTag(text) } return strings.TrimSpace(text) diff --git a/adapters/msteams/auth.go b/adapters/msteams/auth.go index 52997cb..da9ec5d 100644 --- a/adapters/msteams/auth.go +++ b/adapters/msteams/auth.go @@ -28,8 +28,19 @@ const ( defaultJWKSCacheTTL = 24 * time.Hour // clockSkew is the tolerated exp/nbf skew, per Microsoft guidance. clockSkew = 5 * time.Minute + // msteamsChannel is the Bot Framework channel id this adapter serves. The + // endorsement check is bound to this constant, NOT to the channelId in the + // (unauthenticated) Activity body, so the body cannot weaken or skip it. + msteamsChannel = "msteams" ) +// errKeysUnavailable marks an inbound failure caused by the adapter being unable to +// fetch signing keys (metadata/JWKS transport failure with no usable cached key), +// as opposed to a token that is genuinely invalid. The caller maps it to a +// retryable 5xx so the Bot Connector redelivers, rather than a 403 that the +// Connector treats as a permanent rejection and drops. +var errKeysUnavailable = errors.New("msteams: signing keys temporarily unavailable") + // authValidator performs every mandatory inbound Bot Connector JWT check against a // JWKS resolved from the Bot Framework OpenID metadata, plus the Bot Framework // channel-endorsement check. It is a deep adapter-internal module behind a single @@ -84,7 +95,7 @@ type jwtPayload struct { // passes; any failure is an error the caller maps to HTTP 403. The serviceUrl claim // is bound to the Activity's serviceUrl so a token minted for one service URL // cannot be replayed against a spoofed one. -func (v *authValidator) validate(ctx context.Context, authzHeader, serviceURL, channelID string) error { +func (v *authValidator) validate(ctx context.Context, authzHeader, serviceURL string) error { raw, err := bearerToken(authzHeader) if err != nil { return err @@ -114,6 +125,11 @@ func (v *authValidator) validate(ctx context.Context, authzHeader, serviceURL, c if !audienceMatches(payload.Aud, v.appID) { return errors.New("msteams: jwt audience does not match bot app id") } + // exp is mandatory: a token without it must fail closed, never be treated as + // never-expiring. + if payload.Exp == 0 { + return errors.New("msteams: jwt missing exp claim") + } if !validAt(payload, v.now(), clockSkew) { return errors.New("msteams: jwt outside validity window") } @@ -123,7 +139,10 @@ func (v *authValidator) validate(ctx context.Context, authzHeader, serviceURL, c if !serviceURLMatches(payload.ServiceURL, serviceURL) { return errors.New("msteams: jwt serviceurl claim does not match activity") } - if err := checkEndorsement(key, channelID); err != nil { + // Endorsement is checked against this adapter's own channel constant, never the + // channelId in the request body, so a caller cannot skip it by omitting/spoofing + // channelId. + if err := checkEndorsement(key, msteamsChannel); err != nil { return err } return nil @@ -146,7 +165,10 @@ func (v *authValidator) keyForKid(ctx context.Context, kid string) (jwk, error) if ok { return key, nil } - return jwk{}, err + // No cached key and the metadata/JWKS fetch failed: this is an adapter-side + // transient failure, not an invalid token. Mark it so the caller returns a + // retryable 5xx instead of permanently dropping a possibly-valid Activity. + return jwk{}, fmt.Errorf("%w: %v", errKeysUnavailable, err) } v.mu.Lock() @@ -236,16 +258,14 @@ func (v *authValidator) getJSON(ctx context.Context, url string, dest any) error return json.Unmarshal(body, dest) } -// checkEndorsement enforces that an msteams Activity's signing key endorses the -// msteams channel: the matched key's endorsements must contain the channel id, or -// the request is rejected (HTTP 403 at the caller). This is the strict reading of -// ADR 0007; whether msteams strictly requires the endorsement, and the exact rule, -// is spike-required (Open Question 3). Failing closed is the safer default for a -// security check a human will validate before production. +// checkEndorsement enforces that the signing key endorses the required channel: the +// key's endorsements must contain it, or the request is rejected (HTTP 403 at the +// caller). Callers pass the adapter's own channel constant, never an Activity-body +// value, so an empty/spoofed body channelId cannot bypass the check. This is the +// strict reading of ADR 0007; whether msteams strictly requires the endorsement, and +// the exact rule, is spike-required (Open Question 3). Failing closed is the safer +// default for a security check a human will validate before production. func checkEndorsement(key jwk, channelID string) error { - if channelID == "" { - return nil - } for _, e := range key.Endorsements { if strings.EqualFold(e, channelID) { return nil @@ -321,9 +341,19 @@ func rsaPublicKeyFromJWK(k jwk) (*rsa.PublicKey, error) { if len(nBytes) == 0 || len(eBytes) == 0 { return nil, errors.New("msteams: jwk modulus or exponent empty") } + // Guard the exponent against silent truncation by int(...) (it is consumed as a + // platform int, 32-bit on some builds): reject anything that does not fit a + // positive 32-bit int or is implausibly small. + e := new(big.Int).SetBytes(eBytes) + if !e.IsInt64() { + return nil, errors.New("msteams: jwk exponent too large") + } + if ev := e.Int64(); ev < 2 || ev > 1<<31-1 { + return nil, fmt.Errorf("msteams: jwk exponent %d out of range", ev) + } return &rsa.PublicKey{ N: new(big.Int).SetBytes(nBytes), - E: int(new(big.Int).SetBytes(eBytes).Int64()), + E: int(e.Int64()), }, nil } diff --git a/adapters/msteams/auth_test.go b/adapters/msteams/auth_test.go index 1952038..28ff8ab 100644 --- a/adapters/msteams/auth_test.go +++ b/adapters/msteams/auth_test.go @@ -3,10 +3,12 @@ package msteams_test import ( "context" "net/http" + "net/http/httptest" "testing" "time" "github.com/coder/chat" + "github.com/coder/chat/adapters/msteams" ) // recordingHandler returns a Webhook handler plus a pointer that is set when the @@ -103,6 +105,15 @@ func TestWebhookInboundAuthMatrix(t *testing.T) { }, wantStatus: http.StatusForbidden, }, + { + name: "missing exp rejected", + auth: func() string { + c := bf.validClaims() + delete(c, "exp") + return "Bearer " + bf.sign(t, c) + }, + wantStatus: http.StatusForbidden, + }, { name: "expired within skew accepted", auth: func() string { @@ -172,6 +183,44 @@ func TestWebhookChannelEndorsement(t *testing.T) { t.Fatalf("status = %d dispatched = %v, want 403 no dispatch", rec.Code, *dispatched) } }) + + // The endorsement gate must bind to the adapter's own channel, not the + // Activity-body channelId, so an attacker holding a token signed by a key that + // does NOT endorse msteams cannot skip the check by omitting channelId. + t.Run("empty body channelId does not bypass endorsement", func(t *testing.T) { + bf := newFakeBotConnector(t) + bf.endorsements = []string{"directline"} // signing key does NOT endorse msteams + a := newTestAdapter(t, bf, nil) + h, dispatched := recordingHandler(a) + act := messageActivity() + act["channelId"] = "" // attempt to short-circuit the endorsement check + rec := postActivity(t, h, "Bearer "+bf.sign(t, bf.validClaims()), act) + if rec.Code != http.StatusForbidden || *dispatched { + t.Fatalf("status = %d dispatched = %v, want 403 no dispatch (endorsement must not be bypassable)", rec.Code, *dispatched) + } + }) +} + +// TestWebhookKeysUnavailableReturns503 proves that a transient inability to fetch +// signing keys (metadata/JWKS unreachable, cold cache) returns a retryable 5xx, not a +// 403 the Connector would treat as permanent and drop a possibly-valid Activity. +func TestWebhookKeysUnavailableReturns503(t *testing.T) { + t.Parallel() + bf := newFakeBotConnector(t) // used only to sign the token + + dead := httptest.NewServer(http.NotFoundHandler()) + deadURL := dead.URL + dead.Close() // metadata endpoint is now unreachable + + a := newTestAdapter(t, bf, func(o *msteams.Options) { o.OpenIDMetadataURL = deadURL }) + h, dispatched := recordingHandler(a) + rec := postActivity(t, h, "Bearer "+bf.sign(t, bf.validClaims()), messageActivity()) + if rec.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want 503 (transient key-fetch failure must be retryable)", rec.Code) + } + if *dispatched { + t.Fatal("must not dispatch when signing keys are unavailable") + } } // TestWebhookValidationAlwaysOn proves there is no construction path that turns diff --git a/adapters/msteams/connector.go b/adapters/msteams/connector.go index d87147e..55e8771 100644 --- a/adapters/msteams/connector.go +++ b/adapters/msteams/connector.go @@ -79,12 +79,11 @@ func (a *Adapter) PostMessage(ctx context.Context, thread chat.ThreadRef, msg ch return &chat.SentMessage{ID: resp.ID, ThreadID: thread.ID, Raw: resp}, nil } -// connRefForThread reuses the conversationReference already attached to a verified -// ThreadRef.Raw and otherwise decodes the opaque Thread ID. +// connRefForThread decodes the conversationReference from the opaque Thread ID, the +// authoritative source. It deliberately does NOT trust ThreadRef.Raw: a caller-built +// ref whose Raw disagreed with its ID would otherwise post to the wrong conversation +// while reporting the ID's conversation. func connRefForThread(thread chat.ThreadRef) (conversationReference, error) { - if ref, ok := thread.Raw.(conversationReference); ok { - return ref, nil - } return decodeThreadID(thread.ID) } diff --git a/adapters/msteams/msteams.go b/adapters/msteams/msteams.go index 87c85ad..6a91cf3 100644 --- a/adapters/msteams/msteams.go +++ b/adapters/msteams/msteams.go @@ -67,7 +67,6 @@ type Options struct { // minting, Activity normalization, the opaque conversationReference Thread ID, and // Connector posting); the runtime is unchanged. type Adapter struct { - appID string botID string botName string tenantID string @@ -132,7 +131,6 @@ func New(ctx context.Context, opts Options) (*Adapter, error) { } return &Adapter{ - appID: opts.MicrosoftAppID, botID: botID, botName: opts.BotName, tenantID: opts.TenantID, @@ -206,15 +204,24 @@ func (a *Adapter) Webhook(dispatch chat.DispatchFunc) http.Handler { return } - if err := a.validator.validate(r.Context(), r.Header.Get("Authorization"), act.ServiceURL, act.ChannelID); err != nil { + if err := a.validator.validate(r.Context(), r.Header.Get("Authorization"), act.ServiceURL); err != nil { a.logger.Warn("msteams inbound auth rejected", "error", err) + // A transient inability to fetch signing keys is the adapter's failure, + // not a bad token: return a retryable 5xx so the Connector redelivers, + // rather than a 403 it treats as permanent (silently dropping a valid + // Activity). + if errors.Is(err, errKeysUnavailable) { + http.Error(w, "msteams signing keys unavailable", http.StatusServiceUnavailable) + return + } http.Error(w, "invalid msteams authorization", http.StatusForbidden) return } event, ok, err := a.normalizeActivity(act) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + a.logger.Warn("msteams normalize failed", "error", err) + http.Error(w, "invalid msteams activity", http.StatusBadRequest) return } if ok { @@ -245,9 +252,14 @@ func (a *Adapter) ValidateThreadID(id chat.ThreadID) (chat.ThreadRef, error) { }, nil } -// BotActor is the adapter's own identity for runtime Self Message filtering. The -// Tenant is the configured home tenant (single-install); per-tenant self-filtering -// is additionally enforced in normalizeActivity because one bot can span tenants. +// BotActor is the adapter's own identity, exposed for the chat.Adapter contract and +// Thread Handle use. Note: for Teams, Self Message filtering is done authoritatively +// in normalizeActivity (it drops the bot's own echo before dispatch), so the +// runtime's BotActor()/isSelfActor match does not independently fire for Teams +// messages — its Tenant is the configured home tenant while inbound actors carry the +// per-Activity conversation tenant. That self-drop is load-bearing and depends on +// BotID being correct (spike-required, ADR 0007 Open Question 10); set Options.BotID +// if the real Teams bot id is not "28:"+MicrosoftAppID. func (a *Adapter) BotActor() chat.Actor { return chat.Actor{ Adapter: adapterName, From 9f9e24c5fbec723f6cc05a7b431ada8c9479a233 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 4 Jun 2026 11:10:53 +0200 Subject: [PATCH 3/3] Deslop: trim the Teams adapter to house comment density The spike's unexported helpers carried multi-line doc essays where the sibling Slack/Linear adapters leave theirs comment-free. Trimmed those (activity.go normalize/actor/mention/strip, the conversationReference and authValidator type docs, checkEndorsement, the Webhook and PostMessage docs) to 1-3 lines, keeping the non-obvious security/design rationale and the terse spike-required markers. Inlined connRefForThread (a one-line wrapper post-review). Comment-only changes plus the inline; no behavior change, tests unchanged and green. Change-Id: Icfb9c549a5b70b460e7c533ca7ab0462386ac572 Signed-off-by: Thomas Kosiewski --- adapters/msteams/activity.go | 46 ++++++++++++----------------------- adapters/msteams/auth.go | 27 ++++++++------------ adapters/msteams/connector.go | 22 +++++------------ adapters/msteams/msteams.go | 13 ++++------ adapters/msteams/threadid.go | 14 +++-------- 5 files changed, 40 insertions(+), 82 deletions(-) diff --git a/adapters/msteams/activity.go b/adapters/msteams/activity.go index 2c0769d..f765059 100644 --- a/adapters/msteams/activity.go +++ b/adapters/msteams/activity.go @@ -8,10 +8,8 @@ import ( "github.com/coder/chat" ) -// activity is the Supported Platform Shape of a Bot Framework Activity: only the -// fields this slice reads. Decoding is permissive -- unrelated Bot Framework fields -// are tolerated -- and the full raw JSON is preserved as the Platform Escape Hatch -// on both Event.Raw and Message.Raw. +// activity is the supported subset of a Bot Framework Activity; decoding is +// permissive and the full raw JSON is preserved as the Platform Escape Hatch. type activity struct { Type string `json:"type"` ID string `json:"id"` @@ -55,16 +53,11 @@ type entity struct { Mentioned channelAccount `json:"mentioned"` } -// normalizeActivity converts a decoded Activity into a runtime Event. It returns -// ok=false for the Ignored Events of this slice: any non-message activity type, and -// the bot's own messages. -// -// Self-filtering is authoritative HERE, not at the runtime's BotActor()/isSelfActor -// match: a single-install Teams bot can receive activity from more than one Platform -// Tenant, so the runtime's tenant-scoped isSelfActor cannot catch the bot's own echo -// (its Tenant is the home tenant, the echo carries the conversation tenant). This is -// the same reason the Slack multi-tenant path drops self messages in the adapter. It -// depends on a.botID being the bot's true self id (spike-required, Open Question 10). +// normalizeActivity converts a message Activity into an Event, returning ok=false +// for Ignored Events (non-message types and the bot's own messages). The self-drop +// here is the authoritative Self Message filter: a single-install bot can span +// tenants, so the runtime's tenant-scoped isSelfActor cannot catch its echo. It +// relies on a.botID being the bot's true id (spike-required, Open Question 10). func (a *Adapter) normalizeActivity(act activity) (*chat.Event, bool, error) { if !strings.EqualFold(act.Type, "message") { return nil, false, nil @@ -124,11 +117,9 @@ func (a *Adapter) normalizeActivity(act activity) (*chat.Event, bool, error) { }, true, nil } -// actorForActivity maps the inbound author. The canonical Actor.ID prefers -// from.aadObjectId (tenant-stable across conversations) and falls back to from.id; -// a self-authored message (from.id == the configured bot id) is tagged BotBot with -// from.id so it matches BotActor() for Self Message filtering. The exact canonical -// key is spike-required (ADR 0007 Open Question 10). +// actorForActivity maps the inbound author: Actor.ID prefers the tenant-stable +// from.aadObjectId, except a self-authored message keeps from.id so it matches +// a.botID for self-filtering (canonical key spike-required, Open Question 10). func (a *Adapter) actorForActivity(act activity) chat.Actor { id := firstNonEmpty(act.From.AADObjectID, act.From.ID) kind := chat.BotHuman @@ -145,9 +136,8 @@ func (a *Adapter) actorForActivity(act activity) chat.Actor { } } -// botMentioned reports whether the bot is @mentioned, derived from the Activity's -// mention entities (mentioned.id == the bot's recipient id), never from substring -// matching on display-name text. The inbound recipient is, by definition, the bot. +// botMentioned reports whether the bot is @mentioned, from the Activity's mention +// entities (the inbound recipient is the bot), never from substring text matching. func botMentioned(act activity) bool { botID := act.Recipient.ID if botID == "" { @@ -161,15 +151,9 @@ func botMentioned(act activity) bool { return false } -// stripBotMention removes the bot mention from the message text so handlers see the -// user's actual words. Teams embeds the mention as "Bot" both in the text -// and as an entity carrying that exact substring; the entity text is the reliable -// thing to strip. Only the first occurrence is removed, so a token the user -// legitimately repeats later in the message is preserved. The leading- fallback -// applies only when the bot is mentioned but no entity carried text, so it never -// strips another user's leading mention. The caller passes the already-computed -// mentioned flag to avoid re-scanning the entities. Markdown subset fidelity beyond -// this is spike-required (ADR 0007 Open Question 5). +// stripBotMention removes the bot's mention so handlers see the user's words: it +// deletes the mention entity's exact text (first occurrence only), falling back to a +// leading ... only when the bot is mentioned but the entity carried no text. func stripBotMention(act activity, mentioned bool) string { text := act.Text botID := act.Recipient.ID diff --git a/adapters/msteams/auth.go b/adapters/msteams/auth.go index da9ec5d..165b395 100644 --- a/adapters/msteams/auth.go +++ b/adapters/msteams/auth.go @@ -41,16 +41,12 @@ const ( // Connector treats as a permanent rejection and drops. var errKeysUnavailable = errors.New("msteams: signing keys temporarily unavailable") -// authValidator performs every mandatory inbound Bot Connector JWT check against a -// JWKS resolved from the Bot Framework OpenID metadata, plus the Bot Framework -// channel-endorsement check. It is a deep adapter-internal module behind a single -// validate method; there is deliberately no switch to disable any check. It is -// testable with a fake metadata+JWKS server and in-test RSA keys. -// -// JWT parsing and RS256 verification are stdlib-only (crypto/rsa over a public key -// rebuilt from the JWK n/e), so the otherwise zero-dependency core module gains no -// JWT library. This is a deliberate spike finding for ADR 0007 Open Question 9 -// (msbotbuilder-go is not adopted, and even golang-jwt is unnecessary here). +// authValidator performs every mandatory inbound Bot Connector JWT check against the +// Bot Framework JWKS, plus the channel-endorsement check; no switch disables any +// check. JWT parsing and RS256 verification are stdlib-only (crypto/rsa over a key +// rebuilt from the JWK n/e), so the zero-dependency core gains no JWT library -- a +// deliberate spike finding for Open Question 9 (msbotbuilder-go not adopted, +// golang-jwt unnecessary). type authValidator struct { appID string openIDMetaURL string @@ -258,13 +254,10 @@ func (v *authValidator) getJSON(ctx context.Context, url string, dest any) error return json.Unmarshal(body, dest) } -// checkEndorsement enforces that the signing key endorses the required channel: the -// key's endorsements must contain it, or the request is rejected (HTTP 403 at the -// caller). Callers pass the adapter's own channel constant, never an Activity-body -// value, so an empty/spoofed body channelId cannot bypass the check. This is the -// strict reading of ADR 0007; whether msteams strictly requires the endorsement, and -// the exact rule, is spike-required (Open Question 3). Failing closed is the safer -// default for a security check a human will validate before production. +// checkEndorsement enforces that the signing key endorses the required channel. +// Callers pass the adapter's own channel constant, never an Activity-body value, so +// a spoofed/empty channelId cannot bypass it. It fails closed; the exact rule is +// spike-required (Open Question 3). func checkEndorsement(key jwk, channelID string) error { for _, e := range key.Endorsements { if strings.EqualFold(e, channelID) { diff --git a/adapters/msteams/connector.go b/adapters/msteams/connector.go index 55e8771..dcfb433 100644 --- a/adapters/msteams/connector.go +++ b/adapters/msteams/connector.go @@ -46,15 +46,13 @@ func (e *ConnectorError) Error() string { func (e *ConnectorError) Unwrap() error { return e.Err } -// PostMessage maps a Thread.Post to the Connector "send to conversation" REST call -// (POST {serviceUrl}/v3/conversations/{conversationId}/activities) using the -// serviceUrl from the opaque Thread ID as the base URI. Plain Text renders with -// textFormat=plain and Portable Markdown with textFormat=markdown; the Sent Message -// id is the ResourceResponse id. Within a turn this is the bot's reply; out of band -// it is a proactive post -- the same call, which is why proactive 403s surface here -// as ErrBotNotInstalled / ErrMessageWritesBlocked. +// PostMessage maps Thread.Post to the Connector "send to conversation" REST call, +// using the serviceUrl from the opaque Thread ID as the base URI. It is also the +// proactive-post path, so its 403s surface as ErrBotNotInstalled / +// ErrMessageWritesBlocked. The conversationReference is decoded from the Thread ID +// (the authoritative source), not trusted from ThreadRef.Raw. func (a *Adapter) PostMessage(ctx context.Context, thread chat.ThreadRef, msg chat.PostableMessage) (*chat.SentMessage, error) { - ref, err := connRefForThread(thread) + ref, err := decodeThreadID(thread.ID) if err != nil { return nil, err } @@ -79,14 +77,6 @@ func (a *Adapter) PostMessage(ctx context.Context, thread chat.ThreadRef, msg ch return &chat.SentMessage{ID: resp.ID, ThreadID: thread.ID, Raw: resp}, nil } -// connRefForThread decodes the conversationReference from the opaque Thread ID, the -// authoritative source. It deliberately does NOT trust ThreadRef.Raw: a caller-built -// ref whose Raw disagreed with its ID would otherwise post to the wrong conversation -// while reporting the ID's conversation. -func connRefForThread(thread chat.ThreadRef) (conversationReference, error) { - return decodeThreadID(thread.ID) -} - func textFormatFor(format chat.MessageFormat) (string, error) { switch format { case chat.MessageFormatText: diff --git a/adapters/msteams/msteams.go b/adapters/msteams/msteams.go index 6a91cf3..c7b6d3c 100644 --- a/adapters/msteams/msteams.go +++ b/adapters/msteams/msteams.go @@ -172,14 +172,11 @@ func (a *Adapter) Init(ctx context.Context) error { func (a *Adapter) Shutdown(context.Context) error { return nil } -// Webhook handles inbound Activities at the bot messaging endpoint. It decodes the -// Activity (a Supported Platform Shape), validates the inbound JWT before any -// normalization or dispatch (there is no path that skips validation), normalizes a -// message Activity into a runtime Event, dispatches synchronously, then acks with -// HTTP 200. The reply, if any, was sent by the handler via Thread.Post as a separate -// authenticated Connector call during dispatch -- there is no webhook-body reply -// shortcut for message activities. Work exceeding the turn budget is the runtime's -// Ack-Then-Work concern (ADR 0002), delivered later via a proactive Connector post. +// Webhook validates the inbound JWT before any normalization or dispatch (no path +// skips it), normalizes a message Activity into an Event, dispatches synchronously, +// then acks with HTTP 200. The reply is sent by the handler via Thread.Post as a +// separate Connector call -- there is no webhook-body reply for message activities; +// work past the turn budget uses ADR 0002 Ack-Then-Work plus a proactive post. func (a *Adapter) Webhook(dispatch chat.DispatchFunc) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { diff --git a/adapters/msteams/threadid.go b/adapters/msteams/threadid.go index c3eb70a..724b6fc 100644 --- a/adapters/msteams/threadid.go +++ b/adapters/msteams/threadid.go @@ -15,16 +15,10 @@ const ( threadIDPrefix = "msteams:v1:" ) -// conversationReference is the minimal, restart-durable subset of the Bot -// Framework conversationReference needed to post back to a Teams conversation -// later -- either a reply within the turn or a proactive post after it. It is the -// payload serialized into the opaque, adapter-produced Thread ID. -// -// serviceUrl is carried (unlike Slack's human-style channel+ts id) because Teams -// proactive posting cannot be reconstructed from a short id, and Microsoft warns -// serviceUrl can change over time, so the adapter refreshes it from each inbound -// Activity. tenantId is carried so the Thread ID is Platform Tenant scoped and -// cross-tenant collisions are impossible. +// conversationReference is the minimal subset of the Bot Framework +// conversationReference serialized into the opaque Thread ID -- enough to post back +// later (reply or proactive). Unlike Slack's short channel+ts id, it must carry +// serviceUrl (proactive posting can't reconstruct it) and tenantId (tenant scoping). type conversationReference struct { ServiceURL string `json:"service_url"` ConversationID string `json:"conversation_id"`