diff --git a/platform/telegram/internal/bot/bot.go b/platform/telegram/internal/bot/bot.go index a9a7b0b..fbb4ce1 100644 --- a/platform/telegram/internal/bot/bot.go +++ b/platform/telegram/internal/bot/bot.go @@ -54,6 +54,9 @@ type Bot struct { limiter *rate.Limiter // chatID is the moderated discussion chat (0 disables gating). chatID int64 + // botID is the bot's own Telegram user id (resolved at startup); it skips the + // chat_member updates the bot's own restrict actions generate — the grant loop guard. + botID int64 // eligibility resolves a joining user's chat write eligibility; nil leaves a // joiner muted (fail-closed) until it is wired. eligibility EligibilityResolver @@ -134,6 +137,7 @@ func (t *Bot) logChatAdminStatus(ctx context.Context) { t.log.Warn("chat self-check: getMe failed", zap.Error(err)) return } + t.botID = me.ID m, err := t.api.GetChatMember(ctx, &tgbot.GetChatMemberParams{ChatID: t.chatID, UserID: me.ID}) if err != nil { t.log.Warn("chat gating self-check failed: the bot cannot read the chat — is it added and is TELEGRAM_CHAT_ID the discussion group id?", @@ -260,14 +264,19 @@ func (t *Bot) handleChatMember(ctx context.Context, cm *models.ChatMemberUpdated if user != nil { uid = user.ID } - // Log every chat_member update the bot receives: this is the one place to see - // whether Telegram is delivering joins at all, for which chat, and the transition. + // Log every chat_member update the bot receives: the one place to see whether + // Telegram delivers joins, for which chat, the transition, who performed it, and the + // new member's send/membership state. + canSend, isMember := restrictedSendState(cm.NewChatMember) t.log.Info("chat_member update", zap.Int64("chat_id", cm.Chat.ID), zap.Int64("configured_chat_id", t.chatID), zap.Int64("user_id", uid), + zap.Int64("actor_id", cm.From.ID), zap.String("old_status", string(cm.OldChatMember.Type)), - zap.String("new_status", string(cm.NewChatMember.Type))) + zap.String("new_status", string(cm.NewChatMember.Type)), + zap.Bool("new_can_send", canSend), + zap.Bool("new_is_member", isMember)) if t.chatID == 0 || cm.Chat.ID != t.chatID { return @@ -275,22 +284,19 @@ func (t *Bot) handleChatMember(ctx context.Context, cm *models.ChatMemberUpdated if user == nil || user.IsBot { return } - // (Re)grant write to an eligible user who is in the chat but cannot yet send: a join - // into the default-deny chat (where a joined member appears `restricted`, sometimes - // `member`) or a restricted member still lacking send rights. A user who can already - // send is skipped — which also breaks the feedback loop from the bot's own grant, - // since restricting re-fires a chat_member event. Revoking a now-ineligible user is - // the chat-gate path's job (an admin block or chat_muted change), not this one. + // Loop guard: the bot's own restrict re-fires a chat_member update whose performer is + // the bot; skip those so a grant never re-triggers itself. + if t.botID != 0 && cm.From.ID == t.botID { + return + } + // Act only on a user who is in the chat (member or restricted) — a join into the + // default-deny chat appears as `restricted`, not `member`. Left/kicked and admins + // have nothing to grant here. An eligible user is (re)granted write; revoking a + // now-ineligible user is the chat-gate path's job (an admin block or chat_muted). switch cm.NewChatMember.Type { - case models.ChatMemberTypeMember: - // A plain member follows the chat default; in the gated (default-deny) chat that - // denies sending, so (re)grant. - case models.ChatMemberTypeRestricted: - if cm.NewChatMember.Restricted.CanSendMessages { - return // already allowed to send - } + case models.ChatMemberTypeMember, models.ChatMemberTypeRestricted: default: - return // left / kicked / administrator / owner — nothing to grant here + return } if t.eligibility == nil { t.log.Warn("chat access: eligibility resolver not wired", zap.Int64("user_id", uid)) @@ -372,6 +378,16 @@ func chatWritePerms() models.ChatPermissions { } } +// restrictedSendState returns a restricted member's text-send permission and whether +// they are currently a member of the chat; (false, false) for any non-restricted +// status (the fields exist only on the restricted variant). +func restrictedSendState(m models.ChatMember) (canSend, isMember bool) { + if m.Type == models.ChatMemberTypeRestricted && m.Restricted != nil { + return m.Restricted.CanSendMessages, m.Restricted.IsMember + } + return false, false +} + // chatMemberUser returns the user a ChatMember refers to across the union variants, // or nil for an unrecognised type. func chatMemberUser(m models.ChatMember) *models.User { diff --git a/platform/telegram/internal/bot/chat_test.go b/platform/telegram/internal/bot/chat_test.go index acf0330..4641e36 100644 --- a/platform/telegram/internal/bot/chat_test.go +++ b/platform/telegram/internal/bot/chat_test.go @@ -3,6 +3,7 @@ package bot import ( "context" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" @@ -13,7 +14,10 @@ import ( "go.uber.org/zap" ) -const testChatID = 555 +const ( + testChatID = 555 + botSelfID = 111111 // the bot's own id in tests (for the loop guard) +) // chatAPI is a fake Bot API for the chat-gating tests: it answers getMe, returns a // scripted getChatMember status, and records restrictChatMember calls. @@ -59,6 +63,7 @@ func newChatBot(t *testing.T, api *chatAPI) *Bot { if err != nil { t.Fatalf("new bot: %v", err) } + b.botID = botSelfID // normally set at startup; the chat_member updates default actor 0 != this return b } @@ -109,14 +114,22 @@ func TestHandleChatMemberGrantsEligibleMemberJoin(t *testing.T) { } func TestHandleChatMemberGrantsRestrictedMember(t *testing.T) { - // A default-deny group reports a present member as restricted with no send right — - // the real-world case. Both a fresh join (left->restricted) and an already-present - // restricted member (restricted->restricted) must be granted when eligible. - for _, oldType := range []models.ChatMemberType{models.ChatMemberTypeLeft, models.ChatMemberTypeRestricted} { - t.Run(string(oldType), func(t *testing.T) { + // A default-deny group reports a present member as restricted — the real-world case. + // A fresh join (left->restricted) or an already-present restricted member + // (restricted->restricted), and regardless of the reported can_send (which can be + // misleading), an eligible member is granted. + for _, tc := range []struct { + old models.ChatMemberType + canSend bool + }{ + {models.ChatMemberTypeLeft, false}, + {models.ChatMemberTypeRestricted, false}, + {models.ChatMemberTypeRestricted, true}, + } { + t.Run(fmt.Sprintf("%s_cansend=%v", tc.old, tc.canSend), func(t *testing.T) { api := &chatAPI{} b := eligibleBot(t, api, true, nil) - b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, oldType, false)) + b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, tc.old, tc.canSend)) if len(api.restricts) != 1 || !api.restricts[0].canSend { t.Fatalf("restricts = %+v, want one grant for a restricted member", api.restricts) } @@ -124,14 +137,16 @@ func TestHandleChatMemberGrantsRestrictedMember(t *testing.T) { } } -func TestHandleChatMemberSkipsAlreadyAllowed(t *testing.T) { - // The bot's own grant re-fires a chat_member event (restricted, can_send=true); it - // must not loop into another grant. +func TestHandleChatMemberSkipsBotsOwnAction(t *testing.T) { + // The bot's own grant re-fires a chat_member update performed by the bot; it must be + // skipped so a grant never loops into another grant. api := &chatAPI{} b := eligibleBot(t, api, true, nil) - b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, models.ChatMemberTypeRestricted, true)) + upd := restrictedUpdate(testChatID, 777, models.ChatMemberTypeRestricted, false) + upd.From = models.User{ID: botSelfID} + b.handleChatMember(context.Background(), upd) if len(api.restricts) != 0 { - t.Fatalf("restricts = %+v, want none for an already-allowed member (no loop)", api.restricts) + t.Fatalf("restricts = %+v, want none for the bot's own action (no loop)", api.restricts) } }