From cc69d134483d1e10423475735b1084535dd075b7 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Sat, 12 Aug 2006 06:45:07 +0000 Subject: Bug 348057: Move the checks for bug visibility out of Bugzilla::Bug->new Patch By Max Kanat-Alexander r=LpSolit, a=myk --- Bugzilla/Bug.pm | 53 +++++-------------------- attachment.cgi | 4 +- editcomponents.cgi | 4 +- editflagtypes.cgi | 6 +-- editproducts.cgi | 4 +- enter_bug.cgi | 2 +- importxml.pl | 2 +- post_bug.cgi | 5 ++- process_bug.cgi | 14 +++---- show_bug.cgi | 12 ++++-- showdependencytree.cgi | 5 ++- template/en/default/bug/show-multiple.html.tmpl | 2 +- votes.cgi | 2 +- 13 files changed, 47 insertions(+), 68 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index afc125457..03be7156c 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -65,36 +65,18 @@ use constant MAX_COMMENT_LENGTH => 65535; ##################################################################### -# create a new empty bug -# sub new { - my $type = shift(); - my %bug; - - # create a ref to an empty hash and bless it - # - my $self = {%bug}; - bless $self, $type; - - # construct from a hash containing a bug's info - # - if ($#_ == 1) { - $self->initBug(@_); - } else { - confess("invalid number of arguments \($#_\)($_)"); - } - - # bless as a Bug - # + my $invocant = shift; + my $class = ref($invocant) || $invocant; + my $self = {}; + bless $self, $class; + $self->_init(@_); return $self; } -# dump info about bug into hash unless user doesn't have permission -# user_id 0 is used when person is not logged in. -# -sub initBug { +sub _init { my $self = shift(); - my ($bug_id, $user_id) = (@_); + my ($bug_id) = (@_); my $dbh = Bugzilla->dbh; $bug_id = trim($bug_id || 0); @@ -104,18 +86,6 @@ sub initBug { # If the bug ID isn't numeric, it might be an alias, so try to convert it. $bug_id = bug_alias_to_id($bug_id) if $bug_id !~ /^0*[1-9][0-9]*$/; - # If the user is not logged in, sets $user_id to 0. - # Else gets $user_id from the user login name if this - # argument is not numeric. - my $stored_user_id = $user_id; - if (!defined $user_id) { - $user_id = 0; - } elsif (!detaint_natural($user_id)) { - $user_id = login_to_id($stored_user_id); - } - - $self->{'who'} = new Bugzilla::User($user_id); - unless ($bug_id && detaint_natural($bug_id)) { # no bug number given or the alias didn't match a bug $self->{'bug_id'} = $old_bug_id; @@ -166,8 +136,7 @@ sub initBug { $bug_sth->execute($bug_id); my @row; - if ((@row = $bug_sth->fetchrow_array()) - && $self->{'who'}->can_see_bug($bug_id)) { + if (@row = $bug_sth->fetchrow_array) { my $count = 0; my %fields; foreach my $field ("bug_id", "alias", "classification_id", "classification", @@ -188,10 +157,6 @@ sub initBug { } $count++; } - } elsif (@row) { - $self->{'bug_id'} = $bug_id; - $self->{'error'} = "NotPermitted"; - return $self; } else { $self->{'bug_id'} = $bug_id; $self->{'error'} = "NotFound"; @@ -1186,7 +1151,7 @@ sub CheckIfVotedConfirmed { sub check_can_change_field { my $self = shift; my ($field, $oldvalue, $newvalue, $PrivilegesRequired, $data) = (@_); - my $user = $self->{'who'}; + my $user = Bugzilla->user; $oldvalue = defined($oldvalue) ? $oldvalue : ''; $newvalue = defined($newvalue) ? $newvalue : ''; diff --git a/attachment.cgi b/attachment.cgi index 8541e8d5e..6545f149e 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -501,7 +501,7 @@ sub insert ValidateComment(scalar $cgi->param('comment')); my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); - my $bug = new Bugzilla::Bug($bugid, $user->id); + my $bug = new Bugzilla::Bug($bugid); my $attachid = Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR, $bug, $user, $timestamp, \$vars); @@ -652,7 +652,7 @@ sub update Bugzilla::Flag::validate($cgi, $bugid, $attach_id); Bugzilla::FlagType::validate($cgi, $bugid, $attach_id); - my $bug = new Bugzilla::Bug($bugid, $userid); + my $bug = new Bugzilla::Bug($bugid); # Lock database tables in preparation for updating the attachment. $dbh->bz_lock_tables('attachments WRITE', 'flags WRITE' , 'flagtypes READ', 'fielddefs READ', 'bugs_activity WRITE', diff --git a/editcomponents.cgi b/editcomponents.cgi index e66e95195..c87bc0313 100755 --- a/editcomponents.cgi +++ b/editcomponents.cgi @@ -252,7 +252,9 @@ if ($action eq 'delete') { if ($component->bug_count) { if (Bugzilla->params->{"allowbugdeletion"}) { foreach my $bug_id (@{$component->bug_ids}) { - my $bug = new Bugzilla::Bug($bug_id, $whoid); + # Note: We allow admins to delete bugs even if they can't + # see them, as long as they can see the product. + my $bug = new Bugzilla::Bug($bug_id); $bug->remove_from_db(); } } else { diff --git a/editflagtypes.cgi b/editflagtypes.cgi index e7b0908b5..1164177f5 100755 --- a/editflagtypes.cgi +++ b/editflagtypes.cgi @@ -393,7 +393,7 @@ sub update { undef, $id); foreach my $flag (@$flags) { my ($flag_id, $bug_id, $attach_id) = @$flag; - my $bug = new Bugzilla::Bug($bug_id, $user->id); + my $bug = new Bugzilla::Bug($bug_id); my $attachment = $attach_id ? Bugzilla::Attachment->get($attach_id) : undef; Bugzilla::Flag::clear($flag_id, $bug, $attachment); } @@ -412,7 +412,7 @@ sub update { undef, $id); foreach my $flag (@$flags) { my ($flag_id, $bug_id, $attach_id) = @$flag; - my $bug = new Bugzilla::Bug($bug_id, $user->id); + my $bug = new Bugzilla::Bug($bug_id); my $attachment = $attach_id ? Bugzilla::Attachment->get($attach_id) : undef; Bugzilla::Flag::clear($flag_id, $bug, $attachment); } @@ -680,4 +680,4 @@ sub filter_group { || ($_->request_group && $_->request_group->id == $gid)} @$flag_types; return \@flag_types; -} \ No newline at end of file +} diff --git a/editproducts.cgi b/editproducts.cgi index 432e2a9c6..4c4394926 100755 --- a/editproducts.cgi +++ b/editproducts.cgi @@ -373,7 +373,9 @@ if ($action eq 'delete') { if ($product->bug_count) { if (Bugzilla->params->{"allowbugdeletion"}) { foreach my $bug_id (@{$product->bug_ids}) { - my $bug = new Bugzilla::Bug($bug_id, $whoid); + # Note that we allow the user to delete bugs he can't see, + # which is okay, because he's deleting the whole Product. + my $bug = new Bugzilla::Bug($bug_id); $bug->remove_from_db(); } } diff --git a/enter_bug.cgi b/enter_bug.cgi index a5a4cedca..269f14317 100755 --- a/enter_bug.cgi +++ b/enter_bug.cgi @@ -307,7 +307,7 @@ $cloned_bug_id = $cgi->param('cloned_bug_id'); if ($cloned_bug_id) { ValidateBugID($cloned_bug_id); - $cloned_bug = new Bugzilla::Bug($cloned_bug_id, $user->id); + $cloned_bug = new Bugzilla::Bug($cloned_bug_id); } if (scalar(@{$product->components}) == 1) { diff --git a/importxml.pl b/importxml.pl index e048aac40..68b2cd34f 100755 --- a/importxml.pl +++ b/importxml.pl @@ -224,7 +224,7 @@ sub flag_handler { } ); } else { - my $bug = new Bugzilla::Bug( $bugid, $exporterid ); + my $bug = new Bugzilla::Bug($bugid); $flag_types = $bug->flag_types; } unless ($flag_types){ diff --git a/post_bug.cgi b/post_bug.cgi index be061b2f1..aac15f3bf 100755 --- a/post_bug.cgi +++ b/post_bug.cgi @@ -537,7 +537,10 @@ $dbh->do("UPDATE bugs SET creation_ts = ? WHERE bug_id = ?", $dbh->bz_unlock_tables(); -my $bug = new Bugzilla::Bug($id, $user->id); +my $bug = new Bugzilla::Bug($id); +# We don't have to check if the user can see the bug, because a user filing +# a bug can always see it. You can't change reporter_accessible until +# after the bug is filed. # Add an attachment if requested. if (defined($cgi->upload('data')) || $cgi->param('attachurl')) { diff --git a/process_bug.cgi b/process_bug.cgi index 5669c017f..b26352d22 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -141,7 +141,7 @@ scalar(@idlist) || ThrowUserError("no_bugs_chosen"); # Build a bug object using $cgi->param('id') as ID. # If there are more than one bug changed at once, the bug object will be # empty, which doesn't matter. -my $bug = new Bugzilla::Bug(scalar $cgi->param('id'), $whoid); +my $bug = new Bugzilla::Bug(scalar $cgi->param('id')); # Make sure form param 'dontchange' is defined so it can be compared to easily. $cgi->param('dontchange','') unless defined $cgi->param('dontchange'); @@ -190,7 +190,7 @@ foreach my $field ("dependson", "blocked") { # throw an error if any of the changed bugs are not visible. ValidateBugID($id); if (Bugzilla->params->{"strict_isolation"}) { - my $deltabug = new Bugzilla::Bug($id, $user->id); + my $deltabug = new Bugzilla::Bug($id); if (!$user->can_edit_product($deltabug->{'product_id'})) { $vars->{'field'} = $field; ThrowUserError("illegal_change_deps", $vars); @@ -527,7 +527,7 @@ if ($action eq Bugzilla->params->{'move-button-text'}) { my @bugs; # First update all moved bugs. foreach my $id (@idlist) { - my $bug = new Bugzilla::Bug($id, $whoid); + my $bug = new Bugzilla::Bug($id); push(@bugs, $bug); $sth->execute($timestamp, $id); @@ -1346,7 +1346,7 @@ if ($prod_changed && Bugzilla->params->{"strict_isolation"}) { foreach my $id (@idlist) { my $query = $basequery; my @bug_values = @values; - my $old_bug_obj = new Bugzilla::Bug($id, $whoid); + my $old_bug_obj = new Bugzilla::Bug($id); if ($cgi->param('knob') eq 'reassignbycomponent') { # We have to check whether the bug is moved to another product @@ -1897,7 +1897,7 @@ foreach my $id (@idlist) { # and then generate any necessary bug activity entries by seeing # what has changed since before we wrote out the new values. # - my $new_bug_obj = new Bugzilla::Bug($id, $whoid); + my $new_bug_obj = new Bugzilla::Bug($id); my @newvalues = SnapShotBug($id); my %newhash; $i = 0; @@ -2096,7 +2096,7 @@ if ($action eq 'next_bug') { } if ($next_bug) { if (detaint_natural($next_bug) && Bugzilla->user->can_see_bug($next_bug)) { - my $bug = new Bugzilla::Bug($next_bug, $whoid); + my $bug = new Bugzilla::Bug($next_bug); ThrowCodeError("bug_error", { bug => $bug }) if $bug->error; $vars->{'bugs'} = [$bug]; @@ -2110,7 +2110,7 @@ if ($action eq 'next_bug') { } } elsif ($action eq 'same_bug') { if (Bugzilla->user->can_see_bug($cgi->param('id'))) { - my $bug = new Bugzilla::Bug($cgi->param('id'), $whoid); + my $bug = new Bugzilla::Bug($cgi->param('id')); ThrowCodeError("bug_error", { bug => $bug }) if $bug->error; $vars->{'bugs'} = [$bug]; diff --git a/show_bug.cgi b/show_bug.cgi index a23621d78..fda4d1503 100755 --- a/show_bug.cgi +++ b/show_bug.cgi @@ -35,7 +35,7 @@ my $cgi = Bugzilla->cgi; my $template = Bugzilla->template; my $vars = {}; -Bugzilla->login(); +my $user = Bugzilla->login(); # Editable, 'single' HTML bugs are treated slightly specially in a few places my $single = !$cgi->param('format') @@ -60,7 +60,7 @@ if ($single) { # 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, Bugzilla->user->id); + push @bugs, new Bugzilla::Bug($id); if (defined $cgi->param('mark')) { foreach my $range (split ',', $cgi->param('mark')) { if ($range =~ /^(\d+)-(\d+)$/) { @@ -77,7 +77,13 @@ if ($single) { # Be kind enough and accept URLs of the form: id=1,2,3. my @ids = split(/,/, $id); foreach (@ids) { - my $bug = new Bugzilla::Bug($_, Bugzilla->user->id); + my $bug = new Bugzilla::Bug($_); + # This is basically a backwards-compatibility hack from when + # Bugzilla::Bug->new used to set 'NotPermitted' if you couldn't + # see the bug. + if (!$bug->{error} && !$user->can_see_bug($bug->bug_id)) { + $bug->{error} = 'NotPermitted'; + } push(@bugs, $bug); } } diff --git a/showdependencytree.cgi b/showdependencytree.cgi index 1886126b5..5efa0bc9c 100755 --- a/showdependencytree.cgi +++ b/showdependencytree.cgi @@ -51,7 +51,7 @@ my $dbh = Bugzilla->switch_to_shadow_db(); # bug that the user is authorized to access. my $id = $cgi->param('id'); ValidateBugID($id); -my $current_bug = new Bugzilla::Bug($id, $user->id); +my $current_bug = new Bugzilla::Bug($id); my $hide_resolved = $cgi->param('hide_resolved') ? 1 : 0; @@ -120,7 +120,7 @@ sub GenerateTree { # its sub-tree if we haven't already done so (which happens # when bugs appear in dependency trees multiple times). if (!$bugs->{$dep_id}) { - $bugs->{$dep_id} = new Bugzilla::Bug($dep_id, $user->id); + $bugs->{$dep_id} = new Bugzilla::Bug($dep_id); GenerateTree($dep_id, $relationship, $depth+1, $bugs, $ids); } @@ -129,6 +129,7 @@ sub GenerateTree { # wants the tree to go, and if the dependency isn't resolved # (if we're ignoring resolved dependencies). if (!$bugs->{$dep_id}->{'error'} + && Bugzilla->user->can_see_bug($dep_id) && (!$maxdepth || $depth <= $maxdepth) && ($bugs->{$dep_id}->{'isopened'} || !$hide_resolved)) { diff --git a/template/en/default/bug/show-multiple.html.tmpl b/template/en/default/bug/show-multiple.html.tmpl index c76e07699..35df0abb1 100644 --- a/template/en/default/bug/show-multiple.html.tmpl +++ b/template/en/default/bug/show-multiple.html.tmpl @@ -54,7 +54,7 @@

[% terms.Bug %] [% bug.bug_id %] - [% IF Param("usebugaliases") AND bug.alias %] + [% IF Param("usebugaliases") AND bug.alias AND NOT bug.error %] ([% bug.alias FILTER html %]) [% END %]

diff --git a/votes.cgi b/votes.cgi index 016e6ad67..4ff85a410 100755 --- a/votes.cgi +++ b/votes.cgi @@ -266,7 +266,7 @@ sub record_votes { my %products; # XXX - We really need a $bug->product() method. foreach my $bug_id (@buglist) { - my $bug = new Bugzilla::Bug($bug_id, $who); + my $bug = new Bugzilla::Bug($bug_id); my $prod = $bug->{'product'}; $products{$prod} ||= new Bugzilla::Product({name => $prod}); $prodcount{$prod} ||= 0; -- cgit v1.2.3-24-g4f1b