355596 - Your password should be requested to confirm your email address change

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement, P3)

Description

[Frédéric Buclin]

Currently, when you change your email address, the confirmation token sent to the new email address requires you to enter your old email address, which is not too hard to guess as this one is given in the email. In the case where the user entered a wrong new email address, an ill-intentioned person may get this email by error, and decides to use the URL given in the email to steal this user account. As he has no idea what the password is, all this bad guy has to do is to request a new one, and Bugzilla will kindly send him an email with a token in it to change it.

When using the confirmation token, Bugzilla should request the password instead of the old email address. As the user had to type his password to change his email address, this means he remembers what his password is and so we cannot fall in the testcase where the user would have changed his email address but would have forgot what his password is. This would make the email address change much more secure.

Comment 1

[Frédéric Buclin]

To be sure: is there an auth method which lets you have an account in Bugzilla but which doesn't require a password? I think LDAP is one of them. In that case, requesting the password won't work. :-/

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> To be sure: is there an auth method which lets you have an account in Bugzilla
> but which doesn't require a password?

  Not currently. OpenID will be that way, though.

Comment 3

[Frédéric Buclin]

Is the LDAP password really stored in Bugzilla? Looking at bug 392311 comment 1, it seems it's not.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> Is the LDAP password really stored in Bugzilla? Looking at bug 392311 comment
> 1, it seems it's not.

  It's not, but that doesn't matter, because you can still verify it.

Comment 6

[Koosha KM]

Attachment: V1 - Password and old email required.

Should we really remove the "Old Email Address" field?

Comment 7

[Frédéric Buclin]

Comment on attachment 617523 [details] [diff] [review]
V1 - Password and old email required.

=== modified file 'template/en/default/account/email/confirm.html.tmpl'

>+      <th align="right"><label for="email">Old Email Address:</label></th>
>+      <td><input type="text" name="email" id="email" size="36"></td>

There is no need to request both the old email address and the password. Only request the password.


>+  [% ELSIF error == "password_incorrect" %]
>+    [% title = "Incorrect Password" %]
>+    You did not enter your password correctly.
> 
>   [% ELSIF error == "old_password_incorrect" %]
>     [% title = "Incorrect Old Password" %]


We don't need to duplicate the existing error message as we already have old_password_incorrect which does the job, which one single change: replace "old" by "current".



>=== modified file 'token.cgi'

>+    my $user         = new Bugzilla::User($userid);   

I know the current code use this, but it should really be Bugzilla::User->check() here, else Bugzilla would crash if the account is deleted meanwhile.


>-    # The email address has been changed, so we need to rederive the groups
>-    my $user = new Bugzilla::User($userid);

Groups are no longer rederived if you remove this code. To avoid recreating the user object, use $user->set_login() instead of the SQL statement. See how this is done in cancelChangeEmail(), which uses newer code.

Comment 8

[Koosha KM]

Attachment: V2 - not working

I don't know what the problem is?! It always displays the error "There is already an account with the login name ...".

Comment 9

[Frédéric Buclin]

Comment on attachment 620627 [details] [diff] [review]
V2 - not working

>+    $user->set_login($new_email);

$user->set_login() calls check_login_name_for_creation() which calls is_available_username() with only one argument which checks if the new login name is not already reserved in a token. As it's the case here, it incorrectly rejects your change. The bug comes from check_login_name_for_creation(); if it's called by a user object, then it should pass the current login name as the 2nd argument to is_available_username() so that it can do its job correctly. Also, check_login_name_for_creation() should be renamed to check_login_name().

Comment 10

[Koosha KM]

Attachment: v3 - working

