From 41e381d9d5d1fe53fbf92127c3f65eac4f531f36 Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Thu, 24 Aug 2006 22:56:39 +0000 Subject: Bug 343809: Merge FlagType::validate() with Flag::validate() - Patch by Frédéric Buclin a=myk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Attachment.pm | 12 +- Bugzilla/Flag.pm | 301 +++++++++++++++++++++++++++++-------------------- Bugzilla/FlagType.pm | 137 ---------------------- attachment.cgi | 7 +- post_bug.cgi | 7 +- process_bug.cgi | 8 +- 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 + +Returns the ID of the bug this flag belongs to. + +=item C + +Returns the ID of the attachment this flag belongs to, if any. + =item C 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 - -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 -- cgit v1.2.3-24-g4f1b