Bug 2065173 - Migrate Group REST resource to native Mojo API by Xzzz · Pull Request #2745 · mozilla/bmo

Xzzz · 2026-09-15T15:10:32Z

Summary

Ports Bugzilla::WebService::Group's create/update/get methods into a native Bugzilla::API::V1::Group Mojo controller, mirroring the pattern already used for Classification/Component/Teams/Reminders/Configuration/Bugzilla (system info)/BugUserLastVisit.

This is a child bug of 2057358, see there for details.

Changes

Add Bugzilla/API/V1/Group.pm: GET/POST /rest/group and GET/PUT /rest/group/<id_or_name> (login + creategroups required for create/update), same JSON response shape as the legacy endpoints
Delete Bugzilla/WebService/Group.pm and Bugzilla/WebService/Server/REST/Resources/Group.pm
Remove the Group entry from WS_DISPATCH in Bugzilla/WebService/Constants.pm, the corresponding use line in Bugzilla/WebService/Server/REST.pm, and the POD entry in Bugzilla/WebService.pm

Breaking change: removing the WS_DISPATCH entry also removes Group.create/update/get from JSON-RPC and XML-RPC, not just the legacy REST dispatcher, since all three share that table. Native Mojo routes only serve REST. This matches the same tradeoff already made in the Classification, Bugzilla (system-info), and BugUserLastVisit migrations earlier in this series.

Known pre-existing bug, preserved, not fixed: get()'s "filter by blessability" step for non-can_see_groups users calls $user->can_bless($group) with a Group object where can_bless expects a group id, so the filter is a no-op in both the legacy and new code. Flagging for review rather than silently fixing it in a migration PR.

Test plan

GET /rest/group / GET /rest/group?ids=1&ids=2 / GET /rest/group?names=admin
GET /rest/group/<id> / GET /rest/group/<name>
GET /rest/group/<id>?membership=1
POST /rest/group (name/description required, duplicate name, invalid user_regexp)
PUT /rest/group/<id_or_name> (protected admin/insider group rejected for non-admins, per qa/t/rest_group_update_protected.t)
OPTIONS on both routes returns Allow: GET, POST, PUT
Existing qa/t/rest_group_get.t, qa/t/rest_group_create.t, qa/t/rest_group_update_protected.t should pass unchanged

References

Bugzilla link: 2065173
Part of 2057358

Error messages from native-Mojo REST controllers were being word-wrapped at 72 columns, introducing literal newlines that broke message-content assertions like rest_group_create.t.

…failure The 'auth_failure' error template only has a branch for 'groups' (plural), 'group' fell through to the empty default, so the unauthorized-user message read "...not authorized to add new ." with the object word missing.

dklawren · 2026-09-15T22:29:49Z

+  # object, so this filter is a no-op that leaves $groups untouched in
+  # practice rather than actually filtering by blessability.
+  if (!$can_see_groups) {
+    $groups = [map { $user->can_bless($_) } @{$groups}];


this filter isn't a no-op — the comment above is wrong about what happens

can_bless returns 0/1 (Bugzilla/User.pm:2116), so map replaces each Bugzilla::Group object with a plain scalar. the next line then calls ->id on 0 and dies

reachable: a user not in can_see_groups but with bless privileges passes the guard at line 147, gets $groups = $user->bless_groups, and GET /rest/group returns a 500 instead of their blessable groups. admins never hit it, which is why the qa tests are green

the line is a faithful copy of the legacy code so it's pre-existing, but since the PR documents it as harmless it's worth fixing here instead:

$groups = [grep { $user->can_bless($_->id) } @$groups];

that matches how _get_group_membership already calls it on line 239

You right, my comment was wrong: this isn't a harmless no-op, it crashes.
Fixed with $user->can_bless($_->id), matching _get_group_membership's usage.

=> Fixed in "Bug 2065173 - Fix crash filtering groups by blessability"

dklawren · 2026-09-15T22:29:49Z

+    $group->check_can_be_edited();
+  }
+
+  my %values = %$params;


cookie-authenticated PUT breaks here

_request_params returns $self->req->params->to_hash unfiltered, and Bugzilla/App/Plugin/Login.pm:68 reads Bugzilla_api_token without deleting it. so it survives into %values → set_all → set_Bugzilla_api_token → ThrowCodeError('unknown_method')

legacy worked because Bugzilla::Auth::Login::Cookie did delete Bugzilla->input_params->{Bugzilla_api_token} before the method ran. that path isn't used by Mojo controllers, so this is a new regression

include_fields/exclude_fields hit the same wall. suggest whitelisting the documented fields (name, description, user_regexp, is_active, icon_url) before set_all

Good catch. As suggested, switched to whitelisting the five documented update fields (name, description, user_regexp, is_active, icon_url) instead of blacklisting names/ids, so stray keys like Bugzilla_api_token or include_fields/exclude_fields no longer reach set_all().

