diff --git a/libs/phabricator-client/phabricator_client/client.py b/libs/phabricator-client/phabricator_client/client.py index 91950f5701..6268091d4b 100644 --- a/libs/phabricator-client/phabricator_client/client.py +++ b/libs/phabricator-client/phabricator_client/client.py @@ -115,6 +115,19 @@ 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}, + ) + data = result.get("data") or [] + if not data: + return frozenset() + members = data[0].get("attachments", {}).get("members", {}).get("members", []) + return frozenset(member["phid"] for member in members if member.get("phid")) + async def query_latest_diff(self, revision_id: int) -> PhabricatorDiff | None: """The most recent diff for a revision, or ``None`` if it has none. diff --git a/libs/phabricator-client/tests/test_client.py b/libs/phabricator-client/tests/test_client.py index 9f9cf5d626..bbd2ff2682 100644 --- a/libs/phabricator-client/tests/test_client.py +++ b/libs/phabricator-client/tests/test_client.py @@ -172,6 +172,38 @@ 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_get_project_members_missing_project(monkeypatch): + _capture_post(monkeypatch, {"result": {"data": []}}) + assert await _client().get_project_members("PHID-PROJ-1") == frozenset() + + async def test_query_latest_diff_picks_highest_id(monkeypatch): _capture_post( monkeypatch, diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index ef4a877333..d7852627be 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -8,9 +8,14 @@ from __future__ import annotations +import asyncio import logging +import time +from dataclasses import dataclass from typing import TYPE_CHECKING +from cachetools import TTLCache + if TYPE_CHECKING: from phabricator_client import PhabricatorClient @@ -20,6 +25,22 @@ # Transaction types that carry a comment we can scan for the mention. _COMMENT_TYPES = frozenset({"comment", "inline"}) +EDITBUGS_GROUP_PHID = "PHID-PROJ-njo5uuqyyq3oijbkhy55" +_MEMBERSHIP_CACHE_TTL_SECONDS = 60 +_MISSING_MEMBER_REFRESH_COOLDOWN_SECONDS = 30 + +_editbugs_members_cache: TTLCache[str, frozenset[str]] = TTLCache( + maxsize=1, + ttl=_MEMBERSHIP_CACHE_TTL_SECONDS, +) +_editbugs_members_lock = asyncio.Lock() +_last_editbugs_members_refresh = 0.0 + + +@dataclass(frozen=True) +class HackbotMention: + raw: str + author_phid: str def triggering_transaction_phids(payload: dict) -> list[str]: @@ -37,8 +58,8 @@ 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 @@ -46,18 +67,26 @@ def find_hackbot_mentions( 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 @@ -73,6 +102,45 @@ def _join_comments(comments: list[str]) -> str: return "\n\n".join(f"[comment {i}]\n{c}" for i, c in enumerate(comments, 1)) +async def get_authorized_members( + client: PhabricatorClient, author_phids: set[str] +) -> frozenset[str]: + """Return the members authorized to trigger Hackbot for these authors. + + Known authors use the cached project-members snapshot. If any author is + absent, refresh the snapshot once so newly added members take effect without + waiting for the TTL. The cooldown prevents repeated unauthorized mentions + from issuing a Phabricator request for every delivery. + """ + global _last_editbugs_members_refresh + + if not author_phids: + return frozenset() + + cached_members = _editbugs_members_cache.get(EDITBUGS_GROUP_PHID) + if cached_members is not None and author_phids.issubset(cached_members): + return cached_members + + # A shared async lock, so concurrent webhook requests do not issue duplicate refreshes. + async with _editbugs_members_lock: + cached_members = _editbugs_members_cache.get(EDITBUGS_GROUP_PHID) + if cached_members is not None and author_phids.issubset(cached_members): + return cached_members + + now = time.monotonic() + if ( + cached_members is not None + and now - _last_editbugs_members_refresh + < _MISSING_MEMBER_REFRESH_COOLDOWN_SECONDS + ): + return cached_members + + members = await client.get_project_members(EDITBUGS_GROUP_PHID) + _editbugs_members_cache[EDITBUGS_GROUP_PHID] = members + _last_editbugs_members_refresh = time.monotonic() + return members + + async def resolve_revision( client: PhabricatorClient, revision_phid: str ) -> tuple[int | None, int | None]: @@ -111,15 +179,30 @@ 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, ) + authorized_members = await get_authorized_members( + client, + {mention.author_phid for mention in mentions}, + ) + comments: list[str] = [] + for mention in mentions: + if mention.author_phid in authorized_members: + 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, diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index e21eae76b3..e1eb44c5db 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -15,7 +15,10 @@ from app.config import settings from app.main import app from app.phabricator_webhook import ( + EDITBUGS_GROUP_PHID, + 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,17 @@ 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_revision(self, phid): return self._revision + async def get_project_members(self, project_phid): + assert project_phid == EDITBUGS_GROUP_PHID + return self._members + async def test_resolve_revision_with_bug(): client = _FakeClient({"id": 42, "fields": {"bugzilla.bug-id": "12345"}}) @@ -192,6 +203,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 ---