Skip to content

Commit ded4174

Browse files
Bugzilla webhook authorization (#6786)
1 parent 8126f25 commit ded4174

10 files changed

Lines changed: 253 additions & 14 deletions

File tree

docs/hackbot/triggers.md

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -137,10 +137,10 @@ Guards, each closing a specific failure mode:
137137
- **Latest flag wins** — BMO orders flags by id, so the last matching one is the newly
138138
requested one.
139139

140-
Authorization is Bugzilla's own: anyone who can set a needinfo on the bot can ask it for
141-
something. There is no separate group check like the Phabricator trigger's
142-
`bmo-editbugs-team`, because a private bug is already excluded and the flag itself is the
143-
request.
140+
Only requesters in Bugzilla's `editbugs` group are authorized (all Mozilla Corporation
141+
members belong to this group) — see
142+
[bugzilla_authorization.py](../../services/hackbot-api/app/bugzilla_authorization.py).
143+
Membership is checked per login through Bugzilla's REST API.
144144

145145
The receiver passes the requester's login and the change timestamp to the agent as context
146146
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
153153
`bugzilla.update_bug` action once the run produces at least one other action, coalesced with
154154
the reply comment into a single Bugzilla transaction (see [actions.md](actions.md)). A run
155155
that records nothing leaves the flag standing.
156-
157-
Configuration is three env vars — `BUGZILLA_WEBHOOK_SECRET` (required, no default),
158-
`BUGZILLA_WEBHOOK_BOT_LOGIN` and `BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS`; see
159-
[deployment.md](deployment.md).
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
"""Authorization checks for Bugzilla webhook actors."""
2+
3+
from __future__ import annotations
4+
5+
import httpx
6+
from cachetools import TTLCache
7+
8+
AUTHORIZED_GROUP_NAME = "editbugs"
9+
10+
_REQUEST_TIMEOUT_SECONDS = 30
11+
12+
13+
class BugzillaAuthorizer:
14+
"""Cache-backed per-user authorization checks against a Bugzilla group."""
15+
16+
def __init__(
17+
self,
18+
api_url: str,
19+
api_key: str,
20+
authorized_group_name: str,
21+
*,
22+
cache_ttl_seconds: int = 300,
23+
cache_maxsize: int = 4096,
24+
) -> None:
25+
self._api_url = api_url.rstrip("/")
26+
self._api_key = api_key
27+
self._authorized_group_name = authorized_group_name
28+
self._cache: TTLCache[str, bool] = TTLCache(
29+
maxsize=cache_maxsize,
30+
ttl=cache_ttl_seconds,
31+
)
32+
33+
async def is_authorized(self, login: str) -> bool:
34+
"""Return whether a Bugzilla login belongs to the authorized group."""
35+
login = login.lower()
36+
37+
cached = self._cache.get(login)
38+
if cached is not None:
39+
return cached
40+
41+
authorized = await self._is_user_in_group(login, self._authorized_group_name)
42+
self._cache[login] = authorized
43+
return authorized
44+
45+
# TODO: Move this REST call to a shared Bugzilla client library (#6459).
46+
async def _is_user_in_group(self, login: str, group_name: str) -> bool:
47+
"""Return whether a Bugzilla account exists and belongs to a group."""
48+
async with httpx.AsyncClient(timeout=_REQUEST_TIMEOUT_SECONDS) as client:
49+
response = await client.get(
50+
f"{self._api_url}/user",
51+
params={
52+
"names": login,
53+
"groups": group_name,
54+
"include_fields": "name",
55+
# Report an unknown login in ``faults`` instead of failing
56+
# the request, so it maps to "not authorized", not a 500.
57+
"permissive": "1",
58+
},
59+
headers={"X-Bugzilla-API-Key": self._api_key},
60+
)
61+
response.raise_for_status()
62+
return bool(response.json().get("users"))

services/hackbot-api/app/bugzilla_webhook.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ class BugzillaNeedinfoEvent:
1212
bug_id: int
1313
flag_id: int
1414
comment: str
15+
user_login: str
1516

1617

