From d2c2138b20b7a3c8e05fda35d3116862fcda210b Mon Sep 17 00:00:00 2001 From: Simon Green Date: Fri, 6 Jun 2014 09:47:27 +1000 Subject: Bug 442013 - Create Bugzilla::User->set_groups and set_bless_groups and have editusers.cgi use them r=justdave, a=glob --- Bugzilla/User.pm | 206 +++++++++++++++++++++++++- editusers.cgi | 95 ++---------- template/en/default/global/messages.html.tmpl | 54 +++---- 3 files changed, 239 insertions(+), 116 deletions(-) diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 81ce228e7..7aeb9f8ee 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -144,12 +144,80 @@ sub super_user { return $user; } +sub _update_groups { + my $self = shift; + my $group_changes = shift; + my $changes = shift; + my $dbh = Bugzilla->dbh; + + # Update group settings. + my $sth_add_mapping = $dbh->prepare( + qq{INSERT INTO user_group_map ( + user_id, group_id, isbless, grant_type + ) VALUES ( + ?, ?, ?, ? + ) + }); + my $sth_remove_mapping = $dbh->prepare( + qq{DELETE FROM user_group_map + WHERE user_id = ? + AND group_id = ? + AND isbless = ? + AND grant_type = ? + }); + + foreach my $is_bless (keys %$group_changes) { + my ($removed, $added) = @{$group_changes->{$is_bless}}; + + foreach my $group (@$removed) { + $sth_remove_mapping->execute( + $self->id, $group->id, $is_bless, GRANT_DIRECT + ); + } + foreach my $group (@$added) { + $sth_add_mapping->execute( + $self->id, $group->id, $is_bless, GRANT_DIRECT + ); + } + + if (! $is_bless) { + my $query = qq{ + INSERT INTO profiles_activity + (userid, who, profiles_when, fieldid, oldvalue, newvalue) + VALUES ( ?, ?, now(), ?, ?, ?) + }; + + $dbh->do( + $query, undef, + $self->id, Bugzilla->user->id, + get_field_id('bug_group'), + join(', ', map { $_->name } @$removed), + join(', ', map { $_->name } @$added) + ); + } + else { + # XXX: should create profiles_activity entries for blesser changes. + } + + Bugzilla->memcached->clear_config({ key => 'user_groups.' . $self->id }); + + my $type = $is_bless ? 'bless_groups' : 'groups'; + $changes->{$type} = [ + [ map { $_->name } @$removed ], + [ map { $_->name } @$added ], + ]; + } +} + sub update { my $self = shift; my $options = shift; - + + my $group_changes = delete $self->{_group_changes}; + my $changes = $self->SUPER::update(@_); my $dbh = Bugzilla->dbh; + $self->_update_groups($group_changes, $changes); if (exists $changes->{login_name}) { # Delete all the tokens related to the userid @@ -266,6 +334,111 @@ sub set_disabledtext { $_[0]->set('is_enabled', $_[1] ? 0 : 1); } +sub set_groups { + my $self = shift; + $self->_set_groups(GROUP_MEMBERSHIP, @_); +} + +sub set_bless_groups { + my $self = shift; + + # The person making the change needs to be in the editusers group + Bugzilla->user->in_group('editusers') + || ThrowUserError("auth_failure", {group => "editusers", + reason => "cant_bless", + action => "edit", + object => "users"}); + + $self->_set_groups(GROUP_BLESS, @_); +} + +sub _set_groups { + my $self = shift; + my $is_bless = shift; + my $changes = shift; + my $dbh = Bugzilla->dbh; + + # The person making the change is $user, $self is the person being changed + my $user = Bugzilla->user; + + # Input is a hash of arrays. Key is 'set', 'add' or 'remove'. The array + # is a list of group ids and/or names. + + # First turn the arrays into group objects. + $changes = $self->_set_groups_to_object($changes); + + # Get a list of the groups the user currently is a member of + my $ids = $dbh->selectcol_arrayref( + q{SELECT DISTINCT group_id + FROM user_group_map + WHERE user_id = ? AND isbless = ? AND grant_type = ?}, + undef, $self->id, $is_bless, GRANT_DIRECT); + + my $new_groups = my $current_groups = Bugzilla::Group->new_from_list($ids); + + # Record the changes + if (exists $changes->{set}) { + $new_groups = $changes->{set}; + + # We need to check the user has bless rights on the existing groups + # If they don't, then we need to add them back to new_groups + foreach my $group (@$current_groups) { + if (! $user->can_bless($group->id)) { + push @$new_groups, $group + unless grep { $_->id eq $group->id } @$new_groups; + } + } + } + else { + foreach my $group (@{$changes->{removed} // []}) { + @$new_groups = grep { $_->id ne $group->id } @$new_groups; + } + foreach my $group (@{$changes->{added} // []}) { + push @$new_groups, $group + unless grep { $_->id eq $group->id } @$new_groups; + } + } + + # Stash the changes, so self->update can actually make them + my @diffs = diff_arrays($current_groups, $new_groups, 'id'); + if (scalar(@{$diffs[0]}) || scalar(@{$diffs[1]})) { + $self->{_group_changes}{$is_bless} = \@diffs; + } +} + +sub _set_groups_to_object { + my $self = shift; + my $changes = shift; + my $user = Bugzilla->user; + + foreach my $key (keys %$changes) { + # Check we were given an array + unless (ref($changes->{$key}) eq 'ARRAY') { + ThrowCodeError( + 'param_invalid', + { param => $changes->{$key}, function => $key } + ); + } + + # Go through the array, and turn items into group objects + my @groups = (); + foreach my $value (@{$changes->{$key}}) { + my $type = $value =~ /^\d+$/ ? 'id' : 'name'; + my $group = Bugzilla::Group->new({$type => $value}); + + if (! $group || ! $user->can_bless($group->id)) { + ThrowUserError('auth_failure', + { group => $value, reason => 'cant_bless', + action => 'edit', object => 'users' }); + } + push @groups, $group; + } + $changes->{$key} = \@groups; + } + + return $changes; +} + sub update_last_seen_date { my $self = shift; return unless $self->id; @@ -2834,6 +3007,37 @@ User is in the cc list for the bug. =back +=item C + +C These specify the groups that this user is directly a member of. +To set these, you should pass a hash as the value. The hash may contain +the following fields: + +=over + +=item C An array of Cs or Cs. The group ids or group names +that the user should be added to. + +=item C An array of Cs or Cs. The group ids or group names +that the user should be removed from. + +=item C An array of Cs or Cs. An exact set of group ids +and group names that the user should be a member of. NOTE: This does not +remove groups from the user where the person making the change does not +have the bless privilege for. + +If you specify C, then C and C will be ignored. A group in +both the C and C list will be added. Specifying a group that the +user making the change does not have bless rights will generate an error. + +=back + +=item C + +C - This is the same as set_groups, but affects what groups a user +has direct membership to bless that group. It takes the same inputs as +set_groups. + =back =head1 CLASS FUNCTIONS diff --git a/editusers.cgi b/editusers.cgi index 650784d5d..3ce22068e 100755 --- a/editusers.cgi +++ b/editusers.cgi @@ -248,7 +248,7 @@ if ($action eq 'search') { # Lock tables during the check+update session. $dbh->bz_start_transaction(); - + $editusers || $user->can_see_user($otherUser) || ThrowUserError('auth_failure', {reason => "not_visible", action => "modify", @@ -256,6 +256,10 @@ if ($action eq 'search') { $vars->{'loginold'} = $otherUser->login; + # Update groups + my @group_ids = grep { s/group_// } keys %{ Bugzilla->cgi->Vars }; + $otherUser->set_groups({ set => \@group_ids }); + # Update profiles table entry; silently skip doing this if the user # is not authorized. my $changes = {}; @@ -268,87 +272,12 @@ if ($action eq 'search') { $otherUser->set_disable_mail($cgi->param('disable_mail')); $otherUser->set_extern_id($cgi->param('extern_id')) if defined($cgi->param('extern_id')); - $changes = $otherUser->update(); - } - - # Update group settings. - my $sth_add_mapping = $dbh->prepare( - qq{INSERT INTO user_group_map ( - user_id, group_id, isbless, grant_type - ) VALUES ( - ?, ?, ?, ? - ) - }); - my $sth_remove_mapping = $dbh->prepare( - qq{DELETE FROM user_group_map - WHERE user_id = ? - AND group_id = ? - AND isbless = ? - AND grant_type = ? - }); - - my @groupsAddedTo; - my @groupsRemovedFrom; - my @groupsGrantedRightsToBless; - my @groupsDeniedRightsToBless; - - # Regard only groups the user is allowed to bless and skip all others - # silently. - # XXX: checking for existence of each user_group_map entry - # would allow to display a friendlier error message on page reloads. - userDataToVars($otherUserID); - my $permissions = $vars->{'permissions'}; - foreach my $blessable (@{$user->bless_groups()}) { - my $id = $blessable->id; - my $name = $blessable->name; - - # Change memberships. - my $groupid = $cgi->param("group_$id") || 0; - if ($groupid != $permissions->{$id}->{'directmember'}) { - if (!$groupid) { - $sth_remove_mapping->execute( - $otherUserID, $id, 0, GRANT_DIRECT); - push(@groupsRemovedFrom, $name); - } else { - $sth_add_mapping->execute( - $otherUserID, $id, 0, GRANT_DIRECT); - push(@groupsAddedTo, $name); - } - } - # Only members of the editusers group may change bless grants. - # Skip silently if this is not the case. - if ($editusers) { - my $groupid = $cgi->param("bless_$id") || 0; - if ($groupid != $permissions->{$id}->{'directbless'}) { - if (!$groupid) { - $sth_remove_mapping->execute( - $otherUserID, $id, 1, GRANT_DIRECT); - push(@groupsDeniedRightsToBless, $name); - } else { - $sth_add_mapping->execute( - $otherUserID, $id, 1, GRANT_DIRECT); - push(@groupsGrantedRightsToBless, $name); - } - } - } - } - if (@groupsAddedTo || @groupsRemovedFrom) { - $dbh->do(qq{INSERT INTO profiles_activity ( - userid, who, - profiles_when, fieldid, - oldvalue, newvalue - ) VALUES ( - ?, ?, now(), ?, ?, ? - ) - }, - undef, - ($otherUserID, $userid, - get_field_id('bug_group'), - join(', ', @groupsRemovedFrom), join(', ', @groupsAddedTo))); - Bugzilla->memcached->clear_config({ key => "user_groups.$otherUserID" }) + # Update bless groups + my @bless_ids = grep { s/bless_// } keys %{ Bugzilla->cgi->Vars }; + $otherUser->set_bless_groups({ set => \@bless_ids }); } - # XXX: should create profiles_activity entries for blesser changes. + $changes = $otherUser->update(); $dbh->bz_commit_transaction(); @@ -357,11 +286,7 @@ if ($action eq 'search') { delete_token($token); $vars->{'message'} = 'account_updated'; - $vars->{'changed_fields'} = [keys %$changes]; - $vars->{'groups_added_to'} = \@groupsAddedTo; - $vars->{'groups_removed_from'} = \@groupsRemovedFrom; - $vars->{'groups_granted_rights_to_bless'} = \@groupsGrantedRightsToBless; - $vars->{'groups_denied_rights_to_bless'} = \@groupsDeniedRightsToBless; + $vars->{'changes'} = \%$changes; # We already display the updated page. We have to recreate a token now. $vars->{'token'} = issue_session_token('edit_user'); diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl index 34c678e4f..f47a1d6ec 100644 --- a/template/en/default/global/messages.html.tmpl +++ b/template/en/default/global/messages.html.tmpl @@ -26,14 +26,12 @@ canceled. [% ELSIF message_tag == "account_updated" %] - [% IF changed_fields.size - + groups_added_to.size + groups_removed_from.size - + groups_granted_rights_to_bless.size + groups_denied_rights_to_bless.size %] + [% IF changes.size %] [% title = "User $loginold updated" %] The following changes have been made to the user account [%+ loginold FILTER html %]:
    - [% FOREACH field = changed_fields %] + [% FOREACH field = changes.keys %]
  • [% IF field == 'login_name' %] The login is now [% otheruser.login FILTER html %]. @@ -53,35 +51,31 @@ [% ELSE %] [% terms.Bug %]mail has been enabled. [% END %] + [% ELSIF field == 'groups' %] + [% IF changes.groups.1.size %] + The account has been added to the + [%+ changes.groups.1.join(', ') FILTER html %] + group[% 's' IF changes.groups.1.size > 1 %]. + [% END %] + [% IF changes.groups.0.size %] + The account has been removed from the + [%+ changes.groups.0.join(', ') FILTER html %] + group[% 's' IF changes.groups.0.size > 1 %]. + [% END %] + [% ELSIF field == 'bless_groups' %] + [% IF changes.bless_groups.1.size %] + The account has been granted rights to bless the + [%+ changes.bless_groups.1.join(', ') FILTER html %] + group[% 's' IF changes.bless_groups.1.size > 1 %]. + [% END %] + [% IF changes.bless_groups.0.size %] + The account has been denied rights to bless the + [%+ changes.bless_groups.0.join(', ') FILTER html %] + group[% 's' IF changes.bless_groups.0.size > 1 %]. + [% END %] [% END %]
    • [% END %] - [% IF groups_added_to.size %] -
  • - The account has been added to the following group[% 's' IF groups_added_to.size > 1 %]: - [%+ groups_added_to.join(', ') FILTER html %] -
    • - [% END %] - [% IF groups_removed_from.size %] -
  • - The account has been removed from the following group[% 's' IF groups_removed_from.size > 1 %]: - [%+ groups_removed_from.join(', ') FILTER html %] -
    • - [% END %] - [% IF groups_granted_rights_to_bless.size %] -
  • - The account has been granted rights to bless the - [%+ groups_granted_rights_to_bless.join(', ') FILTER html %] - group[% 's' IF groups_granted_rights_to_bless.size > 1 %]. -
    • - [% END %] - [% IF groups_denied_rights_to_bless.size %] -
  • - The account has been denied rights to bless the - [%+ groups_denied_rights_to_bless.join(', ') FILTER html %] - group[% 's' IF groups_denied_rights_to_bless.size > 1 %]. -
    • - [% END %]
[% ELSE %] [% title = "User $otheruser.login not changed" %] -- cgit v1.2.3-24-g4f1b