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.

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

Navigation Menu

Bug 2065173 - Migrate Group REST resource to native Mojo API - #2745

Bug 2065173 - Migrate Group REST resource to native Mojo API#2745
Xzzz wants to merge 9 commits into
mozilla:masterfrom
Xzzz:bug-2065173

Xzzz commented Sep 15, 2026

Uh oh!

dklawren Sep 15, 2026

Uh oh!

Xzzz Sep 17, 2026 •

edited

Loading

Uh oh!

dklawren Sep 15, 2026

Uh oh!

Xzzz Sep 17, 2026

Uh oh!

dklawren Sep 15, 2026

Uh oh!

Xzzz Sep 17, 2026

Uh oh!

dklawren Sep 15, 2026

Uh oh!

Xzzz Sep 17, 2026

Uh oh!

Xzzz commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Xzzz commented Sep 15, 2026

Summary

Changes

Test plan

References

Uh oh!

dklawren Sep 15, 2026

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!

dklawren Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Xzzz Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

dklawren Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Xzzz Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

dklawren Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Xzzz Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

Xzzz commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Xzzz Sep 17, 2026 •

edited

Loading