275477 - Add the ability to make a user an Administrator to checksetup

Status: RESOLVED DUPLICATE of bug 351983

(Bugzilla :: Installation & Upgrading, enhancement)

Description

[:glob ✱]

if you forget the administrator password it's tricky for a non-developer to
reset/recover the password if the associated email address is no longer functional.

as checksetup already knows how to create an administrator, it shouldn't be
difficult to add a switch to checksetup to always prompt for the administrator
email address.

Comment 1

[:glob ✱]

Attachment: add --reset-admin v1

Comment 2

[Marc Schumann [:Wurblzap]]

Comment on attachment 169257 [details] [diff] [review]
add --reset-admin v1

Since bug 264160 landed, this doesn't apply any more.

Comment 3

[GavinS]

There is now a --help parameter to checksetup.pl, so could any updated patch on
this bug add this new param to the --help output?

Comment 4

[:glob ✱]

Attachment: add --reset-admin v2

fixes bitrot, adds entry to help.

Comment 5

[Frédéric Buclin]

Comment on attachment 174024 [details] [diff] [review]
add --reset-admin v2

REPLACE INTO is not DB-cross compatible, see bug 190432. Use INSERT INTO and
UPDATE instead.

Comment 6

[:glob ✱]

*** Bug 283266 has been marked as a duplicate of this bug. ***

Comment 7

[:glob ✱]

Attachment: add --reset-admin v3

Comment 8

[Marc Schumann [:Wurblzap]]

Comment on attachment 175263 [details] [diff] [review]
add --reset-admin v3

