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.

Master has been merged in now that #2751, #2756 and #2743 have landed, so the branch no longer carries its own copy of merge_request_params or the i_am_webservice change; both come from master and Bugzilla/Util.pm has dropped out of this diff. The WS_DISPATCH conflict with #2743 is resolved by dropping both entries: master removed BugUserLastVisit, this PR removes Group.

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
create/update/get unpack ($params, $error) from the shared merge_request_params and report a malformed JSON body as rest_malformed_json, instead of the decode error being swallowed
update() and get() declare ids and names as list parameters, so ?ids=1&ids=2 returns every group asked for rather than only the last. create() takes only scalar fields and is left alone
update() deletes the request-level keys (ids, names, include_fields, exclude_fields, Bugzilla_api_key, Bugzilla_api_token, Bugzilla_login, Bugzilla_password) and passes the rest to set_all(), so an unrecognized field still raises unknown_method rather than being silently dropped
The allowed methods for OPTIONS come from the route, so /rest/group advertises GET, POST and /rest/group/<id_or_name> advertises GET, PUT, rather than one shared GET, POST, PUT for both. Access-Control-Allow-Methods follows the same value
_get_group_membership(): normalise an empty visible_groups_inherited to undef. An empty arrayref is truthy, so the group_not_visible check passed and the query became SELECT userid FROM profiles AND ugm.group_id IN (...), a SQL error. Carried over from the legacy code
qa/t/rest_group_get.t: cover a repeated ids parameter, and a user who can bless one group and requests another

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.

Pre-existing bug, fixed here: get()'s "filter by blessability" step for non-can_see_groups users. An earlier revision of this description claimed the legacy code was a no-op filter preserved as-is; both halves of that were wrong. Legacy ran [map { $user->can_bless($_) } @{$groups}] (a map, not a grep) and can_bless expects a group id, so passing the object numified the ref and returned 0 for every element. _group_to_hash then called ->id on 0, meaning GET /rest/group/<id> as a blesser without can_see_groups returned a 500. Passing $_->id to a grep filters the list as the comment always claimed. This is a real behaviour change: those users now get a filtered list instead of an error, and qa/t/rest_group_get.t covers it.

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
GET /rest/group/<id> as a user who can bless a different group, expecting a filtered list rather than a 500
POST /rest/group (name/description required, duplicate name, invalid user_regexp)
POST/PUT with a malformed JSON body, expecting rest_malformed_json
PUT /rest/group/<id_or_name> with an unrecognized field, expecting unknown_method rather than an empty changes
PUT /rest/group/<id_or_name> (protected admin/insider group rejected for non-admins, per qa/t/rest_group_update_protected.t)
OPTIONS /rest/group returns Allow: GET, POST, OPTIONS /rest/group/<id_or_name> returns Allow: GET, PUT
qa/t/rest_group_create.t and qa/t/rest_group_update_protected.t pass unchanged; qa/t/rest_group_get.t gains the repeated-ids and blessability cases

References

Bugzilla link: 2065173
Part of 2057358
Bug 2074854 - Recognize USAGE_MODE_MOJO_REST as a webservice mode in i_am_webservice #2756 (bug 2074854) and Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API #2743 (bug 2065171) have landed; the i_am_webservice change and the list-parameter support both come from master

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

Done, and it has landed: split into bug 2074854 / #2756, which merged earlier today with a test pinning the unwrapped error-message shape and one covering restrict_comments surviving an automation update. Master has been merged into this branch, so Bugzilla/Util.pm no longer appears in this diff at all.

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

The first half is fixed. Since #2751 landed, the shared merge_request_params in master returns ($params, $error) and raises rest_malformed_json instead of swallowing the decode failure, and create/update/get now unpack both and report it, so a truncated body no longer surfaces as You must enter a name.

The second half is still open, and I would rather have your call than decide it here. You are right that the guard has no method check: it is length $body && !@{$c->req->body_params->names}, so a GET carrying a JSON body still gets it merged, while legacy only merged the body for non-GET requests (Bugzilla/WebService/Server/REST.pm:337).

The catch is that the guard now lives in master and is shared by BugUserLastVisit, Group, Component and Reminders, so fixing it here would put a cross-cutting change back inside a migration PR, which is what you asked me to avoid on the i_am_webservice one. Three options as I see them:

