diff options
| -rw-r--r-- | Bugzilla/Bug.pm | 94 | ||||
| -rw-r--r-- | Bugzilla/Group.pm | 56 | ||||
| -rw-r--r-- | Bugzilla/Product.pm | 10 | ||||
| -rw-r--r-- | Bugzilla/WebService/Constants.pm | 3 | ||||
| -rwxr-xr-x | process_bug.cgi | 10 | ||||
| -rw-r--r-- | template/en/default/global/user-error.html.tmpl | 28 |
6 files changed, 133 insertions, 68 deletions
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index bf4529f6f..1f0034ff7 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -1622,6 +1622,8 @@ sub _check_dup_id { sub _check_groups { my ($invocant, $group_names, undef, $params) = @_; + + my $bug_id = blessed($invocant) ? $invocant->id : undef; my $product = blessed($invocant) ? $invocant->product_obj : $params->{product}; my %add_groups; @@ -1640,14 +1642,12 @@ sub _check_groups { if !ref $group_names; # First check all the groups they chose to set. + my %args = ( product => $product->name, bug_id => $bug_id, action => 'add' ); foreach my $name (@$group_names) { - my $group = Bugzilla::Group->check( - { name => $name, product => $product, - _error => 'group_restriction_not_allowed' }); + my $group = Bugzilla::Group->check_no_disclose({ %args, name => $name }); if (!$product->group_is_settable($group)) { - ThrowUserError('group_restriction_not_allowed', - { name => $name, product => $product }); + ThrowUserError('group_restriction_not_allowed', { %args, name => $name }); } $add_groups{$group->id} = $group; } @@ -2512,20 +2512,18 @@ sub _set_product { $self->set_target_milestone($tm_name); } } - + if ($product_changed) { - # Remove groups that can't be set in the new product, or that aren't - # active. - # + # Remove groups that can't be set in the new product. # We copy this array because the original array is modified while we're # working, and that confuses "foreach". my @current_groups = @{$self->groups_in}; foreach my $group (@current_groups) { - if (!$group->is_active or !$product->group_is_valid($group)) { + if (!$product->group_is_valid($group)) { $self->remove_group($group); } } - + # Make sure the bug is in all the mandatory groups for the new product. foreach my $group (@{$product->groups_mandatory}) { $self->add_group($group); @@ -2757,18 +2755,25 @@ sub modify_keywords { sub add_group { my ($self, $group) = @_; - # Invalid ids are silently ignored. (We can't tell people whether - # or not a group exists.) - $group = Bugzilla::Group->check($group) if !blessed $group; - return if !$group->is_active or !$group->is_bug_group; + # If the user enters "FoO" but the DB has "Foo", $group->name would + # return "Foo" and thus revealing the existence of the group name. + # So we have to store and pass the name as entered by the user to + # the error message, if we have it. + my $group_name = blessed($group) ? $group->name : $group; + my $args = { name => $group_name, product => $self->product, + bug_id => $self->id, action => 'add' }; + + $group = Bugzilla::Group->check_no_disclose($args) if !blessed $group; + + # If the bug is already in this group, then there is nothing to do. + return if $self->in_group($group); + # Make sure that bugs in this product can actually be restricted # to this group by the current user. $self->product_obj->group_is_settable($group) - || ThrowUserError('group_invalid_restriction', - { product => $self->product, group => $group, - bug => $self }); + || ThrowUserError('group_restriction_not_allowed', $args); # OtherControl people can add groups only during a product change, # and only when the group is not NA for them. @@ -2777,37 +2782,38 @@ sub add_group { if (!$self->{_old_product_name} || $controls->{othercontrol} == CONTROLMAPNA) { - ThrowUserError('group_change_denied', - { bug => $self, group => $group }); + ThrowUserError('group_restriction_not_allowed', $args); } } my $current_groups = $self->groups_in; - if (!grep($group->id == $_->id, @$current_groups)) { - push(@$current_groups, $group); - } + push(@$current_groups, $group); } sub remove_group { my ($self, $group) = @_; - $group = Bugzilla::Group->check($group) if !blessed $group; - - # First, check if this is a valid group for this product. - # You can *always* remove a group that is not valid for this product - # or that is not active, so we don't do any other checks if either of - # those are the case. (Users might remove inactive groups, and set_product - # removes groups that aren't valid for this product.) - # - # This particularly happens when isbuggroup is no longer 1, and we're - # moving a bug to a new product. - if ($group->is_active and $self->product_obj->group_is_valid($group)) { + + # See add_group() for the reason why we store the user input. + my $group_name = blessed($group) ? $group->name : $group; + my $args = { name => $group_name, product => $self->product, + bug_id => $self->id, action => 'remove' }; + + $group = Bugzilla::Group->check_no_disclose($args) if !blessed $group; + + # If the bug isn't in this group, then either the name is misspelled, + # or the group really doesn't exist. Let the user know about this problem. + $self->in_group($group) || ThrowUserError('group_invalid_removal', $args); + + # Check if this is a valid group for this product. You can *always* + # remove a group that is not valid for this product (set_product does this). + # This particularly happens when we're moving a bug to a new product. + # You still have to be a member of an inactive group to remove it. + if ($self->product_obj->group_is_valid($group)) { my $controls = $self->product_obj->group_controls->{$group->id}; - # Nobody can ever remove a Mandatory group. - if ($controls->{membercontrol} == CONTROLMAPMANDATORY) { - ThrowUserError('group_invalid_removal', - { product => $self->product, group => $group, - bug => $self }); + # Nobody can ever remove a Mandatory group, unless it became inactive. + if ($controls->{membercontrol} == CONTROLMAPMANDATORY && $group->is_active) { + ThrowUserError('group_invalid_removal', $args); } # OtherControl people can remove groups only during a product change, @@ -2817,12 +2823,11 @@ sub remove_group { || $controls->{othercontrol} == CONTROLMAPMANDATORY || $controls->{othercontrol} == CONTROLMAPNA) { - ThrowUserError('group_change_denied', - { bug => $self, group => $group }); + ThrowUserError('group_invalid_removal', $args); } } } - + my $current_groups = $self->groups_in; @$current_groups = grep { $_->id != $group->id } @$current_groups; } @@ -3529,6 +3534,11 @@ sub groups_in { return $self->{'groups_in'}; } +sub in_group { + my ($self, $group) = @_; + return grep($_->id == $group->id, @{$self->groups_in}) ? 1 : 0; +} + sub user { my $self = shift; return $self->{'user'} if exists $self->{'user'}; diff --git a/Bugzilla/Group.pm b/Bugzilla/Group.pm index 11a27e2e6..b7532fe09 100644 --- a/Bugzilla/Group.pm +++ b/Bugzilla/Group.pm @@ -439,6 +439,21 @@ sub ValidateGroupName { return $ret; } +sub check_no_disclose { + my ($class, $params) = @_; + my $action = delete $params->{action}; + + $action =~ /^(?:add|remove)$/ + or ThrowCodeError('bad_arg', { argument => $action, + function => "${class}::check_no_disclose" }); + + $params->{_error} = ($action eq 'add') ? 'group_restriction_not_allowed' + : 'group_invalid_removal'; + + my $group = $class->check($params); + return $group; +} + ############################### ### Validators ### ############################### @@ -538,6 +553,47 @@ Returns: It returns the group id if successful =over +=item C<check_no_disclose> + +=over + +=item B<Description> + +Throws an error if the user cannot add or remove this group to/from a given +bug, but doesn't specify if this is because the group doesn't exist, or the +user is not allowed to edit this group restriction. + +=item B<Params> + +This method takes a single hashref as argument, with the following keys: + +=over + +=item C<name> + +C<string> The name of the group to add or remove. + +=item C<bug_id> + +C<integer> The ID of the bug to which the group change applies. + +=item C<product> + +C<string> The name of the product the bug belongs to. + +=item C<action> + +C<string> Must be either C<add> or C<remove>, depending on whether the group +must be added or removed from the bug. Any other value will generate an error. + +=back + +=item C<Returns> + +A C<Bugzilla::Group> object on success, else an error is thrown. + +=back + =item C<check_members_are_visible> Throws an error if this group is not visible (according to diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm index b9443e9e6..85524ac47 100644 --- a/Bugzilla/Product.pm +++ b/Bugzilla/Product.pm @@ -680,10 +680,12 @@ sub groups_mandatory { # if this group can be validly set by the currently-logged-in user. sub group_is_settable { my ($self, $group) = @_; - my $group_id = blessed($group) ? $group->id : $group; - my $is_mandatory = grep { $group_id == $_->id } + + return 0 unless ($group->is_active && $group->is_bug_group); + + my $is_mandatory = grep { $group->id == $_->id } @{ $self->groups_mandatory }; - my $is_available = grep { $group_id == $_->id } + my $is_available = grep { $group->id == $_->id } @{ $self->groups_available }; return ($is_mandatory or $is_available) ? 1 : 0; } @@ -948,7 +950,7 @@ a bug. (In fact, the user I<must> set the Mandatory group on the bug.) =over -=item C<$group> - Either a numeric group id or a L<Bugzilla::Group> object. +=item C<$group> - A L<Bugzilla::Group> object. =back diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm index cb518b0bd..123b14d68 100644 --- a/Bugzilla/WebService/Constants.pm +++ b/Bugzilla/WebService/Constants.pm @@ -109,8 +109,7 @@ use constant WS_ERROR_CODE => { dupe_loop_detected => 118, dupe_id_required => 119, # Bug-related group errors - group_change_denied => 120, - group_invalid_restriction => 120, + group_invalid_removal => 120, group_restriction_not_allowed => 120, # Status/Resolution errors missing_resolution => 121, diff --git a/process_bug.cgi b/process_bug.cgi index 0348424fa..acb359f63 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -345,7 +345,17 @@ foreach my $field (@custom_fields) { } } +# We are going to alter the list of removed groups, so we keep a copy here. +my @unchecked_groups = @$removed_groups; foreach my $b (@bug_objects) { + # Don't blindly ask to remove unchecked groups available in the UI. + # A group can be already unchecked, and the user didn't try to remove it. + # In this case, we don't want remove_group() to complain. + my @remove_groups; + foreach my $g (@{$b->groups_in}) { + push(@remove_groups, $g->name) if grep { $_ eq $g->name } @unchecked_groups; + } + local $set_all_fields{groups}->{remove} = \@remove_groups; $b->set_all(\%set_all_fields); } diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 52ac64ddd..3e1b8748e 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -735,12 +735,6 @@ in the database which refer to it. All references to this group must be removed before you can remove it. - [% ELSIF error == "group_change_denied" %] - [% title = "Cannot Add/Remove That Group" %] - You tried to add or remove the '[% group.name FILTER html %]' group - from [% terms.bug %] [%+ bug.id FILTER html %], but you do not - have permissions to do so. - [% ELSIF error == "group_exists" %] [% title = "The group already exists" %] The group [% name FILTER html %] already exists. @@ -761,23 +755,17 @@ [% ELSIF error == "group_invalid_removal" %] - You tried to remove [% terms.bug %] [%+ bug.id FILTER html %] - from the '[% group.name FILTER html %]' group, but [% terms.bugs %] - in the '[% product FILTER html %]' product can not be removed from that - group. - - [% ELSIF error == "group_invalid_restriction" %] - You tried to restrict [% terms.bug %] [%+ bug.id FILTER html %] to - to the '[% group.name FILTER html %]' group, but [% terms.bugs %] in the - '[% product FILTER html %]' product can not be restricted to - that group. + You tried to remove [% terms.bug %] [%+ bug_id FILTER html %] + from the '[% name FILTER html %]' group, but either this group does not exist, + or you are not allowed to remove [% terms.bugs %] from this group in the + '[% product FILTER html %]' product. [% ELSIF error == "group_restriction_not_allowed" %] [% title = "Group Restriction Not Allowed" %] - You tried to restrict [% terms.abug %] to the "[% name FILTER html %]" - group, but either this group does not exist, or you are not allowed - to restrict [% terms.bugs %] to this group in the "[% product.name FILTER html %]" - product. + You tried to restrict [% bug_id ? "$terms.bug $bug_id" : terms.abug FILTER html %] + to the '[% name FILTER html %]' group, but either this group does not exist, + or you are not allowed to restrict [% terms.bugs %] to this group in the + '[% product FILTER html %]' product. [% ELSIF error == "group_not_specified" %] [% title = "Group not specified" %] |