>+if (!$resetadmin) {
>+    $sth = $dbh->prepare("select id from groups");
>+    $sth->execute();
>+    while ( my @row = $sth->fetchrow_array() ) {
>+        push (@groups, $row[0]);
>+    }

I'm thinking filling @groups shouldn't be conditional on !$resetadmin, should
it? This causes groups not to be reset on a --reset-admin run, and I think they
should be. Is this intentional?

>+    $sth = $dbh->prepare("SELECT user_id FROM user_group_map WHERE user_id=?
>+        AND group_id=? AND isbless=? AND grant_type=?");
>+
>+    $sth->execute($userid, $admingroupid, 0, GRANT_DIRECT);
>+    if ($sth->rows == 0) {
>+        $dbh->do("INSERT INTO user_group_map 
>+            (user_id, group_id, isbless, grant_type) 
>+            VALUES ($userid, $admingroupid, 0, " . GRANT_DIRECT . ")");
>+    }
>+
>+    $sth->execute($userid, $admingroupid, 1, GRANT_DIRECT);
>+    if ($sth->rows == 0) {
>+        $dbh->do("INSERT INTO user_group_map 
>+            (user_id, group_id, isbless, grant_type) 
>+            VALUES ($userid, $admingroupid, 1, " . GRANT_DIRECT . ")");
>+    }
> 
>     # Admins get inherited membership and bless capability for all groups
>     foreach my $group ( @groups ) {
>+        $sth = $dbh->prepare("SELECT member_id FROM group_group_map WHERE
>+            member_id=? AND grantor_id=? AND grant_type=?");
>+
>+        $sth->execute($admingroupid, $group, GROUP_MEMBERSHIP);
>+        if ($sth->rows == 0) {
>+            $dbh->do("INSERT INTO group_group_map
>+                (member_id, grantor_id, grant_type)
>+                VALUES ($admingroupid, $group, " . GROUP_MEMBERSHIP . ")");
>+        }
>+
>+        $sth->execute($admingroupid, $group, GROUP_BLESS);
>+        if ($sth->rows == 0) {
>+            $dbh->do("INSERT INTO group_group_map
>+                (member_id, grantor_id, grant_type)
>+                VALUES ($admingroupid, $group, " . GROUP_BLESS . ")");
>+        }

I like that you're using statement handles here. Please move the prepare()
inside the loop out of it to take full advantage of it, though. (For bonus
points, you could convert the do()s of the original to execute()s on a prepared
statement handle.)

Comment 9

[:glob ✱]

Attachment: add --reset-admin v4

> I'm thinking filling @groups shouldn't be conditional on !$resetadmin

oops

> I like that you're using statement handles here. Please move the prepare()
> inside the loop out of it to take full advantage of it, though.

*slaps head*  i meant to do that

Comment 10

[Max Kanat-Alexander]

Comment on attachment 175386 [details] [diff] [review]
add --reset-admin v4

>+    print "   --reset-admin    Interactively create a new Administrator or reset\n";
>+    print "                    and existing Administrator's password.\n";

  Typo.

>+my $resetadmin = grep(/^--reset-admin$/, @ARGV);
>+my $noadmin = 0;

  Use the %switch hash that I created for --no-templates, instead of a custom
variable.

>+if (!$resetadmin) {
>+    $sth = $dbh->prepare("SELECT user_id FROM groups, user_group_map" .
>+                        " WHERE name = 'admin' AND id = group_id");
>+    $sth->execute;
>+    $noadmin = $sth->rows == 0;

  Hrm, I think it'd be cleaner to selectrow_array a COUNT(*) and make that
$noadmin.

>+  if ($noadmin) {
>+    print "\nLooks like we don't have an administrator set up yet.  Either this is your\n";
>+    print "first time using Bugzilla, or your administrator's privileges might have accidently\n";
>+    print "been deleted.\n";

  Nit: I think at least one of those lines is longer than 80 chars.

>+
>+    $sth = $dbh->prepare("SELECT user_id FROM user_group_map WHERE user_id=?
>+        AND group_id=? AND isbless=? AND grant_type=?");
>+
>+    $sth->execute($userid, $admingroupid, 0, GRANT_DIRECT);
>+    if ($sth->rows == 0) {
[snip]

  That section and the next look like part of a different patch...? Or is it
just that you're re-granting things to the admins? Just a brief explanation
would help. :-)

  Also, this overall looks more like "--new-admin" than "--reset-admin." It
doesn't actually *reset* anybody's password, does it?

Comment 11

[Marc Schumann [:Wurblzap]]

Reviewing the review :)

(In reply to comment #10)
> >+my $resetadmin = grep(/^--reset-admin$/, @ARGV);
> >+my $noadmin = 0;
> 
>   Use the %switch hash that I created for --no-templates, instead of a custom
> variable.

I agree.

> >+if (!$resetadmin) {
> >+    $sth = $dbh->prepare("SELECT user_id FROM groups, user_group_map" .
> >+                        " WHERE name = 'admin' AND id = group_id");
> >+    $sth->execute;
> >+    $noadmin = $sth->rows == 0;
> 
>   Hrm, I think it'd be cleaner to selectrow_array a COUNT(*) and make that
> $noadmin.

Old code -- may be kept. But don't hesitate to nicify it in your patch, if you
like :)

> >+  if ($noadmin) {
> >+    print "\nLooks like we don't have an administrator set up yet.  Either
this is your\n";
> >+    print "first time using Bugzilla, or your administrator's privileges
might have accidently\n";
> >+    print "been deleted.\n";
> 
>   Nit: I think at least one of those lines is longer than 80 chars.

Same here.

>   That section and the next look like part of a different patch...? Or is it
> just that you're re-granting things to the admins? Just a brief explanation
> would help. :-)

That part is actually part of the point of the patch -- it makes group
memberships being set in a non-borking way :)

>   Also, this overall looks more like "--new-admin" than "--reset-admin." It
> doesn't actually *reset* anybody's password, does it?

Indeed... You'll probably need to set $admin_ok to 0 after the "Looks like we
don't have an administrator set up yet" message if $resetadmin and move the
password entering part out of the if($admin_create) block.

Comment 12

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 13

[Max Kanat-Alexander]

Okay, I'm changing the subject of this bug to reflect what it actually became, and then duping it to bug 351983 because I ended up handling this there.

*** This bug has been marked as a duplicate of 351983 ***