Bug 1430259 - Update policy code in BMO PhabBugz extension to update custom policy if a private bugs groups have changed.

3 files changed, 295 insertions, 91 deletions

diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 8b6801e91..670bc5d24 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -14,7 +14,10 @@ use List::MoreUtils qw(any);
 use Moo;
 
 use Bugzilla::Constants;
+use Bugzilla::Util qw(diff_arrays);
+
 use Bugzilla::Extension::PhabBugz::Constants;
+use Bugzilla::Extension::PhabBugz::Policy;
 use Bugzilla::Extension::PhabBugz::Revision;
 use Bugzilla::Extension::PhabBugz::Util qw(
     add_security_sync_comments
@@ -104,25 +107,7 @@ sub feed_query {
             }
         }
 
-        my $revision = Bugzilla::Extension::PhabBugz::Revision->new({ phids => [$object_phid] });
-
-        if (!$revision->bug_id) {
-            if ($story_text =~ /\s+created\s+D\d+/) {
-                # If new revision and bug id was omitted, make revision public
-                $self->logger->debug("No bug associated with new revision. Marking public.");
-                $revision->set_policy('view', 'public');
-                $revision->set_policy('edit', 'users');
-                $revision->update();
-                $self->logger->info("SUCCESS");
-            }
-            else {
-                $self->logger->debug("SKIPPING: No bug associated with revision change");
-            }
-            $self->save_feed_last_id($story_id);
-            next;
-        }
-
-        $self->process_revision_change($revision, $story_text);
+        $self->process_revision_change($object_phid, $story_text);
         $self->save_feed_last_id($story_id);
     }
 }
