Bug 114696 - permission checking in queries not optimal

Patch by joel, dkl + me
r=myk, a=justdave

4 files changed, 65 insertions, 27 deletions

diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index a7c329307..cea9294ca 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -63,7 +63,7 @@ sub init {
     my @fields;
     my @supptables;
     my @wherepart;
-    my @having = ("(cntuseringroups = cntbugingroups OR canseeanyway)");
+    my @having;
     @fields = @$fieldsref if $fieldsref;
     my @specialchart;
     my @andlist;
@@ -910,26 +910,38 @@ sub init {
     
     # Make sure we create a legal SQL query.
     @andlist = ("1 = 1") if !@andlist;
-    
-    my $query =  ("SELECT " . join(', ', @fields) .
-                  ", COUNT(DISTINCT ugmap.group_id) AS cntuseringroups, " .
-                  " COUNT(DISTINCT bgmap.group_id) AS cntbugingroups, " .
-                  " ((COUNT(DISTINCT ccmap.who) AND cclist_accessible) " .
-                  "  OR ((bugs.reporter = $::userid) AND bugs.reporter_accessible) " .
-                  "  OR bugs.assigned_to = $::userid ) AS canseeanyway " .
-                  " FROM $suppstring" .
-                  " LEFT JOIN bug_group_map AS bgmap " .
-                  " ON bgmap.bug_id = bugs.bug_id " .
-                  " LEFT JOIN user_group_map AS ugmap " .
-                  " ON bgmap.group_id = ugmap.group_id " .
-                  " AND ugmap.user_id = $::userid " .
-                  " AND ugmap.isbless = 0" .
-                  " LEFT JOIN cc AS ccmap " .
-                  " ON ccmap.who = $::userid AND ccmap.bug_id = bugs.bug_id " .
-                  " WHERE " . join(' AND ', (@wherepart, @andlist)) .
-                  " GROUP BY bugs.bug_id" . 
-                  " HAVING " . join(" AND ", @having));
-    
+   
+    my $query = "SELECT " . join(', ', @fields) .
+                " FROM $suppstring" .
+                " LEFT JOIN bug_group_map " .
+                " ON bug_group_map.bug_id = bugs.bug_id ";
+
+    if (defined @{$::vars->{user}{groupids}} && @{$::vars->{user}{groupids}} > 0) {
+        $query .= " AND bug_group_map.group_id NOT IN (" . join(',', @{$::vars->{user}{groupids}}) . ") ";
+    }
+
+    if ($::vars->{user}{userid}) {
+        $query .= " LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND cc.who = $::userid ";
+    }
+
+    $query .= " WHERE " . join(' AND ', (@wherepart, @andlist)) .
+              " AND ((bug_group_map.group_id IS NULL)";
+
+    if ($::vars->{user}{userid}) {
+        $query .= "    OR (bugs.reporter_accessible = 1 AND bugs.reporter = $::userid) " .
+              "    OR (bugs.cclist_accessible = 1 AND cc.who IS NOT NULL) " .
+              "    OR (bugs.assigned_to = $::userid) ";
+        if (Param('useqacontact')) {
+            $query .= "OR (bugs.qa_contact = $::userid) ";
+        }
+    }
+
+    $query .= ") GROUP BY bugs.bug_id";
+
+    if (@having) {
+        $query .= " HAVING " . join(" AND ", @having);
+    }
+
     if ($debug) {
         print "<p><code>" . value_quote($query) . "</code></p>\n";
         exit;
diff --git a/CGI.pl b/CGI.pl
index 2069d9235..9ff485265 100644
--- a/CGI.pl
+++ b/CGI.pl
@@ -280,6 +280,7 @@ sub GetUserInfo {
     my %user;
     my @queries;
     my %groups;
+    my @groupids;
     
     # No info if not logged in
     return \%user if ($userid == 0);
@@ -304,16 +305,18 @@ sub GetUserInfo {
 
     $user{'canblessany'} = UserCanBlessAnything();
 
-    SendSQL("SELECT name FROM groups, user_group_map " .
+    SendSQL("SELECT DISTINCT id, name FROM groups, user_group_map " .
             "WHERE groups.id = user_group_map.group_id " .
             "AND user_id = $userid " .
             "AND NOT isbless");
     while (MoreSQLData()) {
-        my ($name) = FetchSQLData();    
+        my ($id, $name) = FetchSQLData();    
+        push(@groupids,$id);
         $groups{$name} = 1;
     }
 
     $user{'groups'} = \%groups;
+    $user{'groupids'} = \@groupids;
 
     return \%user;
 }
diff --git a/buglist.cgi b/buglist.cgi
index 1f91bd322..0f33bee7b 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -619,6 +619,7 @@ SendSQL($query);
 my $bugowners = {};
 my $bugproducts = {};
 my $bugstatuses = {};
+my @bugidlist;
 
 my @bugs; # the list of records
 
@@ -628,7 +629,7 @@ while (my @row = FetchSQLData()) {
     # Slurp the row of data into the record.
     # The second from last column in the record is the number of groups
     # to which the bug is restricted.
-    foreach my $column (@selectcolumns, 'dummy', 'groupset', 'dummy' ) {
+    foreach my $column (@selectcolumns) {
         $bug->{$column} = shift @row;
     }
 
@@ -645,8 +646,13 @@ while (my @row = FetchSQLData()) {
     $bugproducts->{$bug->{'product'}} = 1 if $bug->{'product'};
     $bugstatuses->{$bug->{'status'}} = 1 if $bug->{'status'};
 
+    $bug->{isingroups} = 0;
+
     # Add the record to the list.
     push(@bugs, $bug);
+
+    # Add id to list for checking for bug privacy later
+    push(@bugidlist, $bug->{id});
 }
 
 # Switch back from the shadow database to the regular database so PutFooter()
@@ -654,6 +660,23 @@ while (my @row = FetchSQLData()) {
 # in the shadow database.
 SendSQL("USE $::db_name");
 
+# Check for bug privacy and set $bug->{isingroups} = 1 if private 
+# to 1 or more groups
+my %privatebugs;
+if (@bugidlist) {
+    SendSQL("SELECT DISTINCT bugs.bug_id FROM bugs, bug_group_map " .
+            "WHERE bugs.bug_id = bug_group_map.bug_id " .
+            "AND bugs.bug_id IN (" . join(',',@bugidlist) . ")");
+    while (MoreSQLData()) {
+        my ($id) = FetchSQLData();
+        $privatebugs{$id} = 1;
+    }
+    foreach my $bug (@bugs) {
+        if ($privatebugs{$bug->{id}}) {
+            $bug->{isingroups} = 1;
+        }
+    }
+}
 
 ################################################################################
 # Template Variable Definition
@@ -662,7 +685,7 @@ SendSQL("USE $::db_name");
 # Define the variables and functions that will be passed to the UI template.
 
 $vars->{'bugs'} = \@bugs;
-$vars->{'buglist'} = join(',', map($_->{id}, @bugs));
+$vars->{'buglist'} = join(',', @bugidlist);
 $vars->{'columns'} = $columns;
 $vars->{'displaycolumns'} = \@displaycolumns;
 
@@ -767,7 +790,7 @@ if ($format->{'extension'} eq "html") {
         my $qorder = url_quote($order);
         print "Set-Cookie: LASTORDER=$qorder ; path=$cookiepath; expires=Sun, 30-Jun-2029 00:00:00 GMT\n";
     }
-    my $bugids = join(":", map( $_->{'id'}, @bugs));
+    my $bugids = join(":", @bugidlist);
     # See also Bug 111999
     if (length($bugids) < 4000) {
         print "Set-Cookie: BUGLIST=$bugids ; path=$cookiepath; expires=Sun, 30-Jun-2029 00:00:00 GMT\n";
diff --git a/template/en/default/list/table.html.tmpl b/template/en/default/list/table.html.tmpl
index 8c79b5d40..1de7f4efa 100644
--- a/template/en/default/list/table.html.tmpl
+++ b/template/en/default/list/table.html.tmpl
@@ -128,7 +128,7 @@
     [% tableheader %]
   [% END %]
 
-  <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF (bug.groupset && !usebuggroups) %]">
+  <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF bug.isingroups %]">
 
     <td>
       [% IF dotweak %]<input type="checkbox" name="id_[% bug.id %]">[% END %]