31456 - Editing a superuser clears their access flags

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Dave Miller [:justdave]]

If you edit the superuser account (say to change the real name or password) from 
edituser.cgi, it clears any group bits that aren't defined on the editing form, 
thus taking away the superuser status.

Comment 1

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 2

[Dave Miller [:justdave]]

Since this has now been out there so long like this, when this is fixed, 
checksetup.pl should be modified to prompt the operator for the address of the 
maintainer and set him up with maintainer access again if there is no one in the 
database with maintainer access.

Comment 3

[Joe Robins]

Dave: This proposed solution would not be sufficient in all cases.  For example, 
at my site, we have two superusers, and if one of us loses superuser priveleges, 
but not the other one, then this fix would not kick in.

I think it would be reasonable to make it so that editusers.cgi could not change 
the group permissions of any superusers.  It'd also be pretty easy to do, and 
I'd be willing to do it.  Let me know if you think this is a good or bad 
solution.

(If your objection to this is that people might be used to it being the old way, 
we could make it an operating parameter whether to use the new or the old 
behavior.  That'd still be fairly simple to do.)

Comment 4

[Dave Miller [:justdave]]

Fixing editusers.cgi so that it wouldn't allow you to edit the superusers was my 
main idea, yes.  The checksetup thing I mentioned would just be an attempt to 
correct damage already done by having this out there broken for so long.  It 
wouldn't catch all circumstances, but would make sure there was at least one 
maintainer...

If you'd like to take a crack at it, by all means. :)

Comment 5

[Joe Robins]

Oh, OK.  I was misunderstanding what you were saying, I think.  That makes much 
more sense.  I won't bother with the operating parameter, then, which didn't 
really seem worth it to me anyway.  :-)

I'll make and check the changes tonight or tomorrow night.  However, I've got to 
sheepishly ask a quick question first:  Once I make the changes, what's the 
accepted way of posting a patch here?  I assume a cvs diff of some sort, and 
post the diff?  Do I want a diff -c, or some other kind?  This'll be my first 
time checking in changes by attaching them to a bug, so I'm not sure what the 
procedure is.

Comment 6

[Dave Miller [:justdave]]

cvs diff -u [<filename> ...]

then attach the results.

Comment 7

[Joe Robins]

Attachment: Bugzilla patch to prevent editusers.cgi from removing superuser access

Comment 8

[Joe Robins]

The above patch contains modifications to three files:

1) globals.pl: Added a global variable with the value of the superuser groupset, 
$superusergroupset.

2) editusers.cgi: If the user being edited is a superuser, doesn't change the 
groupset for that user, and displays the message "Cannot change permissions of 
superuser."  Otherwise, it functions as before.

3) checksetup.pl: Added a parameter for localconfig called "defaultsuperuser".  
If it's not set, then there is no functional change.  If it is set, then if that 
user exists in the database when checksetup.pl is run, that user is given 
superuser permissions.

Let me know if you think this looks good, or if there's a problem.

Comment 9

[Dave Miller [:justdave]]

Since you yourself mentioned that your site has two superusers, I would make the 

"defaultsuperuser" variable in localconfig be an array, and walk through all of 

them in your routine to put the privs back.



@defaultsuperuser=();



and put an example in the inserted comment to point out that you need a list 

between the (), and why it's a list instead of a scalar :)

Comment 10

[Joe Robins]

That's a good point, which I hadn't thought of.  Unfortunately, it doesn't quite 
work.  :-(

Perl considers a variable defined by '$foo = "";' to be defined, and an array 
with any values to be defined, but an empty array, defined by '@foo = ();' is 
considered undefined.  An array is only defined once it has memory allocated to 
it, which, as far as I can tell, only happens when you put something in it.  In 
checksetup.pl, LocalVar checks whether the variable or array is defined, having 
executed localconfig already, and if not, adds it to localconfig and conks out.  
Since the default setting of '@defaultsuperuser=();' doesn't define it, if you 
don't set any default superusers, it'll never work.  It will just keep appending 
the text for @defaultsuperuser to the end of localconfig.

The only kludge I can even think of is to set a default value of 
'@defaultsuperuser = (1); pop @defaultsuperuser', or some such, but that's 
really, really bad, and I don't want to use it.

Is there a clean way to have @defaultsuperuser be defined even when it's empty, 
so that this will work?  Or will I have to stick with a single string, and 
either limit it to one default superuser or do some kludging in checksetup.pl to 
separate them from the string?

Comment 11

[Dave Miller [:justdave]]

Here you go:

    my $defined = 1;
    $_ = "$name;\n1;";
    $SIG{__WARN__} = sub { };   # don't complain to user
    eval $_ or $defined = 0;
    $SIG{__WARN__} = 'DEFAULT';

if the variable really is defined, but is empty, the eval will successfully 
execute.  If the variable is not defined at all, the eval will fail to compile 
(and thus be undefined, causing our $defined=0 to exexute).  Since the failure to 
compile also throws a warning, we're quieting the warnings around the eval.

Note that for this to work, we need to remove the first --LOCAL-- block at the 
top, that was designed to stop the above warnings. :)

