Adds password history support to passwordPolicy #3102

bhaskaryasa · 2016-11-23T10:24:17Z

passwordPolicy.passwordHistory

Optional setting to specify the number of passwords to retain in history and prevent reusing the same during password reset. The maximum value is 20.

a new Array field _old_passwords is added to _User object to store the old passwords (hash values) whenever password is changed.

facebook-github-bot · 2016-11-23T10:31:42Z

@bhaskaryasa updated the pull request - view changes

cherukumilli · 2016-11-23T17:08:47Z

README.md

@@ -288,6 +288,7 @@ var server = ParseServer({
    validatorCallback: (password) => { return validatePassword(password) }, 
    doNotAllowUsername: true, // optional setting to disallow username in passwords
    maxPasswordAge: 90, // optional setting in days for password expiry. Login fails if user does not reset the password within this period after signup/last reset. 
+    passwordHistory: 5, // optional setting to prevent reuse of previous n passwords. Maximum value that can be specified is 20. Specifying 0 will not enforce history.


@bhaskaryasa
Is the default value 0?
If so, it will be good if you can say that in the Readme.

@cherukumilli - It is optional so default is 'undefined' and does not enforce history. Behavior by setting to 0 would be similar. I will update readme accordingly.

cherukumilli · 2016-11-23T17:47:35Z

src/RestWrite.js

@@ -399,8 +399,39 @@ RestWrite.prototype.transformUser = function() {
            });
        }
      }
+      // check whether password is repeating from specified history


@bhaskaryasa
I think this entire transformUser method should be refactored (even before you changes to it). I think we should use this opportunity to clean it up. WDYT?

Yes. I too felt its become somewhat big and kind of difficult to comprehend. Too many things going on there, more so after I added passwordPolicy. I will try and refactor it.

facebook-github-bot · 2016-11-24T09:49:56Z

@bhaskaryasa updated the pull request - view changes

bhaskaryasa · 2016-11-24T10:11:04Z

@cherukumilli - I have committed changes after some refactoring on transformUser. I hope the PR is good to merge.

cherukumilli · 2016-11-28T23:15:12Z

README.md

@@ -288,6 +288,7 @@ var server = ParseServer({
    validatorCallback: (password) => { return validatePassword(password) }, 
    doNotAllowUsername: true, // optional setting to disallow username in passwords
    maxPasswordAge: 90, // optional setting in days for password expiry. Login fails if user does not reset the password within this period after signup/last reset. 
+    passwordHistory: 5, // optional setting to prevent reuse of previous n passwords. Maximum value that can be specified is 20. Not specifying it or specifying 0 will not enforce history.


@bhaskaryasa
WDYT about using maxPasswordHistory instead of passwordHistory here.

Agree. Will change it.

cherukumilli · 2016-11-28T23:18:41Z

src/Adapters/Storage/Mongo/MongoTransform.js

@@ -786,6 +786,7 @@ const mongoObjectToParseObject = (className, mongoObject, schema) => {
      case '_email_verify_token_expires_at':
      case '_account_lockout_expires_at':
      case '_failed_login_count':
+      case '_old_passwords':


@bhaskaryasa
As you are storing password history in this column name, I think _password_history might be a better name for this variable instead of _old_passwords. It also flows with the entire feature.
WDYT?

I used this name to avoid conflict with "passwordHistory" name used for configuration which is numeric. As we are changing it to "maxPasswordHistory" now, I can rename _old_passwords to _password_history.

cherukumilli · 2016-11-29T00:25:32Z

spec/PasswordPolicy.spec.js

+
+  it('should fail if the new password is same as the last password', done => {
+    const user = new Parse.User();
+    const emailAdapter = {


@bhaskaryasa
Do we really need an email adapter here? I think this test case can be simplified to something like:

it('should fail if the new password is same as the last password', done => { reconfigureServer({ appName: 'passwordPolicy', passwordPolicy: { passwordHistory: 1 }, }).then(() => { Parse.User.signUp('user1', 'password1').then(user1 => { user1.setPassword('password1'); return user1.save(); }).then(() => { // cannot set the same password twice fail(); }).catch(err => { expect(err.message).to.equal('New password should not be the same as last 1 passwords.'); expect(err.code).to.equal(142); // 142 might have to be replaced by Parse.Error.xxx done(); }); }); });

WDYT?

Yes, you are right. Somehow I was thinking of only resetPassword as the way to change password in which case you will need to go through emailAdapter. It skipped my mind that password can be changed by the user after signing in. I will change the test cases.

cherukumilli · 2016-11-29T00:27:43Z

spec/PasswordPolicy.spec.js

+  it('should fail if the new password is same as 5th oldest password in history when policy does not allow last 5 passwords', done => {
+    const user = new Parse.User();
+    let pwCount = 1;
+    const emailAdapter = {


@bhaskaryasa
same comment as in line 1064 above.
I think not using an emailAdapter will simplify this entire test case.

cherukumilli · 2016-11-29T00:51:12Z

src/RestWrite.js

@@ -353,14 +353,16 @@ RestWrite.prototype.transformUser = function() {
  if (this.query) {


@bhaskaryasa
Pls check the beginning of this method (line 347). There is an if statement like:

if (this.className !== '_User') { return; }

Should this return a promise?

I know this is not your code and it is the original code but I think we should correct it if it is incorrect. WDYT?

I think this code simply relies on the fact that then() resolves to SUCCESS if the handler returns undefined. It would be cleaner to return a properly resolved promise.

cherukumilli · 2016-11-29T01:27:43Z

src/RestWrite.js

@@ -369,96 +371,143 @@ RestWrite.prototype.transformUser = function() {
      return;


@bhaskaryasa
On line 370, we have code like:

if (!this.data.password) { return; }

I think we should move this code up to the beginning. Also, it should return a promise. WDYT?

We can't move it up because this section only handles password change. This should not impact if other fields are changed without touching the password.

Will add code to return a resolved promise.

Refactor RestWrite.transformUser

facebook-github-bot · 2016-11-29T06:09:07Z

@bhaskaryasa updated the pull request - view changes

facebook-github-bot · 2016-11-29T06:26:36Z

@bhaskaryasa updated the pull request - view changes

facebook-github-bot · 2016-11-29T07:52:05Z

@bhaskaryasa updated the pull request - view changes

- renamed field "_old_passwords" to "_password_history" - simplified passwordHistory test cases without using emailAdapter

facebook-github-bot · 2016-11-29T08:00:49Z

@bhaskaryasa updated the pull request - view changes

bhaskaryasa · 2016-11-29T08:15:21Z

@cherukumilli - Please review the update. I retained one test case for passwordHistory that uses emailAdapter through requestResetPassword but redone others.

cherukumilli

@bhaskaryasa
LGTM

facebook-github-bot · 2016-11-29T16:17:47Z

@bhaskaryasa updated the pull request - view changes

* password history support in passwordPolicy * Refactor RestWrite.transformUser * fix eslint issues

yomybaby · 2018-04-03T02:28:38Z

@bhaskaryasa I have one questions.

When User query, it return a result with _password_history event without any key :
Is there any reason do not delete _password_history filed on filterSensitiveData function?

flovilmart · 2018-04-03T12:08:31Z

This has been overlooked. Do yo want to make a PR to rectify it?

yomybaby · 2018-04-04T01:16:52Z

@flovilmart OK, I'll make a PR tonight. :)

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

cherukumilli self-assigned this Nov 23, 2016

cherukumilli reviewed Nov 23, 2016

View reviewed changes

cherukumilli reviewed Nov 28, 2016

View reviewed changes

cherukumilli reviewed Nov 29, 2016

View reviewed changes

password history support in passwordPolicy

cc57c6b

Refactor RestWrite.transformUser

fix eslint issues

9f5bac3

- renamed "passwordHistory" setting to "maxPasswordHistory"

75d28f6

- renamed field "_old_passwords" to "_password_history" - simplified passwordHistory test cases without using emailAdapter

cherukumilli approved these changes Nov 29, 2016

View reviewed changes

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

11b6bcd

cherukumilli merged commit bd16891 into parse-community:master Nov 29, 2016

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

Adds password history support to passwordPolicy (parse-community#3102)

79e74a3

* password history support in passwordPolicy * Refactor RestWrite.transformUser * fix eslint issues

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

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

Adds password history support to passwordPolicy (parse-community#3102)

9dbc35b

* password history support in passwordPolicy * Refactor RestWrite.transformUser * fix eslint issues

yomybaby mentioned this pull request Apr 5, 2018

add _password_history to default sensitive data #4699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds password history support to passwordPolicy #3102

Adds password history support to passwordPolicy #3102

bhaskaryasa commented Nov 23, 2016

facebook-github-bot commented Nov 23, 2016

cherukumilli Nov 23, 2016

bhaskaryasa Nov 24, 2016

cherukumilli Nov 23, 2016

bhaskaryasa Nov 24, 2016

facebook-github-bot commented Nov 24, 2016

bhaskaryasa commented Nov 24, 2016

cherukumilli Nov 28, 2016

bhaskaryasa Nov 29, 2016

cherukumilli Nov 28, 2016

bhaskaryasa Nov 29, 2016

cherukumilli Nov 29, 2016

bhaskaryasa Nov 29, 2016

cherukumilli Nov 29, 2016

cherukumilli Nov 29, 2016 •

edited

Loading

bhaskaryasa Nov 29, 2016

cherukumilli Nov 29, 2016

bhaskaryasa Nov 29, 2016

facebook-github-bot commented Nov 29, 2016

facebook-github-bot commented Nov 29, 2016

facebook-github-bot commented Nov 29, 2016

facebook-github-bot commented Nov 29, 2016

bhaskaryasa commented Nov 29, 2016

cherukumilli left a comment

facebook-github-bot commented Nov 29, 2016

yomybaby commented Apr 3, 2018

flovilmart commented Apr 3, 2018

yomybaby commented Apr 4, 2018

		@@ -353,14 +353,16 @@ RestWrite.prototype.transformUser = function() {
		if (this.query) {

		@@ -369,96 +371,143 @@ RestWrite.prototype.transformUser = function() {
		return;

Adds password history support to passwordPolicy #3102

Adds password history support to passwordPolicy #3102

Conversation

bhaskaryasa commented Nov 23, 2016

passwordPolicy.passwordHistory

facebook-github-bot commented Nov 23, 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

facebook-github-bot commented Nov 24, 2016

bhaskaryasa commented Nov 24, 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

cherukumilli Nov 29, 2016 • edited Loading

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

facebook-github-bot commented Nov 29, 2016

facebook-github-bot commented Nov 29, 2016

facebook-github-bot commented Nov 29, 2016

facebook-github-bot commented Nov 29, 2016

bhaskaryasa commented Nov 29, 2016

cherukumilli left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 29, 2016

yomybaby commented Apr 3, 2018

flovilmart commented Apr 3, 2018

yomybaby commented Apr 4, 2018

cherukumilli Nov 29, 2016 •

edited

Loading