318171 - Looking for group members finds grant right holders, too

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Frédéric Buclin]

In 2.20, if a user can grant admin privs but isn't (explicitly) in the admin group himself, all groups, including the admin group, are greyed in editusers.cgi. And looking for users being in the admin group lists this user.

In 2.22, the UI is different. In the same situation, no group is greyed anymore, suggesting that the user doesn't inherit admin privs, but requesting all users having admin privs still lists this user. This is a bit confusing!

The question is then:
Should this user have admin privs as he can grant these privs to another user, or shouldn't he? In both cases, either the UI or the SQL query has to be fixed.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

This is not only confusing, but a bug :)

Plus, it's not limited to the admin group.

Comment 2

[Joel Peshkin]

I don't think we want to force someone to be a member of a group just because they can bless it.  The patch looks like it does the right thing.

Comment 3

[Frédéric Buclin]

Comment on attachment 206662 [details] [diff] [review]
Patch

>             $query .= " $nextCondition profiles.userid = ugm.user_id " .
>-                      "AND ugm.group_id IN($grouplist)";
>+                      "AND ugm.group_id IN($grouplist) " .
>+                      "AND ugm.isbless = 0";


Two comments:

1) "$nextCondition profiles.userid = ugm.user_id" + "AND ugm.isbless = 0" should be moved into the following block at line 100:

    } else {
        $visibleGroups = 1;
        if ($grouprestrict eq '1') {
            $query .= ', user_group_map AS ugm';
        }
        $nextCondition = 'WHERE';
    }

else if you have both usevisibilitygroups turned on and you restrict your query to some given group, you have this information being duplicated:

SELECT DISTINCT userid, login_name, realname, disabledtext FROM profiles, user_group_map AS ugm
                         WHERE ugm.user_id = profiles.userid
                           AND ugm.isbless = 0
                           AND ugm.group_id IN (43, 41)
                         AND LOWER(profiles.login_name) LIKE LOWER(?) AND profiles.userid = ugm.user_id AND ugm.group_id IN(43,41) AND ugm.isbless = 0 ORDER BY profiles.login_name at /var/www/html/bugzilla-pg/editusers.cgi line 146.


Note that you have twice "ugm.user_id = profiles.userid" and "ugm.isbless = 0" which is useless. This doesn't generate errors though. Here I just asked editusers.cgi to die to show me the query it's going to execute.


2) 2.18 and 2.20 still show users having bless privs as beloging to the groups they can bless. This behavior changed in 2.22. Is that intentional? If yes, I'm fine with this change, if not, then it's a bug. joel?


Anyway, the patch works correctly. r=LpSolit for tip only.

Comment 4

[Marc Schumann [:Wurblzap]]

I'll look into your comment about ugm code duplication.

On grey backgrounds: the 2.22 behaviour is the correct one. If you may grant a group's privs, there is no reason to be shown to be a member of the groups that inherit membership from it. I think that, as a positive side effect, bug 305476 fixed this for the tip; the 2.20 backport needs to include this side effect.
I don't know yet about 2.18.

Comment 5

[Marc Schumann [:Wurblzap]]

Frédéric, your suggestion on IRC (leave 2.20 broken because 2.18 was broken in the same way) seems to me to be the most feasible way to go on at the moment, so I'm checking this in to the tip.

If a 2.20 fix is needed, it can be done here or in another bug. Backporting the patch of bug 305476 to 2.20 should be possible.

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.112; previous revision: 1.111
done