=> Fixed in "Bug 2065173 - Whitelist update() fields instead of blacklisting names/ids"

dklawren · 2026-09-15T22:29:49Z

+  my $routes = $r->under(
+    '/group' => sub { Bugzilla->usage_mode(USAGE_MODE_MOJO_REST); });
+  $routes->get('/')->to('V1::Group#get');
+  $routes->get('/:id')->to('V1::Group#get');


:id won't match a group name containing a dot — the : placeholder stops at ., while the legacy resource regex was qr{^/group/([^/]+)$}

GET /rest/group/my.group and PUT /rest/group/my.group would return an HTML 404 instead of JSON. Bugzilla::Group::_check_name only checks for emptiness and uniqueness, so dotted names are allowed

use the relaxed placeholder '/#id' on lines 30, 32 and 34, same as Bugzilla/API/V1/Classification.pm:21. a qa case with a dotted name would lock it in

As suggested, switched to relaxed #id placeholder (same as Classification.pm).

=> Fixed in "Bug 2065173 - Use relaxed #id placeholder to allow dots in group names"

dklawren · 2026-09-15T22:29:49Z

+    foreach my $field (keys %{$changes{$group->id}}) {
+      my $change = $changes{$group->id}->{$field};
+      $hash{changes}{$field}
+        = {removed => "$change->[0]", added => "$change->[1]"};


interpolating $change->[0] turns a legit undef into "" and logs an uninitialized-value warning

legacy used $self->type('string', ...), which passed undef through as JSON null. Bugzilla::Object::update supports transitions from or to undef (e.g. icon_url going from NULL), so this changes the response shape

removed => defined $change->[0] ? "$change->[0]" : undef, added => defined $change->[1] ? "$change->[1]" : undef,

Fixed as suggested, Thanks!

=> Fixed in "Bug 2065173 - Preserve null in changes when a field goes to/from undef"

_request_params duplicated the same query-string/JSON-body merge logic already written for BugUserLastVisit.pm (bug 2065171). Now call a single shared merge_request_params helper, so it's a one-place change to drop later if query-string-on-POST support is ever removed. Please note that BugUserLastVisit.pm (bug 2065171) is being updated separately to call the same helper instead of its own copy.

Xzzz · 2026-09-17T13:40:35Z

Pushed a follow-up commit extracting _request_params's merge logic into a shared Bugzilla::WebService::Util::merge_request_params helper, reused by BugUserLastVisit.pm (bug 2065171, PR #2743) as well.
Group's _request_params had no extra per-field handling, so it's now just a direct call to the shared helper.
=> No behavior change

can_bless() takes a group id, not a Group object. Passing the object made every entry falsy, and the next line's ->id call on that died. Reachable by bless-privileged users without can_see_groups. Was ported faithfully from legacy as a described no-op, although it's actually a genuine crash, so fixing it here.

…/ids set_all() throws unknown_method for any stray key without a matching set_<key> method. Cookie-authenticated PUT hit this via Bugzilla_api_token (legacy deleted it before the method ran, the Mojo cookie-auth path doesn't), include_fields/exclude_fields hit it too. Whitelist the update fields instead.

dklawren · 2026-09-18T20:46:43Z

-  return $usage_mode == USAGE_MODE_JSON || $usage_mode == USAGE_MODE_REST;
+  return $usage_mode == USAGE_MODE_JSON
+    || $usage_mode == USAGE_MODE_REST
+    || $usage_mode == USAGE_MODE_MOJO_REST;


this isn't scoped to Group - it changes i_am_webservice() for every request running in USAGE_MODE_MOJO_REST, which is all ~15 Bugzilla::API::V1::* controllers plus the PhabBugz/Webhooks/SearchAPI/GitHubPullRequests extension controllers

concrete downstream effects I can see:

Bugzilla/Template.pm:866 stops wrap_comment-ing error messages, so the message field of every Mojo REST error response changes shape (this is presumably the motivation, but it's a response-format change for already-shipped endpoints)

extensions/RestrictComments/Extension.pm:58 flips: bug updates through API/V1/Github.pm and PhabBugz previously cleared restrict_comments on every touched bug when the actor was in restrict_comments_enable_group, and now won't

Bugzilla.pm:593 log_user_request starts logging Mojo REST requests when log_user_requests is on

Bugzilla/Auth/Login/APIKey.pm:46 and Bugzilla/Auth/Verify/DB.pm:127 change gating for any code path that reaches Bugzilla->login under MOJO_REST

none of that is wrong as far as I can tell, but it doesn't belong silently inside a Group migration. please either split it into its own bug or at minimum call it out in the description and add a test pinning the new error-message shape

dklawren · 2026-09-18T20:46:44Z

+
+  if (length $c->req->body) {
+    my $body_params;
+    try { $body_params = decode_json($c->req->body); }


swallowing the decode error is a regression from the legacy layer. _retrieve_json_params in Bugzilla/WebService/Server/REST.pm:343 did ThrowUserError('json_rpc_invalid_params', {err_msg => $@})

so POST /rest/group with a truncated body now reports You must enter a name instead of telling the client its JSON is broken

suggest rethrowing as json_rpc_invalid_params when the body is non-empty and looks like JSON. separately, the length $c->req->body guard has no method check, so a GET with a body gets its JSON merged in too - legacy only did this for non-GET

dklawren · 2026-09-18T20:46:44Z

+
+  # Whitelist the documented update fields; set_all() throws unknown_method
+  # for any stray key (e.g. Bugzilla_api_token, include_fields).
+  my %values = map { $_ => $params->{$_} }


this whitelist turns a hard error into a silent no-op. legacy passed everything except ids/names to set_all, so Bugzilla::Object::set_all raised unknown_method on anything unrecognized

now PUT /rest/group/5 with {"is_bug_group": 0} or a typo like {"userregexp": "@foo$"} returns 200 with changes: {} and the client has no way to tell its field was ignored

prefer deleting the known meta keys (ids, names, Bugzilla_api_token, Bugzilla_login, Bugzilla_password, include_fields, exclude_fields) and passing the rest through, so unknown fields still error

dklawren · 2026-09-18T20:46:44Z

+sub options {
+  my ($self) = @_;
+
+  $self->res->headers->header('Allow'                        => 'GET, POST, PUT');


one Allow value is shared by both routes, so OPTIONS /rest/group advertises PUT (no route, 404) and OPTIONS /rest/group/5 advertises POST (no route, 404)

legacy computed this per path - GET, POST on /group and GET, PUT on /group/<id_or_name> (see the deleted Resources/Group.pm and the note at Bugzilla/WebService/Server/REST.pm:615). since Access-Control-Allow-Methods is also set from it, a browser preflight gets told POST /rest/group/5 is fine

suggest passing the allowed methods through the route, e.g. ->to('V1::Group#options', allow => 'GET, POST')

dklawren · 2026-09-18T20:46:44Z

+    }
+  }
+
+  # Filter groups by blessability if user is not allowed to see all groups.


the comment and the PR description disagree with the code, and both descriptions of the legacy behavior are wrong

the description says this bug is "preserved, not fixed", but line 191 does fix it. and legacy wasn't a no-op filter - it was $groups = [map { $user->can_bless($_) } @{$groups}], a map, not a grep. can_bless($group_object) numifies the object ref to 0 and returns 0, so legacy replaced every element with 0 and then _group_to_hash called ->id on 0, i.e. GET /rest/group/<id> as a blesser without can_see_groups returned a 500

the fix is right, but it's a real behavior change (those users now get a filtered list). please fix the description and comment, and add a case to qa/t/rest_group_get.t for a user who can bless one group and requests a different one

dklawren · 2026-09-18T20:46:44Z

+    # Show only users in visible groups.
+    $visible_groups = $user->visible_groups_inherited;
+
+    if (scalar @$visible_groups) {


when usevisibilitygroups is on, the caller isn't in editusers, and visible_groups_inherited comes back empty, $query is never extended - but $visible_groups is [], which is truthy, so the ThrowUserError on line 256 doesn't fire and line 258 appends ' AND ' . sql_in(...) to a bare SELECT userid FROM profiles

that's SELECT userid FROM profiles AND ugm.group_id IN (...) - a SQL error, so GET /rest/group/<id>?membership=1 500s

carried over verbatim from the legacy code, but you're rewriting this function anyway. unless $visible_groups && (!ref $visible_groups || @$visible_groups) or just setting $visible_groups = undef when the list is empty would close it

Xzzz added 3 commits September 15, 2026 17:05

Bug 2065173 - Migrate Group REST resource to native Mojo API

060d6a8

Bug 2065173 - Fix i_am_webservice() to recognize USAGE_MODE_MOJO_REST

740a663

Error messages from native-Mojo REST controllers were being word-wrapped at 72 columns, introducing literal newlines that broke message-content assertions like rest_group_create.t.

dklawren requested changes Sep 16, 2026

View reviewed changes

Xzzz added 2 commits September 17, 2026 14:52

Merge remote-tracking branch 'origin/master' into bug-2065173

20194f1

Xzzz added 4 commits September 17, 2026 16:54

Bug 2065173 - Use relaxed #id placeholder to allow dots in group names

26c885e

Bug 2065173 - Preserve null in changes when a field goes to/from undef

44d1ec9

dklawren requested changes Sep 18, 2026

View reviewed changes

Conversation

Xzzz commented Sep 15, 2026

Summary

Changes

Test plan

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xzzz Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xzzz commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Xzzz Sep 17, 2026 •

edited

Loading