Closed Bug 868330 Opened 11 years ago Closed 11 years ago

Password creation directions incomplete

Categories

(Bugzilla :: User Accounts, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: spencerselander, Assigned: sjoshi)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Severity: normal → trivial
Attached patch Patch-v1 (obsolete) — Splinter Review
Assignee: user-accounts → joshi_sunil
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #778851 - Flags: review?(sgreen)
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)
Attachment #778851 - Flags: review?(sgreen) → review-
 
> 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.
(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.
(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.
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.
(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?
(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.
(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.
Attached patch Patch-v2Splinter Review
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.
Attachment #778851 - Attachment is obsolete: true
Attachment #779862 - Flags: review?(sgreen)
Depends on: 897264
Attachment #779862 - Flags: review?(sgreen) → review+
This should not be committed until the dependant bug is fixed. I hope to write a patch sometime today for that.
Flags: approval?
Flags: approval4.4?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
Do not approve this bug yet if it cannot be committed right now. Also, please set the target milestone correctly when approving the bug.
(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.
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
Target Milestone: --- → Bugzilla 4.4
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Flags: approval4.2+
Target Milestone: Bugzilla 4.4 → Bugzilla 4.2
(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.)
(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)
Flags: approval4.2+ → approval4.2-
Target Milestone: Bugzilla 4.2 → Bugzilla 4.4
Patch backed out on the 4.2 branch.

Committing to: bzr+ssh://lpsolit%40gmail.com@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 8224.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: