From bdd1cc7d85ecfa53c65cd20953a073566e1f8b18 Mon Sep 17 00:00:00 2001 From: Ilia Denisov Date: Sun, 21 Jun 2026 16:50:44 +0200 Subject: [PATCH] =?UTF-8?q?fix(telegram):=20invert=20the=20chat=20gate=20?= =?UTF-8?q?=E2=80=94=20mute=20the=20ineligible=20(default-allow)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Telegram intersects the chat default with each user's permissions, so a per-user grant can never exceed a deny-by-default group: the original default-deny + grant design could not let any user write (can_send=true was AND-ed with the denying default). Invert it — the chat allows sending by default and the bot MUTES an ineligible member (unregistered, admin-suspended, or chat_muted) and restores an eligible one it had muted, acting only when the current state differs (idempotent, no self-loop). The block/unblock/chat_muted/registration path already sets can_send to the eligibility, so it is unchanged. --- platform/telegram/internal/bot/bot.go | 48 +++++--- platform/telegram/internal/bot/chat_test.go | 118 +++++++++++--------- 2 files changed, 100 insertions(+), 66 deletions(-) diff --git a/platform/telegram/internal/bot/bot.go b/platform/telegram/internal/bot/bot.go index fbb4ce1..cb1dc85 100644 --- a/platform/telegram/internal/bot/bot.go +++ b/platform/telegram/internal/bot/bot.go @@ -254,10 +254,12 @@ func (t *Bot) SetEligibilityResolver(resolve EligibilityResolver) { t.eligibility = resolve } -// handleChatMember grants write access to a user who joins the moderated chat when -// they are registered and not blocked. A non-eligible joiner is left muted (the chat -// defaults to no-send), and a resolve failure fails closed (also left muted). Other -// status changes (leaves, restrictions, admin edits) are ignored. +// handleChatMember keeps a chat member's write access in sync with their eligibility. +// The chat allows sending by default, so the bot mutes an ineligible member (not +// registered, or admin-suspended, or chat_muted) and restores an eligible one it had +// muted; an eligible member that can already send is left untouched. It acts only when +// the current state differs from the desired one, so it is idempotent and does not +// re-act on its own change; a resolve failure makes no change. func (t *Bot) handleChatMember(ctx context.Context, cm *models.ChatMemberUpdated) { user := chatMemberUser(cm.NewChatMember) var uid int64 @@ -289,13 +291,22 @@ func (t *Bot) handleChatMember(ctx context.Context, cm *models.ChatMemberUpdated 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). + // The chat allows sending by default and the bot only restricts: Telegram intersects + // the chat default with the per-user permission, so a per-user grant cannot exceed a + // deny-by-default — the gate must mute the ineligible, not grant the eligible. + // Determine whether the user is in the chat and can currently send: a plain member + // follows the permissive default; a restricted member can send only with + // CanSendMessages, and only while a member. + var inChat, currentlyCanSend bool switch cm.NewChatMember.Type { - case models.ChatMemberTypeMember, models.ChatMemberTypeRestricted: + case models.ChatMemberTypeMember: + inChat, currentlyCanSend = true, true + case models.ChatMemberTypeRestricted: + inChat, currentlyCanSend = isMember, canSend default: + return // left / kicked / administrator / owner — not a member to gate + } + if !inChat { return } if t.eligibility == nil { @@ -307,15 +318,20 @@ func (t *Bot) handleChatMember(ctx context.Context, cm *models.ChatMemberUpdated t.log.Warn("chat access eligibility failed", zap.Int64("user_id", user.ID), zap.Error(err)) return } - t.log.Info("chat access evaluated", zap.Int64("user_id", user.ID), zap.Bool("eligible", eligible)) - if !eligible { - return // not registered or blocked: leave muted - } - if err := t.setChatWrite(ctx, user.ID, true); err != nil { - t.log.Warn("grant chat write failed", zap.Int64("user_id", user.ID), zap.Error(err)) + t.log.Info("chat access evaluated", + zap.Int64("user_id", user.ID), zap.Bool("eligible", eligible), zap.Bool("can_send", currentlyCanSend)) + // Desired: an eligible user may send, an ineligible one may not. Act only when the + // current state differs — idempotent, a no-op for the common eligible member, and it + // keeps the bot from re-acting on its own change. + if eligible == currentlyCanSend { return } - t.log.Info("chat write granted", zap.Int64("user_id", user.ID)) + if err := t.setChatWrite(ctx, user.ID, eligible); err != nil { + t.log.Warn("set chat write failed", + zap.Int64("user_id", user.ID), zap.Bool("can_send", eligible), zap.Error(err)) + return + } + t.log.Info("chat access applied", zap.Int64("user_id", user.ID), zap.Bool("can_send", eligible)) } // ApplyChatGate applies a chat-gate command (an admin block/unblock or chat_muted diff --git a/platform/telegram/internal/bot/chat_test.go b/platform/telegram/internal/bot/chat_test.go index 4641e36..6fb09af 100644 --- a/platform/telegram/internal/bot/chat_test.go +++ b/platform/telegram/internal/bot/chat_test.go @@ -3,7 +3,6 @@ package bot import ( "context" "encoding/json" - "fmt" "io" "net/http" "net/http/httptest" @@ -77,13 +76,13 @@ func memberUpdate(chatID, userID int64, oldType models.ChatMemberType) *models.C } // restrictedUpdate builds an oldType -> restricted transition for userID, the new -// member's text-send permission set to canSend. A default-deny group reports a present -// member as restricted with canSend=false. -func restrictedUpdate(chatID, userID int64, oldType models.ChatMemberType, canSend bool) *models.ChatMemberUpdated { +// member's text-send permission set to canSend and membership to isMember. A muted +// member is restricted with canSend=false; an un-muted one with canSend=true. +func restrictedUpdate(chatID, userID int64, oldType models.ChatMemberType, canSend, isMember bool) *models.ChatMemberUpdated { return &models.ChatMemberUpdated{ Chat: models.Chat{ID: chatID}, OldChatMember: models.ChatMember{Type: oldType, Left: &models.ChatMemberLeft{User: &models.User{ID: userID}}}, - NewChatMember: models.ChatMember{Type: models.ChatMemberTypeRestricted, Restricted: &models.ChatMemberRestricted{User: &models.User{ID: userID}, CanSendMessages: canSend}}, + NewChatMember: models.ChatMember{Type: models.ChatMemberTypeRestricted, Restricted: &models.ChatMemberRestricted{User: &models.User{ID: userID}, CanSendMessages: canSend, IsMember: isMember}}, } } @@ -104,45 +103,73 @@ func eligibleBot(t *testing.T, api *chatAPI, eligible bool, err error) *Bot { return b } -func TestHandleChatMemberGrantsEligibleMemberJoin(t *testing.T) { +func TestHandleChatMemberMutesIneligibleMember(t *testing.T) { + // An unregistered/blocked member can send by the permissive default, so the bot mutes. api := &chatAPI{} - b := eligibleBot(t, api, true, nil) + b := eligibleBot(t, api, false, nil) b.handleChatMember(context.Background(), memberUpdate(testChatID, 777, models.ChatMemberTypeLeft)) - if len(api.restricts) != 1 || api.restricts[0].userID != "777" || !api.restricts[0].canSend { - t.Fatalf("restricts = %+v, want one grant (can_send=true) for 777", api.restricts) + if len(api.restricts) != 1 || api.restricts[0].userID != "777" || api.restricts[0].canSend { + t.Fatalf("restricts = %+v, want one mute (can_send=false) for 777", api.restricts) } } -func TestHandleChatMemberGrantsRestrictedMember(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, 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) - } - }) +func TestHandleChatMemberLeavesEligibleMemberAlone(t *testing.T) { + // An eligible plain member already sends (the permissive default); no action needed. + api := &chatAPI{} + b := eligibleBot(t, api, true, nil) + b.handleChatMember(context.Background(), memberUpdate(testChatID, 777, models.ChatMemberTypeLeft)) + if len(api.restricts) != 0 { + t.Fatalf("restricts = %+v, want none for an eligible member (already allowed)", api.restricts) + } +} + +func TestHandleChatMemberUnmutesEligibleRestricted(t *testing.T) { + // An eligible member the bot had muted (restricted, can_send=false) is restored. + api := &chatAPI{} + b := eligibleBot(t, api, true, nil) + b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, models.ChatMemberTypeRestricted, false, true)) + if len(api.restricts) != 1 || !api.restricts[0].canSend { + t.Fatalf("restricts = %+v, want one un-mute (can_send=true)", api.restricts) + } +} + +func TestHandleChatMemberLeavesEligibleAllowedRestrictedAlone(t *testing.T) { + // An eligible restricted member who can already send needs no change — the real case + // from the contour (restricted, can_send=true, eligible). + api := &chatAPI{} + b := eligibleBot(t, api, true, nil) + b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, models.ChatMemberTypeRestricted, true, true)) + if len(api.restricts) != 0 { + t.Fatalf("restricts = %+v, want none for an eligible already-allowed member", api.restricts) + } +} + +func TestHandleChatMemberMutesIneligibleRestricted(t *testing.T) { + // An ineligible member who can still send is muted. + api := &chatAPI{} + b := eligibleBot(t, api, false, nil) + b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, models.ChatMemberTypeRestricted, true, true)) + if len(api.restricts) != 1 || api.restricts[0].canSend { + t.Fatalf("restricts = %+v, want one mute (can_send=false)", api.restricts) + } +} + +func TestHandleChatMemberSkipsNonMember(t *testing.T) { + // A restricted record for a user no longer in the chat (is_member=false) is not acted on. + api := &chatAPI{} + b := eligibleBot(t, api, false, nil) + b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, models.ChatMemberTypeRestricted, true, false)) + if len(api.restricts) != 0 { + t.Fatalf("restricts = %+v, want none for a non-member", api.restricts) } } 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. + // The bot's own restrict re-fires a chat_member update performed by the bot; skip it + // so an action never loops (the resolver here would otherwise mute). api := &chatAPI{} - b := eligibleBot(t, api, true, nil) - upd := restrictedUpdate(testChatID, 777, models.ChatMemberTypeRestricted, false) + b := eligibleBot(t, api, false, nil) + upd := memberUpdate(testChatID, 777, models.ChatMemberTypeLeft) upd.From = models.User{ID: botSelfID} b.handleChatMember(context.Background(), upd) if len(api.restricts) != 0 { @@ -150,30 +177,21 @@ func TestHandleChatMemberSkipsBotsOwnAction(t *testing.T) { } } -func TestHandleChatMemberSkipsIneligible(t *testing.T) { +func TestHandleChatMemberNoChangeOnResolveError(t *testing.T) { api := &chatAPI{} - b := eligibleBot(t, api, false, nil) - b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, models.ChatMemberTypeLeft, false)) + b := eligibleBot(t, api, false, context.DeadlineExceeded) + b.handleChatMember(context.Background(), memberUpdate(testChatID, 777, models.ChatMemberTypeLeft)) if len(api.restricts) != 0 { - t.Fatalf("an ineligible member was granted: %+v (want left muted)", api.restricts) - } -} - -func TestHandleChatMemberFailsClosedOnResolveError(t *testing.T) { - api := &chatAPI{} - b := eligibleBot(t, api, true, context.DeadlineExceeded) - b.handleChatMember(context.Background(), restrictedUpdate(testChatID, 777, models.ChatMemberTypeLeft, false)) - if len(api.restricts) != 0 { - t.Fatalf("a resolve error still granted write: %+v (want fail-closed)", api.restricts) + t.Fatalf("a resolve error still changed access: %+v (want no change)", api.restricts) } } func TestHandleChatMemberIgnoresOtherChatAndLeaves(t *testing.T) { api := &chatAPI{} - b := eligibleBot(t, api, true, nil) + b := eligibleBot(t, api, false, nil) // ineligible — would mute if it acted ctx := context.Background() - b.handleChatMember(ctx, restrictedUpdate(999, 777, models.ChatMemberTypeLeft, false)) // foreign chat - b.handleChatMember(ctx, leftUpdate(testChatID, 777)) // a leave + b.handleChatMember(ctx, memberUpdate(999, 777, models.ChatMemberTypeLeft)) // foreign chat + b.handleChatMember(ctx, leftUpdate(testChatID, 777)) // a leave if len(api.restricts) != 0 { t.Fatalf("restricts = %+v, want none for a foreign chat / a leave", api.restricts) }