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/config.py

@@ -45,7 +45,7 @@ class Settings(BaseSettings):
     # Phabricator Conduit connection config, embedded as a nested model and
     # populated in this single settings parse from PHABRICATOR_URL /
     # PHABRICATOR_API_KEY / PHABRICATOR_TIMEOUT_SECONDS (see env_nested_delimiter
-    # below). Injected directly as PhabricatorClient(settings.phabricator).
+    # below). Constructed once at application startup and injected as a dependency.
     # Required, so a missing/invalid api_key fails at startup.
     phabricator: PhabricatorSettings
 

services/hackbot-api/app/main.py

@@ -3,10 +3,15 @@
 
 import sentry_sdk
 from fastapi import FastAPI
+from phabricator_client import PhabricatorClient
 
 from app import __version__
 from app.config import settings
 from app.database.connection import close_db, init_db
+from app.phabricator_authorization import (
+    AUTHORIZED_GROUP_PHID,
+    PhabricatorAuthorizer,
+)
 from app.routers import events_router, runs_router, webhooks_router
 
 if settings.sentry_dsn:
@@ -38,6 +43,11 @@ async def lifespan(app: FastAPI):
     version=__version__,
     lifespan=lifespan,
 )
+app.state.phabricator_client = PhabricatorClient(settings.phabricator)
+app.state.phabricator_authorizer = PhabricatorAuthorizer(
+    app.state.phabricator_client,
+    AUTHORIZED_GROUP_PHID,
+)
 
 app.include_router(runs_router)
 app.include_router(events_router)

services/hackbot-api/app/phabricator_authorization.py

@@ -0,0 +1,72 @@
+"""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
+
+
+# Members of this project are authorized to trigger Hackbot.
+AUTHORIZED_GROUP_PHID = "PHID-PROJ-njo5uuqyyq3oijbkhy55"  # bmo-editbugs-team
+
+
+class PhabricatorAuthorizer:
+    """Cache-backed authorization checks against a Phabricator project."""
+
+    def __init__(
+        self,
+        client: PhabricatorClient,
+        authorized_group_phid: str,
+        *,
+        cache_ttl_seconds: int = 300,
+        missing_member_refresh_cooldown_seconds: int = 30,
+    ) -> None:
+        self._client = client
+        self._authorized_group_phid = authorized_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, 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._authorized_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._authorized_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 self._client.get_project_members(
+                self._authorized_group_phid
+            )
+            self._members_cache[self._authorized_group_phid] = members
+            self._last_members_refresh = time.monotonic()
+            return author_phid in members

services/hackbot-api/app/phabricator_webhook.py

@@ -9,19 +9,27 @@
 from __future__ import annotations
 
 import logging
+from dataclasses import dataclass
 from typing import TYPE_CHECKING
 
 if TYPE_CHECKING:
     from phabricator_client import PhabricatorClient
 
     from app.config import WebhookSettings
+    from app.phabricator_authorization import PhabricatorAuthorizer
 
 log = logging.getLogger(__name__)
 
 # Transaction types that carry a comment we can scan for the mention.
 _COMMENT_TYPES = frozenset({"comment", "inline"})
 
 
+@dataclass(frozen=True)
+class HackbotMention:
+    comment: 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 +45,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)
+            comment_text = (comment.get("content") or {}).get("raw") or ""
+            if token in comment_text:
+                matches.append(
+                    HackbotMention(
+                        comment=comment_text,
+                        author_phid=author_phid,
+                    )
+                )
                 break
     return matches
 
@@ -99,6 +115,8 @@ async def detect_mention_and_revision(
     webhook: WebhookSettings,
     object_phid: str,
     triggering_phids: list[str],
+    *,
+    authorizer: PhabricatorAuthorizer,
 ) -> tuple[str, int, int] | None:
     """Read Conduit and return ``(comment, revision_id, bug_id)`` or None.
 
@@ -111,15 +129,26 @@ 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 authorizer.is_authorized(mention.author_phid):
+            comments.append(mention.comment)
+        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/app/routers/webhooks.py

@@ -15,6 +15,7 @@
 from app.auth import require_phabricator_signature
 from app.client import HackbotClient
 from app.config import settings
+from app.phabricator_authorization import PhabricatorAuthorizer
 from app.phabricator_webhook import (
     detect_mention_and_revision,
     triggering_transaction_phids,
@@ -25,16 +26,21 @@
 router = APIRouter(prefix="/webhooks")
 
 
-def get_phabricator_client() -> PhabricatorClient:
-    """Dependency: a Conduit client built from the service's Phabricator config."""
-    return PhabricatorClient(settings.phabricator)
+def get_phabricator_client(request: Request) -> PhabricatorClient:
+    """Dependency: the app-scoped Conduit client."""
+    return request.app.state.phabricator_client
 
 
 def get_hackbot_client() -> HackbotClient:
     """Dependency: a client for triggering runs over the public hackbot API."""
     return HackbotClient(settings.hackbot_api_url, settings.external_api_key)
 
 
+def get_phabricator_authorizer(request: Request) -> PhabricatorAuthorizer:
+    """Dependency: the app-scoped authorizer with its shared member cache."""
+    return request.app.state.phabricator_authorizer
+
+
 # Best-effort dedupe of retried deliveries, keyed by triggering transaction PHID.
 # Per-instance and reset on restart; a durable dedupe (using the DB) can replace
 # this if needed. Sized well above the number of mentions expected in a window.
@@ -51,6 +57,7 @@ def get_hackbot_client() -> HackbotClient:
 async def phabricator_webhook(
     request: Request,
     phab_client: PhabricatorClient = Depends(get_phabricator_client),
+    authorizer: PhabricatorAuthorizer = Depends(get_phabricator_authorizer),
     api_client: HackbotClient = Depends(get_hackbot_client),
 ) -> dict:
     payload = await request.json()
@@ -82,6 +89,7 @@ async def phabricator_webhook(
         settings.webhook,
         object_phid,
         fresh,
+        authorizer=authorizer,
     )
     if detected is None:
         return {"status": "ignored", "reason": "no actionable @hackbot mention"}

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():
+    client = _FakeClient(frozenset({"PHID-USER-authorized"}))
+    authorizer = PhabricatorAuthorizer(client, "PHID-PROJ-test")
+
+    assert await authorizer.is_authorized("PHID-USER-authorized") is True
+    assert await authorizer.is_authorized("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():
+    client = _FakeClient(frozenset({"PHID-USER-authorized"}))
+    authorizer = PhabricatorAuthorizer(client, "PHID-PROJ-test")
+
+    assert await authorizer.is_authorized("PHID-USER-unknown-one") is False
+    assert await authorizer.is_authorized("PHID-USER-unknown-two") is False
+    client.get_project_members.assert_awaited_once_with("PHID-PROJ-test")