Browse Source

Resolve foreign mentions locally, without any admin-directory call

A bot account without admin permissions got permission_denied from
admin_get_member on EVERY handle lookup; the fail-open fallthrough let
the bot answer every @colleague message in an open channel. The room
roster fetched for the very same dispatch already knows the logins, so
handle resolution now reads the local member cache only, and the OPEN
admission path fetches the roster projection before the someone-else
check so the cache is guaranteed filled.
Liv Klumpp 2 weeks ago
parent
commit
d6b8ba7e95
2 changed files with 52 additions and 38 deletions
  1. 25 34
      adapter.py
  2. 27 4
      test_adapter.py

+ 25 - 34
adapter.py

@@ -429,8 +429,6 @@ class ChattoAdapter(BasePlatformAdapter):
 
 
         # Member directory cache: user_id -> user info dict
         # Member directory cache: user_id -> user info dict
         self._user_cache: dict[str, User] = {}
         self._user_cache: dict[str, User] = {}
-        # Handle -> does a user hold it. Cached both ways; see _mentions_someone_else.
-        self._known_handles: dict[str, bool] = {}
 
 
         # Chattolib client cache and lock for async access.
         # Chattolib client cache and lock for async access.
         self._chatto_client: ChattoClient | None = None
         self._chatto_client: ChattoClient | None = None
@@ -730,39 +728,25 @@ class ChattoAdapter(BasePlatformAdapter):
     async def _handle_belongs_to_a_user(self, handle: str) -> bool:
     async def _handle_belongs_to_a_user(self, handle: str) -> bool:
         """Whether ``handle`` is the login of a real Chatto user.
         """Whether ``handle`` is the login of a real Chatto user.
 
 
-        The API carries no mention entities — ``mention_confirmation_token`` is
-        reserved in the message descriptor — so an @-token is only a candidate
-        until the directory confirms it. Results are cached both ways under the
-        lowercased handle (the mention namespace is case-insensitive per
-        FDR-006), since the same handles recur and a miss is as reusable as a
-        hit. Only logins are looked up: matching another user's display name,
-        as the web frontend does against its room member list, has no directory
-        equivalent here.
+        Resolved strictly from local knowledge — the member cache the adapter
+        already keeps — never from the admin directory: ``admin_get_member``
+        needs admin permissions a bot account must not be assumed to hold,
+        and in the field its ``permission_denied`` failed every lookup, which
+        fail-open let the bot answer every @colleague message. The cache is
+        filled by the admission pipeline itself (room rosters are fetched for
+        every channel turn), presence patching and the edited-message path.
+
+        An unknown handle is "not confirmed", not "confirmed absent": Chatto
+        carries no mention entities (``mention_confirmation_token`` is
+        reserved in the message descriptor), so a @-token stays a candidate
+        we cannot verify — and staying silent on a false positive is worse
+        than answering one.
         """
         """
-        cache_key = handle.lower()
-        known = self._known_handles.get(cache_key)
-        if known is not None:
-            return known
-        client = await self._get_chatto_client()
-        if client is None:
-            # Unresolved means "not confirmed", so the message goes through.
-            return False
-        try:
-            member = await client.admin_get_member(login=handle)
-        except Exception as exc:
-            logger.debug("Chatto: could not resolve handle @%s: %s", handle, exc)
-            return False
-        # admin_get_member returns pb_to_dict(GetMemberResponse):
-        # {"member": {"user": {...}, ...}, "roles": [...]} when found,
-        # or {} when not found.
-        if not member.get("member"):
-            self._known_handles[cache_key] = False
-            return False
-        from chattolib.types import User
-
-        user = User.parse(member["member"].get("user"))
-        self._known_handles[cache_key] = user is not None
-        return user is not None
+        lowered = handle.lower()
+        return any(
+            user is not None and (user.login or "").lower() == lowered
+            for user in self._user_cache.values()
+        )
 
 
     async def _mentions_someone_else(self, body: str) -> bool:
     async def _mentions_someone_else(self, body: str) -> bool:
         """Whether the message @-mentions a person who is not this bot.
         """Whether the message @-mentions a person who is not this bot.
