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

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Right now Bugzilla::Bug->new calls $user->can_see_bug inside of its constructor. Instead, this should happen in the calling code.

Comment 1

[Frédéric Buclin]

Maybe not in the calling code, because it will miss some checks. We should have a my $bug_object = check_bug($bug_id) where check_bug will make sure you can access the bug and return the bug object on success, as we do in Product.pm (new() vs check_product()). This way, the caller simply has to call check_bug() and we have one central place to do the checks.

Comment 2

[Max Kanat-Alexander]

Attachment: v1

Okay, I checked all of these very carefully, and it turns out that checking whether or not the user could see the bug inside of new() was mostly redundant with calls to ValidateBugID.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #1)
> We should have a my $bug_object = check_bug($bug_id) [snip]

  Yeah, I totally agree. I'd have to change all the ValidateBugID calls to that though, and I'd rather do that in a separate bug.

Comment 4

[Frédéric Buclin]

Comment on attachment 232947 [details] [diff] [review]
v1

You missed some places: 3 more places in process_bug.cgi, 1 in showdependencytree.cgi + 1 in votes.cgi (from memory). Doing a |grep "new Bugzilla::Bug"| will tell you if some still use 2 arguments.

Talking about arguments, you should fix Bug::new() to accept one argument:

  if ($#_ == 1) {

Or even remove this check completely (we don't care anymore).

Also, you have to fix Bug::initBug and remove $user_id:

sub initBug  {
  my $self = shift();
  my ($bug_id, $user_id) = (@_);


I didn't check if all validations were still present. I will do that on an updated patch.

Comment 5

[Max Kanat-Alexander]

Attachment: v2

Okay, thanks for catching all that! :-) Here's a new version.

Comment 6

[Frédéric Buclin]

Comment on attachment 233099 [details] [diff] [review]
v2

>Index: show_bug.cgi

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

This is definitely dangerous. If the user cannot see the bug, we should put an empty bug or exclude it completely. I checked all 3 templates (show.html, show.xml and show-multiple.html) and I found one data leakage, in show-multiple.html.tmpl:

At line 57, the bug alias is displayed even if you are not allowed to view the bug. To fix the problem, write:

-    [% IF Param("usebugaliases") AND bug.alias %]
+    [% IF Param("usebugaliases") AND bug.alias AND NOT bug.error %]

I will attach an updated patch with this line fixed. Else your patch works fine, and all other places are correctly validated. So r=LpSolit with this change above.

Comment 7

[Frédéric Buclin]

Attachment: patch, v2.1 (containing fix for bug aliases)

r=me on this one.

Comment 8

[Max Kanat-Alexander]

Okay, I checked in your patch.

The right fix is to fix the AUTOLOAD in Bug.pm, but we can do that in another bug.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.120; previous revision: 1.119
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cginew revision: 1.76; previous revision: 1.75
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.46; previous revision: 1.45
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.129; previous revision: 1.128
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.145; previous revision: 1.144
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.66; previous revision: 1.65
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.160; previous revision: 1.159
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.336; previous revision: 1.335
done
Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.47; previous revision: 1.46
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  showdependencytree.cgi
new revision: 1.47; previous revision: 1.46
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.46; previous revision: 1.45
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.132; previous revision: 1.131
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.31; previous revision: 1.30
done