Bug 348057: Move the checks for bug visibility out of Bugzilla::Bug->new

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=myk

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index afc1254579331376b8211f09e43c048a07f9e59a..03be7156c4e6daf46316d59c98ba3045fe832d42 100755 (executable)
--- 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 8541e8d5ee775e8793ae09856ef7d30eef007c42..6545f149e3d486fc693c1694fdc6bda72bae1109 100755 (executable)
--- 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 e66e95195fcb17c02a80eda67b2a32e28f89d1b1..c87bc0313cd9a8aec0aadd5b12c1622e1648f3d3 100755 (executable)
--- 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 e7b0908b538bca91dc7ff68d5091edeff6155a82..1164177f513d4cbc2e8e842c658bf58a1fd97368 100755 (executable)
--- 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 432e2a9c634d234189207f027ba5c3531248f713..4c4394926b990f1b14073ffc8f1c4c601f6fb1ac 100755 (executable)
--- 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 a5a4cedcadac936eaafb1b1fb76afe7d03d977f1..269f143176d212511f377cfc05c34d46baf147f6 100755 (executable)
--- 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 e048aac406abb5bd406c3958471326266f86d624..68b2cd34f3fc70f4480e997a6498e07ee0906da3 100755 (executable)
--- 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 be061b2f14fb6c2d086121c037c7f2c59dbb73c9..aac15f3bfc3cb1b5cb741ed6336089d31801a578 100755 (executable)
--- 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 5669c017fc853fa722bcd95f5e1c3821e609f470..b26352d227664cb4d00e8c23033c16efad6efb8c 100755 (executable)
--- 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 a23621d78fd3d1d2710902aa0c11d775dd5b0a62..fda4d1503cecfda374a0714632f1cc3481874d53 100755 (executable)
--- 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 1886126b5941444436361a165874d99c93b6eb2a..5efa0bc9cc07a36fe1f4d1758415a4c2c6ed94fc 100755 (executable)
--- 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 c76e07699ac1d4737038f122440989153d3956f9..35df0abb1b12b67f5f2aed4f1192844abb515e35 100644 (file)
--- a/template/en/default/bug/show-multiple.html.tmpl
+++ b/template/en/default/bug/show-multiple.html.tmpl
@@ -54,7 +54,7 @@
   <h1>
     [% terms.Bug %] 
     <a href="show_bug.cgi?id=[% bug.bug_id %]">[% bug.bug_id %]</a>
-    [% IF Param("usebugaliases") AND bug.alias %]
+    [% IF Param("usebugaliases") AND bug.alias AND NOT bug.error %]
       ([% bug.alias FILTER html %])
     [% END %]
   </h1>

diff --git a/votes.cgi b/votes.cgi
index 016e6ad67684328bea0abb487f03bb7655be2c24..4ff85a41053e01ca575d3bcf7059cb5b6feeeb45 100755 (executable)
--- 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;