add isStrongPassword method - #1348

Merged
profnandaa merged 23 commits into
validatorjs:masterfrom
tbeeck:strong-password
Nov 19, 2020
Merged

add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom
tbeeck:strong-password

Conversation

@tbeeck

@tbeecktbeeck commented Jun 7, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

For issue #1145
This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)

I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.

This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@profnandaa

profnandaa commented Jun 8, 2020

Copy link
Copy Markdown
Member

@rubiin@tux-tn -- can review this one here please?

tux-tn
tux-tn reviewed Jun 11, 2020
View reviewed changes

@tux-tntux-tn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉
I have two comments:

  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system

@tbeeck

tbeeck commented Jun 11, 2020

Copy link
Copy Markdown
ContributorAuthor

@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .

Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?

letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}

I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.

@rubiin

rubiin commented Jun 11, 2020

Copy link
Copy Markdown
Member

@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .

Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?

letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}

I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.

passing options looks good plus it gives user some customizability on the parameters values

@tbeeck

tbeeck commented Jun 11, 2020

Copy link
Copy Markdown
ContributorAuthor

Now the user can decide to check the password by score or by requirements with these defaults specified:

constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};

isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.
Not sure if this is an optimal design, I am open to other suggestions.

@profnandaaprofnandaa mentioned this pull request Jun 13, 2020
Closed
@tbeeck

tbeeck commented Jun 29, 2020

Copy link
Copy Markdown
ContributorAuthor

After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:

constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};

The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)

@profnandaa

profnandaa commented Nov 15, 2020

Copy link
Copy Markdown
Member

@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?

/cc. @tux-tn@ezkemboi

profnandaa
profnandaa approved these changes Nov 15, 2020
View reviewed changes

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!

@tbeeck

tbeeck commented Nov 15, 2020

Copy link
Copy Markdown
ContributorAuthor

@profnandaa Yep I'm available for any more comments!

@profnandaa

profnandaa commented Nov 17, 2020

Copy link
Copy Markdown
Member

@tux-tn@rubiin@ezkemboi -- please have a look.

@ezkemboi

ezkemboi commented Nov 17, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Looks good here @profnandaa but I need to confirm something on percentage scoring.
A minute I test out locally.

ezkemboi
ezkemboi requested changes Nov 17, 2020
View reviewed changes

@ezkemboiezkemboi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.

Comment threadtest/validators.js
'EXAMPLE of very long_password123!',
'mxH_+2vs&54_+H3P',
'+&DxJ=X7-4L8jRCD',
'etV*p%Nr6w&H%FeF',

@ezkemboiezkemboiNov 17, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option

@tbeecktbeeckNov 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense

