diff options
author | myk%mozilla.org <> | 2003-04-25 14:41:20 +0200 |
---|---|---|
committer | myk%mozilla.org <> | 2003-04-25 14:41:20 +0200 |
commit | 47c010537c77f8e7e09e6c19246cdbecbb7b5a26 (patch) | |
tree | 515f996ddc173bcae29f0ede8f77de48d59bc6f4 | |
parent | adc665e91aa228734632e51cb42d671bbbab9f7f (diff) | |
download | bugzilla-47c010537c77f8e7e09e6c19246cdbecbb7b5a26.tar.gz bugzilla-47c010537c77f8e7e09e6c19246cdbecbb7b5a26.tar.xz |
Fix for bug 179510: takes group restrictions into account when sending request notifications
r=bbaetz,jpreed
a=justdave
-rw-r--r-- | Attachment.pm | 8 | ||||
-rw-r--r-- | Bugzilla/Attachment.pm | 8 | ||||
-rw-r--r-- | Bugzilla/Flag.pm | 95 | ||||
-rw-r--r-- | Bugzilla/FlagType.pm | 59 | ||||
-rw-r--r-- | Bugzilla/Util.pm | 6 | ||||
-rwxr-xr-x | attachment.cgi | 33 | ||||
-rw-r--r-- | globals.pl | 106 | ||||
-rwxr-xr-x | process_bug.cgi | 15 | ||||
-rw-r--r-- | template/en/default/global/user-error.html.tmpl | 21 |
9 files changed, 276 insertions, 75 deletions
diff --git a/Attachment.pm b/Attachment.pm index 322a3b2ba..ea5cd531c 100644 --- a/Attachment.pm +++ b/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; diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index 322a3b2ba..ea5cd531c 100644 --- a/Bugzilla/Attachment.pm +++ b/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; diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm index 41cc18071..a327f2922 100644 --- a/Bugzilla/Flag.pm +++ b/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; diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm index 2e272f67c..523f60190 100644 --- a/Bugzilla/FlagType.pm +++ b/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 }); + } + } } } diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index 5aecb5ad9..511ba2592 100644 --- a/Bugzilla/Util.pm +++ b/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; } diff --git a/attachment.cgi b/attachment.cgi index 51a154645..621477ed5 100755 --- a/attachment.cgi +++ b/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. diff --git a/globals.pl b/globals.pl index 911f99278..4caaa3f84 100644 --- a/globals.pl +++ b/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"); diff --git a/process_bug.cgi b/process_bug.cgi index 09b45cf92..83d601d33 100755 --- a/process_bug.cgi +++ b/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, " . diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 934c0511f..1aaa581b6 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/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. |