Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
i_am_webservice()returns true forUSAGE_MODE_JSONandUSAGE_MODE_RESTbut not forUSAGE_MODE_MOJO_REST, so all call sites treat native Mojo REST requests as browser requests.Split out of bug 2072313 at review request: the change flips behaviour for every
USAGE_MODE_MOJO_RESTendpoint at once rather than for the single endpoint being migrated, so it does not belong inside a migration PR. This PR stands on its own and does not depend on any other open PR.Changes
Bugzilla/Util.pm: addUSAGE_MODE_MOJO_RESTtoi_am_webservice()qa/t/rest_components.t: assert the exactauth_failuremessage on an unprivilegedPOST /rest/component/Firefox. At 110 characters it is long enough that an exact match fails while the 72-column wrap is appliedqa/t/rest_github_push_comment.t: setrestrict_commentson a bug, fire a push event against it, and assert a user outsiderestrict_comments_groupstill cannot react to the resulting commentBehaviour changes
Three call sites change observably for native REST endpoints:
Bugzilla/Template.pm:866: theerror_messagefilter no longer wraps at 72 columns, so error messages over that length stop coming back with embedded\ns. This is the motivating fix, but it is a response-shape change for already-shipped endpointsextensions/RestrictComments/Extension.pm:58:object_end_of_set_allstops force-clearingrestrict_commentson bug updates made throughBugzilla/API/V1/Github.pm:364andextensions/PhabBugz/lib/API/V1/PhabBugz.pm:235. Both run as users granted every group, so_can_restrict_comments()is always true there, andBugzilla->input_paramsis never populated on the native Mojo path -- meaning the field is currently cleared on every touched bug. Covered by the new testBugzilla.pm:593:log_user_requeststarts logging native REST requests whenlog_user_requestsis enabled, which will increase the volume written to that logThe MFA and password-reset branches (
Bugzilla.pm:265,:285,:305,Bugzilla/Auth.pm:91) and the Quicksearch shortcuts (Bugzilla/Search/Quicksearch.pm:342,:367) are corrected in the same pass. TheBugzilla::Auth::Login::*andBugzilla::Auth::Verify::DBcall sites are unreachable underUSAGE_MODE_MOJO_REST, since native REST authenticates throughBugzilla/App/Plugin/Login.pm.Test plan
POST /rest/component/Firefoxas a user withouteditcomponents, expect 401 and amessagewith no embedded newlinerestrict_commentson a bug, send apushwebhook naming it, confirm the restriction survives the automation updateqa/t/rest_components.tandqa/t/rest_github_push_comment.tcases unaffectedNote
PRs #2745 and #2751 currently carry this same one-line change to
Bugzilla/Util.pm. It will be dropped from both once this lands, so neither is a prerequisite here and this PR can be reviewed and merged independently.References