Skip to content

Commit 4fa06cd

Browse files
idoubiclaude
andcommitted
refactor(auth): drop first-DM claim — shared-identity channels already carry operator identity
The first-DM heuristic (f43ffa4) was solving a problem the platform had already solved: a channel bound with sharedIdentity resolves every message's UserID to the binding web user (routing.processInbound), so the operator's own IM channel passes isAdminChatter's owner-equality check with zero configuration. The toggle in the agent config page IS the 'this channel is personally mine' declaration; public-facing channels leave it off and their minted app_user chatters stay guests. Also closes the group hole that surfaced while verifying this: on a shared-identity channel EVERY group speaker gets rewritten to the owner id before group routing, and the platform-side sender id is gone by the time the agent sees the message — so groups on shared-identity channels can never vouch for a speaker and are denied admin outright. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 17aa695 commit 4fa06cd

5 files changed

Lines changed: 63 additions & 220 deletions

File tree

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package agent
2+
3+
import (
4+
"testing"
5+
6+
"github.com/fastclaw-ai/fastclaw/internal/bus"
7+
)
8+
9+
// Shared-identity channels resolve the chatter to the channel owner's
10+
// web user id, so an owner-bound personal channel is admin in DMs with
11+
// zero extra configuration. Groups on such channels must NOT inherit
12+
// that: routing rewrites every group speaker to the owner id, so owner
13+
// equality proves nothing there.
14+
func TestAdminChatterSharedIdentity(t *testing.T) {
15+
a := &Agent{ownerUserID: "u_owner"}
16+
17+
dm := bus.InboundMessage{
18+
Channel: "telegram",
19+
UserID: "u_owner",
20+
PeerKind: "dm",
21+
SharedIdentity: true,
22+
}
23+
if !a.isAdminChatter(dm) {
24+
t.Fatal("shared-identity DM resolved to owner should be admin")
25+
}
26+
27+
group := dm
28+
group.PeerKind = "group"
29+
if a.isAdminChatter(group) {
30+
t.Fatal("group speaker on shared-identity channel must not be admin")
31+
}
32+
}
33+
34+
// Regular (non-shared) IM chatters are minted as app_users — never equal
35+
// to the owner id — so they stay guests unless the admins allowlist
36+
// names their platform id.
37+
func TestAdminChatterRegularIMChannel(t *testing.T) {
38+
a := &Agent{
39+
ownerUserID: "u_owner",
40+
admins: map[string][]string{"telegram": {"u_app_listed"}},
41+
}
42+
43+
stranger := bus.InboundMessage{Channel: "telegram", UserID: "u_app_stranger", PeerKind: "dm"}
44+
if a.isAdminChatter(stranger) {
45+
t.Fatal("minted app_user chatter must not be admin")
46+
}
47+
48+
listed := bus.InboundMessage{Channel: "telegram", UserID: "u_app_listed", PeerKind: "dm"}
49+
if !a.isAdminChatter(listed) {
50+
t.Fatal("allowlisted chatter should be admin")
51+
}
52+
}

internal/agent/claim_admin_test.go

Lines changed: 0 additions & 95 deletions
This file was deleted.

internal/agent/loop.go

Lines changed: 0 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -240,98 +240,6 @@ func (a *Agent) bindSession(ctx context.Context, channel, accountID, sessionID,
240240
a.registry.SetExecutor(ex)
241241
}
242242

