381124 - Bugzilla silently does nothing when user requests password and mail_delivery_method disallows outbound email

Status: NEW

(Bugzilla :: Email Notifications, defect, P4)

Description

[Kevin Benton]

The following code from token.pm does nothing to attempt to verify that a user can actually get a password that is emailed to the user.

177 sub requestChangePassword {
178     my ($login_name) = @_;
179     Bugzilla::Token::IssuePasswordToken($login_name);
180 
181     $vars->{'message'} = "password_change_request";
182 
183     print $cgi->header();
184     $template->process("global/message.html.tmpl", $vars)
185       || ThrowTemplateError($template->error());
186 }

The same is true of the code in Bugzilla/Token.pm:

126 sub IssuePasswordToken {
127     my $loginname = shift;
128     my $dbh = Bugzilla->dbh;
129     my $template = Bugzilla->template;
130     my $vars = {};
131 
132     # Retrieve the user's ID from the database.
133     trick_taint($loginname);
134     my ($userid, $too_soon) =
135         $dbh->selectrow_array('SELECT profiles.userid, tokens.issuedate
136                                  FROM profiles
137                             LEFT JOIN tokens
138                                    ON tokens.userid = profiles.userid
139                                   AND tokens.tokentype = ?
140                                   AND tokens.issuedate > NOW() - ' .
141                                       $dbh->sql_interval(10, 'MINUTE') . '
142                                 WHERE ' . $dbh->sql_istrcmp('login_name', '?'),
143                                 undef, ('password', $loginname));
144 
145     ThrowUserError('too_soon_for_new_token', {'type' => 'password'}) if $too_soon;
146 
147     my ($token, $token_ts) = _create_token($userid, 'password', $::ENV{'REMOTE_ADDR'});
148 
149     # Mail the user the token along with instructions for using it.
150     $vars->{'token'} = $token;
151     $vars->{'emailaddress'} = $loginname . Bugzilla->params->{'emailsuffix'};
152 
153     $vars->{'max_token_age'} = MAX_TOKEN_AGE;
154     $vars->{'token_ts'} = $token_ts;
155 
156     my $message = "";
157     $template->process("account/password/forgotten-password.txt.tmpl", 
158                                                                $vars, \$message)
159       || ThrowTemplateError($template->error());
160 
161     MessageToMTA($message);
162 }

The end result is the user sees a message indicating that an email was sent but that's a lie - it wasn't sent because the mail_delivery_method parameter is set to 'none'.

In general, this is not a problem because most installations don't turn off emails to users.  However, for those that do have it turned off, or for those that have it temporarily disabled, not warning the user that an email really wasn't sent (and never will be) seems like a lie.  I would rather see code in Bugzilla/Token.pm just above line 157 in this case to do the following:

    if (Param('mail_delivery_method' eq 'none')) {
        ThrowUserError('mail_turned_off_for_password_token');
    }

(and add an appropriate error message in the template).

At least that way, users would know to contact their administrator to request a password change rather than waiting on an email that will never arrive.

Comment 1

[Frédéric Buclin]

I'm not sure this is the right approach. Bugzilla should behave the same way as when emails are enabled, except that no email is sent. Else you could display on all pages that no email is sent (e.g. when changing flags or when editing bugs), which would quickly irritate users.

Anyway, if we really want to warn the user, this should be done outside Token.pm, whose job is to send an email. If it must not be sent or a warning must be displayed, then it should be done in the caller.

Comment 2

[Kevin Benton]

(In reply to comment #0)
> The following code from token.pm does nothing to attempt to verify that a user
> can actually get a password that is emailed to the user.

OOPS - I just realized that the code here was not from token.pm, rather token.cgi.  At any rate, I believe that token.cgi (or global/message.html.tmpl) should be telling users that email will not be sent because mail_delivery_method is set to 'none'.

(In reply to comment #1)
> I'm not sure this is the right approach. Bugzilla should behave the same way as
> when emails are enabled, except that no email is sent. Else you could display
> on all pages that no email is sent (e.g. when changing flags or when editing
> bugs), which would quickly irritate users.
> 
> Anyway, if we really want to warn the user, this should be done outside
> Token.pm, whose job is to send an email. If it must not be sent or a warning
> must be displayed, then it should be done in the caller.
> 

I believe it would be helpful to separate out email notification parameters a bit more by allowing administrators to shut off bug notifications separately from other administrative emails and whining.  Some sites may choose to allow whining and admin, but not bug updates.  This would be especially helpful in my development since I want to be able to work with live data (and let experts test against live data) but I don't want to alert users to changes I'm made to the test environment bugs.

I also think that this is one case where an exception should be made.  Having real email disabled means that a user can't reset his/her own password if the password has been forgotten; he/she must talk to an administrator to get that done.

Comment 3

[Max Kanat-Alexander]

I'm not sure we should fix this. Here's why:

* If you set mail_delivery_method to None, that means Bugzilla sends no mail whatsoever. The parameter should make that fairly clear. It would be the behavior I expect, certainly.

* We could remove the "Reset Password" link when mail_delivery_method is set to None, but then we'd start getting support questions and bugs like, "There's no way to reset my password!"

* We don't want to use the message system, because that would disrupt other places.

* Only the mail code knows that mail is disabled, and I kind of what to keep it that way.


We could possibly throw an error if mail_delivery_method is set to None, though. I suppose that's why this is marked minor.

Comment 4

[Frédéric Buclin]

The "Forgot Password" link is not the single problem. When creating a new user account, the confirmation email will never be sent, nor will whinemails be sent, same for bugmails and flagmails. There are so many places which would need to be fixed that it probably doesn't worth the code complexity. Also, if mail_delivery_method="Test", no emails are sent either; they are simply stored in bugzilla/data/mailer.testfile. Leaving the bug open for now, but it may be closed as wontfix.