1296831 - allow answers file to override new install defaults

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P1)

Description

[Frank Becker]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/601.7.7 (KHTML, like Gecko) Version/9.1.2 Safari/601.7.7

Steps to reproduce:

create a answer file and include the following line
$answer{'use_email_as_login'} = '1';

run ./checksetup.pl ./answers


Actual results:

you get the following output:
Initializing "Product/Component Changes" email_setting ...
Initializing "Dependency Tree Changes" email_setting ...
Deriving regex group memberships...
Login names may not contain the "@" sign unless you are setting your
login name to be identical to your email address.

and in data/params.json you find the line
   "use_email_as_login" : 0,



Expected results:

creation without an error and data/params.json with the following line
   "use_email_as_login" : 1,

Comment 1

[Frank Becker]

Attachment: patch

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8783195 [details] [diff] [review]
patch

Review of attachment 8783195 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 3

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:bugzilla/bugzilla
   30b5fd6..a8824f3  master -> master

Comment 4

[Frédéric Buclin]

Comment on attachment 8783195 [details] [diff] [review]
patch

>     if ($new_install) {
>         $param->{'or_groups'} = 1;
>-        $param->{'use_email_as_login'} = 0;
>+        $param->{'use_email_as_login'} = 0 unless exists $param->{'use_email_as_login'};
>     }

This patch is wrong and is not the correct fix. $param->{'use_email_as_login'} is always defined, even for new installations, and so this code will no longer be triggered. Please backout this patch asap. You must fix the previous if-else-end block.

Comment 5

[Frank Becker]

