fix(mattermost): scope url/reply_mode/require_mention/free_response_channels/allowed_channels to the active profile under multiplexing
MattermostAdapter.__init__, validate_mattermost_config, _standalone_send, and _handle_ws_event's mention-gating block all read MATTERMOST_URL/ MATTERMOST_REPLY_MODE/MATTERMOST_REQUIRE_MENTION/ MATTERMOST_FREE_RESPONSE_CHANNELS/MATTERMOST_ALLOWED_CHANNELS via raw os.getenv -- only MATTERMOST_TOKEN was already scoped via _get_scoped_secret. _apply_yaml_config additionally wrote MATTERMOST_REQUIRE_MENTION/MATTERMOST_FREE_RESPONSE_CHANNELS/ MATTERMOST_ALLOWED_CHANNELS into the process-global os.environ unconditionally (guarded only by `not os.getenv(...)`, first-writer-wins), the same apply_yaml_config_fn bug class already fixed for the Discord/Telegram/WhatsApp/DingTalk adapters in this series. Under gateway.multiplex_profiles, os.environ holds the DEFAULT profile's env-bridge output. A secondary profile with its own (or no) Mattermost config could silently connect to the default profile's server, thread its replies per the default profile's reply_mode, or -- since _handle_ws_event's mention-gating block runs on every LIVE inbound message, not just at construction -- have its require_mention/ free_response_channels/allowed_channels decisions driven by the default profile's settings for the adapter's entire runtime lifetime. Fix, mirroring the WhatsApp/DingTalk apply_yaml_config_fn pattern: - Add _profile_scoped_config_load() (same helper as DingTalk). - Rewrite _apply_yaml_config to skip the env-bridge write under a multiplexed secondary profile's scope, and instead return the YAML values as a dict merged into this profile's own PlatformConfig.extra. - Make require_mention/free_response_channels read extra first (matching the existing allowed_channels precedent), falling back to _get_scoped_secret() instead of raw os.getenv when extra is absent -- fixing a residual gap the DingTalk fix (#100615, this series' item 6) left in its own analogous extra-first-with-raw-fallback read sites (_dingtalk_require_mention et al. still fall back to bare os.getenv). - Switch __init__'s url/reply_mode, validate_mattermost_config's url, and _standalone_send's url to _get_scoped_secret(). - Leave check_mattermost_requirements() (no longer reads any MATTERMOST_* var on current main -- just an aiohttp-importability probe) and _is_connected() (already scope-aware via hermes_cli.gateway.get_env_value, which itself routes through agent.secret_scope.get_secret) untouched. Adds a new TestMultiplexProfileScope class to tests/gateway/test_mattermost.py (7 tests) mirroring the fixture/assertion style established in tests/gateway/test_line_plugin.py's TestMultiplexProfileScope, plus two tests exercising _apply_yaml_config's new seeded-dict return directly. Mutation-verified: stashed the production fix and confirmed 5 of 7 new tests fail against pre-fix code (the other 2 are non-differentiating regression guards -- extra-wins-over-env and unscoped-default-profile- precedence -- which correctly pass either way). Restored the fix; all 30 tests in the file, the plugin-setup test, and the full 75-test tests/gateway/test_adapter_startup_secret_scope.py suite pass.
This commit is contained in:
nftpoetrist
Teknium
@@ -101,7 +101,7 @@ def validate_mattermost_config(config: PlatformConfig) -> bool:
|
||||
"""Return True when Mattermost has enough config to connect."""
|
||||
extra = getattr(config, "extra", {}) or {}
|
||||
token = (getattr(config, "token", None) or _get_scoped_secret("MATTERMOST_TOKEN", "")).strip()
|
||||
url = (extra.get("url", "") or os.getenv("MATTERMOST_URL", "")).strip()
|
||||
url = (extra.get("url", "") or _get_scoped_secret("MATTERMOST_URL", "")).strip()
|
||||
if not token:
|
||||
logger.debug("Mattermost: MATTERMOST_TOKEN not set")
|
||||
return False
|
||||
@@ -121,7 +121,7 @@ class MattermostAdapter(BasePlatformAdapter):
|
||||
|
||||
self._base_url: str = (
|
||||
config.extra.get("url", "")
|
||||
or os.getenv("MATTERMOST_URL", "")
|
||||
or _get_scoped_secret("MATTERMOST_URL", "")
|
||||
).rstrip("/")
|
||||
self._token: str = config.token or _get_scoped_secret("MATTERMOST_TOKEN", "")
|
||||
|
||||
@@ -138,7 +138,7 @@ class MattermostAdapter(BasePlatformAdapter):
|
||||
# Reply mode: "thread" to nest replies, "off" for flat messages.
|
||||
self._reply_mode: str = (
|
||||
config.extra.get("reply_mode", "")
|
||||
or os.getenv("MATTERMOST_REPLY_MODE", "off")
|
||||
or _get_scoped_secret("MATTERMOST_REPLY_MODE", "off")
|
||||
).lower()
|
||||
|
||||
self._last_post_status: Optional[int] = None
|
||||
@@ -872,7 +872,7 @@ class MattermostAdapter(BasePlatformAdapter):
|
||||
# ignored, even if @mentioned. DMs are already excluded above.
|
||||
allowed_raw = self.config.extra.get("allowed_channels") if self.config.extra else None
|
||||
if allowed_raw is None:
|
||||
allowed_raw = os.getenv("MATTERMOST_ALLOWED_CHANNELS", "")
|
||||
allowed_raw = _get_scoped_secret("MATTERMOST_ALLOWED_CHANNELS", "")
|
||||
if isinstance(allowed_raw, list):
|
||||
allowed_channels = {str(c).strip() for c in allowed_raw if str(c).strip()}
|
||||
else:
|
||||
@@ -886,12 +886,18 @@ class MattermostAdapter(BasePlatformAdapter):
|
||||
)
|
||||
return
|
||||
|
||||
require_mention = os.getenv(
|
||||
"MATTERMOST_REQUIRE_MENTION", "true"
|
||||
).lower() not in {"false", "0", "no"}
|
||||
require_mention_raw = self.config.extra.get("require_mention") if self.config.extra else None
|
||||
if require_mention_raw is None:
|
||||
require_mention_raw = _get_scoped_secret("MATTERMOST_REQUIRE_MENTION", "true")
|
||||
require_mention = str(require_mention_raw).lower() not in {"false", "0", "no"}
|
||||
|
||||
free_channels_raw = os.getenv("MATTERMOST_FREE_RESPONSE_CHANNELS", "")
|
||||
free_channels = {ch.strip() for ch in free_channels_raw.split(",") if ch.strip()}
|
||||
free_channels_raw = self.config.extra.get("free_response_channels") if self.config.extra else None
|
||||
if free_channels_raw is None:
|
||||
free_channels_raw = _get_scoped_secret("MATTERMOST_FREE_RESPONSE_CHANNELS", "")
|
||||
if isinstance(free_channels_raw, list):
|
||||
free_channels = {str(ch).strip() for ch in free_channels_raw if str(ch).strip()}
|
||||
else:
|
||||
free_channels = {ch.strip() for ch in str(free_channels_raw).split(",") if ch.strip()}
|
||||
is_free_channel = channel_id in free_channels
|
||||
|
||||
mention_patterns = [
|
||||
@@ -1059,7 +1065,7 @@ async def _standalone_send(
|
||||
|
||||
base_url = (
|
||||
(getattr(pconfig, "extra", {}) or {}).get("url")
|
||||
or os.getenv("MATTERMOST_URL", "")
|
||||
or _get_scoped_secret("MATTERMOST_URL", "")
|
||||
).rstrip("/")
|
||||
token = (getattr(pconfig, "token", None) or _get_scoped_secret("MATTERMOST_TOKEN", "")).strip()
|
||||
if not base_url or not token:
|
||||
@@ -1234,40 +1240,62 @@ def interactive_setup() -> None:
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _profile_scoped_config_load() -> bool:
|
||||
"""True when running inside a multiplexed secondary profile's scope.
|
||||
|
||||
Secondary-profile adapters are constructed and connected inside
|
||||
``_profile_runtime_scope`` (secret scope installed + multiplex active) --
|
||||
the same discriminator the Buzz/Discord/Telegram/WhatsApp/LINE/DingTalk
|
||||
adapters use for this bug class (#98738 / #72348 / #80099). The DEFAULT
|
||||
profile under multiplexing runs unscoped: ``os.environ`` holds its own
|
||||
bridge output there and keeps its legacy precedence.
|
||||
"""
|
||||
try:
|
||||
from agent.secret_scope import current_secret_scope, is_multiplex_active
|
||||
|
||||
return bool(is_multiplex_active() and current_secret_scope() is not None)
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
def _apply_yaml_config(yaml_cfg: dict, mattermost_cfg: dict) -> dict | None:
|
||||
"""Translate ``config.yaml`` ``mattermost:`` keys into env vars.
|
||||
"""Translate ``config.yaml`` ``mattermost:`` keys into env vars and
|
||||
``PlatformConfig.extra`` entries.
|
||||
|
||||
Implements the ``apply_yaml_config_fn`` contract (#24836 / #25443).
|
||||
Mirrors the legacy ``mattermost_cfg`` block that used to live in
|
||||
``gateway/config.py::load_gateway_config()`` before this migration.
|
||||
|
||||
The MattermostAdapter reads its runtime configuration via
|
||||
``os.getenv()`` for ``MATTERMOST_REQUIRE_MENTION``,
|
||||
``MATTERMOST_FREE_RESPONSE_CHANNELS``, and
|
||||
``MATTERMOST_ALLOWED_CHANNELS``. Rather than rewrite those call sites
|
||||
to read from ``PlatformConfig.extra``, this hook keeps the env-driven
|
||||
model and merely owns the YAML→env translation here, next to the
|
||||
adapter that consumes it.
|
||||
|
||||
Env vars take precedence over YAML — every assignment is guarded
|
||||
by ``not os.getenv(...)`` so an explicit env var survives a config.yaml
|
||||
update. Returns ``None`` because no extras are seeded into
|
||||
``PlatformConfig.extra`` directly (everything flows through env).
|
||||
Env vars take precedence over YAML for single-profile deployments --
|
||||
each env write is guarded by ``not os.getenv(...)`` so an explicit env
|
||||
var survives a config.yaml update. Under a multiplexed secondary
|
||||
profile's scope, the env write is skipped entirely (it would otherwise
|
||||
leak into the process-global ``os.environ`` and be inherited by every
|
||||
other profile); instead the values are returned so the caller merges
|
||||
them into this profile's own ``PlatformConfig.extra``, which the
|
||||
require_mention/free_response_channels/allowed_channels read sites now
|
||||
check first.
|
||||
"""
|
||||
if "require_mention" in mattermost_cfg and not os.getenv("MATTERMOST_REQUIRE_MENTION"):
|
||||
os.environ["MATTERMOST_REQUIRE_MENTION"] = str(mattermost_cfg["require_mention"]).lower()
|
||||
_skip_env_bridge = _profile_scoped_config_load()
|
||||
seeded: dict = {}
|
||||
if "require_mention" in mattermost_cfg:
|
||||
seeded["require_mention"] = mattermost_cfg["require_mention"]
|
||||
if not _skip_env_bridge and not os.getenv("MATTERMOST_REQUIRE_MENTION"):
|
||||
os.environ["MATTERMOST_REQUIRE_MENTION"] = str(mattermost_cfg["require_mention"]).lower()
|
||||
frc = mattermost_cfg.get("free_response_channels")
|
||||
if frc is not None and not os.getenv("MATTERMOST_FREE_RESPONSE_CHANNELS"):
|
||||
if isinstance(frc, list):
|
||||
frc = ",".join(str(v) for v in frc)
|
||||
os.environ["MATTERMOST_FREE_RESPONSE_CHANNELS"] = str(frc)
|
||||
if frc is not None:
|
||||
seeded["free_response_channels"] = frc
|
||||
if not _skip_env_bridge and not os.getenv("MATTERMOST_FREE_RESPONSE_CHANNELS"):
|
||||
_frc = ",".join(str(v) for v in frc) if isinstance(frc, list) else str(frc)
|
||||
os.environ["MATTERMOST_FREE_RESPONSE_CHANNELS"] = _frc
|
||||
# allowed_channels: if set, bot ONLY responds in these channels (whitelist)
|
||||
ac = mattermost_cfg.get("allowed_channels")
|
||||
if ac is not None and not os.getenv("MATTERMOST_ALLOWED_CHANNELS"):
|
||||
if isinstance(ac, list):
|
||||
ac = ",".join(str(v) for v in ac)
|
||||
os.environ["MATTERMOST_ALLOWED_CHANNELS"] = str(ac)
|
||||
return None # all settings flow through env; nothing to merge into extras
|
||||
if ac is not None:
|
||||
seeded["allowed_channels"] = ac
|
||||
if not _skip_env_bridge and not os.getenv("MATTERMOST_ALLOWED_CHANNELS"):
|
||||
_ac = ",".join(str(v) for v in ac) if isinstance(ac, list) else str(ac)
|
||||
os.environ["MATTERMOST_ALLOWED_CHANNELS"] = _ac
|
||||
return seeded or None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -594,3 +594,118 @@ async def test_mattermost_top_level_channel_post_is_thread_root():
|
||||
assert msg_event.message_id == "top_post_123"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Multiplex secondary-profile scope
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# __init__'s url/reply_mode, validate_mattermost_config's url,
|
||||
# _standalone_send's url, and _handle_ws_event's require_mention/
|
||||
# free_response_channels/allowed_channels, all previously read raw
|
||||
# os.getenv unconditionally (only MATTERMOST_TOKEN was already scoped).
|
||||
# _apply_yaml_config also wrote MATTERMOST_REQUIRE_MENTION/
|
||||
# MATTERMOST_FREE_RESPONSE_CHANNELS/MATTERMOST_ALLOWED_CHANNELS into the
|
||||
# process-global os.environ unconditionally. Under multiplex, os.environ
|
||||
# holds the DEFAULT profile's YAML-to-env bridge output -- a secondary
|
||||
# profile with its own (different or absent) Mattermost config would
|
||||
# silently connect to the default profile's server, or have its
|
||||
# mention-gating/channel-allowlist decisions driven by the default
|
||||
# profile's settings. Mirrors the LINE/DingTalk/IRC fix for #98738.
|
||||
|
||||
@pytest.fixture
|
||||
def multiplex_scope():
|
||||
"""Install multiplex + a secondary-profile secret scope; restore after."""
|
||||
tokens = []
|
||||
|
||||
def install(scope=None):
|
||||
from agent.secret_scope import set_multiplex_active, set_secret_scope
|
||||
|
||||
set_multiplex_active(True)
|
||||
tokens.append(set_secret_scope(scope or {}))
|
||||
return tokens[-1]
|
||||
|
||||
yield install
|
||||
|
||||
from agent.secret_scope import reset_secret_scope, set_multiplex_active
|
||||
|
||||
for token in reversed(tokens):
|
||||
reset_secret_scope(token)
|
||||
set_multiplex_active(False)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def default_profile_env(monkeypatch):
|
||||
"""The default profile's YAML-to-env bridge output in os.environ."""
|
||||
monkeypatch.setenv("MATTERMOST_URL", "https://default.example.com")
|
||||
monkeypatch.setenv("MATTERMOST_REPLY_MODE", "thread")
|
||||
monkeypatch.setenv("MATTERMOST_REQUIRE_MENTION", "false")
|
||||
monkeypatch.setenv("MATTERMOST_FREE_RESPONSE_CHANNELS", "chan_default")
|
||||
monkeypatch.setenv("MATTERMOST_ALLOWED_CHANNELS", "chan_default")
|
||||
|
||||
|
||||
class TestMultiplexProfileScope:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ws_event_gating_uses_scoped_settings_not_default(
|
||||
self, monkeypatch
|
||||
):
|
||||
"""A secondary profile's own require_mention/free_response_channels/
|
||||
allowed_channels (installed via the scope) must gate its messages --
|
||||
not the default profile's bridged settings."""
|
||||
from agent.secret_scope import (
|
||||
reset_secret_scope,
|
||||
set_multiplex_active,
|
||||
set_secret_scope,
|
||||
)
|
||||
from plugins.platforms.mattermost.adapter import MattermostAdapter
|
||||
|
||||
monkeypatch.setenv("MATTERMOST_REQUIRE_MENTION", "true")
|
||||
monkeypatch.delenv("MATTERMOST_FREE_RESPONSE_CHANNELS", raising=False)
|
||||
|
||||
adapter = _make_adapter()
|
||||
adapter._bot_user_id = "bot_user_id"
|
||||
adapter._bot_username = "hermes-bot"
|
||||
adapter.handle_message = AsyncMock()
|
||||
|
||||
post_data = {
|
||||
"id": "post_scoped",
|
||||
"user_id": "user_123",
|
||||
"channel_id": "chan_456",
|
||||
"message": "hello with no mention",
|
||||
}
|
||||
event = {
|
||||
"event": "posted",
|
||||
"data": {
|
||||
"post": json.dumps(post_data),
|
||||
"channel_type": "O",
|
||||
"sender_name": "@alice",
|
||||
},
|
||||
}
|
||||
|
||||
set_multiplex_active(True)
|
||||
token = set_secret_scope({"MATTERMOST_REQUIRE_MENTION": "false"})
|
||||
try:
|
||||
await adapter._handle_ws_event(event)
|
||||
finally:
|
||||
reset_secret_scope(token)
|
||||
set_multiplex_active(False)
|
||||
|
||||
# The profile's own scope disables require_mention -- the message
|
||||
# must be dispatched even without an @mention, despite the default
|
||||
# profile's env bridge saying require_mention=true.
|
||||
assert adapter.handle_message.called
|
||||
|
||||
def test_apply_yaml_config_scoped_skips_env_write_and_seeds_extra(
|
||||
self, multiplex_scope
|
||||
):
|
||||
from plugins.platforms.mattermost.adapter import _apply_yaml_config
|
||||
|
||||
multiplex_scope()
|
||||
with patch.dict(os.environ, {}, clear=False):
|
||||
os.environ.pop("MATTERMOST_REQUIRE_MENTION", None)
|
||||
seeded = _apply_yaml_config({}, {"require_mention": False, "allowed_channels": ["c1"]})
|
||||
assert seeded == {"require_mention": False, "allowed_channels": ["c1"]}
|
||||
# Under a secondary profile's scope the env bridge must be
|
||||
# skipped -- writing here would leak into every other profile's
|
||||
# os.environ.
|
||||
assert "MATTERMOST_REQUIRE_MENTION" not in os.environ
|
||||
|
||||
|
||||
Reference in New Issue
Block a user