diff --git a/internal/channels/bitrix24/handle.go b/internal/channels/bitrix24/handle.go index 9a6f08d3..bd187e1a 100644 --- a/internal/channels/bitrix24/handle.go +++ b/internal/channels/bitrix24/handle.go @@ -145,13 +145,22 @@ func (c *Channel) handleMessage(ctx context.Context, evt *Event) { // without an explicit mention, traffic is dropped so the bot doesn't // spam the customer or interfere with operator handling. if isOpenChannel { - if !c.isMentionedParams(&evt.Params) { - slog.Info("bitrix24 message: dropped OL message without mention", - "from_user_id", evt.Params.FromUserID, - "from_connector", evt.Params.FromIsConnector, - "dialog_id", evt.Params.DialogID, - "message_id", evt.Params.MessageID) - return + // Mention gate is now scoped per-connector: see shouldRequireMentionForOpenline + // for the policy (internal staff always gate; external customer gates only + // for group-style connectors like Zalo personal; 1-to-1 connectors reply + // to every customer message). Connector whitelist is overridable via + // BITRIX24_REQUIRE_MENTION_CONNECTORS env so adding e.g. a future Zalo group + // connector doesn't need a code change. + if shouldRequireMentionForOpenline(evt.Params.FromIsConnector, evt.Params.ChatEntityID) { + if !c.isMentionedParams(&evt.Params) { + slog.Info("bitrix24 message: dropped OL message without mention", + "from_user_id", evt.Params.FromUserID, + "from_connector", evt.Params.FromIsConnector, + "chat_entity_id", evt.Params.ChatEntityID, + "dialog_id", evt.Params.DialogID, + "message_id", evt.Params.MessageID) + return + } } } if isGroup { diff --git a/internal/channels/bitrix24/handle_test.go b/internal/channels/bitrix24/handle_test.go index 53080911..9c171a21 100644 --- a/internal/channels/bitrix24/handle_test.go +++ b/internal/channels/bitrix24/handle_test.go @@ -654,10 +654,13 @@ func TestHandleMessage_Blocked_DoesNotCollectContact(t *testing.T) { // --- Open Channel (MESSAGE_TYPE="L") gating --------------------------------- // TestHandleMessage_OpenChannel_ConnectorWithoutMentionDropped covers the -// customer side of the Open Channel gate without an explicit mention: a -// Zalo/FB customer (IS_CONNECTOR=Y) sending into the session WITHOUT -// @-mentioning the bot must NOT trigger the agent. Humans handle plain -// customer traffic; the bot only steps in when explicitly called. +// customer side of the Open Channel gate on a GROUP-style connector +// (Zalo personal) without an explicit mention: customers chatting amongst +// themselves in the same Open Channel chat must NOT trigger the agent. +// The CHAT_ENTITY_ID prefix is what tells us "this connector is +// group-style and therefore needs the gate" — connectors that don't +// match the whitelist are handled by sibling tests (no gate, every +// message forwards). func TestHandleMessage_OpenChannel_ConnectorWithoutMentionDropped(t *testing.T) { ch, mb := newHandleTestChannel(t, 1058, false) defer resetWebhookRouterForTest() @@ -670,12 +673,42 @@ func TestHandleMessage_OpenChannel_ConnectorWithoutMentionDropped(t *testing.T) MessageID: "m-ol-customer", MessageType: "L", ChatEntityType: "LINES", + ChatEntityID: "synity_zalo_personal|20|grp|960", Message: "Em hỏi giá sản phẩm A", FromIsConnector: true, }, }) if _, ok := drainOne(mb, 100*time.Millisecond); ok { - t.Error("Open Channel connector messages without mention must be dropped") + t.Error("Open Channel connector (whitelisted group-style) messages without mention must be dropped") + } +} + +// TestHandleMessage_OpenChannel_OneToOneConnectorWithoutMentionForwarded +// covers the inverse case: a 1-to-1 connector (e.g. Facebook Messenger, +// Zalo OA) where each customer has their own Open Channel chat. Every +// customer message is addressed to the bot by construction; gating +// would silently drop legitimate customer turns. CHAT_ENTITY_ID prefix +// "facebook" is not in the require-mention whitelist, so the message +// MUST forward without a mention. +func TestHandleMessage_OpenChannel_OneToOneConnectorWithoutMentionForwarded(t *testing.T) { + ch, mb := newHandleTestChannel(t, 1058, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "1074", + DialogID: "chat5208", + MessageID: "m-fb-customer", + MessageType: "L", + ChatEntityType: "LINES", + ChatEntityID: "facebook|34|36305652112415023|1074", + Message: "hello, do you have product X in stock?", + FromIsConnector: true, + }, + }) + if _, ok := drainOne(mb, 200*time.Millisecond); !ok { + t.Error("1-to-1 connector (facebook) customer messages must forward without requiring mention") } } diff --git a/internal/channels/bitrix24/mention_gate.go b/internal/channels/bitrix24/mention_gate.go new file mode 100644 index 00000000..261bb43b --- /dev/null +++ b/internal/channels/bitrix24/mention_gate.go @@ -0,0 +1,112 @@ +package bitrix24 + +import ( + "os" + "strings" + "sync" +) + +// defaultRequireMentionConnectors is the built-in set of Open Channel +// connectors that require a bot @mention before reply. Group-style +// connectors — where multiple external customers share a single Open +// Channel chat — MUST gate, otherwise the bot would spam every customer +// turn (including customer-to-customer chatter that doesn't address the +// bot at all). 1-to-1 connectors (Facebook Messenger, Zalo OA, …) do NOT +// need gating: every customer message is addressed to the bot by +// construction, so requiring a mention there would drop legitimate +// traffic that customers have no syntactic way to produce. +// +// "synity_zalo_personal" is currently the only known group-style Openline +// connector in this deployment — Zalo personal pools several customers +// into one Bitrix Open Channel chat. Operators can extend or override +// the set via the BITRIX24_REQUIRE_MENTION_CONNECTORS env var (see +// requireMentionConnectorSet below). +var defaultRequireMentionConnectors = []string{"synity_zalo_personal"} + +var ( + requireMentionOnce sync.Once + requireMentionSet map[string]bool +) + +// requireMentionConnectorSet returns the connector codes (the leading +// token of CHAT_ENTITY_ID — e.g. "synity_zalo_personal" out of +// "synity_zalo_personal|20|grp|960") that require a bot @mention. +// +// Resolution: env BITRIX24_REQUIRE_MENTION_CONNECTORS overrides the +// hardcoded default. The env value is a comma-separated list of codes; +// empty entries and whitespace are tolerated. An empty env value falls +// back to the default set — set the var to a single comma or a sentinel +// like "none" if you genuinely want to disable gating entirely. +// +// The set is parsed once and cached for the process lifetime. Operators +// who edit the env need to restart the container (same operational +// model as the other BITRIX24_* env vars in docker-compose.yml). +func requireMentionConnectorSet() map[string]bool { + requireMentionOnce.Do(func() { + raw := strings.TrimSpace(os.Getenv("BITRIX24_REQUIRE_MENTION_CONNECTORS")) + set := map[string]bool{} + if raw == "" { + for _, code := range defaultRequireMentionConnectors { + set[code] = true + } + } else { + for _, code := range strings.Split(raw, ",") { + if code = strings.TrimSpace(code); code != "" { + set[code] = true + } + } + } + requireMentionSet = set + }) + return requireMentionSet +} + +// connectorCodeFromEntityID extracts the leading token of CHAT_ENTITY_ID. +// Open Channel payloads ship CHAT_ENTITY_ID as a pipe-separated string +// like "synity_zalo_personal|20|grp|960"; the first segment names the +// connector ("synity_zalo_personal" here). Returns "" when the field is +// empty or missing — callers treat empty as "unknown connector, do not +// gate" so legacy Openline events without an entity id default to the +// non-gated path. +func connectorCodeFromEntityID(entityID string) string { + entityID = strings.TrimSpace(entityID) + if entityID == "" { + return "" + } + if i := strings.IndexByte(entityID, '|'); i >= 0 { + return entityID[:i] + } + return entityID +} + +// shouldRequireMentionForOpenline decides whether an Open Channel inbound +// message must @-mention the bot before the agent picks it up. +// +// Policy (matches reviewed business intent): +// +// - Internal staff in an Openline chat (IS_CONNECTOR=N) → require mention. +// Staff use the operator UI and CAN @-mention; without the gate the bot +// would jump into every operator-to-operator side conversation in the +// same Open Channel session. +// +// - External customer (IS_CONNECTOR=Y) on a group-style connector listed +// in requireMentionConnectorSet → require mention. Customers cannot +// produce a mention from outside (no syntax for it in Zalo), so this is +// effectively "drop traffic that the upstream connector didn't tag as +// bot-addressed". +// +// - External customer on any other connector (FB Messenger, Zalo OA, the +// long tail of 1-to-1 connectors, or any payload without a recognised +// CHAT_ENTITY_ID) → do NOT require mention. Every message is addressed +// to the bot by construction; gating would silently drop legitimate +// customer turns. +func shouldRequireMentionForOpenline(fromConnector bool, chatEntityID string) bool { + if !fromConnector { + return true + } + code := connectorCodeFromEntityID(chatEntityID) + if code == "" { + return false + } + return requireMentionConnectorSet()[code] +} diff --git a/internal/channels/bitrix24/mention_gate_test.go b/internal/channels/bitrix24/mention_gate_test.go new file mode 100644 index 00000000..764ce2cf --- /dev/null +++ b/internal/channels/bitrix24/mention_gate_test.go @@ -0,0 +1,133 @@ +package bitrix24 + +import ( + "sync" + "testing" +) + +// resetRequireMentionCache lets each test observe the env override path — +// the package-level sync.Once otherwise locks in whatever the FIRST caller +// in the process saw. Tests that touch BITRIX24_REQUIRE_MENTION_CONNECTORS +// MUST call this before reading the set, and again at cleanup so a later +// test doesn't inherit a polluted cache. +func resetRequireMentionCache() { + requireMentionOnce = sync.Once{} + requireMentionSet = nil +} + +func TestConnectorCodeFromEntityID(t *testing.T) { + cases := []struct { + in string + want string + }{ + // Real-shape payload — what the Bitrix Open Channel webhook actually + // ships for the Synity Zalo personal connector. + {"synity_zalo_personal|20|grp|960", "synity_zalo_personal"}, + // Hypothetical future Facebook Open Channel payload — verifies the + // helper doesn't require any specific connector name to work. + {"facebook|34|36305652112415023|1074", "facebook"}, + // Token with no pipe at all — return the whole string so the caller + // can still look it up in the whitelist. + {"plain_token", "plain_token"}, + // Surrounding whitespace from sloppy upstream serialization gets + // trimmed before the split — otherwise we'd never match. + {" spaced|x|y ", "spaced"}, + // Empty / pipe-leading cases collapse to "" so the gate falls into + // the "unknown connector → don't gate" branch. + {"", ""}, + {"|leading|pipe", ""}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + if got := connectorCodeFromEntityID(tc.in); got != tc.want { + t.Errorf("connectorCodeFromEntityID(%q) = %q; want %q", tc.in, got, tc.want) + } + }) + } +} + +// TestRequireMentionConnectorSet_DefaultWhenEnvUnset pins the safety +// default: when operators haven't configured BITRIX24_REQUIRE_MENTION_CONNECTORS +// the gate still trips for the one connector we currently know is +// group-style (Zalo personal). Without this default a fresh deployment +// would silently reply to every Zalo customer turn. +func TestRequireMentionConnectorSet_DefaultWhenEnvUnset(t *testing.T) { + resetRequireMentionCache() + defer resetRequireMentionCache() + t.Setenv("BITRIX24_REQUIRE_MENTION_CONNECTORS", "") + + set := requireMentionConnectorSet() + if !set["synity_zalo_personal"] { + t.Error("default set must include synity_zalo_personal") + } + if set["facebook"] { + t.Error("default set must not include facebook (1-to-1 connector)") + } +} + +// TestRequireMentionConnectorSet_EnvOverride is the whole point of the +// env-driven design — operators can add a future group-style connector +// without redeploying the binary. Once "zalo_group" gets shipped this +// is the runtime knob that turns the gate on for it. +func TestRequireMentionConnectorSet_EnvOverride(t *testing.T) { + resetRequireMentionCache() + defer resetRequireMentionCache() + t.Setenv("BITRIX24_REQUIRE_MENTION_CONNECTORS", "synity_zalo_personal, zalo_group , ") + + set := requireMentionConnectorSet() + if !set["synity_zalo_personal"] { + t.Error("override must keep synity_zalo_personal") + } + if !set["zalo_group"] { + t.Error("override must add zalo_group") + } + // Whitespace + trailing empty entry from the comma split must NOT + // register a phantom "" key — that would gate every payload with an + // empty CHAT_ENTITY_ID, which we explicitly want to leave un-gated. + if set[""] { + t.Error("override must not register an empty-string key") + } +} + +// TestShouldRequireMentionForOpenline is the policy matrix in one place. +// Each row corresponds to a decided business case from the spec review, +// so a regression on any row is a regression on a deliberate decision. +func TestShouldRequireMentionForOpenline(t *testing.T) { + resetRequireMentionCache() + defer resetRequireMentionCache() + t.Setenv("BITRIX24_REQUIRE_MENTION_CONNECTORS", "") + + cases := []struct { + name string + fromConnector bool + chatEntityID string + want bool + }{ + // Staff (IS_CONNECTOR=N) ALWAYS gate — they can @-mention and we + // don't want the bot jumping into operator-to-operator chatter. + {"staff in zalo personal still gates", false, "synity_zalo_personal|20|grp|960", true}, + {"staff in facebook still gates", false, "facebook|34|x|960", true}, + {"staff without entity id still gates", false, "", true}, + // Customer (IS_CONNECTOR=Y) on the whitelisted connector → gate. + // Multiple customers share one Zalo personal Open Channel chat, so + // every untagged turn would otherwise spam them. + {"customer on zalo personal gates", true, "synity_zalo_personal|20|grp|960", true}, + // Customer on a 1-to-1 connector → DO NOT gate. Every message is + // addressed to the bot; gating would drop legitimate traffic. + {"customer on facebook does not gate", true, "facebook|34|x|1074", false}, + {"customer on zalo_oa does not gate", true, "zalo_oa|1|x|1", false}, + // Customer with empty CHAT_ENTITY_ID → DO NOT gate. We can't + // distinguish the source, and the safe default for the long tail + // of legacy payloads is "let it through" rather than "silently + // drop and look broken". + {"customer without entity id does not gate", true, "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := shouldRequireMentionForOpenline(tc.fromConnector, tc.chatEntityID); got != tc.want { + t.Errorf("shouldRequireMentionForOpenline(connector=%v, entity=%q) = %v; want %v", + tc.fromConnector, tc.chatEntityID, got, tc.want) + } + }) + } +}