Bug 284263: Inherited bless permissions without inherited membership do not work - Patch by Marc Schumann <wurblzap@gmail.com> r=joel,LpSolit a=justdave

webtools/bugzilla/Bugzilla/User.pm

@@ -291,33 +291,51 @@ sub groups {
 sub bless_groups {
     my $self = shift;
 
-    return $self->{bless_groups} if defined $self->{bless_groups};
+    return $self->{'bless_groups'} if defined $self->{'bless_groups'};
     return {} unless $self->id;
 
     my $dbh = Bugzilla->dbh;
-    # Get all groups for the user where:
-    #    + They have direct bless privileges
-    #    + They are a member of a group that inherits bless privs.
-    # Because of the second requirement, derive_groups must be up-to-date
-    # for this to function properly in all circumstances.
-    my $bless_groups = $dbh->selectcol_arrayref(
-        q{SELECT DISTINCT groups.name, groups.id
-            FROM groups, user_group_map, group_group_map AS ggm
-           WHERE user_group_map.user_id = ?
-             AND ((user_group_map.isbless = 1
-                   AND groups.id=user_group_map.group_id)
-                  OR (groups.id = ggm.grantor_id
-                      AND ggm.grant_type = } . GROUP_BLESS .
-                   q{ AND user_group_map.group_id = ggm.member_id
-                      AND user_group_map.isbless = 0))},
-         { Columns=>[1,2] }, $self->{id});
+    my $query;
+    my $connector;
+    my @bindValues;
 
-    # The above gives us an arrayref [name, id, name, id, ...]
-    # Convert that into a hashref
-    my %bless_groups_hashref = @$bless_groups;
-    $self->{bless_groups} = \%bless_groups_hashref;
+    if ($self->in_group('editusers')) {
+        # Users having editusers permissions may bless all groups.
+        $query = 'SELECT DISTINCT id, name, description FROM groups';
+        $connector = 'WHERE';
+    }
+    else {
+        # Get all groups for the user where:
+        #    + They have direct bless privileges
+        #    + They are a member of a group that inherits bless privs.
+        # Because of the second requirement, derive_groups must be up-to-date
+        # for this to function properly in all circumstances.
+        $query = q{
+            SELECT DISTINCT groups.id, groups.name, groups.description
+                       FROM groups, user_group_map, group_group_map AS ggm
+                      WHERE user_group_map.user_id = ?
+                        AND ((user_group_map.isbless = 1
+                              AND groups.id=user_group_map.group_id)
+                             OR (groups.id = ggm.grantor_id
+                                 AND ggm.grant_type = ?
+                                 AND user_group_map.group_id = ggm.member_id
+                                 AND user_group_map.isbless = 0))};
+        $connector = 'AND';
+        @bindValues = ($self->id, GROUP_BLESS);
+    }
 
-    return $self->{bless_groups};
+    # If visibilitygroups are used, restrict the set of groups.
+    if (Param('usevisibilitygroups')) {
+        # Users need to see a group in order to bless it.
+        my $visibleGroups = join(', ', @{$self->visible_groups_direct()})
+            || return $self->{'bless_groups'} = [];
+        $query .= " $connector id in ($visibleGroups)";
+    }
+
+    $query .= ' ORDER BY name';
+
+    return $self->{'bless_groups'} =
+        $dbh->selectall_arrayref($query, {'Slice' => {}}, @bindValues);
 }
 
 sub in_group {
@@ -597,12 +615,12 @@ sub can_bless {
     if (!scalar(@_)) {
         # If we're called without an argument, just return 
         # whether or not we can bless at all.
-        return scalar(keys %{$self->bless_groups}) ? 1 : 0;
+        return scalar(@{$self->bless_groups}) ? 1 : 0;
     }
 
     # Otherwise, we're checking a specific group
     my $group_name = shift;
-    return exists($self->bless_groups->{$group_name});
+    return (grep {$$_{'name'} eq $group_name} (@{$self->bless_groups})) ? 1 : 0;
 }
 
 sub flatten_group_membership {
@@ -1430,10 +1448,12 @@ and getting all of the groups would be overkill.
 
 =item C<bless_groups>
 
-Returns a hashref of group names for groups that the user can bless. The keys
-are the names of the groups, whilst the values are the respective group ids.
-(This is so that a set of all groupids for groups the user can bless can be
-obtained by C<values(%{$user-E<gt>bless_groups})>.)
+Returns an arrayref of hashes of C<groups> entries, where the keys of each hash
+are the names of C<id>, C<name> and C<description> columns of the C<groups>
+table.
+The arrayref consists of the groups the user can bless, taking into account
+that having editusers permissions means that you can bless all groups, and
+that you need to be aware of a group in order to bless a group.
 
 =item C<can_see_user(user)>
 

webtools/bugzilla/editusers.cgi

@@ -66,7 +66,7 @@ mirrorListSelectionValues();
 ###########################################################################
 if ($action eq 'search') {
     # Allow to restrict the search to any group the user is allowed to bless.
-    $vars->{'restrictablegroups'} = groupsUserMayBless($user, 'id', 'name');
+    $vars->{'restrictablegroups'} = $user->bless_groups();
     $template->process('admin/users/search.html.tmpl', $vars)
        || ThrowTemplateError($template->error());
 
@@ -315,7 +315,7 @@ if ($action eq 'search') {
     # silently.
     # XXX: checking for existence of each user_group_map entry
     #      would allow to display a friendlier error message on page reloads.
-    foreach (@{groupsUserMayBless($user, 'id', 'name')}) {
+    foreach (@{$user->bless_groups()}) {
         my $id = $$_{'id'};
         my $name = $$_{'name'};
 
@@ -603,7 +603,7 @@ if ($action eq 'search') {
 
     $vars->{'message'} = 'account_deleted';
     $vars->{'otheruser'}{'login'} = $otherUserLogin;
-    $vars->{'restrictablegroups'} = groupsUserMayBless($user, 'id', 'name');
+    $vars->{'restrictablegroups'} = $user->bless_groups();
     $template->process('admin/users/search.html.tmpl', $vars)
        || ThrowTemplateError($template->error());
 
@@ -628,46 +628,6 @@ sub mirrorListSelectionValues {
     }
 }
 
-# Give a list of IDs of groups the user may bless.
-sub groupsUserMayBless {
-    my $user = shift;
-    my $fieldList = join(', ', @_);
-    my $query;
-    my $connector;
-    my @bindValues;
-
-    $user->derive_groups(1);
-
-    if ($editusers) {
-        $query = "SELECT DISTINCT $fieldList FROM groups";
-        $connector = 'WHERE';
-    } else {
-        $query = qq{SELECT DISTINCT $fieldList
-                    FROM groups
-                    LEFT JOIN user_group_map AS ugm
-                           ON groups.id = ugm.group_id
-                    LEFT JOIN group_group_map AS ggm
-                           ON ggm.member_id = ugm.group_id
-                          AND ggm.grant_type = ?
-                    WHERE user_id = ?
-                      AND (ugm.isbless = 1 OR groups.id = ggm.grantor_id)
-                   };
-        @bindValues = (GROUP_BLESS, $userid);
-        $connector = 'AND';
-    }
-
-    # If visibilitygroups are used, restrict the set of groups.
-    if (Param('usevisibilitygroups')) {
-        # Users need to see a group in order to bless it.
-        my $visibleGroups = $user->visible_groups_as_string() || return {};
-        $query .= " $connector id in ($visibleGroups)";
-    }
-
-    $query .= ' ORDER BY name';
-
-    return $dbh->selectall_arrayref($query, {'Slice' => {}}, @bindValues);
-}
-
 # Retrieve user data for the user editing form. User creation and user
 # editing code rely on this to call derive_groups().
 sub userDataToVars {
@@ -679,7 +639,7 @@ sub userDataToVars {
     $otheruser->derive_groups();
 
     $vars->{'otheruser'} = $otheruser;
-    $vars->{'groups'} = groupsUserMayBless($user, 'id', 'name', 'description');
+    $vars->{'groups'} = $user->bless_groups();
 
     $vars->{'permissions'} = $dbh->selectall_hashref(
         qq{SELECT id,