Editing a superuser clears their access flags

RESOLVED FIXED in Bugzilla 2.12

Status

()

Bugzilla
Bugzilla-General
P1
normal
RESOLVED FIXED
18 years ago
5 years ago

People

(Reporter: justdave, Assigned: justdave)

Tracking

unspecified
Bugzilla 2.12

Details

(Whiteboard: 2.12)

Attachments

(4 attachments)

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

18 years ago
tara@tequilarista.org 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 .)
Assignee: terry → tara
Blocks: 43613
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

18 years ago
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.)
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

18 years ago
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.
cvs diff -u [<filename> ...]

then attach the results.

Comment 7

18 years ago
Created attachment 11009 [details] [diff] [review]
Bugzilla patch to prevent editusers.cgi from removing superuser access

Comment 8

18 years ago
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.
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

18 years ago
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?
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.
Created attachment 11061 [details] [diff] [review]
Patch against the current checksetup.pl to properly check for localconfig declarations
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

18 years ago
Can you throw this up on landfill?
Assignee: tara → cyeh
Hey Joe, have you done anything else with this yet?

Updated

18 years ago
Status: NEW → ASSIGNED

Updated

18 years ago
Whiteboard: 2.12

Comment 16

18 years ago
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

17 years ago
We've come full circle.  Dave--it's all you man.
Assignee: cyeh → dave
Status: ASSIGNED → NEW
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. :-)
Status: NEW → ASSIGNED

Comment 19

17 years ago
Marking P1 as we'll hold 2.12 for this.
Priority: P3 → P1
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...
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.
Created attachment 20942 [details] [diff] [review]
New patch for checksetup.pl to check for localconfig vars.  This one does it the "right" way.
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...
Keywords: patch, review
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.
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

17 years ago
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?
Created attachment 23406 [details] [diff] [review]
Revised patch against current cvs
r=tara in IRC

Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 29

17 years ago
Sorry for the spam, but I needed to be able to query for all of these correctly.
Target Milestone: --- → Bugzilla 2.12
Moving closed bugs to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.