Restrict Hackbot triggers to authorized user with editbugs permissions

libs/phabricator-client/phabricator_client/client.py

@@ -115,6 +115,16 @@ async def search_users(self, phids: list[str]) -> dict[str, dict]:
             if user.get("phid")
         }
 
+    async def get_project_members(self, project_phid: str) -> frozenset[str]:
+        """Return the user PHIDs belonging to a Phabricator project."""
+        result = await self.conduit_request(
+            "project.search",
+            constraints={"phids": [project_phid]},
+            attachments={"members": True},
+        )
+        members = result["data"][0]["attachments"]["members"]["members"]
+        return frozenset(member["phid"] for member in members)
+
     async def query_latest_diff(self, revision_id: int) -> PhabricatorDiff | None:
         """The most recent diff for a revision, or ``None`` if it has none.
 

libs/phabricator-client/tests/test_client.py

@@ -172,6 +172,33 @@ async def test_search_users_skips_call_when_nothing_to_resolve(monkeypatch):
     assert captured == {}  # no request was made
 
 
+async def test_get_project_members(monkeypatch):
+    captured = _capture_post(
+        monkeypatch,
+        {
+            "result": {
+                "data": [
+                    {
+                        "attachments": {
+                            "members": {
+                                "members": [
+                                    {"phid": "PHID-USER-1"},
+                                    {"phid": "PHID-USER-2"},
+                                ]
+                            }
+                        }
+                    }
+                ]
+            }
+        },
+    )
+    assert await _client().get_project_members("PHID-PROJ-1") == frozenset(
+        {"PHID-USER-1", "PHID-USER-2"}
+    )
+    assert captured["params"]["constraints"] == {"phids": ["PHID-PROJ-1"]}
+    assert captured["params"]["attachments"] == {"members": True}
+
+
 async def test_query_latest_diff_picks_highest_id(monkeypatch):
     _capture_post(
         monkeypatch,

services/hackbot-api/app/phabricator_authorization.py

@@ -0,0 +1,69 @@
+"""Authorization checks for Phabricator webhook authors."""
+
+from __future__ import annotations
+
+import asyncio
+import time
+from typing import TYPE_CHECKING
+
+from cachetools import TTLCache
+
+if TYPE_CHECKING:
+    from phabricator_client import PhabricatorClient
+
+
+class PhabricatorAuthorizer:
+    """Cache-backed authorization checks against a Phabricator project."""
+
+    def __init__(
+        self,
+        group_phid: str,
+        *,
+        cache_ttl_seconds: int = 300,
+        missing_member_refresh_cooldown_seconds: int = 30,
+    ) -> None:
+        self._group_phid = group_phid
+        self._members_cache: TTLCache[str, frozenset[str]] = TTLCache(
+            maxsize=1,
+            ttl=cache_ttl_seconds,
+        )
+        self._members_lock = asyncio.Lock()
+        self._last_members_refresh = 0.0
+        self._missing_member_refresh_cooldown_seconds = (
+            missing_member_refresh_cooldown_seconds
+        )
+
+    async def is_authorized(self, client: PhabricatorClient, author_phid: str) -> bool:
+        """Return whether an author belongs to the authorized project.
+
+        Known members use the cached project snapshot. An unknown author causes
+        one refresh so recently added members take effect promptly. Subsequent
+        unknown authors use a short cooldown to avoid a Phabricator request for
+        every unauthorized webhook delivery.
+        """
+        cached_members = self._members_cache.get(self._group_phid)
+        if cached_members is not None and author_phid in cached_members:
+            return True
+
+        async with self._members_lock:
+            cached_members = self._members_cache.get(self._group_phid)
+            if cached_members is not None and author_phid in cached_members:
+                return True
+
+            now = time.monotonic()
+            if (
+                cached_members is not None
+                and now - self._last_members_refresh
+                < self._missing_member_refresh_cooldown_seconds
+            ):
+                return False
+
+            members = await client.get_project_members(self._group_phid)
+            self._members_cache[self._group_phid] = members
+            self._last_members_refresh = time.monotonic()
+            return author_phid in members
+
+
+# Members of this project are authorized to trigger Hackbot.
+AUTHORIZED_GROUP_PHID = "PHID-PROJ-njo5uuqyyq3oijbkhy55"  # bmo-editbugs-team
+phabricator_authorizer = PhabricatorAuthorizer(AUTHORIZED_GROUP_PHID)

services/hackbot-api/app/phabricator_webhook.py

@@ -9,8 +9,11 @@
 from __future__ import annotations
 
 import logging
+from dataclasses import dataclass
 from typing import TYPE_CHECKING
 
+from app.phabricator_authorization import phabricator_authorizer
+
 if TYPE_CHECKING:
     from phabricator_client import PhabricatorClient
 
@@ -22,6 +25,12 @@
 _COMMENT_TYPES = frozenset({"comment", "inline"})
 
 
+@dataclass(frozen=True)
+class HackbotMention:
+    raw: str
+    author_phid: str
+
+
 def triggering_transaction_phids(payload: dict) -> list[str]:
     """The transaction PHIDs this delivery is about (from the webhook body)."""
     return [
@@ -37,27 +46,35 @@ def find_hackbot_mentions(
     *,
     bot_phid: str,
     token: str,
-) -> list[str]:
-    """Return the text of every triggering comment that mentions ``token``.
+) -> list[HackbotMention]:
+    """Return every triggering comment that mentions ``token``.
 
     Only considers transactions named in this delivery, of a comment type, not
     authored by the bot itself (loop prevention). A single review can leave
     several inline comments (each its own transaction), so all matches are
     returned, in transaction order. At most one per transaction: a transaction's
     ``comments`` list is that comment's version history, not distinct comments.
     """
