fix(gateway): revoke WhatsApp sole allowlist entry without restart
Clear live adapter _allow_from on pairing revoke and re-check DM allowlist authz so sole-entry removal takes effect without restart.
This commit is contained in:
fangliquanflq
kshitij
parent
6035f50477
commit
ddfc6342ad
|
|
@ -651,6 +651,26 @@ class GatewayAuthorizationMixin:
|
|||
profile=adapter_profile,
|
||||
)
|
||||
if effective_policy == "allowlist":
|
||||
# Trust allowlist intake only when the live adapter still
|
||||
# allowlists this sender. Pairing revoke can clear
|
||||
# WHATSAPP_ALLOWED_USERS while a construction-time
|
||||
# ``_allow_from`` snapshot would otherwise keep authorizing
|
||||
# until restart; re-check when the adapter exposes a DM
|
||||
# allowlist helper. Adapters without that helper keep the
|
||||
# historical "reached the gateway under allowlist policy"
|
||||
# rubber-stamp (#34515).
|
||||
if source.chat_type not in {"group", "forum", "channel"}:
|
||||
adapter = self._authorization_adapter(
|
||||
source.platform,
|
||||
profile=adapter_profile,
|
||||
)
|
||||
dm_check = (
|
||||
getattr(adapter, "_is_dm_allowed", None)
|
||||
if adapter is not None
|
||||
else None
|
||||
)
|
||||
if callable(dm_check):
|
||||
return bool(dm_check(user_id))
|
||||
return True
|
||||
# Some adapters (e.g. Telegram) gate access via config.extra.allow_from /
|
||||
# group_allow_from at intake but do not override enforces_own_access_policy.
|
||||
|
|
|
|||
|
|
@ -113,6 +113,34 @@ def _split_allowlist(raw: str) -> list:
|
|||
return [uid.strip() for uid in raw.split(",") if uid.strip()]
|
||||
|
||||
|
||||
def _normalize_user_id(platform: str, user_id: str) -> str:
|
||||
"""Normalize platform-specific user IDs before persisting / comparing them."""
|
||||
raw_user_id = str(user_id or "").strip()
|
||||
if platform == "whatsapp":
|
||||
return normalize_whatsapp_identifier(raw_user_id) or raw_user_id
|
||||
return raw_user_id
|
||||
|
||||
|
||||
def _user_id_aliases(platform: str, user_id: str) -> set[str]:
|
||||
"""Return all known equivalent user IDs for auth / allowlist matching."""
|
||||
raw_user_id = str(user_id or "").strip()
|
||||
if not raw_user_id:
|
||||
return set()
|
||||
|
||||
aliases = {raw_user_id, _normalize_user_id(platform, raw_user_id)}
|
||||
if platform == "whatsapp":
|
||||
aliases.update(expand_whatsapp_aliases(raw_user_id))
|
||||
aliases.discard("")
|
||||
return aliases
|
||||
|
||||
|
||||
def _user_ids_match(platform: str, left: str, right: str) -> bool:
|
||||
"""Return True when two user IDs represent the same principal."""
|
||||
left_aliases = _user_id_aliases(platform, left)
|
||||
right_aliases = _user_id_aliases(platform, right)
|
||||
return bool(left_aliases and right_aliases and (left_aliases & right_aliases))
|
||||
|
||||
|
||||
def _sync_allowlist_add(platform: str, user_id: str) -> None:
|
||||
"""Add ``user_id`` to the platform allowlist env var IF one is configured.
|
||||
|
||||
|
|
@ -142,16 +170,118 @@ def _sync_allowlist_add(platform: str, user_id: str) -> None:
|
|||
pass
|
||||
|
||||
|
||||
def _iter_live_gateway_adapters():
|
||||
"""Yield adapters from the in-process GatewayRunner, if one is running."""
|
||||
try:
|
||||
from gateway.run import _gateway_runner_ref
|
||||
|
||||
runner = _gateway_runner_ref()
|
||||
except Exception:
|
||||
return
|
||||
if runner is None:
|
||||
return
|
||||
adapters = getattr(runner, "adapters", None) or {}
|
||||
for adapter in adapters.values():
|
||||
if adapter is not None:
|
||||
yield adapter
|
||||
profile_adapters = getattr(runner, "_profile_adapters", None) or {}
|
||||
for mapping in profile_adapters.values():
|
||||
for adapter in (mapping or {}).values():
|
||||
if adapter is not None:
|
||||
yield adapter
|
||||
|
||||
|
||||
def _adapter_platform_name(adapter) -> str:
|
||||
platform = getattr(adapter, "platform", None)
|
||||
if platform is not None:
|
||||
value = getattr(platform, "value", None)
|
||||
if value:
|
||||
return str(value).strip().lower()
|
||||
name = getattr(adapter, "name", None)
|
||||
return str(name or "").strip().lower()
|
||||
|
||||
|
||||
def _purge_allowlist_entries(entries, platform: str, user_id: str):
|
||||
"""Drop alias-equivalent allowlist entries while preserving ``*``."""
|
||||
if entries is None:
|
||||
return entries
|
||||
if isinstance(entries, str):
|
||||
parts = _split_allowlist(entries)
|
||||
remaining = [
|
||||
part for part in parts
|
||||
if part == "*" or not _user_ids_match(platform, part, str(user_id))
|
||||
]
|
||||
return ",".join(remaining)
|
||||
if isinstance(entries, (set, frozenset)):
|
||||
return {
|
||||
entry for entry in entries
|
||||
if str(entry).strip() == "*"
|
||||
or not _user_ids_match(platform, str(entry), str(user_id))
|
||||
}
|
||||
if isinstance(entries, (list, tuple)):
|
||||
return [
|
||||
entry for entry in entries
|
||||
if str(entry).strip() == "*"
|
||||
or not _user_ids_match(platform, str(entry), str(user_id))
|
||||
]
|
||||
return entries
|
||||
|
||||
|
||||
def _sync_live_adapter_allowlist_remove(platform: str, user_id: str) -> None:
|
||||
"""Clear revoked principals from in-process adapter allowlist snapshots.
|
||||
|
||||
``WhatsAppAdapter`` (and Cloud) snapshot ``_allow_from`` at construction.
|
||||
Pairing revoke updates ``WHATSAPP_ALLOWED_USERS`` / cloud env, but when the
|
||||
revoked principal was the sole entry the env key is removed entirely.
|
||||
Intake must not keep authorizing from the stale snapshot until restart.
|
||||
"""
|
||||
platform_name = (platform or "").strip().lower()
|
||||
if not platform_name or not str(user_id or "").strip():
|
||||
return
|
||||
for adapter in _iter_live_gateway_adapters():
|
||||
if _adapter_platform_name(adapter) != platform_name:
|
||||
continue
|
||||
if hasattr(adapter, "_allow_from"):
|
||||
try:
|
||||
adapter._allow_from = _purge_allowlist_entries(
|
||||
set(adapter._allow_from or ()), platform_name, user_id
|
||||
)
|
||||
except Exception:
|
||||
pass
|
||||
extra = getattr(getattr(adapter, "config", None), "extra", None)
|
||||
if isinstance(extra, dict) and "allow_from" in extra:
|
||||
try:
|
||||
extra["allow_from"] = _purge_allowlist_entries(
|
||||
extra.get("allow_from"), platform_name, user_id
|
||||
)
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
def _sync_allowlist_remove(platform: str, user_id: str) -> None:
|
||||
"""Remove ``user_id`` from the platform allowlist env var if present."""
|
||||
"""Remove ``user_id`` (and WhatsApp alias equivalents) from the allowlist.
|
||||
|
||||
Matching must mirror PairingStore / authz WhatsApp alias rules: approve
|
||||
mirrors a normalized phone into ``WHATSAPP_ALLOWED_USERS``, while revoke
|
||||
is often invoked with a JID or device-suffix form. Exact-string delete
|
||||
would leave the allowlist entry and keep the sender authorized.
|
||||
|
||||
Also clears matching entries from any in-process platform adapter
|
||||
``_allow_from`` snapshot so sole-entry revocation is effective without a
|
||||
gateway restart.
|
||||
"""
|
||||
env_var = _allowlist_env_for_platform(platform)
|
||||
if not env_var:
|
||||
return
|
||||
current = os.getenv(env_var, "").strip()
|
||||
if not current:
|
||||
return
|
||||
return # No allowlist configured — do not touch config-only snapshots.
|
||||
ids = _split_allowlist(current)
|
||||
remaining = [i for i in ids if i != str(user_id)]
|
||||
# Never strip a wildcard grant; drop every entry that aliases-matches.
|
||||
remaining = [
|
||||
i for i in ids
|
||||
if i == "*" or not _user_ids_match(platform, i, str(user_id))
|
||||
]
|
||||
if len(remaining) == len(ids):
|
||||
return # Not present.
|
||||
try:
|
||||
|
|
@ -163,6 +293,7 @@ def _sync_allowlist_remove(platform: str, user_id: str) -> None:
|
|||
remove_env_value(env_var)
|
||||
except Exception:
|
||||
pass
|
||||
_sync_live_adapter_allowlist_remove(platform, user_id)
|
||||
|
||||
|
||||
def _load_json_file(path: Path) -> dict:
|
||||
|
|
@ -339,28 +470,15 @@ class PairingStore:
|
|||
|
||||
def _normalize_user_id(self, platform: str, user_id: str) -> str:
|
||||
"""Normalize platform-specific user IDs before persisting them."""
|
||||
raw_user_id = str(user_id or "").strip()
|
||||
if platform == "whatsapp":
|
||||
return normalize_whatsapp_identifier(raw_user_id) or raw_user_id
|
||||
return raw_user_id
|
||||
return _normalize_user_id(platform, user_id)
|
||||
|
||||
def _user_id_aliases(self, platform: str, user_id: str) -> set[str]:
|
||||
"""Return all known equivalent user IDs for auth/rate-limit checks."""
|
||||
raw_user_id = str(user_id or "").strip()
|
||||
if not raw_user_id:
|
||||
return set()
|
||||
|
||||
aliases = {raw_user_id, self._normalize_user_id(platform, raw_user_id)}
|
||||
if platform == "whatsapp":
|
||||
aliases.update(expand_whatsapp_aliases(raw_user_id))
|
||||
aliases.discard("")
|
||||
return aliases
|
||||
return _user_id_aliases(platform, user_id)
|
||||
|
||||
def _user_ids_match(self, platform: str, left: str, right: str) -> bool:
|
||||
"""Return True when two user IDs represent the same principal."""
|
||||
left_aliases = self._user_id_aliases(platform, left)
|
||||
right_aliases = self._user_id_aliases(platform, right)
|
||||
return bool(left_aliases and right_aliases and (left_aliases & right_aliases))
|
||||
return _user_ids_match(platform, left, right)
|
||||
|
||||
# ----- Approved users -----
|
||||
|
||||
|
|
|
|||
|
|
@ -385,11 +385,16 @@ class WhatsAppCloudAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter):
|
|||
normalized.add(digits or entry)
|
||||
return normalized
|
||||
|
||||
def _dm_allowlist_env_keys(self) -> tuple[str, ...]:
|
||||
"""Cloud pairing mirrors WHATSAPP_CLOUD_ALLOWED_USERS (then legacy ALLOW_FROM)."""
|
||||
return ("WHATSAPP_CLOUD_ALLOWED_USERS", "WHATSAPP_CLOUD_ALLOW_FROM")
|
||||
|
||||
def _is_dm_allowed(self, sender_id: str) -> bool:
|
||||
"""Allowlist check against the normalized bare wa_id."""
|
||||
if self._dm_policy == "allowlist":
|
||||
bare = re.sub(r"\D", "", str(sender_id).split("@", 1)[0])
|
||||
return (bare or sender_id) in self._allow_from
|
||||
allow_from = self._normalize_allow_ids(self._live_dm_allow_from())
|
||||
return (bare or sender_id) in allow_from
|
||||
return super()._is_dm_allowed(sender_id)
|
||||
|
||||
def _open_dm_opted_in(self) -> bool:
|
||||
|
|
|
|||
|
|
@ -134,6 +134,29 @@ class WhatsAppBehaviorMixin:
|
|||
return {str(part).strip() for part in raw if str(part).strip()}
|
||||
return {part.strip() for part in str(raw).split(",") if part.strip()}
|
||||
|
||||
def _dm_allowlist_env_keys(self) -> tuple[str, ...]:
|
||||
"""Env vars that carry the live DM allowlist for this adapter.
|
||||
|
||||
Pairing approve/revoke mutates the documented ``*_ALLOWED_USERS`` var
|
||||
in-process. Intake must prefer that live carrier over the construction
|
||||
snapshot in ``_allow_from`` so sole-entry revocation takes effect
|
||||
without a gateway restart.
|
||||
"""
|
||||
return ("WHATSAPP_ALLOWED_USERS",)
|
||||
|
||||
def _live_dm_allow_from(self) -> set[str]:
|
||||
"""Allowlist currently enforced for DM intake / strict DM auth.
|
||||
|
||||
When a pairing-synced env allowlist key is present in ``os.environ``,
|
||||
that value is authoritative (including the empty set after the last
|
||||
entry is cleared but before the key is removed). Otherwise fall back
|
||||
to the adapter snapshot seeded at construction from config/env.
|
||||
"""
|
||||
for key in self._dm_allowlist_env_keys():
|
||||
if key in os.environ:
|
||||
return self._coerce_allow_list(os.environ.get(key, ""))
|
||||
return set(self._allow_from or ())
|
||||
|
||||
# ------------------------------------------------------------------ JID helpers
|
||||
@staticmethod
|
||||
def _normalize_whatsapp_id(value: Optional[str]) -> str:
|
||||
|
|
@ -214,7 +237,7 @@ class WhatsAppBehaviorMixin:
|
|||
if self._dm_policy == "disabled":
|
||||
return False
|
||||
if self._dm_policy == "allowlist":
|
||||
return self._matches_whatsapp_allowlist(sender_id, self._allow_from)
|
||||
return self._matches_whatsapp_allowlist(sender_id, self._live_dm_allow_from())
|
||||
if self._dm_policy == "open":
|
||||
return self._open_dm_opted_in()
|
||||
return False
|
||||
|
|
@ -227,7 +250,7 @@ class WhatsAppBehaviorMixin:
|
|||
if self._dm_policy == "disabled":
|
||||
return False
|
||||
if self._dm_policy == "allowlist":
|
||||
return self._matches_whatsapp_allowlist(principal, self._allow_from)
|
||||
return self._matches_whatsapp_allowlist(principal, self._live_dm_allow_from())
|
||||
if self._dm_policy == "pairing":
|
||||
return True
|
||||
if self._dm_policy == "open":
|
||||
|
|
|
|||
|
|
@ -408,7 +408,14 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter):
|
|||
))
|
||||
self._reply_prefix: Optional[str] = config.extra.get("reply_prefix")
|
||||
self._dm_policy = str(config.extra.get("dm_policy") or os.getenv("WHATSAPP_DM_POLICY", "pairing")).strip().lower()
|
||||
self._allow_from = self._coerce_allow_list(config.extra.get("allow_from") or config.extra.get("allowFrom"))
|
||||
# Prefer config.extra, then the documented WHATSAPP_ALLOWED_USERS env
|
||||
# (setup wizard / pairing mirror). Construction seeds the snapshot;
|
||||
# intake re-reads the live env via ``_live_dm_allow_from``.
|
||||
self._allow_from = self._coerce_allow_list(
|
||||
config.extra.get("allow_from")
|
||||
or config.extra.get("allowFrom")
|
||||
or os.getenv("WHATSAPP_ALLOWED_USERS")
|
||||
)
|
||||
self._group_policy = str(config.extra.get("group_policy") or os.getenv("WHATSAPP_GROUP_POLICY", "pairing")).strip().lower()
|
||||
self._group_allow_from = self._coerce_allow_list(config.extra.get("group_allow_from") or config.extra.get("groupAllowFrom"))
|
||||
read_receipts = config.extra.get("send_read_receipts", False)
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ def _isolate_env(monkeypatch):
|
|||
"TELEGRAM_ALLOW_ALL_USERS",
|
||||
"TELEGRAM_GROUP_ALLOWED_USERS",
|
||||
"TELEGRAM_GROUP_ALLOWED_CHATS",
|
||||
"WHATSAPP_ALLOWED_USERS",
|
||||
"GATEWAY_ALLOW_ALL_USERS",
|
||||
"GATEWAY_ALLOWED_USERS",
|
||||
):
|
||||
|
|
@ -127,3 +128,152 @@ def test_revoke_removes_from_allowlist(store, monkeypatch):
|
|||
assert saved.get("TELEGRAM_ALLOWED_USERS") == "owner1"
|
||||
|
||||
|
||||
def test_revoke_whatsapp_device_jid_removes_bare_allowlist_entry(store, monkeypatch):
|
||||
"""Revoke with a device-suffix JID must clear the normalized phone allowlist entry.
|
||||
|
||||
Approve persists/mirrors the bare phone; operators often revoke with the
|
||||
bridge's JID form. Exact-string allowlist remove left the user authorized.
|
||||
"""
|
||||
monkeypatch.setenv("WHATSAPP_ALLOWED_USERS", "already,15551234567")
|
||||
saved = {}
|
||||
import hermes_cli.config as cfg
|
||||
|
||||
monkeypatch.setattr(
|
||||
cfg,
|
||||
"save_env_value",
|
||||
lambda k, v: (saved.__setitem__(k, v), os.environ.__setitem__(k, v)),
|
||||
)
|
||||
monkeypatch.setattr(cfg, "remove_env_value", lambda k: os.environ.pop(k, None))
|
||||
|
||||
store._approve_user("whatsapp", "15551234567@s.whatsapp.net", "")
|
||||
assert store.is_approved("whatsapp", "15551234567@s.whatsapp.net") is True
|
||||
|
||||
assert store.revoke("whatsapp", "15551234567:47@s.whatsapp.net") is True
|
||||
assert store.is_approved("whatsapp", "15551234567@s.whatsapp.net") is False
|
||||
assert saved.get("WHATSAPP_ALLOWED_USERS") == "already"
|
||||
assert os.environ.get("WHATSAPP_ALLOWED_USERS") == "already"
|
||||
|
||||
|
||||
def test_revoke_whatsapp_removes_all_alias_forms_from_allowlist(store, monkeypatch):
|
||||
"""Allowlist may hold both bare phone and JID; revoke must drop every alias."""
|
||||
monkeypatch.setenv(
|
||||
"WHATSAPP_ALLOWED_USERS",
|
||||
"keeper,15551234567,15551234567@s.whatsapp.net",
|
||||
)
|
||||
saved = {}
|
||||
import hermes_cli.config as cfg
|
||||
|
||||
monkeypatch.setattr(
|
||||
cfg,
|
||||
"save_env_value",
|
||||
lambda k, v: (saved.__setitem__(k, v), os.environ.__setitem__(k, v)),
|
||||
)
|
||||
store._approve_user("whatsapp", "15551234567", "")
|
||||
|
||||
assert store.revoke("whatsapp", "15551234567@s.whatsapp.net") is True
|
||||
assert saved.get("WHATSAPP_ALLOWED_USERS") == "keeper"
|
||||
|
||||
|
||||
def test_revoke_whatsapp_preserves_wildcard_allowlist_entry(store, monkeypatch):
|
||||
monkeypatch.setenv("WHATSAPP_ALLOWED_USERS", "*,15551234567")
|
||||
saved = {}
|
||||
import hermes_cli.config as cfg
|
||||
|
||||
monkeypatch.setattr(
|
||||
cfg,
|
||||
"save_env_value",
|
||||
lambda k, v: (saved.__setitem__(k, v), os.environ.__setitem__(k, v)),
|
||||
)
|
||||
store._approve_user("whatsapp", "15551234567", "")
|
||||
|
||||
assert store.revoke("whatsapp", "15551234567:47@s.whatsapp.net") is True
|
||||
assert saved.get("WHATSAPP_ALLOWED_USERS") == "*"
|
||||
|
||||
|
||||
def test_revoke_whatsapp_sole_entry_denies_live_adapter_without_restart(
|
||||
store, monkeypatch,
|
||||
):
|
||||
"""Sole allowlist entry revoke must deny immediately on a live gateway.
|
||||
|
||||
Persistence alone is not enough: WhatsAppAdapter snapshots ``_allow_from``
|
||||
at construction, and authz trusts ``dm_policy=allowlist`` when the env
|
||||
allowlist is gone. After revoke, intake and ``_is_user_authorized`` must
|
||||
both deny the device-suffix JID without restarting the gateway.
|
||||
"""
|
||||
from types import SimpleNamespace
|
||||
|
||||
from gateway.config import GatewayConfig, Platform, PlatformConfig
|
||||
from gateway.platforms.whatsapp_common import WhatsAppBehaviorMixin
|
||||
from gateway.run import GatewayRunner
|
||||
import gateway.run as gateway_run
|
||||
import hermes_cli.config as cfg
|
||||
|
||||
monkeypatch.setenv("WHATSAPP_ALLOWED_USERS", "15551234567")
|
||||
monkeypatch.setattr(
|
||||
cfg,
|
||||
"save_env_value",
|
||||
lambda k, v: os.environ.__setitem__(k, v),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
cfg,
|
||||
"remove_env_value",
|
||||
lambda k: (os.environ.pop(k, None), True)[1],
|
||||
)
|
||||
|
||||
class LiveWhatsAppAdapter(WhatsAppBehaviorMixin):
|
||||
def __init__(self):
|
||||
self.config = SimpleNamespace(
|
||||
extra={
|
||||
"dm_policy": "allowlist",
|
||||
"allow_from": ["15551234567"],
|
||||
}
|
||||
)
|
||||
self.platform = Platform.WHATSAPP
|
||||
self._dm_policy = "allowlist"
|
||||
self._allow_from = {"15551234567"}
|
||||
self._group_policy = "pairing"
|
||||
self._group_allow_from = set()
|
||||
|
||||
adapter = LiveWhatsAppAdapter()
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = GatewayConfig(
|
||||
platforms={
|
||||
Platform.WHATSAPP: PlatformConfig(
|
||||
enabled=True,
|
||||
extra={"dm_policy": "allowlist", "allow_from": ["15551234567"]},
|
||||
)
|
||||
}
|
||||
)
|
||||
runner.adapters = {Platform.WHATSAPP: adapter}
|
||||
runner.pairing_store = store
|
||||
runner.pairing_stores = {}
|
||||
monkeypatch.setattr(gateway_run, "_gateway_runner_ref", lambda: runner)
|
||||
|
||||
store._approve_user("whatsapp", "15551234567@s.whatsapp.net", "")
|
||||
sender = "15551234567:47@s.whatsapp.net"
|
||||
assert adapter._is_dm_intake_allowed(sender) is True
|
||||
assert runner._is_user_authorized(
|
||||
SessionSource(
|
||||
platform=Platform.WHATSAPP,
|
||||
user_id=sender,
|
||||
chat_id=sender,
|
||||
user_name="revoked",
|
||||
chat_type="dm",
|
||||
)
|
||||
) is True
|
||||
|
||||
assert store.revoke("whatsapp", sender) is True
|
||||
assert store.is_approved("whatsapp", "15551234567@s.whatsapp.net") is False
|
||||
assert os.environ.get("WHATSAPP_ALLOWED_USERS") in (None, "")
|
||||
assert "15551234567" not in (adapter._allow_from or set())
|
||||
assert adapter._is_dm_intake_allowed(sender) is False
|
||||
assert adapter._is_dm_allowed(sender) is False
|
||||
assert runner._is_user_authorized(
|
||||
SessionSource(
|
||||
platform=Platform.WHATSAPP,
|
||||
user_id=sender,
|
||||
chat_id=sender,
|
||||
user_name="revoked",
|
||||
chat_type="dm",
|
||||
)
|
||||
) is False
|
||||
|
|
|
|||
Loading…
Reference in New Issue