@@ -136,18 +121,28 @@ sub save_feed_last_id {
 }
 
 sub process_revision_change {
-    my ($self, $revision, $story_text) = @_;
+    my ($self, $revision_phid, $story_text) = @_;
 
-    # Pre setup before making changes
-    my $old_user = set_phab_user();
+    # Load the revision from Phabricator
+    my $revision = Bugzilla::Extension::PhabBugz::Revision->new({ phids => [ $revision_phid ] });
 
-    my $is_shadow_db = Bugzilla->is_shadow_db;
-    Bugzilla->switch_to_main_db if $is_shadow_db;
+    # NO BUG ID
 
-    my $dbh = Bugzilla->dbh;
-    $dbh->bz_start_transaction;
-
-    my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
+    if (!$revision->bug_id) {
+        if ($story_text =~ /\s+created\s+D\d+/) {
+            # If new revision and bug id was omitted, make revision public
+            $self->logger->debug("No bug associated with new revision. Marking public.");
+            $revision->set_policy('view', 'public');
+            $revision->set_policy('edit', 'users');
+            $revision->update();
+            $self->logger->info("SUCCESS");
+            return;
+        }
+        else {
+            $self->logger->debug("SKIPPING: No bug associated with revision change");
+            return;
+        }
+    }
 
     my $log_message = sprintf(
         "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s",
@@ -157,39 +152,73 @@ sub process_revision_change {
         $story_text);
     $self->logger->info($log_message);
 
+    # Pre setup before making changes
+    my $old_user = set_phab_user();
+
+    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 $bug = Bugzilla::Bug->new({ id => $revision->bug_id, cache => 1 });
 
     # REVISION SECURITY POLICY
 
-    # Do not set policy if a custom policy has already been set
-    # This keeps from setting new custom policy everytime a change
-    # is made.
-    unless ($revision->view_policy =~ /^PHID-PLCY/) {
-
-        # If bug is public then remove privacy policy
-        if (!@{ $bug->groups_in }) {
-            $revision->set_policy('view', 'public');
-            $revision->set_policy('edit', 'users');
+    # If bug is public then remove privacy policy
+    if (!@{ $bug->groups_in }) {
+        $self->logger->debug('Bug is public so setting view/edit public');
+        $revision->set_policy('view', 'public');
+        $revision->set_policy('edit', 'users');
+    }
+    # 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) {
+            $self->logger->debug('No matching groups. Adding comments to bug and revision');
+            add_security_sync_comments([$revision], $bug);
         }
-        # else bug is private
+        # Otherwise, we create a new custom policy containing the project
+        # groups that are mapped to bugzilla groups.
         else {
-            my @set_groups = get_security_sync_groups($bug);
+            my @set_projects = 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/) {
+                $self->logger->debug("Loading current policy: " . $revision->view_policy);
+                $current_policy
+                    = Bugzilla::Extension::PhabBugz::Policy->new_from_query({ phids => [ $revision->view_policy ]});
+                my $current_projects = $current_policy->rule_projects;
+                $self->logger->debug("Current policy projects: " . join(", ", @$current_projects));
+                my ($added, $removed) = diff_arrays($current_projects, \@set_projects);
+                if (@$added || @$removed) {
+                    $self->logger->debug('Project groups do not match. Need new custom policy');
+                    $current_policy= undef;
+                }
+                else {
+                    $self->logger->debug('Project groups match. Leaving current policy as-is');
+                }
+            }
 
-            # 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) {
-                add_security_sync_comments([$revision], $bug);
+            if (!$current_policy) {
+                $self->logger->debug("Creating new custom policy: " . join(", ", @set_projects));
+                my $new_policy = Bugzilla::Extension::PhabBugz::Policy->create(\@set_projects);
+                $revision->set_policy('view', $new_policy->phid);
+                $revision->set_policy('edit', $new_policy->phid);
             }
 
-            my $policy_phid = create_private_revision_policy($bug, \@set_groups);
             my $subscribers = get_bug_role_phids($bug);
-
-            $revision->set_policy('view', $policy_phid);
-            $revision->set_policy('edit', $policy_phid);
             $revision->set_subscribers($subscribers);
         }
     }
 
+    my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
+
     my $attachment = create_revision_attachment($bug, $revision->id, $revision->title, $timestamp);
 
     # ATTACHMENT OBSOLETES
@@ -203,24 +232,28 @@ sub process_revision_change {
         next if $attach_revision_id != $revision->id;
 
         my $make_obsolete = $revision->status eq 'abandoned' ? 1 : 0;
+        $self->logger->debug('Updating obsolete status on attachmment ' . $attachment->id);
         $attachment->set_is_obsolete($make_obsolete);
 
-        if ($revision->id == $attach_revision_id
-            && $revision->title ne $attachment->description) {
+        if ($revision->title ne $attachment->description) {
+            $self->logger->debug('Updating description on attachment ' . $attachment->id);
             $attachment->set_description($revision->title);
         }
 
         $attachment->update($timestamp);
-        last;
     }
 
     # fixup attachments with same revision id but on different bugs
+    my %other_bugs;
     my $other_attachments = Bugzilla::Attachment->match({
         mimetype => PHAB_CONTENT_TYPE,
         filename => 'phabricator-D' . $revision->id . '-url.txt',
         WHERE    => { 'bug_id != ? AND NOT isobsolete' => $bug->id }
     });
     foreach my $attachment (@$other_attachments) {
+        $other_bugs{$attachment->bug_id}++;
+        $self->logger->debug('Updating obsolete status on attachment ' .
+                             $attachment->id . " for bug " . $attachment->bug_id);
         $attachment->set_is_obsolete(1);
         $attachment->update($timestamp);
     }
@@ -228,9 +261,11 @@ sub process_revision_change {
     # REVIEWER STATUSES
 
     my (@accepted_phids, @denied_phids, @accepted_user_ids, @denied_user_ids);
-    foreach my $reviewer (@{ $revision->reviewers }) {
-        push(@accepted_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'accepted';
-        push(@denied_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'rejected';
+    unless ($revision->status eq 'changes-planned' || $revision->status eq 'needs-review') {
+        foreach my $reviewer (@{ $revision->reviewers }) {
+            push(@accepted_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'accepted';
+            push(@denied_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'rejected';
+        }
     }
 
     my $phab_users = get_phab_bmo_ids({ phids => \@accepted_phids });
@@ -301,7 +336,11 @@ sub process_revision_change {
     $bug->update($timestamp);
     $revision->update();
 
-    Bugzilla::BugMail::Send($revision->bug_id, { changer => Bugzilla->user });
+    # Email changes for this revisions bug and also for any other
+    # bugs that previously had these revision attachments
+    foreach my $bug_id ($revision->bug_id, keys %other_bugs) {
+        Bugzilla::BugMail::Send($bug_id, { changer => Bugzilla->user });
+    }
 
     $dbh->bz_commit_transaction;
     Bugzilla->switch_to_shadow_db if $is_shadow_db;
diff --git a/extensions/PhabBugz/lib/Policy.pm b/extensions/PhabBugz/lib/Policy.pm
new file mode 100644
index 000000000..3205562c3
--- /dev/null
+++ b/extensions/PhabBugz/lib/Policy.pm
@@ -0,0 +1,142 @@
+# 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::Policy;
+
+use 5.10.1;
+use Moo;
+
+use Bugzilla::Error;
+use Bugzilla::Extension::PhabBugz::Util qw(request);
+use Bugzilla::Extension::PhabBugz::Project;
+
+use List::Util qw(first);
+
+use Types::Standard -all;
+use Type::Utils;
+
+has 'phid'      => ( is => 'ro', isa => Str );
+has 'type'      => ( is => 'ro', isa => Str );
+has 'name'      => ( is => 'ro', isa => Str );
+has 'shortName' => ( is => 'ro', isa => Str );
+has 'fullName'  => ( is => 'ro', isa => Str );
+has 'href'      => ( is => 'ro', isa => Maybe[Str] );
+has 'workflow'  => ( is => 'ro', isa => Maybe[Str] );
+has 'icon'      => ( is => 'ro', isa => Str );
+has 'default'   => ( is => 'ro', isa => Str );
+has 'rules' => (
+    is  => 'ro',
+    isa => ArrayRef[
+        Dict[
+            action => Str,
+            rule   => Str,
+            value  => Maybe[ArrayRef[Str]]
+        ]
+    ]
+);
+
+has 'rule_projects' => (
+    is => 'lazy',
+    isa => ArrayRef[Str],
+);
+
+# {
+#   "data": [
+#     {
+#       "phid": "PHID-PLCY-l2mt4yeq4byqgcot7x4j",
+#       "type": "custom",
+#       "name": "Custom Policy",
+#       "shortName": "Custom Policy",
+#       "fullName": "Custom Policy",
+#       "href": null,
+#       "workflow": null,
+#       "icon": "fa-certificate",
+#       "default": "deny",
+#       "rules": [
+#         {
+#           "action": "allow",
+#           "rule": "PhabricatorSubscriptionsSubscribersPolicyRule",
+#           "value": null
+#         },
+#         {
+#           "action": "allow",
+#           "rule": "PhabricatorProjectsPolicyRule",
+#           "value": [
+#             "PHID-PROJ-cvurjiwfvh756mv2vhvi"
+#           ]
+#         }
+#       ]
+#     }
+#   ],
+#   "cursor": {
+#     "limit": 100,
+#     "after": null,
+#     "before": null
+#   }
+# }
+
+sub new_from_query {
+    my ($class, $params) = @_;
+    my $result = request('policy.query', $params);
+    if (exists $result->{result}{data} && @{ $result->{result}{data} }) {
+        return $result->{result}->{data}->[0];
+    }
+    return $class->new($result);
+}
+
+sub create {
+    my ($class, $project_names) = @_;
+
+    my $data = {
+        objectType => 'DREV',
+        default    => 'deny',
+        policy     => [
+            {
+                action => 'allow',
+                rule   => 'PhabricatorSubscriptionsSubscribersPolicyRule',
+            }
+        ]
+    };
+
+    if (@$project_names) {
+        my $project_phids = [];
+        foreach my $project_name (@$project_names) {
+            my $project = Bugzilla::Extension::PhabBugz::Project->new({ name => $project_name });
+            push @$project_phids, $project->phid if $project;
+        }
+
+        ThrowUserError('invalid_phabricator_sync_groups') unless @$project_phids;
+
+        push @{ $data->{policy} }, {
+            action => 'allow',
+            rule   => 'PhabricatorProjectsPolicyRule',
+            value  => $project_phids,
+        };
+    }
+    else {
+        push @{ $data->{policy} }, { action => 'allow', value  => 'admin' };
+    }
+
+    my $result = request('policy.create', $data);
+    return $class->new_from_query({ phids => [ $result->{result}{phid} ] });
+}
+
+sub _build_rule_projects {
+    my ($self) = @_;
+
+    return [] unless $self->rules;
+    my $rule = first { $_->{rule} eq 'PhabricatorProjectsPolicyRule'} @{ $self->rules };
+    return [] unless $rule;
+    return [
+        map  { $_->name }
+        grep { $_ }
+        map  { Bugzilla::Extension::PhabBugz::Project->new( { phids => [$_] } ) }
+        @{ $rule->{value} }
+    ];
+}
+
+1;
\ No newline at end of file
diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm
index 3ad9558ff..ec00b9532 100644
--- a/extensions/PhabBugz/lib/Project.pm
+++ b/extensions/PhabBugz/lib/Project.pm
@@ -18,37 +18,36 @@ use Bugzilla::Extension::PhabBugz::Util qw(
     get_phab_bmo_ids
 );
 
-#########################
-#    Initialization     #
-#########################
-
-sub new {
-    my ($class, $params) = @_;
-    my $self = $params ? _load($params) : {};
-    bless($self, $class);
-    return $self;
-}
-
-sub _load {
-    my ($params) = @_;
-
-    my $data = {
-        queryKey    => 'all',
-        attachments => {
-            projects    => 1,
-            reviewers   => 1,
-            subscribers => 1
-        },
-        constraints => $params
-    };
-
-    my $result = request('project.search', $data);
-    if (exists $result->{result}{data} && @{ $result->{result}{data} }) {
-        return $result->{result}->{data}->[0];
-    }
-
-    return $result;
-}
+use Types::Standard -all;
+use Type::Utils;
+
+my $SearchResult = Dict[
+    id     => Int,
+    type   => Str,
+    phid   => Str,
+    fields => Dict[
+        name         => Str,
+        slug         => Str,
+        depth        => Int,
+        milestone    => Maybe[Str],
+        parent       => Maybe[Str],
+        icon         => Dict[ key => Str, name => Str, icon => Str ],
+        color        => Dict[ key => Str, name => Str ],
+        dateCreated  => Int,
+        dateModified => Int,
+        policy       => Dict[ view => Str, edit => Str, join => Str ],
+        description  => Maybe[Str]
+    ],
+    attachments => Dict[
+        members => Dict[
+            members => ArrayRef[
+                Dict[
+                    phid => Str
+                ],
+            ],
+        ],
+    ],
+];
 
 # {
 #   "data": [
@@ -90,12 +89,6 @@ sub _load {
 #               "phid": "PHID-USER-uif2miph2poiehjeqn5q"
 #             }
 #           ]
-#         },
-#         "ancestors": {
-#           "ancestors": []
-#         },
-#         "watchers": {
-#           "watchers": []
 #         }
 #       }
 #     }
@@ -115,6 +108,36 @@ sub _load {
 # }
 
 #########################
+#    Initialization     #
+#########################
+
+sub new {
+    my ($class, $params) = @_;
+    my $self = $params ? _load($params) : {};
+    $SearchResult->assert_valid($self);
+    return bless($self, $class);
+}
+
+sub _load {
+    my ($params) = @_;
+
+    my $data = {
+        queryKey    => 'all',
+        attachments => {
+            members => 1
+        },
+        constraints => $params
+    };
+
+    my $result = request('project.search', $data);
+    if (exists $result->{result}{data} && @{ $result->{result}{data} }) {
+        return $result->{result}->{data}->[0];
+    }
+
+    return $result;
+}
+
+#########################
 #     Modification      #
 #########################