From bc01b46035304a1bf094c8a8ea2164d97ed19cb9 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Fri, 4 Sep 2026 14:42:54 +0200 Subject: [PATCH 1/6] Restrict Bugzilla webhook triggers to editbugs users --- .../hackbot-api/app/bugzilla_authorization.py | 52 ++++++++++++++ services/hackbot-api/app/bugzilla_client.py | 43 ++++++++++++ services/hackbot-api/app/bugzilla_webhook.py | 5 +- services/hackbot-api/app/config.py | 6 +- services/hackbot-api/app/routers/webhooks.py | 22 ++++++ services/hackbot-api/pyproject.toml | 1 + .../tests/test_bugzilla_authorization.py | 45 +++++++++++++ .../hackbot-api/tests/test_bugzilla_client.py | 67 +++++++++++++++++++ services/hackbot-api/tests/test_webhooks.py | 43 ++++++++++-- uv.lock | 2 + 10 files changed, 278 insertions(+), 8 deletions(-) create mode 100644 services/hackbot-api/app/bugzilla_authorization.py create mode 100644 services/hackbot-api/app/bugzilla_client.py create mode 100644 services/hackbot-api/tests/test_bugzilla_authorization.py create mode 100644 services/hackbot-api/tests/test_bugzilla_client.py diff --git a/services/hackbot-api/app/bugzilla_authorization.py b/services/hackbot-api/app/bugzilla_authorization.py new file mode 100644 index 0000000000..ebf940679d --- /dev/null +++ b/services/hackbot-api/app/bugzilla_authorization.py @@ -0,0 +1,52 @@ +"""Authorization checks for Bugzilla webhook actors.""" + +from __future__ import annotations + +import asyncio +from typing import TYPE_CHECKING + +from cachetools import TTLCache + +if TYPE_CHECKING: + from app.bugzilla_client import BugzillaUserClient + +AUTHORIZED_GROUP_ID = 9 # bmo-editbugs-team + + +class BugzillaAuthorizer: + """Cache-backed per-user authorization checks against a Bugzilla group.""" + + def __init__( + self, + client: BugzillaUserClient, + authorized_group_id: int, + *, + cache_ttl_seconds: int = 300, + cache_maxsize: int = 4096, + ) -> None: + self._client = client + self._authorized_group_id = authorized_group_id + self._cache: TTLCache[str, bool] = TTLCache( + maxsize=cache_maxsize, + ttl=cache_ttl_seconds, + ) + self._lock = asyncio.Lock() + + async def is_authorized(self, login: str) -> bool: + """Return whether a Bugzilla login belongs to the authorized group.""" + login = login.lower() + + cached = self._cache.get(login) + if cached is not None: + return cached + + async with self._lock: + cached = self._cache.get(login) + if cached is not None: + return cached + + authorized = await self._client.is_user_in_group( + login, self._authorized_group_id + ) + self._cache[login] = authorized + return authorized diff --git a/services/hackbot-api/app/bugzilla_client.py b/services/hackbot-api/app/bugzilla_client.py new file mode 100644 index 0000000000..a62967494b --- /dev/null +++ b/services/hackbot-api/app/bugzilla_client.py @@ -0,0 +1,43 @@ +"""Minimal async client for Bugzilla's REST API. + +TODO: Replace with a shared ``bugzilla-client`` workspace lib (the +``phabricator-client`` treatment) once one exists. +""" + +from __future__ import annotations + +import httpx + +_REQUEST_TIMEOUT_SECONDS = 30 + + +class BugzillaUserClient: + """Minimal async client for Bugzilla's user lookup endpoint.""" + + def __init__(self, url: str) -> None: + self._rest_url = url.rstrip("/") + "/rest" + + async def is_user_in_group(self, login: str, group_id: int) -> bool: + """Return whether a Bugzilla account exists and belongs to a group. + + ``group_ids`` filters server-side: the account appears in ``users`` + only when it exists and is a member. The filter needs no API key, + which keeps this internet-facing service free of privileged Bugzilla + credentials (reading another account's ``groups`` directly would + require one). BMO rejects an unknown ``group_ids`` value outright, so + the filter cannot be silently ignored. + """ + async with httpx.AsyncClient(timeout=_REQUEST_TIMEOUT_SECONDS) as client: + response = await client.get( + f"{self._rest_url}/user", + params={ + "names": login, + "group_ids": str(group_id), + "include_fields": "name", + # Report an unknown login in ``faults`` instead of failing + # the request, so it maps to "not authorized", not a 500. + "permissive": "1", + }, + ) + response.raise_for_status() + return bool(response.json().get("users")) diff --git a/services/hackbot-api/app/bugzilla_webhook.py b/services/hackbot-api/app/bugzilla_webhook.py index 92fabee0f1..fb0090ce9d 100644 --- a/services/hackbot-api/app/bugzilla_webhook.py +++ b/services/hackbot-api/app/bugzilla_webhook.py @@ -12,6 +12,7 @@ class BugzillaNeedinfoEvent: bug_id: int flag_id: int comment: str + actor_login: str def detect_needinfo_request( @@ -71,4 +72,6 @@ def detect_needinfo_request( "A needinfo may be requested without a comment, so use the surrounding " "bug context if none exists." ) - return BugzillaNeedinfoEvent(bug_id=bug_id, flag_id=flag_id, comment=comment) + return BugzillaNeedinfoEvent( + bug_id=bug_id, flag_id=flag_id, comment=comment, actor_login=actor_login + ) diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index b7673dc339..251bfd5e87 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -38,6 +38,8 @@ class BugzillaWebhookSettings(BaseModel): bot_login: str = "hackbot@mozilla.tld" # Best-effort in-memory dedupe of retried bug-modification deliveries. dedupe_ttl_seconds: int = 6 * 60 * 60 + # Bugzilla instance queried to authorize the requesting user. + url: str = "https://bugzilla.mozilla.org" class SlackSettings(BaseModel): @@ -80,8 +82,8 @@ class Settings(BaseSettings): webhook: WebhookSettings # Bugzilla uses a separate shared-secret header and bot identity. These map - # from BUGZILLA_WEBHOOK_SECRET, BUGZILLA_WEBHOOK_BOT_LOGIN, and - # BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS. + # from BUGZILLA_WEBHOOK_SECRET, BUGZILLA_WEBHOOK_BOT_LOGIN, + # BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS, and BUGZILLA_WEBHOOK_URL. bugzilla_webhook: BugzillaWebhookSettings slack: SlackSettings diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index f096babcff..156b55d3b2 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -11,6 +11,8 @@ require_bugzilla_webhook_secret, require_phabricator_signature, ) +from app.bugzilla_authorization import AUTHORIZED_GROUP_ID, BugzillaAuthorizer +from app.bugzilla_client import BugzillaUserClient from app.bugzilla_webhook import detect_needinfo_request from app.config import settings from app.phabricator_authorization import ( @@ -52,6 +54,16 @@ def get_phabricator_authorizer( return authorizer +def get_bugzilla_authorizer(request: Request) -> BugzillaAuthorizer: + """Dependency: lazily create the app-scoped authorizer and its user cache.""" + authorizer = getattr(request.app.state, "bugzilla_authorizer", None) + if authorizer is None: + client = BugzillaUserClient(settings.bugzilla_webhook.url) + authorizer = BugzillaAuthorizer(client, AUTHORIZED_GROUP_ID) + request.app.state.bugzilla_authorizer = authorizer + return 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. @@ -149,6 +161,7 @@ async def phabricator_webhook( async def bugzilla_webhook( request: Request, api_client: HackbotClient = Depends(get_hackbot_client), + authorizer: BugzillaAuthorizer = Depends(get_bugzilla_authorizer), ) -> dict: """Trigger a bug-fix follow-up for a bot-directed ``needinfo?`` change.""" payload = await request.json() @@ -170,6 +183,15 @@ async def bugzilla_webhook( ) return {"status": "ignored", "reason": "duplicate delivery"} + # Ignore requests from users without editbugs + if not await authorizer.is_authorized(detected.actor_login): + log.info( + "Ignored Bugzilla needinfo webhook for bug %s: %s is not authorized", + detected.bug_id, + detected.actor_login, + ) + return {"status": "ignored", "reason": "unauthorized user"} + run = await api_client.trigger_run( "bug-fix", { diff --git a/services/hackbot-api/pyproject.toml b/services/hackbot-api/pyproject.toml index 9558779e0b..204f9b1af3 100644 --- a/services/hackbot-api/pyproject.toml +++ b/services/hackbot-api/pyproject.toml @@ -18,6 +18,7 @@ dependencies = [ "google-auth>=2.29.0", "sentry-sdk>=2.51.0", "cachetools>=5.3.0", + "httpx>=0.26.0", "slack-sdk>=3.27.0", "python-multipart>=0.0.9", "hackbot-client", diff --git a/services/hackbot-api/tests/test_bugzilla_authorization.py b/services/hackbot-api/tests/test_bugzilla_authorization.py new file mode 100644 index 0000000000..fc3f76c956 --- /dev/null +++ b/services/hackbot-api/tests/test_bugzilla_authorization.py @@ -0,0 +1,45 @@ +"""Tests for Bugzilla webhook actor authorization.""" + +from unittest.mock import AsyncMock + +from app.bugzilla_authorization import AUTHORIZED_GROUP_ID, BugzillaAuthorizer + + +class _FakeClient: + def __init__(self, member: bool) -> None: + self.is_user_in_group = AsyncMock(return_value=member) + + +def _authorizer(member: bool) -> tuple[BugzillaAuthorizer, _FakeClient]: + client = _FakeClient(member) + return BugzillaAuthorizer(client, AUTHORIZED_GROUP_ID), client + + +async def test_is_authorized_caches_positive_lookup(): + authorizer, client = _authorizer(member=True) + + assert await authorizer.is_authorized("dev@mozilla.com") is True + assert await authorizer.is_authorized("dev@mozilla.com") is True + client.is_user_in_group.assert_awaited_once_with( + "dev@mozilla.com", AUTHORIZED_GROUP_ID + ) + + +async def test_is_authorized_caches_negative_lookup(): + authorizer, client = _authorizer(member=False) + + assert await authorizer.is_authorized("someone@example.com") is False + assert await authorizer.is_authorized("someone@example.com") is False + client.is_user_in_group.assert_awaited_once_with( + "someone@example.com", AUTHORIZED_GROUP_ID + ) + + +async def test_is_authorized_normalizes_login_case(): + authorizer, client = _authorizer(member=True) + + assert await authorizer.is_authorized("Dev@Mozilla.com") is True + assert await authorizer.is_authorized("dev@mozilla.com") is True + client.is_user_in_group.assert_awaited_once_with( + "dev@mozilla.com", AUTHORIZED_GROUP_ID + ) diff --git a/services/hackbot-api/tests/test_bugzilla_client.py b/services/hackbot-api/tests/test_bugzilla_client.py new file mode 100644 index 0000000000..e79a293cf6 --- /dev/null +++ b/services/hackbot-api/tests/test_bugzilla_client.py @@ -0,0 +1,67 @@ +"""Tests for the Bugzilla user client, on BMO's captured payload shapes.""" + +import httpx +from app.bugzilla_client import BugzillaUserClient + + +def _user_client(monkeypatch, json_body: dict) -> tuple[BugzillaUserClient, list]: + """A client whose HTTP layer replays ``json_body``, capturing requests.""" + requests: list[httpx.Request] = [] + + def handler(request: httpx.Request) -> httpx.Response: + requests.append(request) + return httpx.Response(200, json=json_body) + + real_async_client = httpx.AsyncClient + monkeypatch.setattr( + httpx, + "AsyncClient", + lambda **kwargs: real_async_client( + transport=httpx.MockTransport(handler), **kwargs + ), + ) + client = BugzillaUserClient("https://bugzilla.example.com") + return client, requests + + +async def test_is_user_in_group_true_for_member(monkeypatch): + client, requests = _user_client( + monkeypatch, {"users": [{"name": "dev@mozilla.com"}], "faults": []} + ) + + assert await client.is_user_in_group("dev@mozilla.com", 9) is True + + request = requests[0] + assert request.url.host == "bugzilla.example.com" + assert request.url.path == "/rest/user" + assert request.url.params["names"] == "dev@mozilla.com" + assert request.url.params["group_ids"] == "9" + assert request.url.params["permissive"] == "1" + # The membership filter is anonymous: no credential must ever be sent. + assert "X-Bugzilla-API-Key" not in request.headers + + +async def test_is_user_in_group_false_for_non_member(monkeypatch): + # An existing account outside the group is filtered out server-side + # (live BMO shape: empty ``users``, empty ``faults``). + client, _ = _user_client(monkeypatch, {"users": [], "faults": []}) + assert await client.is_user_in_group("outsider@example.com", 9) is False + + +async def test_is_user_in_group_false_for_unknown_user(monkeypatch): + # With permissive=1, BMO reports an unknown login as a 200 with the error + # in ``faults`` and an empty ``users`` list (live BMO shape). + client, _ = _user_client( + monkeypatch, + { + "users": [], + "faults": [ + { + "error": True, + "name": "ghost@example.com", + "message": "There is no user named 'ghost@example.com'.", + } + ], + }, + ) + assert await client.is_user_in_group("ghost@example.com", 9) is False diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index 076ec37b52..c7ee668cc1 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -3,7 +3,8 @@ Covers HMAC signature verification, mention detection / loop prevention, the revision -> (revision_id, bug_id) resolution, and the route's ignore/trigger branches. Bugzilla coverage includes shared-secret auth, structured needinfo -detection, self/private-event suppression, dedupe, and dispatch retry behavior. +detection, self/private-event suppression, actor authorization, dedupe, and +dispatch retry behavior. """ import hashlib @@ -480,6 +481,7 @@ def test_detect_bugzilla_needinfo_from_captured_payload_shape(): assert detected is not None assert detected.bug_id == 2022889 assert detected.flag_id == 2187233 + assert detected.actor_login == "gmierzwinski@mozilla.com" assert "gmierzwinski@mozilla.com" in detected.comment assert "2026-08-07T18:00:05" in detected.comment @@ -541,8 +543,13 @@ async def trigger_run(self, agent_name, inputs): class _FakeAuthorizer: - async def is_authorized(self, author_phid): - return True + def __init__(self, allowed: bool = True): + self.allowed = allowed + self.checked = [] + + async def is_authorized(self, actor): + self.checked.append(actor) + return self.allowed @pytest.fixture @@ -550,13 +557,18 @@ def authorizer(): return _FakeAuthorizer() +@pytest.fixture +def bugzilla_authorizer(): + return _FakeAuthorizer() + + @pytest.fixture def phab_client(): return object() @pytest.fixture -def client(monkeypatch, authorizer, phab_client): +def client(monkeypatch, authorizer, bugzilla_authorizer, phab_client): monkeypatch.setattr(settings, "external_api_key", "test-api-key") monkeypatch.setattr(settings.webhook, "secret", SECRET) monkeypatch.setattr(settings.bugzilla_webhook, "secret", BUGZILLA_SECRET) @@ -566,6 +578,9 @@ def client(monkeypatch, authorizer, phab_client): webhooks._seen_bugzilla_events.clear() app.dependency_overrides[webhooks.get_phabricator_client] = lambda: phab_client app.dependency_overrides[webhooks.get_phabricator_authorizer] = lambda: authorizer + app.dependency_overrides[webhooks.get_bugzilla_authorizer] = lambda: ( + bugzilla_authorizer + ) try: yield TestClient(app) finally: @@ -745,7 +760,7 @@ def test_bugzilla_route_ignores_non_matching_event(client): } -def test_bugzilla_route_triggers_run(client): +def test_bugzilla_route_triggers_run(client, bugzilla_authorizer): fake_api = _FakeHackbotClient() app.dependency_overrides[webhooks.get_hackbot_client] = lambda: fake_api @@ -772,6 +787,24 @@ def test_bugzilla_route_triggers_run(client): }, ) ] + assert bugzilla_authorizer.checked == ["gmierzwinski@mozilla.com"] + + +def test_bugzilla_route_ignores_unauthorized_actor(client, bugzilla_authorizer): + bugzilla_authorizer.allowed = False + fake_api = _FakeHackbotClient() + app.dependency_overrides[webhooks.get_hackbot_client] = lambda: fake_api + payload = _bugzilla_payload() + detected = detect_needinfo_request(payload, bot_login=BUGZILLA_BOT_LOGIN) + + response = _post_bugzilla(client, payload) + + assert response.status_code == 202 + assert response.json() == {"status": "ignored", "reason": "unauthorized user"} + assert fake_api.calls == [] + # The event stays unconsumed: the same flag can still trigger a run once + # the actor is authorized. + assert f"ni{detected.flag_id}" not in webhooks._seen_bugzilla_events def test_bugzilla_route_dedupes_retry_but_not_later_event(client): diff --git a/uv.lock b/uv.lock index c4b7e1f0a7..92e016f316 100644 --- a/uv.lock +++ b/uv.lock @@ -2717,6 +2717,7 @@ dependencies = [ { name = "google-cloud-storage" }, { name = "hackbot-client" }, { name = "hackbot-runtime" }, + { name = "httpx" }, { name = "phabricator-client" }, { name = "pydantic" }, { name = "pydantic-settings" }, @@ -2747,6 +2748,7 @@ requires-dist = [ { name = "google-cloud-storage", specifier = ">=2.16.0" }, { name = "hackbot-client", editable = "libs/hackbot-client" }, { name = "hackbot-runtime", editable = "libs/hackbot-runtime" }, + { name = "httpx", specifier = ">=0.26.0" }, { name = "httpx", marker = "extra == 'dev'", specifier = ">=0.26.0" }, { name = "phabricator-client", editable = "libs/phabricator-client" }, { name = "pydantic", specifier = ">=2.6.0" }, From 7c518e99bcb526a74253810267d83f1e8fb63cf8 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Fri, 4 Sep 2026 14:43:12 +0200 Subject: [PATCH 2/6] update the doc --- docs/hackbot/triggers.md | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/hackbot/triggers.md b/docs/hackbot/triggers.md index 2349b6bd8d..9be644c51c 100644 --- a/docs/hackbot/triggers.md +++ b/docs/hackbot/triggers.md @@ -137,10 +137,14 @@ Guards, each closing a specific failure mode: - **Latest flag wins** — BMO orders flags by id, so the last matching one is the newly requested one. -Authorization is Bugzilla's own: anyone who can set a needinfo on the bot can ask it for -something. There is no separate group check like the Phabricator trigger's -`bmo-editbugs-team`, because a private bug is already excluded and the flag itself is the -request. +Only requesters in Bugzilla's `editbugs` group are authorized (all Mozilla Corporation +members belong to this group) — see +[bugzilla_authorization.py](../../services/hackbot-api/app/bugzilla_authorization.py). +Membership is checked per login with BMO's server-side `group_ids` filter on `/rest/user`. + +- An unauthorized request is ignored without + consuming the dedupe key, leaving the same flag eligible for a later delivery after the + requester becomes authorized. The receiver passes the requester's login and the change timestamp to the agent as context for locating the accompanying comment — a needinfo may be filed without one, in which case @@ -154,6 +158,6 @@ existing one. The needinfo flag is cleared automatically as a recorded the reply comment into a single Bugzilla transaction (see [actions.md](actions.md)). A run that records nothing leaves the flag standing. -Configuration is three env vars — `BUGZILLA_WEBHOOK_SECRET` (required, no default), -`BUGZILLA_WEBHOOK_BOT_LOGIN` and `BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS`; see -[deployment.md](deployment.md). +Configuration is four env vars — `BUGZILLA_WEBHOOK_SECRET` (required, no default), +`BUGZILLA_WEBHOOK_BOT_LOGIN`, `BUGZILLA_WEBHOOK_URL` and +`BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS`; see [deployment.md](deployment.md). From 1ede4fabc35b8834bcc38d58ac3016770435e03f Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sun, 6 Sep 2026 20:31:31 +0200 Subject: [PATCH 3/6] Remove implementation details from the Bugzilla webhook docs --- docs/hackbot/triggers.md | 8 -------- 1 file changed, 8 deletions(-) diff --git a/docs/hackbot/triggers.md b/docs/hackbot/triggers.md index 9be644c51c..6ac256bc99 100644 --- a/docs/hackbot/triggers.md +++ b/docs/hackbot/triggers.md @@ -142,10 +142,6 @@ members belong to this group) — see [bugzilla_authorization.py](../../services/hackbot-api/app/bugzilla_authorization.py). Membership is checked per login with BMO's server-side `group_ids` filter on `/rest/user`. -- An unauthorized request is ignored without - consuming the dedupe key, leaving the same flag eligible for a later delivery after the - requester becomes authorized. - The receiver passes the requester's login and the change timestamp to the agent as context for locating the accompanying comment — a needinfo may be filed without one, in which case the agent falls back to the surrounding bug context. That text is **passed through as data**; @@ -157,7 +153,3 @@ existing one. The needinfo flag is cleared automatically as a recorded `bugzilla.update_bug` action once the run produces at least one other action, coalesced with the reply comment into a single Bugzilla transaction (see [actions.md](actions.md)). A run that records nothing leaves the flag standing. - -Configuration is four env vars — `BUGZILLA_WEBHOOK_SECRET` (required, no default), -`BUGZILLA_WEBHOOK_BOT_LOGIN`, `BUGZILLA_WEBHOOK_URL` and -`BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS`; see [deployment.md](deployment.md). From 465f2f2e200ddd5df4b283c3634595b8966754fc Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sun, 6 Sep 2026 21:01:00 +0200 Subject: [PATCH 4/6] Move the Bugzilla group lookup into BugzillaAuthorize --- .../hackbot-api/app/bugzilla_authorization.py | 37 +++++-- services/hackbot-api/app/bugzilla_client.py | 43 -------- services/hackbot-api/app/routers/webhooks.py | 6 +- .../tests/test_bugzilla_authorization.py | 99 +++++++++++++++---- .../hackbot-api/tests/test_bugzilla_client.py | 67 ------------- 5 files changed, 111 insertions(+), 141 deletions(-) delete mode 100644 services/hackbot-api/app/bugzilla_client.py delete mode 100644 services/hackbot-api/tests/test_bugzilla_client.py diff --git a/services/hackbot-api/app/bugzilla_authorization.py b/services/hackbot-api/app/bugzilla_authorization.py index ebf940679d..707936a5b5 100644 --- a/services/hackbot-api/app/bugzilla_authorization.py +++ b/services/hackbot-api/app/bugzilla_authorization.py @@ -3,28 +3,27 @@ from __future__ import annotations import asyncio -from typing import TYPE_CHECKING +import httpx from cachetools import TTLCache -if TYPE_CHECKING: - from app.bugzilla_client import BugzillaUserClient - AUTHORIZED_GROUP_ID = 9 # bmo-editbugs-team +_REQUEST_TIMEOUT_SECONDS = 30 + class BugzillaAuthorizer: """Cache-backed per-user authorization checks against a Bugzilla group.""" def __init__( self, - client: BugzillaUserClient, + url: str, authorized_group_id: int, *, cache_ttl_seconds: int = 300, cache_maxsize: int = 4096, ) -> None: - self._client = client + self._rest_url = url.rstrip("/") + "/rest" self._authorized_group_id = authorized_group_id self._cache: TTLCache[str, bool] = TTLCache( maxsize=cache_maxsize, @@ -45,8 +44,28 @@ async def is_authorized(self, login: str) -> bool: if cached is not None: return cached - authorized = await self._client.is_user_in_group( - login, self._authorized_group_id - ) + authorized = await self._is_user_in_group(login, self._authorized_group_id) self._cache[login] = authorized return authorized + + # TODO: Move this REST call to a shared Bugzilla client library (#6459). + async def _is_user_in_group(self, login: str, group_id: int) -> bool: + """Return whether a Bugzilla account exists and belongs to a group. + + The ``group_ids`` parameter filters server-side and needs no API key, + so the service holds no Bugzilla credential. + """ + async with httpx.AsyncClient(timeout=_REQUEST_TIMEOUT_SECONDS) as client: + response = await client.get( + f"{self._rest_url}/user", + params={ + "names": login, + "group_ids": str(group_id), + "include_fields": "name", + # Report an unknown login in ``faults`` instead of failing + # the request, so it maps to "not authorized", not a 500. + "permissive": "1", + }, + ) + response.raise_for_status() + return bool(response.json().get("users")) diff --git a/services/hackbot-api/app/bugzilla_client.py b/services/hackbot-api/app/bugzilla_client.py deleted file mode 100644 index a62967494b..0000000000 --- a/services/hackbot-api/app/bugzilla_client.py +++ /dev/null @@ -1,43 +0,0 @@ -"""Minimal async client for Bugzilla's REST API. - -TODO: Replace with a shared ``bugzilla-client`` workspace lib (the -``phabricator-client`` treatment) once one exists. -""" - -from __future__ import annotations - -import httpx - -_REQUEST_TIMEOUT_SECONDS = 30 - - -class BugzillaUserClient: - """Minimal async client for Bugzilla's user lookup endpoint.""" - - def __init__(self, url: str) -> None: - self._rest_url = url.rstrip("/") + "/rest" - - async def is_user_in_group(self, login: str, group_id: int) -> bool: - """Return whether a Bugzilla account exists and belongs to a group. - - ``group_ids`` filters server-side: the account appears in ``users`` - only when it exists and is a member. The filter needs no API key, - which keeps this internet-facing service free of privileged Bugzilla - credentials (reading another account's ``groups`` directly would - require one). BMO rejects an unknown ``group_ids`` value outright, so - the filter cannot be silently ignored. - """ - async with httpx.AsyncClient(timeout=_REQUEST_TIMEOUT_SECONDS) as client: - response = await client.get( - f"{self._rest_url}/user", - params={ - "names": login, - "group_ids": str(group_id), - "include_fields": "name", - # Report an unknown login in ``faults`` instead of failing - # the request, so it maps to "not authorized", not a 500. - "permissive": "1", - }, - ) - response.raise_for_status() - return bool(response.json().get("users")) diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index 156b55d3b2..0774bc9385 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -12,7 +12,6 @@ require_phabricator_signature, ) from app.bugzilla_authorization import AUTHORIZED_GROUP_ID, BugzillaAuthorizer -from app.bugzilla_client import BugzillaUserClient from app.bugzilla_webhook import detect_needinfo_request from app.config import settings from app.phabricator_authorization import ( @@ -58,8 +57,9 @@ def get_bugzilla_authorizer(request: Request) -> BugzillaAuthorizer: """Dependency: lazily create the app-scoped authorizer and its user cache.""" authorizer = getattr(request.app.state, "bugzilla_authorizer", None) if authorizer is None: - client = BugzillaUserClient(settings.bugzilla_webhook.url) - authorizer = BugzillaAuthorizer(client, AUTHORIZED_GROUP_ID) + authorizer = BugzillaAuthorizer( + settings.bugzilla_webhook.url, AUTHORIZED_GROUP_ID + ) request.app.state.bugzilla_authorizer = authorizer return authorizer diff --git a/services/hackbot-api/tests/test_bugzilla_authorization.py b/services/hackbot-api/tests/test_bugzilla_authorization.py index fc3f76c956..24db501b00 100644 --- a/services/hackbot-api/tests/test_bugzilla_authorization.py +++ b/services/hackbot-api/tests/test_bugzilla_authorization.py @@ -2,44 +2,105 @@ from unittest.mock import AsyncMock +import httpx from app.bugzilla_authorization import AUTHORIZED_GROUP_ID, BugzillaAuthorizer -class _FakeClient: - def __init__(self, member: bool) -> None: - self.is_user_in_group = AsyncMock(return_value=member) - - -def _authorizer(member: bool) -> tuple[BugzillaAuthorizer, _FakeClient]: - client = _FakeClient(member) - return BugzillaAuthorizer(client, AUTHORIZED_GROUP_ID), client +def _authorizer(member: bool) -> tuple[BugzillaAuthorizer, AsyncMock]: + """An authorizer whose membership lookup is stubbed to ``member``.""" + authorizer = BugzillaAuthorizer("https://bugzilla.example.com", AUTHORIZED_GROUP_ID) + lookup = AsyncMock(return_value=member) + authorizer._is_user_in_group = lookup + return authorizer, lookup async def test_is_authorized_caches_positive_lookup(): - authorizer, client = _authorizer(member=True) + authorizer, lookup = _authorizer(member=True) assert await authorizer.is_authorized("dev@mozilla.com") is True assert await authorizer.is_authorized("dev@mozilla.com") is True - client.is_user_in_group.assert_awaited_once_with( - "dev@mozilla.com", AUTHORIZED_GROUP_ID - ) + lookup.assert_awaited_once_with("dev@mozilla.com", AUTHORIZED_GROUP_ID) async def test_is_authorized_caches_negative_lookup(): - authorizer, client = _authorizer(member=False) + authorizer, lookup = _authorizer(member=False) assert await authorizer.is_authorized("someone@example.com") is False assert await authorizer.is_authorized("someone@example.com") is False - client.is_user_in_group.assert_awaited_once_with( - "someone@example.com", AUTHORIZED_GROUP_ID - ) + lookup.assert_awaited_once_with("someone@example.com", AUTHORIZED_GROUP_ID) async def test_is_authorized_normalizes_login_case(): - authorizer, client = _authorizer(member=True) + authorizer, lookup = _authorizer(member=True) assert await authorizer.is_authorized("Dev@Mozilla.com") is True assert await authorizer.is_authorized("dev@mozilla.com") is True - client.is_user_in_group.assert_awaited_once_with( - "dev@mozilla.com", AUTHORIZED_GROUP_ID + lookup.assert_awaited_once_with("dev@mozilla.com", AUTHORIZED_GROUP_ID) + + +# --- the membership lookup itself, on BMO's captured payload shapes --- + + +def _http_authorizer( + monkeypatch, json_body: dict +) -> tuple[BugzillaAuthorizer, list[httpx.Request]]: + """An authorizer whose HTTP layer replays ``json_body``, capturing requests.""" + requests: list[httpx.Request] = [] + + def handler(request: httpx.Request) -> httpx.Response: + requests.append(request) + return httpx.Response(200, json=json_body) + + real_async_client = httpx.AsyncClient + monkeypatch.setattr( + httpx, + "AsyncClient", + lambda **kwargs: real_async_client( + transport=httpx.MockTransport(handler), **kwargs + ), + ) + authorizer = BugzillaAuthorizer("https://bugzilla.example.com", AUTHORIZED_GROUP_ID) + return authorizer, requests + + +async def test_lookup_authorizes_group_member(monkeypatch): + authorizer, requests = _http_authorizer( + monkeypatch, {"users": [{"name": "dev@mozilla.com"}], "faults": []} + ) + + assert await authorizer.is_authorized("dev@mozilla.com") is True + + request = requests[0] + assert request.url.host == "bugzilla.example.com" + assert request.url.path == "/rest/user" + assert request.url.params["names"] == "dev@mozilla.com" + assert request.url.params["group_ids"] == str(AUTHORIZED_GROUP_ID) + assert request.url.params["permissive"] == "1" + # The membership filter is anonymous: no credential must ever be sent. + assert "X-Bugzilla-API-Key" not in request.headers + + +async def test_lookup_rejects_non_member(monkeypatch): + # An existing account outside the group is filtered out server-side + # (live BMO shape: empty ``users``, empty ``faults``). + authorizer, _ = _http_authorizer(monkeypatch, {"users": [], "faults": []}) + assert await authorizer.is_authorized("outsider@example.com") is False + + +async def test_lookup_rejects_unknown_user(monkeypatch): + # With permissive=1, BMO reports an unknown login as a 200 with the error + # in ``faults`` and an empty ``users`` list (live BMO shape). + authorizer, _ = _http_authorizer( + monkeypatch, + { + "users": [], + "faults": [ + { + "error": True, + "name": "ghost@example.com", + "message": "There is no user named 'ghost@example.com'.", + } + ], + }, ) + assert await authorizer.is_authorized("ghost@example.com") is False diff --git a/services/hackbot-api/tests/test_bugzilla_client.py b/services/hackbot-api/tests/test_bugzilla_client.py deleted file mode 100644 index e79a293cf6..0000000000 --- a/services/hackbot-api/tests/test_bugzilla_client.py +++ /dev/null @@ -1,67 +0,0 @@ -"""Tests for the Bugzilla user client, on BMO's captured payload shapes.""" - -import httpx -from app.bugzilla_client import BugzillaUserClient - - -def _user_client(monkeypatch, json_body: dict) -> tuple[BugzillaUserClient, list]: - """A client whose HTTP layer replays ``json_body``, capturing requests.""" - requests: list[httpx.Request] = [] - - def handler(request: httpx.Request) -> httpx.Response: - requests.append(request) - return httpx.Response(200, json=json_body) - - real_async_client = httpx.AsyncClient - monkeypatch.setattr( - httpx, - "AsyncClient", - lambda **kwargs: real_async_client( - transport=httpx.MockTransport(handler), **kwargs - ), - ) - client = BugzillaUserClient("https://bugzilla.example.com") - return client, requests - - -async def test_is_user_in_group_true_for_member(monkeypatch): - client, requests = _user_client( - monkeypatch, {"users": [{"name": "dev@mozilla.com"}], "faults": []} - ) - - assert await client.is_user_in_group("dev@mozilla.com", 9) is True - - request = requests[0] - assert request.url.host == "bugzilla.example.com" - assert request.url.path == "/rest/user" - assert request.url.params["names"] == "dev@mozilla.com" - assert request.url.params["group_ids"] == "9" - assert request.url.params["permissive"] == "1" - # The membership filter is anonymous: no credential must ever be sent. - assert "X-Bugzilla-API-Key" not in request.headers - - -async def test_is_user_in_group_false_for_non_member(monkeypatch): - # An existing account outside the group is filtered out server-side - # (live BMO shape: empty ``users``, empty ``faults``). - client, _ = _user_client(monkeypatch, {"users": [], "faults": []}) - assert await client.is_user_in_group("outsider@example.com", 9) is False - - -async def test_is_user_in_group_false_for_unknown_user(monkeypatch): - # With permissive=1, BMO reports an unknown login as a 200 with the error - # in ``faults`` and an empty ``users`` list (live BMO shape). - client, _ = _user_client( - monkeypatch, - { - "users": [], - "faults": [ - { - "error": True, - "name": "ghost@example.com", - "message": "There is no user named 'ghost@example.com'.", - } - ], - }, - ) - assert await client.is_user_in_group("ghost@example.com", 9) is False From a7e5ed4c080bb92bd18ac3a4cc9e391e141741ec Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sun, 6 Sep 2026 21:58:42 +0200 Subject: [PATCH 5/6] move the Bugzilla API URL to the main settings --- services/hackbot-api/app/bugzilla_authorization.py | 6 +++--- services/hackbot-api/app/config.py | 9 +++++---- services/hackbot-api/app/routers/webhooks.py | 4 +--- .../hackbot-api/tests/test_bugzilla_authorization.py | 8 ++++++-- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/services/hackbot-api/app/bugzilla_authorization.py b/services/hackbot-api/app/bugzilla_authorization.py index 707936a5b5..7d8dd4d11d 100644 --- a/services/hackbot-api/app/bugzilla_authorization.py +++ b/services/hackbot-api/app/bugzilla_authorization.py @@ -17,13 +17,13 @@ class BugzillaAuthorizer: def __init__( self, - url: str, + api_url: str, authorized_group_id: int, *, cache_ttl_seconds: int = 300, cache_maxsize: int = 4096, ) -> None: - self._rest_url = url.rstrip("/") + "/rest" + self._api_url = api_url.rstrip("/") self._authorized_group_id = authorized_group_id self._cache: TTLCache[str, bool] = TTLCache( maxsize=cache_maxsize, @@ -57,7 +57,7 @@ async def _is_user_in_group(self, login: str, group_id: int) -> bool: """ async with httpx.AsyncClient(timeout=_REQUEST_TIMEOUT_SECONDS) as client: response = await client.get( - f"{self._rest_url}/user", + f"{self._api_url}/user", params={ "names": login, "group_ids": str(group_id), diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index 251bfd5e87..a8f365fdc3 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -38,8 +38,6 @@ class BugzillaWebhookSettings(BaseModel): bot_login: str = "hackbot@mozilla.tld" # Best-effort in-memory dedupe of retried bug-modification deliveries. dedupe_ttl_seconds: int = 6 * 60 * 60 - # Bugzilla instance queried to authorize the requesting user. - url: str = "https://bugzilla.mozilla.org" class SlackSettings(BaseModel): @@ -82,10 +80,13 @@ class Settings(BaseSettings): webhook: WebhookSettings # Bugzilla uses a separate shared-secret header and bot identity. These map - # from BUGZILLA_WEBHOOK_SECRET, BUGZILLA_WEBHOOK_BOT_LOGIN, - # BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS, and BUGZILLA_WEBHOOK_URL. + # from BUGZILLA_WEBHOOK_SECRET, BUGZILLA_WEBHOOK_BOT_LOGIN, and + # BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS. bugzilla_webhook: BugzillaWebhookSettings + # The Bugzilla REST endpoint this service talks to. Includes /rest. + bugzilla_api_url: str = "https://bugzilla.mozilla.org/rest" + slack: SlackSettings # The webhook receiver triggers runs over the public API (rather than calling diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index 0774bc9385..5c6c02dcef 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -57,9 +57,7 @@ def get_bugzilla_authorizer(request: Request) -> BugzillaAuthorizer: """Dependency: lazily create the app-scoped authorizer and its user cache.""" authorizer = getattr(request.app.state, "bugzilla_authorizer", None) if authorizer is None: - authorizer = BugzillaAuthorizer( - settings.bugzilla_webhook.url, AUTHORIZED_GROUP_ID - ) + authorizer = BugzillaAuthorizer(settings.bugzilla_api_url, AUTHORIZED_GROUP_ID) request.app.state.bugzilla_authorizer = authorizer return authorizer diff --git a/services/hackbot-api/tests/test_bugzilla_authorization.py b/services/hackbot-api/tests/test_bugzilla_authorization.py index 24db501b00..432e3e013a 100644 --- a/services/hackbot-api/tests/test_bugzilla_authorization.py +++ b/services/hackbot-api/tests/test_bugzilla_authorization.py @@ -8,7 +8,9 @@ def _authorizer(member: bool) -> tuple[BugzillaAuthorizer, AsyncMock]: """An authorizer whose membership lookup is stubbed to ``member``.""" - authorizer = BugzillaAuthorizer("https://bugzilla.example.com", AUTHORIZED_GROUP_ID) + authorizer = BugzillaAuthorizer( + "https://bugzilla.example.com/rest", AUTHORIZED_GROUP_ID + ) lookup = AsyncMock(return_value=member) authorizer._is_user_in_group = lookup return authorizer, lookup @@ -59,7 +61,9 @@ def handler(request: httpx.Request) -> httpx.Response: transport=httpx.MockTransport(handler), **kwargs ), ) - authorizer = BugzillaAuthorizer("https://bugzilla.example.com", AUTHORIZED_GROUP_ID) + authorizer = BugzillaAuthorizer( + "https://bugzilla.example.com/rest", AUTHORIZED_GROUP_ID + ) return authorizer, requests From 26dccb5902424bcb083f0bda62041789bdd9c579 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sun, 6 Sep 2026 22:18:06 +0200 Subject: [PATCH 6/6] Rename actor_login to user_login to match Bugzilla --- services/hackbot-api/app/bugzilla_webhook.py | 4 ++-- services/hackbot-api/app/routers/webhooks.py | 4 ++-- services/hackbot-api/tests/test_webhooks.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/services/hackbot-api/app/bugzilla_webhook.py b/services/hackbot-api/app/bugzilla_webhook.py index fb0090ce9d..8c030931f9 100644 --- a/services/hackbot-api/app/bugzilla_webhook.py +++ b/services/hackbot-api/app/bugzilla_webhook.py @@ -12,7 +12,7 @@ class BugzillaNeedinfoEvent: bug_id: int flag_id: int comment: str - actor_login: str + user_login: str def detect_needinfo_request( @@ -73,5 +73,5 @@ def detect_needinfo_request( "bug context if none exists." ) return BugzillaNeedinfoEvent( - bug_id=bug_id, flag_id=flag_id, comment=comment, actor_login=actor_login + bug_id=bug_id, flag_id=flag_id, comment=comment, user_login=actor_login ) diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index 5c6c02dcef..08e83307ca 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -182,11 +182,11 @@ async def bugzilla_webhook( return {"status": "ignored", "reason": "duplicate delivery"} # Ignore requests from users without editbugs - if not await authorizer.is_authorized(detected.actor_login): + if not await authorizer.is_authorized(detected.user_login): log.info( "Ignored Bugzilla needinfo webhook for bug %s: %s is not authorized", detected.bug_id, - detected.actor_login, + detected.user_login, ) return {"status": "ignored", "reason": "unauthorized user"} diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index c7ee668cc1..0ceffa8e84 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -3,7 +3,7 @@ Covers HMAC signature verification, mention detection / loop prevention, the revision -> (revision_id, bug_id) resolution, and the route's ignore/trigger branches. Bugzilla coverage includes shared-secret auth, structured needinfo -detection, self/private-event suppression, actor authorization, dedupe, and +detection, self/private-event suppression, user authorization, dedupe, and dispatch retry behavior. """ @@ -481,7 +481,7 @@ def test_detect_bugzilla_needinfo_from_captured_payload_shape(): assert detected is not None assert detected.bug_id == 2022889 assert detected.flag_id == 2187233 - assert detected.actor_login == "gmierzwinski@mozilla.com" + assert detected.user_login == "gmierzwinski@mozilla.com" assert "gmierzwinski@mozilla.com" in detected.comment assert "2026-08-07T18:00:05" in detected.comment