feat(bitrix24): ship per-user OAuth re-authorization flow (#1417)

feat(bitrix24): ship per-user OAuth re-authorization flow (#1417)
This commit is contained in:
SYNITY authored and GitHub committed 2026-07-10 10:56:45 +07:00
1 parent 9be468edad
commit 641d4f8756
13 files changed
+1697 -23

No files matched your search

@@ -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.
+7
View File
@@ -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
+10 -1
View File
@@ -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),
@@ -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)
}
}
@@ -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
}
@@ -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")
}
}
@@ -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
}
@@ -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 (<tenant>:<portal>).
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())
}
}
+29
View File
@@ -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)
+88 -6
View File
@@ -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
+142 -12
View File
@@ -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
+20
View File
@@ -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)
}
+123 -4
View File
@@ -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.")