(In reply to Frédéric Buclin from comment #4)
> Comment on attachment 8783195 [details] [diff] [review]
> patch
> 
> >     if ($new_install) {
> >         $param->{'or_groups'} = 1;
> >-        $param->{'use_email_as_login'} = 0;
> >+        $param->{'use_email_as_login'} = 0 unless exists $param->{'use_email_as_login'};
> >     }
> 
> This patch is wrong and is not the correct fix.
> $param->{'use_email_as_login'} is always defined, even for new
> installations, and so this code will no longer be triggered. Please backout
> this patch asap. You must fix the previous if-else-end block.

I do not see how I can fix this in the previous if-else-end block.
For a $new_install we always overwrite the settings from the previous if-else-end block.

Comment 6

[Dylan Hardison [:dylan] (he/him)]

I guess the effective behavior is = 1 at the moment. I'm not opposed to it defauling to 1 anyway, as the other situation is a regression on the behavior of every released version of bugzilla. Many sites may want login names that are not emails, but that should be opt in (even for $new_install, as that ends up being true in more modern deployment patterns).

Comment 7

[Dylan Hardison [:dylan] (he/him)]

is my supposition about $new_install being effectively wrong correct, Frank?

Comment 8

[Frank Becker]

Attachment: patch V3

(In reply to Dylan William Hardison [:dylan] from comment #7)
> is my supposition about $new_install being effectively wrong correct, Frank?

Yes, I did some more research and now think that we can fix this with a change in Bugzilla/Config/Auth.pm (line 91 to 96)

sub get_param_list defines   {
   name => 'use_email_as_login',
   type => 'b',
   default => '1',
   onchange => \&change_use_email_as_login
  },

but for new installations we change this in Bugzilla/Config.pm to '0'.
Why we not change Bugzilla/Auth.pm line 94 from
   default => '1',
to
   default => '0',

Thoughts?

Comment 9

[Gervase Markham [:gerv]]

(In reply to Frank Becker from comment #0)
> Steps to reproduce:
> 
> create a answer file and include the following line
> $answer{'use_email_as_login'} = '1';

Just to check: what makes you think this should work? Is it supposed to be possible to put any pref into this file and have it work?

The default value of use_email_as_login is 1 because that's what it should be for upgrades. But there is code in Bugzilla/Config.pm to set use_email_as_login to 0 for new installs:

    if ($new_install) {
        $param->{'or_groups'} = 1;
        $param->{'use_email_as_login'} = 0;
    }

Is that perhaps the code you are looking for?

Gerv

Comment 10

[Frédéric Buclin]

(In reply to Frank Becker from comment #8)
> Yes, I did some more research and now think that we can fix this with a
> change in Bugzilla/Config/Auth.pm (line 91 to 96)

No, this code must be left alone. As gerv said in comment 9, on upgrades we want email = login because that's the current behavior of older installations, and we don't want to suddenly change a rule which didn't exist before the upgrade. But for *new* installations, we want the email != login feature enabled by default.

As I said in comment 4, the right fix is to move

    if ($new_install) {
        $param->{'or_groups'} = 1;
        $param->{'use_email_as_login'} = 0;
    }


inside the previous if-else-end block. This block becomes the 2nd ELSIF block:

+       my %new_defaults = (or_groups => 1, use_email_as_login => 0);

        foreach (...) {
            if (exists $new_params{$name}) {
                $param->{$name} = $new_params{$name};
            }
            elsif (exists $answer->{$name}) {
                $param->{$name} = $answer->{$name};
            }
+           elsif ($new_install and defined $new_defaults{$name}) {
+               $param->{$name} = $new_defaults{$name};
+           }
            else {
                $param->{$name} = $item->{'default'};
            }
        }

-       if ($new_install) {
-           $param->{'or_groups'} = 1;
-           $param->{'use_email_as_login'} = 0;
-       }

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Patch backed out. 

To github.com:bugzilla/bugzilla.git
   a8824f3..3987a28  master -> master

I would still find it acceptable to not turn on this feature by default for anyone. I don't think Frank is working on a new install and I suspect more people will be tripped up by this behavior.

Comment 12

[Gervase Markham [:gerv]]

I think we definitely want this feature on by default for new installs. It's a significant privacy improvement.

Gerv

Comment 13

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #11)
> I don't think Frank is working on a new install

He is, else the code in |if $new_install| wouldn't be triggered.

Comment 14

[Frank Becker]

Attachment: patch V4

This patch has the changes from comment #10

(In reply to Gervase Markham [:gerv] from comment #9)
> (In reply to Frank Becker from comment #0)
> > Steps to reproduce:
> > 
> > create a answer file and include the following line
> > $answer{'use_email_as_login'} = '1';
> 
> Just to check: what makes you think this should work? Is it supposed to be
> possible to put any pref into this file and have it work?
> 
> The default value of use_email_as_login is 1 because that's what it should
> be for upgrades. But there is code in Bugzilla/Config.pm to set
> use_email_as_login to 0 for new installs:
> 
>     if ($new_install) {
>         $param->{'or_groups'} = 1;
>         $param->{'use_email_as_login'} = 0;
>     }
> 
> Is that perhaps the code you are looking for?
> 
> Gerd
Yes when you setup an new install you always get use_email_as_login = 0.
When you add
   $answer{'use_email_as_login'} = '1';
that did not change.

(In reply to Frédéric Buclin from comment #13)
> (In reply to Dylan William Hardison [:dylan] from comment #11)
> > I don't think Frank is working on a new install
> 
> He is, else the code in |if $new_install| wouldn't be triggered.
Yes it is a new install. I use http://git.eclipse.org/c/mylyn/org.eclipse.mylyn.tasks.git/tree/org.eclipse.mylyn.bugzilla.releng to setup the vagrant mylyn test instances or the instances at http://mylyn.org

Comment 15

[Gervase Markham [:gerv]]

(In reply to Gervase Markham [:gerv] from comment #9)
> (In reply to Frank Becker from comment #0)
> > Steps to reproduce:
> > 
> > create a answer file and include the following line
> > $answer{'use_email_as_login'} = '1';
> 
> Just to check: what makes you think this should work? Is it supposed to be
> possible to put any pref into this file and have it work?

Still looking for an answer to this question, if only to enlighten me about how this undocumented part of our install system works...

Gerv

Comment 16

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #15)
> > Just to check: what makes you think this should work? Is it supposed to be
> > possible to put any pref into this file and have it work?
> 
> Still looking for an answer to this question, if only to enlighten me about
> how this undocumented part of our install system works...

This is documented here:

https://www.bugzilla.org/docs/5.0/en/html/integrating/api/checksetup.html#RUNNING_CHECKSETUP_NON-INTERACTIVELY

The relevant part is:

"Any localconfig variable or parameter can be specified as above."

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8784018 [details] [diff] [review]
patch V4

Review of attachment 8784018 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

this also lets the answers file override or_groups being default. My suggested fix would have been:

$param->{'use_email_as_login'} = 0 unless exists $answer->{'use_email_as_login'};

which wouldn't have fixed the other (unreported) problem.

Comment 18

[Dylan Hardison [:dylan] (he/him)]

To github.com:bugzilla/bugzilla.git
   4da9ab1..0ee7422  master -> master