Bug 343809: Merge FlagType::validate() with Flag::validate() - Patch by Frédéric…

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');

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
 ######################################################################

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.

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);

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