diff --git a/docs/hackbot/triggers.md b/docs/hackbot/triggers.md index 2349b6bd8d..6ac256bc99 100644 --- a/docs/hackbot/triggers.md +++ b/docs/hackbot/triggers.md @@ -137,10 +137,10 @@ 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`. 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 @@ -153,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 three env vars — `BUGZILLA_WEBHOOK_SECRET` (required, no default), -`BUGZILLA_WEBHOOK_BOT_LOGIN` and `BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS`; see -[deployment.md](deployment.md). diff --git a/services/hackbot-api/app/bugzilla_authorization.py b/services/hackbot-api/app/bugzilla_authorization.py new file mode 100644 index 0000000000..7d8dd4d11d --- /dev/null +++ b/services/hackbot-api/app/bugzilla_authorization.py @@ -0,0 +1,71 @@ +"""Authorization checks for Bugzilla webhook actors.""" + +from __future__ import annotations + +import asyncio + +import httpx +from cachetools import TTLCache + +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, + api_url: str, + authorized_group_id: int, + *, + cache_ttl_seconds: int = 300, + cache_maxsize: int = 4096, + ) -> None: + self._api_url = api_url.rstrip("/") + 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._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._api_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..8c030931f9 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 + user_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, user_login=actor_login + ) diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index b7673dc339..a8f365fdc3 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -84,6 +84,9 @@ class Settings(BaseSettings): # 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 f096babcff..08e83307ca 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -11,6 +11,7 @@ require_bugzilla_webhook_secret, require_phabricator_signature, ) +from app.bugzilla_authorization import AUTHORIZED_GROUP_ID, BugzillaAuthorizer from app.bugzilla_webhook import detect_needinfo_request from app.config import settings from app.phabricator_authorization import ( @@ -52,6 +53,15 @@ 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: + authorizer = BugzillaAuthorizer(settings.bugzilla_api_url, 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 +159,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 +181,15 @@ async def bugzilla_webhook( ) return {"status": "ignored", "reason": "duplicate delivery"} + # Ignore requests from users without editbugs + 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.user_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..432e3e013a --- /dev/null +++ b/services/hackbot-api/tests/test_bugzilla_authorization.py @@ -0,0 +1,110 @@ +"""Tests for Bugzilla webhook actor authorization.""" + +from unittest.mock import AsyncMock + +import httpx +from app.bugzilla_authorization import AUTHORIZED_GROUP_ID, BugzillaAuthorizer + + +def _authorizer(member: bool) -> tuple[BugzillaAuthorizer, AsyncMock]: + """An authorizer whose membership lookup is stubbed to ``member``.""" + authorizer = BugzillaAuthorizer( + "https://bugzilla.example.com/rest", 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, lookup = _authorizer(member=True) + + assert await authorizer.is_authorized("dev@mozilla.com") is True + assert await authorizer.is_authorized("dev@mozilla.com") is True + lookup.assert_awaited_once_with("dev@mozilla.com", AUTHORIZED_GROUP_ID) + + +async def test_is_authorized_caches_negative_lookup(): + 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 + lookup.assert_awaited_once_with("someone@example.com", AUTHORIZED_GROUP_ID) + + +async def test_is_authorized_normalizes_login_case(): + 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 + 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/rest", 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_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index 076ec37b52..0ceffa8e84 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, user 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.user_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" },