mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-15 10:14:51 +00:00
in groups, resolve group credentials first, then sender credentials (#1482)
This commit is contained in:
1 parent
31f973477d
commit
e71d6a5519
6 files changed
+172
-28
No files matched your search
@@ -162,8 +162,13 @@ func (l *Loop) buildMessages(ctx context.Context, history []providers.Message, s
|
||||
// tools. Otherwise lookupMCPDescFromUserTools surfaces descriptions from
|
||||
// any user's cache → LLM sees tools it can't actually call (executeToolForActor
|
||||
// scoped to actorUserID returns "tool not found"). Compute actor via
|
||||
// resolveActorUserID — same key the agent loop uses to fetch per-user MCP creds.
|
||||
actorUserID := resolveActorUserID(userID, store.SenderIDFromContext(ctx), peerKind, channelType)
|
||||
// CredentialUserID (merged tenant_user identity) to match the cache key
|
||||
// used by getUserMCPTools. Fall back to resolveActorUserID for channels
|
||||
// without merge resolution.
|
||||
actorUserID := store.CredentialUserIDFromContext(ctx)
|
||||
if actorUserID == "" {
|
||||
actorUserID = resolveActorUserID(userID, store.SenderIDFromContext(ctx), peerKind, channelType)
|
||||
}
|
||||
mcpToolDescs := l.buildMCPToolDescs(toolNames, actorUserID)
|
||||
|
||||
// Bootstrap DM mode: only restrict tools for open agents (identity being created).
|
||||
|
||||
@@ -475,3 +475,127 @@ func TestGetUserMCPToolsNilPoolEarlyReturn(t *testing.T) {
|
||||
type errTokenExpiredForTest string
|
||||
|
||||
func (e errTokenExpiredForTest) Error() string { return string(e) }
|
||||
|
||||
// TestGetUserMCPToolsGroupCredentialResolution verifies that when a Telegram user
|
||||
// posts in a group, the group's credentials (CredentialUserID) are used for MCP
|
||||
// tool lookup instead of the individual sender's credentials.
|
||||
//
|
||||
// This is the fix for the calendar event creation bug: the user's calendar email
|
||||
// was known but the MCP tools were being looked up with the wrong user ID,
|
||||
// causing the tools to appear blocked by system policy.
|
||||
func TestGetUserMCPToolsGroupCredentialResolution(t *testing.T) {
|
||||
// Simulate a group chat where the group has its own MCP credentials
|
||||
// (e.g., a Google Calendar MCP server configured for the group).
|
||||
groupID := "group:telegram:-100456"
|
||||
serverID := uuid.New()
|
||||
|
||||
// Track which user ID is used for credential lookup
|
||||
var lookupUserID string
|
||||
st := &trackingMCPServerStore{
|
||||
userCreds: &store.MCPUserCredentials{
|
||||
APIKey: "group-calendar-api-key",
|
||||
Headers: map[string]string{"Authorization": "Bearer group-calendar-api-key"},
|
||||
},
|
||||
onGetUserCredentials: func(_ context.Context, _ uuid.UUID, userID string) {
|
||||
lookupUserID = userID
|
||||
},
|
||||
}
|
||||
|
||||
pool := newTestPool(t)
|
||||
l := &Loop{
|
||||
tenantID: uuid.New(),
|
||||
mcpPool: pool,
|
||||
mcpStore: st,
|
||||
mcpUserCredSrvs: []store.MCPAccessInfo{
|
||||
{Server: store.MCPServerData{
|
||||
BaseModel: store.BaseModel{ID: serverID},
|
||||
Name: "google-calendar",
|
||||
Transport: "http",
|
||||
URL: "http://127.0.0.1:1",
|
||||
}},
|
||||
},
|
||||
}
|
||||
|
||||
// When CredentialUserID is set in context (resolved group identity),
|
||||
// it should be used for MCP credential lookup.
|
||||
ctx := store.WithCredentialUserID(context.Background(), groupID)
|
||||
_ = l.getUserMCPTools(ctx, groupID)
|
||||
|
||||
if lookupUserID != groupID {
|
||||
t.Errorf("expected MCP credentials looked up with group ID %q, got %q", groupID, lookupUserID)
|
||||
}
|
||||
}
|
||||
|
||||
// trackingMCPServerStore is a test store that tracks GetUserCredentials calls.
|
||||
type trackingMCPServerStore struct {
|
||||
userCreds *store.MCPUserCredentials
|
||||
onGetUserCredentials func(_ context.Context, _ uuid.UUID, userID string)
|
||||
}
|
||||
|
||||
func (m *trackingMCPServerStore) GetUserCredentials(_ context.Context, _ uuid.UUID, userID string) (*store.MCPUserCredentials, error) {
|
||||
if m.onGetUserCredentials != nil {
|
||||
m.onGetUserCredentials(context.Background(), uuid.Nil, userID)
|
||||
}
|
||||
return m.userCreds, nil
|
||||
}
|
||||
|
||||
func (m *trackingMCPServerStore) CreateServer(_ context.Context, _ *store.MCPServerData) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) GetServer(_ context.Context, _ uuid.UUID) (*store.MCPServerData, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) GetServerByName(_ context.Context, _ string) (*store.MCPServerData, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) ListServers(_ context.Context) ([]store.MCPServerData, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) UpdateServer(_ context.Context, _ uuid.UUID, _ map[string]any) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) DeleteServer(_ context.Context, _ uuid.UUID) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) GrantToAgent(_ context.Context, _ *store.MCPAgentGrant) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) RevokeFromAgent(_ context.Context, _, _ uuid.UUID) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) ListAgentGrants(_ context.Context, _ uuid.UUID) ([]store.MCPAgentGrant, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) ListServerGrants(_ context.Context, _ uuid.UUID) ([]store.MCPAgentGrant, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) GrantToUser(_ context.Context, _ *store.MCPUserGrant) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) RevokeFromUser(_ context.Context, _ uuid.UUID, _ string) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) CountAgentGrantsByServer(_ context.Context) (map[uuid.UUID]int, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) ListAccessible(_ context.Context, _ uuid.UUID, _ string) ([]store.MCPAccessInfo, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) CreateRequest(_ context.Context, _ *store.MCPAccessRequest) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) ListPendingRequests(_ context.Context) ([]store.MCPAccessRequest, error) {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) ReviewRequest(_ context.Context, _ uuid.UUID, _ bool, _, _ string) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) SetUserCredentials(_ context.Context, _ uuid.UUID, _ string, _ store.MCPUserCredentials) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) DeleteUserCredentials(_ context.Context, _ uuid.UUID, _ string) error {
|
||||
panic("not implemented")
|
||||
}
|
||||
func (m *trackingMCPServerStore) CacheToolDescriptions(_ context.Context, _ uuid.UUID, _ map[string]store.CachedToolInfo) error {
|
||||
return nil
|
||||
}
|
||||
@@ -231,17 +231,22 @@ func (l *Loop) makeBuildFilteredTools(req *RunRequest) func(state *pipeline.RunS
|
||||
// Servers with require_user_credentials are deferred at startup and
|
||||
// connected per-request here with the actual user's credentials.
|
||||
//
|
||||
// Use resolveActorUserID — the gateway consumer rewrites UserID in
|
||||
// two scenarios (group chats AND DM with merged contact), both of
|
||||
// which break per-user MCP credential lookup. ChannelType discriminates
|
||||
// Bitrix24 (always prefer SenderID) from other channels (group-only
|
||||
// rewrite recovery). See resolveActorUserID docstring for full rationale.
|
||||
actorUserID := resolveActorUserID(
|
||||
state.Input.UserID,
|
||||
state.Input.SenderID,
|
||||
state.Input.PeerKind,
|
||||
state.Input.ChannelType,
|
||||
)
|
||||
// Prefer CredentialUserID from context — resolveCredentialUserID already
|
||||
// resolved the merged tenant_user identity (e.g. Telegram group merged
|
||||
// to a tenant_user via UI). Using the raw SenderID or group composite
|
||||
// as the cache key would miss the credential row keyed by the merged
|
||||
// tenant_user UUID. Fall back to resolveActorUserID for channels without
|
||||
// merge resolution (backward compat). See resolveActorUserID docstring
|
||||
// for the group-rewrite recovery rationale.
|
||||
actorUserID := store.CredentialUserIDFromContext(state.Ctx)
|
||||
if actorUserID == "" {
|
||||
actorUserID = resolveActorUserID(
|
||||
state.Input.UserID,
|
||||
state.Input.SenderID,
|
||||
state.Input.PeerKind,
|
||||
state.Input.ChannelType,
|
||||
)
|
||||
}
|
||||
userTools := l.getUserMCPTools(state.Ctx, actorUserID)
|
||||
slog.Debug("mcp.user_tools_context",
|
||||
"peer_kind", state.Input.PeerKind,
|
||||
|
||||
@@ -75,7 +75,12 @@ func (l *Loop) makeExecuteToolCall(req *RunRequest, bridgeRS *runState) func(ctx
|
||||
// C2 fix: route through executeToolForActor so per-user MCP tools
|
||||
// resolve to the calling user's BridgeTool (not the first user's
|
||||
// BridgeTool leaked via shared registry).
|
||||
actorUserID := resolveActorUserID(req.UserID, req.SenderID, req.PeerKind, req.ChannelType)
|
||||
// Prefer CredentialUserID (merged tenant_user identity) over raw
|
||||
// resolveActorUserID so the cache key matches what getUserMCPTools used.
|
||||
actorUserID := store.CredentialUserIDFromContext(ctx)
|
||||
if actorUserID == "" {
|
||||
actorUserID = resolveActorUserID(req.UserID, req.SenderID, req.PeerKind, req.ChannelType)
|
||||
}
|
||||
result := l.executeToolForActor(ctx, registryName, tc.Arguments,
|
||||
req.Channel, req.ChatID, req.PeerKind, req.SessionKey, actorUserID)
|
||||
toolDuration := time.Since(toolStart)
|
||||
@@ -146,7 +151,12 @@ func (l *Loop) makeExecuteToolRaw(req *RunRequest) func(ctx context.Context, tc
|
||||
|
||||
// C2 fix (parallel path): route through executeToolForActor for per-user
|
||||
// MCP tool isolation. Same rationale as makeExecuteToolCall above.
|
||||
actorUserID := resolveActorUserID(req.UserID, req.SenderID, req.PeerKind, req.ChannelType)
|
||||
// Prefer CredentialUserID (merged tenant_user identity) over raw
|
||||
// resolveActorUserID so the cache key matches what getUserMCPTools used.
|
||||
actorUserID := store.CredentialUserIDFromContext(ctx)
|
||||
if actorUserID == "" {
|
||||
actorUserID = resolveActorUserID(req.UserID, req.SenderID, req.PeerKind, req.ChannelType)
|
||||
}
|
||||
result := l.executeToolForActor(ctx, registryName, tc.Arguments,
|
||||
req.Channel, req.ChatID, req.PeerKind, req.SessionKey, actorUserID)
|
||||
dur := time.Since(start)
|
||||
|
||||
@@ -60,7 +60,17 @@ func (l *Loop) resolveCredentialUserID(ctx context.Context, req RunRequest) stri
|
||||
return req.UserID
|
||||
}
|
||||
|
||||
// Group: try individual sender first (most specific)
|
||||
// Group: try group contact first (group has its own credentials),
|
||||
// fall back to individual sender if no group credential exists.
|
||||
if chatID := extractGroupChatID(req.UserID); chatID != "" && channelType != "" {
|
||||
resolved, err := l.userResolver.ResolveTenantUserID(ctx, channelType, chatID)
|
||||
if err != nil {
|
||||
slog.Debug("credential_resolve.group_contact_failed", "chatID", chatID, "channel", channelType, "error", err)
|
||||
} else if resolved != "" {
|
||||
return resolved
|
||||
}
|
||||
}
|
||||
|
||||
if req.SenderID != "" && channelType != "" {
|
||||
senderNumeric := req.SenderID
|
||||
if idx := strings.IndexByte(senderNumeric, '|'); idx > 0 {
|
||||
@@ -74,16 +84,6 @@ func (l *Loop) resolveCredentialUserID(ctx context.Context, req RunRequest) stri
|
||||
}
|
||||
}
|
||||
|
||||
// Group: try group contact resolution (group chatID merged to tenant user)
|
||||
if chatID := extractGroupChatID(req.UserID); chatID != "" && channelType != "" {
|
||||
resolved, err := l.userResolver.ResolveTenantUserID(ctx, channelType, chatID)
|
||||
if err != nil {
|
||||
slog.Debug("credential_resolve.group_contact_failed", "chatID", chatID, "channel", channelType, "error", err)
|
||||
} else if resolved != "" {
|
||||
return resolved
|
||||
}
|
||||
}
|
||||
|
||||
return req.UserID
|
||||
}
|
||||
|
||||
|
||||
@@ -62,13 +62,13 @@ func TestResolveCredentialUserID(t *testing.T) {
|
||||
wantUserID: "team-acme@co.com",
|
||||
},
|
||||
{
|
||||
name: "Group: both sender and group merged — sender takes priority",
|
||||
name: "Group: both sender and group merged — group takes priority",
|
||||
req: RunRequest{UserID: "group:tg:-100456", SenderID: "12345", ChannelType: "telegram", PeerKind: "group"},
|
||||
mergedMap: map[string]string{
|
||||
"telegram:12345": "john@co.com",
|
||||
"telegram:-100456": "team-acme@co.com",
|
||||
},
|
||||
wantUserID: "john@co.com", // individual > group
|
||||
wantUserID: "team-acme@co.com", // group > individual
|
||||
},
|
||||
{
|
||||
name: "Group: nobody merged — returns group composite",
|
||||
|
||||
Reference in new issue
Block a user