Adding support for optional Password Policy #3032

bhaskaryasa · 2016-11-09T11:25:44Z

Introducing two settings under passwordPolicy.

validators

Two settings can be used to enforce strong passwords as required by the application. They will be applied at both sign up and password reset. The settings are optional and either one or both can be specified. If both are specified then both the validations must match to accept the password.
Issue #2734

validatorPattern
Optional setting to specify a regular expression that the password must match
validatorCallback
Optional setting to specify a callback function that returns the validation result as true/false.

doNotAllowUsername

By enabling this setting, the parse server will not allow password to have username in it during signup or password reset

resetTokenValidityDuration

Currently the link that is sent to the user as result of the call to requestPasswordReset never expires. This setting allows the application to define the validity duration of these links. When this is enabled with a valid value, parse server will show invalid_link page if the link has expired.

more settings...

More work needed for additional settings under passwordPolicy. Some of them are:

maxAge: defines the duration after which the password needs to be changed.
historyLength: defines the number of recent passwords to store. The system will not allow reusing any password from this history.

validator support in passwordPolicy that can be used to enforce strong passwords.

flovilmart · 2016-11-09T14:13:46Z

src/Config.js

+  // if the passwordPolicy.validator is configured as a string or regex then convert it to a function to process the pattern
+  static setupPasswordValidator(passwordPolicy) {
+    if (passwordPolicy && passwordPolicy.validator) {
+      if (typeof passwordPolicy.validator === 'string') {


can we support a function here too? this could be useful if the user is not RegExp fluent :)

@flovilmart
Yes, it already does. I added the support after seeing your comment in a related issue :)

I suggest instead of having validator that can be a string, function, or regex, you have a regex option and a function option. Overloading interfaces by using typeoftends to get confusing IMO.

good point Drew !

@drew-gross please correct me if my understanding is wrong.

There will be two mutually exclusive options - validatorPattern (RegExp) and validatorCallback (function)
One of them should be overriding if both options are specified. I think the callback function option should take priority.

Please confirm then I will go ahead and make changes.

I would say both should be required if both are provided. So if you a provide a callback and a pattern, then the password must pass both to be considered valid. Either that or throw if both are provided.

Got your point Drew. I think it's a better idea to require both to pass if both are provided.
@flovilmart @cherukumilli what do you think?

@bhaskaryasa
I agree with @drew-gross. If both are provided then we should check both. If only one of them is provided then just check only that one.

bhaskaryasa · 2016-11-10T08:15:00Z

@flovilmart
Do you see anything missing or to be taken care of before this can be merged to the main branch?

…pdatePassword thus avoid duplicate check for password validator in updateUserPassword.

…e-server into password-policy

facebook-github-bot · 2016-11-10T11:24:40Z

@bhaskaryasa updated the pull request - view changes

facebook-github-bot · 2016-11-10T17:15:43Z

@bhaskaryasa updated the pull request - view changes

flovilmart

Small thing with the fdescribe, otherwise it's 💯!

flovilmart · 2016-11-10T17:16:55Z

spec/PasswordPolicy.spec.js

+const request = require('request');
+const Config = require('../src/Config');
+
+fdescribe("Password Policy: ", () => {


Oh sorry!! corrected that.

facebook-github-bot · 2016-11-10T17:29:31Z

@bhaskaryasa updated the pull request - view changes

flovilmart · 2016-11-10T17:36:37Z

@cherukumilli you had concerns about the naming of a column, I don't want to go over as you already contributed a lot to that section of the platform. Let me know if there's any concern, otherwise I'll merge.

cherukumilli · 2016-11-11T01:07:44Z

spec/PasswordPolicy.spec.js

+      user.set('email', '[email protected]');
+      return user.signUp();
+    })
+      .then(user => {


@bhaskaryasa
one more small nit...
Can you please combine lines 31 and 32?

cherukumilli · 2016-11-11T01:08:14Z

spec/PasswordPolicy.spec.js

+      .then(user => {
+        Parse.User.requestPasswordReset("[email protected]");
+      })
+      .then(() => {


@bhaskaryasa
one more...
Can you please combine lines 34 and 35?

cherukumilli · 2016-11-11T01:10:41Z

spec/PasswordPolicy.spec.js

@@ -0,0 +1,697 @@
+"use strict";
+
+const request = require('request');


@bhaskaryasa
Can you please userequest-promise or request-promise-native instead of just request?

cherukumilli · 2016-11-11T01:12:00Z

spec/PasswordPolicy.spec.js

+        setTimeout(() => {
+          expect(sendEmailOptions).not.toBeUndefined();
+
+          request.get(sendEmailOptions.link, {


@bhaskaryasa
request-promise provides a nice promise way of calling get.
Can u pls look into using it?

cherukumilli · 2016-11-11T01:12:37Z

spec/PasswordPolicy.spec.js

+      },
+      publicServerURL: "http://localhost:8378/1"
+    })
+      .then(() => {


@bhaskaryasa
can u pls merge the above two lines?

cherukumilli · 2016-11-11T01:12:55Z

spec/PasswordPolicy.spec.js

+        user.set('email', '[email protected]');
+        return user.signUp();
+      })
+      .then(user => {


can u pls merge the above two lines?

cherukumilli · 2016-11-11T01:13:09Z

spec/PasswordPolicy.spec.js

+      .then(user => {
+        Parse.User.requestPasswordReset("[email protected]");
+      })
+      .then(() => {


can u pls merge the above two llines?

cherukumilli · 2016-11-11T01:13:46Z

spec/PasswordPolicy.spec.js

+        setTimeout(() => {
+          expect(sendEmailOptions).not.toBeUndefined();
+
+          request.get(sendEmailOptions.link, {


can u pls use request-promise or request-promise-native here?

sure. will change to request-promise as the dependency already exists.

cherukumilli · 2016-11-11T01:14:20Z

spec/PasswordPolicy.spec.js

+      },
+      publicServerURL: "http://localhost:8378/1"
+    })
+      .then(() => {


can u please merge the two above lines?

cherukumilli · 2016-11-11T01:14:32Z

spec/PasswordPolicy.spec.js

+        fail('passwordPolicy.resetTokenValidityDuration "not a number" test failed');
+        done();
+      })
+      .catch(err => {


pls merge the above two lines

cherukumilli · 2016-11-11T01:15:26Z

spec/PasswordPolicy.spec.js

+      },
+      publicServerURL: "http://localhost:8378/1"
+    })
+      .then(() => {


can u pls merge the above two lines and apply the same change to other occurrences of it?

cherukumilli · 2016-11-11T01:17:16Z

spec/PasswordPolicy.spec.js

+      user.set('email', '[email protected]');
+      user.signUp().then(() => {
+        done();
+      }, (error) => {


can we use .catch(error => { here? and also other occurrences of this pattern?

Sure. I think I used this pattern at few other places as well. Will change them.

cherukumilli · 2016-11-11T02:29:22Z

@flovilmart
regarding the column name....
currently, password reset token is called _perishable_token. I don't think this name reflects the purpose.
@bhaskaryasa is creating a _perishable_token_expires_at based on the existing column name _perishable_token.

IMO, we should change the name of _perishable_token to _password_reset_token
I was also requesting @bhaskaryasa, if he could change the name of the new column to _password_reset_token_expires_at.

@bhaskaryasa 's concern was that we might invalidate some of the password reset links when a developer upgrades to this new version of the column name i.e., from _perishable_token to _password_reset_token.

I think the risk is pretty low, as the user can always request to have their password reset again. The new password reset token will be created in the _password_reset_token column instead of the _perishable_token.

WDYT?

…pdatePassword thus avoid duplicate check for password validator in updateUserPassword.

… Callback

facebook-github-bot · 2016-11-11T16:31:12Z

@bhaskaryasa updated the pull request - view changes

bhaskaryasa · 2016-11-11T16:46:02Z

Rebased and fixed the issue. It should be good now.

cherukumilli · 2016-11-11T18:51:42Z

.travis.yml

@@ -7,6 +7,7 @@ node_js:
 services:
  - postgresql
  - redis-server
+  - docker


@bhaskaryasa
The above changes don't appear to be yours but somehow showed up in your push.

Looks like you are pulling in changes from some other branch
or
you might have missed a rebase step

I see there are some other similar changes.

Yes the rebase step seems to be off...

facebook-github-bot · 2016-11-11T20:21:46Z

@bhaskaryasa updated the pull request - view changes

flovilmart · 2016-11-11T20:22:20Z

I've tapped the update button, seems to be good, waiting for travis to finish and merging

flovilmart · 2016-11-11T20:36:55Z

There seems to be an issue with postgres with invalid links. We can't merge it as is, otherwise we'll loose our ability to generate latest branches upon merges.

Can you have a look? I'm pretty sure this is trivial. https://travis-ci.org/ParsePlatform/parse-server/jobs/175176642#L1719

flovilmart

Failing with Postgres, can you please have a look? Seems that the invalid link expiration is not working correctly.

you can also use it_exclude_dbs(['postgres']) to mark the test as not run for postgres

facebook-github-bot · 2016-11-12T12:50:03Z

@bhaskaryasa updated the pull request - view changes

bhaskaryasa · 2016-11-12T13:02:13Z

@flovilmart Fixed the issue with postgres. I see that all tests are through now.

flovilmart · 2016-11-14T16:40:14Z

Fixes: #2734

bhaskaryasa · 2016-11-17T14:52:03Z

@flovilmart - I am working on additional settings to password-policy. Do you think this PR can be merged so that I can base my new work on top of it?

flovilmart · 2016-11-17T15:51:19Z

@cherukumilli , please merge when you think it's good!

cherukumilli · 2016-11-17T16:26:03Z

@flovilmart
I reviewed it and it looks good. I will go ahead and merge it.

* adds resetTokenValidityDuration setting * adds a validator to validate password that can be used to enforce strong passwords * adds unit tests for passwordPolicy.validator * adds unit tests to to fail reset password function if password is not in a valid format * updates README.md for passwordPolicy * prevents duplicate check for password validator in updateUserPassword * adds optional setting to disallow username in password * updates test cases to use fdescribe instead of describe * updates test cases to use request-promise instead of request * adds ability to use a RegExp or Callback function or both for a passwordPolicy.validator * expect username parameter in redirect to password_reset_success * adds support for _perishable_token_expires_at in postgres

bhaskaryasa added 5 commits November 7, 2016 12:08

Introducing passwordPolicy with resetTokenValidityDuration

05cc0d9

validator added to passwordPolicy

bbd5d37

validator support in passwordPolicy that can be used to enforce strong passwords.

Add some unit tests for passwordPolicy.validator

96920bd

Add unit test for reset password failure for non-conformance

e7307be

Update README.md for passwordPolicy

3206b34

facebook-github-bot added the GH Review: review-needed label Nov 9, 2016

flovilmart reviewed Nov 9, 2016

View reviewed changes

bhaskaryasa added 2 commits November 10, 2016 16:38

Added code to handle Parse.Error from rest.update in UserController.u…

052d6eb

…pdatePassword thus avoid duplicate check for password validator in updateUserPassword.

Merge branch 'password-policy' of https://github.com/bhaskaryasa/pars…

6bbdbbe

…e-server into password-policy

Added optional setting to disallow username in password

838d7cb

flovilmart suggested changes Nov 10, 2016

View reviewed changes

fdescribe -> describe

54bf4d1

cherukumilli reviewed Nov 11, 2016

View reviewed changes

bhaskaryasa added 8 commits November 11, 2016 21:40

Added code to handle Parse.Error from rest.update in UserController.u…

1c1a515

…pdatePassword thus avoid duplicate check for password validator in updateUserPassword.

Added optional setting to disallow username in password

bd1673d

fdescribe -> describe

45ee8b5

updated PasswordPolicy.spec.js to use request-promise

f7ce2c7

passwordPolicy.validator split into two separate options - RegExp and…

838eb27

… Callback

fixed some typos

a9f55f8

expect username parameter in redirect to password_reset_success

7401000

pull from origin

4ce59aa

cherukumilli reviewed Nov 11, 2016

View reviewed changes

flovilmart approved these changes Nov 11, 2016

View reviewed changes

flovilmart suggested changes Nov 11, 2016

View reviewed changes

bhaskaryasa added 2 commits November 12, 2016 15:49

Fix postgres issue for _perishable_token_expires_at

9ed141c

fix for _perishable_token_expires_at

72a0670

flovilmart approved these changes Nov 14, 2016

View reviewed changes

flovilmart assigned cherukumilli Nov 17, 2016

cherukumilli merged commit cf6ce5b into parse-community:master Nov 17, 2016

bhaskaryasa deleted the password-policy branch December 11, 2016 01:18

		@@ -0,0 +1,697 @@
		"use strict";

		const request = require('request');

Adding support for optional Password Policy #3032

Adding support for optional Password Policy #3032

Conversation

bhaskaryasa commented Nov 9, 2016 • edited Loading

validators

doNotAllowUsername

resetTokenValidityDuration

more settings...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaskaryasa commented Nov 10, 2016

facebook-github-bot commented Nov 10, 2016

facebook-github-bot commented Nov 10, 2016

flovilmart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 10, 2016

flovilmart commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cherukumilli Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cherukumilli commented Nov 11, 2016

facebook-github-bot commented Nov 11, 2016

bhaskaryasa commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 11, 2016

flovilmart commented Nov 11, 2016

flovilmart commented Nov 11, 2016 • edited Loading

flovilmart left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 12, 2016

bhaskaryasa commented Nov 12, 2016

flovilmart commented Nov 14, 2016

bhaskaryasa commented Nov 17, 2016

flovilmart commented Nov 17, 2016

cherukumilli commented Nov 17, 2016

bhaskaryasa commented Nov 9, 2016 •

edited

Loading

cherukumilli Nov 11, 2016 •

edited

Loading

flovilmart commented Nov 11, 2016 •

edited

Loading