Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Support new minimum password length parameter #1472

Merged
merged 4 commits into from
Aug 28, 2018
Merged

Conversation

luisrudge
Copy link
Contributor

No description provided.

@@ -88,6 +88,7 @@
},
"dependencies": {
"auth0-js": "^9.7.3",
"auth0-password-policies": "auth0/auth0-password-policies#v1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd trust more on password-sheriff than this repo. Sheriff already defines them here: https://github.com/auth0/password-sheriff/blob/4a17c9cba614ed8b75affc7c53ff0d4ba31a5b30/index.js. If this is not possible, then please copy that file and keep it here on this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our change password widget also uses this repo. This is fine

const result = initClient(Immutable.fromJS({}), client).toJS();
expect(result.client.connections.database[0].passwordPolicy).toMatchObject({
length: {
minLength: 6
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the same tests for the remaining policies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be just a test for an external resource, not sure that's needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

How can I trust that your code will behave the same when the policy used is other than low? Please add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed

if (!policy) {
return true;
}
return new PasswordPolicy(policy.toJS()).check(password);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be safer, e.g. checking that if policy == undefined then none policy is still applied? https://github.com/auth0/password-sheriff/blob/master/index.js#L57 Now it will work since none is just length > 0 but if that changes this code will need to be changed as well. Let's reference this to the none policy instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

policy is only undefined when it's a login, which we don't validate passwords.

import util from 'util';

export default class PasswordStrength extends React.Component {
render() {
const { password, policy, messages } = this.props;
const analysis = createPolicy(policy).missing(password);
const analysis = new PasswordPolicy(policy.toJS()).missing(password);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same applies here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this only runs on signup, so we're fine

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Thanks!

@luisrudge luisrudge merged commit 10b68db into master Aug 28, 2018
@luisrudge luisrudge deleted the feature/pwd-length branch August 28, 2018 18:46
@luisrudge luisrudge added this to the v11.9.0 milestone Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants