465818 - The sudo session UI lists all active users, even those who cannot be impersonated

Status: NEW

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

When you access relogin.cgi?action=prepare-sudo with usemenuforusers turned on, all active users are listed, even those who cannot be impersonated. These "protected" users must be excluded from the list.

Comment 1

[arbingersys]

Attachment: First try

Hi, I modified User::get_userlist() to take an optional parameter $directive and if this is set to 'sodufilter' it queries the database for a list of logins that are in either the 'admin' or 'bz_sudo_protect' groups, and makes a string ($filter_by) out of them. Then, when the user list is built, any logins that match the string are omitted.

In relogin.cgi, I assign $vars->{'custom_userlist'} before calling the template. 

runtests.pl was an all pass, and it behaves how it should for me in the UI. Let me know if this works. Thanks, James

Comment 2

[arbingersys]

Attachment: 2nd try - missed 'bz_sudoers' group in filter

I realized that the bz_sudoers group needed to be included in the query. The patch is the same as the first except for this change. James

Comment 3

[Frédéric Buclin]

Comment on attachment 360043 [details] [diff] [review]
2nd try - missed 'bz_sudoers' group in filter

>Index: Bugzilla/User.pm

> sub get_userlist {

>+    if ($directive eq 'sudofilter') {

get_userlist() should take an optional group name, and filter users based on this name (either as inclusion or exclusion). For this bug, the only relevant group is bz_sudo_protect. admin and bz_sudoers groups are not the ones to look at.

For the long term, it would be even better to use Bugzilla::User->match().

Comment 4

[Frédéric Buclin]

Won't go on a branch, and we are already frozen for 3.4.

Comment 5

[arbingersys]

Attachment: diff

Well, I can't seem to get away from constructing a string for filtering, but this time at least I use the Bugzilla::Group members_direct() method (much better!) and added a flag param so you can include or exclude members from the output.

Comment 6

[Frédéric Buclin]

Comment on attachment 362095 [details] [diff] [review]
diff

>Index: Bugzilla/User.pm

> sub get_userlist {
>     my $self = shift;
>+    my $group_name = shift;     # optional group to filter by
>+    my $group_exc = shift;      # optional include or exclude flag

Write: my ($self, $group_name, $group_exclude) = @_; instead. (See my last comment in this review about this.)
That's not the right place to describe variables. You should put this in the POD, which is missing in this patch.


>     return $self->{'userlist'} if defined $self->{'userlist'};

If you already called get_userlist(), you will get the previous list, independently of the new parameters you pass to get_userlist(). So it should only be cached/returned if there is no argument. Also, if the list was already cached, you should reuse it instead of running the SQL query again.


>+    if (defined($group_name)) {
>+        my $group = new Bugzilla::Group({'name' => $group_name}); 
>+        my $users = $group->members_direct();

$group->member_direct() will crash if you passed an invalid group name. You have to make sure the $group object is defined.
Also, I'm not sure member_direct() returns users who inherit membership, e.g. via regexp (joel would know better).


>     while (my($login, $name, $visible) = $sth->fetchrow_array) {
>+        if (defined($filter)) {
>+            if (defined($group_exc)) { # exclude
>+                next if $filter =~ /$login/;
>+            } else { # include
>+                next if $filter !~ /$login/;
>+            }

These regexps are very unsecure. Imagine there are [e-mail] and [e-mail]:
  [e-mail] =~ /[e-mail]/ will return true. You should rather use loops here, I think.



>Index: relogin.cgi

>+    # Use a custom list that excludes members of this group

"You cannot impersonate members of the bz_sudo_protect group" would be a better comment, IMO.


>+    $vars->{'custom_userlist'} = $user->get_userlist('bz_sudo_protect', 1);

get_userlist({ exclude => ['bz_sudo_protect'] }) would probably be a cleaner way to pass groups to include/exclude. This way, you could pass more than one group.

Comment 7

[arbingersys]

Attachment: patch4

Okay, I think I've got everything you mentioned. You can call get_userlist() with hash params 'exclude' or 'include' and they can point to an array of group names.

James

Comment 8

[arbingersys]

Attachment: patch4a

Installed on a second computer and discovered it didn't like the way I was assigning $match. Also, I switched @filter to %filter so you don't get a bunch of duplicates.

Comment 9

[Frédéric Buclin]

Comment on attachment 392125 [details] [diff] [review]
patch4a

>Index: Bugzilla/User.pm

> sub get_userlist {
>     my $self = shift;
>+    my ($param) = @_;

Simply write: my ($self, $params) = @_;


>+    return $self->{'userlist'} if 
>+        !defined $group_names or defined $self->{'userlist'};

The logic is wrong. It must be &&, not or. Also, you should rather look at scalar(@$group_names) rather than defined.


>+    foreach my $group_name (@$group_names) {
>+        my $group = new Bugzilla::Group({'name' => $group_name}); 
>+        if (defined($group)) {
>+            my $users = $group->members_direct();
>+            map {$filter{$_->login} = 1} @$users;
>+        }
>+    }

Use new_from_list() to get all group objects at once. That's faster.
Also, don't call member_direct(), but flatten_group_membership(). See how we use it in editusers.cgi when action=list. This way, we filter the user list in the SQL query itself, which is more efficient than your multi-loop later in the code, especially if the SQL query is going to return many results.

Also, updated POD is still missing.

Comment 10

[arbingersys]

(In reply to comment #9)
> Use new_from_list() to get all group objects at once. That's faster.

Except from what I can see, new_from_list() expects group IDs rather than names.  And since the get_userlist() 'exclude' parameter will be group name(s) I'd first have to query to get group IDs. So, couldn't I just do the following (no group instantiation required)?

- query to get group IDs from the list of names in the 'exclude' parameter
- query for users 
      'WHERE group_id in ' . flatten_group_membership('exclude' group IDs) 
  and build a filter hash
- return only users that aren't in the filter hash

This omits the foreach loop above and removes any need for Group object instantiation, which seems like an unnecessary cost just to get a list of users by which to filter.

> This way, we filter the user list in
> the SQL query itself, which is more efficient than your multi-loop later in the
> code, especially if the SQL query is going to return many results.

The SQL query as it stands would still return a user, if the user belonged to any other group beside the one(s) specified via 'exclude'. E.g. if a user is in 'editbugs' and also 'bz_sudo_protect', they'll still end up in the result set and still be visible in the select list.