Bug 1382225 - Missing code from PhabBugz extension such as http basic auth support and other minor improvements

3 files changed, 95 insertions, 28 deletions

diff --git a/extensions/PhabBugz/lib/Constants.pm b/extensions/PhabBugz/lib/Constants.pm
new file mode 100644
index 000000000..f7485e8c4
--- /dev/null
+++ b/extensions/PhabBugz/lib/Constants.pm
@@ -0,0 +1,25 @@
+# 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::Extension::PhabBugz::Constants;
+
+use 5.10.1;
+use strict;
+use warnings;
+
+use base qw(Exporter);
+our @EXPORT = qw(
+    PHAB_AUTOMATION_USER
+    PHAB_ATTACHMENT_PATTERN
+    PHAB_CONTENT_TYPE
+);
+
+use constant PHAB_ATTACHMENT_PATTERN => qr/^phabricator-D(\d+)/;
+use constant PHAB_AUTOMATION_USER    => 'phab-bot@bmo.tld';
+use constant PHAB_CONTENT_TYPE       => 'text/x-phabricator-request';
+
+1;
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index b12669c9d..57dc1d7d0 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -12,9 +12,11 @@ use strict;
 use warnings;
 
 use Bugzilla::Error;
+use Bugzilla::Util qw(trim);
+use Bugzilla::Extension::PhabBugz::Constants;
 
-use Data::Dumper;
-use JSON qw(encode_json decode_json);
+use JSON::XS qw(encode_json decode_json);
+use List::Util qw(first);
 use LWP::UserAgent;
 
 use base qw(Exporter);