243-
// maybeClaimChannelAdmin implements first-DM-wins operator claiming on
244-
// self-hosted installs: the first human to DM the agent on an IM channel
245-
// that has no admins allowlist AND no prior chat history on that channel
246-
// becomes the channel's admin. Rationale: the operator who just bound
247-
// their own bot token is invariably the first to message it — making
248-
// them hand-edit an admins list for their own agent is friction with no
249-
// security payoff. The claim never fires when:
250-
// - the deploy is hosted (strangers everywhere),
251-
// - the turn is runtime-originated (cron / heartbeat / subagent),
252-
// - the message came from a group (the first speaker could be anyone),
253-
// - an admins list for the channel already exists (explicit config wins),
254-
// - the channel already has other chat history (an agent that has been
255-
// serving the public must not hand admin to whoever messages next
256-
// after an upgrade).
257-
//
258-
// Persisted through the same layer AgentFileConfigLoader reads (the
259-
// agents.config row via dataStore, agent.json in file mode) so the claim
260-
// survives restarts.
261-
func (a *Agent) maybeClaimChannelAdmin(ctx context.Context, msg bus.InboundMessage) {
262-
if buildinfo.IsHostedDeploy() || msg.Source != bus.SourceUser {
263-
return
264-
}
265-
switch msg.Channel {
266-
case "", "web", "api":
267-
return
268-
}
269-
if msg.PeerKind != "dm" || msg.UserID == "" {
270-
return
271-
}
272-
if len(a.admins[msg.Channel]) > 0 {
273-
return
274-
}
275-
if a.channelHasHistory(ctx, msg.Channel, msg.ChatID) {
276-
return
277-
}
278-
if a.admins == nil {
279-
a.admins = map[string][]string{}
280-
}
281-
a.admins[msg.Channel] = []string{msg.UserID}
282-
if err := a.persistAdmins(ctx); err != nil {
283-
slog.Warn("channel-admin claim not persisted (active for this process only)",
284-
"agent", a.name, "channel", msg.Channel, "error", err)
285-
}
286-
slog.Info("channel admin claimed by first DM chatter",
287-
"agent", a.name, "channel", msg.Channel, "userID", msg.UserID, "sender", msg.SenderName)
288-
}
289-
290-
// channelHasHistory reports whether any OTHER chat session exists for
291-
// (agent, channel). Errors count as history — when in doubt, don't grant
292-
// admin. Bounded page walk; sessions are updated_at-ordered so an active
293-
// channel surfaces in the first page.
294-
func (a *Agent) channelHasHistory(ctx context.Context, channel, currentChatID string) bool {
295-
if a.dataStore == nil {
296-
// File-mode single-user install: no cross-chatter history to protect.
297-
return false
298-
}
299-
const pageSize = 200
300-
for page := range 20 {
301-
metas, total, err := a.dataStore.ListSessionsPaginated(ctx, []string{a.name}, page*pageSize, pageSize)
302-
if err != nil {
303-
return true
304-
}
305-
for _, m := range metas {
306-
if m.Channel == channel && m.ChatID != currentChatID {
307-
return true
308-
}
309-
}
310-
if (page+1)*pageSize >= total || len(metas) == 0 {
311-
return false
312-
}
313-
}
314-
// Thousands of sessions and still unsure — stay conservative.
315-
return true
316-
}
317-
318-
// persistAdmins writes the in-memory admins allowlist to the layer
319-
// AgentFileConfigLoader reads it back from on the next boot.
320-
func (a *Agent) persistAdmins(ctx context.Context) error {
321-
if a.dataStore != nil {
322-
rec, err := a.dataStore.GetAgent(ctx, a.name)
323-
if err == nil && rec != nil {
324-
if rec.Config == nil {
325-
rec.Config = map[string]interface{}{}
326-
}
327-
rec.Config["admins"] = a.admins
328-
return a.dataStore.SaveAgent(ctx, rec)
329-
}
330-
// Lookup miss (agent not in store) → fall through to file.
331-
}
332-
return config.MergeAgentFileAdmins(a.homePath, a.admins)
333-
}
334-
335243
// NewAgent creates a new Agent from a resolved config.
336244
func NewAgent(rc config.ResolvedAgent, prov provider.Provider, mb *bus.MessageBus, homeDir string) *Agent {
337245
return NewAgentWithSkillsCfg(rc, prov, mb, homeDir, config.SkillsCfg{})
@@ -2048,7 +1956,6 @@ func (a *Agent) HandleMessage(ctx context.Context, msg bus.InboundMessage) strin
20481956
// admin. File tools use this to refuse identity-file reads from
20491957
// regular chatters (SOUL/IDENTITY/BOOTSTRAP/... leak as verbatim
20501958
// chat replies otherwise).
2051-
a.maybeClaimChannelAdmin(ctx, msg)
20521959
a.registry.SetCallerIsAdmin(a.isTrustedTurn(msg))
20531960
// Plumb the persistent session_key for goal-scoped tools.
20541961
// SetSessionID above uses msg.ChatID (the channel-level chat
@@ -2810,7 +2717,6 @@ func (a *Agent) HandleMessageStream(ctx context.Context, msg bus.InboundMessage)
28102717
sess.SetProviderModel(prov, mdl)
28112718
}
28122719
a.bindSession(ctx, msg.Channel, msg.AccountID, msg.ChatID, msg.ProjectID)
2813-
a.maybeClaimChannelAdmin(ctx, msg)
28142720
a.registry.SetCallerIsAdmin(a.isTrustedTurn(msg))
28152721
a.registry.SetGoalSessionKey(sess.SessionKey())
28162722
// Per-user file writes (USER.md / MEMORY.md) need to land in the

