303784 - Visibility can keep admin from administering groups

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Joel Peshkin]

Either editusers or admin should override group and user visibility on all admin
pages.  Currently, it is possible for an admin to make groups invisible to
himself.  I do not see any reason this should apply to non-administrative pages.
 There is some benefit in permitting an admin to exclude himself from sections
of the system to avoid clutter.

Comment 1

[Joel Peshkin]

Attachment: Patch - have editusers.cgi let users with editusers see any user and every group

A user with editusers can always promote himself anyway.  Don't stand in the
way of that user administering groups.	Users with editusers are NOT forced to
see every group when not acting in an administrative capacity.	Note:  Users
who have bless permissions but no editusers membership are still more
restricted.

Comment 2

[Marc Schumann [:Wurblzap]]

If a site wants to give visibility on a group (all groups) to editusers people,
they can give the editusers group visibility to it (them). If a site wants to
give full user visibility to editusers people, they can introduce an everybody
group with .* regexp and give the editusers group visibility to it.

Why should we effectively force every site do this?

Comment 3

[Joel Peshkin]

Tried that.... it's pretty clunky.
Why would we want to prevent someone with editusers privileges from doing this?
There is nothing else a user with editusers cannot do.

Comment 4

[Frédéric Buclin]

Comment on attachment 192274 [details] [diff] [review]
Patch - have editusers.cgi let users with editusers see any user and every group

groupsUserMayBless() no longer exists, see bug 284263

Comment 5

[Joel Peshkin]

Attachment: v2 - unrotted

Comment 6

[Frédéric Buclin]

Comment on attachment 196321 [details] [diff] [review]
v2 - unrotted

The patch allows me to see all groups/users when I have editusers privs, but I
cannot edit any user account I'm not already allowed to edit, due to
User::can_see_user() which prevents me to access that account.

As can_see_user() is only used in editusers.cgi, I think it's safe to modify it
too in order to take Param('editusers') into account.

Comment 7

[Joel Peshkin]

I agree.  I just hit that one myself.

Comment 8

[Joel Peshkin]

Attachment: v3 - cover the rest of user editing as well

This patch keeps getting bigger.  There are several places where I removed
checks for csn_see_user since the check immediately prior to it checked for
editusers privileges altogether.

Comment 9

[Frédéric Buclin]

Comment on attachment 197235 [details] [diff] [review]
v3 - cover the rest of user editing as well

>Index: editusers.cgi

>+    if (!$editusers && (Param('usevisibilitygroups'))) {

Nit: why are you using so many parens (around Param())?


>+    $user->can_see_user($otherUser) || $editusers

Nit: I would prefer to check for $editusers first. If the user is in the
'editusers' group, we avoid a useless SQL call.


>+    $user->can_see_user($otherUser) || $editusers

Same remark here.



>Index: Bugzilla/User.pm

>+    if ((!$self->in_group('editusers')) && (Param('usevisibilitygroups'))) {

Nit: again, why so many parens??


Please fix the nits about checking $editusers first and carry forward my r+.
r=LpSolit for the tip only (the patch doesn't apply cleanly on 2.20).

Comment 10

[Frédéric Buclin]

As I said, I would like an updated patch with my nit fixed + a backport for 2.20.

Comment 11

[Joel Peshkin]

Attachment: v4 for HEAD with nits fixed

Comment 12

[Joel Peshkin]

Attachment: patch for 2.20

Comment 13

[Frédéric Buclin]

Comment on attachment 197312 [details] [diff] [review]
v4 for HEAD with nits fixed

r=LpSolit

Comment 14

[Frédéric Buclin]

Comment on attachment 197314 [details] [diff] [review]
patch for 2.20

r=LpSolit

Comment 15

[Frédéric Buclin]

tip:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.105; previous revision: 1.104
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.82; previous revision: 1.81
done


2.20rc2:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.90.2.6; previous revision: 1.90.2.5
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.61.2.8; previous revision: 1.61.2.7
done