fold it into Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API #2743, which already touches that helper and is waiting on review

give it its own bug the way we did for 2074854

leave it and I will file it separately

Which would you prefer?

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

Fixed => update() now copies the params and deletes the request-level keys rather than whitelisting the documented ones, so anything unrecognized still reaches set_all() and raises unknown_method instead of returning 200 with empty changes.

Please note that I added Bugzilla_api_key to the list you gave, since fix_credentials produces it from the same family as Bugzilla_api_token and a client sending it as a parameter would hit the same wall.

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')

Fixed => The allowed methods now come through the route as you suggested, so /rest/group answers Allow: GET, POST and /rest/group/<id_or_name> answers Allow: GET, PUT.
Access-Control-Allow-Methods is set from the same value, so the preflight no longer claims POST /rest/group/5 is acceptable. The foreach over both paths is gone in favour of two explicit declarations.

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

You are right on both counts and I have corrected the comment and the PR description.

Legacy was [map { $user->can_bless($_) } @{$groups}], a map, and can_bless($group_object) numifies the ref so every element became 0, _group_to_hash then called ->id on that, which is a 500 for any blesser without can_see_groups, not the no-op I described. The description now says so explicitly, including that my earlier revision of it was wrong, and flags the behaviour change.

qa/t/rest_group_get.t gains a case: a second group, the unprivileged user given bless privileges on it, then a GET for the original group, expecting a filtered list rather than an error. Bless privileges are not settable over the API, so the test inserts the user_group_map row directly and removes it afterwards, following the precedent at qa/t/rest_search_api.t:61. It sits after the can_see_groups cleanup so the existing 400 assertion higher up is unaffected.

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

Fixed => visible_groups_inherited is normalised to undef when it comes back empty, so the group_not_visible check fires instead of being satisfied by a truthy empty arrayref, and the query is never left as a bare SELECT with an AND appended. Traced it through and it behaves exactly as you described.

The shared helper landed in master with a two-element return, so calling it in scalar context assigned the error rather than the params. create/update/get now unpack both and report a malformed JSON body via user_error.

Whitelisting the documented fields made a typo or an unsupported field a silent no-op returning 200 with empty changes. Delete the request-level keys and pass the rest to set_all(), which still raises unknown_method.

Both OPTIONS routes shared one Allow value, so /rest/group advertised PUT and /rest/group/<id> advertised POST, neither of which exists. The allowed methods now come from the route, which also fixes Access-Control-Allow-Methods.

The legacy code mapped rather than grepped can_bless($group_object), so every element became 0 and _group_to_hash called ->id on it: a 500 for a blesser without can_see_groups, not the no-op the comment claimed.

An empty arrayref is truthy, so the group_not_visible check passed and the membership query became a bare SELECT with an ' AND ...' appended, which is a SQL error. Carried over from the legacy code.

merge_request_params collapses parameters to scalars unless the caller says otherwise, so ?ids=1&ids=2 returned only the last group. update() and get() now declare both list parameters; create() takes only scalar fields and is left alone. Covers the repeated form, which no test exercised.

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

This was referenced Sep 23, 2026

Bug 2074854 - Recognize USAGE_MODE_MOJO_REST as a webservice mode in i_am_webservice #2756

Merged

Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API #2743

Merged

Xzzz added 9 commits September 23, 2026 21:19

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

a0fe532

Bug 2065173 - Correct the blessability comment and cover the fixed case

c0b0db2

The legacy code mapped rather than grepped can_bless($group_object), so every element became 0 and _group_to_hash called ->id on it: a 500 for a blesser without can_see_groups, not the no-op the comment claimed.

Bug 2065173 - Treat an empty visible group list as no visible groups

9eaf66e

An empty arrayref is truthy, so the group_not_visible check passed and the membership query became a bare SELECT with an ' AND ...' appended, which is a SQL error. Carried over from the legacy code.

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

381c8f3

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

7cacefc

Xzzz requested a review from dklawren September 23, 2026 21:57

Conversation

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

Uh oh!

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!

Xzzz Sep 23, 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!

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 commented Sep 15, 2026 •

edited

Loading

Xzzz Sep 17, 2026 •

edited

Loading

Xzzz Sep 23, 2026 •

edited

Loading