@@ -2014,6 +1998,13 @@ class ChattoAdapter(BasePlatformAdapter):
                 )
                 )
                 return None
                 return None
 
 
+            # Handle resolution reads the member cache, which the roster
+            # projection fills. Fetch it now so the someone-else check below
+            # sees the room's members — the same projection is reused for
+            # this turn's channel_context further down, one fetch per room.
+            if policy == RoomPolicy.OPEN:
+                await self._roster_for_room(client, room_id=room_id)
+
             # In an open channel we see every message, including ones plainly
             # In an open channel we see every message, including ones plainly
             # aimed at a named colleague. Answering those would be barging in,
             # aimed at a named colleague. Answering those would be barging in,
             # so acknowledge that we read it and stay quiet. Checked after the
             # so acknowledge that we read it and stay quiet. Checked after the

+ 27 - 4
test_adapter.py

@@ -1400,12 +1400,13 @@ class TestForeignMention:
         adapter.handle_message.assert_called_once()
         adapter.handle_message.assert_called_once()
 
 
     async def test_broadcast_handles_are_shared_across_cases_in_the_cache(self):
     async def test_broadcast_handles_are_shared_across_cases_in_the_cache(self):
-        """The directory cache keys on the lowercased handle: one lookup
-        serves '@bob' and '@BOB' alike."""
+        """The mention namespace is case-insensitive (FDR-006): '@BOB' and
+        '@bob' are the same person, and local resolution matches both."""
         adapter = self._adapter()
         adapter = self._adapter()
         await self._dispatch(adapter, "@BOB ping")
         await self._dispatch(adapter, "@BOB ping")
         await self._dispatch(adapter, "@bob again")
         await self._dispatch(adapter, "@bob again")
-        assert adapter._chatto_client.admin_get_member.await_count == 1
+        assert adapter._chatto_client.admin_get_member.await_count == 0
+        assert adapter.handle_message.await_count == 0
 
 
     async def test_talking_about_mentions_is_not_a_mention(self):
     async def test_talking_about_mentions_is_not_a_mention(self):
         """Verbatim from the field: the instruction to send a mention later must
         """Verbatim from the field: the instruction to send a mention later must
@@ -1432,9 +1433,31 @@ class TestForeignMention:
         adapter = self._adapter()
         adapter = self._adapter()
         await self._dispatch(adapter, "@bob ping")
         await self._dispatch(adapter, "@bob ping")
         await self._dispatch(adapter, "@bob again")
         await self._dispatch(adapter, "@bob again")
-        assert adapter._chatto_client.admin_get_member.await_count == 1
+        assert adapter._chatto_client.admin_get_member.await_count == 0
         assert adapter.handle_message.await_count == 0
         assert adapter.handle_message.await_count == 0
 
 
+    async def test_bot_without_admin_rights_resolves_via_local_knowledge(self):
+        """From the field: a bot account without admin permissions gets
+        permission_denied from the admin directory for EVERY lookup, and the
+        fail-open fallthrough made the bot answer every @colleague message.
+        The roster projection of the very same dispatch already knows the
+        login, so a room member must be resolved locally without any
+        directory call at all."""
+        adapter = self._adapter()
+        adapter._chatto_client.admin_get_member = AsyncMock(
+            side_effect=PermissionError("permission_denied: permission denied")
+        )
+        adapter._chatto_client.list_room_members = AsyncMock(
+            return_value=(["user-1"], Page())
+        )
+        adapter._chatto_client.batch_get_room_members = AsyncMock(
+            return_value=[DirectoryMember(user=_make_user("user-1", "nickk"))]
+        )
+        await self._dispatch(adapter, "@nickk ein Test wegen Replies")
+        adapter.handle_message.assert_not_called()
+        adapter.add_reaction.assert_awaited_once()
+        adapter._chatto_client.admin_get_member.assert_not_awaited()
+
     async def test_an_email_address_is_not_a_mention(self):
     async def test_an_email_address_is_not_a_mention(self):
         adapter = self._adapter()
         adapter = self._adapter()
         await self._dispatch(adapter, "schreib an bob@example.com")
         await self._dispatch(adapter, "schreib an bob@example.com")