(In reply to Frédéric Buclin from comment #9)
> Comment on attachment 620627 [details] [diff] [review]
> V2 - not working
> 
> >+    $user->set_login($new_email);
> 
> $user->set_login() calls check_login_name_for_creation() which calls
> is_available_username() with only one argument which checks if the new login
> name is not already reserved in a token. As it's the case here, it
> incorrectly rejects your change. The bug comes from
> check_login_name_for_creation(); if it's called by a user object, then it
> should pass the current login name as the 2nd argument to
> is_available_username() so that it can do its job correctly. Also,
> check_login_name_for_creation() should be renamed to check_login_name().

Well, I think this problem is not going to be solved simply. Because Bugzilla::Object->set calls all validators with two arguments:

303   $value = $self->$validator($value, $field);

As you can see, the $value is the only parameter that is passed as the value to be evaluated. So, it is not possible to pass the current login name (as the second value) to check_login_name_for_creation() and, in turn, to is_available_username(). What I did is to re-assign the $user:

$user = new Bugzilla::User($userid);

This does work indeed.

In addition, what you did on bug #677901 was that you made a major bug in cancelChangeEmail (token.cgi). To see the bug, consider the following scenario:

1. Try to change your current email to a new one on Preferences.
2. Confirm the email change using the new email you provided.
3. Try to cancel the change using the token sent to your old email.

Now, you will see the error "There is already an account with the login name <your old email>". In fact, the $user->set_login causes the same error here. I've solved this problem in this patch. It seems we must use the SQL commands.

Comment 11

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi from comment #10)
> So, it is not possible to pass the current login name (as
> the second value) to check_login_name_for_creation() and, in turn, to
> is_available_username().

You don't need to pass the current login name to check_login_name_for_creation() because its an object method, and so check_login_name_for_creation() can get this information itself by asking its object directly.


> In fact, the $user->set_login causes the same error here.

Which is why check_login_name_for_creation() must be fixed as I said in comment 9. :) Using SQL queries directly is not the right way to go.

Comment 12

[Frédéric Buclin]

Comment on attachment 621012 [details] [diff] [review]
v3 - working

r- per my previous comment.

Comment 13

[Koosha KM]

Unfortunately, is_available_username() acts differently when the user wants to revert the email change. When the user has changed his email but tries to revert the change using his old email address the $old_username and $username variables are swapped in it and then interpreted incorrectly (or maybe correctly indeed). Thus this subroutine always returns 0 and the change won't be possible.

$eventdata is true and the condition:
    if($old_username && ($eventdata eq "$old_username:$username"))

is not satisfied. Thus returns 0.

What should we do?! I think we should change the order of emailold:emailnew when the tokentype is emailold in the tokens table. So we would have:

+-----------+---------------+
| tokentype | eventdata     |
+-----------+---------------+
| emailold  | email1:email2 |
| emailnew  | email2:email1 |
+-----------+---------------+

That would solve the problem at least in this case. I don't know if it would cause some other problems elsewhere.

Comment 14

[Frédéric Buclin]

I'm going to fix the regression about cancelling the email address change in bug 752303 as this part is my fault and is a blocker. Once bug 752303 is fixed, this should make your life easier to fix this bug.

Comment 15

[Koosha KM]

Attachment: patch

Comment 16

[Frédéric Buclin]

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

>=== modified file 'token.cgi'

>+        ThrowUserError("current_password_incorrect");

As I said in comment 7, we don't need a separate error message. Reuse old_password_incorrect.


Otherwise looks good and works fine. r=LpSolit with current_password_incorrect being gone.

Comment 17

[Frédéric Buclin]

koosha: do you want to submit a new patch with comment 16 addressed, or do you want me to fix it on checkin?

Comment 18

[Koosha KM]

(In reply to Frédéric Buclin from comment #17)
> koosha: do you want to submit a new patch with comment 16 addressed, or do
> you want me to fix it on checkin?

I'm not sure if I understand you well. If I use the error "old_password_incorrect", it would be meaningless and or even confusing as there is no current/new password when we are talking about `old'. So, do you mean that I should use the "old_password_incorrect" error without any modifications? I think that's nonsense.

Comment 19

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #18)
> there is no current/new password when we are talking about `old'. So, do you
> mean that I should use the "old_password_incorrect" error without any
> modifications? I think that's nonsense.

I think "old" should either be replaced by "current" or removed entirely from the error message. The "Account Information" tab in User Preferences has no "Old Password" field in the UI, only a "Password" field, so removing "old" from the error message wouldn't be confusing.

Comment 20

[Koosha KM]

Attachment: patch

Comment 21

[Frédéric Buclin]

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

r=LpSolit

Comment 22

[Frédéric Buclin]

I did a minor change on checkin: I kept the "old_password_incorrect" error tag as is to not break extensions which use this error message, and simply replaced "old" by "current" in the error message itself.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified token.cgi
modified template/en/default/account/email/confirm.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 8243.

Comment 23

[Koosha KM]

Sounds OK.

Comment 24

[Frédéric Buclin]

email_confirmation_failed is no longer used. I removed it.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/global/user-error.html.tmpl
Committed revision 8244.

Comment 25

[Frédéric Buclin]

Added to relnotes for 4.4.