diff --git a/docs/journals/260708-2003-oauth-per-user-flow-planning.md b/docs/journals/260708-2003-oauth-per-user-flow-planning.md new file mode 100644 index 00000000..a554f431 --- /dev/null +++ b/docs/journals/260708-2003-oauth-per-user-flow-planning.md @@ -0,0 +1,87 @@ +# Bitrix24 Per-User OAuth Flow: Planning Complete + +**Date**: 2026-07-08 20:03 +**Severity**: High +**Component**: Bitrix24 channel, OAuth, provisioning, MCP credential minting +**Status**: Pending Implementation (6-phase plan drafted, all decisions locked) + +## What Happened + +Bitrix24 support confirmed via official investigation (2026-07-08) that the `ONIMBOTMESSAGEADD` webhook no longer attaches `auth[access_token]` for new users — this is **expected behavior, not a regression**. Imbot subscriptions bind with `USER_ID=0` at registration time, so Bitrix never requests/includes a real OAuth token in the top-level `auth` block; only the bot's own token (`data[BOT][botID][AUTH]`) is guaranteed present. + +**Proof**: goclaw's `mcp_user_credentials` table shows 3 users (614, 1, 610) successfully minted credentials between 2026-05-07 and 2026-05-25. Zero new users since. DB + support confirmation align perfectly. + +**Consequence**: New staff users who message the bot cannot authorize CRM access via webhook token. goclaw must implement a self-serve OAuth re-auth flow: detect when a user has no `mcp_user_credentials` row OR their stored `refresh_token` is dead, then DM them a signed OAuth authorize link. + +## The Brutal Truth + +This is a hard block on goclaw's Bitrix integration expanding to new staff. We shipped assuming Bitrix would ship the user's token in every webhook event. It doesn't. The issue isn't a bug we can wait out — it's by design. We should have pressure-tested the webhook event schema against real multi-user tenants months ago instead of assuming Bitrix's imbot behavior matched simpler channel patterns. + +The good news: the fix is **stateless**. We reuse nearly all existing infrastructure (provisioner checks, autoOnboard flow, state codec). ~230 LOC, no migration, no new tables. + +## Technical Details + +**Root cause verification:** +- `provisioner.go:222` queries `GetUserCredentials(userID)` on every message — results show `existing == nil` for new users. +- Bitrix24 support (agent Aleksei, 2026-07-08): imbot subscription (not user-initiated auth) binds to `USER_ID=0`; OAuth token only appears in webhook if user logs in separately (not the case here). +- Imbot events carry `data[BOT][botID][AUTH]` (bot's own token) but never a user `auth[access_token]` at the top level. + +**Implementation shape (per `design.md` v2):** +- **Trigger v2 (precise):** Changed from "auth is empty" → `existing == nil` (no prior credential row). Separate branch for existing-but-dead tokens (`invalid_grant`/`expired_token`/`NO_AUTH_FOUND` in `APIError.Code`). +- **Delivery channel (locked):** DM user 1-1 (`imbot.v2.Chat.Message.send` with `dialogId = userID`); Bitrix auto-opens private dialog. Optional hint in original group chat if message came from group. Fallback to group if DM send fails. +- **State storage:** Stateless HMAC-signed state (base64url JSON + hex SHA256 signature). No `oauth_pending_states` table. 10-min TTL + 1-time-use. +- **Token lifecycle:** User clicks link → OAuth callback verifies identity (`token.user_id == state.user_id`) → upserts `mcp_user_credentials` (reuses `SetUserCredentials` via `autoOnboard` path). Existing rows are overwritten, not deleted — `mcp_user_credentials` unique constraint `(server_id, user_id, tenant_id)` makes upsert safe. + +**Decisions finalized (§9, design.md):** +1. Delivery channel: DM (1-1), not group reply ✓ +2. TTL: 10 minutes ✓ +3. Debounce: 5 min/user, separate map from `notifyUserOfMCPIssueOnce` ✓ +4. OAuth scope: Not a decision — Bitrix `/oauth/authorize/` doesn't accept `scope` param; portal app's registered scope applies in full ✓ +5. Callback route: Public (`GET /bitrix24/oauth/user/callback`), same security pattern as `/bitrix24/install`, stateless HMAC+TTL makes it safe ✓ + +## What We Tried + +1. **Initial assumption (hypothesis):** Bitrix webhook shipping empty token = transient regression. **Failed** — support confirmed it's by design. +2. **Escalation to Bitrix24 support (2026-07-08):** Raised ticket with DB evidence (3 users, cutoff 2026-05-25). Support replied with 4-point technical explanation. Investigation was correctproof. +3. **Parallel design brainstorm (multiple rounds, 2026-07-08):** Workshopped v1 design (weak on trigger condition, reply in group) → v2 refactor (precise trigger via `existing == nil`, DM-only delivery, error classification). User feedback corrected two major UX decisions: (a) "delete then recreate" credential row → revealed as unnecessary churn (upsert handles it), (b) "reply in original dialog" → switched to DM for privacy. +4. **Scope verification (design.md §9 Q4):** Checked official Bitrix OAuth docs (`Complete OAuth 2.0 Authorization Protocol` via `b24-dev-mcp`). Confirmed: `/oauth/authorize/` takes only `client_id`, `response_type`, `state`, `redirect_uri`; no `scope` param. Portal scope is fixed per app registration. Eliminated that as a planning decision. +5. **i18n thread discovery:** Initially drafted Phase 5 as catalog integration (en/vi/zh) per root CLAUDE.md convention. **Verified against code** — bitrix24 channel doesn't thread locale (`provisioner.go:472-486`, documented in comment). Corrected plan to hardcoded Vietnamese string matching existing `mcpUserNotifyMessage` pattern, avoiding unplanned i18n infrastructure. + +## Root Cause Analysis + +The blocking issue is a **design-webhook mismatch**: +- goclaw designed for "user-centric OAuth" (assume webhook carries user token every event). +- Bitrix imbot designed for "app-centric OAuth" (bot requests OAuth once at install; per-user tokens require separate auth flow). + +This gap surfaced only when real multi-user tenants started messaging. Single-user dev/staging never caught it. + +**Why it hurt:** We iterated on CRM features (agent loops, deal sync) assuming provisioning would just work for all staff once one user got it working. It doesn't. This is a hard floor blocking the entire "staff collaborate via bot" thesis. + +## Lessons Learned + +1. **Pressure-test assumptions against external APIs early.** Bitrix's OAuth model (app-centric vs user-centric) should have been verified against real webhook captures in Q2, not discovered mid-Q3 via support ticket. + +2. **Stateless state codec > DB state table.** HMAC-signed state + TTL eliminates schema complexity and janitor jobs. We should apply this pattern more aggressively. + +3. **Separate error paths by root cause, not just HTTP code.** `APIError.Code` (already present in code) lets us distinguish "token genuinely dead" (`invalid_grant`) from "network hiccup" (5xx). This precision unlocks targeted UX (re-auth vs retry). + +4. **UX beats infrastructure.** The initial design's "delete + recreate" credential row looked clean (full reset), but user insight that it's wasteful churn forced the right call (upsert). Code quality often takes a back seat to user friction. + +5. **i18n threading is infrastructure debt.** The false start on adding locale-threading to a channel that doesn't use it nearly bloated the scope. Verify existing infrastructure before reaching for general-purpose solutions. + +## Next Steps + +**Pending tasks:** 6 phases, dependency-chained, all tasked out: + +1. **Phase 01: OAuth State Codec** — implement HMAC-signed state with TTL (`oauth_state_codec.go` + tests). Deliverable: stateless encode/decode with security guarantees. +2. **Phase 02: OAuth Callback Handler** — implement `/bitrix24/oauth/user/callback` route, code+token exchange, identity validation (`oauth_user_flow.go` + tests). Deliverable: secure exchange logic reusing `Portal.Exchange` + identity guard. +3. **Phase 03: Provisioner Trigger Branches** — split `provisioner.go::provisionIfMissing` at line ~280. Case 1: `existing == nil` (first-time) → build authorize URL. Case 2: existing but `selfRefreshUserCreds` fails with `invalid_grant`/`expired_token`/`NO_AUTH_FOUND` → also trigger re-auth (same flow, no row delete). Deliverable: precise two-path branching with error classification. +4. **Phase 04: Handler DM Delivery** — catch `ErrUserAuthRequired` in `handle.go`, send DM via `imbot.v2.Chat.Message.send` (dialogId=userID), fallback to group on error, debounce 5 min/user. Deliverable: end-to-end user notification with fallback. +5. **Phase 05: Message Strings** — hardcoded Vietnamese strings matching existing channel pattern (no i18n threading). Deliverable: finalized copy for authorize link prompt + rejection/expiry messages. +6. **Phase 06: Tests** — unit coverage (state codec round-trip, HMAC, TTL expiry, identity mismatch rejection), integration (full flow end-to-end with mocked Bitrix OAuth endpoints). Deliverable: >85% coverage, no integration-only gaps. + +**Blockers:** None. All decisions locked. Code can begin immediately after this planning entry. + +**Timeline estimate:** 10 hours (phases + review + merge). Phases 1–2 can parallelize; 3–4 depend on 1–2; 5–6 span all. + +**Files touched:** ~6 (2 new, 4 modified). No migrations. No external API changes. diff --git a/internal/channels/bitrix24/channel.go b/internal/channels/bitrix24/channel.go index 82849005..69cbd089 100644 --- a/internal/channels/bitrix24/channel.go +++ b/internal/channels/bitrix24/channel.go @@ -90,6 +90,13 @@ type Channel struct { notifyMu sync.Mutex notifyDebounce map[string]time.Time + // OAuth re-authorization DM debounce. Deliberately separate map/mutex + // from notifyDebounce above — the two notice types (generic MCP failure + // vs "please re-authorize") are independent; one must never suppress + // the other. Same TTL value (mcpUserNotifyDebounceTTL) but its own state. + oauthInviteMu sync.Mutex + oauthInviteDebounce map[string]time.Time + // Contact-name enrichment cache. Bitrix24 webhooks don't carry // display_name / username, so the channel lazily resolves them via // user.get on first sight of each sender. Cache is per-channel (not diff --git a/internal/channels/bitrix24/handle.go b/internal/channels/bitrix24/handle.go index 4a535682..91b5fc55 100644 --- a/internal/channels/bitrix24/handle.go +++ b/internal/channels/bitrix24/handle.go @@ -384,7 +384,16 @@ func (c *Channel) handleMessage(ctx context.Context, evt *Event) { // the MCP server's tools, which is strictly better UX than the channel // denying the message. The typed errors let tests assert behavior // without string matching. - if err := c.provisionIfMissing(ctx, senderID, evt.Params.FromIsConnector, evt.Auth); err != nil { + if err := c.provisionIfMissing(ctx, senderID, evt.Params.FromIsConnector, evt.Auth, chatID); err != nil { + // ErrUserAuthRequired carries a URL (not a sentinel value), so it + // needs errors.As rather than errors.Is. Checked first and returns + // early — the user has no MCP access yet, so there's nothing useful + // for the agent to answer with; don't publish to the bus. + var authErr *ErrUserAuthRequired + if errors.As(err, &authErr) { + c.sendOAuthInvite(ctx, senderID, chatID, isGroup, authErr.URL) + return + } switch { case errors.Is(err, ErrProvisionDisabled), errors.Is(err, ErrProvisionSkippedOpenChannel), diff --git a/internal/channels/bitrix24/oauth_invite_dm_test.go b/internal/channels/bitrix24/oauth_invite_dm_test.go new file mode 100644 index 00000000..03dadb2f --- /dev/null +++ b/internal/channels/bitrix24/oauth_invite_dm_test.go @@ -0,0 +1,373 @@ +package bitrix24 + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// capturedImbotCall is one recorded imbot.v2.Chat.Message.send request, as +// observed by the fake imbot server in newOAuthInviteTestChannel. +type capturedImbotCall struct { + DialogID string + Message string + Keyboard []map[string]any // decoded from the JSON-string fields[keyboard] param +} + +// fakeImbotServer records every imbot.v2.Chat.Message.send call it receives +// and always answers with a minimal success envelope. +type fakeImbotServer struct { + mu sync.Mutex + calls []capturedImbotCall + // failDialogIDs marks dialog ids whose call should return a Bitrix error + // instead of success — used to simulate the DM-send-fails-falls-back case. + failDialogIDs map[string]bool +} + +func newFakeImbotServer() *fakeImbotServer { + return &fakeImbotServer{failDialogIDs: map[string]bool{}} +} + +func (f *fakeImbotServer) handler() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + dialogID := r.Form.Get("dialogId") + + call := capturedImbotCall{ + DialogID: dialogID, + Message: r.Form.Get("fields[message]"), + } + if kb := r.Form.Get("fields[keyboard]"); kb != "" { + _ = json.Unmarshal([]byte(kb), &call.Keyboard) + } + f.mu.Lock() + f.calls = append(f.calls, call) + fail := f.failDialogIDs[dialogID] + f.mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + if fail { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":"ACCESS_DENIED","error_description":"cannot message this user"}`)) + return + } + _, _ = w.Write([]byte(`{"result":123}`)) + } +} + +func (f *fakeImbotServer) snapshot() []capturedImbotCall { + f.mu.Lock() + defer f.mu.Unlock() + out := make([]capturedImbotCall, len(f.calls)) + copy(out, f.calls) + return out +} + +// newOAuthInviteTestChannel builds a Channel where a webhook event with no +// auth block will fall all the way through provisionIfMissing into +// sendOAuthInvite, and captures whatever imbot.v2.Chat.Message.send calls +// that triggers via imbotSrv. +func newOAuthInviteTestChannel(t *testing.T, imbotSrv *httptest.Server) (*Channel, *bus.MessageBus) { + t.Helper() + resetWebhookRouterForTest() + t.Cleanup(resetWebhookRouterForTest) + + mcpStore := newFakeMCPStore() + serverID := uuid.New() + mcpStore.serversByName["bitrix-mcp"] = &store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "bitrix-mcp", + } + // MCP auto-onboard is never expected to be hit in these tests (no auth + // in the event → brand-new-user branch short-circuits before it). + mcpSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("auto-onboard must not be called when the event carries no auth block") + })) + t.Cleanup(mcpSrv.Close) + + fs := newFakeStore() + mb := bus.New() + fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, testOAuthEncKey) + cfgJSON := `{"portal":"p","bot_code":"c","bot_name":"n","bot_type":"B","dm_policy":"open","group_policy":"open","require_mention":false,"mcp_server_name":"bitrix-mcp","mcp_base_url":"` + mcpSrv.URL + `"}` + ch, err := fn("b1", nil, json.RawMessage(cfgJSON), mb, nil) + if err != nil { + t.Fatalf("factory: %v", err) + } + bc := ch.(*Channel) + bc.SetTenantID(store.GenNewID()) + + bc.startMu.Lock() + bc.botID = 42 + bc.startMu.Unlock() + + if err := bc.initMCPProvisioner(context.Background()); err != nil { + t.Fatalf("initMCPProvisioner: %v", err) + } + + // Portal with BOTH a captured public_url (BuildUserAuthorizeURL) AND a + // pre-seeded, non-expired access token (so Client().Call reaches + // imbotSrv directly, no OAuth refresh round-trip needed). + portalFS := newFakeStore() + portal := newTestPortal(t, httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("no OAuth token call expected — access token is pre-seeded fresh") + })), portalFS, bc.TenantID(), "p", store.BitrixPortalState{ + PublicURL: "https://goclaw.example.com", + AccessToken: "bot-access-tok", + RefreshToken: "bot-refresh-tok", + ExpiresAt: time.Now().Add(time.Hour), + }) + + bc.startMu.Lock() + bc.portal = portal + bc.client = NewClient("portal.bitrix24.com", &http.Client{ + Transport: &rewriteRT{target: imbotSrv.URL, base: http.DefaultTransport}, + }) + bc.client.SetPortal(portal) + bc.startMu.Unlock() + + return bc, mb +} + +func TestHandleMessage_OAuthInvite_GroupOrigin_SendsDMAndHint(t *testing.T) { + imbot := newFakeImbotServer() + srv := httptest.NewServer(imbot.handler()) + defer srv.Close() + + bc, mb := newOAuthInviteTestChannel(t, srv) + + bc.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "1058", + DialogID: "chat777", + ChatID: "777", + MessageID: "m-1", + MessageType: "chat", // group + Message: "hello bot", + }, + }) + + if _, ok := drainOne(mb, 200*time.Millisecond); ok { + t.Error("must not publish to agent bus while user has no MCP access") + } + + calls := imbot.snapshot() + if len(calls) != 2 { + t.Fatalf("expected 2 imbot calls (DM + group hint), got %d: %+v", len(calls), calls) + } + + dm := calls[0] + if dm.DialogID != "1058" { + t.Errorf("DM dialogId = %q, want the plain user id 1058", dm.DialogID) + } + if len(dm.Keyboard) != 1 { + t.Fatalf("DM keyboard should have exactly 1 button, got %d", len(dm.Keyboard)) + } + link, _ := dm.Keyboard[0]["LINK"].(string) + if !strings.Contains(link, "/oauth/authorize/") { + t.Errorf("keyboard button LINK = %q, want an authorize URL", link) + } + if strings.Contains(dm.Message, link) { + t.Error("DM message text must NOT contain the raw URL (must live only in the keyboard button)") + } + + hint := calls[1] + if hint.DialogID != "chat777" { + t.Errorf("group hint dialogId = %q, want original chatID chat777", hint.DialogID) + } + if strings.Contains(hint.Message, "http") { + t.Error("group hint must not leak the authorize URL") + } +} + +func TestHandleMessage_OAuthInvite_DMOrigin_NoHint(t *testing.T) { + imbot := newFakeImbotServer() + srv := httptest.NewServer(imbot.handler()) + defer srv.Close() + + bc, _ := newOAuthInviteTestChannel(t, srv) + + bc.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "1058", + DialogID: "1058", // DM: dialog IS the user id + MessageID: "m-1", + MessageType: "private", + Message: "hello bot", + }, + }) + + calls := imbot.snapshot() + if len(calls) != 1 { + t.Fatalf("expected exactly 1 imbot call (DM only, no hint), got %d: %+v", len(calls), calls) + } + if calls[0].DialogID != "1058" { + t.Errorf("dialogId = %q, want 1058", calls[0].DialogID) + } +} + +func TestHandleMessage_OAuthInvite_DMSendFails_FallsBackToChat(t *testing.T) { + imbot := newFakeImbotServer() + imbot.failDialogIDs["1058"] = true // DM to the user fails + srv := httptest.NewServer(imbot.handler()) + defer srv.Close() + + bc, _ := newOAuthInviteTestChannel(t, srv) + + bc.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "1058", + DialogID: "chat777", + ChatID: "777", + MessageID: "m-1", + MessageType: "chat", + Message: "hello bot", + }, + }) + + calls := imbot.snapshot() + // Expect: failed DM attempt (dialogId=1058) + fallback into chat777 + // carrying the SAME message+keyboard (not the group-hint text). + if len(calls) != 2 { + t.Fatalf("expected 2 calls (failed DM + fallback), got %d: %+v", len(calls), calls) + } + if calls[0].DialogID != "1058" { + t.Errorf("first call dialogId = %q, want 1058 (the attempted DM)", calls[0].DialogID) + } + fallback := calls[1] + if fallback.DialogID != "chat777" { + t.Errorf("fallback dialogId = %q, want chat777", fallback.DialogID) + } + if len(fallback.Keyboard) != 1 { + t.Fatalf("fallback must carry the SAME keyboard button, got %d buttons", len(fallback.Keyboard)) + } + if fallback.Message != oauthInviteMessage { + t.Errorf("fallback message = %q, want the full invite hint (not the group-only hint text)", fallback.Message) + } +} + +// TestHandleMessage_OAuthInvite_TotalFailure_ReleasesDebounce covers the +// double-failure case (DM AND the chatID fallback both error) — the user got +// NOTHING delivered, so the debounce slot taken for this attempt must be +// released immediately rather than blocking a retry for the full 5-minute TTL. +func TestHandleMessage_OAuthInvite_TotalFailure_ReleasesDebounce(t *testing.T) { + imbot := newFakeImbotServer() + imbot.failDialogIDs["1058"] = true // DM fails + imbot.failDialogIDs["chat777"] = true // fallback also fails + srv := httptest.NewServer(imbot.handler()) + defer srv.Close() + + bc, _ := newOAuthInviteTestChannel(t, srv) + + bc.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "1058", + DialogID: "chat777", + ChatID: "777", + MessageID: "m-1", + MessageType: "chat", + Message: "hello bot", + }, + }) + + calls := imbot.snapshot() + if len(calls) != 2 { + t.Fatalf("expected 2 attempted calls (DM + fallback, both failing), got %d: %+v", len(calls), calls) + } + + // The debounce slot must have been released — a second trigger right + // after must be allowed to attempt delivery again immediately. + if !bc.tryAcquireOAuthInviteNotify("1058") { + t.Fatal("debounce slot should have been released after total delivery failure, but a retry was blocked") + } +} + +// TestOAuthInviteDebounce_IndependentOfNotifyDebounce exercises +// tryAcquireOAuthInviteNotify directly (unit-level, not through the full +// DispatchEvent pipeline — the outer 60s auto-onboard debounce in +// provisionIfMissing would otherwise swallow a second same-second webhook +// retry before ever reaching this code, making the two debounces +// impossible to tell apart end-to-end). Verifies: (1) a second acquire for +// the same user within the TTL is rejected, (2) marking notifyDebounce +// (the OTHER notice type's map) does not affect oauthInviteDebounce at all. +func TestOAuthInviteDebounce_IndependentOfNotifyDebounce(t *testing.T) { + imbot := newFakeImbotServer() + srv := httptest.NewServer(imbot.handler()) + defer srv.Close() + bc, _ := newOAuthInviteTestChannel(t, srv) + + if !bc.tryAcquireOAuthInviteNotify("1058") { + t.Fatal("first acquire should succeed") + } + if bc.tryAcquireOAuthInviteNotify("1058") { + t.Fatal("second acquire within TTL should be debounced") + } + + // A DIFFERENT user must not be affected by the first user's debounce. + if !bc.tryAcquireOAuthInviteNotify("999") { + t.Fatal("a different user must not be debounced by user 1058's entry") + } + + // Marking the OTHER notice type's debounce map must not influence this one. + bc.notifyMu.Lock() + if bc.notifyDebounce == nil { + bc.notifyDebounce = make(map[string]time.Time) + } + bc.notifyDebounce["777"] = time.Now() + bc.notifyMu.Unlock() + if !bc.tryAcquireOAuthInviteNotify("777") { + t.Fatal("oauthInviteDebounce must be independent of notifyDebounce — user 777 was only debounced in the OTHER map") + } +} + +// TestOAuthInviteDebounce_SweepsExpiredEntries verifies tryAcquireOAuthInviteNotify +// evicts stale entries instead of letting the map grow unbounded for the life +// of the process (code-review finding: the map must not repeat the never-evicted +// pattern of mcpDebounce/notifyDebounce). +func TestOAuthInviteDebounce_SweepsExpiredEntries(t *testing.T) { + imbot := newFakeImbotServer() + srv := httptest.NewServer(imbot.handler()) + defer srv.Close() + bc, _ := newOAuthInviteTestChannel(t, srv) + + // Seed an already-expired entry directly (can't sleep 5 real minutes in a test). + bc.oauthInviteMu.Lock() + bc.oauthInviteDebounce = map[string]time.Time{ + "stale-user": time.Now().Add(-mcpUserNotifyDebounceTTL - time.Second), + } + bc.oauthInviteMu.Unlock() + + // Any call sweeps expired entries before doing its own check-and-set. + if !bc.tryAcquireOAuthInviteNotify("fresh-user") { + t.Fatal("acquire for a new user should succeed") + } + + bc.oauthInviteMu.Lock() + _, staleStillPresent := bc.oauthInviteDebounce["stale-user"] + _, freshPresent := bc.oauthInviteDebounce["fresh-user"] + mapSize := len(bc.oauthInviteDebounce) + bc.oauthInviteMu.Unlock() + + if staleStillPresent { + t.Error("expired entry 'stale-user' should have been swept, but is still present") + } + if !freshPresent { + t.Error("'fresh-user' should be present after its own acquire") + } + if mapSize != 1 { + t.Errorf("map should contain exactly 1 entry (fresh-user) after sweep, got %d", mapSize) + } +} diff --git a/internal/channels/bitrix24/oauth_state_codec.go b/internal/channels/bitrix24/oauth_state_codec.go new file mode 100644 index 00000000..98e28607 --- /dev/null +++ b/internal/channels/bitrix24/oauth_state_codec.go @@ -0,0 +1,135 @@ +package bitrix24 + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/base64" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "net/url" + "strings" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/crypto" +) + +// oauthStateTTL bounds how long a signed authorize-URL state stays valid. +// Long enough for a user to open the DM and click through Bitrix's approve +// page; short enough that a leaked link (log line, browser history) has a +// tight blast radius. Chosen during design brainstorm (design.md §9 Q2). +const oauthStateTTL = 10 * time.Minute + +// oauthStatePayload is the identity + routing context carried through the +// Bitrix OAuth redirect round-trip. Stateless by design: no DB row to +// insert/delete/janitor — the state itself is self-verifying (HMAC + embedded +// expiry), so decodeOAuthState can reject garbage before any DB or Bitrix +// network call. +type oauthStatePayload struct { + UserID string `json:"u"` + TenantID string `json:"t"` + // BotID routes the callback back to the right Channel instance via + // Router.DispatcherByBotID — Bitrix's redirect carries no bot_id of its + // own, so it must be embedded here at BuildUserAuthorizeURL time. + BotID int `json:"b"` + ChannelName string `json:"c"` // logging only — BotID is the actual routing key + Domain string `json:"d"` + DialogID string `json:"dlg"` + ExpiresAt int64 `json:"exp"` // unix seconds +} + +// encodeOAuthState serializes payload to base64url(json) + "." + hex(HMAC-SHA256). +// key comes from crypto.DeriveKey(GOCLAW_ENCRYPTION_KEY) — same key already used +// for AES-256-GCM elsewhere in this codebase (no second secret introduced). +func encodeOAuthState(p oauthStatePayload, key []byte) (string, error) { + body, err := json.Marshal(p) + if err != nil { + return "", fmt.Errorf("bitrix24 oauth state: marshal payload: %w", err) + } + encoded := base64.RawURLEncoding.EncodeToString(body) + sig := signOAuthStatePart(encoded, key) + return encoded + "." + hex.EncodeToString(sig), nil +} + +// decodeOAuthState verifies the HMAC signature (constant-time) and expiry +// BEFORE returning the payload, so a tampered or expired state never reaches +// a caller that might act on it (e.g. attempt a Bitrix token exchange). +func decodeOAuthState(state string, key []byte) (*oauthStatePayload, error) { + idx := strings.LastIndex(state, ".") + if idx < 0 { + return nil, errors.New("bitrix24 oauth state: malformed (missing signature)") + } + encoded, sigHex := state[:idx], state[idx+1:] + + gotSig, err := hex.DecodeString(sigHex) + if err != nil { + return nil, errors.New("bitrix24 oauth state: malformed signature encoding") + } + wantSig := signOAuthStatePart(encoded, key) + if !hmac.Equal(gotSig, wantSig) { + return nil, errors.New("bitrix24 oauth state: signature mismatch") + } + + body, err := base64.RawURLEncoding.DecodeString(encoded) + if err != nil { + return nil, fmt.Errorf("bitrix24 oauth state: decode payload: %w", err) + } + var p oauthStatePayload + if err := json.Unmarshal(body, &p); err != nil { + return nil, fmt.Errorf("bitrix24 oauth state: unmarshal payload: %w", err) + } + if time.Now().Unix() > p.ExpiresAt { + return nil, errors.New("bitrix24 oauth state: expired") + } + return &p, nil +} + +// signOAuthStatePart computes HMAC-SHA256(encodedPayload, key). +func signOAuthStatePart(encodedPayload string, key []byte) []byte { + mac := hmac.New(sha256.New, key) + mac.Write([]byte(encodedPayload)) + return mac.Sum(nil) +} + +// BuildUserAuthorizeURL builds the Bitrix `/oauth/authorize/` link the DM +// invite (handle.go, sendOAuthInvite) points at. +// +// No `scope` parameter — the portal always grants the app's registered scope +// in full (confirmed against official Bitrix OAuth docs, design.md §9 — no +// way to request a narrower scope per call). +// +// No `redirect_uri` parameter either — Local Apps ignore it. Confirmed +// against live behavior: Bitrix always redirects back to the app's +// registered "Application URL" (handlerPath, /bitrix24/handler, +// webhook.go handleAppPage) regardless of what's passed here. Passing it +// anyway would be dead weight (design.md §12 changelog documents the +// correction) — omitted so a future reader doesn't assume it's honored. +func (c *Channel) BuildUserAuthorizeURL(userID, dialogID string) (string, error) { + portal := c.Portal() + if portal == nil { + return "", errors.New("bitrix24 oauth: portal not available") + } + keyBytes, err := crypto.DeriveKey(c.encKey) + if err != nil { + return "", fmt.Errorf("bitrix24 oauth: derive state key: %w", err) + } + + state, err := encodeOAuthState(oauthStatePayload{ + UserID: userID, + TenantID: c.TenantID().String(), + BotID: c.BotID(), + ChannelName: c.Name(), + Domain: portal.Domain(), + DialogID: dialogID, + ExpiresAt: time.Now().Add(oauthStateTTL).Unix(), + }, keyBytes) + if err != nil { + return "", err + } + + q := url.Values{} + q.Set("client_id", portal.creds.ClientID) + q.Set("state", state) + return "https://" + portal.Domain() + "/oauth/authorize/?" + q.Encode(), nil +} diff --git a/internal/channels/bitrix24/oauth_state_codec_test.go b/internal/channels/bitrix24/oauth_state_codec_test.go new file mode 100644 index 00000000..3ea0151f --- /dev/null +++ b/internal/channels/bitrix24/oauth_state_codec_test.go @@ -0,0 +1,113 @@ +package bitrix24 + +import ( + "strings" + "testing" + "time" +) + +func testOAuthKey() []byte { + return []byte("01234567890123456789012345678901") // 32 bytes, test-only +} + +func TestOAuthState_RoundTrip(t *testing.T) { + key := testOAuthKey() + want := oauthStatePayload{ + UserID: "1058", + TenantID: "0193a5b0-7000-7000-8000-000000000001", + ChannelName: "b24-syn", + Domain: "tamgiac.bitrix24.com", + DialogID: "chat4878", + ExpiresAt: time.Now().Add(10 * time.Minute).Unix(), + } + + state, err := encodeOAuthState(want, key) + if err != nil { + t.Fatalf("encode: %v", err) + } + + got, err := decodeOAuthState(state, key) + if err != nil { + t.Fatalf("decode: %v", err) + } + if *got != want { + t.Fatalf("roundtrip mismatch: got %+v want %+v", *got, want) + } +} + +func TestOAuthState_TamperedSignature(t *testing.T) { + key := testOAuthKey() + state, err := encodeOAuthState(oauthStatePayload{ + UserID: "1", ExpiresAt: time.Now().Add(time.Minute).Unix(), + }, key) + if err != nil { + t.Fatalf("encode: %v", err) + } + + idx := strings.LastIndex(state, ".") + tampered := state[:idx] + ".deadbeef" + if _, err := decodeOAuthState(tampered, key); err == nil { + t.Fatal("expected error for tampered signature, got nil") + } +} + +func TestOAuthState_TamperedPayload(t *testing.T) { + key := testOAuthKey() + state, err := encodeOAuthState(oauthStatePayload{ + UserID: "1", ExpiresAt: time.Now().Add(time.Minute).Unix(), + }, key) + if err != nil { + t.Fatalf("encode: %v", err) + } + + idx := strings.LastIndex(state, ".") + payload, sig := state[:idx], state[idx:] + // Flip the last payload byte — invalidates the signature over that payload. + tampered := payload[:len(payload)-1] + "x" + sig + if _, err := decodeOAuthState(tampered, key); err == nil { + t.Fatal("expected error for tampered payload, got nil") + } +} + +func TestOAuthState_Expired(t *testing.T) { + key := testOAuthKey() + state, err := encodeOAuthState(oauthStatePayload{ + UserID: "1", ExpiresAt: time.Now().Add(-time.Minute).Unix(), // already expired + }, key) + if err != nil { + t.Fatalf("encode: %v", err) + } + + if _, err := decodeOAuthState(state, key); err == nil { + t.Fatal("expected error for expired state, got nil") + } +} + +func TestOAuthState_Malformed(t *testing.T) { + key := testOAuthKey() + cases := []string{ + "", + "no-dot-separator", + "not-base64!!!.deadbeef", + "validbase64part.not-hex-signature", + } + for _, c := range cases { + if _, err := decodeOAuthState(c, key); err == nil { + t.Fatalf("expected error for malformed state %q, got nil", c) + } + } +} + +func TestOAuthState_WrongKeyRejected(t *testing.T) { + state, err := encodeOAuthState(oauthStatePayload{ + UserID: "1", ExpiresAt: time.Now().Add(time.Minute).Unix(), + }, testOAuthKey()) + if err != nil { + t.Fatalf("encode: %v", err) + } + + otherKey := []byte("98765432109876543210987654321098") + if _, err := decodeOAuthState(state, otherKey); err == nil { + t.Fatal("expected error when decoding with a different key, got nil") + } +} diff --git a/internal/channels/bitrix24/oauth_user_flow.go b/internal/channels/bitrix24/oauth_user_flow.go new file mode 100644 index 00000000..5bca5c49 --- /dev/null +++ b/internal/channels/bitrix24/oauth_user_flow.go @@ -0,0 +1,202 @@ +package bitrix24 + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strconv" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// sendOAuthInvite delivers the "please re-authorize" message: a private DM +// (dialogId=userID, NOT chatID) with fields.message (short hint) + a single +// fields.keyboard button whose LINK is the authorize URL (design.md §6 — a +// button, not a raw link in text). If the trigger came from a group chat, a +// separate plain-text hint is left in the original chatID so the user knows +// to check their DMs — the hint never carries the URL. If the DM send itself +// fails (e.g. the bot cannot message this user), falls back to sending the +// SAME message+keyboard into chatID instead of losing the invite entirely. +// +// Debounced 5min/user via its own map (oauthInviteMu/oauthInviteDebounce on +// Channel) — deliberately independent of notifyUserOfMCPIssueOnce's debounce +// (provisioner.go) so the two notice types never suppress each other. +func (c *Channel) sendOAuthInvite(ctx context.Context, userID, chatID string, isGroup bool, url string) { + if !c.tryAcquireOAuthInviteNotify(userID) { + return + } + + fields := map[string]any{ + "message": oauthInviteMessage, + "keyboard": []map[string]any{ + {"TEXT": oauthInviteButtonText, "LINK": url}, + }, + } + sendTo := func(dialogID string) error { + _, err := c.Client().Call(ctx, "imbot.v2.Chat.Message.send", map[string]any{ + "botId": c.BotID(), + "dialogId": dialogID, + "fields": fields, + }) + return err + } + + if err := sendTo(userID); err != nil { + slog.Warn("bitrix24 mcp: oauth invite DM failed, falling back to original dialog", + "channel", c.Name(), "user", userID, "chat_id", chatID, "err", err) + if fbErr := sendTo(chatID); fbErr != nil { + slog.Warn("bitrix24 mcp: oauth invite fallback also failed", + "channel", c.Name(), "user", userID, "chat_id", chatID, "err", fbErr) + // Total delivery failure — the user got NOTHING (not the DM, not the + // fallback). Release the debounce slot we just took so the user's + // next message retries immediately instead of silently waiting out + // the full 5-minute TTL for an invite that never arrived. + c.releaseOAuthInviteNotify(userID) + } + return + } + if isGroup { + if err := c.sendChunk(ctx, chatID, oauthInviteGroupHintMessage, sendOptions{visibility: VisibilityPublic}); err != nil { + slog.Debug("bitrix24 mcp: oauth invite group hint failed", + "channel", c.Name(), "user", userID, "chat_id", chatID, "err", err) + } + } +} + +// tryAcquireOAuthInviteNotify mirrors tryAcquireMCPProvision's atomic +// check-and-set shape (provisioner.go) but guards a DIFFERENT debounce map — +// this one gates the "please re-authorize" DM, not the underlying auto-onboard +// attempt. Returns true when the caller acquired the slot (not debounced). +// +// Keyed by userID alone (no serverID, unlike mcpDebounceKey elsewhere in this +// package) — safe today because one Channel is exactly one bot on one portal +// wired to one mcp_servers row (c.mcpServerID is a single field, not a set). +// If a future refactor lets one Channel serve multiple MCP servers, this key +// would need serverID added too, the same way mcpDebounceKey already has it. +// +// Opportunistically sweeps expired entries on every call (cheap: bounded by +// the number of unique Bitrix user IDs that have ever triggered this flow, +// and only walks the map this function itself owns) so the map doesn't grow +// for the lifetime of the process — same unbounded-growth pattern flagged in +// mcpDebounce/notifyDebounce is deliberately not repeated here. +func (c *Channel) tryAcquireOAuthInviteNotify(userID string) bool { + c.oauthInviteMu.Lock() + defer c.oauthInviteMu.Unlock() + if c.oauthInviteDebounce == nil { + c.oauthInviteDebounce = make(map[string]time.Time) + } + now := time.Now() + for id, ts := range c.oauthInviteDebounce { + if now.Sub(ts) >= mcpUserNotifyDebounceTTL { + delete(c.oauthInviteDebounce, id) + } + } + if ts, ok := c.oauthInviteDebounce[userID]; ok && now.Sub(ts) < mcpUserNotifyDebounceTTL { + return false + } + c.oauthInviteDebounce[userID] = now + return true +} + +// releaseOAuthInviteNotify clears a user's debounce entry early. Only called +// when delivery totally failed (DM AND the chatID fallback both errored) — +// see sendOAuthInvite — so the debounce doesn't block a retry for the full +// TTL when the user never actually received anything. +func (c *Channel) releaseOAuthInviteNotify(userID string) { + c.oauthInviteMu.Lock() + delete(c.oauthInviteDebounce, userID) + c.oauthInviteMu.Unlock() +} + +// UserOnboardResult is what HandleUserOAuthCallback returns so the HTTP +// handler (webhook.go, handleUserOAuthCallback) can render the right outcome +// page. Outcome is one of "success", "declined", "identity_mismatch". +type UserOnboardResult struct { + Outcome string +} + +// HandleUserOAuthCallback finishes a per-user OAuth re-authorization: it +// exchanges the authorization code, validates the response actually belongs +// to the Bitrix user the DM invite targeted, then mints/refreshes MCP +// credentials via the SAME autoOnboard + SetUserCredentials sequence +// provisionIfMissing already uses (provisioner.go) — no separate mint logic, +// no api_key churn (design.md §12). +// +// payload is the ALREADY-DECODED + HMAC-verified state (decoding happens in +// the HTTP handler, which needs the payload's BotID to resolve which Channel +// to call this on in the first place — see handleUserOAuthCallback). +func (c *Channel) HandleUserOAuthCallback(ctx context.Context, code string, payload *oauthStatePayload) (*UserOnboardResult, error) { + portal := c.Portal() + if portal == nil { + return nil, errors.New("bitrix24 oauth callback: portal not available") + } + + tr, err := portal.ExchangeUserAuthCode(ctx, code) + if err != nil { + slog.Warn("bitrix24 oauth callback: exchange failed", + "channel", c.Name(), "user_id", payload.UserID, "err", err) + return nil, fmt.Errorf("bitrix24 oauth callback: exchange: %w", err) + } + + // Identity check: the Bitrix account that just approved MUST be the same + // one the DM invite was built for. Without this, user X's invite link + // could be completed by user Y (e.g. forwarded, or X asks a colleague to + // click it), silently attaching Y's Bitrix identity to X's MCP row. + // + // Portal/domain identity was already checked one layer up, in + // handleUserOAuthCallback (webhook.go), against the redirect's own + // `domain` query param — NOT against tr.Domain here, which is the OAuth + // server's own domain for this kind of exchange, not the portal's (see + // ExchangeUserAuthCode doc comment). This check confirms "same PERSON we + // sent the link to"; that one confirmed "same portal." Both matter; + // neither substitutes for the other. + gotUserID := strconv.FormatInt(tr.UserID, 10) + if gotUserID != payload.UserID { + slog.Warn("bitrix24 oauth callback: identity mismatch — authorized as a different Bitrix user", + "channel", c.Name(), "expected_user_id", payload.UserID, "got_user_id", gotUserID) + return &UserOnboardResult{Outcome: "identity_mismatch"}, nil + } + + if c.mcpStore == nil || c.mcpClient == nil || c.mcpServerID == uuid.Nil { + return nil, errors.New("bitrix24 oauth callback: mcp provisioning not configured for this channel") + } + + // NOTE: tr.Domain is deliberately NOT used here — it's the OAuth server's + // own domain for this kind of exchange, not the portal's (see + // ExchangeUserAuthCode). payload.Domain (signed into the state at + // BuildUserAuthorizeURL time, sourced from the real webhook event that + // triggered the invite, and re-validated against the redirect's own + // `domain` param in handleUserOAuthCallback) is the correct value. + resp, err := c.mcpClient.autoOnboard(ctx, autoOnboardRequest{ + Domain: payload.Domain, + BitrixUserID: payload.UserID, + AccessToken: tr.AccessToken, + RefreshToken: tr.RefreshToken, + ExpiresIn: int(tr.ExpiresIn), + }) + if err != nil { + return nil, fmt.Errorf("bitrix24 oauth callback: auto-onboard: %w", err) + } + + expiresAt := time.Now().Add(time.Duration(tr.ExpiresIn) * time.Second).UTC().Format(time.RFC3339) + creds := store.MCPUserCredentials{ + APIKey: resp.APIKey, + Env: map[string]string{ + "BITRIX_DOMAIN": payload.Domain, + "BITRIX_ACCESS_TOKEN": tr.AccessToken, + "BITRIX_REFRESH_TOKEN": tr.RefreshToken, + "BITRIX_EXPIRES_AT": expiresAt, + }, + } + if err := c.mcpStore.SetUserCredentials(ctx, c.mcpServerID, payload.UserID, creds); err != nil { + return nil, fmt.Errorf("bitrix24 oauth callback: persist credentials: %w", err) + } + + slog.Info("bitrix24 oauth callback: user re-authorized", + "channel", c.Name(), "user_id", payload.UserID, "created", resp.Created) + return &UserOnboardResult{Outcome: "success"}, nil +} diff --git a/internal/channels/bitrix24/oauth_user_flow_test.go b/internal/channels/bitrix24/oauth_user_flow_test.go new file mode 100644 index 00000000..65b50e8d --- /dev/null +++ b/internal/channels/bitrix24/oauth_user_flow_test.go @@ -0,0 +1,368 @@ +package bitrix24 + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/crypto" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// testOAuthEncKey is a raw 32-byte key — crypto.DeriveKey accepts a raw +// 32-byte input as-is, so this doubles as both the Channel's encKey and the +// HMAC key oauth_state_codec_test.go already exercises directly. +const testOAuthEncKey = "01234567890123456789012345678901" + +// oauthTokenHandler builds an httptest handler for the Bitrix OAuth token +// endpoint. Domain in the response is hardcoded to "oauth.bitrix.info" — +// confirmed against live Bitrix behavior that a user-authorize code exchange +// (as opposed to an app-install code exchange) always returns the OAuth +// server's own domain here, never the portal's (see ExchangeUserAuthCode +// doc comment, portal.go). The `domain` param this function used to accept +// was unrealistic test data that masked this. userID feeds the success +// response; when fail is true it returns a Bitrix-style application error +// instead (mirrors makeRefreshHandler in portal_test.go). +func oauthTokenHandler(userID int64, fail bool) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if fail { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"invalid_grant","error_description":"code already used"}`)) + return + } + body, _ := json.Marshal(TokenResponse{ + AccessToken: "user-access-tok", + RefreshToken: "user-refresh-tok", + ExpiresIn: 3600, + Domain: "oauth.bitrix.info", + MemberID: "mem1", + UserID: userID, + }) + _, _ = w.Write(body) + } +} + +// mcpAutoOnboardHandler builds an httptest handler for the MCP server's +// /api/auto-onboard endpoint. +func mcpAutoOnboardHandler() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"api_key":"minted-key","user_id":"u1","tenant_id":"t1","created":true}`)) + } +} + +// newOAuthFlowTestChannel builds a Channel wired for HandleUserOAuthCallback +// tests: mcp provisioning against mcpSrv, a Portal whose OAuth calls hit +// oauthSrv with PublicURL set, and registered with its own router under botID. +func newOAuthFlowTestChannel(t *testing.T, mcpSrv, oauthSrv *httptest.Server) (*Channel, *fakeMCPStore) { + t.Helper() + resetWebhookRouterForTest() + t.Cleanup(resetWebhookRouterForTest) + + mcpStore := newFakeMCPStore() + serverID := uuid.New() + mcpStore.serversByName["bitrix-mcp"] = &store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "bitrix-mcp", + } + + fs := newFakeStore() + fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, testOAuthEncKey) + cfgJSON := `{"portal":"p","bot_code":"c","bot_name":"n","bot_type":"B","mcp_server_name":"bitrix-mcp","mcp_base_url":"` + mcpSrv.URL + `"}` + ch, err := fn("b1", nil, json.RawMessage(cfgJSON), bus.New(), nil) + if err != nil { + t.Fatalf("factory: %v", err) + } + bc := ch.(*Channel) + bc.SetTenantID(store.GenNewID()) + + bc.startMu.Lock() + bc.botID = 42 + bc.client = NewClient("p.bitrix24.com", nil) + bc.startMu.Unlock() + + if err := bc.initMCPProvisioner(context.Background()); err != nil { + t.Fatalf("initMCPProvisioner: %v", err) + } + + portalFS := newFakeStore() + portal := newTestPortal(t, oauthSrv, portalFS, bc.TenantID(), "p", + store.BitrixPortalState{PublicURL: "https://goclaw.example.com"}) + bc.startMu.Lock() + bc.portal = portal + bc.startMu.Unlock() + + bc.router.RegisterBot(bc.botID, bc) + return bc, mcpStore +} + +func testStatePayload(bc *Channel, userID string) *oauthStatePayload { + return &oauthStatePayload{ + UserID: userID, + TenantID: bc.TenantID().String(), + BotID: bc.BotID(), + ChannelName: bc.Name(), + Domain: "portal.bitrix24.com", + DialogID: "chat123", + ExpiresAt: time.Now().Add(10 * time.Minute).Unix(), + } +} + +func TestHandleUserOAuthCallback_Success(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(1058, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, mcpStore := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + payload := testStatePayload(bc, "1058") + + result, err := bc.HandleUserOAuthCallback(context.Background(), "good-code", payload) + if err != nil { + t.Fatalf("HandleUserOAuthCallback: %v", err) + } + if result.Outcome != "success" { + t.Fatalf("Outcome = %q, want success", result.Outcome) + } + + creds, ok := mcpStore.userCreds[credKey(bc.mcpServerID, "1058")] + if !ok { + t.Fatal("expected mcp_user_credentials row to be minted") + } + if creds.Env["BITRIX_ACCESS_TOKEN"] != "user-access-tok" { + t.Errorf("BITRIX_ACCESS_TOKEN = %q", creds.Env["BITRIX_ACCESS_TOKEN"]) + } + // Regression guard: BITRIX_DOMAIN must come from payload.Domain (the real + // portal domain), NOT tr.Domain (the OAuth server's own domain, + // "oauth.bitrix.info" per oauthTokenHandler's mock) — see + // HandleUserOAuthCallback's doc comment for why tr.Domain is unusable here. + if creds.Env["BITRIX_DOMAIN"] != "portal.bitrix24.com" { + t.Errorf("BITRIX_DOMAIN = %q, want payload.Domain (portal.bitrix24.com), not tr.Domain", creds.Env["BITRIX_DOMAIN"]) + } +} + +func TestHandleUserOAuthCallback_IdentityMismatch(t *testing.T) { + // OAuth server authorizes as a DIFFERENT user (999) than the state targets (1058). + oauthSrv := httptest.NewServer(oauthTokenHandler(999, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, mcpStore := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + payload := testStatePayload(bc, "1058") + + result, err := bc.HandleUserOAuthCallback(context.Background(), "good-code", payload) + if err != nil { + t.Fatalf("HandleUserOAuthCallback: %v", err) + } + if result.Outcome != "identity_mismatch" { + t.Fatalf("Outcome = %q, want identity_mismatch", result.Outcome) + } + if mcpStore.setUserCallCount != 0 { + t.Errorf("SetUserCredentials must not be called on identity mismatch; setUserCallCount=%d", mcpStore.setUserCallCount) + } +} + +func TestHandleUserOAuthCallback_ExchangeFailure(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(0, true)) // fail=true → invalid_grant + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, mcpStore := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + payload := testStatePayload(bc, "1058") + + if _, err := bc.HandleUserOAuthCallback(context.Background(), "stale-code", payload); err == nil { + t.Fatal("expected error for failed code exchange, got nil") + } + if mcpStore.setUserCallCount != 0 { + t.Errorf("SetUserCredentials must not be called on exchange failure; setUserCallCount=%d", mcpStore.setUserCallCount) + } +} + +// --- HTTP route-level tests (Router.ServeHTTP → handleUserOAuthCallback) --- + +func TestRouterUserOAuthCallback_Declined(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(1058, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, mcpStore := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + keyBytes, err := crypto.DeriveKey(testOAuthEncKey) + if err != nil { + t.Fatalf("derive key: %v", err) + } + state, err := encodeOAuthState(*testStatePayload(bc, "1058"), keyBytes) + if err != nil { + t.Fatalf("encode state: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/bitrix24/handler?state="+state+"&error=access_denied", nil) + rec := httptest.NewRecorder() + bc.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if !strings.Contains(rec.Body.String(), "từ chối") { + t.Errorf("body doesn't mention decline: %s", rec.Body.String()) + } + if mcpStore.setUserCallCount != 0 { + t.Errorf("declined flow must not mint credentials; setUserCallCount=%d", mcpStore.setUserCallCount) + } +} + +func TestRouterUserOAuthCallback_InvalidState(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(1058, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, _ := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + + req := httptest.NewRequest(http.MethodGet, "/bitrix24/handler?state=garbage-not-signed&code=x", nil) + rec := httptest.NewRecorder() + bc.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (placeholder page)", rec.Code) + } + if !strings.Contains(rec.Body.String(), "không hợp lệ") { + t.Errorf("body doesn't mention invalid link: %s", rec.Body.String()) + } +} + +func TestRouterUserOAuthCallback_HappyPath(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(1058, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, mcpStore := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + keyBytes, err := crypto.DeriveKey(testOAuthEncKey) + if err != nil { + t.Fatalf("derive key: %v", err) + } + state, err := encodeOAuthState(*testStatePayload(bc, "1058"), keyBytes) + if err != nil { + t.Fatalf("encode state: %v", err) + } + + // domain param mirrors a REAL Bitrix redirect (the authorize-step's own + // domain param, matching payload.Domain) — must NOT be confused with + // tr.Domain in the token-exchange response body (oauth.bitrix.info). + req := httptest.NewRequest(http.MethodGet, "/bitrix24/handler?state="+state+"&code=good-code&domain=portal.bitrix24.com", nil) + rec := httptest.NewRecorder() + bc.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + if _, ok := mcpStore.userCreds[credKey(bc.mcpServerID, "1058")]; !ok { + t.Fatal("expected mcp_user_credentials row to be minted via full HTTP route") + } +} + +// TestRouterUserOAuthCallback_HappyPath_ViaLegacyInstallURL covers dev/local +// Bitrix app settings that still point Application URL at /bitrix24/install. +// The signed per-user OAuth state must be rerouted into handleUserOAuthCallback +// instead of being parsed as the old install state format (:). +func TestRouterUserOAuthCallback_HappyPath_ViaLegacyInstallURL(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(1058, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, mcpStore := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + keyBytes, err := crypto.DeriveKey(testOAuthEncKey) + if err != nil { + t.Fatalf("derive key: %v", err) + } + state, err := encodeOAuthState(*testStatePayload(bc, "1058"), keyBytes) + if err != nil { + t.Fatalf("encode state: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/bitrix24/install?state="+state+"&code=good-code&domain=portal.bitrix24.com", nil) + rec := httptest.NewRecorder() + bc.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + if _, ok := mcpStore.userCreds[credKey(bc.mcpServerID, "1058")]; !ok { + t.Fatal("expected mcp_user_credentials row to be minted via legacy install URL reroute") + } +} + +// TestRouterUserOAuthCallback_DomainMismatch verifies the redirect's own +// `domain` query param (the portal domain, per Bitrix's authorize-redirect +// contract) is checked against the signed state's domain BEFORE any +// dispatcher lookup or code exchange — catching a forged/replayed redirect +// aimed at the wrong portal. +func TestRouterUserOAuthCallback_DomainMismatch(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(1058, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("auto-onboard must not be called on domain mismatch") + })) + defer mcpSrv.Close() + + bc, mcpStore := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + keyBytes, err := crypto.DeriveKey(testOAuthEncKey) + if err != nil { + t.Fatalf("derive key: %v", err) + } + state, err := encodeOAuthState(*testStatePayload(bc, "1058"), keyBytes) // payload.Domain = portal.bitrix24.com + if err != nil { + t.Fatalf("encode state: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/bitrix24/handler?state="+state+"&code=good-code&domain=attacker-portal.bitrix24.com", nil) + rec := httptest.NewRecorder() + bc.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (error page)", rec.Code) + } + if !strings.Contains(rec.Body.String(), "không hợp lệ") { + t.Errorf("body doesn't mention invalid link: %s", rec.Body.String()) + } + if mcpStore.setUserCallCount != 0 { + t.Errorf("domain mismatch must not mint credentials; setUserCallCount=%d", mcpStore.setUserCallCount) + } +} + +// TestRouterAppPage_NoStateFallsBackToPlaceholder is a regression guard: a +// plain GET to /bitrix24/handler with no `state` param (the pre-existing +// registration-ping / iframe-load case) must still get the original +// placeholder, not be swallowed by the new OAuth-callback branch handleAppPage +// gained when it started sharing this route with the per-user re-auth flow. +func TestRouterAppPage_NoStateFallsBackToPlaceholder(t *testing.T) { + oauthSrv := httptest.NewServer(oauthTokenHandler(1058, false)) + defer oauthSrv.Close() + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + + bc, _ := newOAuthFlowTestChannel(t, mcpSrv, oauthSrv) + + req := httptest.NewRequest(http.MethodGet, "/bitrix24/handler", nil) + rec := httptest.NewRecorder() + bc.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if !strings.Contains(rec.Body.String(), "GoClaw") { + t.Errorf("expected the original app-page placeholder body, got: %s", rec.Body.String()) + } +} diff --git a/internal/channels/bitrix24/portal.go b/internal/channels/bitrix24/portal.go index 7e879ee0..9663a4ec 100644 --- a/internal/channels/bitrix24/portal.go +++ b/internal/channels/bitrix24/portal.go @@ -445,6 +445,35 @@ func (p *Portal) RefreshUserToken(ctx context.Context, refreshToken string) (*To return p.client.RefreshToken(ctx, p.creds.ClientID, p.creds.ClientSecret, refreshToken) } +// ExchangeUserAuthCode exchanges an authorization code for a per-user OAuth +// token pair WITHOUT touching the portal's own bot-level token state — the +// caller decides what to do with the resulting user tokens (mint MCP +// credentials) instead of persisting them as portal state. Used by the +// per-user OAuth re-auth flow (oauth_user_flow.go). +// +// Deliberately does NOT call validateTokenResponseIdentity (used by Exchange +// for the app-install flow) — confirmed against live Bitrix behavior that for +// THIS kind of exchange (a user-authorize code, not an app-install code), +// the response's `domain` field is the OAuth server's own domain +// ("oauth.bitrix.info"), never the portal's — that check would reject every +// call, for every portal, regardless of Local App vs Marketplace app (the +// portal type is a per-customer choice; Exchange/validateTokenResponseIdentity +// stays untouched for both). The caller (HandleUserOAuthCallback, +// oauth_user_flow.go) instead validates the portal domain from the redirect's +// own `domain` query param against the signed state, and the authorizing +// user's identity against tr.UserID — both are meaningful for this flow; +// tr.Domain is not. +func (p *Portal) ExchangeUserAuthCode(ctx context.Context, code string) (*TokenResponse, error) { + if code == "" { + return nil, errors.New("bitrix24 user exchange: code required") + } + tr, err := p.client.ExchangeAuthCode(ctx, p.creds.ClientID, p.creds.ClientSecret, code) + if err != nil { + return nil, fmt.Errorf("bitrix24 user exchange: %w", err) + } + return tr, nil +} + func (p *Portal) validateTokenResponseIdentity(flow string, tr *TokenResponse) error { if tr == nil { return fmt.Errorf("bitrix24 %s: nil token response", flow) diff --git a/internal/channels/bitrix24/provisioner.go b/internal/channels/bitrix24/provisioner.go index 5b156c91..b9eb4c1a 100644 --- a/internal/channels/bitrix24/provisioner.go +++ b/internal/channels/bitrix24/provisioner.go @@ -64,6 +64,39 @@ var ( ErrProvisionDebounced = errors.New("bitrix24 mcp: provisioning debounced") ) +// deadTokenCodes are Bitrix APIError.Code values (client.go:82-97) that mean +// a refresh_token is dead beyond repair — Bitrix rejected the refresh +// attempt outright, so no amount of retrying will succeed. Only these codes +// escalate to ErrUserAuthRequired; any other error from selfRefreshUserCreds +// (network, 5xx, DB persist failure) is treated as transient and falls +// through to the existing notifyUserOfMCPIssueOnce path instead. +var deadTokenCodes = map[string]bool{ + "invalid_grant": true, + "expired_token": true, + "NO_AUTH_FOUND": true, +} + +// isDeadTokenCode reports whether a Bitrix APIError.Code indicates the +// stored refresh_token is permanently dead (user must re-authorize) rather +// than a transient failure. +func isDeadTokenCode(code string) bool { + return deadTokenCodes[code] +} + +// ErrUserAuthRequired means goclaw has no way to obtain a working Bitrix +// OAuth token for this user — either no mcp_user_credentials row exists yet, +// or the stored refresh_token was rejected outright by Bitrix +// (isDeadTokenCode). The caller (handle.go) must DM the user the URL so they +// can re-authorize; the message must NOT reach the agent bus (no MCP access +// to answer with yet). +type ErrUserAuthRequired struct { + URL string +} + +func (e *ErrUserAuthRequired) Error() string { + return "bitrix24 mcp: user authorization required" +} + // initMCPProvisioner wires the lazy-provisioning plumbing at Start() time. // Safe to call even when provisioning is disabled — in that case it just // returns nil without touching mcpStore. @@ -188,7 +221,10 @@ func (c *Channel) initMCPProvisioner(ctx context.Context) error { // HandleMessage regardless, so user messages always get processed. // // Called from handleMessage after EnsureContact, before HandleMessage. -func (c *Channel) provisionIfMissing(ctx context.Context, userID string, fromConnector bool, auth EventAuth) error { +// dialogID is the inbound event's DialogID — needed to build the OAuth +// authorize URL's state payload (oauth_state_codec.go) when this function +// must return ErrUserAuthRequired. +func (c *Channel) provisionIfMissing(ctx context.Context, userID string, fromConnector bool, auth EventAuth, dialogID string) error { // Skip #1: Open Channel message from an external connector customer. // Transient customers reach the bot via a connector (Zalo/FB/etc.) and // report IS_CONNECTOR=Y — they are not Bitrix users and have no per-user @@ -279,12 +315,39 @@ func (c *Channel) provisionIfMissing(ctx context.Context, userID string, fromCon // to trace to mcp_client.go. if auth.Domain == "" || auth.AccessToken == "" || auth.RefreshToken == "" { // Direct/group chatbot events don't carry OAuth tokens in the top-level - // auth[] block — Bitrix only ships them on Open Channel events. If this - // user already has stored credentials, refresh the USER-context token - // from the stored refresh_token instead of failing, so per-user MCP - // stays alive without depending on the event carrying tokens. + // auth[] block — Bitrix only ships them on Open Channel events (see + // design.md — confirmed by Bitrix24 support as expected behavior, not + // a bug: imbot subscriptions bind with USER_ID=0). + // + // Brand-new user (never onboarded) — nothing to refresh. Send the user + // an OAuth re-authorize link instead of failing silently. + if existing == nil { + url, err := c.BuildUserAuthorizeURL(userID, dialogID) + if err != nil { + return fmt.Errorf("bitrix24 mcp: build authorize url: %w", err) + } + return &ErrUserAuthRequired{URL: url} + } + + // Existing user — try refreshing the USER-context token from the + // stored refresh_token instead of failing, so per-user MCP stays + // alive without depending on the event carrying tokens. if err := c.selfRefreshUserCreds(ctx, userID, existing); err != nil { - return err + var apiErr *APIError + if errors.As(err, &apiErr) && isDeadTokenCode(apiErr.Code) { + // refresh_token is dead beyond repair — same re-auth flow as + // the brand-new-user case above. Do NOT delete the existing + // credential row: SetUserCredentials upserts on + // (server_id, user_id, tenant_id), so completing the OAuth + // flow overwrites this row's env in place (design.md §12 — + // deleting first would be unnecessary churn). + url, urlErr := c.BuildUserAuthorizeURL(userID, dialogID) + if urlErr != nil { + return fmt.Errorf("bitrix24 mcp: build authorize url: %w", urlErr) + } + return &ErrUserAuthRequired{URL: url} + } + return err // transient (network/5xx/DB) — falls through to notifyUserOfMCPIssueOnce } return nil } @@ -485,6 +548,25 @@ const mcpUserNotifyMessage = "⚠️ Hệ thống đang gặp vấn đề với "Vui lòng liên hệ admin kỹ thuật để xem lại. " + "Tôi vẫn có thể trả lời các câu hỏi cơ bản khác." +// oauthInviteMessage is the short hint sent alongside the keyboard button +// (handle.go, sendOAuthInvite) for a user who has no MCP credentials yet or +// whose refresh_token died. Contains NO url — the url lives only in the +// keyboard button's LINK field, built at the call site from +// Channel.BuildUserAuthorizeURL (oauth_state_codec.go). Vietnamese-first for +// the same reason as mcpUserNotifyMessage above (channel doesn't thread +// locale yet). +const oauthInviteMessage = "🔐 Vui lòng bấm nút bên dưới để cấp quyền truy cập CRM " + + "(link có hiệu lực 10 phút)." + +// oauthInviteButtonText is the label of the single keyboard button sent with +// oauthInviteMessage. +const oauthInviteButtonText = "Cấp quyền truy cập CRM" + +// oauthInviteGroupHintMessage is sent in the ORIGINAL group/dialog only when +// the oauth-invite trigger came from a group chat, so the user knows to check +// their private messages. Never contains the URL. +const oauthInviteGroupHintMessage = "Đã gửi bạn tin nhắn riêng để cấp quyền truy cập CRM." + // notifyUserOfMCPIssueOnce sends a one-shot degradation notice to the // Bitrix24 user via imbot.message.add when provisioning fails in an // unexpected way. Debounced per-user with mcpUserNotifyDebounceTTL so diff --git a/internal/channels/bitrix24/provisioner_test.go b/internal/channels/bitrix24/provisioner_test.go index 4dfd30bd..ff05d264 100644 --- a/internal/channels/bitrix24/provisioner_test.go +++ b/internal/channels/bitrix24/provisioner_test.go @@ -214,7 +214,7 @@ func TestProvisionIfMissing_OpenChannelBot_Skipped(t *testing.T) { bc := ch.(*Channel) // IS_CONNECTOR=Y (external connector customer) → skipped: not a Bitrix user. - err = bc.provisionIfMissing(context.Background(), "42", true, validAuth()) + err = bc.provisionIfMissing(context.Background(), "42", true, validAuth(), "chat123") if !errors.Is(err, ErrProvisionSkippedOpenChannel) { t.Fatalf("connector message: err = %v; want ErrProvisionSkippedOpenChannel", err) } @@ -223,7 +223,7 @@ func TestProvisionIfMissing_OpenChannelBot_Skipped(t *testing.T) { } // IS_CONNECTOR=N (internal staff in an Open Channel) → must NOT connector-skip. - if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()); errors.Is(err, ErrProvisionSkippedOpenChannel) { + if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); errors.Is(err, ErrProvisionSkippedOpenChannel) { t.Fatalf("internal staff (IS_CONNECTOR=N) must not be connector-skipped; got %v", err) } } @@ -246,7 +246,7 @@ func TestProvisionIfMissing_Disabled(t *testing.T) { } bc := ch.(*Channel) - err = bc.provisionIfMissing(context.Background(), "42", false, validAuth()) + err = bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123") if !errors.Is(err, ErrProvisionDisabled) { t.Fatalf("err = %v; want ErrProvisionDisabled", err) } @@ -280,7 +280,7 @@ func TestProvisionIfMissing_ExistingCreds_NoHTTP(t *testing.T) { }, } - if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()); err != nil { + if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil { t.Fatalf("err = %v; want nil", err) } if httpCalls != 0 { @@ -318,7 +318,7 @@ func TestProvisionIfMissing_NearExpiry_RefreshHTTP(t *testing.T) { }, } - if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()); err != nil { + if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil { t.Fatalf("err = %v; want nil", err) } if httpCalls != 1 { @@ -351,7 +351,7 @@ func TestProvisionIfMissing_LegacyNoExpiry_RefreshHTTP(t *testing.T) { APIKey: "legacy-key", } - if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()); err != nil { + if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil { t.Fatalf("err = %v; want nil", err) } if httpCalls != 1 { @@ -385,7 +385,7 @@ func TestProvisionIfMissing_WarmExpiry_NoHTTP(t *testing.T) { }, } - if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()); err != nil { + if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil { t.Fatalf("err = %v; want nil", err) } if httpCalls != 0 { @@ -409,7 +409,7 @@ func TestProvisionIfMissing_MintAndPersist(t *testing.T) { bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B") before := time.Now() - err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()) + err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123") if err != nil { t.Fatalf("provisionIfMissing: %v", err) } @@ -467,7 +467,7 @@ func TestProvisionIfMissing_Debounce(t *testing.T) { bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B") // First attempt succeeds and marks the debounce. - if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()); err != nil { + if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil { t.Fatalf("first attempt: %v", err) } if httpCalls != 1 { @@ -482,7 +482,7 @@ func TestProvisionIfMissing_Debounce(t *testing.T) { delete(mcpStore.userCreds, credKey(bc.mcpServerID, "42")) mcpStore.mu.Unlock() - err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()) + err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123") if !errors.Is(err, ErrProvisionDebounced) { t.Fatalf("second attempt: %v; want ErrProvisionDebounced", err) } @@ -505,7 +505,7 @@ func TestProvisionIfMissing_HTTPFailure_Surfaces(t *testing.T) { mcpStore := newFakeMCPStore() bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B") - err := bc.provisionIfMissing(context.Background(), "42", false, validAuth()) + err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123") if err == nil { t.Fatal("401 from MCP must produce an error") } @@ -550,7 +550,7 @@ func TestProvisionIfMissing_MissingAuthBlock(t *testing.T) { before := httpCalls // Use a fresh userID per subcase so the debounce from a prior // case doesn't mask a regression. - err := bc.provisionIfMissing(context.Background(), tc.name, false, tc.auth) + err := bc.provisionIfMissing(context.Background(), tc.name, false, tc.auth, "chat123") if err == nil { t.Fatalf("missing %s should fail", tc.name) } @@ -561,6 +561,136 @@ func TestProvisionIfMissing_MissingAuthBlock(t *testing.T) { } } +// attachTestPortal wires a real *Portal (OAuth calls routed to oauthSrv) onto +// an already-built provisioner test channel, plus a derivable encKey — needed +// for the ErrUserAuthRequired branch tests below, which call +// Channel.BuildUserAuthorizeURL (requires portal.PublicURL() + a valid key). +func attachTestPortal(t *testing.T, bc *Channel, oauthSrv *httptest.Server) { + t.Helper() + bc.encKey = testOAuthEncKey + portalFS := newFakeStore() + portal := newTestPortal(t, oauthSrv, portalFS, bc.TenantID(), "p", + store.BitrixPortalState{PublicURL: "https://goclaw.example.com"}) + bc.startMu.Lock() + bc.portal = portal + bc.startMu.Unlock() +} + +// TestProvisionIfMissing_NewUser_ReturnsAuthRequired covers the brand-new-user +// branch (existing == nil, no auth in the event): must return +// *ErrUserAuthRequired with a non-empty URL, WITHOUT calling the MCP server +// or attempting a Bitrix token refresh (there's nothing to refresh). +func TestProvisionIfMissing_NewUser_ReturnsAuthRequired(t *testing.T) { + mcpSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("brand-new user with no auth in event must not hit MCP auto-onboard") + })) + defer mcpSrv.Close() + oauthSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("brand-new user must not attempt a Bitrix token refresh either") + })) + defer oauthSrv.Close() + + mcpStore := newFakeMCPStore() + bc := newProvisionerTestChannel(t, mcpStore, mcpSrv.URL, "B") + attachTestPortal(t, bc, oauthSrv) + + err := bc.provisionIfMissing(context.Background(), "999", false, EventAuth{}, "chat123") + var authErr *ErrUserAuthRequired + if !errors.As(err, &authErr) { + t.Fatalf("err = %v, want *ErrUserAuthRequired", err) + } + if authErr.URL == "" { + t.Error("ErrUserAuthRequired.URL must not be empty") + } +} + +// TestProvisionIfMissing_DeadRefreshToken_ReturnsAuthRequired_RowUntouched +// covers the "existing user, refresh_token is dead" branch: Bitrix rejects +// the refresh with invalid_grant → must escalate to *ErrUserAuthRequired +// WITHOUT deleting the existing mcp_user_credentials row (design.md §12 — +// SetUserCredentials upserts in place once the user re-authorizes; deleting +// first would be unnecessary churn). +func TestProvisionIfMissing_DeadRefreshToken_ReturnsAuthRequired_RowUntouched(t *testing.T) { + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + oauthSrv := httptest.NewServer(oauthTokenHandler(0, true)) // invalid_grant + defer oauthSrv.Close() + + mcpStore := newFakeMCPStore() + bc := newProvisionerTestChannel(t, mcpStore, mcpSrv.URL, "B") + attachTestPortal(t, bc, oauthSrv) + + staleCreds := store.MCPUserCredentials{ + APIKey: "old-key", + Env: map[string]string{ + "BITRIX_DOMAIN": "portal.bitrix24.com", + "BITRIX_ACCESS_TOKEN": "old-access", + "BITRIX_REFRESH_TOKEN": "dead-refresh", + "BITRIX_EXPIRES_AT": time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), + }, + } + mcpStore.mu.Lock() + mcpStore.userCreds[credKey(bc.mcpServerID, "1058")] = staleCreds + mcpStore.mu.Unlock() + + err := bc.provisionIfMissing(context.Background(), "1058", false, EventAuth{}, "chat123") + var authErr *ErrUserAuthRequired + if !errors.As(err, &authErr) { + t.Fatalf("err = %v, want *ErrUserAuthRequired", err) + } + + mcpStore.mu.Lock() + got, ok := mcpStore.userCreds[credKey(bc.mcpServerID, "1058")] + mcpStore.mu.Unlock() + if !ok { + t.Fatal("existing row must NOT be deleted on dead-token classification") + } + if got.APIKey != "old-key" { + t.Errorf("row was modified; APIKey = %q, want unchanged %q", got.APIKey, "old-key") + } +} + +// TestProvisionIfMissing_TransientRefreshError_NotAuthRequired ensures a +// non-classified refresh failure (network/5xx — here a bare 500 with no +// Bitrix `error` field, so APIError.Code == "") does NOT escalate to +// *ErrUserAuthRequired — it must fall through to the existing generic error +// path (handle.go's notifyUserOfMCPIssueOnce), since a retry might just work +// without bothering the user for re-authorization. +func TestProvisionIfMissing_TransientRefreshError_NotAuthRequired(t *testing.T) { + mcpSrv := httptest.NewServer(mcpAutoOnboardHandler()) + defer mcpSrv.Close() + oauthSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{}`)) + })) + defer oauthSrv.Close() + + mcpStore := newFakeMCPStore() + bc := newProvisionerTestChannel(t, mcpStore, mcpSrv.URL, "B") + attachTestPortal(t, bc, oauthSrv) + + staleCreds := store.MCPUserCredentials{ + Env: map[string]string{ + "BITRIX_DOMAIN": "portal.bitrix24.com", + "BITRIX_ACCESS_TOKEN": "old-access", + "BITRIX_REFRESH_TOKEN": "still-alive-refresh", + "BITRIX_EXPIRES_AT": time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), + }, + } + mcpStore.mu.Lock() + mcpStore.userCreds[credKey(bc.mcpServerID, "42")] = staleCreds + mcpStore.mu.Unlock() + + err := bc.provisionIfMissing(context.Background(), "42", false, EventAuth{}, "chat123") + var authErr *ErrUserAuthRequired + if errors.As(err, &authErr) { + t.Fatalf("transient 5xx must NOT escalate to ErrUserAuthRequired, got %v", err) + } + if err == nil { + t.Fatal("expected a transient error, got nil") + } +} + // TestInitMCPProvisioner_DisabledModes covers the configurations that // leave the provisioner off at startup (all non-fatal): // - nil MCPServerStore diff --git a/internal/channels/bitrix24/router.go b/internal/channels/bitrix24/router.go index 1a440c2d..dccf2d59 100644 --- a/internal/channels/bitrix24/router.go +++ b/internal/channels/bitrix24/router.go @@ -31,6 +31,12 @@ const ( // iframe-loads it (with POST tokens) when a user opens the app inside // their portal. See handleAppPage for behavior. handlerPath = "/bitrix24/handler" + // userOAuthCallbackPath is where Bitrix redirects a user's browser after + // they approve (or decline) the per-user re-authorization link sent via + // DM (see oauth_state_codec.go BuildUserAuthorizeURL, oauth_user_flow.go). + // Public — no gateway auth — same as installPath; safety comes from the + // HMAC-signed state, not route-level auth (design.md §9, §5). + userOAuthCallbackPath = "/bitrix24/oauth/user/callback" ) const ambiguousDomainKey = "\x00ambiguous-domain" @@ -208,6 +214,18 @@ func (r *Router) UnregisterBot(botID int) { r.mu.Unlock() } +// DispatcherByBotID returns the registered dispatcher for a bot id, if any. +// Used by handleUserOAuthCallback to resolve which Channel should mint MCP +// credentials for a completed per-user re-authorization — the decoded state +// payload carries BotID (oauth_state_codec.go) since Bitrix's redirect query +// params carry no bot_id of their own. +func (r *Router) DispatcherByBotID(botID int) (BotDispatcher, bool) { + r.mu.RLock() + defer r.mu.RUnlock() + d, ok := r.byBotID[botID] + return d, ok +} + // PortalByKey returns the portal registered under (tenant, name), if any. // Exported for tests and for Phase 03 channel bootstrap. func (r *Router) PortalByKey(tenantID uuid.UUID, name string) (*Portal, bool) { @@ -311,6 +329,8 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) { r.handleEvent(w, req) case handlerPath: r.handleAppPage(w, req) + case userOAuthCallbackPath: + r.handleUserOAuthCallback(w, req, strings.TrimSpace(req.URL.Query().Get("state"))) default: http.NotFound(w, req) } diff --git a/internal/channels/bitrix24/webhook.go b/internal/channels/bitrix24/webhook.go index c1b561c1..f3011742 100644 --- a/internal/channels/bitrix24/webhook.go +++ b/internal/channels/bitrix24/webhook.go @@ -10,6 +10,9 @@ import ( "strconv" "strings" + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/crypto" "github.com/nextlevelbuilder/goclaw/internal/store" ) @@ -186,6 +189,18 @@ func (r *Router) handleInstall(w http.ResponseWriter, req *http.Request) { tid, name, ok := parseInstallState(stateParam) if !ok { + // Backward-compat for dev/local Bitrix apps whose registered + // "Application URL" still points at /bitrix24/install. The per-user + // OAuth re-authorization flow now signs its own state and expects the + // redirect on /bitrix24/handler, but older app settings can still send + // that exact redirect here. Detect the signed-state shape and hand the + // request to the user-OAuth callback path instead of treating it as a + // broken app-install attempt. + if req.Method == http.MethodGet && strings.Contains(stateParam, ".") && !strings.Contains(stateParam, ":") { + slog.Info("bitrix24 install: rerouting signed user oauth callback from legacy install URL") + r.handleUserOAuthCallback(w, req, stateParam) + return + } http.Error(w, "invalid state format", http.StatusBadRequest) return } @@ -230,6 +245,99 @@ func (r *Router) handleInstall(w http.ResponseWriter, req *http.Request) { _, _ = w.Write([]byte(installSuccessHTML)) } +// handleUserOAuthCallback processes an OAuth per-user re-authorization +// redirect. Called FROM handleAppPage when it detects a `state` query param +// on a GET to /bitrix24/handler — Bitrix always redirects here after a user +// approves/declines, using the app's registered "Application URL" +// (handlerPath). It does NOT honor a `redirect_uri` parameter for Local Apps +// (confirmed against live Bitrix behavior: the redirect landed on +// handlerPath with our signed `state` attached, not on the dedicated route +// this function used to be mounted at — see BuildUserAuthorizeURL, which no +// longer sends redirect_uri at all since Bitrix ignores it). +// +// Public — no gateway auth (handleAppPage already is). Safety comes entirely +// from the HMAC-signed + TTL-bound state (design.md §5): a malformed or +// expired state is rejected before any Bitrix network call or dispatcher +// lookup. +func (r *Router) handleUserOAuthCallback(w http.ResponseWriter, req *http.Request, stateParam string) { + q := req.URL.Query() + code := strings.TrimSpace(q.Get("code")) + errParam := strings.TrimSpace(q.Get("error")) + + keyBytes, err := crypto.DeriveKey(r.encKey) + if err != nil { + slog.Warn("bitrix24 oauth user callback: derive state key failed", "err", err) + renderBitrixPlaceholder(w, "Liên kết không hợp lệ", "Không thể xác thực yêu cầu này. Vui lòng thử lại từ tin nhắn bot.") + return + } + payload, err := decodeOAuthState(stateParam, keyBytes) + if err != nil { + slog.Warn("bitrix24 oauth user callback: invalid state", "err", err) + renderBitrixPlaceholder(w, "Liên kết không hợp lệ hoặc đã hết hạn", "Vui lòng nhắn lại cho bot để nhận link cấp quyền mới.") + return + } + + if errParam != "" { + slog.Info("bitrix24 oauth user callback: user declined", + "user_id", payload.UserID, "bot_id", payload.BotID, "error", errParam) + renderBitrixPlaceholder(w, "Đã từ chối cấp quyền", "Bot sẽ không truy cập được CRM thay mặt bạn. Bạn có thể nhắn lại bot để thử lại.") + return + } + + // Domain check: use the redirect's OWN `domain` query param (the portal + // domain where authorization took place, per Bitrix's authorize-redirect + // contract) against the domain signed into our state at invite-build + // time — NOT the token-exchange response's `domain` field, which is + // always the OAuth server's own domain ("oauth.bitrix.info"), confirmed + // against live Bitrix behavior (see ExchangeUserAuthCode doc comment). + if redirectDomain := strings.TrimSpace(q.Get("domain")); redirectDomain != "" && + !strings.EqualFold(redirectDomain, payload.Domain) { + slog.Warn("bitrix24 oauth user callback: domain mismatch", + "user_id", payload.UserID, "expected", payload.Domain, "got", redirectDomain) + renderBitrixPlaceholder(w, "Liên kết không hợp lệ", "Vui lòng nhắn lại cho bot để nhận link cấp quyền mới.") + return + } + + dispatcher, ok := r.DispatcherByBotID(payload.BotID) + if !ok { + slog.Warn("bitrix24 oauth user callback: bot not registered", + "user_id", payload.UserID, "bot_id", payload.BotID) + renderBitrixPlaceholder(w, "Không tìm thấy bot", "Bot có thể đã bị gỡ khỏi portal. Vui lòng liên hệ admin.") + return + } + ch, ok := dispatcher.(*Channel) + if !ok { + slog.Warn("bitrix24 oauth user callback: dispatcher is not a *Channel", "bot_id", payload.BotID) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + + tid, err := uuid.Parse(payload.TenantID) + if err != nil { + slog.Warn("bitrix24 oauth user callback: invalid tenant id in state", "tenant_id", payload.TenantID, "err", err) + renderBitrixPlaceholder(w, "Liên kết không hợp lệ", "Vui lòng nhắn lại cho bot để nhận link cấp quyền mới.") + return + } + ctx := store.WithTenantID(req.Context(), tid) + + result, err := ch.HandleUserOAuthCallback(ctx, code, payload) + if err != nil { + slog.Warn("bitrix24 oauth user callback: failed", + "user_id", payload.UserID, "bot_id", payload.BotID, "err", err) + renderBitrixPlaceholder(w, "Cấp quyền thất bại", "Đã xảy ra lỗi khi xử lý yêu cầu. Vui lòng nhắn lại cho bot để thử lại.") + return + } + + switch result.Outcome { + case "identity_mismatch": + renderBitrixPlaceholder(w, "Tài khoản không khớp", + "Bạn đã đăng nhập bằng tài khoản Bitrix24 khác với người mà bot đã gửi lời mời. Vui lòng đăng nhập đúng tài khoản và thử lại.") + default: // "success" + renderBitrixPlaceholder(w, "Đã cấp quyền thành công", + "Bạn có thể quay lại chat với bot để tiếp tục.") + } +} + // handleInstallLocalApp finishes install for a Bitrix24 Local App. Body already // parsed by the caller; AUTH_ID + REFRESH_ID presence already checked. // @@ -533,11 +641,16 @@ func renderBitrixPlaceholder(w http.ResponseWriter, title, body string) { } // handleAppPage serves /bitrix24/handler — the URL Bitrix24 iframe-loads -// when a user opens the GoClaw app inside their portal interface. Used as -// the "Application URL" and "Application settings handler" in the partner -// app registration form. +// when a user opens the GoClaw app inside their portal interface, AND the +// fixed "Application URL" Bitrix redirects to after a Local App user +// completes (or declines) the OAuth per-user re-authorization flow +// (oauth_state_codec.go BuildUserAuthorizeURL, handle.go sendOAuthInvite) — +// Local Apps ignore any `redirect_uri` we pass and always come back here +// instead, confirmed against live behavior. A GET carrying `state` is +// therefore that OAuth redirect, not a plain iframe-load/registration ping, +// and is dispatched to handleUserOAuthCallback. // -// Currently responds with the 200 placeholder required by Bitrix24 app URL +// Otherwise responds with the 200 placeholder required by Bitrix24 app URL // validation. A future POST handler can process the opening user's Bitrix24 // tokens and forward them to the MCP onboarding endpoint. func (r *Router) handleAppPage(w http.ResponseWriter, req *http.Request) { @@ -550,6 +663,12 @@ func (r *Router) handleAppPage(w http.ResponseWriter, req *http.Request) { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return } + if req.Method == http.MethodGet { + if stateParam := strings.TrimSpace(req.URL.Query().Get("state")); stateParam != "" { + r.handleUserOAuthCallback(w, req, stateParam) + return + } + } renderBitrixPlaceholder(w, "GoClaw — Bitrix24 Application", "This page is loaded inside Bitrix24 when a user opens the GoClaw bot application.")