allow answers file to override new install defaults

RESOLVED FIXED in Bugzilla 6.0

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Frank, Assigned: Frank)

Tracking

5.1.1
Bugzilla 6.0

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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,
Assignee: installation → dylan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → Bugzilla 6.0
(Assignee)

Comment 1

2 years ago
Created attachment 8783195 [details] [diff] [review]
patch
Attachment #8783195 - Flags: review?(gerv)
Assignee: dylan → Frank
Comment on attachment 8783195 [details] [diff] [review]
patch

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

r=dylan
Attachment #8783195 - Flags: review?(gerv) → review+
To git@github.com:bugzilla/bugzilla
   30b5fd6..a8824f3  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
(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.
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).
Status: RESOLVED → REOPENED
Flags: needinfo?(Frank)
Resolution: FIXED → ---
is my supposition about $new_install being effectively wrong correct, Frank?
(Assignee)

Comment 8

2 years ago
Created attachment 8783794 [details] [diff] [review]
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?
Flags: needinfo?(Frank)
Attachment #8783794 - Flags: review?
(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

2 years ago
(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;
-       }
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.
I think we definitely want this feature on by default for new installs. It's a significant privacy improvement.

Gerv

Comment 13

2 years ago
(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.
(Assignee)

Comment 14

2 years ago
Created attachment 8784018 [details] [diff] [review]
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
Attachment #8783794 - Attachment is obsolete: true
Attachment #8783794 - Flags: review?
Attachment #8784018 - Flags: review?
(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

2 years ago
(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 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.
Attachment #8784018 - Flags: review? → review+
Summary: answer use_email_as_login not used → allow answers file to override new install defaults
To github.com:bugzilla/bugzilla.git
   4da9ab1..0ee7422  master -> master
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Attachment #8783195 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.