425663 - Separate login_name and email into separate database fields

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Marc Schumann [:Wurblzap]]

Attachment: Patch

This is the first step towards fixing bug 218917.

The patch creates the database field profiles.email and makes sure it stays in sync with login_name. A new parameter use_email_as_login is introduced, but stays fixed switched on as for now.

Comment 1

[Frédéric Buclin]

What are these $dbh->quote('0')?? And do they do in this patch?

Comment 2

[Marc Schumann [:Wurblzap]]

This is because bz_add_column has been modified so that it doesn't do the $dbh->quote itself anymore, and callers need to do it themselves, if they need to. Removing $dbh->quote grants more power to bz_add_column, which I use: when adding the new email column to the profiles table, the initial value is not a fixed value, but that of the login_name column.

Comment 3

[Max Kanat-Alexander]

3.2 is not open for enhancements like this one.

Comment 4

[Frédéric Buclin]

Max, do you agree with the change made in Bugzilla/DB.pm (and consequently in Install/DB.pm)? I'm not a fan of this change.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 312261 [details] [diff] [review]
Patch

There's various things I'm not fond of about the patch, but I don't have time for a full review. If there's some way to avoid that DB.pm change, that would be nice, yes.

Comment 6

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.0.1

Behaviour of bz_add_column stays unchanged now, so its callers don't need to be changed.

Comment 7

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.0.2

Removed the testing tool, as Frédéric asked for, so that un-marking the new parameter as fixed is not sufficient any more. Testing now involves removing the corresponding hacks in template/en/default/admin/params/common.html.tmpl and editparams.cgi.

Comment 8

[Frédéric Buclin]

Comment on attachment 321635 [details] [diff] [review]
Patch 1.0.2

>Index: Bugzilla/Config.pm

