Fix for bug 179510: takes group restrictions into account when sending request notifications

Attachment.pm

@@ -48,10 +48,12 @@ sub new {
     bless($self, $class);
     
     &::PushGlobalSQLState();
-    &::SendSQL("SELECT 1, description, bug_id FROM attachments " . 
+    &::SendSQL("SELECT 1, description, bug_id, isprivate FROM attachments " . 
                "WHERE attach_id = $id");
-    ($self->{'exists'}, $self->{'summary'}, $self->{'bug_id'}) = 
-      &::FetchSQLData();
+    ($self->{'exists'},
+     $self->{'summary'},
+     $self->{'bug_id'},
+     $self->{'isprivate'}) = &::FetchSQLData();
     &::PopGlobalSQLState();
 
     return $self;

Bugzilla/Attachment.pm

@@ -48,10 +48,12 @@ sub new {
     bless($self, $class);
     
     &::PushGlobalSQLState();
-    &::SendSQL("SELECT 1, description, bug_id FROM attachments " . 
+    &::SendSQL("SELECT 1, description, bug_id, isprivate FROM attachments " . 
                "WHERE attach_id = $id");
-    ($self->{'exists'}, $self->{'summary'}, $self->{'bug_id'}) = 
-      &::FetchSQLData();
+    ($self->{'exists'},
+     $self->{'summary'},
+     $self->{'bug_id'},
+     $self->{'isprivate'}) = &::FetchSQLData();
     &::PopGlobalSQLState();
 
     return $self;

Bugzilla/Flag.pm

@@ -33,9 +33,12 @@ use Bugzilla::FlagType;
 use Bugzilla::User;
 use Bugzilla::Config;
 use Bugzilla::Util;
+use Bugzilla::Error;
 
 use Attachment;
 
+use constant TABLES_ALREADY_LOCKED => 1;
+
 # Note that this line doesn't actually import these variables for some reason,
 # so I have to use them as $::template and $::vars in the package code.
 use vars qw($template $vars); 
@@ -135,8 +138,8 @@ sub count {
 
 sub validate {
     # Validates fields containing flag modifications.
-    
-    my ($data) = @_;
+
+    my ($data, $bug_id) = @_;
   
     # Get a list of flags to validate.  Uses the "map" function
     # to extract flag IDs from form field names by matching fields
@@ -152,13 +155,62 @@ sub validate {
         my $flag = get($id);
         $flag || &::ThrowCodeError("flag_nonexistent", { id => $id });
 
-        # Don't bother validating flags the user didn't change.
-        next if $status eq $flag->{'status'};
-
         # Make sure the user chose a valid status.
         grep($status eq $_, qw(X + - ?))
           || &::ThrowCodeError("flag_status_invalid", 
                                { id => $id , status => $status });
+                
+        # Make sure the user didn't request the flag unless it's requestable.
+        if ($status eq '?' && !$flag->{type}->{is_requestable}) {
+            ThrowCodeError("flag_status_invalid", 
+                              { id => $id , status => $status });
+        }
+        
+        # Make sure the requestee is authorized to access the bug.
+        # (and attachment, if this installation is using the "insider group"
+        # feature and the attachment is marked private).
+        if ($status eq '?'
+            && $flag->{type}->{is_requesteeble}
+            && trim($data->{"requestee-$id"}))
+        {
+            my $requestee_email = trim($data->{"requestee-$id"});
+            if ($requestee_email ne $flag->{'requestee'}->{'email'}) {
+                # We know the requestee exists because we ran
+                # Bugzilla::User::match_field before getting here.
+                # ConfirmGroup makes sure their group settings
+                # are up-to-date or calls DeriveGroups to update them.
+                my $requestee_id = &::DBname_to_id($requestee_email);
+                &::ConfirmGroup($requestee_id);
+
+                # Throw an error if the user can't see the bug.
+                if (!&::CanSeeBug($bug_id, $requestee_id))
+                {
+                    ThrowUserError("flag_requestee_unauthorized",
+                                   { flag_type => $flag->{'type'},
+                                     requestee =>
+                                       new Bugzilla::User($requestee_id),
+                                     bug_id => $bug_id,
+                                     attach_id =>
+                                       $flag->{target}->{attachment}->{id} });
+                }
+
+                # Throw an error if the target is a private attachment and
+                # the requestee isn't in the group of insiders who can see it.
+                if ($flag->{target}->{attachment}->{exists}
+                    && $data->{'isprivate'}
+                    && &::Param("insidergroup")
+                    && !&::UserInGroup(&::Param("insidergroup"), $requestee_id))
+                {
+                    ThrowUserError("flag_requestee_unauthorized_attachment",
+                                   { flag_type => $flag->{'type'},
+                                     requestee =>
+                                       new Bugzilla::User($requestee_id),
+                                     bug_id    => $bug_id,
+                                     attach_id =>
+                                      $flag->{target}->{attachment}->{id} });
+                }
+            }
+        }
     }
 }
 
@@ -457,14 +509,17 @@ sub GetBug {
     # Save the currently running query (if any) so we do not overwrite it.
     &::PushGlobalSQLState();
 
-    &::SendSQL("SELECT  1, short_desc, product_id, component_id
-                  FROM  bugs
-                 WHERE  bug_id = $id");
+    &::SendSQL("SELECT    1, short_desc, product_id, component_id,
+                          COUNT(bug_group_map.group_id)
+                FROM      bugs LEFT JOIN bug_group_map
+                            ON (bugs.bug_id = bug_group_map.bug_id)
+                WHERE     bugs.bug_id = $id
+                GROUP BY  bugs.bug_id");
 
     my $bug = { 'id' => $id };
     
     ($bug->{'exists'}, $bug->{'summary'}, $bug->{'product_id'}, 
-     $bug->{'component_id'}) = &::FetchSQLData();
+     $bug->{'component_id'}, $bug->{'restricted'}) = &::FetchSQLData();
 
     # Restore the previously running query (if any).
     &::PopGlobalSQLState();
@@ -504,6 +559,28 @@ sub notify {
     
     my ($flag, $template_file) = @_;
     
+    # If the target bug is restricted to one or more groups, then we need
+    # to make sure we don't send email about it to unauthorized users
+    # on the request type's CC: list, so we have to trawl the list for users
+    # not in those groups or email addresses that don't have an account.
+    if ($flag->{'target'}->{'bug'}->{'restricted'}
+        || $flag->{'target'}->{'attachment'}->{'isprivate'})
+    {
+        my @new_cc_list;
+        foreach my $cc (split(/[, ]+/, $flag->{'type'}->{'cc_list'})) {
+            my $user_id = &::DBname_to_id($cc) || next;
+            # re-derive permissions if necessary
+            &::ConfirmGroup($user_id, TABLES_ALREADY_LOCKED);
+            next if $flag->{'target'}->{'bug'}->{'restricted'}
+              && !&::CanSeeBug($flag->{'target'}->{'bug'}->{'id'}, $user_id);
+            next if $flag->{'target'}->{'attachment'}->{'isprivate'}
+              && Param("insidergroup")
+              && !&::UserInGroup(Param("insidergroup"), $user_id);
+            push(@new_cc_list, $cc);
+        }
+        $flag->{'type'}->{'cc_list'} = join(", ", @new_cc_list);
+    }
+
     $::vars->{'flag'} = $flag;
     
     my $message;

Bugzilla/FlagType.pm

@@ -32,6 +32,9 @@ package Bugzilla::FlagType;
 # Use Bugzilla's User module which contains utilities for handling users.
 use Bugzilla::User;
 
+use Bugzilla::Error;
+use Bugzilla::Util;
+
 # Note!  This module requires that its caller have said "require CGI.pl" 
 # to import relevant functions from that script and its companion globals.pl.
 
@@ -177,9 +180,9 @@ sub count {
 }
 
 sub validate {
-    my ($data) = @_;
+    my ($data, $bug_id, $attach_id) = @_;
   
-    # Get a list of flags types to validate.  Uses the "map" function
+    # Get a list of flag types to validate.  Uses the "map" function
     # to extract flag type IDs from form field names by matching columns
     # whose name looks like "flag_type-nnn", where "nnn" is the ID,
     # and returning just the ID portion of matching field names.
@@ -192,14 +195,62 @@ sub validate {
         # Don't bother validating types the user didn't touch.
         next if $status eq "X";
 
-        # Make sure the flag exists.
-        get($id) 
+        # Make sure the flag type exists.
+        my $flag_type = get($id);
+        $flag_type 
           || &::ThrowCodeError("flag_type_nonexistent", { id => $id });
 
         # Make sure the value of the field is a valid status.
         grep($status eq $_, qw(X + - ?))
           || &::ThrowCodeError("flag_status_invalid", 
                                { id => $id , status => $status });
+                
+        # Make sure the user didn't request the flag unless it's requestable.
+        if ($status eq '?' && !$flag_type->{is_requestable}) {
+            ThrowCodeError("flag_status_invalid", 
+                           { id => $id , status => $status });
+        }
+        
+        # Make sure the requestee is authorized to access the bug
+        # (and attachment, if this installation is using the "insider group"
+        # feature and the attachment is marked private).
+        if ($status eq '?'
+            && $flag_type->{is_requesteeble}
+            && trim($data->{"requestee_type-$id"}))
+        {
+            my $requestee_email = trim($data->{"requestee_type-$id"});
+            my $requestee_id = &::DBname_to_id($requestee_email);
+
+            # We know the requestee exists because we ran
+            # Bugzilla::User::match_field before getting here.
+            # ConfirmGroup makes sure their group settings
+            # are up-to-date or calls DeriveGroups to update them.
+            &::ConfirmGroup($requestee_id);
+
+            # Throw an error if the user can't see the bug.
+            if (!&::CanSeeBug($bug_id, $requestee_id))
+            {
+                ThrowUserError("flag_requestee_unauthorized",
+                               { flag_type => $flag_type,
+                                 requestee => new Bugzilla::User($requestee_id),
+                                 bug_id    => $bug_id,
+                                 attach_id => $attach_id });
+            }
+            
+            # Throw an error if the target is a private attachment and
+            # the requestee isn't in the group of insiders who can see it.
+            if ($attach_id
+                && &::Param("insidergroup")
+                && $data->{'isprivate'}
+                && !&::UserInGroup(&::Param("insidergroup"), $requestee_id))
+            {
+                ThrowUserError("flag_requestee_unauthorized_attachment",
+                               { flag_type => $flag_type,
+                                 requestee => new Bugzilla::User($requestee_id),
+                                 bug_id    => $bug_id,
+                                 attach_id => $attach_id });
+            }
+        }
     }
 }
 

Bugzilla/Util.pm

@@ -129,8 +129,10 @@ sub min {
 
 sub trim {
     my ($str) = @_;
-    $str =~ s/^\s+//g;
-    $str =~ s/\s+$//g;
+    if ($str) {
+      $str =~ s/^\s+//g;
+      $str =~ s/\s+$//g;
+    }
     return $str;
 }
 

attachment.cgi

@@ -52,6 +52,17 @@ ConnectToDatabase();
 # Check whether or not the user is logged in and, if so, set the $::userid 
 quietly_check_login();
 
+# The ID of the bug to which the attachment is attached.  Gets set
+# by validateID() (which validates the attachment ID, not the bug ID, but has
+# to check if the user is authorized to access this attachment) and is used 
+# by Flag:: and FlagType::validate() to ensure the requestee (if any) for a 
+# requested flag is authorized to see the bug in question.  Note: This should 
+# really be defined just above validateID() itself, but it's used in the main 
+# body of the script before that function is defined, so we define it up here 
+# instead.  We should move the validation into each function and then move this
+# to just above validateID().
+my $bugid;
+
 ################################################################################
 # Main Body Execution
 ################################################################################
@@ -113,10 +124,15 @@ elsif ($action eq "update")
   validateContentType() unless $::FORM{'ispatch'};
   validateIsObsolete();
   validatePrivate();
+  
+  # The order of these function calls is important, as both Flag::validate
+  # and FlagType::validate assume User::match_field has ensured that the values
+  # in the requestee fields are legitimate user email addresses.
   Bugzilla::User::match_field({ '^requestee(_type)?-(\d+)$' => 
                                     { 'type' => 'single' } });
-  Bugzilla::Flag::validate(\%::FORM);
-  Bugzilla::FlagType::validate(\%::FORM);
+  Bugzilla::Flag::validate(\%::FORM, $bugid);
+  Bugzilla::FlagType::validate(\%::FORM, $bugid, $::FORM{'id'});
+  
   update();
 }
 else 
@@ -146,7 +162,7 @@ sub validateID
       || ThrowUserError("invalid_attach_id");
 
     # Make sure the user is authorized to access this attachment's bug.
-    my ($bugid, $isprivate) = FetchSQLData();
+    ($bugid, my $isprivate) = FetchSQLData();
     ValidateBugID($bugid);
     if (($isprivate > 0 ) && Param("insidergroup") && !(UserInGroup(Param("insidergroup")))) {
         ThrowUserError("attachment_access_denied");
@@ -677,7 +693,16 @@ sub update
   SendSQL("LOCK TABLES attachments WRITE , flags WRITE , " . 
           "flagtypes READ , fielddefs READ , bugs_activity WRITE, " . 
           "flaginclusions AS i READ, flagexclusions AS e READ, " . 
-          "bugs READ, profiles READ");
+          # cc, bug_group_map, user_group_map, and groups are in here so we
+          # can check the permissions of flag requestees and email addresses
+          # on the flag type cc: lists via the ConfirmGroup and CanSeeBug
+          # function calls in Flag::notify.  group_group_map is in here in case
+          # ConfirmGroup needs to call DeriveGroup.  profiles and user_group_map
+          # would be READ locks instead of WRITE locks if it weren't for
+          # DeriveGroup, which needs to write to those tables.
+          "bugs READ, profiles WRITE, " . 
+          "cc READ, bug_group_map READ, user_group_map WRITE, " . 
+          "group_group_map READ, groups READ");
   
   # Get a copy of the attachment record before we make changes
   # so we can record those changes in the activity table.

globals.pl

@@ -549,9 +549,15 @@ sub CanEnterProduct {
 
 #
 # This function returns an alphabetical list of product names to which
-# the user can enter bugs.
+# the user can enter bugs.  If the $by_id parameter is true, also retrieves IDs
+# and pushes them onto the list as id, name [, id, name...] for easy slurping
+# into a hash by the calling code.
 sub GetSelectableProducts {
-    my $query = "SELECT name " .
+    my ($by_id) = @_;
+
+    my $extra_sql = $by_id ? "id, " : "";
+
+    my $query = "SELECT $extra_sql name " .
                 "FROM products " .
                 "LEFT JOIN group_control_map " .
                 "ON group_control_map.product_id = products.id ";
@@ -570,9 +576,7 @@ sub GetSelectableProducts {
     PushGlobalSQLState();
     SendSQL($query);
     my @products = ();
-    while (MoreSQLData()) {
-        push @products,FetchOneColumn();
-    }
+    push(@products, FetchSQLData()) while MoreSQLData();
     PopGlobalSQLState();
     return (@products);
 }
@@ -580,50 +584,50 @@ sub GetSelectableProducts {
 # GetSelectableProductHash
 # returns a hash containing 
 # legal_products => an enterable product list
-# legal_components => the list of components of enterable products
-# components => a hash of component lists for each enterable product
+# legal_(components|versions|milestones) =>
+#   the list of components, versions, and milestones of enterable products
+# (components|versions|milestones)_by_product
+#    => a hash of component lists for each enterable product
+# Milestones only get returned if the usetargetmilestones parameter is set.
 sub GetSelectableProductHash {
-    my $query = "SELECT products.name, components.name " .
-                "FROM products " .
-                "LEFT JOIN components " .
-                "ON components.product_id = products.id " .
-                "LEFT JOIN group_control_map " .
-                "ON group_control_map.product_id = products.id ";
-    if (Param('useentrygroupdefault')) {
-        $query .= "AND group_control_map.entry != 0 ";
-    } else {
-        $query .= "AND group_control_map.membercontrol = " .
-                  CONTROLMAPMANDATORY . " ";
-    }
-    if ((defined @{$::vars->{user}{groupids}}) 
-        && (@{$::vars->{user}{groupids}} > 0)) {
-        $query .= "AND group_id NOT IN(" . 
-                   join(',', @{$::vars->{user}{groupids}}) . ") ";
-    }
-    $query .= "WHERE group_id IS NULL " .
-              "ORDER BY products.name, components.name";
+    # The hash of selectable products and their attributes that gets returned
+    # at the end of this function.
+    my $selectables = {};
+
+    my %products = GetSelectableProducts(1);
+
+    $selectables->{legal_products} = [sort values %products];
+
+    # Run queries that retrieve the list of components, versions,
+    # and target milestones (if used) for the selectable products.
+    my @tables = qw(components versions);
+    push(@tables, 'milestones') if Param('usetargetmilestone');
+
     PushGlobalSQLState();
-    SendSQL($query);
-    my @products = ();
-    my %components = ();
-    my %components_by_product = ();
-    while (MoreSQLData()) {
-        my ($product, $component) = FetchSQLData();
-        if (!grep($_ eq $product, @products)) {
-            push @products, $product;
-        }
-        if ($component) {
-            $components{$component} = 1;
-            push @{$components_by_product{$product}}, $component;
+    foreach my $table (@tables) {
+        # Why oh why can't we standardize on these names?!?
+        my $fld = ($table eq "components" ? "name" : "value");
+
+        my $query = "SELECT $fld, product_id FROM $table WHERE product_id IN " .
+                    "(" . join(",", keys %products) . ") ORDER BY $fld";
+        SendSQL($query);
+
+        my %values;
+        my %values_by_product;
+
+        while (MoreSQLData()) {
+            my ($name, $product_id) = FetchSQLData();
+            next unless $name;
+            $values{$name} = 1;
+            push @{$values_by_product{$products{$product_id}}}, $name;
         }
+
+        $selectables->{"legal_$table"} = [sort keys %values];
+        $selectables->{"${table}_by_product"} = \%values_by_product;
     }
     PopGlobalSQLState();
-    my @componentlist = (sort keys %components);
-    return {
-        legal_products => \@products,
-        legal_components => \@componentlist,
-        components => \%components_by_product,
-    };
+
+    return $selectables;
 }
 
 
@@ -724,24 +728,28 @@ sub Crypt {
 # Permissions must be rederived if ANY groups have a last_changed newer
 # than the profiles.refreshed_when value.
 sub ConfirmGroup {
-    my ($user) = (@_);
+    my ($user, $locked) = (@_);
     PushGlobalSQLState();
     SendSQL("SELECT userid FROM profiles, groups WHERE userid = $user " .
             "AND profiles.refreshed_when <= groups.last_changed ");
     my $ret = FetchSQLData();
     PopGlobalSQLState();
     if ($ret) {
-        DeriveGroup($user);
+        DeriveGroup($user, $locked);
     }
 }
 
 # DeriveGroup removes and rederives all derived group permissions for
-# the specified user.
+# the specified user.  If $locked is true, Bugzilla has already locked
+# the necessary tables as part of a larger transaction, so this function
+# shouldn't lock them again (since then tables not part of this function's
+# lock will get unlocked).
 sub DeriveGroup {
-    my ($user) = (@_);
+    my ($user, $locked) = (@_);
     PushGlobalSQLState();
 
-    SendSQL("LOCK TABLES profiles WRITE, user_group_map WRITE, group_group_map READ, groups READ");
+    SendSQL("LOCK TABLES profiles WRITE, user_group_map WRITE, group_group_map READ, groups READ")
+      unless $locked;
 
     # avoid races,  we are only as up to date as the BEGINNING of this process
     SendSQL("SELECT login_name, NOW() FROM profiles WHERE userid = $user");

process_bug.cgi

@@ -91,12 +91,21 @@ scalar(@idlist) || ThrowUserError("no_bugs_chosen");
 
 # do a match on the fields if applicable
 
+# The order of these function calls is important, as both Flag::validate
+# and FlagType::validate assume User::match_field has ensured that the values
+# in the requestee fields are legitimate user email addresses.
 &Bugzilla::User::match_field({
     'qa_contact'                => { 'type' => 'single' },
     'newcc'                     => { 'type' => 'multi'  },
     'assigned_to'               => { 'type' => 'single' },
     '^requestee(_type)?-(\d+)$' => { 'type' => 'single' },
 });
+# Validate flags, but only if the user is changing a single bug,
+# since the multi-change form doesn't include flag changes.
+if (defined $::FORM{'id'}) {
+    Bugzilla::Flag::validate(\%::FORM, $::FORM{'id'});
+    Bugzilla::FlagType::validate(\%::FORM, $::FORM{'id'});
+}
 
 # If we are duping bugs, let's also make sure that we can change 
 # the original.  This takes care of issue A on bug 96085.
@@ -1080,7 +1089,11 @@ foreach my $id (@idlist) {
             "products READ, components READ, " .
             "keywords $write, longdescs $write, fielddefs $write, " .
             "bug_group_map $write, flags $write, duplicates $write," .
-            "user_group_map READ, flagtypes READ, " . 
+            # user_group_map would be a READ lock except that Flag::process
+            # may call Flag::notify, which calls ConfirmGroup, which might
+            # call DeriveGroup, which wants a WRITE lock on that table.
+            # group_group_map is in here at all because DeriveGroups needs it.
+            "user_group_map $write, group_group_map READ, flagtypes READ, " . 
             "flaginclusions AS i READ, flagexclusions AS e READ, " .
             "keyworddefs READ, groups READ, attachments READ, " .
             "group_control_map AS oldcontrolmap READ, " .

template/en/default/global/user-error.html.tmpl

@@ -180,6 +180,27 @@
     format like JPG or PNG, or put it elsewhere on the web and
     link to it from the bug's URL field or in a comment on the bug.
       
+  [% ELSIF error == "flag_requestee_unauthorized" %]
+    [% title = "Flag Requestee Not Authorized" %]
+
+    You asked [% requestee.identity FILTER html %]
+    for <code>[% flag_type.name FILTER html %]</code> on bug [% bug_id -%] 
+    [% IF attach_id %], attachment [% attach_id %][% END %], but that bug
+    has been restricted to users in certain groups, and the user you asked
+    isn't in all the groups to which the bug has been restricted.
+    Please choose someone else to ask, or make the bug accessible to users
+    on its CC: list and add that user to the list.
+    
+  [% ELSIF error == "flag_requestee_unauthorized_attachment" %]
+    [% title = "Flag Requestee Not Authorized" %]
+
+    You asked [% requestee.identity FILTER html %]
+    for <code>[% flag_type.name FILTER html %]</code> on bug [% bug_id %],
+    attachment [% attach_id %], but that attachment is restricted to users 
+    in the [% Param("insidergroup") FILTER html %] group, and the user
+    you asked isn't in that group.  Please choose someone else to ask,
+    or ask an administrator to add the user to the group.
+    
   [% ELSIF error == "flag_type_cc_list_invalid" %]
     [% title = "Flag Type CC List Invalid" %]
     The CC list [% cc_list FILTER html %] must be less than 200 characters long.