Files
SYNITY 641d4f8756 feat(bitrix24): ship per-user OAuth re-authorization flow (#1417)
feat(bitrix24): ship per-user OAuth re-authorization flow (#1417)
2026-07-10 10:56:45 +07:00

1080 lines
41 KiB
Go
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
package bitrix24
import (
"context"
"encoding/json"
"errors"
"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"
)
// fakeMCPStore implements store.MCPServerStore for provisioner tests.
// Only the methods provisionIfMissing actually exercises are implemented
// with behaviour — the rest satisfy the interface with zero returns so the
// test file compiles cleanly.
//
// All tests that touch fakeMCPStore pass a *preloaded* server row because
// GetServerByName is what initMCPProvisioner calls at startup; the other
// fields exist to be mutated by SetUserCredentials and observed by the
// test assertions.
type fakeMCPStore struct {
mu sync.Mutex
serversByName map[string]*store.MCPServerData
serversByID map[uuid.UUID]*store.MCPServerData
userCreds map[string]store.MCPUserCredentials // key = serverID + ":" + userID
getUserCallCount int
setUserCallCount int
}
func newFakeMCPStore() *fakeMCPStore {
return &fakeMCPStore{
serversByName: map[string]*store.MCPServerData{},
serversByID: map[uuid.UUID]*store.MCPServerData{},
userCreds: map[string]store.MCPUserCredentials{},
}
}
func credKey(serverID uuid.UUID, userID string) string {
return serverID.String() + ":" + userID
}
// --- MCPServerStore methods that matter for the provisioner --------------
func (f *fakeMCPStore) GetServerByName(_ context.Context, name string) (*store.MCPServerData, error) {
f.mu.Lock()
defer f.mu.Unlock()
if s, ok := f.serversByName[name]; ok {
return s, nil
}
return nil, nil // partner's contract: nil + nil when absent
}
func (f *fakeMCPStore) GetUserCredentials(_ context.Context, serverID uuid.UUID, userID string) (*store.MCPUserCredentials, error) {
f.mu.Lock()
defer f.mu.Unlock()
f.getUserCallCount++
if c, ok := f.userCreds[credKey(serverID, userID)]; ok {
return &c, nil
}
return nil, nil
}
func (f *fakeMCPStore) SetUserCredentials(_ context.Context, serverID uuid.UUID, userID string, creds store.MCPUserCredentials) error {
f.mu.Lock()
defer f.mu.Unlock()
f.setUserCallCount++
f.userCreds[credKey(serverID, userID)] = creds
return nil
}
// --- MCPServerStore methods the provisioner doesn't touch ----------------
func (f *fakeMCPStore) CreateServer(_ context.Context, _ *store.MCPServerData) error {
return nil
}
func (f *fakeMCPStore) GetServer(_ context.Context, id uuid.UUID) (*store.MCPServerData, error) {
f.mu.Lock()
defer f.mu.Unlock()
if s, ok := f.serversByID[id]; ok {
return s, nil
}
return nil, nil // partner's contract: nil + nil when absent
}
func (f *fakeMCPStore) ListServers(_ context.Context) ([]store.MCPServerData, error) { return nil, nil }
func (f *fakeMCPStore) UpdateServer(_ context.Context, _ uuid.UUID, _ map[string]any) error {
return nil
}
func (f *fakeMCPStore) DeleteServer(_ context.Context, _ uuid.UUID) error { return nil }
func (f *fakeMCPStore) GrantToAgent(_ context.Context, _ *store.MCPAgentGrant) error {
return nil
}
func (f *fakeMCPStore) RevokeFromAgent(_ context.Context, _, _ uuid.UUID) error { return nil }
func (f *fakeMCPStore) ListAgentGrants(_ context.Context, _ uuid.UUID) ([]store.MCPAgentGrant, error) {
return nil, nil
}
func (f *fakeMCPStore) ListServerGrants(_ context.Context, _ uuid.UUID) ([]store.MCPAgentGrant, error) {
return nil, nil
}
func (f *fakeMCPStore) GrantToUser(_ context.Context, _ *store.MCPUserGrant) error { return nil }
func (f *fakeMCPStore) RevokeFromUser(_ context.Context, _ uuid.UUID, _ string) error {
return nil
}
func (f *fakeMCPStore) CountAgentGrantsByServer(_ context.Context) (map[uuid.UUID]int, error) {
return nil, nil
}
func (f *fakeMCPStore) ListAccessible(_ context.Context, _ uuid.UUID, _ string) ([]store.MCPAccessInfo, error) {
return nil, nil
}
func (f *fakeMCPStore) CreateRequest(_ context.Context, _ *store.MCPAccessRequest) error {
return nil
}
func (f *fakeMCPStore) ListPendingRequests(_ context.Context) ([]store.MCPAccessRequest, error) {
return nil, nil
}
func (f *fakeMCPStore) ReviewRequest(_ context.Context, _ uuid.UUID, _ bool, _, _ string) error {
return nil
}
func (f *fakeMCPStore) DeleteUserCredentials(_ context.Context, _ uuid.UUID, _ string) error {
return nil
}
func (f *fakeMCPStore) CacheToolDescriptions(_ context.Context, _ uuid.UUID, _ map[string]store.CachedToolInfo) error {
return nil
}
// --- Test helpers --------------------------------------------------------
// newProvisionerTestChannel builds a started Channel with provisioning
// enabled against the given (fake MCP server URL, fake MCP store). Used
// by the happy-path tests that need the full provisioner wired up.
func newProvisionerTestChannel(t *testing.T, mcpStore *fakeMCPStore, mcpBaseURL string, botType string) *Channel {
t.Helper()
fs := newFakeStore()
resetWebhookRouterForTest()
t.Cleanup(resetWebhookRouterForTest)
// Seed the fake MCP store so initMCPProvisioner's GetServerByName
// succeeds. ID doesn't need to match anything real — provisioner just
// caches and passes it through.
serverID := uuid.New()
mcpStore.serversByName["bitrix-mcp"] = &store.MCPServerData{
BaseModel: store.BaseModel{ID: serverID},
Name: "bitrix-mcp",
}
fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, "")
cfgJSON := `{"portal":"p","bot_code":"c","bot_name":"n","bot_type":"` + botType +
`","mcp_server_name":"bitrix-mcp","mcp_base_url":"` + mcpBaseURL + `"}`
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())
// Bypass Start() — set the wiring initMCPProvisioner would set.
// Calling Start() would also try to hit Bitrix portal which we don't
// want in these focused tests.
bc.startMu.Lock()
bc.botID = 1
bc.client = NewClient("p.bitrix24.com", nil)
bc.startMu.Unlock()
if err := bc.initMCPProvisioner(context.Background()); err != nil {
t.Fatalf("initMCPProvisioner: %v", err)
}
return bc
}
// validAuth returns an EventAuth with every field provisionIfMissing
// needs populated so individual tests can focus on the outcome, not
// fixture plumbing.
func validAuth() EventAuth {
return EventAuth{
Domain: "acme.bitrix24.com",
AccessToken: "at-tok",
RefreshToken: "rt-tok",
ExpiresIn: 3600,
}
}
// --- Tests ---------------------------------------------------------------
// TestProvisionIfMissing_OpenChannelBot_Skipped verifies bot_type=O short-
// circuits the provisioner without touching the MCP store. This is a
// Phase C skip (not failure) and is the expected outcome for every
// message delivered to an Open Channel bot.
func TestProvisionIfMissing_OpenChannelBot_Skipped(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
mcpStore := newFakeMCPStore()
// Don't seed a server — if provisioner tries to use it we'll catch the
// mistake via the nil-check, but this test is really about the early
// IsOpenChannelBot() exit.
fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, "")
ch, err := fn("b1", nil, json.RawMessage(`{"portal":"p","bot_code":"c","bot_name":"n","bot_type":"O","mcp_server_name":"bitrix-mcp","mcp_base_url":"http://example.test"}`),
bus.New(), nil)
if err != nil {
t.Fatalf("factory: %v", err)
}
bc := ch.(*Channel)
// IS_CONNECTOR=Y (external connector customer) → skipped: not a Bitrix user.
err = bc.provisionIfMissing(context.Background(), "42", true, validAuth(), "chat123")
if !errors.Is(err, ErrProvisionSkippedOpenChannel) {
t.Fatalf("connector message: err = %v; want ErrProvisionSkippedOpenChannel", err)
}
if mcpStore.getUserCallCount != 0 {
t.Errorf("Open Channel connector message must not hit MCP store; got %d GetUserCredentials calls", mcpStore.getUserCallCount)
}
// IS_CONNECTOR=N (internal staff in an Open Channel) → must NOT connector-skip.
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)
}
}
// TestProvisionIfMissing_Disabled verifies a channel built via the
// no-MCP factory variant (or with half-config) reports Disabled without
// touching anything external.
func TestProvisionIfMissing_Disabled(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
// FactoryWithPortalStore (2-arg) leaves mcpStore nil → provisioning
// stays disabled regardless of config fields.
fn := FactoryWithPortalStore(fs, "")
ch, err := fn("b1", nil, json.RawMessage(`{"portal":"p","bot_code":"c","bot_name":"n"}`),
bus.New(), nil)
if err != nil {
t.Fatalf("factory: %v", err)
}
bc := ch.(*Channel)
err = bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123")
if !errors.Is(err, ErrProvisionDisabled) {
t.Fatalf("err = %v; want ErrProvisionDisabled", err)
}
}
// TestProvisionIfMissing_ExistingCreds_NoHTTP ensures the warm path skips
// auto-onboard entirely when the user already has credentials. This is
// the hottest path — all subsequent messages from the same user — so
// verifying it does NOT re-hit the HTTP endpoint protects against a
// regression that'd show up as "burning 1 HTTP request per message".
func TestProvisionIfMissing_ExistingCreds_NoHTTP(t *testing.T) {
httpCalls := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
httpCalls++
w.WriteHeader(http.StatusInternalServerError)
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
// Preload credentials for user 42 before the first provision attempt.
// Warm path requires BITRIX_EXPIRES_AT meta (added 260512 C3 fix). Legacy
// rows without expiry meta now actively refresh on next event to write the
// meta column — see TestProvisionIfMissing_LegacyNoExpiry_RefreshHTTP.
warmExpiry := time.Now().UTC().Add(30 * time.Minute).Format(time.RFC3339)
mcpStore.userCreds[credKey(bc.mcpServerID, "42")] = store.MCPUserCredentials{
APIKey: "prior-key",
Env: map[string]string{
"BITRIX_EXPIRES_AT": warmExpiry,
},
}
if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil {
t.Fatalf("err = %v; want nil", err)
}
if httpCalls != 0 {
t.Errorf("warm path must not call auto-onboard; got %d HTTP calls", httpCalls)
}
if mcpStore.setUserCallCount != 0 {
t.Errorf("warm path must not re-persist; got %d SetUserCredentials calls", mcpStore.setUserCallCount)
}
}
// TestProvisionIfMissing_NearExpiry_RefreshHTTP locks the C3 (Phase 3) fix:
// when cached BITRIX_EXPIRES_AT is within mcpCredsRefreshWindow (5 min) of
// now, the provisioner MUST refresh proactively — preventing the upcoming
// tool call from racing a stale token. Without this, a user chatting just
// before expiry would hit 401 on the tool call and need a follow-up message
// to recover.
func TestProvisionIfMissing_NearExpiry_RefreshHTTP(t *testing.T) {
httpCalls := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
httpCalls++
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"api_key":"refreshed-key","user_id":"u-42","tenant_id":"t-1","created":false}`))
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
// Preload creds with expiry 2 minutes in the future (inside refresh window).
nearExpiry := time.Now().UTC().Add(2 * time.Minute).Format(time.RFC3339)
mcpStore.userCreds[credKey(bc.mcpServerID, "42")] = store.MCPUserCredentials{
APIKey: "stale-key",
Env: map[string]string{
"BITRIX_EXPIRES_AT": nearExpiry,
},
}
if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil {
t.Fatalf("err = %v; want nil", err)
}
if httpCalls != 1 {
t.Errorf("near-expiry path must refresh once; got %d HTTP calls", httpCalls)
}
if mcpStore.setUserCallCount != 1 {
t.Errorf("near-expiry path must persist refreshed creds; got %d SetUserCredentials calls", mcpStore.setUserCallCount)
}
}
// TestProvisionIfMissing_LegacyNoExpiry_RefreshHTTP locks the 260512 fix:
// rows without BITRIX_EXPIRES_AT meta (legacy onboards before C3) MUST be
// refreshed once to write expiry meta. Without this, mcp-bx-syn rejects with
// 401 when its stored token expires (1h TTL after onboard) → loop-side
// purge breaks the in-flight conversation (observed user 1 group chat 2150).
func TestProvisionIfMissing_LegacyNoExpiry_RefreshHTTP(t *testing.T) {
httpCalls := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
httpCalls++
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"api_key":"refreshed-key","user_id":"u-42","tenant_id":"t-1","created":false}`))
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
// Legacy row: APIKey set, NO BITRIX_EXPIRES_AT.
mcpStore.userCreds[credKey(bc.mcpServerID, "42")] = store.MCPUserCredentials{
APIKey: "legacy-key",
}
if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil {
t.Fatalf("err = %v; want nil", err)
}
if httpCalls != 1 {
t.Errorf("legacy path must refresh once; got %d HTTP calls", httpCalls)
}
if mcpStore.setUserCallCount != 1 {
t.Errorf("legacy path must persist refreshed creds; got %d SetUserCredentials", mcpStore.setUserCallCount)
}
}
// TestProvisionIfMissing_WarmExpiry_NoHTTP locks the inverse of C3: when
// cached expiry is comfortably beyond the refresh window (e.g. 30 min away),
// provisioner MUST skip HTTP. Refreshing on every event when token is still
// fresh would burn 1 HTTP per message and DDoS mcp-bx-syn.
func TestProvisionIfMissing_WarmExpiry_NoHTTP(t *testing.T) {
httpCalls := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
httpCalls++
w.WriteHeader(http.StatusInternalServerError)
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
warmExpiry := time.Now().UTC().Add(30 * time.Minute).Format(time.RFC3339)
mcpStore.userCreds[credKey(bc.mcpServerID, "42")] = store.MCPUserCredentials{
APIKey: "warm-key",
Env: map[string]string{
"BITRIX_EXPIRES_AT": warmExpiry,
},
}
if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil {
t.Fatalf("err = %v; want nil", err)
}
if httpCalls != 0 {
t.Errorf("warm-expiry path must not call HTTP; got %d HTTP calls", httpCalls)
}
}
// TestProvisionIfMissing_MintAndPersist covers the full happy path: no
// existing creds → auto-onboard call → credential saved with OAuth tokens
// in Env. This is the core value-add of Phase C.
func TestProvisionIfMissing_MintAndPersist(t *testing.T) {
var gotReq autoOnboardRequest
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_ = json.NewDecoder(r.Body).Decode(&gotReq)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"api_key":"minted-k","user_id":"mu","tenant_id":"mt","created":true}`))
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
before := time.Now()
err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123")
if err != nil {
t.Fatalf("provisionIfMissing: %v", err)
}
// Request body reached the MCP server verbatim.
if gotReq.Domain != "acme.bitrix24.com" || gotReq.BitrixUserID != "42" ||
gotReq.AccessToken != "at-tok" || gotReq.RefreshToken != "rt-tok" {
t.Errorf("unexpected request to MCP server: %+v", gotReq)
}
// Credential was persisted with API key + OAuth Env.
stored, ok := mcpStore.userCreds[credKey(bc.mcpServerID, "42")]
if !ok {
t.Fatalf("credentials were not persisted")
}
if stored.APIKey != "minted-k" {
t.Errorf("APIKey = %q; want minted-k", stored.APIKey)
}
for _, key := range []string{"BITRIX_DOMAIN", "BITRIX_ACCESS_TOKEN", "BITRIX_REFRESH_TOKEN", "BITRIX_EXPIRES_AT"} {
if _, has := stored.Env[key]; !has {
t.Errorf("Env missing %q (full Env: %v)", key, stored.Env)
}
}
if stored.Env["BITRIX_DOMAIN"] != "acme.bitrix24.com" {
t.Errorf("Env[BITRIX_DOMAIN] = %q; want acme.bitrix24.com", stored.Env["BITRIX_DOMAIN"])
}
// Sanity-check EXPIRES_AT is ~now + expires_in, not some fossil.
parsed, err := time.Parse(time.RFC3339, stored.Env["BITRIX_EXPIRES_AT"])
if err != nil {
t.Fatalf("BITRIX_EXPIRES_AT not RFC3339: %v", err)
}
expectedMin := before.Add(3599 * time.Second)
expectedMax := before.Add(3601 * time.Second)
if parsed.Before(expectedMin) || parsed.After(expectedMax) {
t.Errorf("BITRIX_EXPIRES_AT out of expected window (%v–%v): got %v",
expectedMin, expectedMax, parsed)
}
}
// TestProvisionIfMissing_Debounce verifies that after one attempt (success
// OR failure), a second attempt within mcpProvisionDebounceTTL returns
// ErrProvisionDebounced without calling the MCP server again. Critical
// guard against Bitrix24 webhook retry storms.
func TestProvisionIfMissing_Debounce(t *testing.T) {
httpCalls := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
httpCalls++
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"api_key":"k","user_id":"u","tenant_id":"t","created":true}`))
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
// First attempt succeeds and marks the debounce.
if err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123"); err != nil {
t.Fatalf("first attempt: %v", err)
}
if httpCalls != 1 {
t.Fatalf("first attempt should call HTTP once, got %d", httpCalls)
}
// Second attempt within TTL → debounced. We need to wipe the stored
// credential first; otherwise the "existing creds" short-circuit
// returns nil before the debounce check fires. (This ordering is
// documented in provisionIfMissing; the test makes it observable.)
mcpStore.mu.Lock()
delete(mcpStore.userCreds, credKey(bc.mcpServerID, "42"))
mcpStore.mu.Unlock()
err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123")
if !errors.Is(err, ErrProvisionDebounced) {
t.Fatalf("second attempt: %v; want ErrProvisionDebounced", err)
}
if httpCalls != 1 {
t.Errorf("debounced attempt must not hit HTTP; got %d total calls", httpCalls)
}
}
// TestProvisionIfMissing_HTTPFailure_Surfaces ensures auto-onboard errors
// propagate out of provisionIfMissing (wrapped, but distinguishable). The
// caller in handle.go swallows these after logging, but tests need to see
// them to assert the error path exists.
func TestProvisionIfMissing_HTTPFailure_Surfaces(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusUnauthorized)
_, _ = w.Write([]byte(`{"error":"invalid_bitrix_user"}`))
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
err := bc.provisionIfMissing(context.Background(), "42", false, validAuth(), "chat123")
if err == nil {
t.Fatal("401 from MCP must produce an error")
}
// We don't assert exact string, but do check the caller can tell this
// is NOT one of the expected no-op sentinels — otherwise handle.go
// would misclassify it as "skipped" and hide the 401 in Debug logs.
if errors.Is(err, ErrProvisionDisabled) ||
errors.Is(err, ErrProvisionDebounced) ||
errors.Is(err, ErrProvisionSkippedOpenChannel) {
t.Errorf("HTTP 401 should not match any no-op sentinel; got %v", err)
}
// Underlying message should mention the status so operators can debug.
if !strings.Contains(err.Error(), "auto-onboard failed") {
t.Errorf("err = %v; expected wrapped 'auto-onboard failed' prefix", err)
}
}
// TestProvisionIfMissing_MissingAuthBlock catches the case where the
// event somehow reached handleMessage with partial auth data. Should
// fail BEFORE calling the MCP server (saves an HTTP round-trip that
// we know will fail validation client-side).
func TestProvisionIfMissing_MissingAuthBlock(t *testing.T) {
httpCalls := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
httpCalls++
}))
defer srv.Close()
mcpStore := newFakeMCPStore()
bc := newProvisionerTestChannel(t, mcpStore, srv.URL, "B")
cases := []struct {
name string
auth EventAuth
}{
{"empty_domain", EventAuth{AccessToken: "a", RefreshToken: "r"}},
{"empty_access_token", EventAuth{Domain: "d", RefreshToken: "r"}},
{"empty_refresh_token", EventAuth{Domain: "d", AccessToken: "a"}},
}
for _, tc := range cases {
t.Run(tc.name, func(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, "chat123")
if err == nil {
t.Fatalf("missing %s should fail", tc.name)
}
if httpCalls != before {
t.Errorf("incomplete auth must not hit HTTP; got +%d calls", httpCalls-before)
}
})
}
}
// 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
// - mcp_server_name points at a server that doesn't exist in the store
//
// (Half-config — only one of mcp_server_name / mcp_base_url set —
// is rejected earlier at factory load; see TestFactory_HalfConfigRejected.
// Both-empty is accepted with provisioning off, covered by
// TestProvisionIfMissing_Disabled.)
//
// Each case should leave mcpClient nil + mcpServerID zero, so
// provisionIfMissing returns ErrProvisionDisabled.
//
// Auto-onboard auth note: there is no longer an admin-token branch to test —
// the MCP server authenticates each /api/auto-onboard call via the
// caller-supplied Bitrix access_token, not a shared bearer.
func TestInitMCPProvisioner_DisabledModes(t *testing.T) {
t.Run("nil_mcp_store", func(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
fn := FactoryWithPortalStoreAndMCP(fs, nil, "")
ch, _ := fn("b1", nil, json.RawMessage(`{"portal":"p","bot_code":"c","bot_name":"n","mcp_server_name":"x","mcp_base_url":"http://x"}`),
bus.New(), nil)
bc := ch.(*Channel)
if err := bc.initMCPProvisioner(context.Background()); err != nil {
t.Fatalf("init: %v", err)
}
if bc.mcpClient != nil || bc.mcpServerID != uuid.Nil {
t.Errorf("nil mcpStore should leave provisioner off")
}
})
t.Run("server_not_found", func(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
mcpStore := newFakeMCPStore()
// Intentionally do NOT seed serversByName — GetServerByName returns nil.
fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, "")
ch, _ := fn("b1", nil, json.RawMessage(`{"portal":"p","bot_code":"c","bot_name":"n","mcp_server_name":"missing","mcp_base_url":"http://x"}`),
bus.New(), nil)
bc := ch.(*Channel)
if err := bc.initMCPProvisioner(context.Background()); err != nil {
t.Fatalf("init: %v", err)
}
if bc.mcpClient != nil || bc.mcpServerID != uuid.Nil {
t.Errorf("missing server row should leave provisioner off")
}
})
// mcp_server_id path (v3.15+ dashboard wiring): the UUID string must
// parse and resolve to a live mcp_servers row. Broken UUID or missing
// row silently disables provisioning — no fallback to legacy name.
t.Run("mcp_server_id_invalid_uuid", func(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
mcpStore := newFakeMCPStore()
fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, "")
ch, _ := fn("b1", nil, json.RawMessage(`{"portal":"p","bot_code":"c","bot_name":"n","mcp_server_id":"not-a-uuid"}`),
bus.New(), nil)
bc := ch.(*Channel)
if err := bc.initMCPProvisioner(context.Background()); err != nil {
t.Fatalf("init: %v", err)
}
if bc.mcpClient != nil || bc.mcpServerID != uuid.Nil {
t.Errorf("invalid mcp_server_id UUID should leave provisioner off")
}
})
t.Run("mcp_server_id_row_not_found", func(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
mcpStore := newFakeMCPStore()
// Intentionally do NOT seed serversByID — GetServer returns nil.
fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, "")
ch, _ := fn("b1", nil, json.RawMessage(`{"portal":"p","bot_code":"c","bot_name":"n","mcp_server_id":"00000000-0000-0000-0000-000000000042"}`),
bus.New(), nil)
bc := ch.(*Channel)
if err := bc.initMCPProvisioner(context.Background()); err != nil {
t.Fatalf("init: %v", err)
}
if bc.mcpClient != nil || bc.mcpServerID != uuid.Nil {
t.Errorf("missing mcp_servers row for mcp_server_id should leave provisioner off")
}
})
}
// TestDeriveAutoOnboardBaseURL guards the fix for the Phase 2 regression
// where mcp_servers.url (JSON-RPC endpoint under /mcp) was passed verbatim
// to mcp_client.newMCPClient — which appends "/api/auto-onboard" to it,
// producing "…/mcp/api/auto-onboard" (404). The origin extraction is the
// contract the Bitrix24 auto-onboard client expects.
func TestDeriveAutoOnboardBaseURL(t *testing.T) {
cases := []struct {
name string
in string
want string
}{
{"jsonrpc subpath", "https://mcp.example.com/mcp", "https://mcp.example.com"},
{"jsonrpc trailing slash", "https://mcp.example.com/mcp/", "https://mcp.example.com"},
{"origin trailing slash", "https://mcp.example.com/", "https://mcp.example.com"},
{"origin only", "https://mcp.example.com", "https://mcp.example.com"},
{"deep path with query", "http://localhost:8080/some/path?x=1#frag", "http://localhost:8080"},
{"prod b24 syn mcp", "https://b24-mcp-dev.synity.so/mcp", "https://b24-mcp-dev.synity.so"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got, err := deriveAutoOnboardBaseURL(tc.in)
if err != nil {
t.Fatalf("deriveAutoOnboardBaseURL(%q) unexpected error: %v", tc.in, err)
}
if got != tc.want {
t.Errorf("deriveAutoOnboardBaseURL(%q) = %q, want %q", tc.in, got, tc.want)
}
})
}
errCases := []struct {
name string
in string
}{
{"empty", ""},
{"whitespace only", " \t"},
{"no scheme", "mcp.example.com/mcp"},
{"no host", "https:///path"},
}
for _, tc := range errCases {
t.Run("err_"+tc.name, func(t *testing.T) {
if _, err := deriveAutoOnboardBaseURL(tc.in); err == nil {
t.Errorf("deriveAutoOnboardBaseURL(%q) expected error, got nil", tc.in)
}
})
}
}
// TestInitMCPProvisioner_MCPServerID exercises the v3.15+ id-based wiring:
// mcp_server_id resolves to a live mcp_servers row whose URL becomes the
// base URL for /api/auto-onboard. Legacy MCPServerName + MCPBaseURL are
// ignored on this path (single source of truth).
func TestInitMCPProvisioner_MCPServerID(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
mcpStore := newFakeMCPStore()
serverID := uuid.MustParse("019df803-ec13-76c3-b1f5-60b0e80d3eec")
// URL carries a /mcp subpath because that's how partner MCP servers
// really configure the JSON-RPC endpoint. initMCPProvisioner must
// strip it before handing the value to mcp_client (which appends
// /api/auto-onboard). Regression guard for the Phase 2 fix.
mcpStore.serversByID[serverID] = &store.MCPServerData{
BaseModel: store.BaseModel{ID: serverID},
Name: "b24-syn-mcp",
URL: "https://b24-mcp.example.test/mcp",
Enabled: true,
// Field promoted from settings JSONB in Phase 89 — the provisioner
// reads it to log accurately and downstream provisionIfMissing
// respects it via manager-level checks.
RequireUserCredentials: true,
}
fn := FactoryWithPortalStoreAndMCP(fs, mcpStore, "")
cfg := `{"portal":"p","bot_code":"c","bot_name":"n","mcp_server_id":"019df803-ec13-76c3-b1f5-60b0e80d3eec"}`
ch, err := fn("b1", nil, json.RawMessage(cfg), bus.New(), nil)
if err != nil {
t.Fatalf("factory: %v", err)
}
bc := ch.(*Channel)
if err := bc.initMCPProvisioner(context.Background()); err != nil {
t.Fatalf("init: %v", err)
}
if bc.mcpClient == nil {
t.Fatalf("mcp_server_id path should wire mcpClient — got nil")
}
if bc.mcpServerID != serverID {
t.Errorf("mcpServerID = %s, want %s", bc.mcpServerID, serverID)
}
}
// newBareChannelForNotifyTest builds a Channel that's wired enough for
// notifyUserOfMCPIssueOnce tests but skips MCP provisioner setup — the
// function under test only touches c.notifyMu / c.notifyDebounce and then
// delegates to sendChunk, which is out of scope here (send.go owns it).
//
// sendChunk will fail immediately with "portal not bound" because the test
// Client has no Portal attached — notifyUserOfMCPIssueOnce swallows that
// error via slog.Debug, so the debounce state is still the primary
// observable. Tests that care about the wire-level Send behavior should
// use the full newProvisionerTestChannel + portal helper instead.
func newBareChannelForNotifyTest(t *testing.T) *Channel {
t.Helper()
fs := newFakeStore()
resetWebhookRouterForTest()
t.Cleanup(resetWebhookRouterForTest)
fn := FactoryWithPortalStore(fs, "")
ch, err := fn("b1", nil,
json.RawMessage(`{"portal":"p","bot_code":"c","bot_name":"n"}`),
bus.New(), nil)
if err != nil {
t.Fatalf("factory: %v", err)
}
bc := ch.(*Channel)
bc.SetTenantID(store.GenNewID())
bc.startMu.Lock()
bc.botID = 1
bc.client = NewClient("p.bitrix24.com", nil)
bc.startMu.Unlock()
return bc
}
// TestNotifyUserOfMCPIssueOnce_FirstCallMarksDebounce verifies the first
// notification for a user stamps the debounce map, regardless of whether
// the downstream sendChunk actually delivered the message. The debounce
// stamp is what prevents a webhook retry burst from flooding the user —
// if we only stamped on successful Send, a Bitrix-portal outage would
// simultaneously block delivery AND disable the rate limit, giving the
// user a queue of identical notices once the portal recovers.
func TestNotifyUserOfMCPIssueOnce_FirstCallMarksDebounce(t *testing.T) {
bc := newBareChannelForNotifyTest(t)
before := time.Now()
bc.notifyUserOfMCPIssueOnce(context.Background(), "user-42", "chat-9")
after := time.Now()
bc.notifyMu.Lock()
defer bc.notifyMu.Unlock()
ts, ok := bc.notifyDebounce["user-42"]
if !ok {
t.Fatalf("first notify did not stamp debounce map for user-42")
}
if ts.Before(before) || ts.After(after) {
t.Errorf("debounce timestamp %v out of window [%v, %v]", ts, before, after)
}
}
// TestNotifyUserOfMCPIssueOnce_SecondCallWithinTTLIsDebounced verifies
// that a second call within mcpUserNotifyDebounceTTL does NOT refresh
// the stamp. Two invariants matter:
// 1. Rate limit holds — user gets exactly one notice per TTL window.
// 2. Timestamp stays pinned to the FIRST call, so the window rolls
// forward from there (not from every subsequent silenced call).
// Otherwise a sustained outage + steady webhook retry traffic
// would keep bumping the stamp forward and the user would never
// see a refreshed notice when the TTL legitimately expired.
func TestNotifyUserOfMCPIssueOnce_SecondCallWithinTTLIsDebounced(t *testing.T) {
bc := newBareChannelForNotifyTest(t)
bc.notifyUserOfMCPIssueOnce(context.Background(), "user-42", "chat-9")
bc.notifyMu.Lock()
firstStamp := bc.notifyDebounce["user-42"]
bc.notifyMu.Unlock()
// Small sleep so a naive "refresh stamp every call" bug would produce
// a strictly later timestamp than firstStamp. 10ms is enough resolution
// on every supported platform.
time.Sleep(10 * time.Millisecond)
bc.notifyUserOfMCPIssueOnce(context.Background(), "user-42", "chat-9")
bc.notifyMu.Lock()
defer bc.notifyMu.Unlock()
secondStamp := bc.notifyDebounce["user-42"]
if !secondStamp.Equal(firstStamp) {
t.Errorf("debounced call should not refresh stamp: first=%v second=%v", firstStamp, secondStamp)
}
}
// TestNotifyUserOfMCPIssueOnce_ExpiredDebounceAllowsNewNotice verifies
// the stamp gets refreshed once the TTL elapses. We manipulate the
// debounce map directly (planting a stale timestamp) to avoid a real
// 5-minute test runtime — this is the standard pattern for testing
// TTL-based caches without the wall-clock penalty.
func TestNotifyUserOfMCPIssueOnce_ExpiredDebounceAllowsNewNotice(t *testing.T) {
bc := newBareChannelForNotifyTest(t)
// Plant a stamp that's safely outside the TTL window.
stale := time.Now().Add(-(mcpUserNotifyDebounceTTL + time.Minute))
bc.notifyMu.Lock()
bc.notifyDebounce = map[string]time.Time{"user-42": stale}
bc.notifyMu.Unlock()
bc.notifyUserOfMCPIssueOnce(context.Background(), "user-42", "chat-9")
bc.notifyMu.Lock()
defer bc.notifyMu.Unlock()
got := bc.notifyDebounce["user-42"]
if !got.After(stale) {
t.Errorf("expired stamp should have been refreshed: stale=%v got=%v", stale, got)
}
if time.Since(got) > time.Second {
t.Errorf("refreshed stamp should be ~now, got age=%v", time.Since(got))
}
}
// TestNotifyUserOfMCPIssueOnce_EmptyInputsAreNoop guards the two
// defensive branches at the top of notifyUserOfMCPIssueOnce: empty
// chatID (no reply target) and empty userID (shouldn't reach here
// from handle.go but cheap to defend). Either one must short-circuit
// BEFORE the debounce map is touched — otherwise a webhook with a
// blank FromUserID could silently poison the "" key and prevent
// legitimate future notices from firing.
func TestNotifyUserOfMCPIssueOnce_EmptyInputsAreNoop(t *testing.T) {
cases := []struct {
name string
userID string
chatID string
}{
{"empty_chat_id", "user-42", ""},
{"whitespace_chat_id", "user-42", " "},
{"empty_user_id", "", "chat-9"},
{"whitespace_user_id", "\t", "chat-9"},
{"both_empty", "", ""},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
bc := newBareChannelForNotifyTest(t)
bc.notifyUserOfMCPIssueOnce(context.Background(), tc.userID, tc.chatID)
bc.notifyMu.Lock()
defer bc.notifyMu.Unlock()
if len(bc.notifyDebounce) != 0 {
t.Errorf("no-op case must leave debounce map empty, got %v", bc.notifyDebounce)
}
})
}
}
// TestNotifyUserOfMCPIssueOnce_DifferentUsersIndependent verifies per-
// user debounce isolation: user A hitting the rate limit must not
// silence notices for user B. This matters when a single MCP outage
// affects many users — each should be independently informed on their
// next message.
func TestNotifyUserOfMCPIssueOnce_DifferentUsersIndependent(t *testing.T) {
bc := newBareChannelForNotifyTest(t)
bc.notifyUserOfMCPIssueOnce(context.Background(), "alice", "chat-1")
bc.notifyUserOfMCPIssueOnce(context.Background(), "bob", "chat-2")
bc.notifyMu.Lock()
defer bc.notifyMu.Unlock()
if _, ok := bc.notifyDebounce["alice"]; !ok {
t.Errorf("alice missing from debounce map: %v", bc.notifyDebounce)
}
if _, ok := bc.notifyDebounce["bob"]; !ok {
t.Errorf("bob missing from debounce map: %v", bc.notifyDebounce)
}
}
// TestFactory_HalfConfigRejected codifies the "both or neither" rule for
// the mcp_server_name + mcp_base_url pair so admin typos fail fast at
// load rather than manifesting as a silently-disabled provisioner.
func TestFactory_HalfConfigRejected(t *testing.T) {
fs := newFakeStore()
resetWebhookRouterForTest()
defer resetWebhookRouterForTest()
fn := FactoryWithPortalStoreAndMCP(fs, newFakeMCPStore(), "")
cases := []struct {
name string
cfg string
}{
{"only_server_name", `{"portal":"p","bot_code":"c","bot_name":"n","mcp_server_name":"x"}`},
{"only_base_url", `{"portal":"p","bot_code":"c","bot_name":"n","mcp_base_url":"http://x"}`},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
_, err := fn("b1", nil, json.RawMessage(tc.cfg), bus.New(), nil)
if err == nil {
t.Fatal("half-config must fail at factory load")
}
})
}
}