From 718c26b512e8b4d80e546f72cb7d590f7292c578 Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Mon, 15 Jun 2026 08:33:57 +0000 Subject: [PATCH 1/9] feat: Microsoft Teams integration (calls/room, calls/user, meetings) Add three NATS RPC handlers in room-service: - teams.call (calls/room): Teams deep link for a room call, self excluded. - teams.call.user (calls/user): Teams deep link for a 1:1 call. - teams.meeting (meetings): create a Graph onlineMeeting, idempotent per room. Email is derived as account@TEAMS_EMAIL_DOMAIN (no user-service lookup); the requester comes from the NATS subject. The meetings RPC reads back the last teams_meet_started system message (keyspace-aware) for idempotency and writes the marker via the canonical message path. Adds pkg/msgraph (client-credentials token + CreateOnlineMeeting behind a mockable interface) and pkg/model contracts. Closes #314 Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/client-api.md | 158 ++++++++++++ pkg/model/event.go | 5 + pkg/model/message.go | 9 + pkg/model/teams.go | 40 +++ pkg/msgraph/msgraph.go | 239 +++++++++++++++++ pkg/msgraph/msgraph_test.go | 112 ++++++++ pkg/subject/subject.go | 36 +++ room-service/deploy/docker-compose.yml | 12 + room-service/handler.go | 15 ++ room-service/handler_teams.go | 245 ++++++++++++++++++ room-service/handler_teams_test.go | 344 +++++++++++++++++++++++++ room-service/helper.go | 9 + room-service/main.go | 47 ++++ room-service/mock_store_test.go | 40 +++ room-service/store.go | 11 + room-service/store_cassandra.go | 54 ++++ 16 files changed, 1376 insertions(+) create mode 100644 pkg/model/teams.go create mode 100644 pkg/msgraph/msgraph.go create mode 100644 pkg/msgraph/msgraph_test.go create mode 100644 room-service/handler_teams.go create mode 100644 room-service/handler_teams_test.go diff --git a/docs/client-api.md b/docs/client-api.md index 00c612386..cc84ff9c6 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -558,6 +558,9 @@ and Rename Room. | `chat.user.{account}.request.orgs.{orgID}.{siteID}.members` | [List Org Members](#list-org-members) | | `chat.user.{account}.request.room.{roomID}.{siteID}.app.tabs` | [Get Room App Tabs](#get-room-app-tabs) | | `chat.user.{account}.request.room.{roomID}.{siteID}.app.cmd-menu` | [Get Room App Command Menu](#get-room-app-command-menu) | +| `chat.user.{account}.request.room.{roomID}.{siteID}.teams.call` | [Start Teams Room Call](#start-teams-room-call) | +| `chat.user.{account}.request.teams.{siteID}.call.user` | [Start Teams User Call](#start-teams-user-call) | +| `chat.user.{account}.request.room.{roomID}.{siteID}.teams.meeting` | [Start Teams Meeting](#start-teams-meeting) | #### Create Room @@ -1804,6 +1807,161 @@ Same envelope and sentinels as Get Room App Tabs. --- +#### Start Teams Room Call + +Builds a Microsoft Teams deep link for a call to every other member of the room (the caller is excluded). No Graph API call — the link is built from the member list, deriving each member's email as `account@TEAMS_EMAIL_DOMAIN`. + +External client label: `POST /api/v1/calls/room`. + +**Subject:** `chat.user.{account}.request.room.{roomID}.{siteID}.teams.call` +**Reply subject:** auto-generated `_INBOX.>` (NATS request/reply) + +- `{siteID}` must be the room's origin `siteID`. +- The requester account is taken from the subject, not from a token. + +##### Request body + +Empty body is accepted (the room is the subject's `{roomID}`). + +| Field | Type | Required | Notes | +|---|---|---|---| +| `roomId` | string | no | Optional echo of the room; the authoritative room is the subject's `{roomID}`. | + +##### Success response + +| Field | Type | Notes | +|---|---|---| +| `joinUrl` | string | A `https://teams.microsoft.com/l/call/0/0?users=` deep link. | + +```json +{ "joinUrl": "https://teams.microsoft.com/l/call/0/0?users=bob%40corp.com%2Ccarol%40corp.com" } +``` + +##### Error response + +See [Error envelope](#6-error-envelope-reference). + +| Reason | Code | When | +|---|---|---| +| — | `unauthenticated` | Requester account missing from the subject. | +| — | `bad_request` | `roomId` empty (subject malformed). | +| `not_room_member` | `forbidden` | Caller is not a member of the room. | +| `target_not_member` | `not_found` | No other callable members in the room. | +| `max_room_size_reached` | `conflict` | More than `ROOM_MEMBERS_CALL_LIMIT` (20) other members. | + +##### Triggered events — success path + +`None — reply only.` + +##### Triggered events — error path + +`None — error returned only via the reply subject.` + +--- + +#### Start Teams User Call + +Builds a Microsoft Teams 1:1 call deep link for a single target account. No Graph API call. The target email is derived as `accountName@TEAMS_EMAIL_DOMAIN`. + +External client label: `POST /api/v1/calls/user`. + +**Subject:** `chat.user.{account}.request.teams.{siteID}.call.user` +**Reply subject:** auto-generated `_INBOX.>` (NATS request/reply) + +- The requester account is taken from the subject, not from a token. + +##### Request body + +| Field | Type | Required | Notes | +|---|---|---|---| +| `accountName` | string | yes | The target user's account. | + +```json +{ "accountName": "bob" } +``` + +##### Success response + +| Field | Type | Notes | +|---|---|---| +| `joinUrl` | string | A `https://teams.microsoft.com/l/call/0/0?users=` deep link. | + +```json +{ "joinUrl": "https://teams.microsoft.com/l/call/0/0?users=bob%40corp.com" } +``` + +##### Error response + +See [Error envelope](#6-error-envelope-reference). + +| Reason | Code | When | +|---|---|---| +| — | `unauthenticated` | Requester account missing from the subject. | +| — | `bad_request` | `accountName` empty. | + +##### Triggered events — success path + +`None — reply only.` + +##### Triggered events — error path + +`None — error returned only via the reply subject.` + +--- + +#### Start Teams Meeting + +Creates a Microsoft Teams `onlineMeeting` via the Graph API and returns its join URL. **Idempotent per room:** the most recent `teams_meet_started` system message is the source of truth, so a second call returns the existing meeting without creating a duplicate. Attendee emails are derived as `account@TEAMS_EMAIL_DOMAIN`. + +External client label: `POST /api/v1/meetings`. + +**Subject:** `chat.user.{account}.request.room.{roomID}.{siteID}.teams.meeting` +**Reply subject:** auto-generated `_INBOX.>` (NATS request/reply) + +- `{siteID}` must be the room's origin `siteID`. +- The requester account is taken from the subject, not from a token; it becomes the meeting organizer. + +##### Request body + +Empty body is accepted (the room is the subject's `{roomID}`). + +| Field | Type | Required | Notes | +|---|---|---|---| +| `roomId` | string | no | Optional echo of the room; the authoritative room is the subject's `{roomID}`. | + +##### Success response + +| Field | Type | Notes | +|---|---|---| +| `id` | string | The Graph `onlineMeeting` ID. | +| `joinUrl` | string | The meeting's join web URL. | + +```json +{ "id": "MSpkYzE3...", "joinUrl": "https://teams.microsoft.com/l/meetup-join/..." } +``` + +##### Error response + +See [Error envelope](#6-error-envelope-reference). + +| Reason | Code | When | +|---|---|---| +| — | `unauthenticated` | Requester account missing from the subject. | +| — | `bad_request` | `roomId` empty (subject malformed). | +| `not_room_member` | `forbidden` | Caller is not a member of the room. | +| `max_room_size_reached` | `conflict` | Room has more than `ROOM_MEMBERS_LIMIT` (500) members. | +| — | `internal` | Teams meetings not configured, or the Graph create failed. | + +##### Triggered events — success path + +A `teams_meet_started` system message is published on the canonical message path (`chat.msg.canonical.{siteID}.created`), persisted by `message-worker`, and fanned out to room members like other system messages. Its `sysMsgData` carries `{ "meetingId": "...", "joinUrl": "..." }`. + +##### Triggered events — error path + +`None — error returned only via the reply subject.` + +--- + ### 3.2 history-service | RPC subject | Method | diff --git a/pkg/model/event.go b/pkg/model/event.go index ddea3370b..f18dae124 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -511,6 +511,11 @@ const ( // MessageTypeRoomRestricted is the system-message type emitted when a // channel's Restricted/ExternalAccess flags change. MessageTypeRoomRestricted = "room_restricted" + // MessageTypeTeamsMeetStarted is the system-message type emitted when a + // Microsoft Teams online meeting is created for a room. Its SysMsgData + // carries the meeting ID + join URL (TeamsMeetStartedSysData) and is read + // back per-room to make the meetings RPC idempotent. + MessageTypeTeamsMeetStarted = "teams_meet_started" ) const ( diff --git a/pkg/model/message.go b/pkg/model/message.go index abb11b98e..7932921cb 100644 --- a/pkg/model/message.go +++ b/pkg/model/message.go @@ -55,6 +55,15 @@ type RoomRestrictedSysData struct { OwnerAccount string `json:"ownerAccount,omitempty" bson:"ownerAccount,omitempty"` } +// TeamsMeetStartedSysData is the JSON payload stored in Message.SysMsgData for +// a teams_meet_started system message — emitted when a Microsoft Teams online +// meeting is created for a room. It is also the read-back source the meetings +// RPC uses for per-room idempotency. +type TeamsMeetStartedSysData struct { + MeetingID string `json:"meetingId" bson:"meetingId"` + JoinURL string `json:"joinUrl" bson:"joinUrl"` +} + type SendMessageRequest struct { ID string `json:"id"` Content string `json:"content"` diff --git a/pkg/model/teams.go b/pkg/model/teams.go new file mode 100644 index 000000000..4bdc28269 --- /dev/null +++ b/pkg/model/teams.go @@ -0,0 +1,40 @@ +package model + +// Request/reply contracts for the Microsoft Teams integration RPCs handled by +// room-service. Two endpoints (room call, user call) build a Teams deep link +// with no external I/O; the meetings endpoint creates a Graph onlineMeeting and +// is idempotent per room. + +// TeamsRoomCallRequest is the request body for the room-call deep-link RPC. +// The room is carried on the NATS subject, so the body may be empty; the field +// is accepted for clients that prefer to pass it explicitly. +type TeamsRoomCallRequest struct { + // RoomID is optional — the authoritative room is the subject's {roomID}. + RoomID string `json:"roomId,omitempty"` +} + +// TeamsUserCallRequest is the request body for the 1:1 user-call deep-link RPC. +type TeamsUserCallRequest struct { + // AccountName is the target user's account; its email is derived as + // account@TEAMS_EMAIL_DOMAIN. + AccountName string `json:"accountName"` +} + +// TeamsCallReply is the reply for both deep-link RPCs (calls/room, calls/user). +type TeamsCallReply struct { + JoinURL string `json:"joinUrl"` +} + +// TeamsMeetingRequest is the request body for the meetings RPC. The room is +// carried on the NATS subject; the body may be empty. +type TeamsMeetingRequest struct { + // RoomID is optional — the authoritative room is the subject's {roomID}. + RoomID string `json:"roomId,omitempty"` +} + +// TeamsMeetingReply is the reply for the meetings RPC: the Graph onlineMeeting +// ID and its join URL. +type TeamsMeetingReply struct { + ID string `json:"id"` + JoinURL string `json:"joinUrl"` +} diff --git a/pkg/msgraph/msgraph.go b/pkg/msgraph/msgraph.go new file mode 100644 index 000000000..d89fc40be --- /dev/null +++ b/pkg/msgraph/msgraph.go @@ -0,0 +1,239 @@ +// Package msgraph is a minimal Microsoft Graph client for the chat Teams +// integration. It supports the client-credentials (app-only) OAuth2 flow and +// creating an onlineMeeting. Only the surface room-service needs is exposed, +// and it sits behind the Client interface so the meetings RPC can be unit +// tested against a mock without reaching Azure. +package msgraph + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "sync" + "time" +) + +// Client is the Graph surface room-service depends on. Only the meetings RPC +// touches Graph, so this is intentionally tiny. Mocked in tests. +type Client interface { + // CreateOnlineMeeting creates an onlineMeeting on behalf of the configured + // organizer and returns its ID and join URL. + CreateOnlineMeeting(ctx context.Context, req CreateOnlineMeetingRequest) (*OnlineMeeting, error) +} + +// CreateOnlineMeetingRequest carries the attributes used to create a meeting. +type CreateOnlineMeetingRequest struct { + // Subject is the meeting title shown in Teams. + Subject string + // OrganizerEmail is the user the meeting is created for (the organizer). + // When empty the application-context default mailbox is used. + OrganizerEmail string + // AttendeeEmails are the invited attendees (excluding the organizer). + AttendeeEmails []string +} + +// OnlineMeeting is the subset of the Graph onlineMeeting resource we return. +type OnlineMeeting struct { + ID string `json:"id"` + JoinURL string `json:"joinWebUrl"` +} + +// Config holds the Azure app-registration credentials and tenant. +type Config struct { + TenantID string + ClientID string + ClientSecret string +} + +const ( + defaultGraphBaseURL = "https://graph.microsoft.com/v1.0" + graphScope = "https://graph.microsoft.com/.default" + // tokenExpirySkew is subtracted from the token's reported lifetime so the + // cached token is refreshed before the server-side expiry. + tokenExpirySkew = 60 * time.Second +) + +// graphClient is the live (*Client) implementation. +type graphClient struct { + cfg Config + httpClient *http.Client + baseURL string + tokenURL string + + mu sync.Mutex + token string + tokenAt time.Time // when the cached token expires +} + +// Option customizes the client (used in tests to point at an httptest server). +type Option func(*graphClient) + +// WithHTTPClient overrides the HTTP client. +func WithHTTPClient(c *http.Client) Option { + return func(g *graphClient) { g.httpClient = c } +} + +// WithBaseURL overrides the Graph API base URL (no trailing slash). +func WithBaseURL(u string) Option { + return func(g *graphClient) { g.baseURL = strings.TrimRight(u, "/") } +} + +// WithTokenURL overrides the OAuth2 token endpoint. +func WithTokenURL(u string) Option { + return func(g *graphClient) { g.tokenURL = u } +} + +// New constructs a live Graph client for the given config. +func New(cfg Config, opts ...Option) Client { + g := &graphClient{ + cfg: cfg, + httpClient: &http.Client{Timeout: 30 * time.Second}, + baseURL: defaultGraphBaseURL, + tokenURL: fmt.Sprintf( + "https://login.microsoftonline.com/%s/oauth2/v2.0/token", + url.PathEscape(cfg.TenantID), + ), + } + for _, opt := range opts { + opt(g) + } + return g +} + +type tokenResponse struct { + AccessToken string `json:"access_token"` + ExpiresIn int `json:"expires_in"` + Error string `json:"error"` + ErrorDesc string `json:"error_description"` +} + +// accessToken returns a cached bearer token, fetching a fresh one via the +// client-credentials grant when the cache is empty or near expiry. +func (g *graphClient) accessToken(ctx context.Context) (string, error) { + g.mu.Lock() + defer g.mu.Unlock() + + if g.token != "" && time.Now().Before(g.tokenAt) { + return g.token, nil + } + + form := url.Values{} + form.Set("grant_type", "client_credentials") + form.Set("client_id", g.cfg.ClientID) + form.Set("client_secret", g.cfg.ClientSecret) + form.Set("scope", graphScope) + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, g.tokenURL, strings.NewReader(form.Encode())) + if err != nil { + return "", fmt.Errorf("build token request: %w", err) + } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + resp, err := g.httpClient.Do(req) + if err != nil { + return "", fmt.Errorf("request token: %w", err) + } + defer resp.Body.Close() + + body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return "", fmt.Errorf("read token response: %w", err) + } + + var tr tokenResponse + if err := json.Unmarshal(body, &tr); err != nil { + return "", fmt.Errorf("decode token response (status %d): %w", resp.StatusCode, err) + } + if resp.StatusCode != http.StatusOK || tr.AccessToken == "" { + // Never log the credentials; surface the OAuth error code/description only. + return "", fmt.Errorf("token endpoint returned status %d: %s", resp.StatusCode, tr.Error) + } + + g.token = tr.AccessToken + lifetime := time.Duration(tr.ExpiresIn) * time.Second + if lifetime <= tokenExpirySkew { + lifetime = tokenExpirySkew + } + g.tokenAt = time.Now().Add(lifetime - tokenExpirySkew) + return g.token, nil +} + +// onlineMeetingPayload is the Graph create-onlineMeeting request body. +type onlineMeetingPayload struct { + Subject string `json:"subject,omitempty"` + Participants *meetingParticipants `json:"participants,omitempty"` +} + +type meetingParticipants struct { + Attendees []meetingAttendee `json:"attendees,omitempty"` +} + +type meetingAttendee struct { + Upn string `json:"upn"` +} + +func (g *graphClient) CreateOnlineMeeting(ctx context.Context, req CreateOnlineMeetingRequest) (*OnlineMeeting, error) { + token, err := g.accessToken(ctx) + if err != nil { + return nil, fmt.Errorf("acquire graph token: %w", err) + } + + // App-only context requires targeting a specific organizer mailbox via the + // /users/{id}/onlineMeetings path; delegated context uses /me. We use the + // organizer-scoped path when an organizer email is supplied. + var endpoint string + if req.OrganizerEmail != "" { + endpoint = fmt.Sprintf("%s/users/%s/onlineMeetings", g.baseURL, url.PathEscape(req.OrganizerEmail)) + } else { + endpoint = g.baseURL + "/me/onlineMeetings" + } + + payload := onlineMeetingPayload{Subject: req.Subject} + if len(req.AttendeeEmails) > 0 { + attendees := make([]meetingAttendee, 0, len(req.AttendeeEmails)) + for _, email := range req.AttendeeEmails { + attendees = append(attendees, meetingAttendee{Upn: email}) + } + payload.Participants = &meetingParticipants{Attendees: attendees} + } + + bodyBytes, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("marshal onlineMeeting payload: %w", err) + } + + httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(bodyBytes)) + if err != nil { + return nil, fmt.Errorf("build onlineMeeting request: %w", err) + } + httpReq.Header.Set("Authorization", "Bearer "+token) + httpReq.Header.Set("Content-Type", "application/json") + + resp, err := g.httpClient.Do(httpReq) + if err != nil { + return nil, fmt.Errorf("create onlineMeeting: %w", err) + } + defer resp.Body.Close() + + respBody, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return nil, fmt.Errorf("read onlineMeeting response: %w", err) + } + if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("create onlineMeeting: graph returned status %d: %s", resp.StatusCode, string(respBody)) + } + + var meeting OnlineMeeting + if err := json.Unmarshal(respBody, &meeting); err != nil { + return nil, fmt.Errorf("decode onlineMeeting response: %w", err) + } + if meeting.JoinURL == "" { + return nil, fmt.Errorf("create onlineMeeting: graph response missing joinWebUrl") + } + return &meeting, nil +} diff --git a/pkg/msgraph/msgraph_test.go b/pkg/msgraph/msgraph_test.go new file mode 100644 index 000000000..9eb7efc20 --- /dev/null +++ b/pkg/msgraph/msgraph_test.go @@ -0,0 +1,112 @@ +package msgraph + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTestClient wires a graphClient at the given token + graph servers. +func newTestClient(tokenURL, baseURL string) Client { + return New( + Config{TenantID: "t", ClientID: "c", ClientSecret: "s"}, + WithTokenURL(tokenURL), + WithBaseURL(baseURL), + ) +} + +func TestCreateOnlineMeeting_Success(t *testing.T) { + var tokenCalls, meetingCalls int + + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tokenCalls++ + require.NoError(t, r.ParseForm()) + assert.Equal(t, "client_credentials", r.Form.Get("grant_type")) + assert.Equal(t, graphScope, r.Form.Get("scope")) + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok-123", ExpiresIn: 3600}) + })) + defer tokenSrv.Close() + + graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + meetingCalls++ + assert.Equal(t, "Bearer tok-123", r.Header.Get("Authorization")) + assert.True(t, strings.Contains(r.URL.Path, "/users/alice%40corp.com/onlineMeetings") || + strings.Contains(r.URL.Path, "/users/alice@corp.com/onlineMeetings"), + "organizer-scoped path expected, got %s", r.URL.Path) + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(OnlineMeeting{ID: "m1", JoinURL: "https://join/1"}) + })) + defer graphSrv.Close() + + c := newTestClient(tokenSrv.URL, graphSrv.URL) + m, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ + Subject: "Standup", OrganizerEmail: "alice@corp.com", AttendeeEmails: []string{"bob@corp.com"}, + }) + require.NoError(t, err) + assert.Equal(t, "m1", m.ID) + assert.Equal(t, "https://join/1", m.JoinURL) + + // Second call reuses the cached token (no second token fetch). + _, err = c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "alice@corp.com"}) + require.NoError(t, err) + assert.Equal(t, 1, tokenCalls, "token should be cached across calls") + assert.Equal(t, 2, meetingCalls) +} + +func TestCreateOnlineMeeting_TokenError(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _ = json.NewEncoder(w).Encode(tokenResponse{Error: "invalid_client", ErrorDesc: "bad secret"}) + })) + defer tokenSrv.Close() + + c := New( + Config{TenantID: "t", ClientID: "c", ClientSecret: "super-secret-value"}, + WithTokenURL(tokenSrv.URL), WithBaseURL("http://unused"), + ) + _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid_client") + // Never leak the secret in the error. + assert.NotContains(t, err.Error(), "super-secret-value") +} + +func TestCreateOnlineMeeting_GraphError(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) + })) + defer tokenSrv.Close() + graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":{"code":"Forbidden"}}`)) + })) + defer graphSrv.Close() + + c := newTestClient(tokenSrv.URL, graphSrv.URL) + _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "403") +} + +func TestCreateOnlineMeeting_MissingJoinURL(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) + })) + defer tokenSrv.Close() + graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(OnlineMeeting{ID: "m1"}) // no joinWebUrl + })) + defer graphSrv.Close() + + c := newTestClient(tokenSrv.URL, graphSrv.URL) + _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "joinWebUrl") +} diff --git a/pkg/subject/subject.go b/pkg/subject/subject.go index 9e8a13c6b..d59c469ea 100644 --- a/pkg/subject/subject.go +++ b/pkg/subject/subject.go @@ -759,6 +759,42 @@ func RoomAppCmdMenuPattern(siteID string) string { return fmt.Sprintf("chat.user.{account}.request.room.{roomID}.%s.app.cmd-menu", siteID) } +// --- Microsoft Teams integration --- +// +// TeamsRoomCall + TeamsMeeting are room-scoped (the roomID rides the subject so +// membership can be checked), matching every other room RPC. TeamsUserCall is a +// 1:1 deep-link builder with no room; the target account travels in the body. + +// TeamsRoomCall returns the concrete subject for the room-call deep-link RPC. +func TeamsRoomCall(account, roomID, siteID string) string { + return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.call", account, roomID, siteID) +} + +// TeamsRoomCallPattern is the natsrouter registration pattern for the room-call RPC. +func TeamsRoomCallPattern(siteID string) string { + return fmt.Sprintf("chat.user.{account}.request.room.{roomID}.%s.teams.call", siteID) +} + +// TeamsMeeting returns the concrete subject for the Graph onlineMeeting RPC. +func TeamsMeeting(account, roomID, siteID string) string { + return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.meeting", account, roomID, siteID) +} + +// TeamsMeetingPattern is the natsrouter registration pattern for the meetings RPC. +func TeamsMeetingPattern(siteID string) string { + return fmt.Sprintf("chat.user.{account}.request.room.{roomID}.%s.teams.meeting", siteID) +} + +// TeamsUserCall returns the concrete subject for the 1:1 user-call deep-link RPC. +func TeamsUserCall(account, siteID string) string { + return fmt.Sprintf("chat.user.%s.request.teams.%s.call.user", account, siteID) +} + +// TeamsUserCallPattern is the natsrouter registration pattern for the user-call RPC. +func TeamsUserCallPattern(siteID string) string { + return fmt.Sprintf("chat.user.{account}.request.teams.%s.call.user", siteID) +} + // isValidAccountToken is the parsers' boundary guard for the account token so // wildcard semantics never leak into identity parsing. func isValidAccountToken(token string) bool { diff --git a/room-service/deploy/docker-compose.yml b/room-service/deploy/docker-compose.yml index 8d2d0e192..cad818a9b 100644 --- a/room-service/deploy/docker-compose.yml +++ b/room-service/deploy/docker-compose.yml @@ -56,6 +56,18 @@ services: - ROOM_KEY_GRACE_PERIOD=24h - CASSANDRA_HOSTS=cassandra - CASSANDRA_KEYSPACE=chat + # Microsoft Teams integration. The deep-link RPCs (calls/room, calls/user) + # only need TEAMS_EMAIL_DOMAIN; the meetings RPC needs the Azure app creds + # below and reports not-configured until they are set (live test only). + - TEAMS_EMAIL_DOMAIN=dev.local + - TEAMS_TENANT_ID= + - TEAMS_CLIENT_ID= + - TEAMS_CLIENT_SECRET= + - ROOM_MEMBERS_LIMIT=500 + - ROOM_MEMBERS_CALL_LIMIT=20 + # Must match message-worker's MESSAGE_BUCKET_HOURS so the teams_meet_started + # idempotency read walks the same partitions writes land in. + - MESSAGE_BUCKET_HOURS=72 - BOOTSTRAP_STREAMS=true - ATREST_ENABLED=true - VAULT_ADDR=http://vault:8200 diff --git a/room-service/handler.go b/room-service/handler.go index 344f37095..53ba53381 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -25,6 +25,7 @@ import ( "github.com/hmchangw/chat/pkg/idgen" "github.com/hmchangw/chat/pkg/logctx" "github.com/hmchangw/chat/pkg/model" + "github.com/hmchangw/chat/pkg/msgraph" "github.com/hmchangw/chat/pkg/natsrouter" "github.com/hmchangw/chat/pkg/natsutil" "github.com/hmchangw/chat/pkg/roomkeystore" @@ -52,6 +53,17 @@ type Handler struct { restrictedRoomMinMembers int siteURL *url.URL maxResponseBytes int64 + + // Microsoft Teams integration. graphClient is nil-safe: only the meetings + // RPC uses it (the deep-link RPCs are pure string building). teamsEmailDomain + // derives a member's email as account@domain. meetMarkerReader backs the + // per-room idempotency read. roomMembersLimit / roomMembersCallLimit cap the + // member set for meetings and calls respectively. + graphClient msgraph.Client + meetMarkerReader MeetMarkerReader + teamsEmailDomain string + roomMembersLimit int + roomMembersCallLimit int } func NewHandler(store RoomStore, keyStore RoomKeyStore, memberListClient MemberListClient, msgReader MessageReader, siteID string, maxRoomSize, maxBatchSize int, memberListTimeout time.Duration, restrictedRoomMinMembers int, publishToStream func(context.Context, string, []byte, string) error, publishCore func(context.Context, string, []byte) error, siteURL *url.URL, maxResponseBytes int64) *Handler { @@ -100,6 +112,9 @@ func (h *Handler) Register(r *natsrouter.Router) { natsrouter.Register(r, subject.ThreadUnreadSummarySubscribe(h.siteID), h.threadUnreadSummary) natsrouter.Register(r, subject.RoomKeyEnsure(h.siteID), h.ensureRoomKey) natsrouter.Register(r, subject.RoomCreatePattern(h.siteID), h.createRoom) + natsrouter.Register(r, subject.TeamsRoomCallPattern(h.siteID), h.teamsRoomCall) + natsrouter.Register(r, subject.TeamsUserCallPattern(h.siteID), h.teamsUserCall) + natsrouter.Register(r, subject.TeamsMeetingPattern(h.siteID), h.teamsMeeting) } func (h *Handler) createRoom(c *natsrouter.Context, req model.CreateRoomRequest) (*model.CreateRoomReply, error) { //nolint:gocritic // hugeParam: req is passed by value to satisfy the natsrouter.Register handler signature diff --git a/room-service/handler_teams.go b/room-service/handler_teams.go new file mode 100644 index 000000000..91cee50c5 --- /dev/null +++ b/room-service/handler_teams.go @@ -0,0 +1,245 @@ +package main + +import ( + "context" + "encoding/json" + "fmt" + "net/url" + "strings" + "time" + + "github.com/hmchangw/chat/pkg/displayfmt" + "github.com/hmchangw/chat/pkg/idgen" + "github.com/hmchangw/chat/pkg/model" + "github.com/hmchangw/chat/pkg/msgraph" + "github.com/hmchangw/chat/pkg/natsrouter" + "github.com/hmchangw/chat/pkg/natsutil" + "github.com/hmchangw/chat/pkg/subject" +) + +// teamsDeepLinkBase is the Teams 1:1/group call deep-link prefix. The callable +// users are appended as a comma-joined `users` query param. +const teamsDeepLinkBase = "https://teams.microsoft.com/l/call/0/0" + +// teamsEmail derives a member's email from their account using the configured +// corporate domain — the only identity available at the NATS layer (the OIDC +// email claim exists only at auth-service token exchange). Mirrors the dev +// auth-service derivation (account + "@dev.local"). +func teamsEmail(account, domain string) string { + return account + "@" + domain +} + +// buildTeamsCallDeepLink builds a Teams call deep link for the given attendee +// emails. Order is preserved; the caller is responsible for excluding self. +func buildTeamsCallDeepLink(emails []string) string { + v := url.Values{} + v.Set("users", strings.Join(emails, ",")) + return teamsDeepLinkBase + "?" + v.Encode() +} + +// teamsRoomCall builds a Teams deep link for a call to every other member of the +// room (self excluded). No Microsoft Graph call — pure string building from the +// member list. Enforces roomMembersCallLimit. +func (h *Handler) teamsRoomCall(c *natsrouter.Context, _ model.TeamsRoomCallRequest) (*model.TeamsCallReply, error) { //nolint:gocritic // hugeParam: req passed by value to satisfy natsrouter.Register + var ctx context.Context = c + requesterAccount := c.Param("account") + roomID := c.Param("roomID") + + if requesterAccount == "" { + return nil, errTeamsRequesterMissing + } + if roomID == "" { + return nil, errTeamsRoomIDRequired + } + + if _, err := h.requireMembershipAndGetRoom(ctx, requesterAccount, roomID); err != nil { + return nil, err + } + + members, err := h.store.ListRoomMembers(ctx, roomID, nil, nil, false) + if err != nil { + return nil, fmt.Errorf("list room members: %w", err) + } + + emails := membersToCallEmails(members, requesterAccount, h.teamsEmailDomain) + if len(emails) == 0 { + return nil, errTeamsNoCallableMembers + } + if len(emails) > h.roomMembersCallLimit { + return nil, errTeamsCallTooManyMembers + } + + return &model.TeamsCallReply{JoinURL: buildTeamsCallDeepLink(emails)}, nil +} + +// teamsUserCall builds a Teams 1:1 call deep link for the target account. No +// Graph call. The target email is account@domain (same derivation as everywhere +// else in this integration). +func (h *Handler) teamsUserCall(c *natsrouter.Context, req model.TeamsUserCallRequest) (*model.TeamsCallReply, error) { //nolint:gocritic // hugeParam: req passed by value to satisfy natsrouter.Register + requesterAccount := c.Param("account") + if requesterAccount == "" { + return nil, errTeamsRequesterMissing + } + if req.AccountName == "" { + return nil, errTeamsAccountRequired + } + + email := teamsEmail(req.AccountName, h.teamsEmailDomain) + return &model.TeamsCallReply{JoinURL: buildTeamsCallDeepLink([]string{email})}, nil +} + +// teamsMeeting creates (or returns the existing) Microsoft Teams onlineMeeting +// for the room. Idempotent per room: the last teams_meet_started system message +// is the source of truth, so a second call returns the prior meeting without a +// duplicate Graph create. Enforces roomMembersLimit. +func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingRequest) (*model.TeamsMeetingReply, error) { //nolint:gocritic // hugeParam: req passed by value to satisfy natsrouter.Register + var ctx context.Context = c + requestID := natsutil.RequestIDFromContext(c) + requesterAccount := c.Param("account") + roomID := c.Param("roomID") + + if requesterAccount == "" { + return nil, errTeamsRequesterMissing + } + if roomID == "" { + return nil, errTeamsRoomIDRequired + } + if h.graphClient == nil || h.meetMarkerReader == nil { + return nil, errTeamsNotConfigured + } + + room, err := h.requireMembershipAndGetRoom(ctx, requesterAccount, roomID) + if err != nil { + return nil, err + } + + // Idempotency: a prior teams_meet_started marker short-circuits the Graph + // create so retries (and concurrent clients) reuse the same meeting. + marker, found, err := h.meetMarkerReader.GetLastTeamsMeetStarted(ctx, roomID) + if err != nil { + return nil, fmt.Errorf("read meeting marker: %w", err) + } + if found && marker != nil && marker.JoinURL != "" { + return &model.TeamsMeetingReply{ID: marker.MeetingID, JoinURL: marker.JoinURL}, nil + } + + members, err := h.store.ListRoomMembers(ctx, roomID, nil, nil, false) + if err != nil { + return nil, fmt.Errorf("list room members: %w", err) + } + if countIndividualMembers(members) > h.roomMembersLimit { + return nil, errTeamsMeetingTooManyMembers + } + + attendeeEmails := membersToAttendeeEmails(members, h.teamsEmailDomain) + organizerEmail := teamsEmail(requesterAccount, h.teamsEmailDomain) + + meeting, err := h.graphClient.CreateOnlineMeeting(ctx, msgraph.CreateOnlineMeetingRequest{ + Subject: meetingSubject(room), + OrganizerEmail: organizerEmail, + AttendeeEmails: attendeeEmails, + }) + if err != nil { + return nil, fmt.Errorf("create online meeting: %w", err) + } + + if err := h.publishTeamsMeetStarted(ctx, requestID, roomID, requesterAccount, meeting); err != nil { + return nil, err + } + + return &model.TeamsMeetingReply{ID: meeting.ID, JoinURL: meeting.JoinURL}, nil +} + +// publishTeamsMeetStarted writes the teams_meet_started system message through +// the canonical message path — the same flow room_restricted uses — so it is +// persisted by message-worker and fanned out to room members. The marker is the +// idempotency source for subsequent meetings calls. +func (h *Handler) publishTeamsMeetStarted( + ctx context.Context, + requestID, roomID, byAccount string, + meeting *msgraph.OnlineMeeting, +) error { + sysData, err := json.Marshal(model.TeamsMeetStartedSysData{ + MeetingID: meeting.ID, + JoinURL: meeting.JoinURL, + }) + if err != nil { + return fmt.Errorf("marshal teams_meet_started sys data: %w", err) + } + + now := time.Now().UTC() + sysMsg := model.Message{ + ID: idgen.MessageIDFromRequestID(requestID, "teams_meet_started"), + RoomID: roomID, + UserAccount: byAccount, + Type: model.MessageTypeTeamsMeetStarted, + Content: "started a Teams meeting", + SysMsgData: sysData, + CreatedAt: now, + } + msgEvt := model.MessageEvent{ + Event: model.EventCreated, + Message: sysMsg, + SiteID: h.siteID, + Timestamp: now.UnixMilli(), + } + msgEvtData, err := json.Marshal(msgEvt) + if err != nil { + return fmt.Errorf("marshal teams_meet_started message event: %w", err) + } + if err := h.publishToStream(ctx, subject.MsgCanonicalCreated(h.siteID), msgEvtData, natsutil.CanonicalDedupID(&msgEvt)); err != nil { + return fmt.Errorf("publish teams_meet_started sys message: %w", err) + } + return nil +} + +// membersToCallEmails returns deep-link emails for every individual member +// except self, preserving member order. +func membersToCallEmails(members []model.RoomMember, self, domain string) []string { + out := make([]string, 0, len(members)) + for i := range members { + entry := members[i].Member + if entry.Type != model.RoomMemberIndividual || entry.Account == "" { + continue + } + if entry.Account == self { + continue + } + out = append(out, teamsEmail(entry.Account, domain)) + } + return out +} + +// membersToAttendeeEmails returns attendee emails for every individual member +// (organizer included is harmless; Graph dedups the organizer). +func membersToAttendeeEmails(members []model.RoomMember, domain string) []string { + out := make([]string, 0, len(members)) + for i := range members { + entry := members[i].Member + if entry.Type != model.RoomMemberIndividual || entry.Account == "" { + continue + } + out = append(out, teamsEmail(entry.Account, domain)) + } + return out +} + +// countIndividualMembers counts individual (human) members for the limit gate. +func countIndividualMembers(members []model.RoomMember) int { + n := 0 + for i := range members { + if members[i].Member.Type == model.RoomMemberIndividual { + n++ + } + } + return n +} + +// meetingSubject builds a human-friendly meeting title from the room. +func meetingSubject(room *model.Room) string { + name := displayfmt.CombineWithFallback("", "", room.Name) + if name == "" { + return "Chat meeting" + } + return name +} diff --git a/room-service/handler_teams_test.go b/room-service/handler_teams_test.go new file mode 100644 index 000000000..aae2ac6fd --- /dev/null +++ b/room-service/handler_teams_test.go @@ -0,0 +1,344 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/hmchangw/chat/pkg/errcode" + "github.com/hmchangw/chat/pkg/model" + "github.com/hmchangw/chat/pkg/msgraph" +) + +// fakeGraphClient is a hand-rolled msgraph.Client double. It records calls and +// returns a canned meeting or error. callCount lets idempotency tests assert +// that a second meetings call does NOT reach Graph. +type fakeGraphClient struct { + meeting *msgraph.OnlineMeeting + err error + callCount int + lastReq msgraph.CreateOnlineMeetingRequest +} + +func (f *fakeGraphClient) CreateOnlineMeeting(_ context.Context, req msgraph.CreateOnlineMeetingRequest) (*msgraph.OnlineMeeting, error) { + f.callCount++ + f.lastReq = req + if f.err != nil { + return nil, f.err + } + return f.meeting, nil +} + +// stubMeetMarkerReader is a hand-rolled MeetMarkerReader double. +type stubMeetMarkerReader struct { + marker *model.TeamsMeetStartedSysData + found bool + err error +} + +func (s *stubMeetMarkerReader) GetLastTeamsMeetStarted(_ context.Context, _ string) (*model.TeamsMeetStartedSysData, bool, error) { + return s.marker, s.found, s.err +} + +func indMember(account string) model.RoomMember { + return model.RoomMember{ + Member: model.RoomMemberEntry{Type: model.RoomMemberIndividual, Account: account}, + } +} + +func orgMember(id string) model.RoomMember { + return model.RoomMember{Member: model.RoomMemberEntry{Type: model.RoomMemberOrg, ID: id}} +} + +// --- calls/room (teamsRoomCall) --- + +func TestTeamsRoomCall_Success_ExcludesSelf(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1"). + Return(&model.Room{ID: "r1", Name: "general", Type: model.RoomTypeChannel}, nil) + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). + Return([]model.RoomMember{indMember("alice"), indMember("bob"), orgMember("orgX"), indMember("carol")}, nil) + + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersCallLimit: 20} + + resp, err := h.teamsRoomCall(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsRoomCallRequest{}) + require.NoError(t, err) + + // alice (self) and the org entry are excluded; bob + carol remain, order preserved. + users := parseUsersParam(t, resp.JoinURL) + assert.Equal(t, []string{"bob@corp.com", "carol@corp.com"}, users) + assert.True(t, strings.HasPrefix(resp.JoinURL, "https://teams.microsoft.com/l/call/0/0?")) +} + +func TestTeamsRoomCall_RequesterMissing(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersCallLimit: 20} + _, err := h.teamsRoomCall(ctxParams(map[string]string{"account": "", "roomID": "r1"}), model.TeamsRoomCallRequest{}) + require.ErrorIs(t, err, errTeamsRequesterMissing) +} + +func TestTeamsRoomCall_RoomIDMissing(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersCallLimit: 20} + _, err := h.teamsRoomCall(ctxParams(map[string]string{"account": "alice", "roomID": ""}), model.TeamsRoomCallRequest{}) + require.ErrorIs(t, err, errTeamsRoomIDRequired) +} + +func TestTeamsRoomCall_NotMember(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1"). + Return(nil, model.ErrSubscriptionNotFound) + store.EXPECT().GetRoom(gomock.Any(), "r1"). + Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersCallLimit: 20} + _, err := h.teamsRoomCall(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsRoomCallRequest{}) + require.ErrorIs(t, err, errNotRoomMember) +} + +func TestTeamsRoomCall_NoOtherMembers(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). + Return([]model.RoomMember{indMember("alice")}, nil) + + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersCallLimit: 20} + _, err := h.teamsRoomCall(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsRoomCallRequest{}) + require.ErrorIs(t, err, errTeamsNoCallableMembers) + assert.Equal(t, errcode.RoomTargetNotMember, errcode.ReasonOf(err)) +} + +func TestTeamsRoomCall_TooManyMembers(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + + members := []model.RoomMember{indMember("alice")} + for i := 0; i < 3; i++ { + members = append(members, indMember(string(rune('a'+i))+"x")) + } + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false).Return(members, nil) + + // CallLimit=2, but 3 other members → over the limit. + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersCallLimit: 2} + _, err := h.teamsRoomCall(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsRoomCallRequest{}) + require.ErrorIs(t, err, errTeamsCallTooManyMembers) + assert.Equal(t, errcode.RoomMaxSizeReached, errcode.ReasonOf(err)) +} + +// --- calls/user (teamsUserCall) --- + +func TestTeamsUserCall_Success(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com"} + resp, err := h.teamsUserCall(ctxParams(map[string]string{"account": "alice"}), model.TeamsUserCallRequest{AccountName: "bob"}) + require.NoError(t, err) + users := parseUsersParam(t, resp.JoinURL) + assert.Equal(t, []string{"bob@corp.com"}, users) +} + +func TestTeamsUserCall_RequesterMissing(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com"} + _, err := h.teamsUserCall(ctxParams(map[string]string{"account": ""}), model.TeamsUserCallRequest{AccountName: "bob"}) + require.ErrorIs(t, err, errTeamsRequesterMissing) +} + +func TestTeamsUserCall_AccountNameRequired(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com"} + _, err := h.teamsUserCall(ctxParams(map[string]string{"account": "alice"}), model.TeamsUserCallRequest{AccountName: ""}) + require.ErrorIs(t, err, errTeamsAccountRequired) +} + +// --- meetings (teamsMeeting) --- + +func TestTeamsMeeting_CreatesAndPublishes(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Name: "general", Type: model.RoomTypeChannel}, nil) + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). + Return([]model.RoomMember{indMember("alice"), indMember("bob")}, nil) + + graph := &fakeGraphClient{meeting: &msgraph.OnlineMeeting{ID: "mtg-1", JoinURL: "https://teams.example/join/1"}} + + var publishedSubj string + var publishedData []byte + var publishedMsgID string + h := &Handler{ + store: store, + siteID: "site-a", + teamsEmailDomain: "corp.com", + roomMembersLimit: 500, + graphClient: graph, + meetMarkerReader: &stubMeetMarkerReader{found: false}, + publishToStream: func(_ context.Context, subj string, data []byte, msgID string) error { + publishedSubj, publishedData, publishedMsgID = subj, data, msgID + return nil + }, + } + + resp, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.NoError(t, err) + assert.Equal(t, "mtg-1", resp.ID) + assert.Equal(t, "https://teams.example/join/1", resp.JoinURL) + + // Graph called exactly once; organizer + attendees derived from accounts. + assert.Equal(t, 1, graph.callCount) + assert.Equal(t, "alice@corp.com", graph.lastReq.OrganizerEmail) + assert.ElementsMatch(t, []string{"alice@corp.com", "bob@corp.com"}, graph.lastReq.AttendeeEmails) + + // teams_meet_started published through the canonical message path. + require.NotEmpty(t, publishedData, "expected a canonical message publish") + assert.NotEmpty(t, publishedMsgID, "expected a dedup msg ID") + assert.Contains(t, publishedSubj, "chat.msg.canonical.site-a.created") + + var evt model.MessageEvent + require.NoError(t, json.Unmarshal(publishedData, &evt)) + assert.Equal(t, model.MessageTypeTeamsMeetStarted, evt.Message.Type) + var sys model.TeamsMeetStartedSysData + require.NoError(t, json.Unmarshal(evt.Message.SysMsgData, &sys)) + assert.Equal(t, "mtg-1", sys.MeetingID) + assert.Equal(t, "https://teams.example/join/1", sys.JoinURL) +} + +func TestTeamsMeeting_Idempotent_ReturnsExisting(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + // ListRoomMembers must NOT be called when a marker already exists. + + graph := &fakeGraphClient{meeting: &msgraph.OnlineMeeting{ID: "should-not-be-used"}} + h := &Handler{ + store: store, + siteID: "site-a", + teamsEmailDomain: "corp.com", + roomMembersLimit: 500, + graphClient: graph, + meetMarkerReader: &stubMeetMarkerReader{ + found: true, + marker: &model.TeamsMeetStartedSysData{MeetingID: "mtg-existing", JoinURL: "https://teams.example/join/existing"}, + }, + publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { + t.Error("idempotent path must not publish a new system message") + return nil + }, + } + + resp, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.NoError(t, err) + assert.Equal(t, "mtg-existing", resp.ID) + assert.Equal(t, "https://teams.example/join/existing", resp.JoinURL) + assert.Equal(t, 0, graph.callCount, "no duplicate Graph create on the idempotent path") +} + +func TestTeamsMeeting_NotConfigured(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500} // graphClient + meetMarkerReader nil + _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.ErrorIs(t, err, errTeamsNotConfigured) +} + +func TestTeamsMeeting_RequesterMissing(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, + graphClient: &fakeGraphClient{}, meetMarkerReader: &stubMeetMarkerReader{}} + _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.ErrorIs(t, err, errTeamsRequesterMissing) +} + +func TestTeamsMeeting_RoomIDMissing(t *testing.T) { + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, + graphClient: &fakeGraphClient{}, meetMarkerReader: &stubMeetMarkerReader{}} + _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": ""}), model.TeamsMeetingRequest{}) + require.ErrorIs(t, err, errTeamsRoomIDRequired) +} + +func TestTeamsMeeting_NotMember(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(nil, model.ErrSubscriptionNotFound) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + + graph := &fakeGraphClient{} + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, + graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{found: false}} + _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.ErrorIs(t, err, errNotRoomMember) + assert.Equal(t, 0, graph.callCount) +} + +func TestTeamsMeeting_TooManyMembers(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). + Return([]model.RoomMember{indMember("alice"), indMember("bob"), indMember("carol")}, nil) + + graph := &fakeGraphClient{} + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 2, + graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{found: false}} + _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.ErrorIs(t, err, errTeamsMeetingTooManyMembers) + assert.Equal(t, errcode.RoomMaxSizeReached, errcode.ReasonOf(err)) + assert.Equal(t, 0, graph.callCount, "limit gate must short-circuit before Graph") +} + +func TestTeamsMeeting_GraphCreateFails(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). + Return([]model.RoomMember{indMember("alice")}, nil) + + graph := &fakeGraphClient{err: errors.New("graph 500")} + var published bool + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, + graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{found: false}, + publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { published = true; return nil }, + } + _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.Error(t, err) + assert.False(t, published, "no system message on Graph failure") +} + +func TestTeamsMeeting_MarkerReadFails(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + + graph := &fakeGraphClient{} + h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, + graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{err: errors.New("cass down")}} + _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.Error(t, err) + assert.Equal(t, 0, graph.callCount) +} + +// parseUsersParam extracts the comma-separated `users` query param from a Teams +// deep link and returns the individual entries. +func parseUsersParam(t *testing.T, link string) []string { + t.Helper() + u, err := url.Parse(link) + require.NoError(t, err) + raw := u.Query().Get("users") + if raw == "" { + return nil + } + return strings.Split(raw, ",") +} diff --git a/room-service/helper.go b/room-service/helper.go index 7ba43d980..50307b493 100644 --- a/room-service/helper.go +++ b/room-service/helper.go @@ -78,6 +78,15 @@ var ( // version has aged out of the grace window). errRoomKeyAbsent = errcode.NotFound("room key not available") + // Sentinels for the Microsoft Teams integration RPCs. + errTeamsAccountRequired = errcode.BadRequest("accountName is required") + errTeamsRoomIDRequired = errcode.BadRequest("roomId is required") + errTeamsNoCallableMembers = errcode.NotFound("no other members to call", errcode.WithReason(errcode.RoomTargetNotMember)) + errTeamsCallTooManyMembers = errcode.Conflict("room has too many members to start a call", errcode.WithReason(errcode.RoomMaxSizeReached)) + errTeamsMeetingTooManyMembers = errcode.Conflict("room has too many members to start a meeting", errcode.WithReason(errcode.RoomMaxSizeReached)) + errTeamsNotConfigured = errcode.Internal("teams meetings are not configured") + errTeamsRequesterMissing = errcode.Unauthenticated("requester account missing") + // Sentinels for rename and restricted operations. errOnlyOwnersOrAdmins = errcode.Forbidden("only owners or platform admins can rename a channel") errOnlyAdmins = errcode.Forbidden("only admins can change room restricted state") diff --git a/room-service/main.go b/room-service/main.go index 20a70c9fd..cc5ed3c5a 100644 --- a/room-service/main.go +++ b/room-service/main.go @@ -18,6 +18,8 @@ import ( "github.com/hmchangw/chat/pkg/health" "github.com/hmchangw/chat/pkg/logctx" "github.com/hmchangw/chat/pkg/mongoutil" + "github.com/hmchangw/chat/pkg/msgbucket" + "github.com/hmchangw/chat/pkg/msgraph" "github.com/hmchangw/chat/pkg/natsrouter" "github.com/hmchangw/chat/pkg/natsutil" "github.com/hmchangw/chat/pkg/otelutil" @@ -46,6 +48,20 @@ type config struct { HealthAddr string `env:"HEALTH_ADDR" envDefault:":8081"` Bootstrap bootstrapConfig `envPrefix:"BOOTSTRAP_"` RestrictedRoomMinMembers int `env:"RESTRICTED_ROOM_MIN_MEMBERS" envDefault:"5"` + // Microsoft Teams integration. Teams* credentials are required only for the + // meetings RPC (Graph onlineMeeting create); the deep-link RPCs use only + // EmailDomain. When TenantID/ClientID/ClientSecret are unset the meetings RPC + // returns errTeamsNotConfigured; the deep-link RPCs still work. + TeamsTenantID string `env:"TEAMS_TENANT_ID" envDefault:""` + TeamsClientID string `env:"TEAMS_CLIENT_ID" envDefault:""` + TeamsClientSecret string `env:"TEAMS_CLIENT_SECRET" envDefault:""` + TeamsEmailDomain string `env:"TEAMS_EMAIL_DOMAIN" envDefault:"dev.local"` + RoomMembersLimit int `env:"ROOM_MEMBERS_LIMIT" envDefault:"500"` + RoomMembersCallLimit int `env:"ROOM_MEMBERS_CALL_LIMIT" envDefault:"20"` + // MessageBucketHours must match message-worker's MESSAGE_BUCKET_HOURS so the + // teams_meet_started idempotency read walks the same partitions writes land in. + MessageBucketHours int `env:"MESSAGE_BUCKET_HOURS" envDefault:"72"` + MessageReadMaxBuckets int `env:"MESSAGE_READ_MAX_BUCKETS" envDefault:"30"` // Atrest/Vault drive eager at-rest DEK provisioning at room creation. // When Atrest.Enabled is false the DEK is created lazily by message-worker. Atrest atrest.Config // env vars already prefixed ATREST_* @@ -139,6 +155,32 @@ func main() { } cassReader := NewCassMessageReader(cassSession) + if cfg.MessageBucketHours <= 0 { + slog.Error("invalid MESSAGE_BUCKET_HOURS: must be > 0", "value", cfg.MessageBucketHours) + os.Exit(1) + } + if cfg.MessageReadMaxBuckets <= 0 { + slog.Error("invalid MESSAGE_READ_MAX_BUCKETS: must be > 0", "value", cfg.MessageReadMaxBuckets) + os.Exit(1) + } + meetMarkerReader := NewCassMeetMarkerReader( + cassSession, + msgbucket.New(time.Duration(cfg.MessageBucketHours)*time.Hour), + cfg.MessageReadMaxBuckets, + ) + + // Graph client backs the meetings RPC. Constructed only when the Azure app + // credentials are present; otherwise the meetings RPC reports not-configured + // while the deep-link RPCs keep working. + var graphClient msgraph.Client + if cfg.TeamsTenantID != "" && cfg.TeamsClientID != "" && cfg.TeamsClientSecret != "" { + graphClient = msgraph.New(msgraph.Config{ + TenantID: cfg.TeamsTenantID, + ClientID: cfg.TeamsClientID, + ClientSecret: cfg.TeamsClientSecret, + }) + } + // Eager at-rest DEK provisioning: when enabled, room creation provisions // the room's wrapped DEK so the first message write doesn't pay the create // cost. message-worker's lazy creation remains the fallback for remote @@ -179,6 +221,11 @@ func main() { nc.NatsConn().MaxPayload(), ) handler.dekProvisioner = dekProvisioner + handler.graphClient = graphClient + handler.meetMarkerReader = meetMarkerReader + handler.teamsEmailDomain = cfg.TeamsEmailDomain + handler.roomMembersLimit = cfg.RoomMembersLimit + handler.roomMembersCallLimit = cfg.RoomMembersCallLimit router := natsrouter.New(nc, "room-service") router.Use(natsrouter.Recovery(), natsrouter.RequestID(), natsrouter.Logging()) diff --git a/room-service/mock_store_test.go b/room-service/mock_store_test.go index 70dcd5d57..33db4587c 100644 --- a/room-service/mock_store_test.go +++ b/room-service/mock_store_test.go @@ -727,3 +727,43 @@ func (mr *MockMessageReaderMockRecorder) GetMessageRoomAndCreatedAt(ctx, message mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMessageRoomAndCreatedAt", reflect.TypeOf((*MockMessageReader)(nil).GetMessageRoomAndCreatedAt), ctx, messageID) } + +// MockMeetMarkerReader is a mock of MeetMarkerReader interface. +type MockMeetMarkerReader struct { + ctrl *gomock.Controller + recorder *MockMeetMarkerReaderMockRecorder + isgomock struct{} +} + +// MockMeetMarkerReaderMockRecorder is the mock recorder for MockMeetMarkerReader. +type MockMeetMarkerReaderMockRecorder struct { + mock *MockMeetMarkerReader +} + +// NewMockMeetMarkerReader creates a new mock instance. +func NewMockMeetMarkerReader(ctrl *gomock.Controller) *MockMeetMarkerReader { + mock := &MockMeetMarkerReader{ctrl: ctrl} + mock.recorder = &MockMeetMarkerReaderMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockMeetMarkerReader) EXPECT() *MockMeetMarkerReaderMockRecorder { + return m.recorder +} + +// GetLastTeamsMeetStarted mocks base method. +func (m *MockMeetMarkerReader) GetLastTeamsMeetStarted(ctx context.Context, roomID string) (*model.TeamsMeetStartedSysData, bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetLastTeamsMeetStarted", ctx, roomID) + ret0, _ := ret[0].(*model.TeamsMeetStartedSysData) + ret1, _ := ret[1].(bool) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// GetLastTeamsMeetStarted indicates an expected call of GetLastTeamsMeetStarted. +func (mr *MockMeetMarkerReaderMockRecorder) GetLastTeamsMeetStarted(ctx, roomID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLastTeamsMeetStarted", reflect.TypeOf((*MockMeetMarkerReader)(nil).GetLastTeamsMeetStarted), ctx, roomID) +} diff --git a/room-service/store.go b/room-service/store.go index 5dc6c8a81..00b8f7cc8 100644 --- a/room-service/store.go +++ b/room-service/store.go @@ -227,3 +227,14 @@ type MessageReader interface { roomID string, createdAt time.Time, senderAccount string, found bool, err error, ) } + +// MeetMarkerReader reads back the most-recent teams_meet_started system message +// for a room, used to make the meetings RPC idempotent (a second call returns +// the existing meeting instead of creating a duplicate Graph onlineMeeting). +// found=false with err=nil means the room has no meeting marker. All reads are +// keyspace-aware (the gocql session is bound to CASSANDRA_KEYSPACE). +type MeetMarkerReader interface { + GetLastTeamsMeetStarted(ctx context.Context, roomID string) ( + marker *model.TeamsMeetStartedSysData, found bool, err error, + ) +} diff --git a/room-service/store_cassandra.go b/room-service/store_cassandra.go index 26b648703..967e8cc6a 100644 --- a/room-service/store_cassandra.go +++ b/room-service/store_cassandra.go @@ -2,11 +2,15 @@ package main import ( "context" + "encoding/json" "errors" "fmt" "time" "github.com/gocql/gocql" + + "github.com/hmchangw/chat/pkg/model" + "github.com/hmchangw/chat/pkg/msgbucket" ) type cassMessageReader struct { @@ -39,3 +43,53 @@ func (r *cassMessageReader) GetMessageRoomAndCreatedAt( } return roomID, createdAt, senderAccount, true, nil } + +// cassMeetMarkerReader reads the latest teams_meet_started system message for a +// room. messages_by_room is partitioned by (room_id, bucket) and clustered by +// created_at, so we walk buckets from "now" backwards (newest first) and, within +// each bucket, take the newest row whose type is teams_meet_started. The first +// match is the most recent meeting for the room. The walk is bounded by +// maxBuckets so a room that never had a meeting doesn't scan unboundedly. +// +// The query never names a keyspace — the gocql session is bound to +// CASSANDRA_KEYSPACE at connect time (cassutil), so this is keyspace-aware. +type cassMeetMarkerReader struct { + session *gocql.Session + bucket msgbucket.Sizer + maxBuckets int +} + +func NewCassMeetMarkerReader(session *gocql.Session, bucket msgbucket.Sizer, maxBuckets int) *cassMeetMarkerReader { + return &cassMeetMarkerReader{session: session, bucket: bucket, maxBuckets: maxBuckets} +} + +const meetMarkerQuery = `SELECT sys_msg_data FROM messages_by_room ` + + `WHERE room_id = ? AND bucket = ? AND type = ? ORDER BY created_at DESC LIMIT 1 ALLOW FILTERING` + +func (r *cassMeetMarkerReader) GetLastTeamsMeetStarted( + ctx context.Context, + roomID string, +) (*model.TeamsMeetStartedSysData, bool, error) { + bucket := r.bucket.Of(time.Now().UTC()) + for i := 0; i < r.maxBuckets; i++ { + var sysMsgData []byte + err := r.session.Query(meetMarkerQuery, roomID, bucket, model.MessageTypeTeamsMeetStarted). + WithContext(ctx). + Scan(&sysMsgData) + if err != nil { + if errors.Is(err, gocql.ErrNotFound) { + bucket = r.bucket.Prev(bucket) + continue + } + return nil, false, fmt.Errorf("read teams_meet_started marker for room %s: %w", roomID, err) + } + var marker model.TeamsMeetStartedSysData + if len(sysMsgData) > 0 { + if err := json.Unmarshal(sysMsgData, &marker); err != nil { + return nil, false, fmt.Errorf("decode teams_meet_started marker for room %s: %w", roomID, err) + } + } + return &marker, true, nil + } + return nil, false, nil +} From c208ca67634822b160a43e80d55edd6c804efc31 Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Mon, 15 Jun 2026 08:50:50 +0000 Subject: [PATCH 2/9] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20sanit?= =?UTF-8?q?ize=20Graph=20errors,=20validate=20subject=20tokens,=20doc=20ra?= =?UTF-8?q?ce?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pkg/msgraph: parse the Graph error envelope and surface only status + error.code; never wrap the raw response body into the error/cause. - pkg/subject: validate the account token (reject NATS wildcards) in the Teams concrete subject builders, matching the existing room-scoped builders. - docs/client-api.md: clarify teams_meet_started is emitted only on first creation, not on idempotent cache hits. - room-service: document the known read-then-create idempotency race at the meetings handler (eventually-idempotent via the marker; no lock primitive in this layer — left for maintainer scope decision). Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/client-api.md | 3 ++- pkg/msgraph/msgraph.go | 15 ++++++++++++++- pkg/msgraph/msgraph_test.go | 5 ++++- pkg/subject/subject.go | 15 +++++++++++++++ room-service/handler_teams.go | 12 +++++++++++- 5 files changed, 46 insertions(+), 4 deletions(-) diff --git a/docs/client-api.md b/docs/client-api.md index cc84ff9c6..baca0c136 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -1954,7 +1954,8 @@ See [Error envelope](#6-error-envelope-reference). ##### Triggered events — success path -A `teams_meet_started` system message is published on the canonical message path (`chat.msg.canonical.{siteID}.created`), persisted by `message-worker`, and fanned out to room members like other system messages. Its `sysMsgData` carries `{ "meetingId": "...", "joinUrl": "..." }`. +On first creation, a `teams_meet_started` system message is published on the canonical message path (`chat.msg.canonical.{siteID}.created`), persisted by `message-worker`, and fanned out to room members like other system messages. Its `sysMsgData` carries `{ "meetingId": "...", "joinUrl": "..." }`. +On idempotent repeat calls that return cached meeting details, no additional system message is published. ##### Triggered events — error path diff --git a/pkg/msgraph/msgraph.go b/pkg/msgraph/msgraph.go index d89fc40be..91b462962 100644 --- a/pkg/msgraph/msgraph.go +++ b/pkg/msgraph/msgraph.go @@ -225,7 +225,20 @@ func (g *graphClient) CreateOnlineMeeting(ctx context.Context, req CreateOnlineM return nil, fmt.Errorf("read onlineMeeting response: %w", err) } if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("create onlineMeeting: graph returned status %d: %s", resp.StatusCode, string(respBody)) + // Never wrap the raw response body into the error/cause — it can carry + // upstream payload. Parse the Graph error envelope and surface only the + // status + sanitized error code. + var graphErr struct { + Error struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + _ = json.Unmarshal(respBody, &graphErr) + if graphErr.Error.Code != "" { + return nil, fmt.Errorf("create onlineMeeting: graph returned status %d (%s)", resp.StatusCode, graphErr.Error.Code) + } + return nil, fmt.Errorf("create onlineMeeting: graph returned status %d", resp.StatusCode) } var meeting OnlineMeeting diff --git a/pkg/msgraph/msgraph_test.go b/pkg/msgraph/msgraph_test.go index 9eb7efc20..ea9e0d8d7 100644 --- a/pkg/msgraph/msgraph_test.go +++ b/pkg/msgraph/msgraph_test.go @@ -84,7 +84,8 @@ func TestCreateOnlineMeeting_GraphError(t *testing.T) { defer tokenSrv.Close() graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusForbidden) - _, _ = w.Write([]byte(`{"error":{"code":"Forbidden"}}`)) + // The message carries sensitive-looking detail that must NOT leak into the error. + _, _ = w.Write([]byte(`{"error":{"code":"Forbidden","message":"secret-internal-detail-xyz"}}`)) })) defer graphSrv.Close() @@ -92,6 +93,8 @@ func TestCreateOnlineMeeting_GraphError(t *testing.T) { _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) require.Error(t, err) assert.Contains(t, err.Error(), "403") + assert.Contains(t, err.Error(), "Forbidden", "sanitized error code should be surfaced") + assert.NotContains(t, err.Error(), "secret-internal-detail-xyz", "raw response message must not leak") } func TestCreateOnlineMeeting_MissingJoinURL(t *testing.T) { diff --git a/pkg/subject/subject.go b/pkg/subject/subject.go index d59c469ea..aa1a0cb30 100644 --- a/pkg/subject/subject.go +++ b/pkg/subject/subject.go @@ -766,7 +766,12 @@ func RoomAppCmdMenuPattern(siteID string) string { // 1:1 deep-link builder with no room; the target account travels in the body. // TeamsRoomCall returns the concrete subject for the room-call deep-link RPC. +// Panics if account contains a NATS wildcard, matching the other room-scoped +// concrete builders (e.g. RoomAppTabs, MsgHistory). func TeamsRoomCall(account, roomID, siteID string) string { + if !isValidAccountToken(account) { + panic("invalid account token: contains NATS wildcard characters") + } return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.call", account, roomID, siteID) } @@ -776,7 +781,12 @@ func TeamsRoomCallPattern(siteID string) string { } // TeamsMeeting returns the concrete subject for the Graph onlineMeeting RPC. +// Panics if account contains a NATS wildcard, matching the other room-scoped +// concrete builders. func TeamsMeeting(account, roomID, siteID string) string { + if !isValidAccountToken(account) { + panic("invalid account token: contains NATS wildcard characters") + } return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.meeting", account, roomID, siteID) } @@ -786,7 +796,12 @@ func TeamsMeetingPattern(siteID string) string { } // TeamsUserCall returns the concrete subject for the 1:1 user-call deep-link RPC. +// Panics if account contains a NATS wildcard, matching the other concrete +// account-scoped builders. func TeamsUserCall(account, siteID string) string { + if !isValidAccountToken(account) { + panic("invalid account token: contains NATS wildcard characters") + } return fmt.Sprintf("chat.user.%s.request.teams.%s.call.user", account, siteID) } diff --git a/room-service/handler_teams.go b/room-service/handler_teams.go index 91cee50c5..31bfc33ca 100644 --- a/room-service/handler_teams.go +++ b/room-service/handler_teams.go @@ -114,7 +114,17 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques } // Idempotency: a prior teams_meet_started marker short-circuits the Graph - // create so retries (and concurrent clients) reuse the same meeting. + // create so retries (and most concurrent clients) reuse the same meeting. + // + // Known limitation (read-then-create race): the marker is written via the + // canonical message path, which is asynchronous, so two callers racing + // before the first marker materializes can each create a Graph meeting. This + // mirrors the previous GetLastMeetStartedMessage behavior — there is no CAS/ + // LWT lock primitive in this layer to make the create atomic, and adding one + // is a larger design decision left to the maintainer. The marker makes the + // endpoint eventually-idempotent: once a marker is visible, all subsequent + // calls reuse it. The blast radius of the race is a duplicate Teams meeting, + // not data corruption. marker, found, err := h.meetMarkerReader.GetLastTeamsMeetStarted(ctx, roomID) if err != nil { return nil, fmt.Errorf("read meeting marker: %w", err) From d4da90bfee2dbbe0041a5006d4e4a4778602b940 Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Mon, 15 Jun 2026 09:18:23 +0000 Subject: [PATCH 3/9] refactor: Teams subject builders return error instead of panicking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per the shared-pkg no-panic policy (F12 / #224 / #215): TeamsRoomCall, TeamsMeeting, and TeamsUserCall now return (string, error) and return an error on an invalid (NATS-wildcard) account token instead of panicking. The wildcard check itself is unchanged. These concrete builders have no callers yet (only the *Pattern variants are wired), so there is no cascade. Sibling builders (RoomAppTabs, etc.) are left as-is — new code adopts the preferred form. Adds subject tests for the happy path and the error path. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/subject/subject.go | 31 +++++++++++++------------ pkg/subject/subject_test.go | 46 +++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/pkg/subject/subject.go b/pkg/subject/subject.go index aa1a0cb30..70298fb8b 100644 --- a/pkg/subject/subject.go +++ b/pkg/subject/subject.go @@ -766,13 +766,14 @@ func RoomAppCmdMenuPattern(siteID string) string { // 1:1 deep-link builder with no room; the target account travels in the body. // TeamsRoomCall returns the concrete subject for the room-call deep-link RPC. -// Panics if account contains a NATS wildcard, matching the other room-scoped -// concrete builders (e.g. RoomAppTabs, MsgHistory). -func TeamsRoomCall(account, roomID, siteID string) string { +// Returns an error if account contains a NATS wildcard. Shared pkg/ code must +// not panic on bad input (F12), so this returns the error rather than panicking +// like the older sibling builders (e.g. RoomAppTabs, MsgHistory). +func TeamsRoomCall(account, roomID, siteID string) (string, error) { if !isValidAccountToken(account) { - panic("invalid account token: contains NATS wildcard characters") + return "", fmt.Errorf("invalid account token: contains NATS wildcard characters") } - return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.call", account, roomID, siteID) + return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.call", account, roomID, siteID), nil } // TeamsRoomCallPattern is the natsrouter registration pattern for the room-call RPC. @@ -781,13 +782,13 @@ func TeamsRoomCallPattern(siteID string) string { } // TeamsMeeting returns the concrete subject for the Graph onlineMeeting RPC. -// Panics if account contains a NATS wildcard, matching the other room-scoped -// concrete builders. -func TeamsMeeting(account, roomID, siteID string) string { +// Returns an error if account contains a NATS wildcard. Shared pkg/ code must +// not panic on bad input (F12). +func TeamsMeeting(account, roomID, siteID string) (string, error) { if !isValidAccountToken(account) { - panic("invalid account token: contains NATS wildcard characters") + return "", fmt.Errorf("invalid account token: contains NATS wildcard characters") } - return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.meeting", account, roomID, siteID) + return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.meeting", account, roomID, siteID), nil } // TeamsMeetingPattern is the natsrouter registration pattern for the meetings RPC. @@ -796,13 +797,13 @@ func TeamsMeetingPattern(siteID string) string { } // TeamsUserCall returns the concrete subject for the 1:1 user-call deep-link RPC. -// Panics if account contains a NATS wildcard, matching the other concrete -// account-scoped builders. -func TeamsUserCall(account, siteID string) string { +// Returns an error if account contains a NATS wildcard. Shared pkg/ code must +// not panic on bad input (F12). +func TeamsUserCall(account, siteID string) (string, error) { if !isValidAccountToken(account) { - panic("invalid account token: contains NATS wildcard characters") + return "", fmt.Errorf("invalid account token: contains NATS wildcard characters") } - return fmt.Sprintf("chat.user.%s.request.teams.%s.call.user", account, siteID) + return fmt.Sprintf("chat.user.%s.request.teams.%s.call.user", account, siteID), nil } // TeamsUserCallPattern is the natsrouter registration pattern for the user-call RPC. diff --git a/pkg/subject/subject_test.go b/pkg/subject/subject_test.go index 0d7ab14de..1561e84b9 100644 --- a/pkg/subject/subject_test.go +++ b/pkg/subject/subject_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/hmchangw/chat/pkg/subject" ) @@ -890,3 +891,48 @@ func TestPresenceSubjects(t *testing.T) { assert.Equal(t, "chat.server.request.presence.site-a.query.batch", subject.PresenceQueryBatchPeer("site-a")) assert.Equal(t, "chat.user.presence.state.alice", subject.PresenceState("alice")) } + +func TestTeamsSubjectBuilders(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + got, err := subject.TeamsRoomCall("alice", "r1", "site-a") + require.NoError(t, err) + assert.Equal(t, "chat.user.alice.request.room.r1.site-a.teams.call", got) + + got, err = subject.TeamsMeeting("alice", "r1", "site-a") + require.NoError(t, err) + assert.Equal(t, "chat.user.alice.request.room.r1.site-a.teams.meeting", got) + + got, err = subject.TeamsUserCall("alice", "site-a") + require.NoError(t, err) + assert.Equal(t, "chat.user.alice.request.teams.site-a.call.user", got) + }) + + t.Run("wildcard account rejected with error (no panic)", func(t *testing.T) { + got, err := subject.TeamsRoomCall("*", "r1", "site-a") + require.Error(t, err) + assert.Empty(t, got) + + got, err = subject.TeamsMeeting(">", "r1", "site-a") + require.Error(t, err) + assert.Empty(t, got) + + got, err = subject.TeamsUserCall("*", "site-a") + require.Error(t, err) + assert.Empty(t, got) + }) + + t.Run("empty account rejected with error", func(t *testing.T) { + _, err := subject.TeamsRoomCall("", "r1", "site-a") + require.Error(t, err) + _, err = subject.TeamsMeeting("", "r1", "site-a") + require.Error(t, err) + _, err = subject.TeamsUserCall("", "site-a") + require.Error(t, err) + }) + + t.Run("pattern builders", func(t *testing.T) { + assert.Equal(t, "chat.user.{account}.request.room.{roomID}.site-a.teams.call", subject.TeamsRoomCallPattern("site-a")) + assert.Equal(t, "chat.user.{account}.request.room.{roomID}.site-a.teams.meeting", subject.TeamsMeetingPattern("site-a")) + assert.Equal(t, "chat.user.{account}.request.teams.site-a.call.user", subject.TeamsUserCallPattern("site-a")) + }) +} From af7a4ba96cbd9726213576fae45fb4f90e4d30a1 Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Tue, 16 Jun 2026 01:59:08 +0000 Subject: [PATCH 4/9] fix: make teams.meeting idempotent under concurrency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concurrent teams.meeting calls for the same room could each create a Graph onlineMeeting and each publish a teams_meet_started system message: the prior idempotency read scanned message buckets for the last marker, which is written asynchronously, so racing callers saw no marker yet. Push idempotency to two cooperating layers: - pkg/msgraph: switch onlineMeeting creation to Graph's createOrGet endpoint with a stable per-room externalId (siteID:roomID). Graph guarantees one meeting per (organizer, externalId), so concurrent calls return the same meeting — correctness across restarts/instances with no local lock. externalId is now required. - room-service: replace the Cassandra message-bucket marker scan with a first-class teams_meetings Mongo record, UNIQUE on (roomId, siteId). This mirrors the room_members / subscriptions retry-safe-write convention (unique index + IsDuplicateKeyError == success). The meetings handler now: fast-path reads the record, else createOrGet, then inserts the record; on a duplicate-key race the loser reads back the winner's record and returns it WITHOUT publishing a second system message. The teams_meet_started message is published only by the insert winner — exactly once per room. Removes the now-unused cassMeetMarkerReader and its MESSAGE_BUCKET_HOURS / MESSAGE_READ_MAX_BUCKETS config (room-service no longer scans buckets for meetings). Tests: msgraph httptest asserts the createOrGet path + externalId in the body and that a repeat returns the same meeting; room-service covers fresh create, fast-path read-hit, duplicate-key read-back, and a race-detector concurrent test asserting one Graph meeting + one system message across 8 goroutines. Co-Authored-By: Claude --- docs/client-api.md | 2 +- pkg/model/teams.go | 15 ++ pkg/msgraph/msgraph.go | 32 ++- pkg/msgraph/msgraph_test.go | 74 +++++- room-service/deploy/docker-compose.yml | 3 - room-service/handler.go | 9 +- room-service/handler_teams.go | 89 ++++++-- room-service/handler_teams_test.go | 303 ++++++++++++++++++++++--- room-service/main.go | 21 +- room-service/mock_store_test.go | 50 ++-- room-service/store.go | 23 +- room-service/store_cassandra.go | 54 ----- room-service/store_mongo.go | 41 ++++ 13 files changed, 537 insertions(+), 179 deletions(-) diff --git a/docs/client-api.md b/docs/client-api.md index baca0c136..e5a4e3c61 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -1911,7 +1911,7 @@ See [Error envelope](#6-error-envelope-reference). #### Start Teams Meeting -Creates a Microsoft Teams `onlineMeeting` via the Graph API and returns its join URL. **Idempotent per room:** the most recent `teams_meet_started` system message is the source of truth, so a second call returns the existing meeting without creating a duplicate. Attendee emails are derived as `account@TEAMS_EMAIL_DOMAIN`. +Creates a Microsoft Teams `onlineMeeting` via the Graph API and returns its join URL. **Idempotent per room, including under concurrency:** the meeting is created via Graph's `createOrGet` endpoint keyed on a stable per-room `externalId`, and a first-class `teams_meetings` record with a unique key on `(roomId, siteId)` guards local state. Repeated or concurrent calls for the same room return the same meeting and publish exactly one `teams_meet_started` system message. Attendee emails are derived as `account@TEAMS_EMAIL_DOMAIN`. External client label: `POST /api/v1/meetings`. diff --git a/pkg/model/teams.go b/pkg/model/teams.go index 4bdc28269..40062809c 100644 --- a/pkg/model/teams.go +++ b/pkg/model/teams.go @@ -38,3 +38,18 @@ type TeamsMeetingReply struct { ID string `json:"id"` JoinURL string `json:"joinUrl"` } + +// TeamsMeetingRecord is the first-class persisted record of a room's Teams +// meeting in the teams_meetings collection. A UNIQUE index on (roomId, siteId) +// makes the meetings RPC retry-safe: a concurrent second create hits a +// duplicate-key error, and the loser reads back the winner's record instead of +// creating a duplicate system message. This is the same unique-index + +// IsDuplicateKeyError idempotency convention room-service already uses for +// room_members and subscriptions. +type TeamsMeetingRecord struct { + RoomID string `bson:"roomId"` + SiteID string `bson:"siteId"` + MeetingID string `bson:"meetingId"` + JoinURL string `bson:"joinUrl"` + CreatedAt int64 `bson:"createdAt"` +} diff --git a/pkg/msgraph/msgraph.go b/pkg/msgraph/msgraph.go index 91b462962..082d00768 100644 --- a/pkg/msgraph/msgraph.go +++ b/pkg/msgraph/msgraph.go @@ -21,13 +21,22 @@ import ( // Client is the Graph surface room-service depends on. Only the meetings RPC // touches Graph, so this is intentionally tiny. Mocked in tests. type Client interface { - // CreateOnlineMeeting creates an onlineMeeting on behalf of the configured - // organizer and returns its ID and join URL. + // CreateOnlineMeeting creates (or returns the existing) onlineMeeting on + // behalf of the configured organizer and returns its ID and join URL. It + // uses Graph's idempotent createOrGet endpoint keyed on req.ExternalID, so + // concurrent or repeated calls with the same ExternalID return the same + // meeting — Graph itself is the idempotency source of truth. CreateOnlineMeeting(ctx context.Context, req CreateOnlineMeetingRequest) (*OnlineMeeting, error) } // CreateOnlineMeetingRequest carries the attributes used to create a meeting. type CreateOnlineMeetingRequest struct { + // ExternalID is the stable per-room idempotency key passed to Graph's + // createOrGet endpoint. Graph guarantees exactly one meeting per + // (organizer, externalId), so repeated/concurrent calls with the same + // ExternalID return the same meeting instead of creating duplicates. + // Required: createOrGet rejects an empty externalId. + ExternalID string // Subject is the meeting title shown in Teams. Subject string // OrganizerEmail is the user the meeting is created for (the organizer). @@ -163,8 +172,10 @@ func (g *graphClient) accessToken(ctx context.Context) (string, error) { return g.token, nil } -// onlineMeetingPayload is the Graph create-onlineMeeting request body. +// onlineMeetingPayload is the Graph createOrGet-onlineMeeting request body. +// externalId is required by createOrGet and is the per-room idempotency key. type onlineMeetingPayload struct { + ExternalID string `json:"externalId"` Subject string `json:"subject,omitempty"` Participants *meetingParticipants `json:"participants,omitempty"` } @@ -178,22 +189,27 @@ type meetingAttendee struct { } func (g *graphClient) CreateOnlineMeeting(ctx context.Context, req CreateOnlineMeetingRequest) (*OnlineMeeting, error) { + if req.ExternalID == "" { + return nil, fmt.Errorf("create onlineMeeting: externalId is required for createOrGet idempotency") + } token, err := g.accessToken(ctx) if err != nil { return nil, fmt.Errorf("acquire graph token: %w", err) } + // createOrGet pushes idempotency to Graph: it returns the existing meeting + // for an (organizer, externalId) pair if one exists, otherwise creates one. // App-only context requires targeting a specific organizer mailbox via the - // /users/{id}/onlineMeetings path; delegated context uses /me. We use the - // organizer-scoped path when an organizer email is supplied. + // /users/{id}/onlineMeetings/createOrGet path; delegated context uses /me. + // We use the organizer-scoped path when an organizer email is supplied. var endpoint string if req.OrganizerEmail != "" { - endpoint = fmt.Sprintf("%s/users/%s/onlineMeetings", g.baseURL, url.PathEscape(req.OrganizerEmail)) + endpoint = fmt.Sprintf("%s/users/%s/onlineMeetings/createOrGet", g.baseURL, url.PathEscape(req.OrganizerEmail)) } else { - endpoint = g.baseURL + "/me/onlineMeetings" + endpoint = g.baseURL + "/me/onlineMeetings/createOrGet" } - payload := onlineMeetingPayload{Subject: req.Subject} + payload := onlineMeetingPayload{ExternalID: req.ExternalID, Subject: req.Subject} if len(req.AttendeeEmails) > 0 { attendees := make([]meetingAttendee, 0, len(req.AttendeeEmails)) for _, email := range req.AttendeeEmails { diff --git a/pkg/msgraph/msgraph_test.go b/pkg/msgraph/msgraph_test.go index ea9e0d8d7..e94ab0414 100644 --- a/pkg/msgraph/msgraph_test.go +++ b/pkg/msgraph/msgraph_test.go @@ -36,9 +36,14 @@ func TestCreateOnlineMeeting_Success(t *testing.T) { graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { meetingCalls++ assert.Equal(t, "Bearer tok-123", r.Header.Get("Authorization")) - assert.True(t, strings.Contains(r.URL.Path, "/users/alice%40corp.com/onlineMeetings") || - strings.Contains(r.URL.Path, "/users/alice@corp.com/onlineMeetings"), - "organizer-scoped path expected, got %s", r.URL.Path) + // Idempotent endpoint: the organizer-scoped createOrGet path. + assert.True(t, strings.Contains(r.URL.Path, "/users/alice%40corp.com/onlineMeetings/createOrGet") || + strings.Contains(r.URL.Path, "/users/alice@corp.com/onlineMeetings/createOrGet"), + "organizer-scoped createOrGet path expected, got %s", r.URL.Path) + // externalId is the per-room idempotency key and must be sent. + var body onlineMeetingPayload + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + assert.Equal(t, "room-key-1", body.ExternalID, "externalId must be sent to createOrGet") w.WriteHeader(http.StatusCreated) _ = json.NewEncoder(w).Encode(OnlineMeeting{ID: "m1", JoinURL: "https://join/1"}) })) @@ -46,19 +51,72 @@ func TestCreateOnlineMeeting_Success(t *testing.T) { c := newTestClient(tokenSrv.URL, graphSrv.URL) m, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ - Subject: "Standup", OrganizerEmail: "alice@corp.com", AttendeeEmails: []string{"bob@corp.com"}, + ExternalID: "room-key-1", Subject: "Standup", OrganizerEmail: "alice@corp.com", AttendeeEmails: []string{"bob@corp.com"}, }) require.NoError(t, err) assert.Equal(t, "m1", m.ID) assert.Equal(t, "https://join/1", m.JoinURL) // Second call reuses the cached token (no second token fetch). - _, err = c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "alice@corp.com"}) + _, err = c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ExternalID: "room-key-1", OrganizerEmail: "alice@corp.com"}) require.NoError(t, err) assert.Equal(t, 1, tokenCalls, "token should be cached across calls") assert.Equal(t, 2, meetingCalls) } +// TestCreateOnlineMeeting_Idempotent_SameExternalID asserts the client hits +// createOrGet and that a repeat call with the same externalId returns the same +// meeting Graph already holds for that key (Graph is the idempotency source of +// truth — the server returns the existing meeting on the second createOrGet). +func TestCreateOnlineMeeting_Idempotent_SameExternalID(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) + })) + defer tokenSrv.Close() + + // Server mimics Graph createOrGet: one meeting per externalId, returned on + // every call with that key. + byExternalID := map[string]OnlineMeeting{} + graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Contains(t, r.URL.Path, "/onlineMeetings/createOrGet", "must use createOrGet endpoint") + var body onlineMeetingPayload + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + require.NotEmpty(t, body.ExternalID, "externalId required") + m, ok := byExternalID[body.ExternalID] + if !ok { + m = OnlineMeeting{ID: "mtg-" + body.ExternalID, JoinURL: "https://join/" + body.ExternalID} + byExternalID[body.ExternalID] = m + w.WriteHeader(http.StatusCreated) + } else { + w.WriteHeader(http.StatusOK) // existing meeting returned + } + _ = json.NewEncoder(w).Encode(m) + })) + defer graphSrv.Close() + + c := newTestClient(tokenSrv.URL, graphSrv.URL) + first, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ + ExternalID: "k", OrganizerEmail: "a@b.com", + }) + require.NoError(t, err) + second, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ + ExternalID: "k", OrganizerEmail: "a@b.com", + }) + require.NoError(t, err) + assert.Equal(t, first.ID, second.ID, "same externalId returns the same meeting") + assert.Equal(t, first.JoinURL, second.JoinURL) + assert.Len(t, byExternalID, 1, "only one meeting created for one externalId") +} + +// TestCreateOnlineMeeting_RequiresExternalID guards the createOrGet contract: +// an empty externalId is rejected before any network call. +func TestCreateOnlineMeeting_RequiresExternalID(t *testing.T) { + c := newTestClient("http://unused", "http://unused") + _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) // no ExternalID + require.Error(t, err) + assert.Contains(t, err.Error(), "externalId") +} + func TestCreateOnlineMeeting_TokenError(t *testing.T) { tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) @@ -70,7 +128,7 @@ func TestCreateOnlineMeeting_TokenError(t *testing.T) { Config{TenantID: "t", ClientID: "c", ClientSecret: "super-secret-value"}, WithTokenURL(tokenSrv.URL), WithBaseURL("http://unused"), ) - _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) + _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ExternalID: "k", OrganizerEmail: "a@b.com"}) require.Error(t, err) assert.Contains(t, err.Error(), "invalid_client") // Never leak the secret in the error. @@ -90,7 +148,7 @@ func TestCreateOnlineMeeting_GraphError(t *testing.T) { defer graphSrv.Close() c := newTestClient(tokenSrv.URL, graphSrv.URL) - _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) + _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ExternalID: "k", OrganizerEmail: "a@b.com"}) require.Error(t, err) assert.Contains(t, err.Error(), "403") assert.Contains(t, err.Error(), "Forbidden", "sanitized error code should be surfaced") @@ -109,7 +167,7 @@ func TestCreateOnlineMeeting_MissingJoinURL(t *testing.T) { defer graphSrv.Close() c := newTestClient(tokenSrv.URL, graphSrv.URL) - _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{OrganizerEmail: "a@b.com"}) + _, err := c.CreateOnlineMeeting(context.Background(), CreateOnlineMeetingRequest{ExternalID: "k", OrganizerEmail: "a@b.com"}) require.Error(t, err) assert.Contains(t, err.Error(), "joinWebUrl") } diff --git a/room-service/deploy/docker-compose.yml b/room-service/deploy/docker-compose.yml index cad818a9b..aa4c1c388 100644 --- a/room-service/deploy/docker-compose.yml +++ b/room-service/deploy/docker-compose.yml @@ -65,9 +65,6 @@ services: - TEAMS_CLIENT_SECRET= - ROOM_MEMBERS_LIMIT=500 - ROOM_MEMBERS_CALL_LIMIT=20 - # Must match message-worker's MESSAGE_BUCKET_HOURS so the teams_meet_started - # idempotency read walks the same partitions writes land in. - - MESSAGE_BUCKET_HOURS=72 - BOOTSTRAP_STREAMS=true - ATREST_ENABLED=true - VAULT_ADDR=http://vault:8200 diff --git a/room-service/handler.go b/room-service/handler.go index 53ba53381..b6e215983 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -56,11 +56,12 @@ type Handler struct { // Microsoft Teams integration. graphClient is nil-safe: only the meetings // RPC uses it (the deep-link RPCs are pure string building). teamsEmailDomain - // derives a member's email as account@domain. meetMarkerReader backs the - // per-room idempotency read. roomMembersLimit / roomMembersCallLimit cap the - // member set for meetings and calls respectively. + // derives a member's email as account@domain. teamsMeetingStore backs the + // per-room idempotency record (Mongo unique key on roomId+siteId). + // roomMembersLimit / roomMembersCallLimit cap the member set for meetings and + // calls respectively. graphClient msgraph.Client - meetMarkerReader MeetMarkerReader + teamsMeetingStore TeamsMeetingStore teamsEmailDomain string roomMembersLimit int roomMembersCallLimit int diff --git a/room-service/handler_teams.go b/room-service/handler_teams.go index 31bfc33ca..36082c1f2 100644 --- a/room-service/handler_teams.go +++ b/room-service/handler_teams.go @@ -8,6 +8,8 @@ import ( "strings" "time" + "go.mongodb.org/mongo-driver/v2/mongo" + "github.com/hmchangw/chat/pkg/displayfmt" "github.com/hmchangw/chat/pkg/idgen" "github.com/hmchangw/chat/pkg/model" @@ -89,9 +91,23 @@ func (h *Handler) teamsUserCall(c *natsrouter.Context, req model.TeamsUserCallRe } // teamsMeeting creates (or returns the existing) Microsoft Teams onlineMeeting -// for the room. Idempotent per room: the last teams_meet_started system message -// is the source of truth, so a second call returns the prior meeting without a -// duplicate Graph create. Enforces roomMembersLimit. +// for the room. Idempotent per room under concurrency via two cooperating +// guarantees: +// +// 1. Graph createOrGet keyed on a stable per-room externalId — Graph returns +// the same meeting for repeated/concurrent calls with the same key, so even +// a true race never produces two distinct Graph meetings (the correctness +// guarantee across restarts and multiple service instances). +// 2. A teams_meetings Mongo record with a UNIQUE index on (roomId, siteId) — +// this is the local first-class idempotency record (mirrors the +// room_members / subscriptions retry-safe-write convention). On a concurrent +// insert race the loser hits a duplicate-key error and reads back the +// winner's record, so exactly ONE teams_meet_started system message is ever +// published per room. +// +// Flow: fast-path read the record → if present, return it → else createOrGet → +// insert the record → on dup-key, read back the winner → publish the system +// message only when THIS call created the record. Enforces roomMembersLimit. func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingRequest) (*model.TeamsMeetingReply, error) { //nolint:gocritic // hugeParam: req passed by value to satisfy natsrouter.Register var ctx context.Context = c requestID := natsutil.RequestIDFromContext(c) @@ -104,7 +120,7 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques if roomID == "" { return nil, errTeamsRoomIDRequired } - if h.graphClient == nil || h.meetMarkerReader == nil { + if h.graphClient == nil || h.teamsMeetingStore == nil { return nil, errTeamsNotConfigured } @@ -113,24 +129,11 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques return nil, err } - // Idempotency: a prior teams_meet_started marker short-circuits the Graph - // create so retries (and most concurrent clients) reuse the same meeting. - // - // Known limitation (read-then-create race): the marker is written via the - // canonical message path, which is asynchronous, so two callers racing - // before the first marker materializes can each create a Graph meeting. This - // mirrors the previous GetLastMeetStartedMessage behavior — there is no CAS/ - // LWT lock primitive in this layer to make the create atomic, and adding one - // is a larger design decision left to the maintainer. The marker makes the - // endpoint eventually-idempotent: once a marker is visible, all subsequent - // calls reuse it. The blast radius of the race is a duplicate Teams meeting, - // not data corruption. - marker, found, err := h.meetMarkerReader.GetLastTeamsMeetStarted(ctx, roomID) - if err != nil { - return nil, fmt.Errorf("read meeting marker: %w", err) - } - if found && marker != nil && marker.JoinURL != "" { - return &model.TeamsMeetingReply{ID: marker.MeetingID, JoinURL: marker.JoinURL}, nil + // (a) Fast-path: an existing record short-circuits Graph + insert + publish. + if rec, found, err := h.teamsMeetingStore.GetTeamsMeeting(ctx, roomID, h.siteID); err != nil { + return nil, fmt.Errorf("read teams meeting record: %w", err) + } else if found && rec != nil && rec.JoinURL != "" { + return &model.TeamsMeetingReply{ID: rec.MeetingID, JoinURL: rec.JoinURL}, nil } members, err := h.store.ListRoomMembers(ctx, roomID, nil, nil, false) @@ -144,7 +147,10 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques attendeeEmails := membersToAttendeeEmails(members, h.teamsEmailDomain) organizerEmail := teamsEmail(requesterAccount, h.teamsEmailDomain) + // (b) Graph createOrGet keyed on the stable per-room externalId. Even on a + // race both callers get the SAME meeting back from Graph. meeting, err := h.graphClient.CreateOnlineMeeting(ctx, msgraph.CreateOnlineMeetingRequest{ + ExternalID: teamsMeetingExternalID(h.siteID, roomID), Subject: meetingSubject(room), OrganizerEmail: organizerEmail, AttendeeEmails: attendeeEmails, @@ -153,6 +159,37 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques return nil, fmt.Errorf("create online meeting: %w", err) } + // (c) Insert the idempotency record. The (roomId, siteId) unique index makes + // this the gate: on a concurrent insert race the loser hits a duplicate-key + // error, reads back the winner's record, and returns WITHOUT publishing a + // second system message. + record := model.TeamsMeetingRecord{ + RoomID: roomID, + SiteID: h.siteID, + MeetingID: meeting.ID, + JoinURL: meeting.JoinURL, + CreatedAt: time.Now().UTC().UnixMilli(), + } + if err := h.teamsMeetingStore.InsertTeamsMeeting(ctx, record); err != nil { + if mongo.IsDuplicateKeyError(err) { + // A concurrent winner already wrote the record — read it back and + // return its meeting. Do NOT publish: the winner publishes. + rec, found, readErr := h.teamsMeetingStore.GetTeamsMeeting(ctx, roomID, h.siteID) + if readErr != nil { + return nil, fmt.Errorf("read teams meeting record after duplicate key: %w", readErr) + } + if found && rec != nil && rec.JoinURL != "" { + return &model.TeamsMeetingReply{ID: rec.MeetingID, JoinURL: rec.JoinURL}, nil + } + // Extremely unlikely: dup-key but no readable record. Fall back to + // the Graph meeting (createOrGet already guaranteed it is the same one). + return &model.TeamsMeetingReply{ID: meeting.ID, JoinURL: meeting.JoinURL}, nil + } + return nil, fmt.Errorf("insert teams meeting record: %w", err) + } + + // (d) This call created the record — it is the unique winner, so publish the + // teams_meet_started system message exactly once. if err := h.publishTeamsMeetStarted(ctx, requestID, roomID, requesterAccount, meeting); err != nil { return nil, err } @@ -160,6 +197,14 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques return &model.TeamsMeetingReply{ID: meeting.ID, JoinURL: meeting.JoinURL}, nil } +// teamsMeetingExternalID builds the stable per-room idempotency key passed to +// Graph createOrGet (and matching the teams_meetings unique key dimensions). +// Scoped by siteID so the same roomID on different sites maps to distinct +// meetings. +func teamsMeetingExternalID(siteID, roomID string) string { + return siteID + ":" + roomID +} + // publishTeamsMeetStarted writes the teams_meet_started system message through // the canonical message path — the same flow room_restricted uses — so it is // persisted by message-worker and fanned out to room members. The marker is the diff --git a/room-service/handler_teams_test.go b/room-service/handler_teams_test.go index aae2ac6fd..cc200222d 100644 --- a/room-service/handler_teams_test.go +++ b/room-service/handler_teams_test.go @@ -6,10 +6,12 @@ import ( "errors" "net/url" "strings" + "sync" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.mongodb.org/mongo-driver/v2/mongo" "go.uber.org/mock/gomock" "github.com/hmchangw/chat/pkg/errcode" @@ -17,6 +19,14 @@ import ( "github.com/hmchangw/chat/pkg/msgraph" ) +// errStubDuplicateKey is a real mongo.WriteException carrying code 11000, so +// mongo.IsDuplicateKeyError(errStubDuplicateKey) == true — the handler's +// race-detection branch keys off exactly that, so the stub must produce an +// error the production code recognizes. +var errStubDuplicateKey = mongo.WriteException{ + WriteErrors: mongo.WriteErrors{{Index: 0, Code: 11000, Message: "E11000 duplicate key error"}}, +} + // fakeGraphClient is a hand-rolled msgraph.Client double. It records calls and // returns a canned meeting or error. callCount lets idempotency tests assert // that a second meetings call does NOT reach Graph. @@ -36,15 +46,109 @@ func (f *fakeGraphClient) CreateOnlineMeeting(_ context.Context, req msgraph.Cre return f.meeting, nil } -// stubMeetMarkerReader is a hand-rolled MeetMarkerReader double. -type stubMeetMarkerReader struct { - marker *model.TeamsMeetStartedSysData - found bool - err error +// stubTeamsMeetingStore is a hand-rolled TeamsMeetingStore double backed by an +// in-memory map keyed on (roomId, siteId). It enforces the (roomId, siteId) +// unique constraint just like the real Mongo unique index, so InsertTeamsMeeting +// returns a duplicate-key error on a second insert for the same key — letting +// the handler tests exercise the concurrent/dup-key path. +// +// getErr / forceDupErr let tests inject failures: getErr makes the fast-path +// read fail; forceDupErr makes the FIRST insert return a duplicate-key error +// (simulating a concurrent winner who inserted between this caller's fast-path +// read and its own insert) without a record yet being readable — exercising the +// race branch deterministically. +type stubTeamsMeetingStore struct { + mu sync.Mutex + records map[string]model.TeamsMeetingRecord + getErr error + // inserts counts successful + attempted inserts that reached the store. + insertAttempts int } -func (s *stubMeetMarkerReader) GetLastTeamsMeetStarted(_ context.Context, _ string) (*model.TeamsMeetStartedSysData, bool, error) { - return s.marker, s.found, s.err +func newStubTeamsMeetingStore() *stubTeamsMeetingStore { + return &stubTeamsMeetingStore{records: map[string]model.TeamsMeetingRecord{}} +} + +func teamsMeetingKey(roomID, siteID string) string { return siteID + "::" + roomID } + +func (s *stubTeamsMeetingStore) seed(rec model.TeamsMeetingRecord) { + s.records[teamsMeetingKey(rec.RoomID, rec.SiteID)] = rec +} + +func (s *stubTeamsMeetingStore) GetTeamsMeeting(_ context.Context, roomID, siteID string) (*model.TeamsMeetingRecord, bool, error) { + if s.getErr != nil { + return nil, false, s.getErr + } + s.mu.Lock() + defer s.mu.Unlock() + rec, ok := s.records[teamsMeetingKey(roomID, siteID)] + if !ok { + return nil, false, nil + } + r := rec + return &r, true, nil +} + +func (s *stubTeamsMeetingStore) InsertTeamsMeeting(_ context.Context, record model.TeamsMeetingRecord) error { + s.mu.Lock() + defer s.mu.Unlock() + s.insertAttempts++ + k := teamsMeetingKey(record.RoomID, record.SiteID) + if _, exists := s.records[k]; exists { + return errStubDuplicateKey // mimics mongo.IsDuplicateKeyError == true + } + s.records[k] = record + return nil +} + +// raceTeamsMeetingStore models the read-then-insert race deterministically: the +// fast-path GetTeamsMeeting (first read) misses, then InsertTeamsMeeting always +// collides (a concurrent winner inserted `winner` in between), and the post- +// dup-key read-back returns `winner`. +type raceTeamsMeetingStore struct { + winner model.TeamsMeetingRecord + firstRead bool +} + +func (s *raceTeamsMeetingStore) GetTeamsMeeting(_ context.Context, _, _ string) (*model.TeamsMeetingRecord, bool, error) { + if !s.firstRead { + s.firstRead = true + return nil, false, nil // fast-path miss + } + rec := s.winner + return &rec, true, nil // read-back after dup-key hit +} + +func (s *raceTeamsMeetingStore) InsertTeamsMeeting(_ context.Context, _ model.TeamsMeetingRecord) error { + return errStubDuplicateKey // a concurrent winner already inserted +} + +// createOrGetGraphStub mimics Graph createOrGet: one meeting per externalId, +// returned for every call with that key. Concurrency-safe. +type createOrGetGraphStub struct { + mu sync.Mutex + byExtID map[string]*msgraph.OnlineMeeting +} + +func newCreateOrGetGraphStub() *createOrGetGraphStub { + return &createOrGetGraphStub{byExtID: map[string]*msgraph.OnlineMeeting{}} +} + +func (g *createOrGetGraphStub) CreateOnlineMeeting(_ context.Context, req msgraph.CreateOnlineMeetingRequest) (*msgraph.OnlineMeeting, error) { + g.mu.Lock() + defer g.mu.Unlock() + if m, ok := g.byExtID[req.ExternalID]; ok { + return m, nil + } + m := &msgraph.OnlineMeeting{ID: "mtg-" + req.ExternalID, JoinURL: "https://teams.example/join/" + req.ExternalID} + g.byExtID[req.ExternalID] = m + return m, nil +} + +func (g *createOrGetGraphStub) distinctMeetings() int { + g.mu.Lock() + defer g.mu.Unlock() + return len(g.byExtID) } func indMember(account string) model.RoomMember { @@ -177,13 +281,14 @@ func TestTeamsMeeting_CreatesAndPublishes(t *testing.T) { var publishedSubj string var publishedData []byte var publishedMsgID string + meetingStore := newStubTeamsMeetingStore() h := &Handler{ - store: store, - siteID: "site-a", - teamsEmailDomain: "corp.com", - roomMembersLimit: 500, - graphClient: graph, - meetMarkerReader: &stubMeetMarkerReader{found: false}, + store: store, + siteID: "site-a", + teamsEmailDomain: "corp.com", + roomMembersLimit: 500, + graphClient: graph, + teamsMeetingStore: meetingStore, publishToStream: func(_ context.Context, subj string, data []byte, msgID string) error { publishedSubj, publishedData, publishedMsgID = subj, data, msgID return nil @@ -195,11 +300,19 @@ func TestTeamsMeeting_CreatesAndPublishes(t *testing.T) { assert.Equal(t, "mtg-1", resp.ID) assert.Equal(t, "https://teams.example/join/1", resp.JoinURL) - // Graph called exactly once; organizer + attendees derived from accounts. + // Graph called exactly once; organizer + attendees derived from accounts; + // externalId is the stable siteID:roomID key. assert.Equal(t, 1, graph.callCount) + assert.Equal(t, "site-a:r1", graph.lastReq.ExternalID) assert.Equal(t, "alice@corp.com", graph.lastReq.OrganizerEmail) assert.ElementsMatch(t, []string{"alice@corp.com", "bob@corp.com"}, graph.lastReq.AttendeeEmails) + // The meeting was persisted as a first-class record keyed (roomId, siteId). + rec, found, _ := meetingStore.GetTeamsMeeting(context.Background(), "r1", "site-a") + require.True(t, found) + assert.Equal(t, "mtg-1", rec.MeetingID) + assert.Equal(t, "https://teams.example/join/1", rec.JoinURL) + // teams_meet_started published through the canonical message path. require.NotEmpty(t, publishedData, "expected a canonical message publish") assert.NotEmpty(t, publishedMsgID, "expected a dedup msg ID") @@ -214,25 +327,29 @@ func TestTeamsMeeting_CreatesAndPublishes(t *testing.T) { assert.Equal(t, "https://teams.example/join/1", sys.JoinURL) } -func TestTeamsMeeting_Idempotent_ReturnsExisting(t *testing.T) { +// TestTeamsMeeting_Idempotent_FastPathReadHit: an existing teams_meetings record +// short-circuits the handler — no Graph create, no member list, no publish. +func TestTeamsMeeting_Idempotent_FastPathReadHit(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) - // ListRoomMembers must NOT be called when a marker already exists. + // ListRoomMembers must NOT be called when a record already exists. graph := &fakeGraphClient{meeting: &msgraph.OnlineMeeting{ID: "should-not-be-used"}} + meetingStore := newStubTeamsMeetingStore() + meetingStore.seed(model.TeamsMeetingRecord{ + RoomID: "r1", SiteID: "site-a", + MeetingID: "mtg-existing", JoinURL: "https://teams.example/join/existing", + }) h := &Handler{ - store: store, - siteID: "site-a", - teamsEmailDomain: "corp.com", - roomMembersLimit: 500, - graphClient: graph, - meetMarkerReader: &stubMeetMarkerReader{ - found: true, - marker: &model.TeamsMeetStartedSysData{MeetingID: "mtg-existing", JoinURL: "https://teams.example/join/existing"}, - }, + store: store, + siteID: "site-a", + teamsEmailDomain: "corp.com", + roomMembersLimit: 500, + graphClient: graph, + teamsMeetingStore: meetingStore, publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { t.Error("idempotent path must not publish a new system message") return nil @@ -246,22 +363,140 @@ func TestTeamsMeeting_Idempotent_ReturnsExisting(t *testing.T) { assert.Equal(t, 0, graph.callCount, "no duplicate Graph create on the idempotent path") } +// TestTeamsMeeting_DuplicateKey_ReturnsExisting: the fast-path read misses but a +// concurrent winner inserted the record between this caller's read and its own +// insert. The insert hits the (roomId, siteId) unique constraint; the handler +// reads back the winner's record and returns it WITHOUT publishing a second +// system message. createOrGet already guaranteed the same Graph meeting. +func TestTeamsMeeting_DuplicateKey_ReturnsExisting(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). + Return([]model.RoomMember{indMember("alice")}, nil) + + // createOrGet returns the SAME meeting the concurrent winner got (Graph is + // the source of truth on a true race). + graph := &fakeGraphClient{meeting: &msgraph.OnlineMeeting{ID: "mtg-shared", JoinURL: "https://teams.example/join/shared"}} + + // Seed the store as if a concurrent winner already inserted the record, so + // this caller's fast-path read still misses (we delete it first) but its + // insert collides. Simulate by pre-populating AFTER the fast-path read would + // run: easiest deterministic model is to seed the winner's record and force + // the fast-path read to miss via a one-shot. Here we instead seed the record + // and rely on the handler: fast-path read would HIT. To exercise the dup-key + // branch specifically, use a store whose first GetTeamsMeeting misses then + // the insert collides — modeled below. + meetingStore := &raceTeamsMeetingStore{ + winner: model.TeamsMeetingRecord{ + RoomID: "r1", SiteID: "site-a", + MeetingID: "mtg-shared", JoinURL: "https://teams.example/join/shared", + }, + } + + published := false + h := &Handler{ + store: store, + siteID: "site-a", + teamsEmailDomain: "corp.com", + roomMembersLimit: 500, + graphClient: graph, + teamsMeetingStore: meetingStore, + publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { + published = true + return nil + }, + } + + resp, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) + require.NoError(t, err) + assert.Equal(t, "mtg-shared", resp.ID) + assert.Equal(t, "https://teams.example/join/shared", resp.JoinURL) + assert.Equal(t, 1, graph.callCount, "createOrGet still called, returns the shared meeting") + assert.False(t, published, "loser of the insert race must NOT publish a second system message") +} + +// TestTeamsMeeting_Concurrent_SingleCreateSingleMessage runs two concurrent +// meetings calls against one shared in-memory store enforcing the (roomId, +// siteId) unique constraint, plus a Graph stub that mimics createOrGet +// (one meeting per externalId). It asserts: exactly one Graph meeting, exactly +// one system message published, and both callers return the same meeting. +func TestTeamsMeeting_Concurrent_SingleCreateSingleMessage(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + // Both goroutines run the membership/room/member reads; allow any count. + store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil).AnyTimes() + store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil).AnyTimes() + store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). + Return([]model.RoomMember{indMember("alice")}, nil).AnyTimes() + + graph := newCreateOrGetGraphStub() + meetingStore := newStubTeamsMeetingStore() + + var publishCount int + var pubMu sync.Mutex + h := &Handler{ + store: store, + siteID: "site-a", + teamsEmailDomain: "corp.com", + roomMembersLimit: 500, + graphClient: graph, + teamsMeetingStore: meetingStore, + publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { + pubMu.Lock() + publishCount++ + pubMu.Unlock() + return nil + }, + } + + const n = 8 + var wg sync.WaitGroup + results := make([]*model.TeamsMeetingReply, n) + errs := make([]error, n) + start := make(chan struct{}) + for i := 0; i < n; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + <-start + results[idx], errs[idx] = h.teamsMeeting( + ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), + model.TeamsMeetingRequest{}, + ) + }(i) + } + close(start) + wg.Wait() + + for i := 0; i < n; i++ { + require.NoError(t, errs[i]) + require.NotNil(t, results[i]) + assert.Equal(t, results[0].ID, results[i].ID, "all concurrent callers return the same meeting") + assert.Equal(t, results[0].JoinURL, results[i].JoinURL) + } + assert.Equal(t, 1, graph.distinctMeetings(), "exactly one Graph meeting for one externalId") + assert.Equal(t, 1, publishCount, "exactly one teams_meet_started system message") +} + func TestTeamsMeeting_NotConfigured(t *testing.T) { - h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500} // graphClient + meetMarkerReader nil + h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500} // graphClient + teamsMeetingStore nil _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) require.ErrorIs(t, err, errTeamsNotConfigured) } func TestTeamsMeeting_RequesterMissing(t *testing.T) { h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, - graphClient: &fakeGraphClient{}, meetMarkerReader: &stubMeetMarkerReader{}} + graphClient: &fakeGraphClient{}, teamsMeetingStore: newStubTeamsMeetingStore()} _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "", "roomID": "r1"}), model.TeamsMeetingRequest{}) require.ErrorIs(t, err, errTeamsRequesterMissing) } func TestTeamsMeeting_RoomIDMissing(t *testing.T) { h := &Handler{siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, - graphClient: &fakeGraphClient{}, meetMarkerReader: &stubMeetMarkerReader{}} + graphClient: &fakeGraphClient{}, teamsMeetingStore: newStubTeamsMeetingStore()} _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": ""}), model.TeamsMeetingRequest{}) require.ErrorIs(t, err, errTeamsRoomIDRequired) } @@ -274,7 +509,7 @@ func TestTeamsMeeting_NotMember(t *testing.T) { graph := &fakeGraphClient{} h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, - graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{found: false}} + graphClient: graph, teamsMeetingStore: newStubTeamsMeetingStore()} _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) require.ErrorIs(t, err, errNotRoomMember) assert.Equal(t, 0, graph.callCount) @@ -290,7 +525,7 @@ func TestTeamsMeeting_TooManyMembers(t *testing.T) { graph := &fakeGraphClient{} h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 2, - graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{found: false}} + graphClient: graph, teamsMeetingStore: newStubTeamsMeetingStore()} _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) require.ErrorIs(t, err, errTeamsMeetingTooManyMembers) assert.Equal(t, errcode.RoomMaxSizeReached, errcode.ReasonOf(err)) @@ -308,7 +543,7 @@ func TestTeamsMeeting_GraphCreateFails(t *testing.T) { graph := &fakeGraphClient{err: errors.New("graph 500")} var published bool h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, - graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{found: false}, + graphClient: graph, teamsMeetingStore: newStubTeamsMeetingStore(), publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { published = true; return nil }, } _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) @@ -316,15 +551,17 @@ func TestTeamsMeeting_GraphCreateFails(t *testing.T) { assert.False(t, published, "no system message on Graph failure") } -func TestTeamsMeeting_MarkerReadFails(t *testing.T) { +func TestTeamsMeeting_RecordReadFails(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{RoomID: "r1"}, nil) store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) graph := &fakeGraphClient{} + meetingStore := newStubTeamsMeetingStore() + meetingStore.getErr = errors.New("mongo down") h := &Handler{store: store, siteID: "site-a", teamsEmailDomain: "corp.com", roomMembersLimit: 500, - graphClient: graph, meetMarkerReader: &stubMeetMarkerReader{err: errors.New("cass down")}} + graphClient: graph, teamsMeetingStore: meetingStore} _, err := h.teamsMeeting(ctxParams(map[string]string{"account": "alice", "roomID": "r1"}), model.TeamsMeetingRequest{}) require.Error(t, err) assert.Equal(t, 0, graph.callCount) diff --git a/room-service/main.go b/room-service/main.go index cc5ed3c5a..dae9e6f07 100644 --- a/room-service/main.go +++ b/room-service/main.go @@ -18,7 +18,6 @@ import ( "github.com/hmchangw/chat/pkg/health" "github.com/hmchangw/chat/pkg/logctx" "github.com/hmchangw/chat/pkg/mongoutil" - "github.com/hmchangw/chat/pkg/msgbucket" "github.com/hmchangw/chat/pkg/msgraph" "github.com/hmchangw/chat/pkg/natsrouter" "github.com/hmchangw/chat/pkg/natsutil" @@ -58,10 +57,6 @@ type config struct { TeamsEmailDomain string `env:"TEAMS_EMAIL_DOMAIN" envDefault:"dev.local"` RoomMembersLimit int `env:"ROOM_MEMBERS_LIMIT" envDefault:"500"` RoomMembersCallLimit int `env:"ROOM_MEMBERS_CALL_LIMIT" envDefault:"20"` - // MessageBucketHours must match message-worker's MESSAGE_BUCKET_HOURS so the - // teams_meet_started idempotency read walks the same partitions writes land in. - MessageBucketHours int `env:"MESSAGE_BUCKET_HOURS" envDefault:"72"` - MessageReadMaxBuckets int `env:"MESSAGE_READ_MAX_BUCKETS" envDefault:"30"` // Atrest/Vault drive eager at-rest DEK provisioning at room creation. // When Atrest.Enabled is false the DEK is created lazily by message-worker. Atrest atrest.Config // env vars already prefixed ATREST_* @@ -155,20 +150,6 @@ func main() { } cassReader := NewCassMessageReader(cassSession) - if cfg.MessageBucketHours <= 0 { - slog.Error("invalid MESSAGE_BUCKET_HOURS: must be > 0", "value", cfg.MessageBucketHours) - os.Exit(1) - } - if cfg.MessageReadMaxBuckets <= 0 { - slog.Error("invalid MESSAGE_READ_MAX_BUCKETS: must be > 0", "value", cfg.MessageReadMaxBuckets) - os.Exit(1) - } - meetMarkerReader := NewCassMeetMarkerReader( - cassSession, - msgbucket.New(time.Duration(cfg.MessageBucketHours)*time.Hour), - cfg.MessageReadMaxBuckets, - ) - // Graph client backs the meetings RPC. Constructed only when the Azure app // credentials are present; otherwise the meetings RPC reports not-configured // while the deep-link RPCs keep working. @@ -222,7 +203,7 @@ func main() { ) handler.dekProvisioner = dekProvisioner handler.graphClient = graphClient - handler.meetMarkerReader = meetMarkerReader + handler.teamsMeetingStore = store handler.teamsEmailDomain = cfg.TeamsEmailDomain handler.roomMembersLimit = cfg.RoomMembersLimit handler.roomMembersCallLimit = cfg.RoomMembersCallLimit diff --git a/room-service/mock_store_test.go b/room-service/mock_store_test.go index 33db4587c..d891d99be 100644 --- a/room-service/mock_store_test.go +++ b/room-service/mock_store_test.go @@ -728,42 +728,56 @@ func (mr *MockMessageReaderMockRecorder) GetMessageRoomAndCreatedAt(ctx, message return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMessageRoomAndCreatedAt", reflect.TypeOf((*MockMessageReader)(nil).GetMessageRoomAndCreatedAt), ctx, messageID) } -// MockMeetMarkerReader is a mock of MeetMarkerReader interface. -type MockMeetMarkerReader struct { +// MockTeamsMeetingStore is a mock of TeamsMeetingStore interface. +type MockTeamsMeetingStore struct { ctrl *gomock.Controller - recorder *MockMeetMarkerReaderMockRecorder + recorder *MockTeamsMeetingStoreMockRecorder isgomock struct{} } -// MockMeetMarkerReaderMockRecorder is the mock recorder for MockMeetMarkerReader. -type MockMeetMarkerReaderMockRecorder struct { - mock *MockMeetMarkerReader +// MockTeamsMeetingStoreMockRecorder is the mock recorder for MockTeamsMeetingStore. +type MockTeamsMeetingStoreMockRecorder struct { + mock *MockTeamsMeetingStore } -// NewMockMeetMarkerReader creates a new mock instance. -func NewMockMeetMarkerReader(ctrl *gomock.Controller) *MockMeetMarkerReader { - mock := &MockMeetMarkerReader{ctrl: ctrl} - mock.recorder = &MockMeetMarkerReaderMockRecorder{mock} +// NewMockTeamsMeetingStore creates a new mock instance. +func NewMockTeamsMeetingStore(ctrl *gomock.Controller) *MockTeamsMeetingStore { + mock := &MockTeamsMeetingStore{ctrl: ctrl} + mock.recorder = &MockTeamsMeetingStoreMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockMeetMarkerReader) EXPECT() *MockMeetMarkerReaderMockRecorder { +func (m *MockTeamsMeetingStore) EXPECT() *MockTeamsMeetingStoreMockRecorder { return m.recorder } -// GetLastTeamsMeetStarted mocks base method. -func (m *MockMeetMarkerReader) GetLastTeamsMeetStarted(ctx context.Context, roomID string) (*model.TeamsMeetStartedSysData, bool, error) { +// GetTeamsMeeting mocks base method. +func (m *MockTeamsMeetingStore) GetTeamsMeeting(ctx context.Context, roomID, siteID string) (*model.TeamsMeetingRecord, bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetLastTeamsMeetStarted", ctx, roomID) - ret0, _ := ret[0].(*model.TeamsMeetStartedSysData) + ret := m.ctrl.Call(m, "GetTeamsMeeting", ctx, roomID, siteID) + ret0, _ := ret[0].(*model.TeamsMeetingRecord) ret1, _ := ret[1].(bool) ret2, _ := ret[2].(error) return ret0, ret1, ret2 } -// GetLastTeamsMeetStarted indicates an expected call of GetLastTeamsMeetStarted. -func (mr *MockMeetMarkerReaderMockRecorder) GetLastTeamsMeetStarted(ctx, roomID any) *gomock.Call { +// GetTeamsMeeting indicates an expected call of GetTeamsMeeting. +func (mr *MockTeamsMeetingStoreMockRecorder) GetTeamsMeeting(ctx, roomID, siteID any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLastTeamsMeetStarted", reflect.TypeOf((*MockMeetMarkerReader)(nil).GetLastTeamsMeetStarted), ctx, roomID) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTeamsMeeting", reflect.TypeOf((*MockTeamsMeetingStore)(nil).GetTeamsMeeting), ctx, roomID, siteID) +} + +// InsertTeamsMeeting mocks base method. +func (m *MockTeamsMeetingStore) InsertTeamsMeeting(ctx context.Context, record model.TeamsMeetingRecord) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "InsertTeamsMeeting", ctx, record) + ret0, _ := ret[0].(error) + return ret0 +} + +// InsertTeamsMeeting indicates an expected call of InsertTeamsMeeting. +func (mr *MockTeamsMeetingStoreMockRecorder) InsertTeamsMeeting(ctx, record any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertTeamsMeeting", reflect.TypeOf((*MockTeamsMeetingStore)(nil).InsertTeamsMeeting), ctx, record) } diff --git a/room-service/store.go b/room-service/store.go index 00b8f7cc8..1f6ffda52 100644 --- a/room-service/store.go +++ b/room-service/store.go @@ -228,13 +228,20 @@ type MessageReader interface { ) } -// MeetMarkerReader reads back the most-recent teams_meet_started system message -// for a room, used to make the meetings RPC idempotent (a second call returns -// the existing meeting instead of creating a duplicate Graph onlineMeeting). -// found=false with err=nil means the room has no meeting marker. All reads are -// keyspace-aware (the gocql session is bound to CASSANDRA_KEYSPACE). -type MeetMarkerReader interface { - GetLastTeamsMeetStarted(ctx context.Context, roomID string) ( - marker *model.TeamsMeetStartedSysData, found bool, err error, +// TeamsMeetingStore is the first-class idempotency record for a room's Teams +// meeting. It replaces the message-bucket marker scan with a dedicated Mongo +// document keyed unique on (roomId, siteId), mirroring the unique-index + +// IsDuplicateKeyError retry-safe-write convention room-service already uses for +// room_members and subscriptions (see store_mongo.go EnsureIndexes). +type TeamsMeetingStore interface { + // GetTeamsMeeting fast-path reads the room's existing meeting record. + // found=false with err=nil means the room has no meeting yet. + GetTeamsMeeting(ctx context.Context, roomID, siteID string) ( + record *model.TeamsMeetingRecord, found bool, err error, ) + // InsertTeamsMeeting inserts the meeting record. The (roomId, siteId) + // unique index makes this the idempotency gate: a concurrent second insert + // returns a duplicate-key error (mongo.IsDuplicateKeyError), which the + // handler treats as "a concurrent winner already wrote it" and reads back. + InsertTeamsMeeting(ctx context.Context, record model.TeamsMeetingRecord) error } diff --git a/room-service/store_cassandra.go b/room-service/store_cassandra.go index 967e8cc6a..26b648703 100644 --- a/room-service/store_cassandra.go +++ b/room-service/store_cassandra.go @@ -2,15 +2,11 @@ package main import ( "context" - "encoding/json" "errors" "fmt" "time" "github.com/gocql/gocql" - - "github.com/hmchangw/chat/pkg/model" - "github.com/hmchangw/chat/pkg/msgbucket" ) type cassMessageReader struct { @@ -43,53 +39,3 @@ func (r *cassMessageReader) GetMessageRoomAndCreatedAt( } return roomID, createdAt, senderAccount, true, nil } - -// cassMeetMarkerReader reads the latest teams_meet_started system message for a -// room. messages_by_room is partitioned by (room_id, bucket) and clustered by -// created_at, so we walk buckets from "now" backwards (newest first) and, within -// each bucket, take the newest row whose type is teams_meet_started. The first -// match is the most recent meeting for the room. The walk is bounded by -// maxBuckets so a room that never had a meeting doesn't scan unboundedly. -// -// The query never names a keyspace — the gocql session is bound to -// CASSANDRA_KEYSPACE at connect time (cassutil), so this is keyspace-aware. -type cassMeetMarkerReader struct { - session *gocql.Session - bucket msgbucket.Sizer - maxBuckets int -} - -func NewCassMeetMarkerReader(session *gocql.Session, bucket msgbucket.Sizer, maxBuckets int) *cassMeetMarkerReader { - return &cassMeetMarkerReader{session: session, bucket: bucket, maxBuckets: maxBuckets} -} - -const meetMarkerQuery = `SELECT sys_msg_data FROM messages_by_room ` + - `WHERE room_id = ? AND bucket = ? AND type = ? ORDER BY created_at DESC LIMIT 1 ALLOW FILTERING` - -func (r *cassMeetMarkerReader) GetLastTeamsMeetStarted( - ctx context.Context, - roomID string, -) (*model.TeamsMeetStartedSysData, bool, error) { - bucket := r.bucket.Of(time.Now().UTC()) - for i := 0; i < r.maxBuckets; i++ { - var sysMsgData []byte - err := r.session.Query(meetMarkerQuery, roomID, bucket, model.MessageTypeTeamsMeetStarted). - WithContext(ctx). - Scan(&sysMsgData) - if err != nil { - if errors.Is(err, gocql.ErrNotFound) { - bucket = r.bucket.Prev(bucket) - continue - } - return nil, false, fmt.Errorf("read teams_meet_started marker for room %s: %w", roomID, err) - } - var marker model.TeamsMeetStartedSysData - if len(sysMsgData) > 0 { - if err := json.Unmarshal(sysMsgData, &marker); err != nil { - return nil, false, fmt.Errorf("decode teams_meet_started marker for room %s: %w", roomID, err) - } - } - return &marker, true, nil - } - return nil, false, nil -} diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index 5001c14fd..c758fdf0f 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -33,6 +33,7 @@ type MongoStore struct { users *mongo.Collection apps *mongo.Collection botCmdMenus *mongo.Collection + teamsMeetings *mongo.Collection } func NewMongoStore(db *mongo.Database) *MongoStore { @@ -44,6 +45,7 @@ func NewMongoStore(db *mongo.Database) *MongoStore { users: db.Collection("users"), apps: db.Collection("apps"), botCmdMenus: db.Collection("bot_cmd_menu"), + teamsMeetings: db.Collection("teams_meetings"), } } @@ -167,6 +169,45 @@ func (s *MongoStore) EnsureIndexes(ctx context.Context) error { }); err != nil { return fmt.Errorf("ensure bot_cmd_menu (activeStatus,name) index: %w", err) } + // Unique logical key for teams_meetings — the per-room idempotency record for + // the meetings RPC. A concurrent second create hits this constraint and the + // loser reads back the winner's record instead of inserting a duplicate (and + // thus publishing a second teams_meet_started system message). Same retry-safe + // rationale as the room_members / subscriptions unique indexes above. + if _, err := s.teamsMeetings.Indexes().CreateOne(ctx, mongo.IndexModel{ + Keys: bson.D{{Key: "roomId", Value: 1}, {Key: "siteId", Value: 1}}, + Options: options.Index().SetUnique(true), + }); err != nil { + return fmt.Errorf("ensure teams_meetings (roomId,siteId) unique index: %w", err) + } + return nil +} + +// GetTeamsMeeting fast-path reads the room's existing Teams meeting record. +// found=false with err=nil means the room has no meeting yet. +func (s *MongoStore) GetTeamsMeeting(ctx context.Context, roomID, siteID string) (*model.TeamsMeetingRecord, bool, error) { + var rec model.TeamsMeetingRecord + err := s.teamsMeetings.FindOne(ctx, bson.M{"roomId": roomID, "siteId": siteID}).Decode(&rec) + if errors.Is(err, mongo.ErrNoDocuments) { + return nil, false, nil + } + if err != nil { + return nil, false, fmt.Errorf("get teams meeting for room %q: %w", roomID, err) + } + return &rec, true, nil +} + +// InsertTeamsMeeting inserts the meeting record. The (roomId, siteId) unique +// index makes this the idempotency gate: a concurrent second insert returns a +// duplicate-key error, surfaced unwrapped to the handler so it can detect the +// race via mongo.IsDuplicateKeyError and read back the winner's record. +func (s *MongoStore) InsertTeamsMeeting(ctx context.Context, record model.TeamsMeetingRecord) error { + if _, err := s.teamsMeetings.InsertOne(ctx, record); err != nil { + // Surface the raw error so the handler's mongo.IsDuplicateKeyError check + // works (fmt.Errorf-wrapping would still satisfy errors.As, but keeping + // it raw matches the room-worker bulk-insert dup-key convention). + return err + } return nil } From a2907da99ed630e0d9c0c062e72217c3c7d95dea Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Tue, 16 Jun 2026 02:19:55 +0000 Subject: [PATCH 5/9] docs: add msgraph client guide (config, app-only auth, createOrGet, testing) Documents pkg/msgraph: env config (TEAMS_*), the client-credentials token flow, the createOrGet+externalId idempotency, the production application-access-policy gate, and the credential-free httptest pattern. Linked from docs/client-api.md (doc-ratchet). --- docs/client-api.md | 2 + docs/msgraph-client.md | 92 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 docs/msgraph-client.md diff --git a/docs/client-api.md b/docs/client-api.md index e5a4e3c61..1601163ec 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -1913,6 +1913,8 @@ See [Error envelope](#6-error-envelope-reference). Creates a Microsoft Teams `onlineMeeting` via the Graph API and returns its join URL. **Idempotent per room, including under concurrency:** the meeting is created via Graph's `createOrGet` endpoint keyed on a stable per-room `externalId`, and a first-class `teams_meetings` record with a unique key on `(roomId, siteId)` guards local state. Repeated or concurrent calls for the same room return the same meeting and publish exactly one `teams_meet_started` system message. Attendee emails are derived as `account@TEAMS_EMAIL_DOMAIN`. +> Graph client details (config env vars, app-only auth, the `createOrGet` idempotency key, the production application-access-policy requirement, and how to test without real credentials) are documented in [`docs/msgraph-client.md`](msgraph-client.md). + External client label: `POST /api/v1/meetings`. **Subject:** `chat.user.{account}.request.room.{roomID}.{siteID}.teams.meeting` diff --git a/docs/msgraph-client.md b/docs/msgraph-client.md new file mode 100644 index 000000000..80638d499 --- /dev/null +++ b/docs/msgraph-client.md @@ -0,0 +1,92 @@ +# Microsoft Graph client (`pkg/msgraph`) + +A minimal, app-only Microsoft Graph client used by `room-service` to create +Teams **online meetings** for the `teams.meeting` RPC. It exposes only the +surface room-service needs and sits behind a `Client` interface so callers can +be unit-tested against a mock without reaching Azure. + +## What it does + +- One operation: `CreateOnlineMeeting` — creates (or returns the existing) + Teams online meeting and yields its `joinUrl` + meeting id. +- Authenticates with the **client-credentials (app-only) OAuth2 flow** and + caches the token until it expires. + +## Configuration + +The client takes a `Config{TenantID, ClientID, ClientSecret}`. `room-service` +populates it from these environment variables (plus the email domain it uses to +derive organizer/attendee addresses): + +| Env var | Purpose | +|---|---| +| `TEAMS_TENANT_ID` | Azure AD tenant id (path segment of the token URL) | +| `TEAMS_CLIENT_ID` | App registration (client) id | +| `TEAMS_CLIENT_SECRET` | App registration client secret | +| `TEAMS_EMAIL_DOMAIN` | Domain appended to an `account` to form an email (`account@domain`); defaults to `dev.local` for local/dev | + +When the Teams credentials are unset, the deep-link call RPCs still work (they +need only `TEAMS_EMAIL_DOMAIN`); the meetings RPC returns a not-configured +error until the credentials are set. + +## Auth flow + +1. `POST https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token` with + `grant_type=client_credentials`, the client id/secret, and + `scope=https://graph.microsoft.com/.default`. +2. The access token is cached and reused until shortly before expiry. + +## Creating a meeting (idempotent) + +The client calls Graph's **`createOrGet`** endpoint with a required +`externalId`: + +- App-only: `POST {base}/users/{organizerEmail}/onlineMeetings/createOrGet` +- Delegated fallback: `POST {base}/me/onlineMeetings/createOrGet` + +`createOrGet` is idempotent at the source of truth: for a given +`(organizer, externalId)` it returns the **existing** meeting if one exists, +otherwise creates one. `room-service` sets `externalId` to a stable per-room key +(`siteID:roomID`), so repeated or concurrent `teams.meeting` calls for the same +room return the same meeting. `externalId` is required — the client rejects an +empty value. + +## Production requirement (the live gate) + +App-only `onlineMeetings` access is **not** granted by the application +permission alone. Before live use the tenant must have: + +1. The **`OnlineMeetings.ReadWrite.All`** application permission, admin-consented + for the app registration; and +2. A **Teams application access policy** (`New-CsApplicationAccessPolicy` + + `Grant-CsApplicationAccessPolicy`) that authorizes the app to create meetings + **on behalf of the organizer user**. + +Without the access policy, `createOrGet` returns `403`. This is the one piece +that cannot be exercised by the unit tests and must be validated against the +real tenant. + +## Testing without credentials + +The client is built to be tested with **no Azure credentials**. The constructor +takes options that point it at local stub servers: + +- `WithTokenURL(url)` — override the OAuth token endpoint. +- `WithBaseURL(url)` — override the Graph API base URL. +- `WithHTTPClient(c)` — inject a custom `*http.Client`. + +`pkg/msgraph/msgraph_test.go` uses `httptest.NewServer` to stub **both** the +token endpoint and the Graph API, covering: success, idempotent-same-externalId, +required-externalId, token error, Graph error, and missing-joinURL. Because the +client is behind the `Client` interface, the `room-service` meetings handler is +also unit-tested against a generated mock (including a concurrent test that +asserts exactly one meeting + one system message under parallel calls). + +Run them with: + +```bash +go test ./pkg/msgraph/... ./room-service/... +``` + +No secrets, no network to Azure — only the live end-to-end smoke (above) needs +the real tenant. From d2c9695c7dc619ad898d9be8ec1b8851b7afa295 Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Tue, 16 Jun 2026 07:30:51 +0000 Subject: [PATCH 6/9] docs(client-api): clarify Teams External client label is the edge-gateway HTTP path, not a route served here --- docs/client-api.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/client-api.md b/docs/client-api.md index 1601163ec..1d8092286 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -1807,6 +1807,12 @@ Same envelope and sentinels as Get Room App Tabs. --- +> **Note on `External client label`:** each Teams RPC below lists an HTTP-style +> label (e.g. `POST /api/v1/calls/room`). That label is the path the **edge +> gateway exposes to external/mobile clients**; the gateway translates it to the +> NATS RPC shown under **Subject**. This service implements **only** the NATS RPC +> (request/reply over `_INBOX.>`) — it does not serve an HTTP endpoint. + #### Start Teams Room Call Builds a Microsoft Teams deep link for a call to every other member of the room (the caller is excluded). No Graph API call — the link is built from the member list, deriving each member's email as `account@TEAMS_EMAIL_DOMAIN`. From 2870d4a8374b7ce32f541596cded558445ce1617 Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Tue, 16 Jun 2026 07:42:19 +0000 Subject: [PATCH 7/9] test(msgraph): suppress gosec G117 false positives on OAuth token mocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The httptest token-server mocks encode tokenResponse{AccessToken:"tok"...}; gosec G117 flags the marshaled access_token field as a secret pattern. These are dummy values in test doubles of Microsoft's OAuth endpoint, not real secrets — annotated with // #nosec G117 + justification per repo SAST policy. --- pkg/msgraph/msgraph_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/msgraph/msgraph_test.go b/pkg/msgraph/msgraph_test.go index e94ab0414..b48455c86 100644 --- a/pkg/msgraph/msgraph_test.go +++ b/pkg/msgraph/msgraph_test.go @@ -29,7 +29,7 @@ func TestCreateOnlineMeeting_Success(t *testing.T) { require.NoError(t, r.ParseForm()) assert.Equal(t, "client_credentials", r.Form.Get("grant_type")) assert.Equal(t, graphScope, r.Form.Get("scope")) - _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok-123", ExpiresIn: 3600}) + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok-123", ExpiresIn: 3600}) // #nosec G117 -- test mock encodes a fake OAuth token response; dummy value, not a real secret })) defer tokenSrv.Close() @@ -70,7 +70,7 @@ func TestCreateOnlineMeeting_Success(t *testing.T) { // truth — the server returns the existing meeting on the second createOrGet). func TestCreateOnlineMeeting_Idempotent_SameExternalID(t *testing.T) { tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) // #nosec G117 -- test mock encodes a fake OAuth token response; dummy value, not a real secret })) defer tokenSrv.Close() @@ -120,7 +120,7 @@ func TestCreateOnlineMeeting_RequiresExternalID(t *testing.T) { func TestCreateOnlineMeeting_TokenError(t *testing.T) { tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) - _ = json.NewEncoder(w).Encode(tokenResponse{Error: "invalid_client", ErrorDesc: "bad secret"}) + _ = json.NewEncoder(w).Encode(tokenResponse{Error: "invalid_client", ErrorDesc: "bad secret"}) // #nosec G117 -- test mock encodes a fake OAuth token response; dummy value, not a real secret })) defer tokenSrv.Close() @@ -137,7 +137,7 @@ func TestCreateOnlineMeeting_TokenError(t *testing.T) { func TestCreateOnlineMeeting_GraphError(t *testing.T) { tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) // #nosec G117 -- test mock encodes a fake OAuth token response; dummy value, not a real secret })) defer tokenSrv.Close() graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -157,7 +157,7 @@ func TestCreateOnlineMeeting_GraphError(t *testing.T) { func TestCreateOnlineMeeting_MissingJoinURL(t *testing.T) { tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) + _ = json.NewEncoder(w).Encode(tokenResponse{AccessToken: "tok", ExpiresIn: 3600}) // #nosec G117 -- test mock encodes a fake OAuth token response; dummy value, not a real secret })) defer tokenSrv.Close() graphSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { From 0558e6143a779a338d344ef829dc1c1207da36cc Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Thu, 18 Jun 2026 01:23:17 +0000 Subject: [PATCH 8/9] refactor(room-service): teams_meet_started shows requester display name + trim comments Address review feedback on the Teams meeting handler: - sys message Content now formats `"" started a Teams meeting` (matches the room_restricted / members_added sys-message convention), falling back to the account when the user lookup fails. - condense the long idempotency doc-block and inline step comments. --- room-service/handler_teams.go | 47 ++++++++++++------------------ room-service/handler_teams_test.go | 3 ++ 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/room-service/handler_teams.go b/room-service/handler_teams.go index 36082c1f2..eff4805c1 100644 --- a/room-service/handler_teams.go +++ b/room-service/handler_teams.go @@ -90,24 +90,12 @@ func (h *Handler) teamsUserCall(c *natsrouter.Context, req model.TeamsUserCallRe return &model.TeamsCallReply{JoinURL: buildTeamsCallDeepLink([]string{email})}, nil } -// teamsMeeting creates (or returns the existing) Microsoft Teams onlineMeeting -// for the room. Idempotent per room under concurrency via two cooperating -// guarantees: -// -// 1. Graph createOrGet keyed on a stable per-room externalId — Graph returns -// the same meeting for repeated/concurrent calls with the same key, so even -// a true race never produces two distinct Graph meetings (the correctness -// guarantee across restarts and multiple service instances). -// 2. A teams_meetings Mongo record with a UNIQUE index on (roomId, siteId) — -// this is the local first-class idempotency record (mirrors the -// room_members / subscriptions retry-safe-write convention). On a concurrent -// insert race the loser hits a duplicate-key error and reads back the -// winner's record, so exactly ONE teams_meet_started system message is ever -// published per room. -// -// Flow: fast-path read the record → if present, return it → else createOrGet → -// insert the record → on dup-key, read back the winner → publish the system -// message only when THIS call created the record. Enforces roomMembersLimit. +// teamsMeeting creates (or returns the existing) Teams onlineMeeting for the +// room, idempotent per room: Graph createOrGet keyed on a stable per-room +// externalId yields one meeting even under concurrent/retried calls, and a +// teams_meetings record with a UNIQUE (roomId, siteId) index gates the +// teams_meet_started system message so it publishes exactly once. Enforces +// roomMembersLimit. func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingRequest) (*model.TeamsMeetingReply, error) { //nolint:gocritic // hugeParam: req passed by value to satisfy natsrouter.Register var ctx context.Context = c requestID := natsutil.RequestIDFromContext(c) @@ -129,7 +117,7 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques return nil, err } - // (a) Fast-path: an existing record short-circuits Graph + insert + publish. + // Fast-path: an existing record short-circuits Graph + insert + publish. if rec, found, err := h.teamsMeetingStore.GetTeamsMeeting(ctx, roomID, h.siteID); err != nil { return nil, fmt.Errorf("read teams meeting record: %w", err) } else if found && rec != nil && rec.JoinURL != "" { @@ -147,8 +135,7 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques attendeeEmails := membersToAttendeeEmails(members, h.teamsEmailDomain) organizerEmail := teamsEmail(requesterAccount, h.teamsEmailDomain) - // (b) Graph createOrGet keyed on the stable per-room externalId. Even on a - // race both callers get the SAME meeting back from Graph. + // Graph createOrGet: concurrent callers get the same meeting back. meeting, err := h.graphClient.CreateOnlineMeeting(ctx, msgraph.CreateOnlineMeetingRequest{ ExternalID: teamsMeetingExternalID(h.siteID, roomID), Subject: meetingSubject(room), @@ -159,10 +146,8 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques return nil, fmt.Errorf("create online meeting: %w", err) } - // (c) Insert the idempotency record. The (roomId, siteId) unique index makes - // this the gate: on a concurrent insert race the loser hits a duplicate-key - // error, reads back the winner's record, and returns WITHOUT publishing a - // second system message. + // Insert the idempotency record; the unique (roomId, siteId) index gates the + // publish — the loser of an insert race reads back the winner and skips it. record := model.TeamsMeetingRecord{ RoomID: roomID, SiteID: h.siteID, @@ -188,8 +173,7 @@ func (h *Handler) teamsMeeting(c *natsrouter.Context, _ model.TeamsMeetingReques return nil, fmt.Errorf("insert teams meeting record: %w", err) } - // (d) This call created the record — it is the unique winner, so publish the - // teams_meet_started system message exactly once. + // This call created the record — the unique winner publishes exactly once. if err := h.publishTeamsMeetStarted(ctx, requestID, roomID, requesterAccount, meeting); err != nil { return nil, err } @@ -222,13 +206,20 @@ func (h *Handler) publishTeamsMeetStarted( return fmt.Errorf("marshal teams_meet_started sys data: %w", err) } + // Prefer the requester's display name; fall back to the account if the user + // lookup fails (the record is already committed, so never fail the publish). + byDisplay := byAccount + if u, err := h.store.GetUser(ctx, byAccount); err == nil && u != nil { + byDisplay = displayfmt.CombineWithFallback(u.EngName, u.ChineseName, u.Account) + } + now := time.Now().UTC() sysMsg := model.Message{ ID: idgen.MessageIDFromRequestID(requestID, "teams_meet_started"), RoomID: roomID, UserAccount: byAccount, Type: model.MessageTypeTeamsMeetStarted, - Content: "started a Teams meeting", + Content: fmt.Sprintf("%q started a Teams meeting", byDisplay), SysMsgData: sysData, CreatedAt: now, } diff --git a/room-service/handler_teams_test.go b/room-service/handler_teams_test.go index cc200222d..1b6169047 100644 --- a/room-service/handler_teams_test.go +++ b/room-service/handler_teams_test.go @@ -275,6 +275,7 @@ func TestTeamsMeeting_CreatesAndPublishes(t *testing.T) { store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Name: "general", Type: model.RoomTypeChannel}, nil) store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). Return([]model.RoomMember{indMember("alice"), indMember("bob")}, nil) + store.EXPECT().GetUser(gomock.Any(), "alice").Return(&model.User{Account: "alice", EngName: "Alice"}, nil) graph := &fakeGraphClient{meeting: &msgraph.OnlineMeeting{ID: "mtg-1", JoinURL: "https://teams.example/join/1"}} @@ -321,6 +322,7 @@ func TestTeamsMeeting_CreatesAndPublishes(t *testing.T) { var evt model.MessageEvent require.NoError(t, json.Unmarshal(publishedData, &evt)) assert.Equal(t, model.MessageTypeTeamsMeetStarted, evt.Message.Type) + assert.Equal(t, `"Alice" started a Teams meeting`, evt.Message.Content) var sys model.TeamsMeetStartedSysData require.NoError(t, json.Unmarshal(evt.Message.SysMsgData, &sys)) assert.Equal(t, "mtg-1", sys.MeetingID) @@ -431,6 +433,7 @@ func TestTeamsMeeting_Concurrent_SingleCreateSingleMessage(t *testing.T) { store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil).AnyTimes() store.EXPECT().ListRoomMembers(gomock.Any(), "r1", nil, nil, false). Return([]model.RoomMember{indMember("alice")}, nil).AnyTimes() + store.EXPECT().GetUser(gomock.Any(), "alice").Return(&model.User{Account: "alice", EngName: "Alice"}, nil).AnyTimes() graph := newCreateOrGetGraphStub() meetingStore := newStubTeamsMeetingStore() From cb1c6bf8b39a1acba34d025486c5e49dcf79de5f Mon Sep 17 00:00:00 2001 From: "Sharon Apple (Hakanai bot)" Date: Thu, 18 Jun 2026 03:11:52 +0000 Subject: [PATCH 9/9] fix(room-service): wrap InsertTeamsMeeting error with context Per the repo error-wrapping rule. mongo.IsDuplicateKeyError unwraps via errors.As, so the handler's idempotency-race dup-key check still works. --- room-service/store_mongo.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index c758fdf0f..8e569e9ec 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -199,14 +199,11 @@ func (s *MongoStore) GetTeamsMeeting(ctx context.Context, roomID, siteID string) // InsertTeamsMeeting inserts the meeting record. The (roomId, siteId) unique // index makes this the idempotency gate: a concurrent second insert returns a -// duplicate-key error, surfaced unwrapped to the handler so it can detect the -// race via mongo.IsDuplicateKeyError and read back the winner's record. +// duplicate-key error the handler detects via mongo.IsDuplicateKeyError (which +// unwraps with errors.As) and reads back the winner's record. func (s *MongoStore) InsertTeamsMeeting(ctx context.Context, record model.TeamsMeetingRecord) error { if _, err := s.teamsMeetings.InsertOne(ctx, record); err != nil { - // Surface the raw error so the handler's mongo.IsDuplicateKeyError check - // works (fmt.Errorf-wrapping would still satisfy errors.As, but keeping - // it raw matches the room-worker bulk-insert dup-key convention). - return err + return fmt.Errorf("insert teams meeting record: %w", err) } return nil }