Adds password expiry support to password policy #3068

bhaskaryasa · 2016-11-18T07:17:17Z

passwordPolicy.daysBeforeExpiry

Optional setting, specifies the duration in days after which the password expires.

Login after expiry will throw an error with code OBJECT_NOT_FOUND and message 'Your password has expired. Please reset your password.'

Added a timestamp field (_password_changed_at) to _User object which is used as reference to calculate expiry date. The timestamp is populated during signup or password reset.

For users signedup before this policy is enabled, the timestamp is updated at the time of first login after it is enabled.

facebook-github-bot · 2016-11-18T12:31:29Z

@bhaskaryasa updated the pull request - view changes

cherukumilli · 2016-11-18T21:52:43Z

README.md

@@ -287,6 +287,7 @@ var server = ParseServer({
    // 2. a callback function to be invoked to validate the password  
    validatorCallback: (password) => { return validatePassword(password) }, 
    doNotAllowUsername: true, // optional setting to disallow username in passwords
+    daysBeforeExpiry: 90, // threshold (days) for password expiry. Login fails if user does not reset the password within this period after signup/last reset. 


@bhaskaryasa
Can you pls change daysBeforeExpiry to
maximumPasswordAge? Pls refer to https://technet.microsoft.com/en-us/library/ff741764.aspx for good naming conventions for these variable names.

Yes, I have seen that. I think the name specified there may not be really a best-practice but how it's label is displayed in the configuration screen (in windows group policies).

I have chosen this name because you immediately know that the number is in "days". I have no particular preference though. Let me know if you really think it should be changed.

@bhaskaryasa
daysBeforeExpiry to me sounds like you are keeping a counter instead of it being a constant i.e., lets say I changed my password today. If this value is say: 90 days. daysBeforeExpiry name feels like if tomorrow the value be 89 days and keeps decrementing.

If you pls don't mind then I would really appreciate if u can change it to maximumPasswordAge

Sure. Will change it :)

cherukumilli · 2016-11-18T21:55:32Z

spec/PasswordPolicy.spec.js

@@ -458,7 +460,7 @@ describe("Password Policy: ", () => {
            resolveWithFullResponse: true
          }).then((response) => {
            expect(response.statusCode).toEqual(302);
-            expect(response.body).toEqual(`Found. Redirecting to http://localhost:8378/1/apps/choose_password?username=user1&token=${token}&id=test&error=Password%20does%20not%20confirm%20to%20the%20Password%20Policy.&app=passwordPolicy`);
+            expect(response.body).toEqual(`Found. Redirecting to http://localhost:8378/1/apps/choose_password?username=user1&token=${token}&id=test&error=Password%20does%20not%20conform%20to%20the%20Password%20Policy.&app=passwordPolicy`);


@bhaskaryasa

one small nit :)

is the following a better error message?
Password does not meet the Password Policy requirements ?

Yes. Looks better :)

cherukumilli · 2016-11-18T22:01:06Z

spec/PasswordPolicy.spec.js

+    reconfigureServer({
+      appName: 'passwordPolicy',
+      passwordPolicy: {
+        daysBeforeExpiry: 0.000005787 // 0.5 sec


@bhaskaryasa
another small nit...

can you pls use
0.5/(24*60*60)
instead of
0.000005787

cherukumilli · 2016-11-18T22:08:33Z

spec/PasswordPolicy.spec.js

+      user.setPassword("user1");
+      user.set('email', '[email protected]');
+      user.signUp().then(() => {
+        Parse.User.logOut();


@bhaskaryasa
This call returns a promise btw. Any reason we shouldn't wait for the call here?

Yes we should. Thanks for the close look :)

cherukumilli · 2016-11-18T22:10:40Z

spec/PasswordPolicy.spec.js

+          },
+          publicServerURL: "http://localhost:8378/1"
+        }).then(() => {
+          Parse.User.logIn("user1", "user1").then((u) => {


@bhaskaryasa
Shouldn't this first login call fail? (as this is the first login call after setting the new password policy?)

No because there was no password timestamp to check against. The timestamp is created at the first login for existing accounts.

cherukumilli · 2016-11-18T22:14:34Z

spec/PasswordPolicy.spec.js

+          publicServerURL: "http://localhost:8378/1"
+        }).then(() => {
+          Parse.User.logIn("user1", "user1").then((u) => {
+            Parse.User.logOut();


@bhaskaryasa
This again returns a promise.
Also, to reduce nesting calls, you should be able to return and handle most of the test case in the following manner:

call().then(() => { return promise1; // like Parse.User.logOut(); }).then(() => { return promise2; // like reconfig }).then(() => { // do something else }).catch(err => { // handle error });

I know this is more of a coding style preference but it will make it more readable...my 2 cents :)

I agree chaining is better practice in general. It also allows us to handle errors at one place. However it may be better to handle separately in cases where the test cases expect a mix of SUCCESS and FAILURE resolutions in order to display appropriate messages.

I will take a look again and refactor the pieces that make sense.

cherukumilli · 2016-11-18T22:20:52Z

spec/PasswordPolicy.spec.js

+  });
+
+  it('should reset password timestamp when password is reset', done => {
+    var user = new Parse.User();


@bhaskaryasa
Can you pls use let instead of var for better scoping of variables?

cherukumilli · 2016-11-18T22:27:55Z

spec/PasswordPolicy.spec.js

+          }).catch((error) => {
+            expect(error.code).toEqual(Parse.Error.OBJECT_NOT_FOUND);
+            expect(error.message).toEqual('Your password has expired. Please reset your password.');
+            Parse.User.requestPasswordReset('[email protected]', {


@bhaskaryasa
Any reason we can't use the promise returned by this call instead of a function callback?

javascript sdk documentation shows requestPasswordReset does NOT return a promise. I will check the code to verify.

After looking through the code I figured that the call does return a Promise so modified the test cases accordingly.

Perhaps the Javascript client sdk documentation needs correction.

cherukumilli · 2016-11-18T22:29:52Z

spec/PasswordPolicy.spec.js

+              error: (err) => {
+                jfail(err);
+                fail("Reset password request should not fail");
+                done();


@bhaskaryasa
I don't see you checking for the reset password timestamp
Can you pls check the DB to make sure the _password_changed_at for the password has been updated?

The test case follows through the requestPasswordReset and ensures that the user is able to login after resetting the password. This implies that the password timestamp was updated when password was reset.

cherukumilli

@bhaskaryasa
I added a bunch of comments :)

Please review them and pls let us know if u disagree with any of them.

facebook-github-bot · 2016-11-19T10:03:26Z

@bhaskaryasa updated the pull request - view changes

cherukumilli · 2016-11-19T16:15:19Z

spec/PasswordPolicy.spec.js

@@ -29,7 +29,11 @@ describe("Password Policy: ", () => {
      user.set('email', '[email protected]');
      return user.signUp();
    }).then(user => {
-      Parse.User.requestPasswordReset("[email protected]");
+      Parse.User.requestPasswordReset('[email protected]').catch((err) => {


@bhaskaryasa
This should just be a simple
return Parse.User.requestPasswordReset("[email protected]");

You already have a catch handler at the bottom. Any errors will be caught by the catch at the bottom of the test case.
Pls let me know if I am missing something here.

@cherukumilli
I understand that. But the point is to be able to highlight exactly which branch has caused the error and log appropriate message.

@bhaskaryasa
I think we should use the DRY principle here as in if there is a another place where this kind of requestPasswordReset test is already tested then you don't need to test again in your code.

This will make the whole testing process much faster.

In this case requestPasswordReset is tested in test cases outside of your code already. Those basic test cases will fail if there is a problem with your code. That is the reason I was saying we can catch it in the one catch block you have.

thoughts?

I am not testing basic requestPasswordReset functionality here. Only incremental functionality is being tested. For example I am testing whether the password expiry is being reset after password is changed.

Do you mean I should avoid checking whether requestPasswordReset succeeded?

@bhaskaryasa
LGTM then

cherukumilli · 2016-11-19T16:16:37Z

spec/PasswordPolicy.spec.js

-        }).catch((err) => {
-          jfail(err);
-          fail("Should be able to login");
+        Parse.User.logOut().then(() => {


@bhaskaryasa
I think u should just do a
return Parse.User.logOut(); here

@cherukumilli
Please see my earlier comment

cherukumilli · 2016-11-19T16:17:10Z

spec/PasswordPolicy.spec.js

-          jfail(err);
-          fail("Should be able to login");
+        Parse.User.logOut().then(() => {
+          Parse.User.logIn("user1", "1digit").then(function (user) {


same here
return Parse.User.logIn("user1", "1digit");

I see similar patterns in other test cases below.
I think the code will be much more readable if we don't nest the promises. :)

@cherukumilli
Please see my earlier comment

facebook-github-bot · 2016-11-20T01:15:00Z

@bhaskaryasa updated the pull request - view changes

bhaskaryasa · 2016-11-20T01:30:46Z

@cherukumilli - Updated the PR with more commits. Please check.

facebook-github-bot · 2016-11-20T23:33:59Z

@bhaskaryasa updated the pull request - view changes

facebook-github-bot · 2016-11-21T00:07:24Z

@bhaskaryasa updated the pull request - view changes

flovilmart · 2016-11-21T00:43:15Z

@cherukumilli the PG part is failing because of that #3074 , there is a PR to resolve that: #3084

flovilmart · 2016-11-21T14:38:26Z

@cherukumilli is that ok for you? You can merge ahead ;)

facebook-github-bot · 2016-11-21T15:01:00Z

@bhaskaryasa updated the pull request - view changes

facebook-github-bot · 2016-11-21T15:01:02Z

@bhaskaryasa updated the pull request - view changes

cherukumilli · 2016-11-21T15:01:02Z

@flovilmart
I will merge as soon as travis is done. :)

* Adding support for password expiry policy * Renamed daysBeforeExpiry -> maxPasswordAge

Adding support for password expiry policy

ea81f33

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

bhaskaryasa changed the title ~~Adds support for password expiry support to password policy~~ Adds password expiry support to password policy Nov 18, 2016

Corrected typo 'confirm' to 'conform'

bde1387

cherukumilli reviewed Nov 18, 2016

View reviewed changes

cherukumilli suggested changes Nov 18, 2016

View reviewed changes

changes suggested by @cherukumilli

a1acbb2

cherukumilli reviewed Nov 19, 2016

View reviewed changes

Renamed daysBeforeExpiry -> maxPasswordAge

952caf2

Merge branch 'master' into password-policy--expiry

e488d58

Merge branch 'master' into password-policy--expiry

fb0c638

cherukumilli mentioned this pull request Nov 21, 2016

Adds support for postgres when saving relation #3074 #3084

Merged

cherukumilli added the GH Review: accepted label Nov 21, 2016

cherukumilli removed the GH Review: review-needed label Nov 21, 2016

Merge branch 'master' into password-policy--expiry

d2091a9

cherukumilli merged commit edb7b70 into parse-community:master Nov 21, 2016

rsouzas pushed a commit to back4app/parse-server that referenced this pull request Dec 3, 2016

Adds password expiry support to password policy (parse-community#3068)

77e8c9e

* Adding support for password expiry policy * Renamed daysBeforeExpiry -> maxPasswordAge

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

Jcarlosjunior pushed a commit to back4app/parse-server that referenced this pull request Dec 13, 2016

Adds password expiry support to password policy (parse-community#3068)

1a489c5

* Adding support for password expiry policy * Renamed daysBeforeExpiry -> maxPasswordAge

Uh oh!

Adds password expiry support to password policy #3068

Adds password expiry support to password policy #3068

Uh oh!

Conversation

bhaskaryasa commented Nov 18, 2016

passwordPolicy.daysBeforeExpiry

Uh oh!

facebook-github-bot commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cherukumilli Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cherukumilli Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cherukumilli left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cherukumilli Nov 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

cherukumilli Nov 18, 2016 •

edited

Loading

cherukumilli Nov 18, 2016 •

edited

Loading

cherukumilli Nov 19, 2016 •

edited

Loading