@@ -30,6 +32,7 @@ our @EXPORT = qw(
     get_project_phid
     get_revisions_by_ids
     intersect
+    is_attachment_phab_revision
     make_revision_private
     make_revision_public
     request
@@ -55,28 +58,47 @@ sub get_revisions_by_ids {
 }
 
 sub create_revision_attachment {
-    my ($bug, $revision_id, $revision_title) = @_;
+    my ( $bug, $revision_id, $revision_title ) = @_;
+
+    my $phab_base_uri = Bugzilla->params->{phabricator_base_uri};
+    ThrowUserError('invalid_phabricator_uri') unless $phab_base_uri;
+
+    my $revision_uri = $phab_base_uri . "D" . $revision_id;
+
+    # Check for previous attachment with same revision id.
+    # If one matches then return it instead. This is fine as
+    # BMO does not contain actual diff content.
+    my @review_attachments = grep { is_attachment_phab_revision($_) } @{ $bug->attachments };
+    my $review_attachment = first { trim($_->data) eq $revision_uri } @review_attachments;
+    return $review_attachment if defined $review_attachment;
+
+    # No attachment is present, so we can now create new one
+    my $is_shadow_db = Bugzilla->is_shadow_db;
+    Bugzilla->switch_to_main_db if $is_shadow_db;
 
     my $dbh = Bugzilla->dbh;
     $dbh->bz_start_transaction;
 
     my ($timestamp) = $dbh->selectrow_array("SELECT NOW()");
 
-    my $attachment = Bugzilla::Attachment->create({
-        bug           => $bug,
-        creation_ts   => $timestamp,
-        data          => 'http://phabricator.test/D' . $revision_id,
-        description   => $revision_title,
-        filename      => 'phabricator-D' . $revision_id . '-url.txt',
-        ispatch       => 0,
-        isprivate     => 0,
-        mimetype      => 'text/x-phabricator-request',
-    });
+    my $attachment = Bugzilla::Attachment->create(
+        {
+            bug         => $bug,
+            creation_ts => $timestamp,
+            data        => $revision_uri,
+            description => $revision_title,
+            filename    => 'phabricator-D' . $revision_id . '-url.txt',
+            ispatch     => 0,
+            isprivate   => 0,
+            mimetype    => PHAB_CONTENT_TYPE,
+        }
+    );
 
     $bug->update($timestamp);
     $attachment->update($timestamp);
 
     $dbh->bz_commit_transaction;
+    Bugzilla->switch_to_shadow_db if $is_shadow_db;
 
     return $attachment;
 }
@@ -92,7 +114,7 @@ sub get_bug_role_phids {
 
     my @bug_users = ( $bug->reporter );
     push(@bug_users, $bug->assigned_to)
-        if !$bug->assigned_to->email !~ /^nobody\@mozilla\.org$/;
+        if $bug->assigned_to->email !~ /^nobody\@mozilla\.org$/;
     push(@bug_users, $bug->qa_contact) if $bug->qa_contact;
     push(@bug_users, @{ $bug->cc_users }) if @{ $bug->cc_users };
 
@@ -271,12 +293,19 @@ sub get_members_by_bmo_id {
     return \@phab_ids;
 }
 
+sub is_attachment_phab_revision {
+    my ($attachment, $include_obsolete) = @_;
+    return ($attachment->contenttype eq PHAB_CONTENT_TYPE
+            && ($include_obsolete || !$attachment->isobsolete)
+            && $attachment->attacher->login eq 'phab-bot@bmo.tld') ? 1 : 0;
+}
+
 sub request {
     my ($method, $data) = @_;
     my $request_cache = Bugzilla->request_cache;
     my $params        = Bugzilla->params;
 
-    my $ua = $request_cache->{phabricato_ua};
+    my $ua = $request_cache->{phabricator_ua};
     unless ($ua) {
         $ua = $request_cache->{phabricator_ua} = LWP::UserAgent->new(timeout => 10);
         if ($params->{proxy_url}) {
@@ -285,10 +314,10 @@ sub request {
         $ua->default_header('Content-Type' => 'application/x-www-form-urlencoded');
     }
 
-    my $phab_api_key  = $params->{phabricator_api_key};
+    my $phab_api_key = $params->{phabricator_api_key};
+    ThrowUserError('invalid_phabricator_api_key') unless $phab_api_key;
     my $phab_base_uri = $params->{phabricator_base_uri};
     ThrowUserError('invalid_phabricator_uri') unless $phab_base_uri;
-    ThrowUserError('invalid_phabricator_api_key') unless $phab_api_key;
 
     my $full_uri = $phab_base_uri . '/api/' . $method;
 
@@ -297,13 +326,14 @@ sub request {
     my $response = $ua->post($full_uri, { params => encode_json($data) });
 
     ThrowCodeError('phabricator_api_error', { reason => $response->message })
-        if $response->is_error;
-
-    my $result = decode_json($response->content);
-    if ($result->{error_code}) {
-        ThrowCodeError('phabricator_api_error',
-                       { code   => $result->{error_code},
-                         reason => $result->{error_info} });
+      if $response->is_error;
+
+    my $result;
+    my $result_ok = eval { $result = decode_json( $response->content); 1 };
+    if ( !$result_ok ) {
+        ThrowCodeError(
+            'phabricator_api_error',
+            { reason => 'JSON decode failure' } );
     }
 
     return $result;
diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm
index 11439ba70..0d88114f6 100644
--- a/extensions/PhabBugz/lib/WebService.pm
+++ b/extensions/PhabBugz/lib/WebService.pm
@@ -16,6 +16,7 @@ use base qw(Bugzilla::WebService);
 use Bugzilla::Attachment;
 use Bugzilla::Bug;
 use Bugzilla::BugMail;
+use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Extension::Push::Util qw(is_public);
 use Bugzilla::User;
@@ -34,7 +35,7 @@ use Bugzilla::Extension::PhabBugz::Util qw(
     request
 );
 
-use Data::Dumper;
+use MIME::Base64 qw(decode_base64);
 
 use constant PUBLIC_METHODS => qw(
     revision
@@ -43,12 +44,23 @@ use constant PUBLIC_METHODS => qw(
 sub revision {
     my ($self, $params) = @_;
 
+    # Phabricator only supports sending credentials via HTTP Basic Auth
+    # so we exploit that function to pass in an API key as the password
+    # of basic auth. BMO does not support basic auth but does support
+    # use of API keys.
+    my $http_auth = Bugzilla->cgi->http('Authorization');
+    $http_auth =~ s/^Basic\s+//;
+    $http_auth = decode_base64($http_auth);
+    my ($login, $api_key) = split(':', $http_auth);
+    $params->{'Bugzilla_login'} = $login;
+    $params->{'Bugzilla_api_key'} = $api_key;
+
+    my $user = Bugzilla->login(LOGIN_REQUIRED);
+
     unless (defined $params->{revision} && detaint_natural($params->{revision})) {
         ThrowCodeError('param_required', { param => 'revision' })
     }
 
-    my $user = Bugzilla->set_user(Bugzilla::User->new({ name => 'conduit@mozilla.bugs' }));
-
     # Obtain more information about the revision from Phabricator
     my $revision_id = $params->{revision};
     my @revisions = get_revisions_by_ids([$revision_id]);
@@ -65,7 +77,7 @@ sub revision {
     if (is_public($bug)) {
         $result = make_revision_public($revision_id);
     }
-    # Else bug is private
+    # else bug is private
     else {
         my $phab_sync_groups = Bugzilla->params->{phabricator_sync_groups}
             || ThrowUserError('invalid_phabricator_sync_groups');