Bug 1450343 - Make the SES handler use Bugzilla::Logging and log more details

diff --git a/Bugzilla/ModPerl/BasicAuth.pm b/Bugzilla/ModPerl/BasicAuth.pm
index e93680e9d1a9fcfff7e28bb0cb4601414d6db727..7248a19f30e39765363b76e4ee2f1414a43e552f 100644 (file)
--- a/Bugzilla/ModPerl/BasicAuth.pm
+++ b/Bugzilla/ModPerl/BasicAuth.pm
@@ -25,18 +25,22 @@ use warnings;
 # AUTH_VAR_NAME and AUTH_VAR_PASS are the names of variables defined in
 # `localconfig` which hold the authentication credentials.
 
-use Apache2::Const -compile => qw(OK HTTP_UNAUTHORIZED);
+use Apache2::Const -compile => qw(OK HTTP_UNAUTHORIZED); ## no critic (Freenode::ModPerl)
+use Bugzilla::Logging;
 use Bugzilla ();
 
 sub handler {
     my $r = shift;
     my ($status, $password) = $r->get_basic_auth_pw;
-    return $status if $status != Apache2::Const::OK;
+    if ($status != Apache2::Const::OK) {
+        WARN("Got non-OK status: $status when trying to get password");
+        return $status
+    }
 
     my $auth_var_name = $ENV{AUTH_VAR_NAME};
     my $auth_var_pass = $ENV{AUTH_VAR_PASS};
     unless ($auth_var_name && $auth_var_pass) {
-        warn "AUTH_VAR_NAME and AUTH_VAR_PASS environmental vars not set\n";
+        ERROR('AUTH_VAR_NAME and AUTH_VAR_PASS environmental vars not set');
         $r->note_basic_auth_failure;
         return Apache2::Const::HTTP_UNAUTHORIZED;
     }
@@ -44,13 +48,14 @@ sub handler {
     my $auth_user = Bugzilla->localconfig->{$auth_var_name};
     my $auth_pass = Bugzilla->localconfig->{$auth_var_pass};
     unless ($auth_user && $auth_pass) {
-        warn "$auth_var_name and $auth_var_pass not configured\n";
+        ERROR("$auth_var_name and $auth_var_pass not configured");
         $r->note_basic_auth_failure;
         return Apache2::Const::HTTP_UNAUTHORIZED;
     }
 
     unless ($r->user eq $auth_user && $password eq $auth_pass) {
         $r->note_basic_auth_failure;
+        WARN('username and password do not match');
         return Apache2::Const::HTTP_UNAUTHORIZED;
     }
 

diff --git a/ses/index.cgi b/ses/index.cgi
index aa5b3470480c85898aa9537e612b9fa4707e4372..9e1632586a7bde88e86c82b45b26d769f35f2033 100755 (executable)
--- a/ses/index.cgi
+++ b/ses/index.cgi
@@ -13,49 +13,55 @@ use warnings;
 use lib qw(.. ../lib ../local/lib/perl5);
 
 use Bugzilla ();
+use Bugzilla::Logging;
 use Bugzilla::Constants qw( ERROR_MODE_DIE );
 use Bugzilla::Mailer qw( MessageToMTA );
 use Bugzilla::User ();
 use Bugzilla::Util qw( html_quote remote_ip );
-use JSON::XS qw( decode_json encode_json );
+use JSON::MaybeXS qw( decode_json encode_json );
 use LWP::UserAgent ();
 use Try::Tiny qw( try catch );
 
 Bugzilla->error_mode(ERROR_MODE_DIE);
 try {
     main();
-} catch {
-    warn "SES: Fatal error: $_\n";
-    respond(500 => 'Internal Server Error');
+}
+catch {
+    FATAL("Fatal error: $_");
+    respond( 500 => 'Internal Server Error' );
 };
 
 sub main {
-    my $message = decode_json_wrapper(Bugzilla->cgi->param('POSTDATA')) // return;
+    my $message = decode_json_wrapper( Bugzilla->cgi->param('POSTDATA') ) // return;
     my $message_type = $ENV{HTTP_X_AMZ_SNS_MESSAGE_TYPE} // '(missing)';
 
-    if ($message_type eq 'SubscriptionConfirmation') {
+    if ( $message_type eq 'SubscriptionConfirmation' ) {
         confirm_subscription($message);
     }
 
-    elsif ($message_type eq 'Notification') {
-        my $notification = decode_json_wrapper($message->{Message}) // return;
+    elsif ( $message_type eq 'Notification' ) {
+        my $notification = decode_json_wrapper( $message->{Message} ) // return;
 
         my $notification_type = $notification->{notificationType} // '';
-        if ($notification_type eq 'Bounce') {
+        if ( $notification_type eq '' ) {
+            my $keys = join ', ', keys %$notification;
+            WARN("No notificationType in notification (keys: $keys)");
+        }
+        if ( $notification_type eq 'Bounce' ) {
             process_bounce($notification);
         }
-        elsif ($notification_type eq 'Complaint') {
+        elsif ( $notification_type eq 'Complaint' ) {
             process_complaint($notification);
         }
         else {
-            warn "SES: Unsupported notification-type: $notification_type\n";
-            respond(200 => 'OK');
+            WARN("Unsupported notification-type: $notification_type");
+            respond( 200 => 'OK' );
         }
     }
 
     else {
-        warn "SES: Unsupported message-type: $message_type\n";
-        respond(200 => 'OK');
+        WARN("Unsupported message-type: $message_type");
+        respond( 200 => 'OK' );
     }
 }
 
@@ -63,112 +69,117 @@ sub confirm_subscription {
     my ($message) = @_;
 
     my $subscribe_url = $message->{SubscribeURL};
-    if (!$subscribe_url) {
-        warn "SES: Bad SubscriptionConfirmation request: missing SubscribeURL\n";
-        respond(400 => 'Bad Request');
+    if ( !$subscribe_url ) {
+        WARN('Bad SubscriptionConfirmation request: missing SubscribeURL');
+        respond( 400 => 'Bad Request' );
         return;
     }
 
-    my $ua = ua();
-    my $res = $ua->get($message->{SubscribeURL});
-    if (!$res->is_success) {
-        warn "SES: Bad response from SubscribeURL: " . $res->status_line . "\n";
-        respond(400 => 'Bad Request');
+    my $ua  = ua();
+    my $res = $ua->get( $message->{SubscribeURL} );
+    if ( !$res->is_success ) {
+        WARN( 'Bad response from SubscribeURL: ' . $res->status_line );
+        respond( 400 => 'Bad Request' );
         return;
     }
 
-    respond(200 => 'OK');
+    respond( 200 => 'OK' );
 }
 
 sub process_bounce {
     my ($notification) = @_;
     my $type = $notification->{bounce}->{bounceType};
 
-    # these should be infrequent and hopefully small
-    warn("SES: notification: " . encode_json($notification));
+    if ( $type eq 'Transient' ) {
 
-    if ($type eq 'Transient') {
         # just log transient bounces
-        foreach my $recipient (@{ $notification->{bounce}->{bouncedRecipients} }) {
+        foreach my $recipient ( @{ $notification->{bounce}->{bouncedRecipients} } ) {
             my $address = $recipient->{emailAddress};
-            Bugzilla->audit("SES: transient bounce for <$address>");
+            Bugzilla->audit("transient bounce for <$address>");
         }
     }
 
-    elsif ($type eq 'Permanent') {
+    elsif ( $type eq 'Permanent' ) {
+
         # disable each account that is permanently bouncing
-        foreach my $recipient (@{ $notification->{bounce}->{bouncedRecipients} }) {
+        foreach my $recipient ( @{ $notification->{bounce}->{bouncedRecipients} } ) {
             my $address = $recipient->{emailAddress};
-            my $reason = sprintf('(%s) %s', $recipient->{action} // 'error',
-                                            $recipient->{diagnosticCode} // 'unknown');
+            my $reason
+                = sprintf( '(%s) %s', $recipient->{action} // 'error', $recipient->{diagnosticCode} // 'unknown' );
 
-            my $user = Bugzilla::User->new({ name => $address, cache => 1 });
+            my $user = Bugzilla::User->new( { name => $address, cache => 1 } );
             if ($user) {
+
                 # never auto-disable admin accounts
-                if ($user->in_group('admin')) {
-                    Bugzilla->audit("SES: ignoring permanent bounce for admin <$address>: $reason");
+                if ( $user->in_group('admin') ) {
+                    Bugzilla->audit("ignoring permanent bounce for admin <$address>: $reason");
                 }
 
                 else {
                     my $template = Bugzilla->template_inner();
-                    my $vars = {
-                        mta    => $notification->{bounce}->{reportingMTA} // 'unknown',
+                    my $vars     = {
+                        mta => $notification->{bounce}->{reportingMTA} // 'unknown',
                         reason => $reason,
                     };
                     my $disable_text;
-                    $template->process('admin/users/bounce-disabled.txt.tmpl', $vars, \$disable_text)
+                    $template->process( 'admin/users/bounce-disabled.txt.tmpl', $vars, \$disable_text )
                         || die $template->error();
 
-                                       $user->set_disabledtext($disable_text);
-                                       $user->set_disable_mail(1);
-                                       $user->update();
-                    Bugzilla->audit("SES: permanent bounce for <$address> disabled userid-" . $user->id . ": $reason");
+                    $user->set_disabledtext($disable_text);
+                    $user->set_disable_mail(1);
+                    $user->update();
+                    Bugzilla->audit(
+                        "permanent bounce for <$address> disabled userid-" . $user->id . ": $reason" );
                 }
             }
 
             else {
-                Bugzilla->audit("SES: permanent bounce for <$address> has no user: $reason");
+                Bugzilla->audit("permanent bounce for <$address> has no user: $reason");
             }
         }
     }
 
     else {
-        warn "SES: Unsupported bounce type: $type\n";
+        WARN("Unsupported bounce type: $type\n");
     }
 
-    respond(200 => 'OK');
+    respond( 200 => 'OK' );
 }
 
 sub process_complaint {
+
     # email notification to bugzilla admin
     my ($notification) = @_;
-    my $template = Bugzilla->template_inner();
-    my $json = JSON::XS->new->pretty->utf8->canonical;
+    my $template       = Bugzilla->template_inner();
+    my $json           = JSON::MaybeXS->new(
+        pretty    => 1,
+        utf8      => 1,
+        canonical => 1,
+    );
 
-    foreach my $recipient (@{ $notification->{complaint}->{complainedRecipients} }) {
-        my $reason  = $notification->{complaint}->{complaintFeedbackType} // 'unknown';
+    foreach my $recipient ( @{ $notification->{complaint}->{complainedRecipients} } ) {
+        my $reason = $notification->{complaint}->{complaintFeedbackType} // 'unknown';
         my $address = $recipient->{emailAddress};
-        Bugzilla->audit("SES: complaint for <$address> for '$reason'");
+        Bugzilla->audit("complaint for <$address> for '$reason'");
         my $vars = {
             email        => $address,
-            user         => Bugzilla::User->new({ name => $address, cache => 1 }),
+            user         => Bugzilla::User->new( { name => $address, cache => 1 } ),
             reason       => $reason,
             notification => $json->encode($notification),
         };
         my $message;
-        $template->process('email/ses-complaint.txt.tmpl', $vars, \$message)
+        $template->process( 'email/ses-complaint.txt.tmpl', $vars, \$message )
             || die $template->error();
         MessageToMTA($message);
     }
 
-    respond(200 => 'OK');
+    respond( 200 => 'OK' );
 }
 
 sub respond {
-    my ($code, $message) = @_;
-    print Bugzilla->cgi->header(
-        -status => "$code $message",
-    );
+    my ( $code, $message ) = @_;
+    print Bugzilla->cgi->header( -status => "$code $message" );
+
     # apache will generate non-200 response pages for us
     say html_quote($message) if $code == 200;
 }
@@ -176,17 +187,17 @@ sub respond {
 sub decode_json_wrapper {
     my ($json) = @_;
     my $result;
-    if (!defined $json) {
-        warn 'SES: Missing JSON from ' . remote_ip() . "\n";
-        respond(400 => 'Bad Request');
+    if ( !defined $json ) {
+        WARN( 'Missing JSON from ' . remote_ip() );
+        respond( 400 => 'Bad Request' );
         return undef;
     }
     my $ok = try {
         $result = decode_json($json);
     }
     catch {
-        warn 'SES: Malformed JSON from ' . remote_ip() . "\n";
-        respond(400 => 'Bad Request');
+        WARN( 'Malformed JSON from ' . remote_ip() );
+        respond( 400 => 'Bad Request' );
         return undef;
     };
     return $ok ? $result : undef;
@@ -195,9 +206,9 @@ sub decode_json_wrapper {
 sub ua {
     my $ua = LWP::UserAgent->new();
     $ua->timeout(10);
-    $ua->protocols_allowed(['http', 'https']);
-    if (my $proxy_url = Bugzilla->params->{'proxy_url'}) {
-        $ua->proxy(['http', 'https'], $proxy_url);
+    $ua->protocols_allowed( [ 'http', 'https' ] );
+    if ( my $proxy_url = Bugzilla->params->{'proxy_url'} ) {
+        $ua->proxy( [ 'http', 'https' ], $proxy_url );
     }
     else {
         $ua->env_proxy;