Comment threadREADME.md Outdated
**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`

@ezkemboiezkemboiNov 17, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.
But, from function arguments call, it takes str & option.
I expected something like below...

isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.

@tbeecktbeeckNov 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!

@profnandaa

profnandaa commented Nov 17, 2020

Copy link
Copy Markdown
Member

@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.

@tbeeck

tbeeck commented Nov 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go

@codecov

codecovBot commented Nov 18, 2020•
edited
Loading

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.

profnandaa
profnandaa approved these changes Nov 19, 2020
View reviewed changes

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉

@profnandaa
profnandaa merged commit 27aa419 into validatorjs:masterNov 19, 2020
@rubiin

rubiin commented Nov 19, 2020

Copy link
Copy Markdown
Member

@profnandaa when will the next release be available

@profnandaa

profnandaa commented Nov 23, 2020 via email

Copy link
Copy Markdown
Member

@profnandaaprofnandaa mentioned this pull request Nov 29, 2020
Merged
This was referenced Mar 12, 2021
Open
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees

No one assigned

Labels

✅ LGTMneeds-more-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
add isStrongPassword method - #1348
Merged
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Nov 19, 2020
Merged
add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Conversation
@tbeeck
@tbeecktbeeck
commented
Jun 7, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
For issue #1145

This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)
I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.
This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)
Checklist
  •  PR contains only changes related; no stray files, etc.
  •  README updated (where applicable)
  •  Tests written (where applicable)
@profnandaa
profnandaa
commented
Jun 8, 2020

Copy link


Copy Markdown

Member
@rubiin@tux-tn -- can review this one here please?
tux-tn
tux-tn
reviewed
Jun 11, 2020
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉

I have two comments:
  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
@rubiin
rubiin
commented
Jun 11, 2020

Copy link


Copy Markdown

Member
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
passing options looks good plus it gives user some customizability on the parameters values
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Now the user can decide to check the password by score or by requirements with these defaults specified:
constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};
isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.

Not sure if this is an optimal design, I am open to other suggestions.
@profnandaaprofnandaa
mentioned this pull request
Jun 13, 2020
 Closed

@tbeeck
tbeeck
commented
Jun 29, 2020

Copy link


Copy Markdown

ContributorAuthor
After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:
constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};
The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)
@profnandaa
profnandaa
commented
Nov 15, 2020

Copy link


Copy Markdown

Member
@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?
/cc. @tux-tn@ezkemboi
profnandaa
profnandaa
approved these changes
Nov 15, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!
@tbeeck
tbeeck
commented
Nov 15, 2020

Copy link


Copy Markdown

ContributorAuthor
@profnandaa Yep I'm available for any more comments!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@tux-tn@rubiin@ezkemboi -- please have a look.
@ezkemboi
ezkemboi
commented
Nov 17, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Looks good here @profnandaa but I need to confirm something on percentage scoring.

A minute I test out locally.
ezkemboi
ezkemboi
requested changes
Nov 17, 2020
View reviewed changes
@ezkemboiezkemboi
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.
Comment threadtest/validators.js
 'EXAMPLE of very long_password123!',
 'mxH_+2vs&54_+H3P',
 '+&DxJ=X7-4L8jRCD',
 'etV*p%Nr6w&H%FeF',
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense
Comment threadREADME.md
Outdated

**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.

But, from function arguments call, it takes str & option.

I expected something like below...
isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.
@tbeeck
tbeeck
commented
Nov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go
@codecov
codecovBot
commented
Nov 18, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0 
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.
profnandaa
profnandaa
approved these changes
Nov 19, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉
@profnandaa
profnandaa
merged commit 27aa419
into
validatorjs:masterNov 19, 2020
@rubiin
rubiin
commented
Nov 19, 2020

Copy link


Copy Markdown

Member
@profnandaa when will the next release be available
@profnandaa
profnandaa
commented
Nov 23, 2020
via email 

Copy link


Copy Markdown

Member
@profnandaaprofnandaa
mentioned this pull request
Nov 29, 2020
 Merged


This was referenced Mar 12, 2021
 Open

 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees


No one assigned


Labels

✅ LGTMneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
add isStrongPassword method - #1348
Merged
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Nov 19, 2020
Merged
add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Conversation
@tbeeck
@tbeecktbeeck
commented
Jun 7, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
For issue #1145

This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)
I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.
This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)
Checklist
  •  PR contains only changes related; no stray files, etc.
  •  README updated (where applicable)
  •  Tests written (where applicable)
@profnandaa
profnandaa
commented
Jun 8, 2020

Copy link


Copy Markdown

Member
@rubiin@tux-tn -- can review this one here please?
tux-tn
tux-tn
reviewed
Jun 11, 2020
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉

I have two comments:
  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
@rubiin
rubiin
commented
Jun 11, 2020

Copy link


Copy Markdown

Member
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
passing options looks good plus it gives user some customizability on the parameters values
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Now the user can decide to check the password by score or by requirements with these defaults specified:
constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};
isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.

Not sure if this is an optimal design, I am open to other suggestions.
@profnandaaprofnandaa
mentioned this pull request
Jun 13, 2020
 Closed

@tbeeck
tbeeck
commented
Jun 29, 2020

Copy link


Copy Markdown

ContributorAuthor
After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:
constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};
The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)
@profnandaa
profnandaa
commented
Nov 15, 2020

Copy link


Copy Markdown

Member
@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?
/cc. @tux-tn@ezkemboi
profnandaa
profnandaa
approved these changes
Nov 15, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!
@tbeeck
tbeeck
commented
Nov 15, 2020

Copy link


Copy Markdown

ContributorAuthor
@profnandaa Yep I'm available for any more comments!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@tux-tn@rubiin@ezkemboi -- please have a look.
@ezkemboi
ezkemboi
commented
Nov 17, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Looks good here @profnandaa but I need to confirm something on percentage scoring.

A minute I test out locally.
ezkemboi
ezkemboi
requested changes
Nov 17, 2020
View reviewed changes
@ezkemboiezkemboi
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.
Comment threadtest/validators.js
 'EXAMPLE of very long_password123!',
 'mxH_+2vs&54_+H3P',
 '+&DxJ=X7-4L8jRCD',
 'etV*p%Nr6w&H%FeF',
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense
Comment threadREADME.md
Outdated

**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.

But, from function arguments call, it takes str & option.

I expected something like below...
isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.
@tbeeck
tbeeck
commented
Nov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go
@codecov
codecovBot
commented
Nov 18, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0 
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.
profnandaa
profnandaa
approved these changes
Nov 19, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉
@profnandaa
profnandaa
merged commit 27aa419
into
validatorjs:masterNov 19, 2020
@rubiin
rubiin
commented
Nov 19, 2020

Copy link


Copy Markdown

Member
@profnandaa when will the next release be available
@profnandaa
profnandaa
commented
Nov 23, 2020
via email 

Copy link


Copy Markdown

Member
@profnandaaprofnandaa
mentioned this pull request
Nov 29, 2020
 Merged


This was referenced Mar 12, 2021
 Open

 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees


No one assigned


Labels

✅ LGTMneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
add isStrongPassword method - #1348
Merged
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Nov 19, 2020
Merged
add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Conversation
@tbeeck
@tbeecktbeeck
commented
Jun 7, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
For issue #1145

This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)
I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.
This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)
Checklist
  •  PR contains only changes related; no stray files, etc.
  •  README updated (where applicable)
  •  Tests written (where applicable)
@profnandaa
profnandaa
commented
Jun 8, 2020

Copy link


Copy Markdown

Member
@rubiin@tux-tn -- can review this one here please?
tux-tn
tux-tn
reviewed
Jun 11, 2020
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉

I have two comments:
  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
@rubiin
rubiin
commented
Jun 11, 2020

Copy link


Copy Markdown

Member
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
passing options looks good plus it gives user some customizability on the parameters values
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Now the user can decide to check the password by score or by requirements with these defaults specified:
constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};
isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.

Not sure if this is an optimal design, I am open to other suggestions.
@profnandaaprofnandaa
mentioned this pull request
Jun 13, 2020
 Closed

@tbeeck
tbeeck
commented
Jun 29, 2020

Copy link


Copy Markdown

ContributorAuthor
After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:
constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};
The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)
@profnandaa
profnandaa
commented
Nov 15, 2020

Copy link


Copy Markdown

Member
@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?
/cc. @tux-tn@ezkemboi
profnandaa
profnandaa
approved these changes
Nov 15, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!
@tbeeck
tbeeck
commented
Nov 15, 2020

Copy link


Copy Markdown

ContributorAuthor
@profnandaa Yep I'm available for any more comments!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@tux-tn@rubiin@ezkemboi -- please have a look.
@ezkemboi
ezkemboi
commented
Nov 17, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Looks good here @profnandaa but I need to confirm something on percentage scoring.

A minute I test out locally.
ezkemboi
ezkemboi
requested changes
Nov 17, 2020
View reviewed changes
@ezkemboiezkemboi
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.
Comment threadtest/validators.js
 'EXAMPLE of very long_password123!',
 'mxH_+2vs&54_+H3P',
 '+&DxJ=X7-4L8jRCD',
 'etV*p%Nr6w&H%FeF',
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense
Comment threadREADME.md
Outdated

**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.

But, from function arguments call, it takes str & option.

I expected something like below...
isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.
@tbeeck
tbeeck
commented
Nov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go
@codecov
codecovBot
commented
Nov 18, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0 
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.
profnandaa
profnandaa
approved these changes
Nov 19, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉
@profnandaa
profnandaa
merged commit 27aa419
into
validatorjs:masterNov 19, 2020
@rubiin
rubiin
commented
Nov 19, 2020

Copy link


Copy Markdown

Member
@profnandaa when will the next release be available
@profnandaa
profnandaa
commented
Nov 23, 2020
via email 

Copy link


Copy Markdown

Member
@profnandaaprofnandaa
mentioned this pull request
Nov 29, 2020
 Merged


This was referenced Mar 12, 2021
 Open

 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees


No one assigned


Labels

✅ LGTMneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
add isStrongPassword method - #1348
Merged
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Nov 19, 2020
Merged
add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Conversation
@tbeeck
@tbeecktbeeck
commented
Jun 7, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
For issue #1145

This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)
I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.
This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)
Checklist
  •  PR contains only changes related; no stray files, etc.
  •  README updated (where applicable)
  •  Tests written (where applicable)
@profnandaa
profnandaa
commented
Jun 8, 2020

Copy link


Copy Markdown

Member
@rubiin@tux-tn -- can review this one here please?
tux-tn
tux-tn
reviewed
Jun 11, 2020
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉

I have two comments:
  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
@rubiin
rubiin
commented
Jun 11, 2020

Copy link


Copy Markdown

Member
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
passing options looks good plus it gives user some customizability on the parameters values
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Now the user can decide to check the password by score or by requirements with these defaults specified:
constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};
isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.

Not sure if this is an optimal design, I am open to other suggestions.
@profnandaaprofnandaa
mentioned this pull request
Jun 13, 2020
 Closed

@tbeeck
tbeeck
commented
Jun 29, 2020

Copy link


Copy Markdown

ContributorAuthor
After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:
constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};
The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)
@profnandaa
profnandaa
commented
Nov 15, 2020

Copy link


Copy Markdown

Member
@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?
/cc. @tux-tn@ezkemboi
profnandaa
profnandaa
approved these changes
Nov 15, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!
@tbeeck
tbeeck
commented
Nov 15, 2020

Copy link


Copy Markdown

ContributorAuthor
@profnandaa Yep I'm available for any more comments!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@tux-tn@rubiin@ezkemboi -- please have a look.
@ezkemboi
ezkemboi
commented
Nov 17, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Looks good here @profnandaa but I need to confirm something on percentage scoring.

A minute I test out locally.
ezkemboi
ezkemboi
requested changes
Nov 17, 2020
View reviewed changes
@ezkemboiezkemboi
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.
Comment threadtest/validators.js
 'EXAMPLE of very long_password123!',
 'mxH_+2vs&54_+H3P',
 '+&DxJ=X7-4L8jRCD',
 'etV*p%Nr6w&H%FeF',
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense
Comment threadREADME.md
Outdated

**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.

But, from function arguments call, it takes str & option.

I expected something like below...
isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.
@tbeeck
tbeeck
commented
Nov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go
@codecov
codecovBot
commented
Nov 18, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0 
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.
profnandaa
profnandaa
approved these changes
Nov 19, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉
@profnandaa
profnandaa
merged commit 27aa419
into
validatorjs:masterNov 19, 2020
@rubiin
rubiin
commented
Nov 19, 2020

Copy link


Copy Markdown

Member
@profnandaa when will the next release be available
@profnandaa
profnandaa
commented
Nov 23, 2020
via email 

Copy link


Copy Markdown

Member
@profnandaaprofnandaa
mentioned this pull request
Nov 29, 2020
 Merged


This was referenced Mar 12, 2021
 Open

 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees


No one assigned


Labels

✅ LGTMneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
add isStrongPassword method - #1348
Merged
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Nov 19, 2020
Merged
add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Conversation
@tbeeck
@tbeecktbeeck
commented
Jun 7, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
For issue #1145

This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)
I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.
This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)
Checklist
  •  PR contains only changes related; no stray files, etc.
  •  README updated (where applicable)
  •  Tests written (where applicable)
@profnandaa
profnandaa
commented
Jun 8, 2020

Copy link


Copy Markdown

Member
@rubiin@tux-tn -- can review this one here please?
tux-tn
tux-tn
reviewed
Jun 11, 2020
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉

I have two comments:
  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
@rubiin
rubiin
commented
Jun 11, 2020

Copy link


Copy Markdown

Member
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
passing options looks good plus it gives user some customizability on the parameters values
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Now the user can decide to check the password by score or by requirements with these defaults specified:
constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};
isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.

Not sure if this is an optimal design, I am open to other suggestions.
@profnandaaprofnandaa
mentioned this pull request
Jun 13, 2020
 Closed

@tbeeck
tbeeck
commented
Jun 29, 2020

Copy link


Copy Markdown

ContributorAuthor
After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:
constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};
The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)
@profnandaa
profnandaa
commented
Nov 15, 2020

Copy link


Copy Markdown

Member
@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?
/cc. @tux-tn@ezkemboi
profnandaa
profnandaa
approved these changes
Nov 15, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!
@tbeeck
tbeeck
commented
Nov 15, 2020

Copy link


Copy Markdown

ContributorAuthor
@profnandaa Yep I'm available for any more comments!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@tux-tn@rubiin@ezkemboi -- please have a look.
@ezkemboi
ezkemboi
commented
Nov 17, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Looks good here @profnandaa but I need to confirm something on percentage scoring.

A minute I test out locally.
ezkemboi
ezkemboi
requested changes
Nov 17, 2020
View reviewed changes
@ezkemboiezkemboi
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.
Comment threadtest/validators.js
 'EXAMPLE of very long_password123!',
 'mxH_+2vs&54_+H3P',
 '+&DxJ=X7-4L8jRCD',
 'etV*p%Nr6w&H%FeF',
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense
Comment threadREADME.md
Outdated

**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.

But, from function arguments call, it takes str & option.

I expected something like below...
isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.
@tbeeck
tbeeck
commented
Nov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go
@codecov
codecovBot
commented
Nov 18, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0 
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.
profnandaa
profnandaa
approved these changes
Nov 19, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉
@profnandaa
profnandaa
merged commit 27aa419
into
validatorjs:masterNov 19, 2020
@rubiin
rubiin
commented
Nov 19, 2020

Copy link


Copy Markdown

Member
@profnandaa when will the next release be available
@profnandaa
profnandaa
commented
Nov 23, 2020
via email 

Copy link


Copy Markdown

Member
@profnandaaprofnandaa
mentioned this pull request
Nov 29, 2020
 Merged


This was referenced Mar 12, 2021
 Open

 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees


No one assigned


Labels

✅ LGTMneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
add isStrongPassword method - #1348
Merged
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Nov 19, 2020
Merged
add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Conversation
@tbeeck
@tbeecktbeeck
commented
Jun 7, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
For issue #1145

This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)
I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.
This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)
Checklist
  •  PR contains only changes related; no stray files, etc.
  •  README updated (where applicable)
  •  Tests written (where applicable)
@profnandaa
profnandaa
commented
Jun 8, 2020

Copy link


Copy Markdown

Member
@rubiin@tux-tn -- can review this one here please?
tux-tn
tux-tn
reviewed
Jun 11, 2020
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉

I have two comments:
  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
@rubiin
rubiin
commented
Jun 11, 2020

Copy link


Copy Markdown

Member
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
passing options looks good plus it gives user some customizability on the parameters values
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Now the user can decide to check the password by score or by requirements with these defaults specified:
constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};
isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.

Not sure if this is an optimal design, I am open to other suggestions.
@profnandaaprofnandaa
mentioned this pull request
Jun 13, 2020
 Closed

@tbeeck
tbeeck
commented
Jun 29, 2020

Copy link


Copy Markdown

ContributorAuthor
After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:
constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};
The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)
@profnandaa
profnandaa
commented
Nov 15, 2020

Copy link


Copy Markdown

Member
@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?
/cc. @tux-tn@ezkemboi
profnandaa
profnandaa
approved these changes
Nov 15, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!
@tbeeck
tbeeck
commented
Nov 15, 2020

Copy link


Copy Markdown

ContributorAuthor
@profnandaa Yep I'm available for any more comments!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@tux-tn@rubiin@ezkemboi -- please have a look.
@ezkemboi
ezkemboi
commented
Nov 17, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Looks good here @profnandaa but I need to confirm something on percentage scoring.

A minute I test out locally.
ezkemboi
ezkemboi
requested changes
Nov 17, 2020
View reviewed changes
@ezkemboiezkemboi
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.
Comment threadtest/validators.js
 'EXAMPLE of very long_password123!',
 'mxH_+2vs&54_+H3P',
 '+&DxJ=X7-4L8jRCD',
 'etV*p%Nr6w&H%FeF',
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense
Comment threadREADME.md
Outdated

**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.

But, from function arguments call, it takes str & option.

I expected something like below...
isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.
@tbeeck
tbeeck
commented
Nov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go
@codecov
codecovBot
commented
Nov 18, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0 
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.
profnandaa
profnandaa
approved these changes
Nov 19, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉
@profnandaa
profnandaa
merged commit 27aa419
into
validatorjs:masterNov 19, 2020
@rubiin
rubiin
commented
Nov 19, 2020

Copy link


Copy Markdown

Member
@profnandaa when will the next release be available
@profnandaa
profnandaa
commented
Nov 23, 2020
via email 

Copy link


Copy Markdown

Member
@profnandaaprofnandaa
mentioned this pull request
Nov 29, 2020
 Merged


This was referenced Mar 12, 2021
 Open

 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees


No one assigned


Labels

✅ LGTMneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
add isStrongPassword method - #1348
Merged
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Nov 19, 2020
Merged
add isStrongPassword method#1348
profnandaa merged 23 commits into
validatorjs:masterfrom 
tbeeck:strong-password
Conversation
@tbeeck
@tbeecktbeeck
commented
Jun 7, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
For issue #1145

This adds the isStrongPassword method. It does include tests for when returning true/false, but not for when returning the score due to issues with test design decried here: #1145 (comment)
I am open to suggestions on how the passwords are scored since this algorithm is pretty rudimentary.
This is my first contribution to a public project, please let me know if there is anything else I can change to make this better :)
Checklist
  •  PR contains only changes related; no stray files, etc.
  •  README updated (where applicable)
  •  Tests written (where applicable)
@profnandaa
profnandaa
commented
Jun 8, 2020

Copy link


Copy Markdown

Member
@rubiin@tux-tn -- can review this one here please?
tux-tn
tux-tn
reviewed
Jun 11, 2020
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Great job @door-bell 🎉

I have two comments:
  • I think Readme is missing default values for strongThreshold and score
  • Since there is no standard defined for a strong password. I think the validator is a little biased. Generally validators are based on norms, standards or definitions but here this is not the case. I'm not saying that this validator is a bad thing but I think we should rather let the user choose his own definition rather than create a custom scoring system
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
@rubiin
rubiin
commented
Jun 11, 2020

Copy link


Copy Markdown

Member
@tux-tn Yeah, I agree that the validator is biased and won't necessarily meet the user's expectations every time. I implemented the scoring system since it was suggested in the feature request #1145 .
Do you think it would be helpful to allow the user to pass in a list of requirements like this as an optional usage?
letrequirements={
minCharCount =8,
mustContainUpper =true,
mustContainNumber =true,
mustContainSymbol =true}
I will hold off on fixing readme just in case we want to change things further, but I'll be sure to update that as well.
passing options looks good plus it gives user some customizability on the parameters values
@tbeeck
tbeeck
commented
Jun 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Now the user can decide to check the password by score or by requirements with these defaults specified:
constdefaultRequirementOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,};constdefaultScoringOptions={returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,minStrongScore: 50,};
isStrongPassword defaults to checking based on requirements. To use the scoring system, the user needs to use the named parameter scoringOptions.

Not sure if this is an optimal design, I am open to other suggestions.
@profnandaaprofnandaa
mentioned this pull request
Jun 13, 2020
 Closed

@tbeeck
tbeeck
commented
Jun 29, 2020

Copy link


Copy Markdown

ContributorAuthor
After some more consideration I decided to change how the options work. Now there is a single options parameter with these defaults:
constdefaultOptions={minLength: 8,minLowercase: 1,minUppercase: 1,minNumbers: 1,minSymbols: 1,returnScore: false,pointsPerUnique: 1,pointsPerRepeat: 0.5,pointsForContainingLower: 10,pointsForContainingUpper: 10,pointsForContainingNumber: 10,pointsForContainingSymbol: 10,};
The user simply decides if they want a score or not using returnScore. Otherwise it defaults to returning a boolean based on the minimum requirements for the password. This seems like a more intuitive usage to me, and the user can still test the password against a minimum score on their own. (Also, sorry this PR got messy with commits, I had some trouble syncing my remote branches across two machines)
@profnandaa
profnandaa
commented
Nov 15, 2020

Copy link


Copy Markdown

Member
@door-bell -- I'm sorry for my delayed response on this; this sounds good to me. Are you available for any further review comments this week so that we land this with the November release?
/cc. @tux-tn@ezkemboi
profnandaa
profnandaa
approved these changes
Nov 15, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for the most part. I like the flexibility in scoring!
@tbeeck
tbeeck
commented
Nov 15, 2020

Copy link


Copy Markdown

ContributorAuthor
@profnandaa Yep I'm available for any more comments!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@tux-tn@rubiin@ezkemboi -- please have a look.
@ezkemboi
ezkemboi
commented
Nov 17, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Looks good here @profnandaa but I need to confirm something on percentage scoring.

A minute I test out locally.
ezkemboi
ezkemboi
requested changes
Nov 17, 2020
View reviewed changes
@ezkemboiezkemboi
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good work.
Comment threadtest/validators.js
 'EXAMPLE of very long_password123!',
 'mxH_+2vs&54_+H3P',
 '+&DxJ=X7-4L8jRCD',
 'etV*p%Nr6w&H%FeF',
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a need for tests that pass returnScore as an option
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the tests for this case in the sanitizers.js test file since with this option, the function essentially becomes a sanitizer, turning the string into a number. Hope that makes sense
Comment threadREADME.md
Outdated

**isSurrogatePair(str)** | check if the string contains any surrogate pairs chars.
**isUppercase(str)** | check if the string is uppercase.
**isSlug** | Check if the string is of type slug. `Options` allow a single hyphen between string. e.g. [`cn-cn`, `cn-c-c`]
**isStrongPassword(str, requirementOptions?, scoringOptions?)** | Check if a password is strong or not. Allows for custom requirements or scoring rules. If `returnScore` is true, then the function returns an integer score for the password rather than a boolean.<br/>Default options: <br/>`{ minLength: 8, minLowercase: 1, minUppercase: 1, minNumbers: 1, minSymbols: 1, returnScore: false, pointsPerUnique: 1, pointsPerRepeat: 0.5, pointsForContainingLower: 10, pointsForContainingUpper: 10, pointsForContainingNumber: 10, pointsForContainingSymbol: 10 }`
@ezkemboiezkemboiNov 17, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my look of the eye, I thought, I needed to pass 3 parameters, though 2 are optional.

But, from function arguments call, it takes str & option.

I expected something like below...
isStrongPassword(str [, options])

It is not a priority but my suggestion for consistency and easy readability/usage.
@tbeecktbeeckNov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, thanks!
@profnandaa
profnandaa
commented
Nov 17, 2020

Copy link


Copy Markdown

Member
@door-bell -- you can address Ez's comments and we land this soonest. We would like to make a release this Friday.
@tbeeck
tbeeck
commented
Nov 17, 2020

Copy link


Copy Markdown

ContributorAuthor
Travis job stuck in the queue D: However I pushed some changes to address those comments and this should be good to go
@codecov
codecovBot
commented
Nov 18, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
❗ No coverage uploaded for pull request base (master@a3cddc1). Click here to learn what that means.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #1348 +/- ##
=========================================
Coverage ? 99.92% =========================================
Files ? 97 Lines ? 1332 Branches ? 0 =========================================
Hits ? 1331 Misses ? 1 Partials ? 0 
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a3cddc1...8ab1d36. Read the comment docs.
profnandaa
profnandaa
approved these changes
Nov 19, 2020
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once again, thanks for your contrib! 🎉
@profnandaa
profnandaa
merged commit 27aa419
into
validatorjs:masterNov 19, 2020
@rubiin
rubiin
commented
Nov 19, 2020

Copy link


Copy Markdown

Member
@profnandaa when will the next release be available
@profnandaa
profnandaa
commented
Nov 23, 2020
via email 

Copy link


Copy Markdown

Member
@profnandaaprofnandaa
mentioned this pull request
Nov 29, 2020
 Merged


This was referenced Mar 12, 2021
 Open

 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tux-tntux-tntux-tn left review comments
@profnandaaprofnandaaprofnandaa approved these changes
@ezkemboiezkemboiezkemboi approved these changes

Assignees


No one assigned


Labels

✅ LGTMneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@tbeeck@profnandaa@rubiin@ezkemboi@tux-tn