-    matches: list[str] = []
+    matches: list[HackbotMention] = []
     for transaction in transactions:
         if transaction.get("phid") not in triggering_phids:
             continue
         if transaction.get("type") not in _COMMENT_TYPES:
             continue
-        if bot_phid and transaction.get("authorPHID") == bot_phid:
+        author_phid = transaction.get("authorPHID")
+        if not author_phid:
+            continue
+        if bot_phid and author_phid == bot_phid:
             continue
         for comment in transaction.get("comments") or []:
             raw = (comment.get("content") or {}).get("raw") or ""
             if token in raw:
-                matches.append(raw)
+                matches.append(
+                    HackbotMention(
+                        raw=raw,
+                        author_phid=author_phid,
+                    )
+                )
                 break
     return matches
 
@@ -111,15 +128,29 @@ async def detect_mention_and_revision(
     revision can't be resolved, or it has no Bugzilla bug id (bug-fix needs one).
     """
     transactions = await client.search_transactions(object_phid)
-    comments = find_hackbot_mentions(
+    mentions = find_hackbot_mentions(
         transactions,
         set(triggering_phids),
         bot_phid=webhook.bot_phid,
         token=webhook.mention_token,
     )
+    comments: list[str] = []
+    for mention in mentions:
+        if await phabricator_authorizer.is_authorized(
+            client,
+            mention.author_phid,
+        ):
+            comments.append(mention.raw)
+        else:
+            log.warning(
+                "Ignoring %s mention from non-editbugs user %s on %s",
+                webhook.mention_token,
+                mention.author_phid,
+                object_phid,
+            )
     if not comments:
         log.warning(
-            "No %s mention found in triggering transactions %s on %s",
+            "No actionable %s mention found in triggering transactions %s on %s",
             webhook.mention_token,
             triggering_phids,
             object_phid,

services/hackbot-api/tests/test_phabricator_authorization.py

@@ -0,0 +1,28 @@
+"""Tests for Phabricator webhook author authorization."""
+
+from unittest.mock import AsyncMock
+
+from app.phabricator_authorization import PhabricatorAuthorizer
+
+
+class _FakeClient:
+    def __init__(self, members: frozenset[str]) -> None:
+        self.get_project_members = AsyncMock(return_value=members)
+
+
+async def test_is_authorized_uses_cached_member_list():
+    authorizer = PhabricatorAuthorizer("PHID-PROJ-test")
+    client = _FakeClient(frozenset({"PHID-USER-authorized"}))
+
+    assert await authorizer.is_authorized(client, "PHID-USER-authorized") is True
+    assert await authorizer.is_authorized(client, "PHID-USER-authorized") is True
+    client.get_project_members.assert_awaited_once_with("PHID-PROJ-test")
+
+
+async def test_is_authorized_refreshes_once_for_unknown_authors():
+    authorizer = PhabricatorAuthorizer("PHID-PROJ-test")
+    client = _FakeClient(frozenset({"PHID-USER-authorized"}))
+
+    assert await authorizer.is_authorized(client, "PHID-USER-unknown-one") is False
+    assert await authorizer.is_authorized(client, "PHID-USER-unknown-two") is False
+    client.get_project_members.assert_awaited_once_with("PHID-PROJ-test")

services/hackbot-api/tests/test_webhooks.py

@@ -14,8 +14,11 @@
 from app.auth import verify_phabricator_signature
 from app.config import settings
 from app.main import app
+from app.phabricator_authorization import AUTHORIZED_GROUP_PHID
 from app.phabricator_webhook import (
+    HackbotMention,
     _join_comments,
+    detect_mention_and_revision,
     find_hackbot_mentions,
     resolve_revision,
     triggering_transaction_phids,
@@ -70,7 +73,7 @@ def test_find_mention_matches():
     txns = [_comment_txn("PHID-XACT-1", "PHID-USER-a", "hey @hackbot please fix")]
     assert find_hackbot_mentions(
         txns, {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot"
-    ) == ["hey @hackbot please fix"]
+    ) == [HackbotMention("hey @hackbot please fix", "PHID-USER-a")]
 
 
 def test_find_mention_no_token():
@@ -120,7 +123,7 @@ def test_find_mention_matches_inline_comment():
     ]
     assert find_hackbot_mentions(
         txns, {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot"
-    ) == ["@hackbot here"]
+    ) == [HackbotMention("@hackbot here", "PHID-USER-a")]
 
 
 def test_find_mention_collects_all_inline_matches():
@@ -136,7 +139,10 @@ def test_find_mention_collects_all_inline_matches():
         {"PHID-XACT-1", "PHID-XACT-2", "PHID-XACT-3"},
         bot_phid="PHID-USER-bot",
         token="@hackbot",
-    ) == ["@hackbot fix this", "@hackbot and this too"]
+    ) == [
+        HackbotMention("@hackbot fix this", "PHID-USER-a"),
+        HackbotMention("@hackbot and this too", "PHID-USER-a"),
+    ]
 
 
 def test_find_mention_one_per_transaction_ignores_comment_versions():
@@ -153,7 +159,7 @@ def test_find_mention_one_per_transaction_ignores_comment_versions():
     }
     assert find_hackbot_mentions(
         [txn], {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot"
-    ) == ["@hackbot v1"]
+    ) == [HackbotMention("@hackbot v1", "PHID-USER-a")]
 
 
 def test_join_comments_single_passthrough():
@@ -170,12 +176,20 @@ def test_join_comments_numbers_multiple():
 
 
 class _FakeClient:
-    def __init__(self, revision):
+    def __init__(self, revision, members=()):
         self._revision = revision
+        self._members = frozenset(members)
+
+    async def search_transactions(self, phid):
+        return []
 
     async def search_revision(self, phid):
         return self._revision
 
+    async def get_project_members(self, project_phid):
+        assert project_phid == AUTHORIZED_GROUP_PHID
+        return self._members
+
 
 async def test_resolve_revision_with_bug():
     client = _FakeClient({"id": 42, "fields": {"bugzilla.bug-id": "12345"}})
@@ -192,6 +206,52 @@ async def test_resolve_revision_not_found():
     assert await resolve_revision(client, "PHID-DREV-x") == (None, None)
 
 
+async def test_detect_mention_requires_editbugs_membership(monkeypatch):
+    client = _FakeClient(
+        {"id": 42, "fields": {"bugzilla.bug-id": "12345"}},
+        members={"PHID-USER-authorized"},
+    )
+    transactions = [
+        _comment_txn("PHID-XACT-1", "PHID-USER-unauthorized", "@hackbot ignore"),
+    ]
+    monkeypatch.setattr(
+        client,
+        "search_transactions",
+        AsyncMock(return_value=transactions),
+    )
+
+    result = await detect_mention_and_revision(
+        client,
+        settings.webhook,
+        "PHID-DREV-x",
+        ["PHID-XACT-1"],
+    )
+    assert result is None
+
+
+async def test_detect_mention_accepts_editbugs_member(monkeypatch):
+    client = _FakeClient(
+        {"id": 42, "fields": {"bugzilla.bug-id": "12345"}},
+        members={"PHID-USER-authorized"},
+    )
+    transactions = [
+        _comment_txn("PHID-XACT-1", "PHID-USER-authorized", "@hackbot please fix"),
+    ]
+    monkeypatch.setattr(
+        client,
+        "search_transactions",
+        AsyncMock(return_value=transactions),
+    )
+
+    result = await detect_mention_and_revision(
+        client,
+        settings.webhook,
+        "PHID-DREV-x",
+        ["PHID-XACT-1"],
+    )
+    assert result == ("@hackbot please fix", 42, 12345)
+
+
 # --- payload parsing ---