Bug 1478897 - ensure phabbugs doesn't fail outright when encountering invalid bug ids

4 files changed, 64 insertions, 95 deletions

diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 69ce2227d..7a5e8d6d6 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -30,10 +30,8 @@ use Bugzilla::Extension::PhabBugz::Policy;
 use Bugzilla::Extension::PhabBugz::Revision;
 use Bugzilla::Extension::PhabBugz::User;
 use Bugzilla::Extension::PhabBugz::Util qw(
-    add_security_sync_comments
     create_revision_attachment
     get_bug_role_phids
-    get_security_sync_groups
     is_attachment_phab_revision
     request
     set_phab_user
@@ -371,7 +369,7 @@ sub process_revision_change {
             return;
         }
     }
-
+    
     my $log_message = sprintf(
         "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s",
         $revision->id,
@@ -384,6 +382,26 @@ sub process_revision_change {
     my $old_user = set_phab_user();
     my $bug = Bugzilla::Bug->new({ id => $revision->bug_id, cache => 1 });
 
+    # Check to make sure bug id is valid and author can see it
+    if ($bug->{error}
+        ||!$revision->author->bugzilla_user->can_see_bug($revision->bug_id))
+    {
+        if ($story_text =~ /\s+created\s+D\d+/) {
+            INFO('Invalid bug ID or author does not have access to the bug. ' .
+                 'Waiting til next revision update to notify author.');
+            return;
+        }
+
+        INFO('Invalid bug ID or author does not have access to the bug');
+        my $phab_error_message = "";
+        Bugzilla->template->process('revision/comments.html.tmpl',
+                                    { message => 'invalid_bug_id' },
+                                    \$phab_error_message);
+        $revision->add_comment($phab_error_message);
+        $revision->update();
+        return;
+    }
+
     # REVISION SECURITY POLICY
 
     # If bug is public then remove privacy policy
@@ -393,48 +411,38 @@ sub process_revision_change {
     }
     # else bug is private.
     else {
-        my @set_groups = get_security_sync_groups($bug);
-
-        # 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) {
-            INFO('No matching groups. Adding comments to bug and revision');
-            add_security_sync_comments([$revision], $bug);
-        }
-        # Otherwise, we create a new custom policy containing the project
+        # Here we create a new custom policy containing the project
         # groups that are mapped to bugzilla groups.
-        else {
-            my $set_project_names = [ map { "bmo-" . $_ } @set_groups ];
-
-            # If current policy projects matches what we want to set, then
-            # we leave the current policy alone.
-            my $current_policy;
-            if ($revision->view_policy =~ /^PHID-PLCY/) {
-                INFO("Loading current policy: " . $revision->view_policy);
-                $current_policy
-                    = Bugzilla::Extension::PhabBugz::Policy->new_from_query({ phids => [ $revision->view_policy ]});
-                my $current_project_names = [ map { $_->name } @{ $current_policy->rule_projects } ];
-                INFO("Current policy projects: " . join(", ", @$current_project_names));
-                my ($added, $removed) = diff_arrays($current_project_names, $set_project_names);
-                if (@$added || @$removed) {
-                    INFO('Project groups do not match. Need new custom policy');
-                    $current_policy = undef;
-                }
-                else {
-                    INFO('Project groups match. Leaving current policy as-is');
-                }
+        my $set_project_names = [ map { "bmo-" . $_ } @{ $bug->groups_in } ];
+
+        # If current policy projects matches what we want to set, then
+        # we leave the current policy alone.
+        my $current_policy;
+        if ($revision->view_policy =~ /^PHID-PLCY/) {
+            INFO("Loading current policy: " . $revision->view_policy);
+            $current_policy
+                = Bugzilla::Extension::PhabBugz::Policy->new_from_query({ phids => [ $revision->view_policy ]});
+            my $current_project_names = [ map { $_->name } @{ $current_policy->rule_projects } ];
+            INFO("Current policy projects: " . join(", ", @$current_project_names));
+            my ($added, $removed) = diff_arrays($current_project_names, $set_project_names);
+            if (@$added || @$removed) {
+                INFO('Project groups do not match. Need new custom policy');
+                $current_policy = undef;
             }
-
-            if (!$current_policy) {
-                INFO("Creating new custom policy: " . join(", ", @$set_project_names));
-                $revision->make_private($set_project_names);
+            else {
+                INFO('Project groups match. Leaving current policy as-is');
             }
+        }
 
-            # Subscriber list of the private revision should always match
-            # the bug roles such as assignee, qa contact, and cc members.
-            my $subscribers = get_bug_role_phids($bug);
-            $revision->set_subscribers($subscribers);
+        if (!$current_policy) {
+            INFO("Creating new custom policy: " . join(", ", @$set_project_names));
+            $revision->make_private($set_project_names);
         }
+
+        # Subscriber list of the private revision should always match
+        # the bug roles such as assignee, qa contact, and cc members.
+        my $subscribers = get_bug_role_phids($bug);
+        $revision->set_subscribers($subscribers);
     }
 
     my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index 5dbe31d0e..34a322240 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -27,12 +27,10 @@ use Try::Tiny;
 use base qw(Exporter);
 
 our @EXPORT = qw(
-    add_security_sync_comments
     create_revision_attachment
     get_attachment_revisions
     get_bug_role_phids
     get_needs_review
-    get_security_sync_groups
     intersect
     is_attachment_phab_revision
     request
@@ -201,20 +199,6 @@ sub request {
     return $result;
 }
 
-sub get_security_sync_groups {
-    my $bug = shift;
-
-    my $sync_groups = Bugzilla::Group->match( { isactive => 1, isbuggroup => 1 } );
-    my $sync_group_names = [ map { $_->name } @$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);
-
-    return @set_groups;
-}
-
 sub set_phab_user {
     my $old_user = Bugzilla->user;
     my $user = Bugzilla::User->new( { name => PHAB_AUTOMATION_USER } );
@@ -223,29 +207,6 @@ sub set_phab_user {
     return $old_user;
 }
 
-sub add_security_sync_comments {
-    my ($revisions, $bug) = @_;
-
-    my $phab_error_message = 'Revision is being made private due to unknown Bugzilla groups.';
-
-    foreach my $revision (@$revisions) {
-        $revision->add_comment($phab_error_message);
-    }
-
-    my $num_revisions = scalar @$revisions;
-    my $bmo_error_message =
-    ( $num_revisions > 1
-    ? $num_revisions.' revisions were'
-    : 'One revision was' )
-    . ' made private due to unknown Bugzilla groups.';
-
-    my $old_user = set_phab_user();
-
-    $bug->add_comment( $bmo_error_message, { isprivate => 0 } );
-
-    Bugzilla->set_user($old_user);
-}
-
 sub get_needs_review {
     my ($user) = @_;
     $user //= Bugzilla->user;
diff --git a/extensions/PhabBugz/template/en/default/revision/comments.html.tmpl b/extensions/PhabBugz/template/en/default/revision/comments.html.tmpl
new file mode 100644
index 000000000..b18daf376
--- /dev/null
+++ b/extensions/PhabBugz/template/en/default/revision/comments.html.tmpl
@@ -0,0 +1,14 @@
+[%# 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 message == "invalid_bug_id" %]
+Revision is being kept private due to invalid [% terms.bug %] ID
+or author does not have access to the [% terms.bug %]. Either remove
+the [% terms.bug %] ID, automatically making the revision public, or
+enter the correct [% terms.bug %] ID for this revision.
+[% END %]
\ No newline at end of file
diff --git a/extensions/Push/lib/Connector/Phabricator.pm b/extensions/Push/lib/Connector/Phabricator.pm
index e59ba6c0d..33e2bb6ad 100644
--- a/extensions/Push/lib/Connector/Phabricator.pm
+++ b/extensions/Push/lib/Connector/Phabricator.pm
@@ -21,10 +21,8 @@ use Bugzilla::Extension::PhabBugz::Policy;
 use Bugzilla::Extension::PhabBugz::Project;
 use Bugzilla::Extension::PhabBugz::Revision;
 use Bugzilla::Extension::PhabBugz::Util qw(
-  add_security_sync_comments
   get_attachment_revisions
   get_bug_role_phids
-  get_security_sync_groups
 );
 
 use Bugzilla::Extension::Push::Constants;
@@ -68,8 +66,6 @@ sub send {
 
     my $is_public = is_public($bug);
 
-    my @set_groups = get_security_sync_groups($bug);
-
     my $revisions = get_attachment_revisions($bug);
 
     my $group_change =
@@ -86,24 +82,14 @@ sub send {
             ));
             $revision->make_public();
         }
-        elsif ( !$is_public && !@set_groups ) {
-            Bugzilla->audit(sprintf(
-              'Making revision %s for bug %s private due to unkown Bugzilla groups: %s',
-              $revision->id,
-              $bug->id,
-              join(', ', @set_groups)
-            ));
-            $revision->make_private(['secure-revision']);
-            add_security_sync_comments([$revision], $bug);
-        }
         elsif ( !$is_public && $group_change ) {
             Bugzilla->audit(sprintf(
               'Giving revision %s a custom policy for bug %s',
               $revision->id,
               $bug->id
             ));
-            my @set_project_names = map { "bmo-" . $_ } @set_groups;
-            $revision->make_private(\@set_project_names);
+            my $set_project_names = [ map { "bmo-" . $_->name } @{ $bug->groups_in } ];
+            $revision->make_private($set_project_names);
         }
 
         # Subscriber list of the private revision should always match