Bug 1675625 - Refactor error handing in Mojolicious to allow for nati…

…ve mojo code to something other than Bugzilla::Error
mozilla-bteam · Nov 30, 2020 · ac77615 · ac77615
1 parent 1db5dad
commit ac77615
Show file tree

Hide file tree

Showing 7 changed files with 126 additions and 53 deletions.
diff --git a/Bugzilla/API/V1/Teams.pm b/Bugzilla/API/V1/Teams.pm
@@ -14,25 +14,22 @@ use JSON::MaybeXS qw(decode_json);
 sub setup_routes {
   my ($class, $r) = @_;
   $r->get('/config/component_teams')->to('V1::Teams#component_teams');
-  $r->get('/config/component_security_teams')->to('V1::Teams#component_security_teams');
+  $r->get('/config/component_security_teams')
+    ->to('V1::Teams#component_security_teams');
 }
 
 sub component_teams {
   my ($self) = @_;
   $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) = @_;
   $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
@@ -35,7 +35,7 @@ sub user_profile {
     );
   }
   else {
-    $self->render(status => 401, text => 'Unauthorized');
+    return $self->user_error('invalid_username');
   }
 }
 

diff --git 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
@@ -8,13 +8,15 @@
 package Bugzilla::App::API;
 
 use 5.10.1;
-use Bugzilla::Constants;
-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 Bugzilla::Constants;
+use Bugzilla::Logging;
+
 use constant SUPPORTED_VERSIONS => qw(V1);
 
 sub setup_routes {

diff --git a/Bugzilla/App/OAuth2/Clients.pm b/Bugzilla/App/OAuth2/Clients.pm
@@ -22,9 +22,10 @@ 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',
+        || return $c->user_error('auth_failure',
         {group => 'admin', action => 'edit', object => 'oauth_clients'});
       return 1;
     }
@@ -67,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);
@@ -114,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')) {
@@ -156,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
@@ -183,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 = ?',
@@ -196,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);

diff --git a/Bugzilla/App/Plugin/Error.pm 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
@@ -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) = @_;
-Original file line number
+Diff line change
@@ Expand Up / @@ -35,7 +35,7 @@ sub user_profile { @@
         );
       }
       else {
-        $self->render(status => 401, text => 'Unauthorized');
+        return $self->user_error('invalid_username');
       }
     }
@@ Expand Down @@