Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add minimum lowercase rule for password validation. #24230

Merged
merged 6 commits into from Mar 22, 2019

Conversation

Projects
None yet
@HLeithner
Copy link
Member

HLeithner commented Mar 18, 2019

Pull Request for Issue #24156 .

Summary of Changes

Add missing lower case requirement for passwords

Testing Instructions

Set minimum lowercase count to 0 or higher.
Change password. Use password with more and less lowercase characters

Expected result

It should be possible to force a minimum of lowercase characters.

Actual result

Option is missing.

@@ -193,6 +193,8 @@ COM_ADMIN_ZLIB_ENABLED="Zlib Enabled"
; Messages
COM_USERS_MSG_NOT_ENOUGH_INTEGERS_N="Password does not have enough digits. At least %s digits are required."
COM_USERS_MSG_NOT_ENOUGH_INTEGERS_N_1="Password does not have enough digits. At least 1 digit is required."
COM_USERS_MSG_NOT_ENOUGH_LOWERCASE_LETTERS_N="Password does not have enough lowercase characters. At least %s lower case characters are required."

This comment has been minimized.

Copy link
@brianteeman

brianteeman Mar 18, 2019

Contributor

Lower case can be written as either one word or two. At a minimum we should be consistent in only using one variant. Using the gov.uk website as a source we should standardise on one word not two

@@ -193,6 +193,8 @@ COM_ADMIN_ZLIB_ENABLED="Zlib Enabled"
; Messages
COM_USERS_MSG_NOT_ENOUGH_INTEGERS_N="Password does not have enough digits. At least %s digits are required."
COM_USERS_MSG_NOT_ENOUGH_INTEGERS_N_1="Password does not have enough digits. At least 1 digit is required."
COM_USERS_MSG_NOT_ENOUGH_LOWERCASE_LETTERS_N="Password does not have enough lowercase characters. At least %s lower case characters are required."
COM_USERS_MSG_NOT_ENOUGH_LOWERCASE_LETTERS_N_1="Password does not have enough lowercase characters. At least 1 lower case character is required."

This comment has been minimized.

Copy link
@brianteeman

brianteeman Mar 18, 2019

Contributor

as above

@HLeithner

This comment has been minimized.

Copy link
Member Author

HLeithner commented Mar 18, 2019

I did a copy of the uppercase sentence,.. should be changed too?

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Mar 18, 2019

I just realised that too ;)

I don't care if we use one word or two but we should be consistent - especially in the same sentence. Should help the translators as well

@@ -46,6 +46,8 @@ COM_USERS_CONFIG_FIELD_MAILTOADMIN_DESC="If set to Yes then a notification mail
COM_USERS_CONFIG_FIELD_MAILTOADMIN_LABEL="Send Mail to Administrators"
COM_USERS_CONFIG_FIELD_MINIMUM_INTEGERS="Minimum Integers"
COM_USERS_CONFIG_FIELD_MINIMUM_INTEGERS_DESC="Set the minimum number of integers that must be included in a password."
COM_USERS_CONFIG_FIELD_MINIMUM_LOWERCASE="Minimum Lower Case"
COM_USERS_CONFIG_FIELD_MINIMUM_LOWERCASE_DESC="Set the minimum number of lower case alphabetical characters required for a password."

This comment has been minimized.

Copy link
@Quy

Quy Mar 18, 2019

Contributor

Change here? Label too?

@agerix

This comment has been minimized.

Copy link

agerix commented Mar 18, 2019

Tested with Joomla! Patch Tester and it works fine

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

franz-wohlkoenig commented Mar 18, 2019

@agerix please mark your Test at Issue Tracker as "Tested successfully".

@agerix

This comment has been minimized.

Copy link

agerix commented Mar 18, 2019

I have tested this item successfully on ee1d90a

Hello, I've test with one and with more than one lower case


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24230.

@agerix

This comment has been minimized.

Copy link

agerix commented Mar 18, 2019

I've just do it Franz, I'm a new tester... :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24230.

@HLeithner

This comment has been minimized.

Copy link
Member Author

HLeithner commented Mar 18, 2019

@Quy you are right, I changed all to two words as in the label.

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Mar 18, 2019

@HLeithner you need to reflect these changes here: https://github.com/joomla/joomla-cms/blob/staging/media/system/js/passwordstrength.js

A fair warning tho, that file is Mootools based so in essence wouldn't be advisable to ask anyone to add any new functionality there. In short better do this in the 4.0 repo...

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Mar 18, 2019

This is already available in j4

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Mar 18, 2019

This is already available in j4

Well I lost track of the things I did already for J4 😉

Anyways, then at a bare minimum, the last function in the Mootools version needs to be adapted

@HLeithner

This comment has been minimized.

Copy link
Member Author

HLeithner commented Mar 18, 2019

@dgrammatiko this file looks like its only show the password strength and doesn't have anything to do with the requirement.

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Mar 18, 2019

@HLeithner it should read the requirements for calculating the strength, or maybe not...

@HLeithner

This comment has been minimized.

Copy link
Member Author

HLeithner commented Mar 18, 2019

The script uses the threshold that is set as parameter I didn't find any calculation based on the requirements parameters...

@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Mar 20, 2019

I have tested this item successfully on da122f3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24230.

@jeckodevelopment

This comment has been minimized.

Copy link
Member

jeckodevelopment commented Mar 21, 2019

@agerix can you please submit again your test?

if ($nLowercase < $minimumLowercase)
{
\JFactory::getApplication()->enqueueMessage(

This comment has been minimized.

Copy link
@alikon

alikon Mar 21, 2019

Contributor

should be easy to namespace

if ($nLowercase < $minimumLowercase)
{
\JFactory::getApplication()->enqueueMessage(
\JText::plural('COM_USERS_MSG_NOT_ENOUGH_LOWERCASE_LETTERS_N', $minimumLowercase),

This comment has been minimized.

Copy link
@alikon

alikon Mar 21, 2019

Contributor

here too

@HLeithner

This comment has been minimized.

Copy link
Member Author

HLeithner commented Mar 21, 2019

If I add the name spaces here I have to change the complete file and I think this would maybe give a merge conflict when george merge it into 4.0

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Mar 21, 2019

J4 already has a lowercase rule

@alikon

This comment has been minimized.

Copy link
Contributor

alikon commented Mar 21, 2019

maybe it's me but the 4.0 https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Rule/PasswordRule.php didn't have lowercase yet....

@HLeithner not an expert on "easy merging" matter but looking the 2 file side by side ....

@alikon

This comment has been minimized.

Copy link
Contributor

alikon commented Mar 21, 2019

I have tested this item successfully on e386929


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24230.

1 similar comment
@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Mar 21, 2019

I have tested this item successfully on e386929


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24230.

@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Mar 21, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24230.

@joomla-cms-bot joomla-cms-bot added the RTC label Mar 21, 2019

@wilsonge wilsonge merged commit 857fa53 into joomla:staging Mar 22, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Mar 22, 2019

@zero-24 zero-24 added this to the Joomla 3.9.5 milestone Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.