Bug 1367604 - BMO extension to apply security policies to Phabricator revisions as needed

* - Updated based on dylans review
- Fixed custom policy to instead allow projects and subscribers and then
  add BMO roles to the subscriber list
- Some other bug fixes

* fix lifetime of phabricator_url_re()

Instead of passing the value (which depends on runtime configuration)
pass in a reference.
Also edit extensions/BMO/Extension.pm to allow %autodetect_attach_urls
regex option to be a callback instead of just a plain regexp ref.

* - Fixed regex in BMO extension to detect phabricator attachments
- Use request_cache for useragent handle in Util.pm

9 files changed, 449 insertions, 120 deletions

diff --git a/extensions/BMO/Extension.pm b/extensions/BMO/Extension.pm
index 2a84cc077..99986f89f 100644
--- a/extensions/BMO/Extension.pm
+++ b/extensions/BMO/Extension.pm
@@ -1158,7 +1158,11 @@ sub _detect_attached_url {
     return if $url =~ m<[^A-Za-z0-9._~:/?#\[\]@!\$&'()*+,;=`.%-]>;
 
     foreach my $key (keys %autodetect_attach_urls) {
-        if ($url =~ $autodetect_attach_urls{$key}->{regex}) {
+        my $regex = $autodetect_attach_urls{$key}->{regex};
+        if (ref($regex) eq 'CODE') {
+            $regex = $regex->();
+        }
+        if ($url =~ $regex) {
             return $autodetect_attach_urls{$key};
         }
     }
diff --git a/extensions/BMO/lib/Data.pm b/extensions/BMO/lib/Data.pm
index fcb96a558..dbf0de108 100644
--- a/extensions/BMO/lib/Data.pm
+++ b/extensions/BMO/lib/Data.pm
@@ -42,6 +42,11 @@ my $mozreview_url_re = qr{
     $
 }ix;
 
+sub phabricator_url_re {
+    my $phab_uri = Bugzilla->params->{phabricator_base_uri} || 'https://example.com';
+    return qr/^\Q${phab_uri}\ED\d+$/i;
+}
+
 our %autodetect_attach_urls = (
     github_pr => {
         title        => 'GitHub Pull Request',
@@ -55,6 +60,12 @@ our %autodetect_attach_urls = (
         content_type => 'text/x-review-board-request',
         can_review   => 1,
     },
+    Phabricator => {
+        title        => 'Phabricator',
+        regex        => \&phabricator_url_re,
+        content_type => 'text/x-phabricator-request',
+        can_review   => 1,
+    },
     google_docs => {
         title        => 'Google Doc',
         regex        => qr#^https://docs\.google\.com/(?:document|spreadsheets|presentation)/d/#i,
diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm
index 6c81e17eb..68090aa10 100644
--- a/extensions/PhabBugz/Extension.pm
+++ b/extensions/PhabBugz/Extension.pm
@@ -35,4 +35,9 @@ sub auth_delegation_confirm {
     }
 }
 
+sub webservice {
+    my ($self,  $args) = @_;
+    $args->{dispatch}->{PhabBugz} = "Bugzilla::Extension::PhabBugz::WebService";
+}
+
 __PACKAGE__->NAME;
diff --git a/extensions/PhabBugz/bin/update_project_members.pl b/extensions/PhabBugz/bin/update_project_members.pl
index 0aa51e17d..6cea1b431 100755
--- a/extensions/PhabBugz/bin/update_project_members.pl
+++ b/extensions/PhabBugz/bin/update_project_members.pl
@@ -20,12 +20,16 @@ use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Group;
 
-use LWP::UserAgent;
-use JSON qw(encode_json decode_json);
+use Bugzilla::Extension::PhabBugz::Util qw(
+    create_project
+    get_members_by_bmo_id
+    get_project_phid
+    set_project_members
+);
 
 Bugzilla->usage_mode(USAGE_MODE_CMDLINE);
 
-my ($phab_uri, $phab_api_key, $phab_sync_groups, $ua);
+my ($phab_uri, $phab_api_key, $phab_sync_groups);
 
 if (!Bugzilla->params->{phabricator_enabled}) {
     exit;
@@ -59,23 +63,21 @@ foreach my $group (@$sync_groups) {
 
     # Create group project if one does not yet exist
     my $phab_project_name = 'bmo-' . $group->name;
-    my $project_id = get_phab_project($phab_project_name);
-    if (!$project_id) {
-        $project_id = create_phab_project($phab_project_name, 'BMO Security Group for ' . $group->name);
+    my $project_phid = get_project_phid($phab_project_name);
+    if (!$project_phid) {
+        $project_phid = create_project($phab_project_name, 'BMO Security Group for ' . $group->name);
     }
 
     # Get the internal user ids for the bugzilla group members
     my $phab_user_ids = [];
     if (@users) {
-        $phab_user_ids = get_phab_members_by_bmo_id(\@users);
+        $phab_user_ids = get_members_by_bmo_id(\@users);
     }
 
     # Set the project members to the exact list
-    set_phab_project_members($project_id, $phab_user_ids);
+    set_project_members($project_phid, $phab_user_ids);
 }
 
-# Bugzilla
-
 sub get_group_members {
     my ($group) = @_;
     my $group_obj = ref $group ? $group : Bugzilla::Group->check({ name => $group });
@@ -88,107 +90,3 @@ sub get_group_members {
     }
     return values %users;
 }
-
-# Projects
-
-sub get_phab_project {
-    my ($project) = @_;
-
-    my $data = {
-        queryKey => 'active',
-        constraints => {
-            name => $project
-        }
-    };
-
-    my $result = request('project.search', $data);
-    if (!$result->{result}{data}) {
-        return undef;
-    }
-    return $result->{result}{data}[0]{phid};
-}
-
-sub create_phab_project {
-    my ($project, $description, $members) = @_;
-
-    my $data = {
-        transactions => [
-            { type => 'name',  value => $project },
-            { type => 'description', value => $description },
-            { type => 'edit',  value => 'admin'},
-            { type => 'join',  value => 'admin' },
-            { type => 'icon',  value => 'group' },
-            { type => 'color', value => 'red' }
-        ]
-    };
-
-    my $result = request('project.edit', $data);
-    return $result->{result}{object}{phid};
-}
-
-sub set_phab_project_members {
-    my ($project_id, $phab_user_ids) = @_;
-
-    my $data = {
-        objectIdentifier => $project_id,
-        transactions => [
-            { type => 'members.set',  value => $phab_user_ids }
-        ]
-    };
-
-    my $result = request('project.edit', $data);
-    return $result->{result}{object}{phid};
-}
-
-# Members
-
-sub get_phab_members_by_bmo_id {
-    my ($users) = @_;
-
-    my $data = {
-        accountids => [ map { $_->id } @$users ]
-    };
-
-    my $result = request('bmoexternalaccount.search', $data);
-    if (!$result->{result}) {
-        return [];
-    }
-
-    my @phab_ids;
-    foreach my $user (@{ $result->{result} }) {
-        push(@phab_ids, $user->{phid});
-    }
-    return \@phab_ids;
-}
-
-# Utility
-
-sub request {
-    my ($method, $data) = @_;
-
-    if (!$ua) {
-        $ua = LWP::UserAgent->new(timeout => 10);
-        if (Bugzilla->params->{proxy_url}) {
-            $ua->proxy('https', Bugzilla->params->{proxy_url});
-        }
-        $ua->default_header('Content-Type' => 'application/x-www-form-urlencoded');
-    }
-
-    my $full_uri = $phab_uri . '/api/' . $method;
-
-    $data->{__conduit__} = { token => $phab_api_key };
-
-    my $response = $ua->post($full_uri, { params => encode_json($data) });
-
-    $response->is_error
-        && ThrowCodeError('phabricator_api_error',
-                          { reason => $response->message });
-
-    my $result = decode_json($response->content);
-    if ($result->{error_code}) {
-        ThrowCodeError('phabricator_api_error',
-                       { code   => $result->{error_code},
-                         reason => $result->{error_info} });
-    }
-    return $result;
-}
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
new file mode 100644
index 000000000..d86cd01a5
--- /dev/null
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -0,0 +1,277 @@
+# 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::Util;
+
+use 5.10.1;
+use strict;
+use warnings;
+
+use Bugzilla::Error;
+
+use Data::Dumper;
+use JSON qw(encode_json decode_json);
+use LWP::UserAgent;
+
+use base qw(Exporter);
+
+our @EXPORT = qw(
+    create_revision_attachment
+    create_private_revision_policy
+    create_project
+    edit_revision_policy
+    get_bug_role_phids
+    get_members_by_bmo_id
+    get_project_phid
+    get_revision_by_id
+    intersect
+    make_revision_public
+    request
+    set_project_members
+);
+
+sub get_revision_by_id {
+    my $id = shift;
+
+    my $data = {
+        queryKey => 'all',
+        constraints => {
+            ids => [ int($id) ]
+        }
+    };
+
+    my $result = request('differential.revision.search', $data);
+
+    ThrowUserError('invalid_phabricator_revision_id')
+        unless (exists $result->{result}{data} && @{ $result->{result}{data} });
+
+    return $result->{result}{data}[0];
+}
+
+sub create_revision_attachment {
+    my ($bug, $revision_id, $revision_title) = @_;
+
+    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',
+    });
+
+    $bug->update($timestamp);
+    $attachment->update($timestamp);
+
+    $dbh->bz_commit_transaction;
+
+    return $attachment;
+}
+
+sub intersect {
+    my ($list1, $list2) = @_;
+    my %e = map { $_ => undef } @{$list1};
+    return grep { exists( $e{$_} ) } @{$list2};
+}
+
+sub get_bug_role_phids {
+    my ($bug) = @_;
+
+    my @bug_users = ( $bug->reporter );
+    push(@bug_users, $bug->assigned_to)
+        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 };
+
+    return get_members_by_bmo_id(\@bug_users);
+}
+
+sub create_private_revision_policy {
+    my ($bug, $groups) = @_;
+
+    my $project_phids = [];
+    foreach my $group (@$groups) {
+        my $phid = get_project_phid('bmo-' . $group);
+        push(@$project_phids, $phid) if $phid;
+    }
+
+    ThrowUserError('invalid_phabricator_sync_groups') unless @$project_phids;
+
+    my $data = {
+        objectType => 'DREV',
+        default    => 'deny',
+        policy     => [
+            {
+                action => 'allow',
+                rule   => 'PhabricatorProjectsPolicyRule',
+                value  => $project_phids,
+            },
+            {
+                action => 'allow',
+                rule   => 'PhabricatorSubscriptionsSubscribersPolicyRule',
+            }
+        ]
+    };
+
+    my $result = request('policy.create', $data);
+    return $result->{result}{phid};
+}
+
+sub make_revision_public {
+    my ($revision_phid) = @_;
+    return request('differential.revision.edit', {
+        transactions => [
+            {
+                type  => "view",
+                value => "users"
+            }
+        ],
+        objectIdentifier => $revision_phid
+    });
+}
+
+sub edit_revision_policy {
+    my ($revision_phid, $policy_phid, $subscribers) = @_;
+
+    my $data = {
+        transactions => [
+            {
+                type  => 'view',
+                value => $policy_phid
+            },
+            {
+                type  => 'edit',
+                value => $policy_phid
+            }
+        ],
+        objectIdentifier => $revision_phid
+    };
+
+    if (@$subscribers) {
+        push(@{ $data->{transactions} }, {
+            type  => 'subscribers.add',
+            value => $subscribers
+        });
+    }
+
+    return request('differential.revision.edit', $data);
+}
+
+sub get_project_phid {
+    my $project = shift;
+
+    my $data = {
+        queryKey => 'all',
+        constraints => {
+            name => $project
+        }
+    };
+
+    my $result = request('project.search', $data);
+    return undef 
+        unless (exists $result->{result}{data} && @{ $result->{result}{data} });
+
+    return $result->{result}{data}[0]{phid};
+}
+
+sub create_project {
+    my ($project, $description, $members) = @_;
+
+    my $data = {
+        transactions => [
+            { type => 'name',  value => $project           },
+            { type => 'description', value => $description },
+            { type => 'edit',  value => 'admin'            },
+            { type => 'join',  value => 'admin'            },
+            { type => 'view',  value => 'admin'            },
+            { type => 'icon',  value => 'group'            },
+            { type => 'color', value => 'red'              }
+        ]
+    };
+
+    my $result = request('project.edit', $data);
+    return $result->{result}{object}{phid};
+}
+
+sub set_project_members {
+    my ($project_id, $phab_user_ids) = @_;
+
+    my $data = {
+        objectIdentifier => $project_id,
+        transactions => [
+            { type => 'members.set',  value => $phab_user_ids }
+        ]
+    };
+
+    my $result = request('project.edit', $data);
+    return $result->{result}{object}{phid};
+}
+
+sub get_members_by_bmo_id {
+    my $users = shift;
+
+    my $data = {
+        accountids => [ map { $_->id } @$users ]
+    };
+
+    my $result = request('bmoexternalaccount.search', $data);
+    return [] if (!$result->{result});
+
+    my @phab_ids;
+    foreach my $user (@{ $result->{result} }) {
+        push(@phab_ids, $user->{phid});
+    }
+
+    return \@phab_ids;
+}
+
+sub request {
+    my ($method, $data) = @_;
+    my $request_cache = Bugzilla->request_cache;
+    my $params        = Bugzilla->params;
+
+    my $ua = $request_cache->{phabricato_ua};
+    unless ($ua) {
+        $ua = $request_cache->{phabricator_ua} = LWP::UserAgent->new(timeout => 10);
+        if ($params->{proxy_url}) {
+            $ua->proxy('https', $params->{proxy_url});
+        }
+        $ua->default_header('Content-Type' => 'application/x-www-form-urlencoded');
+    }
+
+    my $phab_api_key  = $params->{phabricator_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;
+
+    $data->{__conduit__} = { token => $phab_api_key };
+
+    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} });
+    }
+
+    return $result;
+}
+
+1;
diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm
new file mode 100644
index 000000000..0e2574582
--- /dev/null
+++ b/extensions/PhabBugz/lib/WebService.pm
@@ -0,0 +1,112 @@
+# 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::WebService;
+
+use 5.10.1;
+use strict;
+use warnings;
+
+use base qw(Bugzilla::WebService);
+
+use Bugzilla::Attachment;
+use Bugzilla::Bug;
+use Bugzilla::BugMail;
+use Bugzilla::Error;
+use Bugzilla::User;
+use Bugzilla::Util qw(correct_urlbase detaint_natural);
+use Bugzilla::WebService::Constants;
+
+use Bugzilla::Extension::PhabBugz::Util qw(
+    create_revision_attachment
+    create_private_revision_policy
+    edit_revision_policy
+    get_bug_role_phids
+    get_project_phid
+    get_revision_by_id
+    intersect
+    make_revision_public
+    request
+);
+
+use Data::Dumper;
+
+use constant PUBLIC_METHODS => qw(
+    revision
+);
+
+sub revision {
+    my ($self, $params) = @_;
+
+    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 $revision = get_revision_by_id($revision_id);
+
+    my $revision_phid  = $revision->{phid};
+    my $revision_title = $revision->{fields}{title} || 'Unknown Description';
+    my $bug_id         = $revision->{fields}{'bugzilla.bug-id'};
+
+    my $bug = Bugzilla::Bug->check($bug_id);
+
+    # If bug is public then remove privacy policy
+    my $result;
+    if (!@{ $bug->groups_in }) {
+        $result = make_revision_public($revision_id);
+    }
+    # Else bug is private
+    else {
+        my $phab_sync_groups = Bugzilla->params->{phabricator_sync_groups}
+            || ThrowUserError('invalid_phabricator_sync_groups');
+        my $sync_group_names = [ split('[,\s]+', $phab_sync_groups) ];
+
+        my $bug_groups = $bug->groups_in;
+        my $bug_group_names = [ map { $_->name } @$bug_groups ];
+
+        my @set_groups = intersect($bug_group_names, $sync_group_names);
+
+        # If bug privacy groups do not have any matching synchronized groups,
+        # then leave revision private and it will have be dealt with manually.
+        if (!@set_groups) {
+            ThrowUserError('invalid_phabricator_sync_groups');
+        }
+
+        my $policy_phid = create_private_revision_policy($bug, \@set_groups);
+        my $subscribers = get_bug_role_phids($bug);
+        $result = edit_revision_policy($revision_phid, $policy_phid, $subscribers);
+    }
+
+    my $attachment = create_revision_attachment($bug, $revision_id, $revision_title);
+
+    Bugzilla::BugMail::Send($bug_id, { changer => $user });
+
+    return {
+        result          => $result,
+        attachment_id   => $attachment->id,
+        attachment_link => correct_urlbase() . "attachment.cgi?id=" . $attachment->id
+    };
+}
+
+sub rest_resources {
+    return [
+        qr{^/phabbugz/revision/([^/]+)$}, {
+            POST => {
+                method => 'revision',
+                params => sub {
+                    return { revision => $_[0] };
+                }
+            }
+        }
+    ];
+}
+
+1;
diff --git a/extensions/PhabBugz/template/en/default/hook/attachment/edit-view.html.tmpl b/extensions/PhabBugz/template/en/default/hook/attachment/edit-view.html.tmpl
new file mode 100644
index 000000000..17db585f8
--- /dev/null
+++ b/extensions/PhabBugz/template/en/default/hook/attachment/edit-view.html.tmpl
@@ -0,0 +1,17 @@
+[%# 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.
+  #%]
+
+[%
+  IF attachment.mimetype == "text/x-phabricator-request" && attachment.external_redirect;
+  custom_attachment_viewer = 1;
+  url = attachment.data;
+%]
+  <h3>
+    <a href="[% url FILTER html %]" title="[% url FILTER html %]">Show review on Phabricator</a><br>
+  </h3>
+[% END %]
diff --git a/extensions/PhabBugz/template/en/default/hook/global/user-error-errors.html.tmpl b/extensions/PhabBugz/template/en/default/hook/global/user-error-errors.html.tmpl
index 6959c759d..60cd08923 100644
--- a/extensions/PhabBugz/template/en/default/hook/global/user-error-errors.html.tmpl
+++ b/extensions/PhabBugz/template/en/default/hook/global/user-error-errors.html.tmpl
@@ -18,5 +18,8 @@
   [% title = "Invalid Phabricator Sync Groups" %]
   You must provide a comma delimited list of security groups
   to sync with Phabricator.
+[% ELSIF error == "invalid_phabricator_revision_id" %]
+  [% title = "Invalid Phabricator Revision ID" %]
+  You must provide a valid Phabricator revision ID.
 
 [% END %]
diff --git a/extensions/UserProfile/lib/Util.pm b/extensions/UserProfile/lib/Util.pm
index 509b131c1..e50260af2 100644
--- a/extensions/UserProfile/lib/Util.pm
+++ b/extensions/UserProfile/lib/Util.pm
@@ -74,9 +74,10 @@ EOF
     SELECT COUNT(*)
       FROM attachments
      WHERE submitter_id = ?
-           AND (ispatch = 1
-                OR mimetype = 'text/x-github-pull-request'
-                OR mimetype = 'text/x-review-board-request')
+           AND (ispatch = 1 
+                OR mimetype IN ('text/x-github-pull-request',
+                                'text/x-review-board-request',
+                                'text/x-phabricator-request'))
 EOF
 
     # patches reviewed
@@ -86,8 +87,9 @@ EOF
            INNER JOIN attachments ON attachments.attach_id = flags.attach_id
      WHERE setter_id = ?
            AND (attachments.ispatch = 1
-                OR attachments.mimetype = 'text/x-github-pull-request'
-                OR attachments.mimetype = 'text/x-review-board-request')
+                OR mimetype IN ('text/x-github-pull-request',
+                                'text/x-review-board-request',
+                                'text/x-phabricator-request'))
            AND status IN ('+', '-')
 EOF