fix(gateway): trust adapter-owned access policy over env default-deny (#34515)
Config-driven platform policies (dm_policy / group_policy / allow_from / group_allow_from) for WeCom, Weixin, Yuanbao, and QQBot now work without also setting a PLATFORM_ALLOWED_USERS env var. These adapters enforce their access policy at intake — a message is dropped inside the adapter and never dispatched unless it already passed the policy. The gateway's env-based check (_is_user_authorized) ran afterward and, with no env allowlist set, fell through to an env-only default-deny — silently rejecting `dm_policy: open` and config-only allowlists the adapter had already authorized. Rather than re-implement each adapter's policy a second time in run.py (which would drift), adapters that own their gate now declare it via a new BasePlatformAdapter.enforces_own_access_policy property (default False). The gateway trusts that flag and skips the env-only default-deny for those platforms. Env allowlists still take precedence when set. Also resolves unauthorized DM behavior from config dm_policy so allowlist / disabled policies drop unauthorized DMs silently instead of leaking pairing codes, while an explicit pairing policy opts back in. Co-authored-by: Frowtek <frowte3k@gmail.com>
This commit is contained in:
teknium1
@ -1655,6 +1655,29 @@ class BasePlatformAdapter(ABC):
|
||||
"""
|
||||
return len
|
||||
|
||||
@property
|
||||
def enforces_own_access_policy(self) -> bool:
|
||||
"""Whether this adapter gates inbound access before dispatch.
|
||||
|
||||
Some adapters (WeCom, Weixin, Yuanbao, QQBot) implement a documented
|
||||
config-driven access surface — ``dm_policy`` / ``group_policy`` /
|
||||
``allow_from`` / ``group_allow_from`` in ``PlatformConfig.extra`` — and
|
||||
enforce it at intake: a message is dropped inside the adapter and never
|
||||
reaches the gateway unless it already passed that policy.
|
||||
|
||||
The gateway's env-based allowlist check runs *after* the adapter, so for
|
||||
these platforms a message arriving at ``_is_user_authorized`` has, by
|
||||
definition, already been authorized by the adapter. Without this flag the
|
||||
gateway would then deny it again (no env allowlist → default deny),
|
||||
silently breaking ``dm_policy: open`` and config-only allowlists.
|
||||
|
||||
Adapters that own their access policy override this to return ``True``.
|
||||
The gateway treats that as "already authorized at intake" and skips the
|
||||
env-allowlist default-deny. Adapters that delegate access control to the
|
||||
gateway leave it ``False`` (the default).
|
||||
"""
|
||||
return False
|
||||
|
||||
def supports_draft_streaming(
|
||||
self,
|
||||
chat_type: Optional[str] = None,
|
||||
|
||||
@ -269,6 +269,11 @@ class QQAdapter(BasePlatformAdapter):
|
||||
def name(self) -> str:
|
||||
return "QQBot"
|
||||
|
||||
@property
|
||||
def enforces_own_access_policy(self) -> bool:
|
||||
"""QQBot gates DM/group access at intake via dm_policy/group_policy."""
|
||||
return True
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Connection lifecycle
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@ -847,6 +847,11 @@ class WeComAdapter(BasePlatformAdapter):
|
||||
# Policy helpers
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@property
|
||||
def enforces_own_access_policy(self) -> bool:
|
||||
"""WeCom gates DM/group access at intake via dm_policy/group_policy."""
|
||||
return True
|
||||
|
||||
def _is_dm_allowed(self, sender_id: str) -> bool:
|
||||
if self._dm_policy == "disabled":
|
||||
return False
|
||||
|
||||
@ -1397,6 +1397,11 @@ class WeixinAdapter(BasePlatformAdapter):
|
||||
logger.info("[%s] inbound from=%s type=%s media=%d", self.name, _safe_id(sender_id), source.chat_type, len(media_paths))
|
||||
await self.handle_message(event)
|
||||
|
||||
@property
|
||||
def enforces_own_access_policy(self) -> bool:
|
||||
"""Weixin gates DM/group access at intake via dm_policy/group_policy."""
|
||||
return True
|
||||
|
||||
def _is_dm_allowed(self, sender_id: str) -> bool:
|
||||
if self._dm_policy == "disabled":
|
||||
return False
|
||||
|
||||
@ -4691,6 +4691,11 @@ class YuanbaoAdapter(BasePlatformAdapter):
|
||||
# Abstract method implementations
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@property
|
||||
def enforces_own_access_policy(self) -> bool:
|
||||
"""Yuanbao gates DM/group access at intake via dm_policy/group_policy."""
|
||||
return True
|
||||
|
||||
async def connect(self) -> bool:
|
||||
"""Connect to Yuanbao WS gateway and authenticate.
|
||||
|
||||
|
||||
@ -6542,6 +6542,25 @@ class GatewayRunner:
|
||||
return YuanbaoAdapter(config)
|
||||
|
||||
return None
|
||||
|
||||
def _adapter_enforces_own_access_policy(self, platform: Optional[Platform]) -> bool:
|
||||
"""Whether the adapter for *platform* gates access at intake itself.
|
||||
|
||||
Mirrors ``BasePlatformAdapter.enforces_own_access_policy``. Adapters
|
||||
such as WeCom, Weixin, Yuanbao, and QQBot evaluate their documented
|
||||
``dm_policy`` / ``group_policy`` / ``allow_from`` config before a
|
||||
message is dispatched to the gateway, so a message that reaches
|
||||
``_is_user_authorized`` has already been authorized by the adapter.
|
||||
Defaults to ``False`` when the adapter is unknown or doesn't expose
|
||||
the flag.
|
||||
"""
|
||||
if not platform:
|
||||
return False
|
||||
adapter = self.adapters.get(platform)
|
||||
if adapter is None:
|
||||
return False
|
||||
return bool(getattr(adapter, "enforces_own_access_policy", False))
|
||||
|
||||
def _is_user_authorized(self, source: SessionSource) -> bool:
|
||||
"""
|
||||
Check if a user is authorized to use the bot.
|
||||
@ -6681,6 +6700,15 @@ class GatewayRunner:
|
||||
global_allowlist = os.getenv("GATEWAY_ALLOWED_USERS", "").strip()
|
||||
|
||||
if not platform_allowlist and not group_user_allowlist and not group_chat_allowlist and not global_allowlist:
|
||||
# No env allowlists configured. Adapters that own their own
|
||||
# config-driven access policy (dm_policy / group_policy /
|
||||
# allow_from / group_allow_from) already gated this message at
|
||||
# intake — it would not have reached the gateway otherwise — so
|
||||
# honor that decision instead of falling through to the
|
||||
# env-only default-deny below, which would silently break
|
||||
# `dm_policy: open` and config-only allowlists. (#34515)
|
||||
if self._adapter_enforces_own_access_policy(source.platform):
|
||||
return True
|
||||
# No allowlists configured -- check global allow-all flag
|
||||
return os.getenv("GATEWAY_ALLOW_ALL_USERS", "").lower() in {"true", "1", "yes"}
|
||||
|
||||
@ -6788,6 +6816,20 @@ class GatewayRunner:
|
||||
if config.unauthorized_dm_behavior != "pair": # non-default → explicit override
|
||||
return config.unauthorized_dm_behavior
|
||||
|
||||
# Config-driven dm_policy (WeCom / Weixin / Yuanbao / QQBot). An
|
||||
# allowlist or disabled DM policy means the operator restricted access,
|
||||
# so unauthorized DMs should be dropped silently rather than answered
|
||||
# with a pairing code. An explicit pairing policy opts back into codes.
|
||||
if platform and config and hasattr(config, "platforms"):
|
||||
platform_cfg = config.platforms.get(platform)
|
||||
extra = getattr(platform_cfg, "extra", None) if platform_cfg else None
|
||||
if isinstance(extra, dict):
|
||||
dm_policy = str(extra.get("dm_policy") or "").strip().lower()
|
||||
if dm_policy == "pairing":
|
||||
return "pair"
|
||||
if dm_policy in {"allowlist", "disabled"}:
|
||||
return "ignore"
|
||||
|
||||
# No explicit override. Fall back to allowlist-aware default:
|
||||
# if any allowlist is configured for this platform, silently drop
|
||||
# unauthorized messages instead of sending pairing codes.
|
||||
|
||||
@ -59,6 +59,7 @@ AUTHOR_MAP = {
|
||||
"wangpuv@hotmail.com": "wangpuv",
|
||||
"202622897+ticketclosed-wontfix@users.noreply.github.com": "ticketclosed-wontfix",
|
||||
"wuxuebin1993@gmail.com": "victorGPT",
|
||||
"frowte3k@gmail.com": "Frowtek",
|
||||
"211828103+julio-cloudvisor@users.noreply.github.com": "julio-cloudvisor",
|
||||
"17778+kweiner@users.noreply.github.com": "kweiner",
|
||||
"223516181+faisfamilytravel@users.noreply.github.com": "faisfamilytravel",
|
||||
|
||||
234
tests/gateway/test_config_driven_access_policy.py
Normal file
234
tests/gateway/test_config_driven_access_policy.py
Normal file
@ -0,0 +1,234 @@
|
||||
"""Tests for config-driven platform access policies at the gateway layer.
|
||||
|
||||
Background (#34515): WeCom, Weixin, Yuanbao, and QQBot expose a documented
|
||||
config-driven access surface (``dm_policy`` / ``group_policy`` / ``allow_from``
|
||||
/ ``group_allow_from`` in ``PlatformConfig.extra``) and enforce it at intake —
|
||||
a message is dropped inside the adapter and never reaches the gateway unless it
|
||||
already passed that policy.
|
||||
|
||||
The gateway's env-based allowlist check (``_is_user_authorized``) runs *after*
|
||||
the adapter. Before the fix it fell through to an env-only default-deny when no
|
||||
``PLATFORM_ALLOWED_USERS`` env var was set, silently rejecting ``dm_policy:
|
||||
open`` and config-only allowlists even though the adapter had already
|
||||
authorized the sender.
|
||||
|
||||
The fix is a single drift-proof contract: adapters that own their access policy
|
||||
declare ``enforces_own_access_policy`` (a ``BasePlatformAdapter`` property,
|
||||
default ``False``). The gateway trusts that flag and skips the env-only
|
||||
default-deny for those platforms, rather than re-implementing each adapter's
|
||||
policy logic a second time.
|
||||
"""
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.config import GatewayConfig, Platform, PlatformConfig
|
||||
from gateway.session import SessionSource
|
||||
|
||||
|
||||
# Platforms whose adapters own their access policy at intake.
|
||||
_OWN_POLICY_PLATFORMS = [
|
||||
Platform.WECOM,
|
||||
Platform.WEIXIN,
|
||||
Platform.YUANBAO,
|
||||
Platform.QQBOT,
|
||||
]
|
||||
|
||||
|
||||
def _clear_auth_env(monkeypatch) -> None:
|
||||
for key in (
|
||||
"WECOM_ALLOWED_USERS",
|
||||
"WEIXIN_ALLOWED_USERS",
|
||||
"YUANBAO_ALLOWED_USERS",
|
||||
"QQ_ALLOWED_USERS",
|
||||
"QQ_GROUP_ALLOWED_USERS",
|
||||
"TELEGRAM_ALLOWED_USERS",
|
||||
"GATEWAY_ALLOWED_USERS",
|
||||
"GATEWAY_ALLOW_ALL_USERS",
|
||||
"WECOM_ALLOW_ALL_USERS",
|
||||
"WEIXIN_ALLOW_ALL_USERS",
|
||||
"YUANBAO_ALLOW_ALL_USERS",
|
||||
"QQ_ALLOW_ALL_USERS",
|
||||
):
|
||||
monkeypatch.delenv(key, raising=False)
|
||||
|
||||
|
||||
def _make_runner(platform: Platform, config: GatewayConfig, *, enforces: bool):
|
||||
"""Build a bare GatewayRunner with one adapter for *platform*.
|
||||
|
||||
``enforces`` controls whether the adapter declares
|
||||
``enforces_own_access_policy`` — i.e. whether it owns its access gate.
|
||||
"""
|
||||
from gateway.run import GatewayRunner
|
||||
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = config
|
||||
adapter = SimpleNamespace(send=AsyncMock(), enforces_own_access_policy=enforces)
|
||||
runner.adapters = {platform: adapter}
|
||||
runner.pairing_store = MagicMock()
|
||||
runner.pairing_store.is_approved.return_value = False
|
||||
runner.pairing_store._is_rate_limited.return_value = False
|
||||
return runner, adapter
|
||||
|
||||
|
||||
def _source(platform: Platform, *, chat_type: str = "dm") -> SessionSource:
|
||||
return SessionSource(
|
||||
platform=platform,
|
||||
user_id="some-user",
|
||||
chat_id="some-chat",
|
||||
user_name="tester",
|
||||
chat_type=chat_type,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Layer 1: the base-class contract and per-adapter overrides
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_base_adapter_defaults_to_not_owning_access_policy():
|
||||
"""Adapters that don't override the property delegate to the gateway."""
|
||||
from gateway.platforms.base import BasePlatformAdapter
|
||||
|
||||
# The default lives on the base property descriptor.
|
||||
assert BasePlatformAdapter.enforces_own_access_policy.fget(object()) is False
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"module_path, class_name",
|
||||
[
|
||||
("gateway.platforms.wecom", "WeComAdapter"),
|
||||
("gateway.platforms.weixin", "WeixinAdapter"),
|
||||
("gateway.platforms.yuanbao", "YuanbaoAdapter"),
|
||||
("gateway.platforms.qqbot.adapter", "QQAdapter"),
|
||||
],
|
||||
)
|
||||
def test_own_policy_adapters_declare_the_flag(module_path, class_name):
|
||||
"""The four config-policy adapters override the flag to True."""
|
||||
import importlib
|
||||
|
||||
module = importlib.import_module(module_path)
|
||||
adapter_cls = getattr(module, class_name)
|
||||
# Property is overridden on the subclass and returns True regardless of
|
||||
# instance state (it reflects a static capability, not runtime config).
|
||||
value = adapter_cls.enforces_own_access_policy.fget(object.__new__(adapter_cls))
|
||||
assert value is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Layer 2: gateway trusts the adapter-enforced flag
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
||||
def test_own_policy_platform_authorized_without_env_allowlist(monkeypatch, platform):
|
||||
"""A message reaching the gateway from an own-policy adapter is trusted.
|
||||
|
||||
With no env allowlist set, the gateway must NOT default-deny — the adapter
|
||||
already authorized the sender at intake (e.g. ``dm_policy: open``).
|
||||
"""
|
||||
_clear_auth_env(monkeypatch)
|
||||
config = GatewayConfig(
|
||||
platforms={platform: PlatformConfig(enabled=True, extra={"dm_policy": "open"})}
|
||||
)
|
||||
runner, _adapter = _make_runner(platform, config, enforces=True)
|
||||
|
||||
assert runner._is_user_authorized(_source(platform)) is True
|
||||
|
||||
|
||||
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
||||
def test_own_policy_platform_authorized_for_group_chat(monkeypatch, platform):
|
||||
"""Group traffic from an own-policy adapter is trusted the same way."""
|
||||
_clear_auth_env(monkeypatch)
|
||||
config = GatewayConfig(
|
||||
platforms={platform: PlatformConfig(enabled=True, extra={"group_policy": "open"})}
|
||||
)
|
||||
runner, _adapter = _make_runner(platform, config, enforces=True)
|
||||
|
||||
assert runner._is_user_authorized(_source(platform, chat_type="group")) is True
|
||||
|
||||
|
||||
def test_non_owning_platform_still_default_denies(monkeypatch):
|
||||
"""Adapters that don't own their policy keep the env-only default-deny."""
|
||||
_clear_auth_env(monkeypatch)
|
||||
config = GatewayConfig(
|
||||
platforms={Platform.TELEGRAM: PlatformConfig(enabled=True, token="t")}
|
||||
)
|
||||
runner, _adapter = _make_runner(Platform.TELEGRAM, config, enforces=False)
|
||||
|
||||
assert runner._is_user_authorized(_source(Platform.TELEGRAM)) is False
|
||||
|
||||
|
||||
def test_env_allowlist_still_takes_precedence_for_own_policy_platform(monkeypatch):
|
||||
"""When an env allowlist IS set, it governs — adapter trust is a fallback.
|
||||
|
||||
The adapter-trust branch only fires when no env allowlist exists, so an
|
||||
operator who sets ``WECOM_ALLOWED_USERS`` still gets env-based gating and
|
||||
a non-listed user is denied.
|
||||
"""
|
||||
_clear_auth_env(monkeypatch)
|
||||
monkeypatch.setenv("WECOM_ALLOWED_USERS", "allowed-user")
|
||||
config = GatewayConfig(
|
||||
platforms={Platform.WECOM: PlatformConfig(enabled=True, extra={"dm_policy": "open"})}
|
||||
)
|
||||
runner, _adapter = _make_runner(Platform.WECOM, config, enforces=True)
|
||||
|
||||
listed = SessionSource(
|
||||
platform=Platform.WECOM, user_id="allowed-user", chat_id="c",
|
||||
user_name="t", chat_type="dm",
|
||||
)
|
||||
stranger = SessionSource(
|
||||
platform=Platform.WECOM, user_id="stranger", chat_id="c",
|
||||
user_name="t", chat_type="dm",
|
||||
)
|
||||
assert runner._is_user_authorized(listed) is True
|
||||
assert runner._is_user_authorized(stranger) is False
|
||||
|
||||
|
||||
def test_unknown_adapter_does_not_crash_trust_check(monkeypatch):
|
||||
"""No adapter registered for the platform → safe default-deny."""
|
||||
_clear_auth_env(monkeypatch)
|
||||
config = GatewayConfig(platforms={Platform.WECOM: PlatformConfig(enabled=True)})
|
||||
runner, _adapter = _make_runner(Platform.WECOM, config, enforces=True)
|
||||
runner.adapters = {} # nothing registered
|
||||
|
||||
assert runner._adapter_enforces_own_access_policy(Platform.WECOM) is False
|
||||
assert runner._is_user_authorized(_source(Platform.WECOM)) is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Layer 3: unauthorized-DM behavior reads config dm_policy
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"dm_policy, expected",
|
||||
[
|
||||
("allowlist", "ignore"),
|
||||
("disabled", "ignore"),
|
||||
("pairing", "pair"),
|
||||
],
|
||||
)
|
||||
def test_unauthorized_dm_behavior_follows_config_dm_policy(monkeypatch, dm_policy, expected):
|
||||
"""A restrictive dm_policy drops unauthorized DMs; pairing opts back in."""
|
||||
_clear_auth_env(monkeypatch)
|
||||
config = GatewayConfig(
|
||||
platforms={Platform.WECOM: PlatformConfig(enabled=True, extra={"dm_policy": dm_policy})}
|
||||
)
|
||||
runner, _adapter = _make_runner(Platform.WECOM, config, enforces=True)
|
||||
|
||||
assert runner._get_unauthorized_dm_behavior(Platform.WECOM) == expected
|
||||
|
||||
|
||||
def test_unauthorized_dm_behavior_open_policy_keeps_default(monkeypatch):
|
||||
"""``dm_policy: open`` is not restrictive → falls through to the default."""
|
||||
_clear_auth_env(monkeypatch)
|
||||
config = GatewayConfig(
|
||||
platforms={Platform.WECOM: PlatformConfig(enabled=True, extra={"dm_policy": "open"})}
|
||||
)
|
||||
runner, _adapter = _make_runner(Platform.WECOM, config, enforces=True)
|
||||
|
||||
# No allowlist + no restrictive policy → open-gateway pairing default.
|
||||
assert runner._get_unauthorized_dm_behavior(Platform.WECOM) == "pair"
|
||||
Reference in New Issue
Block a user