mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-05-22 09:32:33 +00:00
fix(signal): consult pairing store in is_allowed
BaseChannel.is_allowed ORs is_approved (the pairing store) into the allow decision; the signal override dropped that step and only looked at config.allow_from. With the new DM-pairing flow in place, an approved-via-pairing sender's next message would have failed the allow check and triggered another pairing code in a loop. OR in a normalized check against the pairing store: walk each part of the pipe-joined sender_id through _normalize_signal_id and call is_approved for each variant, so an approval stored under one form (phone with/without "+", UUID/ACI) still matches when the next inbound uses a different form. Mirrors how slack.py:643 handles it. Also tightens the empty-allowlist warning to only fire when nothing else granted access, since pairing-store hits are now a valid path. Not part of the original review, but Comments 2 and 3 turn this latent gap into a broken round-trip — included so the pairing UX actually works.
This commit is contained in:
parent
82dfe8c1f7
commit
b3d0d24a52
@ -22,6 +22,7 @@ from nanobot.bus.queue import MessageBus
|
|||||||
from nanobot.channels.base import BaseChannel
|
from nanobot.channels.base import BaseChannel
|
||||||
from nanobot.config.paths import get_media_dir
|
from nanobot.config.paths import get_media_dir
|
||||||
from nanobot.config.schema import Base
|
from nanobot.config.schema import Base
|
||||||
|
from nanobot.pairing import is_approved
|
||||||
from nanobot.utils.helpers import safe_filename, split_message
|
from nanobot.utils.helpers import safe_filename, split_message
|
||||||
|
|
||||||
|
|
||||||
@ -375,12 +376,28 @@ class SignalChannel(BaseChannel):
|
|||||||
matches the per-policy DM gate.
|
matches the per-policy DM gate.
|
||||||
"""
|
"""
|
||||||
allow_list = self.config.allow_from
|
allow_list = self.config.allow_from
|
||||||
if not allow_list:
|
|
||||||
self.logger.warning("allow_from is empty — all access denied")
|
|
||||||
return False
|
|
||||||
if "*" in allow_list:
|
if "*" in allow_list:
|
||||||
return True
|
return True
|
||||||
return self._sender_matches_allowlist(sender_id, allow_list)
|
if self._sender_matches_allowlist(sender_id, allow_list):
|
||||||
|
return True
|
||||||
|
if self._sender_approved_via_pairing(sender_id):
|
||||||
|
return True
|
||||||
|
if not allow_list:
|
||||||
|
self.logger.warning("allow_from is empty — all access denied")
|
||||||
|
return False
|
||||||
|
|
||||||
|
def _sender_approved_via_pairing(self, sender_id: str) -> bool:
|
||||||
|
"""Return True if any normalized variant of sender_id is in the pairing store.
|
||||||
|
|
||||||
|
Pairing approval may be recorded under any of the identifier forms
|
||||||
|
signal exposes (phone with/without ``+``, UUID, ACI), so we check
|
||||||
|
each part of the pipe-joined composite against ``is_approved``.
|
||||||
|
"""
|
||||||
|
for part in str(sender_id).split("|"):
|
||||||
|
for variant in self._normalize_signal_id(part):
|
||||||
|
if is_approved(self.name, variant):
|
||||||
|
return True
|
||||||
|
return False
|
||||||
|
|
||||||
async def start(self) -> None:
|
async def start(self) -> None:
|
||||||
"""Start the Signal channel and connect to signal-cli daemon."""
|
"""Start the Signal channel and connect to signal-cli daemon."""
|
||||||
|
|||||||
@ -681,6 +681,22 @@ class TestHandleDataMessageDM:
|
|||||||
assert handled[0]["is_dm"] is True
|
assert handled[0]["is_dm"] is True
|
||||||
assert handled[0]["chat_id"] == "+19995550002"
|
assert handled[0]["chat_id"] == "+19995550002"
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_dm_paired_sender_allowed_without_allowlist_entry(self, monkeypatch):
|
||||||
|
# Once a sender completes pairing they should pass is_allowed on every
|
||||||
|
# subsequent message — otherwise the pairing reply loops forever.
|
||||||
|
approved = {"+19995550002"}
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"nanobot.channels.signal.is_approved",
|
||||||
|
lambda channel, sender_id: sender_id in approved,
|
||||||
|
)
|
||||||
|
ch = _make_channel(dm_enabled=True, dm_policy="allowlist", dm_allow_from=[])
|
||||||
|
assert ch.is_allowed("+19995550002") is True
|
||||||
|
# Variant forms (with/without "+") must still match a stored approval.
|
||||||
|
assert ch.is_allowed("19995550002") is True
|
||||||
|
# Unpaired sender stays denied.
|
||||||
|
assert ch.is_allowed("+19995559999") is False
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_dm_allowlist_matches_without_plus_prefix(self):
|
async def test_dm_allowlist_matches_without_plus_prefix(self):
|
||||||
"""An allowlist entry without '+' must match a sender that carries '+'."""
|
"""An allowlist entry without '+' must match a sender that carries '+'."""
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user