First migration of an existing built-in platform adapter to the plugin system established by IRC / Teams / LINE / Google Chat. Closes #24325; advances the umbrella refactor in #3823. Matches Teams' shape exactly — adapter under ``plugins/platforms/discord/`` with the standard ``__init__.py`` / ``adapter.py`` / ``plugin.yaml`` shell, ``register(ctx)`` entry point, **no back-compat shim** at the old import path, and full parity for the four hooks Teams uses plus the ``apply_yaml_config_fn`` hook that landed in #25443 (the Discord plugin is the first consumer of that hook): * ``standalone_sender_fn`` — out-of-process cron delivery via REST API * ``setup_fn`` — interactive ``hermes setup gateway`` wizard * ``apply_yaml_config_fn`` — translate ``config.yaml`` ``discord:`` keys into ``DISCORD_*`` env vars (replaces the hardcoded block in ``gateway/config.py``) * ``is_connected`` — declares connection state from ``DISCORD_BOT_TOKEN`` * ``check_fn`` — lazy-installs ``discord.py`` on demand * plus ``allowed_users_env``, ``allow_all_env``, ``cron_deliver_env_var``, ``max_message_length``, ``emoji``, ``required_env``, ``install_hint`` * ``gateway/platforms/discord.py`` (5,101 LOC) → ``plugins/platforms/discord/adapter.py`` (git rename, R090). * New ``plugins/platforms/discord/{__init__.py, plugin.yaml}`` with ``requires_env`` / ``optional_env`` declarations. * Append ``register(ctx)`` block + new hook implementations (``_standalone_send``, ``interactive_setup``, ``_apply_yaml_config``, ``_clean_discord_user_ids``, ``_is_connected``, ``_build_adapter``, plus helpers ``_DISCORD_CHANNEL_TYPE_PROBE_CACHE`` etc.) to the adapter. * Replace the ``Platform.DISCORD elif`` branch in ``GatewayRunner._create_adapter()`` (−9 LOC) with a generic post-creation hook (+6 LOC) in the registry path: any plugin adapter that declares a ``gateway_runner`` attribute now gets it auto-injected. Webhook's built-in branch is unchanged (it doesn't go through the registry path). * Move ``_send_discord`` (190 LOC) and helpers (``_DISCORD_CHANNEL_TYPE_PROBE_CACHE``, ``_remember_channel_is_forum``, ``_probe_is_forum_cached``, ``_derive_forum_thread_name``) from ``tools/send_message_tool.py`` into the plugin as ``_standalone_send``. * Wire via ``standalone_sender_fn=_standalone_send`` (Teams pattern; same gap fixed in #21804 for other plugin platforms). * Replace the Discord ``elif`` in ``tools/send_message_tool.py`` ``_send_to_platform`` with a 10-line registry-hook dispatch. * Drop the ``DiscordAdapter`` import and the ``Platform.DISCORD: DiscordAdapter.MAX_MESSAGE_LENGTH`` ``_MAX_LENGTHS`` entry — the registry's ``max_message_length=2000`` covers it. * Move ``_setup_discord`` and ``_clean_discord_user_ids`` (68 LOC) from ``hermes_cli/setup.py`` into the plugin as ``interactive_setup``. * Wire via ``setup_fn=interactive_setup``. CLI helpers (``prompt``, ``print_info``, etc.) are lazy-imported so the plugin's module-load surface stays minimal. * Remove ``"discord": _s._setup_discord`` from ``hermes_cli/gateway.py::_builtin_setup_fn``. * Remove the entire 32-line ``_PLATFORMS["discord"]`` static dict entry — Discord's setup metadata is now discovered dynamically via ``_all_platforms()`` from the registry entry. * Move the 59-line ``discord_cfg`` YAML→env bridge from ``gateway/config.py::load_gateway_config()`` into the plugin as ``_apply_yaml_config``. Covers ``require_mention``, ``thread_require_mention``, ``free_response_channels``, ``auto_thread``, ``reactions``, ``ignored_channels``, ``allowed_channels``, ``no_thread_channels``, ``allow_mentions.{everyone,roles,users, replied_user}``, and ``reply_to_mode`` (including the YAML 1.1 ``off``-as-False coercion and the ``extra.reply_to_mode`` fallback). * Wire via ``apply_yaml_config_fn=_apply_yaml_config``. * The hook runs BEFORE ``_apply_env_overrides`` and after the generic shared-key loop, exactly as documented in ``website/docs/developer-guide/adding-platform-adapters.md``. * Behavior is preserved exactly — every assignment still uses ``not os.getenv(...)`` guards so env vars take precedence over YAML. All 78 references to the old import path are rewritten — no back-compat shim: * 51 ``from gateway.platforms.discord import X`` → ``from plugins.platforms.discord.adapter import X`` * 5 ``import gateway.platforms.discord as discord_platform`` → ``import plugins.platforms.discord.adapter as discord_platform`` * 1 ``from gateway.platforms import discord as discord_mod`` → ``from plugins.platforms.discord import adapter as discord_mod`` * 21 ``mock.patch("gateway.platforms.discord.X")`` strings → ``mock.patch("plugins.platforms.discord.adapter.X")`` * 1 docstring reference in ``hermes_cli/commands.py`` * 1 import in ``tools/send_message_tool.py`` (now removed entirely) The import-safety test in ``tests/gateway/test_discord_imports.py`` is updated to purge the new canonical module name from ``sys.modules``. **38 files changed, +621 / −473** — net positive due to the YAML hook implementation (89 new LOC in the plugin trading for 59 deleted in core), but every line moved has a clear plugin home now. The git rename is detected at R090 because the adapter gained ~340 LOC of moved-in hook implementations (``_standalone_send`` + ``interactive_setup`` + ``_apply_yaml_config`` + helpers). * All 568 Discord-specific tests pass across 25 ``test_discord_*.py`` files plus voice/send/text-batching/reload-skills/stream-consumer/ integration tests. * All 147 tests in the YAML-touching subset (``test_discord_reply_mode``, ``test_discord_free_response``, ``test_discord_allowed_channels``, ``test_discord_allowed_mentions``, ``test_discord_channel_controls``, ``test_discord_reactions``, ``test_discord_thread_persistence``, ``test_runtime_footer``) pass — this is the strongest signal that the YAML→env hook behaves identically to the legacy block. * Broader gateway/cron/integration sweep (1297 tests) introduces zero new failures vs ``main``. Pre-existing failures in ``tests/gateway/test_tts_media_routing.py`` and ``tests/e2e/test_platform_commands.py`` reproduce identically on the unchanged ``main`` revision. * Plugin discovery sanity check confirms Discord registers alongside the other four platform plugins: Registered platforms: ['discord', 'google_chat', 'irc', 'line', 'teams'] These Discord-shaped tendrils in core were **deliberately not moved** — they are generic platform-registry concerns affecting every platform, not Discord-specific: * ``gateway/config.py:1205`` ``DISCORD_BOT_TOKEN → config.token`` env enablement — same shape Telegram has. The existing ``env_enablement_fn`` registry hook only seeds ``extra``, not ``.token``, so it can't replace this without an adapter refactor to read from ``extra["bot_token"]``. * ``gateway/run.py`` voice-mode hooks (``self.adapters.get(Platform.DISCORD)`` for ``start_voice_mode``/``stop_voice_mode``), role-based auth, ``DISCORD_ALLOW_BOTS`` branch in ``_is_user_authorized``, ``_UPDATE_ALLOWED_PLATFORMS`` frozenset, and the per-platform allowlist maps — generic platform-registry concerns. * ``Platform.DISCORD`` enum literal — stable identifier used as dict keys throughout the codebase; removing it is a separate refactor with no real benefit. * ``tools/discord_tool.py`` and ``tools/environments/local.py`` — first-class agent tools and env-passthrough config, neither is the gateway adapter. Each of these is worth its own scoping issue when the time comes.
231 lines
8.9 KiB
Python
231 lines
8.9 KiB
Python
"""Security regression tests: Discord component views honor role allowlists.
|
|
|
|
The four interactive component views (ExecApprovalView, SlashConfirmView,
|
|
UpdatePromptView, ModelPickerView) historically accepted only
|
|
``allowed_user_ids``. Deployments that configure DISCORD_ALLOWED_ROLES
|
|
without DISCORD_ALLOWED_USERS therefore had a wide-open component
|
|
surface: any guild member who could see the prompt could approve exec
|
|
commands, cancel slash confirmations, or switch the model -- even when
|
|
the same user would be rejected at the slash and on_message gates.
|
|
|
|
These tests pin the user-or-role OR semantics and the fail-closed
|
|
behavior on missing role data so the parity cannot regress.
|
|
"""
|
|
|
|
from types import SimpleNamespace
|
|
|
|
import pytest
|
|
|
|
# Trigger the shared discord mock from tests/gateway/conftest.py before
|
|
# importing the production module.
|
|
from plugins.platforms.discord.adapter import ( # noqa: E402
|
|
ExecApprovalView,
|
|
ModelPickerView,
|
|
SlashConfirmView,
|
|
UpdatePromptView,
|
|
_component_check_auth,
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Direct helper coverage -- the four views all delegate to this helper, so
|
|
# pinning the helper's contract pins all four call sites.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _interaction(user_id, role_ids=None, *, drop_user=False, drop_roles=False):
|
|
"""Build a mock interaction with the requested user/role shape.
|
|
|
|
drop_user simulates a payload whose .user attribute is None.
|
|
drop_roles simulates a payload where .user has no .roles attribute
|
|
at all (DM-context Member, raw User payload).
|
|
"""
|
|
if drop_user:
|
|
return SimpleNamespace(user=None)
|
|
|
|
user_kwargs = {"id": user_id}
|
|
if not drop_roles:
|
|
user_kwargs["roles"] = [SimpleNamespace(id=r) for r in (role_ids or [])]
|
|
return SimpleNamespace(user=SimpleNamespace(**user_kwargs))
|
|
|
|
|
|
# ── back-compat: empty allowlists -> allow everyone ────────────────────────
|
|
|
|
|
|
def test_component_check_empty_allowlists_allows_everyone():
|
|
"""SECURITY-CRITICAL backwards-compat: deployments without any
|
|
DISCORD_ALLOWED_* env vars set must continue to allow component
|
|
interactions from anyone (no regression for unconfigured setups)."""
|
|
interaction = _interaction(11111)
|
|
assert _component_check_auth(interaction, set(), set()) is True
|
|
assert _component_check_auth(interaction, None, None) is True
|
|
|
|
|
|
# ── user allowlist ─────────────────────────────────────────────────────────
|
|
|
|
|
|
def test_component_check_user_in_user_allowlist_passes():
|
|
interaction = _interaction(11111)
|
|
assert _component_check_auth(interaction, {"11111"}, set()) is True
|
|
|
|
|
|
def test_component_check_user_not_in_user_allowlist_rejected():
|
|
interaction = _interaction(99999)
|
|
assert _component_check_auth(interaction, {"11111"}, set()) is False
|
|
|
|
|
|
# ── role allowlist OR semantics ────────────────────────────────────────────
|
|
|
|
|
|
def test_component_check_role_only_user_with_matching_role_passes():
|
|
"""Role-only deployment (DISCORD_ALLOWED_ROLES set, DISCORD_ALLOWED_USERS
|
|
empty) where the user is not in the empty user list but DOES carry a
|
|
matching role: must pass. This is the regression that prompted the
|
|
fix -- previously _check_auth allowed everyone when the user set was
|
|
empty, ignoring the role allowlist."""
|
|
interaction = _interaction(99999, role_ids=[42])
|
|
assert _component_check_auth(interaction, set(), {42}) is True
|
|
|
|
|
|
def test_component_check_role_only_user_without_matching_role_rejected():
|
|
"""Role-only deployment where the user has no matching role: reject.
|
|
Previously this allowed everyone because allowed_user_ids was empty."""
|
|
interaction = _interaction(99999, role_ids=[7, 8])
|
|
assert _component_check_auth(interaction, set(), {42}) is False
|
|
|
|
|
|
def test_component_check_user_or_role_user_match():
|
|
"""Both allowlists set; user matches user allowlist: pass."""
|
|
interaction = _interaction(11111, role_ids=[7])
|
|
assert _component_check_auth(interaction, {"11111"}, {42}) is True
|
|
|
|
|
|
def test_component_check_user_or_role_role_match():
|
|
"""Both allowlists set; user not in user list but in role list: pass."""
|
|
interaction = _interaction(99999, role_ids=[42])
|
|
assert _component_check_auth(interaction, {"11111"}, {42}) is True
|
|
|
|
|
|
def test_component_check_user_or_role_neither_match():
|
|
"""Both allowlists set; user matches neither: reject."""
|
|
interaction = _interaction(99999, role_ids=[7])
|
|
assert _component_check_auth(interaction, {"11111"}, {42}) is False
|
|
|
|
|
|
# ── fail-closed on missing role data ───────────────────────────────────────
|
|
|
|
|
|
def test_component_check_role_policy_with_no_roles_attr_rejects():
|
|
"""Role allowlist configured but interaction.user has no .roles
|
|
attribute (DM-context Member, raw User payload): must reject. A user
|
|
without resolvable roles cannot satisfy a role allowlist."""
|
|
interaction = _interaction(11111, drop_roles=True)
|
|
assert _component_check_auth(interaction, set(), {42}) is False
|
|
|
|
|
|
def test_component_check_missing_user_with_allowlist_rejects():
|
|
"""interaction.user is None with any allowlist configured: fail
|
|
closed without raising AttributeError."""
|
|
interaction = _interaction(0, drop_user=True)
|
|
assert _component_check_auth(interaction, {"11111"}, set()) is False
|
|
assert _component_check_auth(interaction, set(), {42}) is False
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# View construction: every view must accept allowed_role_ids and route
|
|
# through the shared helper. Default value preserves prior call-sites.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_exec_approval_view_accepts_role_allowlist():
|
|
view = ExecApprovalView(
|
|
session_key="sess-1",
|
|
allowed_user_ids={"11111"},
|
|
allowed_role_ids={42},
|
|
)
|
|
# Role-only user passes
|
|
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
|
# Neither user nor role match: reject
|
|
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
|
|
|
|
|
def test_exec_approval_view_role_default_is_empty_set():
|
|
"""Existing call sites that pass only allowed_user_ids must continue
|
|
working with the legacy semantics (no role gate)."""
|
|
view = ExecApprovalView(session_key="sess-1", allowed_user_ids={"11111"})
|
|
assert view.allowed_role_ids == set()
|
|
assert view._check_auth(_interaction(11111)) is True
|
|
assert view._check_auth(_interaction(99999)) is False
|
|
|
|
|
|
def test_slash_confirm_view_accepts_role_allowlist():
|
|
view = SlashConfirmView(
|
|
session_key="sess-1",
|
|
confirm_id="c1",
|
|
allowed_user_ids=set(),
|
|
allowed_role_ids={42},
|
|
)
|
|
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
|
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
|
|
|
|
|
def test_update_prompt_view_accepts_role_allowlist():
|
|
view = UpdatePromptView(
|
|
session_key="sess-1",
|
|
allowed_user_ids=set(),
|
|
allowed_role_ids={42},
|
|
)
|
|
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
|
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
|
|
|
|
|
def test_model_picker_view_accepts_role_allowlist():
|
|
async def _noop(*_a, **_k):
|
|
return ""
|
|
|
|
view = ModelPickerView(
|
|
providers=[],
|
|
current_model="m",
|
|
current_provider="p",
|
|
session_key="sess-1",
|
|
on_model_selected=_noop,
|
|
allowed_user_ids=set(),
|
|
allowed_role_ids={42},
|
|
)
|
|
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
|
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Empty allowlists across views: legacy "allow everyone" must hold.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"view_factory",
|
|
[
|
|
lambda: ExecApprovalView(session_key="s", allowed_user_ids=set()),
|
|
lambda: SlashConfirmView(session_key="s", confirm_id="c", allowed_user_ids=set()),
|
|
lambda: UpdatePromptView(session_key="s", allowed_user_ids=set()),
|
|
],
|
|
)
|
|
def test_views_empty_allowlists_allow_everyone(view_factory):
|
|
view = view_factory()
|
|
assert view._check_auth(_interaction(99999)) is True
|
|
|
|
|
|
def test_model_picker_view_empty_allowlists_allow_everyone():
|
|
async def _noop(*_a, **_k):
|
|
return ""
|
|
|
|
view = ModelPickerView(
|
|
providers=[],
|
|
current_model="m",
|
|
current_provider="p",
|
|
session_key="s",
|
|
on_model_selected=_noop,
|
|
allowed_user_ids=set(),
|
|
)
|
|
assert view.allowed_role_ids == set()
|
|
assert view._check_auth(_interaction(99999)) is True
|