Bug 343809: Merge FlagType::validate() with Flag::validate() - Patch by Frédéric Buclin <LpSolit@gmail.com> a=myk

6 files changed, 193 insertions, 279 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 90ec68974..ab2562521 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -729,8 +729,8 @@ sub insert_attachment_for_bug {
         $isurl = 0;
     }
 
-    # The order of these function calls is important, as both Flag::validate
-    # and FlagType::validate assume User::match_field has ensured that the
+    # The order of these function calls is important, as Flag::validate
+    # assumes User::match_field has ensured that the
     # values in the requestee fields are legitimate user email addresses.
     my $match_status = Bugzilla::User::match_field($cgi, {
         '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' },
@@ -744,13 +744,11 @@ sub insert_attachment_for_bug {
         $$hr_vars->{'message'} = 'user_match_multiple';
     }
 
-    # FlagType::validate() and Flag::validate() should not detect
-    # any reference to existing flags when creating a new attachment.
-    # Setting the third param to -1 will force this function to check this
-    # point.
+    # Flag::validate() should not detect any reference to existing flags
+    # when creating a new attachment. Setting the third param to -1 will
+    # force this function to check this point.
     # XXX needs $throw_error treatment
     Bugzilla::Flag::validate($cgi, $bug->bug_id, -1);
-    Bugzilla::FlagType::validate($cgi, $bug->bug_id, -1);
 
     # Escape characters in strings that will be used in SQL statements.
     my $description = $cgi->param('description');
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 04623524e..8b09102d1 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -96,6 +96,14 @@ Returns the ID of the flag.
 
 Returns the name of the flagtype the flag belongs to.
 
+=item C<bug_id>
+
+Returns the ID of the bug this flag belongs to.
+
+=item C<attach_id>
+
+Returns the ID of the attachment this flag belongs to, if any.
+
 =item C<status>
 
 Returns the status '+', '-', '?' of the flag.
@@ -106,6 +114,8 @@ Returns the status '+', '-', '?' of the flag.
 
 sub id     { return $_[0]->{'id'};     }
 sub name   { return $_[0]->type->name; }
+sub bug_id { return $_[0]->{'bug_id'}; }
+sub attach_id { return $_[0]->{'attach_id'}; }
 sub status { return $_[0]->{'status'}; }
 
 ###############################
@@ -235,152 +245,203 @@ to -1 to force its check anyway.
 sub validate {
     my ($cgi, $bug_id, $attach_id) = @_;
 
-    my $user = Bugzilla->user;
     my $dbh = Bugzilla->dbh;
 
     # Get a list of flags to validate.  Uses the "map" function
     # to extract flag IDs from form field names by matching fields
-    # whose name looks like "flag-nnn", where "nnn" is the ID,
-    # and returning just the ID portion of matching field names.
-    my @ids = map(/^flag-(\d+)$/ ? $1 : (), $cgi->param());
+    # whose name looks like "flag_type-nnn" (new flags) or "flag-nnn"
+    # (existing flags), where "nnn" is the ID, and returning just
+    # the ID portion of matching field names.
+    my @flagtype_ids = map(/^flag_type-(\d+)$/ ? $1 : (), $cgi->param());
+    my @flag_ids = map(/^flag-(\d+)$/ ? $1 : (), $cgi->param());
+
+    return unless (scalar(@flagtype_ids) || scalar(@flag_ids));
 
-    return unless scalar(@ids);
-    
     # No flag reference should exist when changing several bugs at once.
     ThrowCodeError("flags_not_available", { type => 'b' }) unless $bug_id;
 
-    # No reference to existing flags should exist when creating a new
-    # attachment.
-    if ($attach_id && ($attach_id < 0)) {
-        ThrowCodeError("flags_not_available", { type => 'a' });
+    # We don't check that these new flags are valid for this bug/attachment,
+    # because the bug may be moved into another product meanwhile.
+    # This check will be done later when creating new flags, see FormToNewFlags().
+
+    # All new flags must belong to active flag types.
+    if (scalar(@flagtype_ids)) {
+        my $inactive_flagtypes =
+            $dbh->selectrow_array('SELECT 1 FROM flagtypes
+                                   WHERE id IN (' . join(',', @flagtype_ids) . ')
+                                   AND is_active = 0 ' .
+                                   $dbh->sql_limit(1));
+
+        ThrowCodeError('flag_type_inactive') if $inactive_flagtypes;
     }
 
-    # Make sure all flags belong to the bug/attachment they pretend to be.
-    my $field = ($attach_id) ? "attach_id" : "bug_id";
-    my $field_id = $attach_id || $bug_id;
-    my $not = ($attach_id) ? "" : "NOT";
-
-    my $invalid_data =
-        $dbh->selectrow_array("SELECT 1 FROM flags
-                               WHERE id IN (" . join(',', @ids) . ")
-                               AND ($field != ? OR attach_id IS $not NULL) " .
-                               $dbh->sql_limit(1),
-                               undef, $field_id);
-
-    if ($invalid_data) {
-        ThrowCodeError("invalid_flag_association",
-                       { bug_id    => $bug_id,
-                         attach_id => $attach_id });
+    if (scalar(@flag_ids)) {
+        # No reference to existing flags should exist when creating a new
+        # attachment.
+        if ($attach_id && ($attach_id < 0)) {
+            ThrowCodeError('flags_not_available', { type => 'a' });
+        }
+
+        # Make sure all existing flags belong to the bug/attachment
+        # they pretend to be.
+        my $field = ($attach_id) ? "attach_id" : "bug_id";
+        my $field_id = $attach_id || $bug_id;
+        my $not = ($attach_id) ? "" : "NOT";
+
+        my $invalid_data =
+            $dbh->selectrow_array("SELECT 1 FROM flags
+                                   WHERE id IN (" . join(',', @flag_ids) . ")
+                                   AND ($field != ? OR attach_id IS $not NULL) " .
+                                   $dbh->sql_limit(1),
+                                   undef, $field_id);
+
+        if ($invalid_data) {
+            ThrowCodeError('invalid_flag_association',
+                           { bug_id    => $bug_id,
+                             attach_id => $attach_id });
+        }
     }
 
-    foreach my $id (@ids) {
+    # Validate new flags.
+    foreach my $id (@flagtype_ids) {
+        my $status = $cgi->param("flag_type-$id");
+        my @requestees = $cgi->param("requestee_type-$id");
+        my $private_attachment = $cgi->param('isprivate') ? 1 : 0;
+
+        # Don't bother validating types the user didn't touch.
+        next if $status eq 'X';
+
+        # Make sure the flag type exists.
+        my $flag_type = new Bugzilla::FlagType($id);
+        $flag_type || ThrowCodeError('flag_type_nonexistent', { id => $id });
+
+        _validate(undef, $flag_type, $status, \@requestees, $private_attachment,
+                  $bug_id, $attach_id);
+    }
+
+    # Validate existing flags.
+    foreach my $id (@flag_ids) {
         my $status = $cgi->param("flag-$id");
         my @requestees = $cgi->param("requestee-$id");
+        my $private_attachment = $cgi->param('isprivate') ? 1 : 0;
 
         # Make sure the flag exists.
         my $flag = new Bugzilla::Flag($id);
         $flag || ThrowCodeError("flag_nonexistent", { id => $id });
 
-        # Make sure the user chose a valid status.
-        grep($status eq $_, qw(X + - ?))
-          || ThrowCodeError("flag_status_invalid", 
-                            { id => $id, status => $status });
-
-        # Make sure the user didn't request the flag unless it's requestable.
-        # If the flag was requested before it became unrequestable, leave it
-        # as is.
-        if ($status eq '?'
-            && $flag->status ne '?'
-            && !$flag->type->is_requestable)
-        {
-            ThrowCodeError("flag_status_invalid", 
-                           { id => $id, status => $status });
-        }
+        _validate($flag, $flag->type, $status, \@requestees, $private_attachment);
+    }
+}
 
-        # Make sure the user didn't specify a requestee unless the flag
-        # is specifically requestable. If the requestee was set before
-        # the flag became specifically unrequestable, don't let the user
-        # change the requestee, but let the user remove it by entering
-        # an empty string for the requestee.
-        if ($status eq '?' && !$flag->type->is_requesteeble) {
-            my $old_requestee = $flag->requestee ? $flag->requestee->login : '';
-            my $new_requestee = join('', @requestees);
-            if ($new_requestee && $new_requestee ne $old_requestee) {
-                ThrowCodeError("flag_requestee_disabled",
-                               { type => $flag->type });
-            }
-        }
+sub _validate {
+    my ($flag, $flag_type, $status, $requestees, $private_attachment,
+        $bug_id, $attach_id) = @_;
 
-        # Make sure the user didn't enter multiple requestees for a flag
-        # that can't be requested from more than one person at a time.
-        if ($status eq '?'
-            && !$flag->type->is_multiplicable
-            && scalar(@requestees) > 1)
-        {
-            ThrowUserError("flag_not_multiplicable", { type => $flag->type });
+    my $user = Bugzilla->user;
+
+    my $id = $flag ? $flag->id : $flag_type->id; # Used in the error messages below.
+    $bug_id ||= $flag->bug_id;
+    $attach_id ||= $flag->attach_id if $flag; # Maybe it's a bug flag.
+
+    # Make sure the user chose a valid status.
+    grep($status eq $_, qw(X + - ?))
+      || ThrowCodeError('flag_status_invalid',
+                        { id => $id, status => $status });
+
+    # Make sure the user didn't request the flag unless it's requestable.
+    # If the flag existed and was requested before it became unrequestable,
+    # leave it as is.
+    if ($status eq '?'
+        && (!$flag || $flag->status ne '?')
+        && !$flag_type->is_requestable)
+    {
+        ThrowCodeError('flag_status_invalid',
+                       { id => $id, status => $status });
+    }
+
+    # Make sure the user didn't specify a requestee unless the flag
+    # is specifically requestable. For existing flags, if the requestee
+    # was set before the flag became specifically unrequestable, don't
+    # let the user change the requestee, but let the user remove it by
+    # entering an empty string for the requestee.
+    if ($status eq '?' && !$flag_type->is_requesteeble) {
+        my $old_requestee = ($flag && $flag->requestee) ?
+                                $flag->requestee->login : '';
+        my $new_requestee = join('', @$requestees);
+        if ($new_requestee && $new_requestee ne $old_requestee) {
+            ThrowCodeError('flag_requestee_disabled',
+                           { type => $flag_type });
         }
+    }
 
-        # Make sure the requestees are authorized to access the bug.
-        # (and attachment, if this installation is using the "insider group"
-        # feature and the attachment is marked private).
-        if ($status eq '?' && $flag->type->is_requesteeble) {
-            my $old_requestee = $flag->requestee ? $flag->requestee->login : '';
-            foreach my $login (@requestees) {
-                next if $login eq $old_requestee;
-
-                # We know the requestee exists because we ran
-                # Bugzilla::User::match_field before getting here.
-                my $requestee = new Bugzilla::User({ name => $login });
-                
-                # Throw an error if the user can't see the bug.
-                # Note that if permissions on this bug are changed,
-                # can_see_bug() will refer to old settings.
-                if (!$requestee->can_see_bug($bug_id)) {
-                    ThrowUserError("flag_requestee_unauthorized",
-                                   { flag_type  => $flag->type,
-                                     requestee  => $requestee,
-                                     bug_id     => $bug_id,
-                                     attach_id  => $attach_id
-                                   });
-                }
-    
-                # Throw an error if the target is a private attachment and
-                # the requestee isn't in the group of insiders who can see it.
-                if ($attach_id
-                    && $cgi->param('isprivate')
-                    && Bugzilla->params->{"insidergroup"}
-                    && !$requestee->in_group(Bugzilla->params->{"insidergroup"}))
-                {
-                    ThrowUserError("flag_requestee_unauthorized_attachment",
-                                   { flag_type  => $flag->type,
-                                     requestee  => $requestee,
-                                     bug_id     => $bug_id,
-                                     attach_id  => $attach_id
-                                   });
-                }
+    # Make sure the user didn't enter multiple requestees for a flag
+    # that can't be requested from more than one person at a time.
+    if ($status eq '?'
+        && !$flag_type->is_multiplicable
+        && scalar(@$requestees) > 1)
+    {
+        ThrowUserError('flag_not_multiplicable', { type => $flag_type });
+    }
+
+    # Make sure the requestees are authorized to access the bug
+    # (and attachment, if this installation is using the "insider group"
+    # feature and the attachment is marked private).
+    if ($status eq '?' && $flag_type->is_requesteeble) {
+        my $old_requestee = ($flag && $flag->requestee) ?
+                                $flag->requestee->login : '';
+        foreach my $login (@$requestees) {
+            next if $login eq $old_requestee;
+
+            # We know the requestee exists because we ran
+            # Bugzilla::User::match_field before getting here.
+            my $requestee = new Bugzilla::User({ name => $login });
+
+            # Throw an error if the user can't see the bug.
+            # Note that if permissions on this bug are changed,
+            # can_see_bug() will refer to old settings.
+            if (!$requestee->can_see_bug($bug_id)) {
+                ThrowUserError('flag_requestee_unauthorized',
+                               { flag_type  => $flag_type,
+                                 requestee  => $requestee,
+                                 bug_id     => $bug_id,
+                                 attach_id  => $attach_id });
             }
-        }
 
-        # Make sure the user is authorized to modify flags, see bug 180879
-        # - The flag is unchanged
-        next if ($status eq $flag->status);
-
-        # - User in the request_group can clear pending requests and set flags
-        #   and can rerequest set flags.
-        next if (($status eq 'X' || $status eq '?')
-                 && (!$flag->type->request_group
-                     || $user->in_group_id($flag->type->request_group->id)));
-
-        # - User in the grant_group can set/clear flags, including "+" and "-".
-        next if (!$flag->type->grant_group
-                 || $user->in_group_id($flag->type->grant_group->id));
-
-        # - Any other flag modification is denied
-        ThrowUserError("flag_update_denied",
-                        { name       => $flag->type->name,
-                          status     => $status,
-                          old_status => $flag->status });
+            # Throw an error if the target is a private attachment and
+            # the requestee isn't in the group of insiders who can see it.
+            if ($attach_id
+                && $private_attachment
+                && Bugzilla->params->{'insidergroup'}
+                && !$requestee->in_group(Bugzilla->params->{'insidergroup'}))
+            {
+                ThrowUserError('flag_requestee_unauthorized_attachment',
+                               { flag_type  => $flag_type,
+                                 requestee  => $requestee,
+                                 bug_id     => $bug_id,
+                                 attach_id  => $attach_id });
+            }
+        }
     }
+
+    # Make sure the user is authorized to modify flags, see bug 180879
+    # - The flag exists and is unchanged.
+    return if ($flag && ($status eq $flag->status));
+
+    # - User in the request_group can clear pending requests and set flags
+    #   and can rerequest set flags.
+    return if (($status eq 'X' || $status eq '?')
+               && (!$flag_type->request_group
+                   || $user->in_group_id($flag_type->request_group->id)));
+
+    # - User in the grant_group can set/clear flags, including "+" and "-".
+    return if (!$flag_type->grant_group
+               || $user->in_group_id($flag_type->grant_group->id));
+
+    # - Any other flag modification is denied
+    ThrowUserError('flag_update_denied',
+                    { name       => $flag_type->name,
+                      status     => $status,
+                      old_status => $flag ? $flag->status : 'X' });
 }
 
 sub snapshot {
diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index 47efbd68a..1504be87d 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -359,143 +359,6 @@ sub count {
     return $count;
 }
 
-=pod
-
-=over
-
-=item C<validate($cgi, $bug_id, $attach_id)>
-
-Get a list of flag types to validate.  Uses the "map" function
-to extract flag type IDs from form field names by matching columns
-whose name looks like "flag_type-nnn", where "nnn" is the ID,
-and returning just the ID portion of matching field names.
-
-If the attachment is new, it has no ID yet and $attach_id is set
-to -1 to force its check anyway.
-
-=back
-
-=cut
-
-sub validate {
-    my ($cgi, $bug_id, $attach_id) = @_;
-
-    my $user = Bugzilla->user;
-    my $dbh = Bugzilla->dbh;
-
-    my @ids = map(/^flag_type-(\d+)$/ ? $1 : (), $cgi->param());
-  
-    return unless scalar(@ids);
-    
-    # No flag reference should exist when changing several bugs at once.
-    ThrowCodeError("flags_not_available", { type => 'b' }) unless $bug_id;
-
-    # We don't check that these flag types are valid for
-    # this bug/attachment. This check will be done later when
-    # processing new flags, see Flag::FormToNewFlags().
-
-    # All flag types have to be active
-    my $inactive_flagtypes =
-        $dbh->selectrow_array("SELECT 1 FROM flagtypes
-                               WHERE id IN (" . join(',', @ids) . ")
-                               AND is_active = 0 " .
-                               $dbh->sql_limit(1));
-
-    ThrowCodeError("flag_type_inactive") if $inactive_flagtypes;
-
-    foreach my $id (@ids) {
-        my $status = $cgi->param("flag_type-$id");
-        my @requestees = $cgi->param("requestee_type-$id");
-        
-        # Don't bother validating types the user didn't touch.
-        next if $status eq "X";
-
-        # Make sure the flag type exists.
-        my $flag_type = new Bugzilla::FlagType($id);
-        $flag_type 
-          || ThrowCodeError("flag_type_nonexistent", { id => $id });
-
-        # Make sure the value of the field is a valid status.
-        grep($status eq $_, qw(X + - ?))
-          || ThrowCodeError("flag_status_invalid", 
-                            { id => $id , status => $status });
-
-        # Make sure the user didn't request the flag unless it's requestable.
-        if ($status eq '?' && !$flag_type->is_requestable) {
-            ThrowCodeError("flag_status_invalid", 
-                           { id => $id , status => $status });
-        }
-
-        # Make sure the user didn't specify a requestee unless the flag
-        # is specifically requestable.
-        if ($status eq '?'
-            && !$flag_type->is_requesteeble
-            && scalar(@requestees) > 0)
-        {
-            ThrowCodeError("flag_requestee_disabled", { type => $flag_type });
-        }
-
-        # Make sure the user didn't enter multiple requestees for a flag
-        # that can't be requested from more than one person at a time.
-        if ($status eq '?'
-            && !$flag_type->is_multiplicable
-            && scalar(@requestees) > 1)
-        {
-            ThrowUserError("flag_not_multiplicable", { type => $flag_type });
-        }
-
-        # Make sure the requestees are authorized to access the bug
-        # (and attachment, if this installation is using the "insider group"
-        # feature and the attachment is marked private).
-        if ($status eq '?' && $flag_type->is_requesteeble) {
-            foreach my $login (@requestees) {
-                # We know the requestee exists because we ran
-                # Bugzilla::User::match_field before getting here.
-                my $requestee = new Bugzilla::User({ name => $login });
-    
-                # Throw an error if the user can't see the bug.
-                if (!$requestee->can_see_bug($bug_id)) {
-                    ThrowUserError("flag_requestee_unauthorized",
-                                   { flag_type => $flag_type,
-                                     requestee => $requestee,
-                                     bug_id    => $bug_id,
-                                     attach_id => $attach_id });
-                }
-                
-                # Throw an error if the target is a private attachment and
-                # the requestee isn't in the group of insiders who can see it.
-                if ($attach_id
-                    && Bugzilla->params->{"insidergroup"}
-                    && $cgi->param('isprivate')
-                    && !$requestee->in_group(Bugzilla->params->{"insidergroup"}))
-                {
-                    ThrowUserError("flag_requestee_unauthorized_attachment",
-                                   { flag_type => $flag_type,
-                                     requestee => $requestee,
-                                     bug_id    => $bug_id,
-                                     attach_id => $attach_id });
-                }
-            }
-        }
-
-        # Make sure the user is authorized to modify flags, see bug 180879
-        # - User in the grant_group can set flags, including "+" and "-".
-        next if (!$flag_type->grant_group
-                 || $user->in_group_id($flag_type->grant_group->id));
-
-        # - User in the request_group can request flags.
-        next if ($status eq '?'
-                 && (!$flag_type->request_group
-                     || $user->in_group_id($flag_type->request_group->id)));
-
-        # - Any other flag modification is denied
-        ThrowUserError("flag_update_denied",
-                        { name       => $flag_type->name,
-                          status     => $status,
-                          old_status => "X" });
-    }
-}
-
 ######################################################################
 # Private Functions
 ######################################################################
diff --git a/attachment.cgi b/attachment.cgi
index 6545f149e..7292f014d 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -643,14 +643,13 @@ sub update
     validatePrivate();
     my $dbh = Bugzilla->dbh;
 
-    # The order of these function calls is important, as both Flag::validate
-    # and FlagType::validate assume User::match_field has ensured that the
-    # values in the requestee fields are legitimate user email addresses.
+    # The order of these function calls is important, as Flag::validate
+    # assumes User::match_field has ensured that the values in the
+    # requestee fields are legitimate user email addresses.
     Bugzilla::User::match_field($cgi, {
         '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' }
     });
     Bugzilla::Flag::validate($cgi, $bugid, $attach_id);
-    Bugzilla::FlagType::validate($cgi, $bugid, $attach_id);
 
     my $bug = new Bugzilla::Bug($bugid);
     # Lock database tables in preparation for updating the attachment.
diff --git a/post_bug.cgi b/post_bug.cgi
index f90585020..95621c3ed 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -39,6 +39,7 @@ use Bugzilla::Product;
 use Bugzilla::Component;
 use Bugzilla::Keyword;
 use Bugzilla::Token;
+use Bugzilla::Flag;
 
 my $user = Bugzilla->login(LOGIN_REQUIRED);
 
@@ -447,11 +448,7 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
 my $error_mode_cache = Bugzilla->error_mode;
 Bugzilla->error_mode(ERROR_MODE_DIE);
 eval {
-    # Make sure no flags have already been set for this bug.
-    # Impossible? - Well, depends if you hack the URL or not.
-    # Passing a bug ID of 0 will make it complain if it finds one.
-    Bugzilla::Flag::validate($cgi, 0);
-    Bugzilla::FlagType::validate($cgi, $id);
+    Bugzilla::Flag::validate($cgi, $id);
     Bugzilla::Flag::process($bug, undef, $timestamp, $cgi);
 };
 Bugzilla->error_mode($error_mode_cache);
diff --git a/process_bug.cgi b/process_bug.cgi
index 156f75620..e14900245 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -54,10 +54,7 @@ use Bugzilla::Field;
 use Bugzilla::Product;
 use Bugzilla::Component;
 use Bugzilla::Keyword;
-
-# Use the Flag module to modify flag data if the user set flags.
 use Bugzilla::Flag;
-use Bugzilla::FlagType;
 
 my $user = Bugzilla->login(LOGIN_REQUIRED);
 local our $whoid = $user->id;
@@ -214,8 +211,8 @@ foreach my $field ("dependson", "blocked") {
 
 # do a match on the fields if applicable
 
-# The order of these function calls is important, as both Flag::validate
-# and FlagType::validate assume User::match_field has ensured that the values
+# The order of these function calls is important, as Flag::validate
+# assumes User::match_field has ensured that the values
 # in the requestee fields are legitimate user email addresses.
 &Bugzilla::User::match_field($cgi, {
     'qa_contact'                => { 'type' => 'single' },
@@ -228,7 +225,6 @@ foreach my $field ("dependson", "blocked") {
 # Validate flags in all cases. validate() should not detect any
 # reference to flags if $cgi->param('id') is undefined.
 Bugzilla::Flag::validate($cgi, $cgi->param('id'));
-Bugzilla::FlagType::validate($cgi, $cgi->param('id'));
 
 ######################################################################
 # End Data/Security Validation