From c1ca86053ed276aa05eac8468cea61785629ac5e Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Mon, 30 Jun 2008 02:57:54 +0000 Subject: Bug 440612 – Use Bugzilla::Bug->check everywhere instead of ValidateBugID Patch By Max Kanat-Alexander r=LpSolit, a=LpSolit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Bug.pm | 16 ++++++---------- Bugzilla/WebService/Bug.pm | 4 ++-- attachment.cgi | 21 ++++++++++----------- buglist.cgi | 4 ++-- email_in.pl | 5 ++--- enter_bug.cgi | 4 ++-- process_bug.cgi | 15 ++++----------- show_activity.cgi | 8 ++++---- show_bug.cgi | 5 +---- showdependencygraph.cgi | 4 ++-- showdependencytree.cgi | 9 ++++----- summarize_time.cgi | 2 +- votes.cgi | 10 +++++++--- 13 files changed, 47 insertions(+), 60 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index c32672ce2..e2620203e 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -49,7 +49,7 @@ use Storable qw(dclone); use base qw(Bugzilla::Object Exporter); @Bugzilla::Bug::EXPORT = qw( - bug_alias_to_id ValidateBugID + bug_alias_to_id RemoveVotes CheckIfVotedConfirmed LogActivityEntry editable_bug_fields @@ -1091,8 +1091,8 @@ sub _check_dependencies { my @bug_ids = split(/[\s,]+/, $deps_in{$type}); # Eliminate nulls. @bug_ids = grep {$_} @bug_ids; - # We do Validate up here to make sure all aliases are converted to IDs. - ValidateBugID($_, $type) foreach @bug_ids; + # We do this up here to make sure all aliases are converted to IDs. + @bug_ids = map { $invocant->check($_, $type)->id } @bug_ids; my @check_access = @bug_ids; # When we're updating a bug, only added or removed bug_ids are @@ -1114,11 +1114,10 @@ sub _check_dependencies { my $user = Bugzilla->user; foreach my $modified_id (@check_access) { - ValidateBugID($modified_id); + my $delta_bug = $invocant->check($modified_id); # Under strict isolation, you can't modify a bug if you can't # edit it, even if you can see it. if (Bugzilla->params->{"strict_isolation"}) { - my $delta_bug = new Bugzilla::Bug($modified_id); if (!$user->can_edit_product($delta_bug->{'product_id'})) { ThrowUserError("illegal_change_deps", {field => $type}); } @@ -1142,7 +1141,7 @@ sub _check_dup_id { $dupe_of = trim($dupe_of); $dupe_of || ThrowCodeError('undefined_field', { field => 'dup_id' }); # Make sure we can change the original bug (issue A on bug 96085) - ValidateBugID($dupe_of, 'dup_id'); + my $dupe_of_bug = $self->check($dupe_of, 'dup_id'); # Make sure a loop isn't created when marking this bug # as duplicate. @@ -1174,7 +1173,6 @@ sub _check_dup_id { # Should we add the reporter to the CC list of the new bug? # If he can see the bug... if ($self->reporter->can_see_bug($dupe_of)) { - my $dupe_of_bug = new Bugzilla::Bug($dupe_of); # We only add him if he's not the reporter of the other bug. $self->{_add_dup_cc} = 1 if $dupe_of_bug->reporter->id != $self->reporter->id; @@ -1199,9 +1197,7 @@ sub _check_dup_id { my $vars = {}; my $template = Bugzilla->template; # Ask the user what they want to do about the reporter. - $vars->{'cclist_accessible'} = $dbh->selectrow_array( - q{SELECT cclist_accessible FROM bugs WHERE bug_id = ?}, - undef, $dupe_of); + $vars->{'cclist_accessible'} = $dupe_of_bug->cclist_accessible; $vars->{'original_bug_id'} = $dupe_of; $vars->{'duplicate_bug_id'} = $self->id; print $cgi->header(); diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index dd866a67b..3323fd5dd 100755 --- a/Bugzilla/WebService/Bug.pm +++ b/Bugzilla/WebService/Bug.pm @@ -123,7 +123,8 @@ sub get_history { my @return; foreach my $bug_id (@$ids) { my %item; - ValidateBugID($bug_id); + my $bug = Bugzilla::Bug->check($bug_id); + $bug_id = $bug->id; my ($activity) = Bugzilla::Bug::GetBugActivity($bug_id); $item{$bug_id} = []; @@ -155,7 +156,6 @@ sub get_history { # alias is returned in case users passes a mixture of ids and aliases # then they get to know which bug activity relates to which value # they passed - my $bug = new Bugzilla::Bug($bug_id); if (Bugzilla->params->{'usebugaliases'}) { $item{alias} = type('string')->value($bug->alias); } diff --git a/attachment.cgi b/attachment.cgi index 2520c0032..c28a300a0 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -165,8 +165,10 @@ sub validateID { || ThrowUserError("invalid_attach_id", { attach_id => $attach_id }); # Make sure the user is authorized to access this attachment's bug. - ValidateBugID($attachment->bug_id); - if ($attachment->isprivate && $user->id != $attachment->attacher->id && !$user->is_insider) { + Bugzilla::Bug->check($attachment->bug_id); + if ($attachment->isprivate && $user->id != $attachment->attacher->id + && !$user->is_insider) + { ThrowUserError('auth_failure', {action => 'access', object => 'attachment'}); } @@ -281,9 +283,8 @@ sub diff { # HTML page. sub viewall { # Retrieve and validate parameters - my $bugid = $cgi->param('bugid'); - ValidateBugID($bugid); - my $bug = new Bugzilla::Bug($bugid); + my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid')); + my $bugid = $bug->id; my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid); @@ -301,13 +302,12 @@ sub viewall { # Display a form for entering a new attachment. sub enter { # Retrieve and validate parameters - my $bugid = $cgi->param('bugid'); - ValidateBugID($bugid); + my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid')); + my $bugid = $bug->id; validateCanChangeBug($bugid); my $dbh = Bugzilla->dbh; my $user = Bugzilla->user; - my $bug = new Bugzilla::Bug($bugid, $user->id); # Retrieve the attachments the user can edit from the database and write # them into an array of hashes where each hash represents one attachment. my $canEdit = ""; @@ -344,8 +344,8 @@ sub insert { $dbh->bz_start_transaction; # Retrieve and validate parameters - my $bugid = $cgi->param('bugid'); - ValidateBugID($bugid); + my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid')); + my $bugid = $bug->id; validateCanChangeBug($bugid); my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); @@ -373,7 +373,6 @@ sub insert { } } - my $bug = new Bugzilla::Bug($bugid); my $attachment = Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR, $bug, $user, $timestamp, $vars); diff --git a/buglist.cgi b/buglist.cgi index 1ae9f84a7..2d4e323b2 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -562,8 +562,8 @@ elsif (($cgi->param('cmdtype') eq "doit") && defined $cgi->param('remtype')) { my $changes = 0; foreach my $bug_id (split(/[\s,]+/, $cgi->param('bug_ids'))) { next unless $bug_id; - ValidateBugID($bug_id); - $bug_ids{$bug_id} = $keep_bug; + my $bug = Bugzilla::Bug->check($bug_id); + $bug_ids{$bug->id} = $keep_bug; $changes = 1; } ThrowUserError('no_bug_ids', diff --git a/email_in.pl b/email_in.pl index 12be24471..742b84f5f 100644 --- a/email_in.pl +++ b/email_in.pl @@ -41,7 +41,7 @@ use Pod::Usage; use Encode; use Bugzilla; -use Bugzilla::Bug qw(ValidateBugID); +use Bugzilla::Bug; use Bugzilla::Constants qw(USAGE_MODE_EMAIL); use Bugzilla::Error; use Bugzilla::Mailer; @@ -172,8 +172,7 @@ sub process_bug { debug_print("Updating Bug $fields{id}..."); - ValidateBugID($bug_id); - my $bug = new Bugzilla::Bug($bug_id); + my $bug = Bugzilla::Bug->check($bug_id); if ($fields{'bug_status'}) { $fields{'knob'} = $fields{'bug_status'}; diff --git a/enter_bug.cgi b/enter_bug.cgi index ee362e302..9ee58ea53 100755 --- a/enter_bug.cgi +++ b/enter_bug.cgi @@ -350,8 +350,8 @@ my $has_canconfirm = $user->in_group('canconfirm', $product->id); $cloned_bug_id = $cgi->param('cloned_bug_id'); if ($cloned_bug_id) { - ValidateBugID($cloned_bug_id); - $cloned_bug = new Bugzilla::Bug($cloned_bug_id); + $cloned_bug = Bugzilla::Bug->check($cloned_bug_id); + $cloned_bug_id = $cloned_bug->id; } if (scalar(@{$product->components}) == 1) { diff --git a/process_bug.cgi b/process_bug.cgi index bb680fab2..814ff5612 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -112,23 +112,16 @@ sub should_set { # Create a list of objects for all bugs being modified in this request. my @bug_objects; if (defined $cgi->param('id')) { - my $id = $cgi->param('id'); - ValidateBugID($id); - - # Store the validated, and detainted id back in the cgi data, as - # lots of later code will need it, and will obtain it from there - $cgi->param('id', $id); - push(@bug_objects, new Bugzilla::Bug($id)); + my $bug = Bugzilla::Bug->check(scalar $cgi->param('id')); + $cgi->param('id', $bug->id); + push(@bug_objects, $bug); } else { - my @ids; foreach my $i ($cgi->param()) { if ($i =~ /^id_([1-9][0-9]*)/) { my $id = $1; - ValidateBugID($id); - push(@ids, $id); + push(@bug_objects, Bugzilla::Bug->check($id)); } } - @bug_objects = @{Bugzilla::Bug->new_from_list(\@ids)}; } # Make sure there are bugs to process. diff --git a/show_activity.cgi b/show_activity.cgi index d2570f8b1..f7db3dd0b 100755 --- a/show_activity.cgi +++ b/show_activity.cgi @@ -43,17 +43,17 @@ Bugzilla->login(); # Make sure the bug ID is a positive integer representing an existing # bug that the user is authorized to access. -my $bug_id = $cgi->param('id'); -ValidateBugID($bug_id); +my $id = $cgi->param('id'); +my $bug = Bugzilla::Bug->check($id); ############################################################################### # End Data/Security Validation ############################################################################### ($vars->{'operations'}, $vars->{'incomplete_data'}) = - Bugzilla::Bug::GetBugActivity($bug_id); + Bugzilla::Bug::GetBugActivity($bug->id); -$vars->{'bug'} = new Bugzilla::Bug($bug_id); +$vars->{'bug'} = $bug; print $cgi->header(); diff --git a/show_bug.cgi b/show_bug.cgi index 782293af5..b3251b9d1 100755 --- a/show_bug.cgi +++ b/show_bug.cgi @@ -57,10 +57,7 @@ my %marks; if ($single) { my $id = $cgi->param('id'); - # Its a bit silly to do the validation twice - that functionality should - # probably move into Bug.pm at some point - ValidateBugID($id); - push @bugs, new Bugzilla::Bug($id); + push @bugs, Bugzilla::Bug->check($id); if (defined $cgi->param('mark')) { foreach my $range (split ',', $cgi->param('mark')) { if ($range =~ /^(\d+)-(\d+)$/) { diff --git a/showdependencygraph.cgi b/showdependencygraph.cgi index f7977446e..327f73c9d 100755 --- a/showdependencygraph.cgi +++ b/showdependencygraph.cgi @@ -135,8 +135,8 @@ if ($display eq 'doall') { } } else { foreach my $i (split('[\s,]+', $cgi->param('id'))) { - ValidateBugID($i); - $baselist{$i} = 1; + my $bug = Bugzilla::Bug->check($i); + $baselist{$bug->id} = 1; } my @stack = keys(%baselist); diff --git a/showdependencytree.cgi b/showdependencytree.cgi index 80e67716a..8683c190c 100755 --- a/showdependencytree.cgi +++ b/showdependencytree.cgi @@ -49,9 +49,8 @@ my $dbh = Bugzilla->switch_to_shadow_db(); # Make sure the bug ID is a positive integer representing an existing # bug that the user is authorized to access. -my $id = $cgi->param('id') || ThrowUserError('improper_bug_id_field_value'); -ValidateBugID($id); -my $current_bug = new Bugzilla::Bug($id); +my $bug = Bugzilla::Bug->check(scalar $cgi->param('id')); +my $id = $bug->id; local our $hide_resolved = $cgi->param('hide_resolved') ? 1 : 0; @@ -67,7 +66,7 @@ local our $realdepth = 0; # Generate the tree of bugs that this bug depends on and a list of IDs # appearing in the tree. -my $dependson_tree = { $id => $current_bug }; +my $dependson_tree = { $id => $bug }; my $dependson_ids = {}; GenerateTree($id, "dependson", 1, $dependson_tree, $dependson_ids); $vars->{'dependson_tree'} = $dependson_tree; @@ -75,7 +74,7 @@ $vars->{'dependson_ids'} = [keys(%$dependson_ids)]; # Generate the tree of bugs that this bug blocks and a list of IDs # appearing in the tree. -my $blocked_tree = { $id => $current_bug }; +my $blocked_tree = { $id => $bug }; my $blocked_ids = {}; GenerateTree($id, "blocked", 1, $blocked_tree, $blocked_ids); $vars->{'blocked_tree'} = $blocked_tree; diff --git a/summarize_time.cgi b/summarize_time.cgi index 071f89a67..0330f9dcf 100755 --- a/summarize_time.cgi +++ b/summarize_time.cgi @@ -251,7 +251,7 @@ $user->in_group(Bugzilla->params->{"timetrackinggroup"}) object => "timetracking_summaries"}); my @ids = split(",", $cgi->param('id')); -map { ValidateBugID($_) } @ids; +@ids = map { Bugzilla::Bug->check($_)->id } @ids; scalar(@ids) || ThrowUserError('no_bugs_chosen', {action => 'view'}); my $group_by = $cgi->param('group_by') || "number"; diff --git a/votes.cgi b/votes.cgi index 961db7aa5..af41af0e4 100755 --- a/votes.cgi +++ b/votes.cgi @@ -67,7 +67,10 @@ else { # Make sure the bug ID is a positive integer representing an existing # bug that the user is authorized to access. -ValidateBugID($bug_id) if defined $bug_id; +if (defined $bug_id) { + my $bug = Bugzilla::Bug->check($bug_id); + $bug_id = $bug->id; +} ################################################################################ # End Data/Security Validation @@ -244,14 +247,15 @@ sub record_votes { } } - # Call ValidateBugID on each bug ID to make sure it is a positive + # Call check() on each bug ID to make sure it is a positive # integer representing an existing bug that the user is authorized # to access, and make sure the number of votes submitted is also # a non-negative integer (a series of digits not preceded by a # minus sign). my %votes; foreach my $id (@buglist) { - ValidateBugID($id); + my $bug = Bugzilla::Bug->check($id); + $id = $bug->id; $votes{$id} = $cgi->param($id); detaint_natural($votes{$id}) || ThrowUserError("votes_must_be_nonnegative"); -- cgit v1.2.3-24-g4f1b