Last Comment Bug 897264 - letters_numbers_specialchars password restriction is incorrect
: letters_numbers_specialchars password restriction is incorrect
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Accounts (show other bugs)
: 4.4
: All All
: -- normal (vote)
: Bugzilla 4.2
Assigned To: mail
: default-qa
Mentors:
Depends on: 558803
Blocks: 868330
  Show dependency treegraph
 
Reported: 2013-07-23 15:55 PDT by mail
Modified: 2013-08-08 20:58 PDT (History)
3 users (show)
mail: approval+
mail: approval4.4+
justdave: approval4.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
bug897264-v1.patch (2.52 KB, patch)
2013-07-24 18:09 PDT, mail
LpSolit: review-
Details | Diff | Splinter Review
v2 patch (2.23 KB, patch)
2013-07-28 23:15 PDT, mail
LpSolit: review-
Details | Diff | Splinter Review
bug897264-v3.patch (2.14 KB, patch)
2013-08-01 17:22 PDT, mail
LpSolit: review+
Details | Diff | Splinter Review

Description mail 2013-07-23 15:55:22 PDT
The regular expression used for the letters_numbers_specialchars password is incorrect, as a letter does not need to be used. For example, '1234567!' is would be considered to be a valid password.
Comment 1 mail 2013-07-24 18:09:06 PDT
Created attachment 780721 [details] [diff] [review]
bug897264-v1.patch

Also fixed up the alignment of the error message since I was in that area anyway.
Comment 2 Frédéric Buclin 2013-07-25 03:45:42 PDT
Why do you replace \d by [[:digit:]]?
Comment 3 mail 2013-07-25 15:32:29 PDT
(In reply to Frédéric Buclin from comment #2)
> Why do you replace \d by [[:digit:]]?

For consistency. AFAIK, they are the same (whereas [:alpha:] and \w are different). Plain English '[:digit:]' is also easier to read than the more cryptic '\d'

  -- simon
Comment 4 Frédéric Buclin 2013-07-26 07:32:25 PDT
Comment on attachment 780721 [details] [diff] [review]
bug897264-v1.patch

I don't care about readability. \d is used everywhere in our codebase and is well understood. We don't use [[:digit:]] anywhere else. I don't call that consistency. Please revert this change.
Comment 5 mail 2013-07-28 23:15:55 PDT
Created attachment 782426 [details] [diff] [review]
v2 patch
Comment 6 Frédéric Buclin 2013-07-31 06:59:17 PDT
Comment on attachment 782426 [details] [diff] [review]
v2 patch

>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+      [% IF passregex.search('letters') %]
>+        [% IF passregex.search('specialchars') %]
>+          <li>letter</li>
>+        [% ELSE %]
>+          <li>UPPERCASE letter</li>
>+          <li>lowercase letter</li>
>+        [% END %]
>+      [% END %]
>+      [% IF passregex.search('numbers') %]
>+        <li>digit</li>
>+      [% END %]
>+      [% IF passregex.search('specialchars') %]
>+        <li>special character</li>
>+      [% END %]

As you are now looking twice for 'specialchars', please combine them together:

 [% IF passregex == 'letters_numbers_specialchars' %]
   <li>letter</li>
   <li>special character</li>
 [% ELSIF passregex.search('letters') %]
   <li>UPPERCASE letter</li>
   <li>lowercase letter</li>
 [% END %]

 [% IF passregex.search('numbers') %]
   <li>digit</li>
 [% END %]
Comment 7 mail 2013-08-01 17:22:30 PDT
Created attachment 784690 [details] [diff] [review]
bug897264-v3.patch
Comment 8 Frédéric Buclin 2013-08-02 03:29:51 PDT
Comment on attachment 784690 [details] [diff] [review]
bug897264-v3.patch

r=LpSolit
Comment 9 mail 2013-08-06 16:21:24 PDT
09:20 <@LpSolit> pfff, I reviewed it, you can approve it
Comment 10 mail 2013-08-06 22:17:13 PDT
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/User.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8688.                                                       

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.4/
modified Bugzilla/User.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8588.
Comment 11 Frédéric Buclin 2013-08-07 01:40:00 PDT
Actually, this parameter has been implemented in Bugzilla 4.2, see bug 558803. As it affects the security-level of your passwords, it may be important to fix this bug there too.
Comment 12 mail 2013-08-08 20:58:30 PDT
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.2/                         
modified Bugzilla/User.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8221.

Note You need to log in before you can comment on or make changes to this bug.