>@@ -201,6 +209,9 @@ sub update_params {

>+    # XXX Next line to be activated with last patch of bug 218917
>+    #$param->{'use_email_as_login'} = 0 if $new_install;

I'm not sure that's the right place to put this line, nor that 0 should be the default. We never set parameters this way AFAIK.



>Index: Bugzilla/DB.pm

>+sub bz_add_column {
>+    my ($self, $table, $name, $new_def, $init_value) = @_;
>+
>+    return $self->bz_add_column_raw($table, $name, $new_def,
>+                                    $self->quote($init_value));

This won't work AFAIK. If $init_value is undefined, $dbh->quote($init_value) returns NULL, not undef, and so bz_add_column_raw() will always see this argument as defined.



>Index: Bugzilla/User.pm

>@@ -109,6 +112,7 @@ sub UPDATE_COLUMNS {
>+        email

Shouldn't it be added to REQUIRED_CREATE_FIELDS too?



>Index: Bugzilla/Config/Common.pm

>+sub change_handler_use_email_as_login {
>+    my $option = shift;
>+    if ($option eq '1') {
>+        Bugzilla->dbh->do('UPDATE profiles SET login_name = email');
>+    }

What happens if editparams.cgi throws an error on a subsequent parameter? This SQL call will happen without the use_email_as_login parameter to be updated, isn't it? This would be a pretty common case.



>Index: Bugzilla/Install/DB.pm

>     $dbh->bz_add_column('attachments', 'submitter_id',
>-                        {TYPE => 'INT3', NOTNULL => 1}, 0); 
>+                        {TYPE => 'INT3', NOTNULL => 1}, 0);

Nit: if the only change is about the trailing whitespace, then this shouldn't be included in this patch.



>Index: template/en/default/admin/params/common.html.tmpl

>+    [%# XXX Next line is transitionary code during development
>+      #     for bug 218917, to be removed afterwards %]
>+    [% NEXT IF param.name == 'use_email_as_login' %]

Nit: I think it doesn't hurt to have it in the UI but having no effect. This would be one less file edited.

On the updated patch, please ask mkanat for review as well as I want his opinion on this bug/implementation.

Comment 9

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.0.3

All right, I mananged to come up with a new patch.

(In reply to comment #8)
> I'm not sure that's the right place to put this line, nor that 0 should be the
> default. We never set parameters this way AFAIK.

Yes we do, if the new parameter's value depends on whether it's a new install or not. We initialize the utf8 parameter this way.

> >Index: Bugzilla/DB.pm
> 
> >+sub bz_add_column {
> >+    my ($self, $table, $name, $new_def, $init_value) = @_;
> >+
> >+    return $self->bz_add_column_raw($table, $name, $new_def,
> >+                                    $self->quote($init_value));
> 
> This won't work AFAIK. If $init_value is undefined, $dbh->quote($init_value)
> returns NULL, not undef, and so bz_add_column_raw() will always see this
> argument as defined.

Fixed.

> >Index: Bugzilla/User.pm
> 
> >@@ -109,6 +112,7 @@ sub UPDATE_COLUMNS {
> >+        email
> 
> Shouldn't it be added to REQUIRED_CREATE_FIELDS too?

Yes. Thanks.

> >Index: Bugzilla/Config/Common.pm
> 
> >+sub change_handler_use_email_as_login {
> >+    my $option = shift;
> >+    if ($option eq '1') {
> >+        Bugzilla->dbh->do('UPDATE profiles SET login_name = email');
> >+    }
> 
> What happens if editparams.cgi throws an error on a subsequent parameter? This
> SQL call will happen without the use_email_as_login parameter to be updated,
> isn't it? This would be a pretty common case.

I see. I moved change handler code after all param checks and before write_params, so that handler code runs only if all checks have succeeded.

> >Index: Bugzilla/Install/DB.pm
> 
> >     $dbh->bz_add_column('attachments', 'submitter_id',
> >-                        {TYPE => 'INT3', NOTNULL => 1}, 0); 
> >+                        {TYPE => 'INT3', NOTNULL => 1}, 0);
> 
> Nit: if the only change is about the trailing whitespace, then this shouldn't
> be included in this patch.

True. Reverted.

> >Index: template/en/default/admin/params/common.html.tmpl
> 
> >+    [%# XXX Next line is transitionary code during development
> >+      #     for bug 218917, to be removed afterwards %]
> >+    [% NEXT IF param.name == 'use_email_as_login' %]
> 
> Nit: I think it doesn't hurt to have it in the UI but having no effect. This
> would be one less file edited.

Removed.

Comment 10

[Frédéric Buclin]

Comment on attachment 335201 [details] [diff] [review]
Patch 1.0.3

>Index: editparams.cgi

>+        # XXX Next line is transitionary code during development
>+        #     for bug 218917, to be removed afterwards
>+        next if $name eq 'use_email_as_login';

Why is this line required? You don't care if it's already set or not.


>+    call_param_change_handlers(@changes);

IMO, would be better to pass by reference.



>Index: Bugzilla/Config.pm

>+sub call_param_change_handlers {
>+            $entry->{'change_handler'}->(Bugzilla->params->{$name}, $entry);

Why passing $entry? And why passing Bugzilla->params->{$name} as the function being called can already call Bugzilla->params itself?


>+=item C<call_param_change_handlers(@changes)>
>+
>+Expects a list of parameter names, and considers them a list of parameters

Is it me or is there a missing word somewhere in this sentence? I don't understand its meaning.



>Index: Bugzilla/User.pm

> use constant VALIDATORS => {

You need a validator for the email.


> sub set_name {

>+    ${$_[0]}{email} = ${$_[0]}{login_name};

This is really unreadable as is. $_[0]->{email} is really easier to read. Also, this won't work if $_[0] is undefined.



>Index: Bugzilla/Auth/Verify.pm

>-        $user->set_login($username);
>+        $user->set_email($username);

Is it a permanent change?



>Index: Bugzilla/Config/Common.pm

>+sub change_handler_use_email_as_login {
>+    if ($option eq '1') {

if ($option == 1) or if ($option) would make more sense. You treat an integer as a string here.



>Index: Bugzilla/Install/DB.pm

>+    # 2008-08-23 [e-mail] - Bug 425663
>+    $dbh->bz_add_column_raw('profiles', 'email',
>+                            {TYPE => 'varchar(255)', NOTNULL => 1},
>+                            'login_name');
>+    $dbh->bz_add_index('profiles', 'profiles_email_idx',
>+                       {TYPE => 'UNIQUE', FIELDS => ['email']});

Tiny bitrot here. Trivial to fix.


This *looks* good, otherwise. But I still want mkanat's review on this. Also, I have a timing problem. I fear the full implementation won't be ready on time for 3.4, which makes me think we should either create a branch to land patches when they are ready (the annoying part being bitrots when merging with the trunk later) or land them all at once, or wait for the 3.4 branch before landing this first patch.

Comment 11

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.0.4

(In reply to comment #10)
> >+        # XXX Next line is transitionary code during development
> >+        #     for bug 218917, to be removed afterwards
> >+        next if $name eq 'use_email_as_login';
> 
> Why is this line required? You don't care if it's already set or not.

This prevents the param from being changed.

> >+    call_param_change_handlers(@changes);
> 
> IMO, would be better to pass by reference.

Appeasing LpSolit now.

> >Index: Bugzilla/Config.pm
> 
> >+sub call_param_change_handlers {
> >+            $entry->{'change_handler'}->(Bugzilla->params->{$name}, $entry);
> 
> Why passing $entry? And why passing Bugzilla->params->{$name} as the function
> being called can already call Bugzilla->params itself?

This is so that the interface of the change_handler callbacks is identical to the checker callbacks.

> >+=item C<call_param_change_handlers(@changes)>
> >+
> >+Expects a list of parameter names, and considers them a list of parameters
> 
> Is it me or is there a missing word somewhere in this sentence? I don't
> understand its meaning.

Reworded.

> >Index: Bugzilla/User.pm
> 
> > use constant VALIDATORS => {
> 
> You need a validator for the email.

This is coming in with stage 2 (bug 425840).

> > sub set_name {
> 
> >+    ${$_[0]}{email} = ${$_[0]}{login_name};
> 
> This is really unreadable as is. $_[0]->{email} is really easier to read. Also,
> this won't work if $_[0] is undefined.

$_[0] is always defined; this is part of sub create(), not of set_name. It's to force the email to be identical to the login_name for the time being. It goes away with stage 3 (bug 426071).

> >Index: Bugzilla/Auth/Verify.pm
> 
> >-        $user->set_login($username);
> >+        $user->set_email($username);
> 
> Is it a permanent change?

No, this is a bug :)
There was Auth code in need of patching.

> >Index: Bugzilla/Config/Common.pm
> 
> >+sub change_handler_use_email_as_login {
> >+    if ($option eq '1') {
> 
> if ($option == 1) or if ($option) would make more sense. You treat an integer
> as a string here.

It is, in fact, a string. It's coming in from a CGI param.
But I'm fine with |if ($option)|.

As a side note for a separate bug: it would actually be a good idea if it wasn't a string. An integer would be better. We should make sure all incoming param values are valid for the param type, and we could turn values of yes/no radio buttons into integers at the same time.

> >Index: Bugzilla/Install/DB.pm
> Tiny bitrot here. Trivial to fix.

Should be fixed. (I don't know what was broken.)

> have a timing problem. I fear the full implementation won't be ready on time
> for 3.4, which makes me think we should either create a branch to land patches
> when they are ready (the annoying part being bitrots when merging with the
> trunk later) or land them all at once, or wait for the 3.4 branch before
> landing this first patch.

I think it can be ready for 3.4.

Comment 12

[Frédéric Buclin]

Comment on attachment 340829 [details] [diff] [review]
Patch 1.0.4

It's hard to say if this code is correct and works as expected without its full implementation. So I will r+ it for now as I see nothing wrong. Note that I didn't test it *at all*, which means my r+ assumes you tested it heavily, especially code in DB/* and Auth/*.

I want mkanat's review before approving this bug.

Comment 13

[ravi]

Marc,   Frédéric, Max, can I contribute in any way to this bug? I am interested in these changes since it will help with some LDAP integration issues I am working on (for one thing, I would like Bugzilla when configured with LDAP settings to (at least) copy in the email address from the LDAP directory). I can help test this, comment on the code (if that is worthwhile), etc.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 340829 [details] [diff] [review]
Patch 1.0.4

I've been thinking about this, and actually, we already have a login field--extern_id. Probably instead, what needs to happen is that we rename extern_id to "login", and "login_name" to email. We could start with a patch that does just that, for simplicity's sake.

Also, I'd still like to allow people to log in with either their email or their login id. (Same with user-matching--I'd like it to match against both.)

Also, I don't want to have a parameter, I just want this feature to be on all the time.

Comment 15

[Max Kanat-Alexander]

Jochen--see my last comment. Maybe you and Marc can get together on this and use your extern_id editing code, after he's renamed the fields. Then we won't have to have extern_id_used at all, because we'll always be using it.

Comment 16

[Jérôme Bouat]

This bug is critical because it makes a security hole into the worlwide mail system.

Comment 17

[Jens Müller (:tessarakt)]

(In reply to comment #16)
> This bug is critical because it makes a security hole into the worlwide mail
> system.

It does WHAT exactly?

Comment 18

[Max Kanat-Alexander]

(In reply to comment #17)
> It does WHAT exactly?

  Please keep comments in Bugzilla focused on technical discussion of the issue at hand.

Comment 19

[Jérôme Bouat]

A spammer can easily create a bugzilla account and grab all the email addresses of all the bugzilla user.

Next he can use those email addresses in order to feed his spam robots.

Finally the worldwide mail system is compromised with undesirable messages.

Comment 20

[srareczek]

Is anyone working on this bug? I registered here just to ask for this and I would use my real mail if Bugzilla could secure it correctly. It's funny how Google Code and Launchpad can already do it fine.

Comment 21

[rojanu]

Attachment: Renamed login_name

This patch is partial, I have just renamed login_name to be email.  If nobody else is working on the bug I am planning to work.

I am thinking of
1- Add login field to User.pm
2- Rename extern_id to login and make use of User->login

Comment 22

[Max Kanat-Alexander]

Thanks for the patch, rojanu! The work on this is currently being done mostly in the blockers. We wouldn't want to rename login_name, though, BTW, we'd just want to add a new field.

I'm actually not totally convinced of the value of this enhancement, by the way, even though many people have asked for it. I think there's a particular value in seeing the domain that somebody's username is at.

Comment 23

[Mark A. Hershberger]

(In reply to comment #22)
> I'm actually not totally convinced of the value of this enhancement, by the
> way, even though many people have asked for it.

Doing my part to convince you: As the bugmeister for Wikimedia, I would like to have an extension built for MediaWiki that would let Wikimedia Users create and track bugs using their SUL account from Wikipedia.

Some of these SUL accounts don't have email addresses and, for those that do, we cannot automatically expose them.

Perhaps there is already a way that I don't know about that we could make login_names be some non-email value.  I was just searching bugs here and this bug (as well as bug 218917) was the most likely candidate.

Comment 24

[Max Kanat-Alexander]

(In reply to comment #23)
> Perhaps there is already a way that I don't know about that we could make
> login_names be some non-email value.

  Actually, at the moment you could try using extern_id. That'd require writing your own custom auth stuff, but it should be possible (you can search the Auth modules for extern_id if you want to know more).

Comment 25

[Denis Roy]

> I've been thinking about this, and actually, we already have a login
> field--extern_id. Probably instead, what needs to happen is that we rename
> extern_id to "login", and "login_name" to email.

I'm not sure if this matters, but we use extern_id to link BZ accounts with other accounts.  Renaming the field would definitely cause breakage  :)

Comment 26

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 27

[rojanu]

What is the stance on this bug now, specially on comment 24

Comment 28

[Frédéric Buclin]

Fixed in Bugzilla 5.1 in bug 218917.