internal/agent/slash.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,17 @@ func (a *Agent) isAdminChatter(msg bus.InboundMessage) bool {
190190
if msg.Channel == "web" || msg.Channel == "api" {
191191
return msg.UserID != "" && msg.UserID == a.ownerUserID
192192
}
193+
// Shared-identity channels rewrite EVERY speaker's UserID to the
194+
// channel owner's id (routing.processInbound), which makes the
195+
// owner-equality check below meaningless in groups: any group member
196+
// would pass as the owner. The platform-side sender id is gone by
197+
// this point, so there's nothing to match against an allowlist
198+
// either — deny. DMs on a shared-identity channel are fine: the
199+
// owner marked the channel as personally theirs, and a DM sender on
200+
// their own bot is them by construction.
201+
if msg.SharedIdentity && msg.PeerKind == "group" {
202+
return false
203+
}
193204
list, ok := a.admins[msg.Channel]
194205
if !ok || len(list) == 0 {
195206
// No allowlist configured for this channel. Fall back to

internal/config/config.go

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"context"
1212
"encoding/json"
1313
"errors"
14-
"fmt"
1514
"os"
1615
"path/filepath"
1716
"strings"
@@ -536,36 +535,6 @@ func defaultAgentFileConfigLoader(_, home string) (AgentFileConfig, bool) {
536535
return cfg, true
537536
}
538537

539-
// MergeAgentFileAdmins writes the admins allowlist into <home>/agent.json,
540-
// preserving every other field already in the file (raw-map merge, not a
541-
// struct round-trip, so keys this build doesn't know about survive).
542-
// Fallback persistence for the first-DM admin claim when no store-backed
543-
// agents.config row is available; the store path lives on the Agent.
544-
func MergeAgentFileAdmins(home string, admins map[string][]string) error {
545-
if home == "" {
546-
return fmt.Errorf("agent home not set")
547-
}
548-
path := filepath.Join(home, "agent.json")
549-
raw := map[string]json.RawMessage{}
550-
if data, err := os.ReadFile(path); err == nil {
551-
// Corrupt existing JSON → start fresh rather than fail the claim.
552-
_ = json.Unmarshal(data, &raw)
553-
}
554-
blob, err := json.Marshal(admins)
555-
if err != nil {
556-
return err
557-
}
558-
raw["admins"] = blob
559-
out, err := json.MarshalIndent(raw, "", " ")
560-
if err != nil {
561-
return err
562-
}
563-
if err := os.MkdirAll(home, 0o755); err != nil {
564-
return err
565-
}
566-
return os.WriteFile(path, out, 0o600)
567-
}
568-
569538
// AgentFileConfig is the schema for an agent's per-row override JSON
570539
// (agents.config column). Per-agent providers/channels live in their own
571540
// scoped DB tables and are NOT persisted here.

0 commit comments

Comments
 (0)