mirror of
https://github.com/onyx-dot-app/onyx.git
synced 2026-10-23 03:42:59 -05:00
feat(slack-bot): gate invocation by respond_member_group_list allowlist (#10992) to release v3.3 (#11012)
This commit is contained in:
1 parent
e2c6299df8
commit
cea440552e
8 files changed
+248
-77
No files matched your search
@@ -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}")
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
@@ -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
|
||||
}
|
||||
/>
|
||||
|
||||
<StandardAnswerCategoryDropdownField
|
||||
|
||||
@@ -45,7 +45,9 @@ const buildRequestBodyFromCreationRequest = (
|
||||
is_ephemeral: creationRequest.is_ephemeral,
|
||||
show_continue_in_web_ui: creationRequest.show_continue_in_web_ui,
|
||||
enable_auto_filters: creationRequest.enable_auto_filters,
|
||||
respond_member_group_list: creationRequest.respond_member_group_list,
|
||||
respond_member_group_list: creationRequest.is_ephemeral
|
||||
? []
|
||||
: creationRequest.respond_member_group_list,
|
||||
answer_filters: buildFiltersFromCreationRequest(creationRequest),
|
||||
follow_up_tags: creationRequest.follow_up_tags?.filter((tag) => tag !== ""),
|
||||
...(creationRequest.usePersona
|
||||
|
||||
Reference in new issue
Block a user