diff --git a/backend/ee/onyx/onyxbot/slack/handlers/handle_standard_answers.py b/backend/ee/onyx/onyxbot/slack/handlers/handle_standard_answers.py index 73d562818b..bb201f3909 100644 --- a/backend/ee/onyx/onyxbot/slack/handlers/handle_standard_answers.py +++ b/backend/ee/onyx/onyxbot/slack/handlers/handle_standard_answers.py @@ -18,7 +18,6 @@ from onyx.db.models import SlackChannelConfig from onyx.db.models import StandardAnswer as StandardAnswerModel from onyx.onyxbot.slack.blocks import get_restate_blocks from onyx.onyxbot.slack.constants import GENERATE_ANSWER_BUTTON_ACTION_ID -from onyx.onyxbot.slack.handlers.utils import send_team_member_message from onyx.onyxbot.slack.models import SlackMessageInfo from onyx.onyxbot.slack.utils import respond_in_thread_or_channel from onyx.onyxbot.slack.utils import update_emote_react @@ -222,14 +221,6 @@ def _handle_standard_answers( unfurl=False, ) - if receiver_ids and slack_thread_id: - send_team_member_message( - client=client, - channel=message_info.channel_to_respond, - thread_ts=slack_thread_id, - receiver_ids=receiver_ids, - ) - return True except Exception as e: logger.exception(f"Unable to send standard answer message: {e}") diff --git a/backend/onyx/onyxbot/slack/handlers/handle_message.py b/backend/onyx/onyxbot/slack/handlers/handle_message.py index a848d83afe..71581cfa35 100644 --- a/backend/onyx/onyxbot/slack/handlers/handle_message.py +++ b/backend/onyx/onyxbot/slack/handlers/handle_message.py @@ -7,6 +7,7 @@ from onyx.configs.onyxbot_configs import ONYX_BOT_FEEDBACK_REMINDER from onyx.configs.onyxbot_configs import ONYX_BOT_REACT_EMOJI from onyx.db.engine.sql_engine import get_session_with_current_tenant from onyx.db.enums import AccountType +from onyx.db.models import ChannelConfig from onyx.db.models import SlackChannelConfig from onyx.db.user_preferences import activate_user from onyx.db.users import add_slack_user_if_not_exists @@ -109,6 +110,26 @@ def remove_scheduled_feedback_reminder( ) +def _resolve_allowlist_user_ids( + channel_conf: ChannelConfig | None, + client: WebClient, +) -> tuple[list[str] | None, list[str]]: + """Resolve `respond_member_group_list` (emails + group names) to Slack user IDs. + + Returns (resolved_user_ids, missing_entries). `resolved_user_ids` is `None` + when no allowlist is configured, meaning the bot has no invocation gate or + response-visibility scope for the channel. + """ + allowlist = (channel_conf or {}).get("respond_member_group_list") or None + if not allowlist: + return None, [] + + user_ids, missing_ids = fetch_slack_user_ids_from_emails(allowlist, client) + group_user_ids, missing = fetch_user_ids_from_groups(missing_ids, client) + resolved = list(set(user_ids + group_user_ids)) + return resolved, missing + + def handle_message( message_info: SlackMessageInfo, slack_channel_config: SlackChannelConfig, @@ -132,6 +153,45 @@ def handle_message( is_slash_command = message_info.is_slash_command is_bot_dm = message_info.is_bot_dm + channel_conf: ChannelConfig | None = ( + slack_channel_config.channel_config + if slack_channel_config and slack_channel_config.channel_config + else None + ) + + # Resolve the allowlist once. Drives both the invocation gate (who can + # trigger the bot) and the response-visibility scope (who sees responses). + # `None` means no allowlist configured -> no gate, no visibility scoping. + allowed_user_ids, missing_allowlist_entries = _resolve_allowlist_user_ids( + channel_conf, client + ) + if missing_allowlist_entries: + logger.warning( + "Failed to find these users/groups in respond_member_group_list: %s", + missing_allowlist_entries, + ) + if allowed_user_ids is not None and not allowed_user_ids: + # Allowlist is configured but every entry failed to resolve — bot is + # silent to everyone until lookups recover. Surface at ERROR so it + # alerts instead of hiding in WARN noise. + logger.error( + "respond_member_group_list is configured but no entries resolved; " + "OnyxBot will be silent in this channel until lookups recover" + ) + + # Invocation gate: drop non-allowlisted senders before emitting telemetry or + # creating a Slack-user account row (which would consume a license seat). + # Applies even to direct @-tags / DMs — this is a license-seat gate, not a + # content filter. + if allowed_user_ids is not None and ( + sender_id is None or sender_id not in allowed_user_ids + ): + logger.info( + "Skipping message: sender %s is not in respond_member_group_list", + sender_id, + ) + return False + action = "slack_message" if is_slash_command: action = "slack_slash_message" @@ -149,11 +209,8 @@ def handle_message( ] respond_tag_only = False - respond_member_group_list = None - channel_conf = None - if slack_channel_config and slack_channel_config.channel_config: - channel_conf = slack_channel_config.channel_config + if channel_conf: if not bypass_filters and "answer_filters" in channel_conf: if ( "questionmark_prefilter" in channel_conf["answer_filters"] @@ -171,7 +228,6 @@ def handle_message( ) respond_tag_only = channel_conf.get("respond_tag_only") or False - respond_member_group_list = channel_conf.get("respond_member_group_list", None) # Only default config can be disabled. # If channel config is disabled, bot should not respond to this message (including DMs) @@ -184,19 +240,8 @@ def handle_message( logger.info("Skipping message: OnyxBot only responds to tags in this channel") return False - # List of user id to send message to, if None, send to everyone in channel - send_to: list[str] | None = None - missing_users: list[str] | None = None - if respond_member_group_list: - send_to, missing_ids = fetch_slack_user_ids_from_emails( - respond_member_group_list, client - ) - - user_ids, missing_users = fetch_user_ids_from_groups(missing_ids, client) - send_to = list(set(send_to + user_ids)) if send_to else user_ids - - if missing_users: - logger.warning(f"Failed to find these users/groups: {missing_users}") + # Reuses the resolved allowlist as the ephemeral response-visibility scope. + send_to: list[str] | None = allowed_user_ids # If configured to respond to team members only, then cannot be used with a /OnyxBot command # which would just respond to the sender diff --git a/backend/onyx/onyxbot/slack/handlers/handle_regular_answer.py b/backend/onyx/onyxbot/slack/handlers/handle_regular_answer.py index b9a5208318..4790e657e0 100644 --- a/backend/onyx/onyxbot/slack/handlers/handle_regular_answer.py +++ b/backend/onyx/onyxbot/slack/handlers/handle_regular_answer.py @@ -26,7 +26,6 @@ from onyx.db.persona import get_persona_by_id from onyx.db.users import get_user_by_email from onyx.onyxbot.slack.blocks import build_slack_response_blocks from onyx.onyxbot.slack.constants import SLACK_CHANNEL_REF_PATTERN -from onyx.onyxbot.slack.handlers.utils import send_team_member_message from onyx.onyxbot.slack.models import SlackMessageInfo from onyx.onyxbot.slack.models import ThreadMessage from onyx.onyxbot.slack.utils import get_channel_from_id @@ -412,24 +411,6 @@ def handle_regular_answer( send_as_ephemeral=send_as_ephemeral, ) - # For DM (ephemeral message), we need to create a thread via a normal message so the user can see - # the ephemeral message. This also will give the user a notification which ephemeral message does not. - # if there is no message_ts_to_respond_to, and we have made it this far, then this is a /onyx message - # so we shouldn't send_team_member_message - if ( - target_receiver_ids - and message_ts_to_respond_to is not None - and not send_as_ephemeral - and target_thread_ts is not None - ): - send_team_member_message( - client=client, - channel=channel, - thread_ts=target_thread_ts, - receiver_ids=target_receiver_ids, - send_as_ephemeral=send_as_ephemeral, - ) - return False except Exception: diff --git a/backend/onyx/onyxbot/slack/handlers/utils.py b/backend/onyx/onyxbot/slack/handlers/utils.py deleted file mode 100644 index 8fcb8aa3ab..0000000000 --- a/backend/onyx/onyxbot/slack/handlers/utils.py +++ /dev/null @@ -1,23 +0,0 @@ -from slack_sdk import WebClient - -from onyx.onyxbot.slack.utils import respond_in_thread_or_channel - - -def send_team_member_message( - client: WebClient, - channel: str, - thread_ts: str, - receiver_ids: list[str] | None = None, # noqa: ARG001 - send_as_ephemeral: bool = False, -) -> None: - respond_in_thread_or_channel( - client=client, - channel=channel, - text=( - "👋 Hi, we've just gathered and forwarded the relevant " - + "information to the team. They'll get back to you shortly!" - ), - thread_ts=thread_ts, - receiver_ids=None, - send_as_ephemeral=send_as_ephemeral, - ) diff --git a/backend/onyx/server/manage/slack_bot.py b/backend/onyx/server/manage/slack_bot.py index 21f01078d2..cae386a59b 100644 --- a/backend/onyx/server/manage/slack_bot.py +++ b/backend/onyx/server/manage/slack_bot.py @@ -69,11 +69,6 @@ def _form_channel_config( detail=str(e), ) - if respond_tag_only and respond_member_group_list: - raise ValueError( - "Cannot set OnyxBot to only respond to tags only and also respond to a predetermined set of users." - ) - if ( slack_channel_config_creation_request.is_ephemeral and slack_channel_config_creation_request.respond_member_group_list diff --git a/backend/tests/unit/onyx/onyxbot/test_slack_gating.py b/backend/tests/unit/onyx/onyxbot/test_slack_gating.py index 4da98e84b1..218638d2ee 100644 --- a/backend/tests/unit/onyx/onyxbot/test_slack_gating.py +++ b/backend/tests/unit/onyx/onyxbot/test_slack_gating.py @@ -418,3 +418,176 @@ class TestGetUsedSeats: assert get_used_seats() == 3 mock_session.execute.assert_called_once() + + +# --------------------------------------------------------------------------- +# handle_message invocation allowlist (respond_member_group_list as gate) +# --------------------------------------------------------------------------- + + +class TestHandleMessageInvocationAllowlist: + """The `respond_member_group_list` field gates bot invocation: non-allowlisted + senders are dropped before telemetry fires or a Slack-user account row is + created (no license seat consumed). + """ + + @pytest.fixture(autouse=True) + def _common_patches(self) -> Any: + with ( + patch(f"{_HANDLE_MSG}.send_msg_ack_to_user"), + patch(f"{_HANDLE_MSG}.handle_regular_answer", return_value=False), + patch(f"{_HANDLE_MSG}.handle_standard_answers", return_value=False), + patch(f"{_HANDLE_MSG}.add_slack_user_if_not_exists"), + patch(f"{_HANDLE_MSG}.get_user_by_email") as mock_get_user, + patch(f"{_HANDLE_MSG}.fetch_ee_implementation_or_noop") as mock_fetch_ee, + ): + mock_get_user.return_value = MagicMock() + mock_fetch_ee.return_value = lambda **_kw: MagicMock(available=True) + yield + + @pytest.fixture + def db_session(self) -> Generator[MagicMock, None, None]: + with patch(f"{_HANDLE_MSG}.get_session_with_current_tenant") as mock: + session = MagicMock() + mock.return_value.__enter__ = MagicMock(return_value=session) + mock.return_value.__exit__ = MagicMock(return_value=False) + yield session + + def _make_config( + self, allowlist: list[str] | None = None, **extra: Any + ) -> MagicMock: + config = MagicMock() + config.persona = None + channel_config: dict[str, Any] = {**extra} + if allowlist is not None: + channel_config["respond_member_group_list"] = allowlist + config.channel_config = channel_config + return config + + def _call( + self, + slack_channel_config: MagicMock, + client: MagicMock | None = None, + ) -> bool: + from onyx.onyxbot.slack.handlers.handle_message import handle_message + + return handle_message( + message_info=_make_message_info(), + slack_channel_config=slack_channel_config, + client=client or MagicMock(), + feedback_reminder_id=None, + ) + + @pytest.mark.usefixtures("db_session") + @patch(f"{_HANDLE_MSG}.slack_usage_report") + def test_empty_allowlist_processes_normally( + self, mock_usage_report: MagicMock + ) -> None: + self._call(self._make_config(allowlist=None)) + mock_usage_report.assert_called_once() + + @pytest.mark.usefixtures("db_session") + @patch(f"{_HANDLE_MSG}.fetch_user_ids_from_groups", return_value=([], [])) + @patch(f"{_HANDLE_MSG}.fetch_slack_user_ids_from_emails") + @patch(f"{_HANDLE_MSG}.slack_usage_report") + def test_sender_in_allowlist_by_email_processes( + self, + mock_usage_report: MagicMock, + mock_fetch_emails: MagicMock, + _mock_fetch_groups: MagicMock, + ) -> None: + # _make_message_info() returns sender_id="U123" + mock_fetch_emails.return_value = (["U123"], []) + + self._call(self._make_config(allowlist=["allowed@test.com"])) + + mock_usage_report.assert_called_once() + + @pytest.mark.usefixtures("db_session") + @patch(f"{_HANDLE_MSG}.fetch_user_ids_from_groups") + @patch(f"{_HANDLE_MSG}.fetch_slack_user_ids_from_emails") + @patch(f"{_HANDLE_MSG}.slack_usage_report") + def test_sender_in_allowlist_by_group_processes( + self, + mock_usage_report: MagicMock, + mock_fetch_emails: MagicMock, + mock_fetch_groups: MagicMock, + ) -> None: + # Email lookup fails for the group name, group lookup resolves it to U123 + mock_fetch_emails.return_value = ([], ["sales-team"]) + mock_fetch_groups.return_value = (["U123"], []) + + self._call(self._make_config(allowlist=["sales-team"])) + + mock_usage_report.assert_called_once() + + @pytest.mark.usefixtures("db_session") + @patch(f"{_HANDLE_MSG}.add_slack_user_if_not_exists") + @patch(f"{_HANDLE_MSG}.fetch_user_ids_from_groups", return_value=([], [])) + @patch(f"{_HANDLE_MSG}.fetch_slack_user_ids_from_emails") + @patch(f"{_HANDLE_MSG}.slack_usage_report") + def test_sender_not_in_allowlist_is_dropped( + self, + mock_usage_report: MagicMock, + mock_fetch_emails: MagicMock, + _mock_fetch_groups: MagicMock, + mock_add_user: MagicMock, + ) -> None: + # Allowlist resolves to a different user ID than the sender + mock_fetch_emails.return_value = (["U999"], []) + + result = self._call(self._make_config(allowlist=["allowed@test.com"])) + + assert result is False + mock_usage_report.assert_not_called() + mock_add_user.assert_not_called() + + @pytest.mark.usefixtures("db_session") + @patch(f"{_HANDLE_MSG}.add_slack_user_if_not_exists") + @patch(f"{_HANDLE_MSG}.fetch_user_ids_from_groups", return_value=([], [])) + @patch(f"{_HANDLE_MSG}.fetch_slack_user_ids_from_emails") + @patch(f"{_HANDLE_MSG}.slack_usage_report") + def test_unresolved_allowlist_entries_deny_invocation( + self, + mock_usage_report: MagicMock, + mock_fetch_emails: MagicMock, + _mock_fetch_groups: MagicMock, + mock_add_user: MagicMock, + ) -> None: + # Email never resolves to any Slack user; allowlist non-empty -> deny + mock_fetch_emails.return_value = ([], ["unknown@test.com"]) + + result = self._call(self._make_config(allowlist=["unknown@test.com"])) + + assert result is False + mock_usage_report.assert_not_called() + mock_add_user.assert_not_called() + + @pytest.mark.usefixtures("db_session") + @patch(f"{_HANDLE_MSG}.add_slack_user_if_not_exists") + @patch(f"{_HANDLE_MSG}.fetch_user_ids_from_groups", return_value=([], [])) + @patch(f"{_HANDLE_MSG}.fetch_slack_user_ids_from_emails") + @patch(f"{_HANDLE_MSG}.slack_usage_report") + def test_missing_sender_id_with_allowlist_is_dropped( + self, + mock_usage_report: MagicMock, + mock_fetch_emails: MagicMock, + _mock_fetch_groups: MagicMock, + mock_add_user: MagicMock, + ) -> None: + mock_fetch_emails.return_value = (["U123"], []) + + info = _make_message_info() + info.sender_id = None + from onyx.onyxbot.slack.handlers.handle_message import handle_message + + result = handle_message( + message_info=info, + slack_channel_config=self._make_config(allowlist=["allowed@test.com"]), + client=MagicMock(), + feedback_reminder_id=None, + ) + + assert result is False + mock_usage_report.assert_not_called() + mock_add_user.assert_not_called() diff --git a/web/src/app/admin/bots/[bot-id]/channels/SlackChannelConfigFormFields.tsx b/web/src/app/admin/bots/[bot-id]/channels/SlackChannelConfigFormFields.tsx index dce0be2e3b..a842356e2e 100644 --- a/web/src/app/admin/bots/[bot-id]/channels/SlackChannelConfigFormFields.tsx +++ b/web/src/app/admin/bots/[bot-id]/channels/SlackChannelConfigFormFields.tsx @@ -544,11 +544,18 @@ export function SlackChannelConfigFormFields({ name="respond_member_group_list" label="(Optional) Respond to Certain Users / Groups" subtext={ - "If specified, OnyxBot responses will only " + - "be visible to the members or groups in this list." + "If specified, only these users / groups can invoke " + + "OnyxBot in this channel, and responses are visible only " + + "to them." } values={values} placeholder="User email or user group name..." + disabled={values.is_ephemeral} + tooltip={ + values.is_ephemeral + ? "Disabled while 'Respond to user in a private (ephemeral) message' is on — ephemeral responses target a single user only." + : undefined + } /> tag !== ""), ...(creationRequest.usePersona