264160 - All default groups need to inherit the admin group on a new install

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Dave Miller [:justdave]]

Currently on a new install, the newly-created admin is placed individually in
each group on the system, and there is no group inheritance set up.  Rather than
placing the admin in all the groups, we should just place them in admin and make
the other groups all inherit the admin membership.

Right now this makes it a pain in the butt to add a second admin because they
wind up not being able to access editparams.cgi, for example, even though they
have admin checked, because they don't have tweakparams checked and tweakparams
doesn't inherit admin.  This was supposed to be the whole point of the admin group.

Comment 1

[Dave Miller [:justdave]]

I'm calling this a regression because this is expected behavior from 2.16 that
no longer works.

Comment 2

[Marc Schumann [:Wurblzap]]

Attachment: Patch

I used to think that checksetup.pl restored group admins' group memberships. It
seems to me it doesn't. Is this intentional?

Comment 3

[Joel Peshkin]

(In reply to comment #2)

It should be possible for an admin to get out of a group and stay out while
keeping the ability to bless himself back in.  Being in too many groups creates
a very cluttered user interface.

Comment 4

[Marc Schumann [:Wurblzap]]

(In reply to comment #3)

Right, so shouldn't then checksetup.pl at least make sure that members of the
admin group get back their permission to bless editgroups if they lose it?

Comment 5

[Jacob Steenhagen]

(In reply to comment #3)
> (In reply to comment #2)
> 
> It should be possible for an admin to get out of a group and stay out while
> keeping the ability to bless himself back in.  Being in too many groups creates
> a very cluttered user interface.
> 

I disagree. If you don't like the "cluttered interface," use a second account.
After all, you don't run linux as root all day, do you? If I'm logged in as an
"admin," I'd expect to be able to do everything w/out having to first give
myself permission to do something... this comes from a background of network
administration.

Comment 6

[Jacob Steenhagen]

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

So, this problem is only on the initial admin created when ./checksetup.pl is
used a brand new installation? Perhaps we should also put something in
sanitycheck.cgi to look for groups that "admin" isn't a part of along with an
option to fix it.

Comment 7

[Max Kanat-Alexander]

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

justdave says I can review! Hooray. :-) OK, the code looks good (although the
patch is kind of weird and it's not CVS-diffed, so it took me a while to figure
it out.)

I tested and verified it manually on the tip, it works there.

However, this patch does not apply cleanly against the 2.18 branch.

Comment 8

[Max Kanat-Alexander]

Should be good for the tip, however I need to fix it for 2.18.

Comment 9

[Max Kanat-Alexander]

Attachment: Patch against 2.18

OK, here's the same patch, just changed for 2.18. The only real difference is
that we use isbless instead of grant_type in group_group_map for 2.18.

I tested it on a branch here, and it works. I'm not sure if I need to review
this, or if it needs review, or if I can review it myself, or what...

Comment 10

[Max Kanat-Alexander]

Comment on attachment 169513 [details] [diff] [review]
Patch against 2.18

Travis, could you look this over and make sure that it makes some kind of
sense? I know you normally only do doc reviews, but this is just a sort of
spot-check review, really.

I'd ask the patch author, I'm just concerned because he hasn't commented in two
months.

Comment 11

[Shane H. W. Travis]

Comment on attachment 169513 [details] [diff] [review]
Patch against 2.18

Max's patch does what he says it does, and applies to the 2.18 branch, so I'll
give it an r+

My concern is purely philosophical. When we create the admin account, we give
that account access to every *existing* group. As far as I know, though,
there's nothing in place to ensure that the admin has similar access to any
groups newly-created after the upgrade... is there? 

If not, then that seems wrong to me, because it's inconsistent. Either the
admin group should automatically be given inheritance access to every group
regardless of when it's created (my preferred solution), or we shouldn't be
giving them the impression that this is what's *going* to happen by doing so on
an upgrade.

I couldn't turn up any other bugs on this issue, so either I'm wrong in my
assumption, or nobody else has reported it yet. Anyone know one way or the
other?

Comment 12

[Dave Miller [:justdave]]

Admin should always inherit all permission groups always, period.  That's the
whole point of having the admin group.

non-system groups should be inherited by default, but let you remove yourself
from them if necessary.  Sometimes it's a professional courtesy thing, too.  I
haven't always been a direct contractor for the Mozilla Foundation, but I've had
admin privs on bmo for ages.  I purposely removed myself from the staff-related
groups when I first got admin privs here out of professional courtesy, because I
didn't have a need to know what happened there, and that way I wouldn't get
bugmail that I'd be tempted to read.

Comment 13

[Vlad Dascalu]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.319; previous revision: 1.318
done

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.17; previous revision: 1.289.2.16
done