868330 - Password creation directions incomplete

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Spencer Selander [greenknight]]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:23.0) Gecko/20130502 Firefox/23.0
Build ID: 20130502030939

Steps to reproduce:

Tried to log in with my old password, got "invalid login/password". Clicked "Lost Password", followed steps to change password.


Actual results:

Password creation dialog said "Must be at least 8 characters", created password met that requirement. Got rejection message that it must contain at least one capital letter, one lower-case letter, and one digit. Entered password met those requirements, finally managed to log in.


Expected results:

Password creation dialog should have included the full requirements for an acceptable password.

Comment 1

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v1

Comment 2

[Simon Green]

Comment on attachment 778851 [details] [diff] [review]
Patch-v1

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

::: template/en/default/account/password/set-forgotten-password.html.tmpl
@@ +18,5 @@
> +  <li>Password must contain at least one UPPER and one lowercase letter.</li>
> +  [% ELSIF password_complexity == "letters_numbers" %] 
> +  <li>Password must contain at least one UPPER and one lower case letter and a number.</li>
> +  [% ELSIF password_complexity == "letters_numbers_specialchars" %] 
> +  <li>Password must contain at least one UPPER or one lower case letter, a number and a special character.</li>

Better written as "Password must contain at least one letter, one number and a special character" (i.e. don't mention upper and lower case at all)

Comment 3

[Sunil Joshi [:sjoshi]]

 
> Better written as "Password must contain at least one letter, one number and
> a special character" (i.e. don't mention upper and lower case at all)

I have taken these messages from /bugzilla/editparams.cgi , password_complexity param where all options are described.

And also it is mandatory to use upper case, lower case letters and numbers if "letters_numbers_specialchars" is used as password_complexity same is true for "mixed_letters" and "letters_numbers"

So I think its worth mentioning the appropriate password rules.

Comment 4

[Frédéric Buclin]

(In reply to Sunil Joshi from comment #3)
> So I think its worth mentioning the appropriate password rules.

Correct, you must provide one UPPER and one LOWER letter, so it's not enough to just say to provide a letter, else the user won't understand what he is doing wrong.

@Sunil: please re-request review.

Comment 5

[Simon Green]

(In reply to Frédéric Buclin from comment #4)
> (In reply to Sunil Joshi from comment #3)
> > So I think its worth mentioning the appropriate password rules.
> 
> Correct, you must provide one UPPER and one LOWER letter, so it's not enough
> to just say to provide a letter, else the user won't understand what he is
> doing wrong.

That's not what the code says (validate_password in Bugzilla::User)

    if ($complexity_level eq 'letters_numbers_specialchars') {
        ThrowUserError('password_not_complex')
          if ($password !~ /\w/ || $password !~ /\d/ || $password !~ /[[:punct:]]/);

Therefore, '1234567!' is considered a valid password in the above case. You don't have to have any letters in it at all (as \w matches numbers too). If that is a bug, then I would expect to see a separate bug created for fixing that.

> @Sunil: please re-request review.

Unless the above is fixed, then I stand by my r-. Feel free to assign it to a different reviewer though.

Comment 6

[Frédéric Buclin]

Ah, I see what you meant. Your comment 2 was only about the letters_numbers_specialchars message. You quoted them all, so I thought you were asking to not mention UPPER and LOWER in all cases. Then yes, your suggestion in comment 2 makes sense.

Note that letters_numbers_specialchars uses OR while other params use AND.

Comment 7

[Spencer Selander [greenknight]]

(In reply to Simon Green from comment #5)
> 
> That's not what the code says (validate_password in Bugzilla::User)
> 
>     if ($complexity_level eq 'letters_numbers_specialchars') {
>         ThrowUserError('password_not_complex')
>           if ($password !~ /\w/ || $password !~ /\d/ || $password !~
> /[[:punct:]]/);
> 
> Therefore, '1234567!' is considered a valid password in the above case. You
> don't have to have any letters in it at all (as \w matches numbers too). If
> that is a bug, then I would expect to see a separate bug created for fixing
> that. 

I tried creating a new profile with 1234567! as the password, and it was rejected. Did I misunderstand what you were saying?

Comment 8

[Simon Green]

(In reply to Spencer Selander [greenknight] from comment #7)
> I tried creating a new profile with 1234567! as the password, and it was
> rejected. Did I misunderstand what you were saying?

It only happens when the 'password_complexity' parameter is set to 'letters_numbers_specialchars'. If it set to letters_numbers or mixed_letters, then it works as expected.

Comment 9

[Spencer Selander [greenknight]]

(In reply to Simon Green from comment #8)
Sunil's patch isn't going to cause any problems, though, is it? It will just prompt users to create a more complex password than actually required, in the event the parameter is set to require special chars. What would that hurt? Of course, it could be easily changed by deleting a few words, but why bother? It reflects the way the code was intended to work.
 
> It only happens when the 'password_complexity' parameter is set to
> 'letters_numbers_specialchars'. If it set to letters_numbers or
> mixed_letters, then it works as expected.

I'd say that's definitely a bug. I would file it, but it's too technical for me; I think someone who codes needs to do it. I wouldn't know how to write that up, and I couldn't provide a test case.

Comment 10

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v2

Simon,

I also got the same impression as Lpsolit, but yes i also missed testing with 'letters_numbers_specialchars' :)

But we need to be consistent, so it needs to be reworded in editparams also.

Patch contains this issue + rewording on editparams page.

Comment 11

[Simon Green]

This should not be committed until the dependant bug is fixed. I hope to write a patch sometime today for that.

Comment 12

[Frédéric Buclin]

Do not approve this bug yet if it cannot be committed right now. Also, please set the target milestone correctly when approving the bug.

Comment 13

[Simon Green]

(In reply to Frédéric Buclin from comment #12)
> Do not approve this bug yet if it cannot be committed right now. Also,
> please set the target milestone correctly when approving the bug.

Ack.

Comment 14

[Simon Green]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.4/                         
modified template/en/default/account/password/set-forgotten-password.html.tmpl
modified template/en/default/admin/params/auth.html.tmpl
Committed revision 8589.                                                       

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/account/password/set-forgotten-password.html.tmpl
modified template/en/default/admin/params/auth.html.tmpl
Committed revision 8689.

Comment 15

[Simon Green]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.2/                         
modified template/en/default/account/password/set-forgotten-password.html.tmpl
modified template/en/default/admin/params/auth.html.tmpl
Committed revision 8222.

Comment 16

[Frédéric Buclin]

(This patch shouldn't have landed into the 4.2 branch. This is not a security fix but a UI fix only. The security fix was bug 897264.)

Comment 17

[Dave Miller [:justdave]]

(In reply to Frédéric Buclin from comment #16)
> (This patch shouldn't have landed into the 4.2 branch. This is not a
> security fix but a UI fix only. The security fix was bug 897264.)

What Frédéric said.  Back it out on 4.2 please. (I know it's annoying, but l10n is annoying on security branches, and it's just another incentive to get people to upgrade if they care ;)  (easy enough to backport on their own if they really want it, too)

Comment 18

[Frédéric Buclin]

Patch backed out on the 4.2 branch.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified template/en/default/account/password/set-forgotten-password.html.tmpl
modified template/en/default/admin/params/auth.html.tmpl
Committed revision 8224.