From c05319ad966018edb812ac0651e70082c13e9364 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Fri, 17 Jan 2020 15:22:44 -0500 Subject: [PATCH 1/5] Bump version to 20200117.1 --- Bugzilla.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Bugzilla.pm b/Bugzilla.pm index 2bddc1d511..b9d0460010 100644 --- a/Bugzilla.pm +++ b/Bugzilla.pm @@ -13,7 +13,7 @@ use warnings; use Bugzilla::Logging; -our $VERSION = '20191212.1'; +our $VERSION = '20200117.1'; use Bugzilla::Auth; use Bugzilla::Auth::Persist::Cookie; From 561592dbfd8d83e72990a66a71dfb412a8b794e8 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sun, 8 Nov 2020 20:51:32 -0500 Subject: [PATCH 2/5] Bug 1675625 - Refactor error handing in Mojolicious to allow for native mojo code to use Bugzilla::Error --- Bugzilla/App.pm | 1 + Bugzilla/App/API.pm | 2 + Bugzilla/App/OAuth2/Clients.pm | 1 + Bugzilla/App/Plugin/Error.pm | 88 ++++++++++++++++++++++++++++++++++ Bugzilla/App/Plugin/Glue.pm | 17 ------- 5 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 Bugzilla/App/Plugin/Error.pm diff --git a/Bugzilla/App.pm b/Bugzilla/App.pm index 414f78f168..b6ff12c5d4 100644 --- a/Bugzilla/App.pm +++ b/Bugzilla/App.pm @@ -46,6 +46,7 @@ sub startup { $self->plugin('Bugzilla::App::Plugin::BlockIP'); $self->plugin('Bugzilla::App::Plugin::Glue'); $self->plugin('Bugzilla::App::Plugin::Login'); + $self->plugin('Bugzilla::App::Plugin::Error'); $self->plugin('Bugzilla::App::Plugin::Hostage') unless $ENV{BUGZILLA_DISABLE_HOSTAGE}; $self->plugin('Bugzilla::App::Plugin::SizeLimit') diff --git a/Bugzilla/App/API.pm b/Bugzilla/App/API.pm index a22710fa31..f62b89d36c 100644 --- a/Bugzilla/App/API.pm +++ b/Bugzilla/App/API.pm @@ -16,6 +16,8 @@ use Try::Tiny; use constant SUPPORTED_VERSIONS => qw(V1); +use Bugzilla::Constants; + sub setup_routes { my ($class, $r) = @_; diff --git a/Bugzilla/App/OAuth2/Clients.pm b/Bugzilla/App/OAuth2/Clients.pm index 66a5dad405..1ce80444c2 100644 --- a/Bugzilla/App/OAuth2/Clients.pm +++ b/Bugzilla/App/OAuth2/Clients.pm @@ -22,6 +22,7 @@ sub setup_routes { my $client_route = $r->under( '/admin/oauth' => sub { my ($c) = @_; + Bugzilla->usage_mode(USAGE_MODE_MOJO); my $user = $c->bugzilla->login(LOGIN_REQUIRED) || return undef; $user->in_group('admin') || ThrowUserError('auth_failure', diff --git a/Bugzilla/App/Plugin/Error.pm b/Bugzilla/App/Plugin/Error.pm new file mode 100644 index 0000000000..d9e6f30fe1 --- /dev/null +++ b/Bugzilla/App/Plugin/Error.pm @@ -0,0 +1,88 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. + +package Bugzilla::App::Plugin::Error; +use 5.10.1; +use Mojo::Base 'Mojolicious::Plugin'; + +use Bugzilla::Constants; +use Bugzilla::Logging; +use Bugzilla::WebService::Constants; + +sub register { + my ($self, $app) = @_; + $app->helper('code_error' => sub { _render_error('code', @_); }); + $app->helper('user_error' => sub { _render_error('user', @_); }); +} + +sub _render_error { + my ($type, $c, $error, $vars) = @_; + my $logfunc = _make_logfunc(ucfirst($type)); + + # Errors displayed in a web page + if (Bugzilla->error_mode == ERROR_MODE_MOJO + || Bugzilla->error_mode == ERROR_MODE_WEBPAGE) + { + $logfunc->("webpage error: $error"); + + $c->render( + handler => 'bugzilla', + template => "global/$type-error", + format => 'html', + error => $error, + %{$vars} + ); + } + + # Errors returned in an API request + elsif (Bugzilla->error_mode == ERROR_MODE_REST) { + my %error_map = %{WS_ERROR_CODE()}; + my $code = $error_map{$error}; + + if (!$code) { + $code = ERROR_UNKNOWN_FATAL if $type =~ /code/i; + $code = ERROR_UNKNOWN_TRANSIENT if $type =~ /user/i; + } + + my %status_code_map = %{REST_STATUS_CODE_MAP()}; + my $status_code = $status_code_map{$code} || $status_code_map{'_default'}; + + $logfunc->("REST error: $error (HTTP $status_code, internal code $code)"); + + # Find the full error message + my $message; + $vars->{error} = $error; + my $template = Bugzilla->template; + $template->process("global/$type-error.html.tmpl", $vars, \$message) + || die $template->error(); + + my $error = { + error => 1, + code => $code, + message => $message, + documentation => 'https://bmo.readthedocs.io/en/latest/api/', + }; + + $c->render(json => $error, status => $status_code); + } +} + +sub _make_logfunc { + my ($type) = @_; + my $logger = Log::Log4perl->get_logger("Bugzilla.Error.$type"); + return sub { + local $Log::Log4perl::caller_depth = $Log::Log4perl::caller_depth + 3; + if ($type eq 'User') { + $logger->warn(@_); + } + else { + $logger->error(@_); + } + }; +} + +1; diff --git a/Bugzilla/App/Plugin/Glue.pm b/Bugzilla/App/Plugin/Glue.pm index 2958883c70..43c0ab5010 100644 --- a/Bugzilla/App/Plugin/Glue.pm +++ b/Bugzilla/App/Plugin/Glue.pm @@ -67,23 +67,6 @@ sub register { } ); - $app->helper( - 'bugzilla.error_page' => sub { - my ($c, $error) = @_; - if (blessed $error && $error->isa('Bugzilla::Error::Base')) { - $c->render( - handler => 'bugzilla', - template => $error->template, - error => $error->message, - %{$error->vars} - ); - } - else { - $c->reply->exception($error); - } - } - ); - $app->helper( 'url_is_attachment_base' => sub { my ($c, $id) = @_; From 621632fafcc67d94043c5c50576e806a853fff19 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sun, 8 Nov 2020 23:24:18 -0500 Subject: [PATCH 3/5] - Fixed an issue with oauth2 clients and selecting multiple scopes - Updated oauth2 client UI to use new error helpers --- Bugzilla/App/OAuth2/Clients.pm | 47 +++++++++++++++++----------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/Bugzilla/App/OAuth2/Clients.pm b/Bugzilla/App/OAuth2/Clients.pm index 1ce80444c2..11d8b53777 100644 --- a/Bugzilla/App/OAuth2/Clients.pm +++ b/Bugzilla/App/OAuth2/Clients.pm @@ -25,7 +25,7 @@ sub setup_routes { Bugzilla->usage_mode(USAGE_MODE_MOJO); my $user = $c->bugzilla->login(LOGIN_REQUIRED) || return undef; $user->in_group('admin') - || ThrowUserError('auth_failure', + || return $c->user_error('auth_failure', {group => 'admin', action => 'edit', object => 'oauth_clients'}); return 1; } @@ -68,31 +68,32 @@ sub create { my $description = $self->param('description'); my $id = $self->param('id'); my $secret = $self->param('secret'); - my @scopes = $self->param('scopes'); - $description or ThrowCodeError('param_required', {param => 'description'}); - $id or ThrowCodeError('param_required', {param => 'id'}); - $secret or ThrowCodeError('param_required', {param => 'secret'}); - any { $_ > 0 } @scopes or ThrowCodeError('param_required', {param => 'scopes'}); + my $scopes = $self->every_param('scopes'); + $description + or return $self->code_error('param_required', {param => 'description'}); + $id or return $self->code_error('param_required', {param => 'id'}); + $secret or return $self->code_error('param_required', {param => 'secret'}); + any { $_ > 0 } @{$scopes} + or return $self->code_error('param_required', {param => 'scopes'}); my $token = $self->param('token'); check_token_data($token, 'create_oauth_client'); - $dbh->do('INSERT INTO oauth2_client (client_id, description, secret) VALUES (?, ?, ?)', + $dbh->do( + 'INSERT INTO oauth2_client (client_id, description, secret) VALUES (?, ?, ?)', undef, $id, $description, $secret); my $client_data = $dbh->selectrow_hashref('SELECT * FROM oauth2_client WHERE client_id = ?', undef, $id); - foreach my $scope_id (@scopes) { + foreach my $scope_id (@{$scopes}) { $scope_id = $dbh->selectrow_array('SELECT id FROM oauth2_scope WHERE id = ?', undef, $scope_id); if (!$scope_id) { - ThrowCodeError('param_required', {param => 'scopes'}); + return $self->code_error('param_required', {param => 'scopes'}); } - $dbh->do( - 'INSERT INTO oauth2_client_scope (client_id, scope_id) VALUES (?, ?)', - undef, $client_data->{id}, $scope_id - ); + $dbh->do('INSERT INTO oauth2_client_scope (client_id, scope_id) VALUES (?, ?)', + undef, $client_data->{id}, $scope_id); } delete_token($token); @@ -115,8 +116,9 @@ sub delete { my $dbh = Bugzilla->dbh; my $vars = {}; - my $id = $self->param('id'); - my $client_data = $dbh->selectrow_hashref('SELECT * FROM oauth2_client WHERE id = ?', + my $id = $self->param('id'); + my $client_data + = $dbh->selectrow_hashref('SELECT * FROM oauth2_client WHERE id = ?', undef, $id); if (!$self->param('deleteme')) { @@ -157,14 +159,15 @@ sub edit { my $vars = {}; my $id = $self->param('id'); - my $client_data = $dbh->selectrow_hashref('SELECT * FROM oauth2_client WHERE id = ?', + my $client_data + = $dbh->selectrow_hashref('SELECT * FROM oauth2_client WHERE id = ?', undef, $id); my $client_scopes = $dbh->selectall_arrayref( 'SELECT scope_id FROM oauth2_client_scope WHERE client_id = ?', undef, $client_data->{id}); $client_data->{scopes} = [map { $_->[0] } @{$client_scopes}]; - $vars->{client} = $client_data; + $vars->{client} = $client_data; # All scopes my $all_scopes @@ -184,7 +187,7 @@ sub edit { my $description = $self->param('description'); my $active = $self->param('active'); - my @scopes = $self->param('scopes'); + my $scopes = $self->every_param('scopes'); if ($description ne $client_data->{description}) { $dbh->do('UPDATE oauth2_client SET description = ? WHERE id = ?', @@ -197,11 +200,9 @@ sub edit { } $dbh->do('DELETE FROM oauth2_client_scope WHERE client_id = ?', undef, $id); - foreach my $scope_id (@scopes) { - $dbh->do( - 'INSERT INTO oauth2_client_scope (client_id, scope_id) VALUES (?, ?)', - undef, $client_data->{id}, $scope_id - ); + foreach my $scope_id (@{$scopes}) { + $dbh->do('INSERT INTO oauth2_client_scope (client_id, scope_id) VALUES (?, ?)', + undef, $client_data->{id}, $scope_id); } delete_token($token); From 4a5d168d21ea041248f859b6545b8d98d2287599 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Mon, 9 Nov 2020 06:49:33 -0500 Subject: [PATCH 4/5] - Refactoring to allow setting USAGE_MODE_REST for API error reporting --- Bugzilla/API/V1/Configuration.pm | 4 +--- Bugzilla/API/V1/User.pm | 5 ++--- Bugzilla/App/API.pm | 25 ++++++++++++++++++++----- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/Bugzilla/API/V1/Configuration.pm b/Bugzilla/API/V1/Configuration.pm index 43280102a9..374b99a108 100644 --- a/Bugzilla/API/V1/Configuration.pm +++ b/Bugzilla/API/V1/Configuration.pm @@ -22,9 +22,7 @@ use Mojo::JSON qw( true false ); sub setup_routes { my ($class, $r) = @_; - $r->get('/latest/configuration')->to('V1::Configuration#configuration'); - $r->get('/rest/configuration')->to('V1::Configuration#configuration'); - $r->get('/bzapi/configuration')->to('V1::Configuration#configuration'); + $r->get('/configuration')->to('V1::Configuration#configuration'); } sub configuration { diff --git a/Bugzilla/API/V1/User.pm b/Bugzilla/API/V1/User.pm index f0d7c86edd..1cb1723b99 100644 --- a/Bugzilla/API/V1/User.pm +++ b/Bugzilla/API/V1/User.pm @@ -13,8 +13,7 @@ use Mojo::JSON qw( true false ); sub setup_routes { my ($class, $r) = @_; - $r->get('/api/user/profile')->to('V1::User#user_profile'); - $r->get('/rest/user/profile')->to('V1::User#user_profile'); + $r->get('/profile')->to('V1::User#user_profile'); } sub user_profile { @@ -35,7 +34,7 @@ sub user_profile { ); } else { - $self->render(status => 401, text => 'Unauthorized'); + return $self->user_error('invalid_username'); } } diff --git a/Bugzilla/App/API.pm b/Bugzilla/App/API.pm index f62b89d36c..662d8a821c 100644 --- a/Bugzilla/App/API.pm +++ b/Bugzilla/App/API.pm @@ -8,15 +8,16 @@ package Bugzilla::App::API; use 5.10.1; -use Bugzilla::Logging; -use Module::Runtime qw(require_module); use Mojo::Base qw( Mojolicious::Controller ); + use Mojo::Loader qw( find_modules ); +use Module::Runtime qw(require_module); use Try::Tiny; -use constant SUPPORTED_VERSIONS => qw(V1); - use Bugzilla::Constants; +use Bugzilla::Logging; + +use constant SUPPORTED_VERSIONS => qw(V1); sub setup_routes { my ($class, $r) = @_; @@ -26,13 +27,27 @@ sub setup_routes { push @$namespaces, 'Bugzilla::API'; $r->namespaces($namespaces); + # Backwards compat with /api/user/profile which Phabricator + $r->under('/api' => sub { Bugzilla->usage_mode(USAGE_MODE_REST); }) + ->get('/user/profile')->to('V1::User#user_profile'); + + # Other backwards compat routes + $r->under('/latest' => sub { Bugzilla->usage_mode(USAGE_MODE_REST); }) + ->get('/configuration')->to('V1::Configuration#configuration'); + $r->under('/bzapi' => sub { Bugzilla->usage_mode(USAGE_MODE_REST); }) + ->get('/configuration')->to('V1::Configuration#configuration'); + + # Set the usage mode for all routes under /rest + my $rest_routes + = $r->under('/rest' => sub { Bugzilla->usage_mode(USAGE_MODE_REST); }); + foreach my $version (SUPPORTED_VERSIONS) { foreach my $module (find_modules("Bugzilla::API::$version")) { try { require_module($module); my $controller = $module->new; if ($controller->can('setup_routes')) { - $controller->setup_routes($r); + $controller->setup_routes($rest_routes); } } catch { From 79a6ca6832dab9b164407b220d8456120695f121 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 18 Nov 2020 10:54:38 -0500 Subject: [PATCH 5/5] Update base on review comments. --- Bugzilla/API/V1/Teams.pm | 19 +++++++------------ Bugzilla/API/V1/User.pm | 4 +--- Bugzilla/App/API.pm | 2 +- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/Bugzilla/API/V1/Teams.pm b/Bugzilla/API/V1/Teams.pm index 57ae200ab5..49bdd0373b 100644 --- a/Bugzilla/API/V1/Teams.pm +++ b/Bugzilla/API/V1/Teams.pm @@ -13,28 +13,23 @@ use JSON::MaybeXS qw(decode_json); sub setup_routes { my ($class, $r) = @_; - $r->get('/rest/config/component_teams')->to('V1::Teams#component_teams'); - $r->get('/rest/config/component_security_teams')->to('V1::Teams#component_security_teams'); + $r->get('/config/component_teams')->to('V1::Teams#component_teams'); + $r->get('/config/component_security_teams') + ->to('V1::Teams#component_security_teams'); } sub component_teams { my ($self) = @_; - Bugzilla->usage_mode(USAGE_MODE_REST); $self->bugzilla->login(LOGIN_REQUIRED) - || return $self->render(status => 401, text => 'Unauthorized'); - $self->render( - json => decode_json(Bugzilla->params->{report_component_teams}) - ); + || return $self->user_error('invalid_username'); + $self->render(json => decode_json(Bugzilla->params->{report_component_teams})); } sub component_security_teams { my ($self) = @_; - Bugzilla->usage_mode(USAGE_MODE_REST); $self->bugzilla->login(LOGIN_REQUIRED) - || return $self->render(status => 401, text => 'Unauthorized'); - $self->render( - json => decode_json(Bugzilla->params->{report_secbugs_teams}) - ); + || return $self->user_error('invalid_username'); + $self->render(json => decode_json(Bugzilla->params->{report_secbugs_teams})); } 1; diff --git a/Bugzilla/API/V1/User.pm b/Bugzilla/API/V1/User.pm index 3370b5e303..23f6392473 100644 --- a/Bugzilla/API/V1/User.pm +++ b/Bugzilla/API/V1/User.pm @@ -15,13 +15,11 @@ use Bugzilla::Constants; sub setup_routes { my ($class, $r) = @_; - $r->get('/profile')->to('V1::User#user_profile'); + $r->get('/user_profile')->to('V1::User#user_profile'); } sub user_profile { my ($self) = @_; - Bugzilla->usage_mode(USAGE_MODE_REST); - my $user = $self->bugzilla->oauth('user:read'); if ($user && $user->id) { $self->render( diff --git a/Bugzilla/App/API.pm b/Bugzilla/App/API.pm index 662d8a821c..a48f4757b8 100644 --- a/Bugzilla/App/API.pm +++ b/Bugzilla/App/API.pm @@ -27,7 +27,7 @@ sub setup_routes { push @$namespaces, 'Bugzilla::API'; $r->namespaces($namespaces); - # Backwards compat with /api/user/profile which Phabricator + # Backwards compat with /api/user/profile which Phabricator requires $r->under('/api' => sub { Bugzilla->usage_mode(USAGE_MODE_REST); }) ->get('/user/profile')->to('V1::User#user_profile');