From ee2ff48c8131db41d96d5e496352a238f3315372 Mon Sep 17 00:00:00 2001 From: sdhfsl Date: Sun, 27 Sep 2026 02:31:39 +0800 Subject: [PATCH] fix(tgbot): scope the add-client wizard to the admin, not the chat (#6604) Two admins in one group chat shared one draft and one wizard step: clientDrafts and userStateStore were keyed by chat id alone, so the second admin's wizard opened on the first one's email and limits, and whichever of them tapped a control last decided what the other created. Key both stores by (chat, user) instead. A private chat is unaffected: its two ids are equal, so the key matches what the chat alone used to be. A message with no sender (a channel post) keys to user 0, which no admin holds. Fixes #6593. --- internal/web/service/tgbot/tgbot.go | 66 ++++++++---- .../tgbot/tgbot_add_client_expiry_test.go | 4 +- .../tgbot_client_draft_per_admin_test.go | 102 ++++++++++++++++++ .../tgbot/tgbot_client_draft_per_chat_test.go | 4 +- .../service/tgbot/tgbot_delete_after_test.go | 4 +- .../service/tgbot/tgbot_draft_render_test.go | 8 +- internal/web/service/tgbot/tgbot_router.go | 44 ++++---- 7 files changed, 179 insertions(+), 53 deletions(-) create mode 100644 internal/web/service/tgbot/tgbot_client_draft_per_admin_test.go diff --git a/internal/web/service/tgbot/tgbot.go b/internal/web/service/tgbot/tgbot.go index 1f9d0f5fd..247efc6c8 100644 --- a/internal/web/service/tgbot/tgbot.go +++ b/internal/web/service/tgbot/tgbot.go @@ -82,30 +82,52 @@ type clientDraft struct { reset int } -// clientDrafts keys a draft by chat: the steps arrive on the worker pool, so a -// single draft let two admins fill in one client between them. -type clientDrafts struct { - mu sync.Mutex - drafts map[int64]*clientDraft +// chatUser names the admin a wizard belongs to. A private chat's ids are equal; +// in a group they are not, and each admin at its keyboard fills in their own. +type chatUser struct { + chatID int64 + userID int64 } -var addClientDrafts = &clientDrafts{drafts: make(map[int64]*clientDraft)} +// messageActor reads the sender off a message. A post without one (a channel) +// keys to user 0, an id no admin can hold. +func messageActor(message telego.Message) chatUser { + if message.From == nil { + return chatUser{chatID: message.Chat.ID} + } + return chatUser{chatID: message.Chat.ID, userID: message.From.ID} +} -func (s *clientDrafts) forChat(chatID int64) *clientDraft { +// callbackActor reads the admin who tapped the button, not the chat the keyboard +// sits in: every admin in a group sees the same keyboard. +func callbackActor(callbackQuery *telego.CallbackQuery) chatUser { + return chatUser{chatID: callbackQuery.Message.GetChat().ID, userID: callbackQuery.From.ID} +} + +// clientDrafts keys a draft by the admin filling it in: the steps arrive on the +// worker pool, so one draft let two admins fill in one client between them. +type clientDrafts struct { + mu sync.Mutex + drafts map[chatUser]*clientDraft +} + +var addClientDrafts = &clientDrafts{drafts: make(map[chatUser]*clientDraft)} + +func (s *clientDrafts) forActor(actor chatUser) *clientDraft { s.mu.Lock() defer s.mu.Unlock() - draft, ok := s.drafts[chatID] + draft, ok := s.drafts[actor] if !ok { draft = &clientDraft{} - s.drafts[chatID] = draft + s.drafts[actor] = draft } return draft } -func (s *clientDrafts) reset(chatID int64) { +func (s *clientDrafts) reset(actor chatUser) { s.mu.Lock() defer s.mu.Unlock() - delete(s.drafts, chatID) + delete(s.drafts, actor) } // isAddClientStep reports whether callback data belongs to the add-client @@ -117,17 +139,17 @@ func isAddClientStep(data string) bool { func (s *clientDrafts) resetAll() { s.mu.Lock() defer s.mu.Unlock() - s.drafts = make(map[int64]*clientDraft) + s.drafts = make(map[chatUser]*clientDraft) } -// userStateStore guards the per-chat conversation states. The Telegram command +// userStateStore guards the per-admin conversation states. The Telegram command // and callback handlers run on a worker-pool goroutine while the message handler // runs on the dispatch goroutine, so a bare map would be a concurrent-map-write // crash. It also expires abandoned conversations so a user who starts a flow and // goes silent doesn't leave an entry forever. type userStateStore struct { mu sync.Mutex - states map[int64]userStateEntry + states map[chatUser]userStateEntry lastPrune time.Time } @@ -136,30 +158,30 @@ type userStateEntry struct { at time.Time } -var userStateMgr = &userStateStore{states: make(map[int64]userStateEntry)} +var userStateMgr = &userStateStore{states: make(map[chatUser]userStateEntry)} -func (s *userStateStore) set(chatID int64, state string) { +func (s *userStateStore) set(actor chatUser, state string) { s.mu.Lock() - s.states[chatID] = userStateEntry{state: state, at: time.Now()} + s.states[actor] = userStateEntry{state: state, at: time.Now()} s.mu.Unlock() } -func (s *userStateStore) get(chatID int64) (string, bool) { +func (s *userStateStore) get(actor chatUser) (string, bool) { s.mu.Lock() defer s.mu.Unlock() - e, ok := s.states[chatID] + e, ok := s.states[actor] return e.state, ok } -func (s *userStateStore) clear(chatID int64) { +func (s *userStateStore) clear(actor chatUser) { s.mu.Lock() - delete(s.states, chatID) + delete(s.states, actor) s.mu.Unlock() } func (s *userStateStore) reset() { s.mu.Lock() - s.states = make(map[int64]userStateEntry) + s.states = make(map[chatUser]userStateEntry) s.mu.Unlock() } diff --git a/internal/web/service/tgbot/tgbot_add_client_expiry_test.go b/internal/web/service/tgbot/tgbot_add_client_expiry_test.go index d3bc66162..1f6217490 100644 --- a/internal/web/service/tgbot/tgbot_add_client_expiry_test.go +++ b/internal/web/service/tgbot/tgbot_add_client_expiry_test.go @@ -20,10 +20,10 @@ func TestAddClientExpiryPresetReplacesTheTerm(t *testing.T) { swapTestBot(t, url) // A fresh draft attaches no inbound, so the card never looks one up by remark. - draft := addClientDrafts.forChat(chatID) + draft := addClientDrafts.forActor(chatUser{chatID: chatID, userID: 1}) origRunning := isRunning t.Cleanup(func() { - addClientDrafts.reset(chatID) + addClientDrafts.reset(chatUser{chatID: chatID, userID: 1}) isRunning = origRunning }) isRunning = true diff --git a/internal/web/service/tgbot/tgbot_client_draft_per_admin_test.go b/internal/web/service/tgbot/tgbot_client_draft_per_admin_test.go new file mode 100644 index 000000000..a5f9c83fc --- /dev/null +++ b/internal/web/service/tgbot/tgbot_client_draft_per_admin_test.go @@ -0,0 +1,102 @@ +package tgbot + +import ( + "path/filepath" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + + "github.com/mymmrac/telego" +) + +// Regression test: keying the add-client wizard by chat alone left the two +// admins of a group chat filling in one client between them. +func TestAddClientDraftIsPerAdminInGroupChat(t *testing.T) { + if err := database.InitDB(filepath.Join(t.TempDir(), "x-ui.db")); err != nil { + t.Fatalf("InitDB: %v", err) + } + t.Cleanup(func() { _ = database.CloseDB() }) + + const ( + groupChat = int64(-1001234567890) + adminA = int64(8101) + adminB = int64(8202) + ) + url, textsFor := draftTexts(t) + swapTestBot(t, url) + origRunning := isRunning + t.Cleanup(func() { + isRunning = origRunning + userStateMgr.reset() + }) + isRunning = true + + // Both admins tap in the same chat; only the sender tells them apart. + tap := func(userID int64, data string) { + t.Helper() + (&Tgbot{}).answerCallback(&telego.CallbackQuery{ + ID: "q1", + From: telego.User{ID: userID}, + Data: data, + Message: &telego.Message{MessageID: 7, Chat: telego.Chat{ID: groupChat}}, + }, true) + } + + tap(adminA, "add_client_to 1") + emailA := cardEmail(t, lastDraftCard(t, textsFor(groupChat))) + tap(adminB, "add_client_to 2") + emailB := cardEmail(t, lastDraftCard(t, textsFor(groupChat))) + if emailA == "" || emailA == emailB { + t.Fatalf("both admins start with email %q, want one draft per admin", emailA) + } + + // Admin A renders again, with admin B's wizard already past its start. + tap(adminA, "add_client_default_traffic_exp") + if got := cardEmail(t, lastDraftCard(t, textsFor(groupChat))); got != emailA { + t.Errorf("admin A's card shows email %q, want its own %q from admin B's draft", got, emailA) + } + + // The step they are each on is their own too: A's prompt must not put B into + // the same step, or B's next message lands in A's wizard. + tap(adminA, "add_client_ch_default_email") + if st, ok := userStateMgr.get(chatUser{chatID: groupChat, userID: adminA}); !ok || st != "awaiting_email" { + t.Errorf("admin A's step = %q (set: %v), want awaiting_email", st, ok) + } + if st, ok := userStateMgr.get(chatUser{chatID: groupChat, userID: adminB}); ok { + t.Errorf("admin B's step = %q, want none: only the tapper's step may change", st) + } +} + +// The wizard's typed steps arrive as messages, and that handler is a closure no +// test can drive, so pin the key it takes there: sender, not chat. +func TestMessageActorSeparatesAdminsInOneChat(t *testing.T) { + const ( + groupChat = int64(-1001234567890) + adminA = int64(8101) + adminB = int64(8202) + ) + t.Cleanup(userStateMgr.reset) + + fromA := messageActor(telego.Message{Chat: telego.Chat{ID: groupChat}, From: &telego.User{ID: adminA}}) + fromB := messageActor(telego.Message{Chat: telego.Chat{ID: groupChat}, From: &telego.User{ID: adminB}}) + // A channel post carries no sender and must not land on an admin's step. + fromChannel := messageActor(telego.Message{Chat: telego.Chat{ID: groupChat}}) + + if want := (chatUser{chatID: groupChat, userID: adminA}); fromA != want { + t.Errorf("message from admin A keyed as %+v, want %+v", fromA, want) + } + if fromA == fromB || fromA == fromChannel || fromB == fromChannel { + t.Fatalf("keys collide: %+v, %+v, %+v", fromA, fromB, fromChannel) + } + + userStateMgr.set(fromA, "awaiting_email") + if st, ok := userStateMgr.get(fromB); ok { + t.Errorf("admin B sees step %q: admin A's answer would land in B's wizard", st) + } + if st, ok := userStateMgr.get(fromChannel); ok { + t.Errorf("a senderless post sees step %q", st) + } + if st, ok := userStateMgr.get(fromA); !ok || st != "awaiting_email" { + t.Errorf("admin A's step = %q (set: %v), want awaiting_email", st, ok) + } +} diff --git a/internal/web/service/tgbot/tgbot_client_draft_per_chat_test.go b/internal/web/service/tgbot/tgbot_client_draft_per_chat_test.go index 548beee66..e08e02fbc 100644 --- a/internal/web/service/tgbot/tgbot_client_draft_per_chat_test.go +++ b/internal/web/service/tgbot/tgbot_client_draft_per_chat_test.go @@ -139,7 +139,7 @@ func TestNonWizardCallbackTakesNoDraftLock(t *testing.T) { ) decliningServer(t) - held := addClientDrafts.forChat(heldChat) + held := addClientDrafts.forActor(chatUser{chatID: heldChat, userID: 1}) held.Lock() defer held.Unlock() @@ -170,7 +170,7 @@ func TestNonWizardCallbackTakesNoDraftLock(t *testing.T) { tap(spareChat, false, "add_client_to 1") addClientDrafts.mu.Lock() - _, stored := addClientDrafts.drafts[spareChat] + _, stored := addClientDrafts.drafts[chatUser{chatID: spareChat, userID: 1}] addClientDrafts.mu.Unlock() if stored { t.Errorf("draft stored for chat %d, want none until its wizard starts", spareChat) diff --git a/internal/web/service/tgbot/tgbot_delete_after_test.go b/internal/web/service/tgbot/tgbot_delete_after_test.go index 4a6ca7329..a74c19cce 100644 --- a/internal/web/service/tgbot/tgbot_delete_after_test.go +++ b/internal/web/service/tgbot/tgbot_delete_after_test.go @@ -7,12 +7,12 @@ func TestDeleteMessageAfterDelayKeepsUserState(t *testing.T) { t.Cleanup(userStateMgr.reset) const chatID = int64(4242) - userStateMgr.set(chatID, "awaiting_comment") + userStateMgr.set(chatUser{chatID: chatID, userID: 1}, "awaiting_comment") tg := &Tgbot{} tg.deleteMessageAfterDelay(chatID, 1, 0) - if st, ok := userStateMgr.get(chatID); !ok || st != "awaiting_comment" { + if st, ok := userStateMgr.get(chatUser{chatID: chatID, userID: 1}); !ok || st != "awaiting_comment" { t.Fatalf("delayed message deletion cleared the conversation state: got (%q, %v), want (%q, true)", st, ok, "awaiting_comment") } } diff --git a/internal/web/service/tgbot/tgbot_draft_render_test.go b/internal/web/service/tgbot/tgbot_draft_render_test.go index 1ec6464c2..aacc14be2 100644 --- a/internal/web/service/tgbot/tgbot_draft_render_test.go +++ b/internal/web/service/tgbot/tgbot_draft_render_test.go @@ -20,8 +20,8 @@ const clientDraftTestChatID = -9001 // Regression test: the draft is sent with ParseMode HTML, so Markdown markers // were rendered literally and an unescaped value could break the whole message. func TestClientDraftMessageRendersHTML(t *testing.T) { - draft := addClientDrafts.forChat(clientDraftTestChatID) - t.Cleanup(func() { addClientDrafts.reset(clientDraftTestChatID) }) + draft := addClientDrafts.forActor(chatUser{chatID: clientDraftTestChatID, userID: 1}) + t.Cleanup(func() { addClientDrafts.reset(chatUser{chatID: clientDraftTestChatID, userID: 1}) }) draft.email = "a@b.c" draft.comment = "promo & <10 GB>" @@ -67,10 +67,10 @@ func TestAddClientPromptsEscapeDraftValues(t *testing.T) { url, texts := draftTexts(t) swapTestBot(t, url) - draft := addClientDrafts.forChat(1) + draft := addClientDrafts.forActor(chatUser{chatID: 1, userID: 1}) origRunning := isRunning t.Cleanup(func() { - addClientDrafts.reset(1) + addClientDrafts.reset(chatUser{chatID: 1, userID: 1}) isRunning = origRunning }) isRunning = true diff --git a/internal/web/service/tgbot/tgbot_router.go b/internal/web/service/tgbot/tgbot_router.go index 1664b2049..ebccaf9dd 100644 --- a/internal/web/service/tgbot/tgbot_router.go +++ b/internal/web/service/tgbot/tgbot_router.go @@ -82,7 +82,7 @@ func (t *Tgbot) OnReceive() { h.HandleMessage(func(ctx *th.Context, message telego.Message) error { defer recoverBotPanic() - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(messageActor(message)) t.SendMsgToTgbot(message.Chat.ID, t.I18nBot("tgbot.keyboardClosed"), tu.ReplyKeyboardRemove()) return nil }, th.TextEqual(t.I18nBot("tgbot.buttons.closeKeyboard"))) @@ -95,7 +95,7 @@ func (t *Tgbot) OnReceive() { // Use goroutine with worker pool for concurrent command processing go runBotHandler(func() { - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(messageActor(message)) t.answerCommand(&message, message.Chat.ID, checkAdmin(message.From.ID)) }) return nil @@ -104,7 +104,7 @@ func (t *Tgbot) OnReceive() { h.HandleCallbackQuery(func(ctx *th.Context, query telego.CallbackQuery) error { // Use goroutine with worker pool for concurrent callback processing go runBotHandler(func() { - userStateMgr.clear(query.Message.GetChat().ID) + userStateMgr.clear(callbackActor(&query)) t.answerCallback(&query, checkAdmin(query.From.ID)) }) return nil @@ -113,22 +113,23 @@ func (t *Tgbot) OnReceive() { h.HandleMessage(func(ctx *th.Context, message telego.Message) error { defer recoverBotPanic() userStateMgr.maybePrune(time.Hour) - if userState, exists := userStateMgr.get(message.Chat.ID); exists { + actor := messageActor(message) + if userState, exists := userStateMgr.get(actor); exists { // Only a wizard step touches the draft, so only it takes the lock. - draft := addClientDrafts.forChat(message.Chat.ID) + draft := addClientDrafts.forActor(actor) draft.Lock() defer draft.Unlock() switch userState { case "awaiting_email": if draft.email == strings.TrimSpace(message.Text) { t.SendMsgToTgbotDeleteAfter(message.Chat.ID, t.I18nBot("tgbot.messages.using_default_value"), 3, tu.ReplyKeyboardRemove()) - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(actor) return nil } draft.email = strings.TrimSpace(message.Text) if t.isSingleWord(draft.email) { - userStateMgr.set(message.Chat.ID, "awaiting_email") + userStateMgr.set(actor, "awaiting_email") cancel_btn_markup := tu.InlineKeyboard( tu.InlineKeyboardRow( @@ -139,26 +140,26 @@ func (t *Tgbot) OnReceive() { t.SendMsgToTgbot(message.Chat.ID, t.I18nBot("tgbot.messages.incorrect_input"), cancel_btn_markup) } else { t.SendMsgToTgbotDeleteAfter(message.Chat.ID, t.I18nBot("tgbot.messages.received_email"), 3, tu.ReplyKeyboardRemove()) - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(actor) t.addClient(message.Chat.ID, draft, t.BuildClientDraftMessage(draft)) } case "awaiting_comment": if draft.comment == strings.TrimSpace(message.Text) { t.SendMsgToTgbotDeleteAfter(message.Chat.ID, t.I18nBot("tgbot.messages.using_default_value"), 3, tu.ReplyKeyboardRemove()) - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(actor) return nil } draft.comment = strings.TrimSpace(message.Text) t.SendMsgToTgbotDeleteAfter(message.Chat.ID, t.I18nBot("tgbot.messages.received_comment"), 3, tu.ReplyKeyboardRemove()) - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(actor) t.addClient(message.Chat.ID, draft, t.BuildClientDraftMessage(draft)) case "awaiting_tg_id": input := strings.TrimSpace(message.Text) if input == "" || input == "-" || strings.EqualFold(input, "none") { draft.tgID = "" t.SendMsgToTgbotDeleteAfter(message.Chat.ID, t.I18nBot("tgbot.messages.using_default_value"), 3, tu.ReplyKeyboardRemove()) - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(actor) t.addClient(message.Chat.ID, draft, t.BuildClientDraftMessage(draft)) return nil } @@ -173,7 +174,7 @@ func (t *Tgbot) OnReceive() { } draft.tgID = input t.SendMsgToTgbotDeleteAfter(message.Chat.ID, t.I18nBot("tgbot.messages.userSaved"), 3, tu.ReplyKeyboardRemove()) - userStateMgr.clear(message.Chat.ID) + userStateMgr.clear(actor) t.addClient(message.Chat.ID, draft, t.BuildClientDraftMessage(draft)) } } else { @@ -315,12 +316,13 @@ func isCommandForBot(text string, username string) bool { // answerCallback processes callback queries from inline keyboards. func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool) { chatId := callbackQuery.Message.GetChat().ID + actor := callbackActor(callbackQuery) // Only an admin's wizard callbacks touch a draft, so only they take its lock: // a report tap must not wait on a slot, a rejected chat must not be stored. var draft *clientDraft if isAdmin && isAddClientStep(callbackQuery.Data) { - draft = addClientDrafts.forChat(chatId) + draft = addClientDrafts.forActor(actor) draft.Lock() defer draft.Unlock() } @@ -1066,7 +1068,7 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool t.SendMsgToTgbot(chatId, t.I18nBot("tgbot.answers.chooseInbound"), inbounds) case "add_client_ch_default_email": t.deleteMessageTgBot(chatId, callbackQuery.Message.GetMessageID()) - userStateMgr.set(chatId, "awaiting_email") + userStateMgr.set(actor, "awaiting_email") cancel_btn_markup := tu.InlineKeyboard( tu.InlineKeyboardRow( tu.InlineKeyboardButton(t.I18nBot("tgbot.buttons.use_default")).WithCallbackData("add_client_default_info"), @@ -1076,7 +1078,7 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool t.SendMsgToTgbot(chatId, prompt_message, cancel_btn_markup) case "add_client_ch_default_comment": t.deleteMessageTgBot(chatId, callbackQuery.Message.GetMessageID()) - userStateMgr.set(chatId, "awaiting_comment") + userStateMgr.set(actor, "awaiting_comment") cancel_btn_markup := tu.InlineKeyboard( tu.InlineKeyboardRow( tu.InlineKeyboardButton(t.I18nBot("tgbot.buttons.use_default")).WithCallbackData("add_client_default_info"), @@ -1086,7 +1088,7 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool t.SendMsgToTgbot(chatId, prompt_message, cancel_btn_markup) case "add_client_ch_default_tg_id": t.deleteMessageTgBot(chatId, callbackQuery.Message.GetMessageID()) - userStateMgr.set(chatId, "awaiting_tg_id") + userStateMgr.set(actor, "awaiting_tg_id") cancel_btn_markup := tu.InlineKeyboard( tu.InlineKeyboardRow( tu.InlineKeyboardButton(t.I18nBot("tgbot.buttons.use_default")).WithCallbackData("add_client_default_info"), @@ -1189,11 +1191,11 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool case "add_client_default_info": t.deleteMessageTgBot(chatId, callbackQuery.Message.GetMessageID()) t.SendMsgToTgbotDeleteAfter(chatId, t.I18nBot("tgbot.messages.using_default_value"), 3, tu.ReplyKeyboardRemove()) - userStateMgr.clear(chatId) + userStateMgr.clear(actor) t.addClient(chatId, draft, t.BuildClientDraftMessage(draft)) case "add_client_cancel": - userStateMgr.clear(chatId) - addClientDrafts.reset(chatId) + userStateMgr.clear(actor) + addClientDrafts.reset(actor) t.deleteMessageTgBot(chatId, callbackQuery.Message.GetMessageID()) t.SendMsgToTgbotDeleteAfter(chatId, t.I18nBot("tgbot.messages.cancel"), 3, tu.ReplyKeyboardRemove()) case "add_client_default_traffic_exp": @@ -1235,7 +1237,7 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool t.SendMsgToTgbot(chatId, t.I18nBot("tgbot.answers.successfulOperation"), tu.ReplyKeyboardRemove()) t.sendClientIndividualLinks(chatId, draft.email) t.sendClientQRLinks(chatId, draft.email) - addClientDrafts.reset(chatId) + addClientDrafts.reset(actor) } case "add_client_submit_enable": draft.enable = true @@ -1248,7 +1250,7 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool t.SendMsgToTgbot(chatId, t.I18nBot("tgbot.answers.successfulOperation"), tu.ReplyKeyboardRemove()) t.sendClientIndividualLinks(chatId, draft.email) t.sendClientQRLinks(chatId, draft.email) - addClientDrafts.reset(chatId) + addClientDrafts.reset(actor) } case "reset_all_traffics_cancel": t.deleteMessageTgBot(chatId, callbackQuery.Message.GetMessageID())