From 4d7c9b5eec24993f7fc2107caa96b0fadd4ed5e1 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Fri, 31 Jul 2026 15:55:17 +0200 Subject: [PATCH 1/6] Authorize Hackbot mentions by editbugs membership --- .../phabricator_client/client.py | 13 ++++ libs/phabricator-client/tests/test_client.py | 32 +++++++++ .../hackbot-api/app/phabricator_webhook.py | 40 +++++++++-- services/hackbot-api/tests/test_webhooks.py | 67 +++++++++++++++++-- 4 files changed, 141 insertions(+), 11 deletions(-) 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..6837dc6917 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -9,6 +9,7 @@ from __future__ import annotations import logging +from dataclasses import dataclass from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -20,6 +21,13 @@ # Transaction types that carry a comment we can scan for the mention. _COMMENT_TYPES = frozenset({"comment", "inline"}) +EDITBUGS_GROUP_PHID = "PHID-PROJ-njo5uuqyyq3oijbkhy55" + + +@dataclass(frozen=True) +class HackbotMention: + raw: str + author_phid: str def triggering_transaction_phids(payload: dict) -> list[str]: @@ -37,8 +45,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,7 +54,7 @@ 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 @@ -57,7 +65,12 @@ def find_hackbot_mentions( 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=transaction.get("authorPHID", ""), + ) + ) break return matches @@ -111,13 +124,13 @@ 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, ) - if not comments: + if not mentions: log.warning( "No %s mention found in triggering transactions %s on %s", webhook.mention_token, @@ -125,6 +138,21 @@ async def detect_mention_and_revision( object_phid, ) return None + + authorized_members = await client.get_project_members(EDITBUGS_GROUP_PHID) + 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 or "", + object_phid, + ) + if not comments: + return None comment = _join_comments(comments) revision_id, bug_id = await resolve_revision(client, 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 --- From 141534efe63c6229dd9cd87cb1cb9fe4344e5e62 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sat, 1 Aug 2026 15:15:14 +0200 Subject: [PATCH 2/6] skip transaction without an authorPHID --- .../hackbot-api/app/phabricator_webhook.py | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index 6837dc6917..baa04397f7 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -60,7 +60,10 @@ def find_hackbot_mentions( 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 "" @@ -68,7 +71,7 @@ def find_hackbot_mentions( matches.append( HackbotMention( raw=raw, - author_phid=transaction.get("authorPHID", ""), + author_phid=author_phid, ) ) break @@ -130,15 +133,6 @@ async def detect_mention_and_revision( bot_phid=webhook.bot_phid, token=webhook.mention_token, ) - if not mentions: - log.warning( - "No %s mention found in triggering transactions %s on %s", - webhook.mention_token, - triggering_phids, - object_phid, - ) - return None - authorized_members = await client.get_project_members(EDITBUGS_GROUP_PHID) comments: list[str] = [] for mention in mentions: @@ -148,10 +142,16 @@ async def detect_mention_and_revision( log.warning( "Ignoring %s mention from non-editbugs user %s on %s", webhook.mention_token, - mention.author_phid or "", + mention.author_phid, object_phid, ) if not comments: + log.warning( + "No actionable %s mention found in triggering transactions %s on %s", + webhook.mention_token, + triggering_phids, + object_phid, + ) return None comment = _join_comments(comments) From 075c6ccd7baf00cb6ce5652ecf440b2c48b00789 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sat, 1 Aug 2026 16:44:04 +0200 Subject: [PATCH 3/6] Cache authorized Hackbot trigger members --- .../hackbot-api/app/phabricator_webhook.py | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index baa04397f7..d7852627be 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -8,10 +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 @@ -22,6 +26,15 @@ # 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) @@ -89,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]: @@ -133,7 +185,10 @@ async def detect_mention_and_revision( bot_phid=webhook.bot_phid, token=webhook.mention_token, ) - authorized_members = await client.get_project_members(EDITBUGS_GROUP_PHID) + 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: From 1281e68ddc813fefbcb67f17063984c387a75277 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sun, 2 Aug 2026 15:14:28 +0200 Subject: [PATCH 4/6] replace get by direct access --- libs/phabricator-client/phabricator_client/client.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/libs/phabricator-client/phabricator_client/client.py b/libs/phabricator-client/phabricator_client/client.py index 6268091d4b..4a00462777 100644 --- a/libs/phabricator-client/phabricator_client/client.py +++ b/libs/phabricator-client/phabricator_client/client.py @@ -122,11 +122,8 @@ async def get_project_members(self, project_phid: str) -> frozenset[str]: 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")) + 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. From 46d265c665b0edd62bc27196735684c2398a8b01 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sun, 2 Aug 2026 16:31:57 +0200 Subject: [PATCH 5/6] Encapsulate Phabricator author authorization --- libs/phabricator-client/tests/test_client.py | 5 -- .../app/phabricator_authorization.py | 69 +++++++++++++++++++ .../hackbot-api/app/phabricator_webhook.py | 62 ++--------------- .../tests/test_phabricator_authorization.py | 28 ++++++++ services/hackbot-api/tests/test_webhooks.py | 7 +- 5 files changed, 107 insertions(+), 64 deletions(-) create mode 100644 services/hackbot-api/app/phabricator_authorization.py create mode 100644 services/hackbot-api/tests/test_phabricator_authorization.py diff --git a/libs/phabricator-client/tests/test_client.py b/libs/phabricator-client/tests/test_client.py index bbd2ff2682..37baed0b14 100644 --- a/libs/phabricator-client/tests/test_client.py +++ b/libs/phabricator-client/tests/test_client.py @@ -199,11 +199,6 @@ async def test_get_project_members(monkeypatch): 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_authorization.py b/services/hackbot-api/app/phabricator_authorization.py new file mode 100644 index 0000000000..de13318801 --- /dev/null +++ b/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) diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index d7852627be..e3c0ae9eee 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -8,13 +8,11 @@ from __future__ import annotations -import asyncio import logging -import time from dataclasses import dataclass from typing import TYPE_CHECKING -from cachetools import TTLCache +from app.phabricator_authorization import phabricator_authorizer if TYPE_CHECKING: from phabricator_client import PhabricatorClient @@ -25,16 +23,6 @@ # 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) @@ -102,45 +90,6 @@ 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]: @@ -185,13 +134,12 @@ async def detect_mention_and_revision( 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: + if await phabricator_authorizer.is_authorized( + client, + mention.author_phid, + ): comments.append(mention.raw) else: log.warning( diff --git a/services/hackbot-api/tests/test_phabricator_authorization.py b/services/hackbot-api/tests/test_phabricator_authorization.py new file mode 100644 index 0000000000..5b0107bc05 --- /dev/null +++ b/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") diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index e1eb44c5db..f8c4a4a6da 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -14,8 +14,8 @@ 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 ( - EDITBUGS_GROUP_PHID, HackbotMention, _join_comments, detect_mention_and_revision, @@ -180,11 +180,14 @@ 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 == EDITBUGS_GROUP_PHID + assert project_phid == AUTHORIZED_GROUP_PHID return self._members From d1521fc10f876654861a56619283564c5a619d10 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Mon, 3 Aug 2026 13:18:25 +0200 Subject: [PATCH 6/6] Inject Phabricator authorization dependencies --- services/hackbot-api/app/config.py | 2 +- services/hackbot-api/app/main.py | 10 +++++ .../app/phabricator_authorization.py | 27 ++++++++------ .../hackbot-api/app/phabricator_webhook.py | 20 +++++----- services/hackbot-api/app/routers/webhooks.py | 14 +++++-- .../tests/test_phabricator_authorization.py | 12 +++--- services/hackbot-api/tests/test_webhooks.py | 37 +++++++++++++++---- 7 files changed, 81 insertions(+), 41 deletions(-) diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index ded3f8cdad..b28364af3b 100644 --- a/services/hackbot-api/app/config.py +++ b/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 diff --git a/services/hackbot-api/app/main.py b/services/hackbot-api/app/main.py index 080b7a076d..4a66714235 100644 --- a/services/hackbot-api/app/main.py +++ b/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) diff --git a/services/hackbot-api/app/phabricator_authorization.py b/services/hackbot-api/app/phabricator_authorization.py index de13318801..64b060f4bd 100644 --- a/services/hackbot-api/app/phabricator_authorization.py +++ b/services/hackbot-api/app/phabricator_authorization.py @@ -12,17 +12,23 @@ 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, - group_phid: str, + client: PhabricatorClient, + authorized_group_phid: str, *, cache_ttl_seconds: int = 300, missing_member_refresh_cooldown_seconds: int = 30, ) -> None: - self._group_phid = group_phid + self._client = client + self._authorized_group_phid = authorized_group_phid self._members_cache: TTLCache[str, frozenset[str]] = TTLCache( maxsize=1, ttl=cache_ttl_seconds, @@ -33,7 +39,7 @@ def __init__( missing_member_refresh_cooldown_seconds ) - async def is_authorized(self, client: PhabricatorClient, author_phid: str) -> bool: + 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 @@ -41,12 +47,12 @@ async def is_authorized(self, client: PhabricatorClient, author_phid: str) -> bo 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) + 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._group_phid) + cached_members = self._members_cache.get(self._authorized_group_phid) if cached_members is not None and author_phid in cached_members: return True @@ -58,12 +64,9 @@ async def is_authorized(self, client: PhabricatorClient, author_phid: str) -> bo ): return False - members = await client.get_project_members(self._group_phid) - self._members_cache[self._group_phid] = members + 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 - - -# Members of this project are authorized to trigger Hackbot. -AUTHORIZED_GROUP_PHID = "PHID-PROJ-njo5uuqyyq3oijbkhy55" # bmo-editbugs-team -phabricator_authorizer = PhabricatorAuthorizer(AUTHORIZED_GROUP_PHID) diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index e3c0ae9eee..b17b436e57 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -12,12 +12,11 @@ from dataclasses import dataclass from typing import TYPE_CHECKING -from app.phabricator_authorization import phabricator_authorizer - if TYPE_CHECKING: from phabricator_client import PhabricatorClient from app.config import WebhookSettings + from app.phabricator_authorization import PhabricatorAuthorizer log = logging.getLogger(__name__) @@ -27,7 +26,7 @@ @dataclass(frozen=True) class HackbotMention: - raw: str + comment: str author_phid: str @@ -67,11 +66,11 @@ def find_hackbot_mentions( 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: + comment_text = (comment.get("content") or {}).get("raw") or "" + if token in comment_text: matches.append( HackbotMention( - raw=raw, + comment=comment_text, author_phid=author_phid, ) ) @@ -116,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. @@ -136,11 +137,8 @@ async def detect_mention_and_revision( ) comments: list[str] = [] for mention in mentions: - if await phabricator_authorizer.is_authorized( - client, - mention.author_phid, - ): - comments.append(mention.raw) + 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", diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index 61db54ea73..aad4a8ad18 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/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,9 +26,9 @@ 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: @@ -35,6 +36,11 @@ def get_hackbot_client() -> HackbotClient: 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"} diff --git a/services/hackbot-api/tests/test_phabricator_authorization.py b/services/hackbot-api/tests/test_phabricator_authorization.py index 5b0107bc05..47d1e4976f 100644 --- a/services/hackbot-api/tests/test_phabricator_authorization.py +++ b/services/hackbot-api/tests/test_phabricator_authorization.py @@ -11,18 +11,18 @@ def __init__(self, members: frozenset[str]) -> None: async def test_is_authorized_uses_cached_member_list(): - authorizer = PhabricatorAuthorizer("PHID-PROJ-test") client = _FakeClient(frozenset({"PHID-USER-authorized"})) + authorizer = PhabricatorAuthorizer(client, "PHID-PROJ-test") - assert await authorizer.is_authorized(client, "PHID-USER-authorized") is True - assert await authorizer.is_authorized(client, "PHID-USER-authorized") is True + 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(): - authorizer = PhabricatorAuthorizer("PHID-PROJ-test") client = _FakeClient(frozenset({"PHID-USER-authorized"})) + authorizer = PhabricatorAuthorizer(client, "PHID-PROJ-test") - assert await authorizer.is_authorized(client, "PHID-USER-unknown-one") is False - assert await authorizer.is_authorized(client, "PHID-USER-unknown-two") is False + 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") diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index f8c4a4a6da..bc408f0a78 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -14,7 +14,10 @@ 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_authorization import ( + AUTHORIZED_GROUP_PHID, + PhabricatorAuthorizer, +) from app.phabricator_webhook import ( HackbotMention, _join_comments, @@ -225,6 +228,7 @@ async def test_detect_mention_requires_editbugs_membership(monkeypatch): settings.webhook, "PHID-DREV-x", ["PHID-XACT-1"], + authorizer=PhabricatorAuthorizer(client, AUTHORIZED_GROUP_PHID), ) assert result is None @@ -248,6 +252,7 @@ async def test_detect_mention_accepts_editbugs_member(monkeypatch): settings.webhook, "PHID-DREV-x", ["PHID-XACT-1"], + authorizer=PhabricatorAuthorizer(client, AUTHORIZED_GROUP_PHID), ) assert result == ("@hackbot please fix", 42, 12345) @@ -274,11 +279,28 @@ async def trigger_run(self, agent_name, inputs): return "run-abc" +class _FakeAuthorizer: + async def is_authorized(self, author_phid): + return True + + +@pytest.fixture +def authorizer(): + return _FakeAuthorizer() + + +@pytest.fixture +def phab_client(): + return object() + + @pytest.fixture -def client(monkeypatch): +def client(monkeypatch, authorizer, phab_client): monkeypatch.setattr(settings.webhook, "secret", SECRET) # Fresh dedupe cache per test. webhooks._seen_transactions.clear() + app.dependency_overrides[webhooks.get_phabricator_client] = lambda: phab_client + app.dependency_overrides[webhooks.get_phabricator_authorizer] = lambda: authorizer try: yield TestClient(app) finally: @@ -331,12 +353,9 @@ def test_route_ignores_no_mention(client, monkeypatch): assert resp.json()["reason"] == "no actionable @hackbot mention" -def test_route_triggers_run(client, monkeypatch): - monkeypatch.setattr( - webhooks, - "detect_mention_and_revision", - AsyncMock(return_value=("@hackbot please fix", 42, 12345)), - ) +def test_route_triggers_run(client, phab_client, authorizer, monkeypatch): + detect = AsyncMock(return_value=("@hackbot please fix", 42, 12345)) + monkeypatch.setattr(webhooks, "detect_mention_and_revision", detect) fake_api = _FakeHackbotClient() app.dependency_overrides[webhooks.get_hackbot_client] = lambda: fake_api @@ -349,6 +368,8 @@ def test_route_triggers_run(client, monkeypatch): ) assert resp.status_code == 202 assert resp.json() == {"status": "triggered", "run_id": "run-abc"} + assert detect.call_args.args[0] is phab_client + assert detect.call_args.kwargs["authorizer"] is authorizer assert fake_api.calls == [ ( "bug-fix",