1718
def detect_needinfo_request(
@@ -71,4 +72,6 @@ def detect_needinfo_request(
7172
"A needinfo may be requested without a comment, so use the surrounding "
7273
"bug context if none exists."
7374
)
74-
return BugzillaNeedinfoEvent(bug_id=bug_id, flag_id=flag_id, comment=comment)
75+
return BugzillaNeedinfoEvent(
76+
bug_id=bug_id, flag_id=flag_id, comment=comment, user_login=actor_login
77+
)

services/hackbot-api/app/config.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ class Settings(BaseSettings):
8484
# BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS.
8585
bugzilla_webhook: BugzillaWebhookSettings
8686

87+
bugzilla_api_url: str = "https://bugzilla.mozilla.org/rest"
88+
bugzilla_api_key: str
89+
8790
slack: SlackSettings
8891

8992
# The webhook receiver triggers runs over the public API (rather than calling

services/hackbot-api/app/routers/webhooks.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
require_bugzilla_webhook_secret,
1212
require_phabricator_signature,
1313
)
14+
from app.bugzilla_authorization import AUTHORIZED_GROUP_NAME, BugzillaAuthorizer
1415
from app.bugzilla_webhook import detect_needinfo_request
1516
from app.config import settings
1617
from app.phabricator_authorization import (
@@ -52,6 +53,19 @@ def get_phabricator_authorizer(
5253
return authorizer
5354

5455

56+
def get_bugzilla_authorizer(request: Request) -> BugzillaAuthorizer:
57+
"""Dependency: lazily create the app-scoped authorizer and its user cache."""
58+
authorizer = getattr(request.app.state, "bugzilla_authorizer", None)
59+
if authorizer is None:
60+
authorizer = BugzillaAuthorizer(
61+
settings.bugzilla_api_url,
62+
settings.bugzilla_api_key,
63+
AUTHORIZED_GROUP_NAME,
64+
)
65+
request.app.state.bugzilla_authorizer = authorizer
66+
return authorizer
67+
68+
5569
# Best-effort dedupe of retried deliveries, keyed by triggering transaction PHID.
5670
# Per-instance and reset on restart; a durable dedupe (using the DB) can replace
5771
# this if needed. Sized well above the number of mentions expected in a window.
@@ -149,6 +163,7 @@ async def phabricator_webhook(
149163
async def bugzilla_webhook(
150164
request: Request,
151165
api_client: HackbotClient = Depends(get_hackbot_client),
166+
authorizer: BugzillaAuthorizer = Depends(get_bugzilla_authorizer),
152167
) -> dict:
153168
"""Trigger a bug-fix follow-up for a bot-directed ``needinfo?`` change."""
154169
payload = await request.json()
@@ -170,6 +185,14 @@ async def bugzilla_webhook(
170185
)
171186
return {"status": "ignored", "reason": "duplicate delivery"}
172187

188+
if not await authorizer.is_authorized(detected.user_login):
189+
log.info(
190+
"Ignored Bugzilla needinfo webhook for bug %s: %s is not authorized",
191+
detected.bug_id,
192+
detected.user_login,
193+
)
194+
return {"status": "ignored", "reason": "unauthorized user"}
195+
173196
run = await api_client.trigger_run(
174197
"bug-fix",
175198
{

services/hackbot-api/pyproject.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ dependencies = [
1818
"google-auth>=2.29.0",
1919
"sentry-sdk>=2.51.0",
2020
"cachetools>=5.3.0",
21+
"httpx>=0.26.0",
2122
"slack-sdk>=3.27.0",
2223
"python-multipart>=0.0.9",
2324
"hackbot-client",

services/hackbot-api/tests/conftest.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,5 @@
1010
os.environ.setdefault("WEBHOOK_SECRET", "test-webhook-secret")
1111
os.environ.setdefault("BUGZILLA_WEBHOOK_SECRET", "test-bugzilla-webhook-secret")
1212
os.environ.setdefault("BUGZILLA_WEBHOOK_BOT_LOGIN", "hackbot@mozilla.tld")
13+
os.environ.setdefault("BUGZILLA_API_KEY", "test-bugzilla-api-key")
1314
os.environ.setdefault("SLACK_SIGNING_SECRET", "test-signing-secret")
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
"""Tests for Bugzilla webhook actor authorization."""
2+
3+
from unittest.mock import AsyncMock
4+
5+
import httpx
6+
from app.bugzilla_authorization import AUTHORIZED_GROUP_NAME, BugzillaAuthorizer
7+
8+
BUGZILLA_API_KEY = "test-bugzilla-api-key"
9+
10+
11+
def _authorizer(member: bool) -> tuple[BugzillaAuthorizer, AsyncMock]:
12+
"""An authorizer whose membership lookup is stubbed to ``member``."""
13+
authorizer = BugzillaAuthorizer(
14+
"https://bugzilla.example.com/rest",
15+
BUGZILLA_API_KEY,
16+
AUTHORIZED_GROUP_NAME,
17+
)
18+
lookup = AsyncMock(return_value=member)
19+
authorizer._is_user_in_group = lookup
20+
return authorizer, lookup
21+
22+
23+
async def test_is_authorized_caches_positive_lookup():
24+
authorizer, lookup = _authorizer(member=True)
25+
26+
assert await authorizer.is_authorized("dev@mozilla.com") is True
27+
assert await authorizer.is_authorized("dev@mozilla.com") is True
28+
lookup.assert_awaited_once_with("dev@mozilla.com", AUTHORIZED_GROUP_NAME)
29+
30+
31+
async def test_is_authorized_caches_negative_lookup():
32+
authorizer, lookup = _authorizer(member=False)
33+
34+
assert await authorizer.is_authorized("someone@example.com") is False
35+
assert await authorizer.is_authorized("someone@example.com") is False
36+
lookup.assert_awaited_once_with("someone@example.com", AUTHORIZED_GROUP_NAME)
37+
38+
39+
async def test_is_authorized_normalizes_login_case():
40+
authorizer, lookup = _authorizer(member=True)
41+
42+
assert await authorizer.is_authorized("Dev@Mozilla.com") is True
43+
assert await authorizer.is_authorized("dev@mozilla.com") is True
44+
lookup.assert_awaited_once_with("dev@mozilla.com", AUTHORIZED_GROUP_NAME)
45+
46+
47+
# --- the membership lookup itself, on BMO's captured payload shapes ---
48+
49+
50+
def _http_authorizer(
51+
monkeypatch, json_body: dict
52+
) -> tuple[BugzillaAuthorizer, list[httpx.Request]]:
53+
"""An authorizer whose HTTP layer replays ``json_body``, capturing requests."""
54+
requests: list[httpx.Request] = []
55+
56+
def handler(request: httpx.Request) -> httpx.Response:
57+
requests.append(request)
58+
return httpx.Response(200, json=json_body)
59+
60+
real_async_client = httpx.AsyncClient
61+
monkeypatch.setattr(
62+
httpx,
63+
"AsyncClient",
64+
lambda **kwargs: real_async_client(
65+
transport=httpx.MockTransport(handler), **kwargs
66+
),
67+
)
68+
authorizer = BugzillaAuthorizer(
69+
"https://bugzilla.example.com/rest",
70+
BUGZILLA_API_KEY,
71+
AUTHORIZED_GROUP_NAME,
72+
)
73+
return authorizer, requests
74+
75+
76+
async def test_lookup_authorizes_group_member(monkeypatch):
77+
authorizer, requests = _http_authorizer(
78+
monkeypatch, {"users": [{"name": "dev@mozilla.com"}], "faults": []}
79+
)
80+
81+
assert await authorizer.is_authorized("dev@mozilla.com") is True
82+
83+
request = requests[0]
84+
assert request.url.host == "bugzilla.example.com"
85+
assert request.url.path == "/rest/user"
86+
assert request.url.params["names"] == "dev@mozilla.com"
87+
assert request.url.params["groups"] == AUTHORIZED_GROUP_NAME
88+
assert request.url.params["permissive"] == "1"
89+
assert request.headers["X-Bugzilla-API-Key"] == BUGZILLA_API_KEY
90+
91+
92+
async def test_lookup_rejects_non_member(monkeypatch):
93+
# An existing account outside the group is filtered out server-side
94+
# (live BMO shape: empty ``users``, empty ``faults``).
95+
authorizer, _ = _http_authorizer(monkeypatch, {"users": [], "faults": []})
96+
assert await authorizer.is_authorized("outsider@example.com") is False
97+
98+
99+
async def test_lookup_rejects_unknown_user(monkeypatch):
100+
# With permissive=1, BMO reports an unknown login as a 200 with the error
101+
# in ``faults`` and an empty ``users`` list (live BMO shape).
102+
authorizer, _ = _http_authorizer(
103+
monkeypatch,
104+
{
105+
"users": [],
106+
"faults": [
107+
{
108+
"error": True,
109+
"name": "ghost@example.com",
110+
"message": "There is no user named 'ghost@example.com'.",
111+
}
112+
],
113+
},
114+
)
115+
assert await authorizer.is_authorized("ghost@example.com") is False

services/hackbot-api/tests/test_webhooks.py

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
Covers HMAC signature verification, mention detection / loop prevention, the
44
revision -> (revision_id, bug_id) resolution, and the route's ignore/trigger
55
branches. Bugzilla coverage includes shared-secret auth, structured needinfo
6-
detection, self/private-event suppression, dedupe, and dispatch retry behavior.
6+
detection, self/private-event suppression, user authorization, dedupe, and
7+
dispatch retry behavior.
78
"""
89

910
import hashlib
@@ -480,6 +481,7 @@ def test_detect_bugzilla_needinfo_from_captured_payload_shape():
480481
assert detected is not None
481482
assert detected.bug_id == 2022889
482483
assert detected.flag_id == 2187233
484+
assert detected.user_login == "gmierzwinski@mozilla.com"
483485
assert "gmierzwinski@mozilla.com" in detected.comment
484486
assert "2026-08-07T18:00:05" in detected.comment
485487

@@ -541,22 +543,32 @@ async def trigger_run(self, agent_name, inputs):
541543

542544

543545
class _FakeAuthorizer:
544-
async def is_authorized(self, author_phid):
545-
return True
546+
def __init__(self, allowed: bool = True):
547+
self.allowed = allowed
548+
self.checked = []
549+
550+
async def is_authorized(self, actor):
551+
self.checked.append(actor)
552+
return self.allowed
546553

547554

548555
@pytest.fixture
549556
def authorizer():
550557
return _FakeAuthorizer()
551558

552559

560+
@pytest.fixture
561+
def bugzilla_authorizer():
562+
return _FakeAuthorizer()
563+
564+
553565
@pytest.fixture
554566
def phab_client():
555567
return object()
556568

557569

558570
@pytest.fixture
559-
def client(monkeypatch, authorizer, phab_client):
571+
def client(monkeypatch, authorizer, bugzilla_authorizer, phab_client):
560572
monkeypatch.setattr(settings, "external_api_key", "test-api-key")
561573
monkeypatch.setattr(settings.webhook, "secret", SECRET)
562574
monkeypatch.setattr(settings.bugzilla_webhook, "secret", BUGZILLA_SECRET)
@@ -566,6 +578,9 @@ def client(monkeypatch, authorizer, phab_client):
566578
webhooks._seen_bugzilla_events.clear()
567579
app.dependency_overrides[webhooks.get_phabricator_client] = lambda: phab_client
568580
app.dependency_overrides[webhooks.get_phabricator_authorizer] = lambda: authorizer
581+
app.dependency_overrides[webhooks.get_bugzilla_authorizer] = lambda: (
582+
bugzilla_authorizer
583+
)
569584
try:
570585
yield TestClient(app)
571586
finally:
@@ -745,7 +760,7 @@ def test_bugzilla_route_ignores_non_matching_event(client):
745760
}
746761

747762

748-
def test_bugzilla_route_triggers_run(client):
763+
def test_bugzilla_route_triggers_run(client, bugzilla_authorizer):
749764
fake_api = _FakeHackbotClient()
750765
app.dependency_overrides[webhooks.get_hackbot_client] = lambda: fake_api
751766

@@ -772,6 +787,24 @@ def test_bugzilla_route_triggers_run(client):
772787
},
773788
)
774789
]
790+
assert bugzilla_authorizer.checked == ["gmierzwinski@mozilla.com"]
791+
792+
793+
def test_bugzilla_route_ignores_unauthorized_actor(client, bugzilla_authorizer):
794+
bugzilla_authorizer.allowed = False
795+
fake_api = _FakeHackbotClient()
796+
app.dependency_overrides[webhooks.get_hackbot_client] = lambda: fake_api
797+
payload = _bugzilla_payload()
798+
detected = detect_needinfo_request(payload, bot_login=BUGZILLA_BOT_LOGIN)
799+
800+
response = _post_bugzilla(client, payload)
801+
802+
assert response.status_code == 202
803+
assert response.json() == {"status": "ignored", "reason": "unauthorized user"}
804+
assert fake_api.calls == []
805+
# The event stays unconsumed: the same flag can still trigger a run once
806+
# the actor is authorized.
807+
assert f"ni{detected.flag_id}" not in webhooks._seen_bugzilla_events
775808

776809

777810
def test_bugzilla_route_dedupes_retry_but_not_later_event(client):

0 commit comments

Comments
 (0)