#
# This are the --LOCAL-- variables defined in 'localconfig'
# 

use vars qw(
    $webservergroup
    $db_host $db_port $db_name $db_user $db_pass
);

Not everything that should have been got defined here anyway.

Comment 12

[Dave Miller [:justdave]]

Attachment: Patch against the current checksetup.pl to properly check for localconfig declarations

Comment 13

[Dave Miller [:justdave]]

OK, after having some time to play around with it and test it, the attachment I 
just posted is a little more robust than what I quoted above.  Try it out and see 
if it works for you.

Comment 14

[Tara Hernandez]

Can you throw this up on landfill?

Comment 15

[Dave Miller [:justdave]]

Hey Joe, have you done anything else with this yet?

Comment 16

[Chris Yeh]

I checked in the front half of the fixes (globals.pl, editusers.cgi) but didn't 
checkin the change to checksetup.pl because I got tossed some perl errors that I 
don't have anymore and don't have time to go back and look at. 

Comment 17

[Tara Hernandez]

We've come full circle.  Dave--it's all you man.

Comment 18

[Dave Miller [:justdave]]

I've found a much better way to do this since then anyway...  (which is why Tara 
gave this back to me).  Have to do some travelling this weekend, but when I get 
back next week, I'll be throwing together a revised patch which does the 
incredibly obvious (if you knew it was there) and just looks in the symbol table 
to see if it's been declared. :-)

Comment 19

[Tara Hernandez]

Marking P1 as we'll hold 2.12 for this.

Comment 20

[Dave Miller [:justdave]]

OK...  

Been playing around with this, and I have come to the conclusion that to do this 
"right", we need to do one of the following:

a) the check for the variables being declared is done in a different file than 
the checks that actually use the data that was read in from localconfig...  this 
would just mean taking everything after the --LOCAL-- section in checksetup.pl 
and placing it into a second file which would then get required in after it was 
done checking for the variables in localconfig. That way the variables it's using 
don't get declared at compile time (thus screwing up the test) just because 
they're used later in the file.

-or-

b) I can just dereference the values in the symbol table instead of using the 
variable names when we need to access them later in the file.  This would 
probably be the least destructive of the two.  Basically, instead of having 
$::db_check, you would have ${*{$main::{'db_check'}}{SCALAR}}.  Now that's just 
plain nasty...  but it keeps the variable from being declared at compile time 
just because you used it later in the file.  Of course, I could create new 
variables with different prefixes to use inside the file later on, and make an 
assignment to those with constructs like the above to copy the values in from 
localconfig.   As hacky as this looks, this is the documented way for doing such 
things according to O'Reilly's Programming Perl book, and is less of a hack than 
the way it's being done now...

Comment 21

[Dave Miller [:justdave]]

OK, I went with choice b).  Here's a patch.  Adding some CCs because this one is 
a bit of a whopper and needs some review.

Comment 22

[Dave Miller [:justdave]]

Attachment: New patch for checksetup.pl to check for localconfig vars. This one does it the "right" way.

Comment 23

[Dave Miller [:justdave]]

OK, the patch for this one is here, and has been here since mid-December.  Just
needs someone besides me (since I wrote it) to review it before it can be
checked in...

Comment 24

[Dave Miller [:justdave]]

ping Joe Robins....

The initial patch on this bug is WAY stale....

also, I don't think it works at all if you have more than one user in the
database already.

And if we have default superusers defined already, why not insert them in the
database if they don't exist?  :)

I'm off to dinner, will play more later tonight.

Comment 25

[Dave Miller [:justdave]]

Hmm, I think we let this bug get a little out of hand...  looking over it again, 
it's drifted to something completely different than what it started out as...

The original bug (editing a superuser clears their access flags) has been fixed 
and checked in, but this is still open because other stuff that came up here 
hasn't been dealt with yet.

The one piece of the original patch here that hadn't been checked in (the 
localconfig var for the default superuser) I'm not sure is necessary now.  I 
think the better way to deal with this is to have sanitycheck.cgi make sure at 
least one superuser exists and complain if there isn't one.  checksetup.pl could 
possibly also make the same check, possibly tied in with bug 17773, which I think 
is a Really Cool Idea.  I would like to see this bug closed out, after the last 
patch here for checksetup.pl is checked in (no r= yet...  come on guys!) and 
we'll forgo the superuser check in favor of including it in bug 17773. :)

Comment 26

[Tara Hernandez]

Dave -- The first chunk of this patch is now stale -- probably due to one of the
last 3 checkins to checksetup in the last 2 days.  Wanna check it out?

Comment 27

[Dave Miller [:justdave]]

Attachment: Revised patch against current cvs

Comment 28

[Dave Miller [:justdave]]

r=tara in IRC

Checked in.

Comment 29

[Tara Hernandez]

Sorry for the spam, but I needed to be able to query for all of these correctly.

Comment 30

[Dave Miller [:justdave]]

Moving closed bugs to Bugzilla product