Skip to content

Adds password history support to passwordPolicy #3102

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

Merged
merged 4 commits into from
Nov 29, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
maxPasswordHistory: 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.
//optional setting to set a validity duration for password reset links (in seconds)
resetTokenValidityDuration: 24*60*60, // expire after 24 hours
}
Expand Down
238 changes: 238 additions & 0 deletions spec/PasswordPolicy.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1010,4 +1010,242 @@ describe("Password Policy: ", () => {
});
});

it('should fail if passwordPolicy.maxPasswordHistory is not a number', done => {
reconfigureServer({
appName: 'passwordPolicy',
passwordPolicy: {
maxPasswordHistory: "not a number"
},
publicServerURL: "http://localhost:8378/1"
}).then(() => {
fail('passwordPolicy.maxPasswordHistory "not a number" test failed');
done();
}).catch(err => {
expect(err).toEqual('passwordPolicy.maxPasswordHistory must be an integer ranging 0 - 20');
done();
});
});

it('should fail if passwordPolicy.maxPasswordHistory is a negative number', done => {
reconfigureServer({
appName: 'passwordPolicy',
passwordPolicy: {
maxPasswordHistory: -10
},
publicServerURL: "http://localhost:8378/1"
}).then(() => {
fail('passwordPolicy.maxPasswordHistory negative number test failed');
done();
}).catch(err => {
expect(err).toEqual('passwordPolicy.maxPasswordHistory must be an integer ranging 0 - 20');
done();
});
});

it('should fail if passwordPolicy.maxPasswordHistory is greater than 20', done => {
reconfigureServer({
appName: 'passwordPolicy',
passwordPolicy: {
maxPasswordHistory: 21
},
publicServerURL: "http://localhost:8378/1"
}).then(() => {
fail('passwordPolicy.maxPasswordHistory negative number test failed');
done();
}).catch(err => {
expect(err).toEqual('passwordPolicy.maxPasswordHistory must be an integer ranging 0 - 20');
done();
});
});

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

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

sendVerificationEmail: () => Promise.resolve(),
sendPasswordResetEmail: options => {
requestp.get({
uri: options.link,
followRedirect: false,
simple: false,
resolveWithFullResponse: true
}).then(response => {
expect(response.statusCode).toEqual(302);
const re = /http:\/\/localhost:8378\/1\/apps\/choose_password\?token=([a-zA-Z0-9]+)\&id=test\&username=user1/;
const match = response.body.match(re);
if (!match) {
fail("should have a token");
return Promise.reject("Invalid password link");
}
return Promise.resolve(match[1]); // token
}).then(token => {
return new Promise((resolve, reject) => {
requestp.post({
uri: "http://localhost:8378/1/apps/test/request_password_reset",
body: `new_password=user1&token=${token}&username=user1`,
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
},
followRedirect: false,
simple: false,
resolveWithFullResponse: true
}).then(response => {
resolve([response, token]);
}).catch(error => {
reject(error);
});
});
}).then(data => {
const response = data[0];
const token = data[1];
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=New%20password%20should%20not%20be%20the%20same%20as%20last%201%20passwords.&app=passwordPolicy`);
done();
return Promise.resolve();
}).catch(error => {
jfail(error);
fail("Repeat password test failed");
done();
});
},
sendMail: () => {
}
};
reconfigureServer({
appName: 'passwordPolicy',
verifyUserEmails: false,
emailAdapter: emailAdapter,
passwordPolicy: {
maxPasswordHistory: 1
},
publicServerURL: "http://localhost:8378/1"
}).then(() => {
user.setUsername("user1");
user.setPassword("user1");
user.set('email', '[email protected]');
user.signUp().then(() => {
return Parse.User.logOut();
}).then(() => {
return Parse.User.requestPasswordReset('[email protected]');
}).catch(error => {
jfail(error);
fail("SignUp or reset request failed");
done();
});
});
});


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

reconfigureServer({
appName: 'passwordPolicy',
verifyUserEmails: false,
passwordPolicy: {
maxPasswordHistory: 5
},
publicServerURL: "http://localhost:8378/1"
}).then(() => {
user.setUsername("user1");
user.setPassword("user1");
user.set('email', '[email protected]');
user.signUp().then(() => {
// try to set the same password as the previous one
user.setPassword('user1');
return user.save();
}).then(() => {
fail("should have failed because the new password is same as the old");
done();
}).catch(error => {
expect(error.message).toEqual('New password should not be the same as last 5 passwords.');
expect(error.code).toEqual(Parse.Error.VALIDATION_ERROR);
done();
});
});
});

it('should fail if the new password is same as the 5th oldest one and policy does not allow the previous 5', done => {
const user = new Parse.User();

reconfigureServer({
appName: 'passwordPolicy',
verifyUserEmails: false,
passwordPolicy: {
maxPasswordHistory: 5
},
publicServerURL: "http://localhost:8378/1"
}).then(() => {
user.setUsername("user1");
user.setPassword("user1");
user.set('email', '[email protected]');
user.signUp().then(() => {
// build history
user.setPassword('user2');
return user.save();
}).then(() => {
user.setPassword('user3');
return user.save();
}).then(() => {
user.setPassword('user4');
return user.save();
}).then(() => {
user.setPassword('user5');
return user.save();
}).then(() => {
// set the same password as the initial one
user.setPassword('user1');
return user.save();
}).then(() => {
fail("should have failed because the new password is same as the old");
done();
}).catch(error => {
expect(error.message).toEqual('New password should not be the same as last 5 passwords.');
expect(error.code).toEqual(Parse.Error.VALIDATION_ERROR);
done();
});
});
});

it('should succeed if the new password is same as the 6th oldest one and policy does not allow only previous 5', done => {
const user = new Parse.User();

reconfigureServer({
appName: 'passwordPolicy',
verifyUserEmails: false,
passwordPolicy: {
maxPasswordHistory: 5
},
publicServerURL: "http://localhost:8378/1"
}).then(() => {
user.setUsername("user1");
user.setPassword("user1");
user.set('email', '[email protected]');
user.signUp().then(() => {
// build history
user.setPassword('user2');
return user.save();
}).then(() => {
user.setPassword('user3');
return user.save();
}).then(() => {
user.setPassword('user4');
return user.save();
}).then(() => {
user.setPassword('user5');
return user.save();
}).then(() => {
user.setPassword('user6'); // this pushes initial password out of history
return user.save();
}).then(() => {
// set the same password as the initial one
user.setPassword('user1');
return user.save();
}).then(() => {
done();
}).catch(() => {
fail("should have succeeded because the new password is not in history");
done();
});
});
});
})
1 change: 1 addition & 0 deletions src/Adapters/Storage/Mongo/MongoTransform.js
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,7 @@ const mongoObjectToParseObject = (className, mongoObject, schema) => {
case '_email_verify_token_expires_at':
case '_account_lockout_expires_at':
case '_failed_login_count':
case '_password_history':
// Those keys will be deleted if needed in the DB Controller
restObject[key] = mongoObject[key];
break;
Expand Down
5 changes: 4 additions & 1 deletion src/Adapters/Storage/Postgres/PostgresStorageAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ const toPostgresSchema = (schema) => {
schema.fields._rperm = {type: 'Array', contents: {type: 'String'}}
if (schema.className === '_User') {
schema.fields._hashed_password = {type: 'String'};
schema.fields._password_history = {type: 'Array'};
}
return schema;
}
Expand Down Expand Up @@ -471,6 +472,7 @@ export class PostgresStorageAdapter {
fields._perishable_token = {type: 'String'};
fields._perishable_token_expires_at = {type: 'Date'};
fields._password_changed_at = {type: 'Date'};
fields._password_history = { type: 'Array'};
}
let index = 2;
let relations = [];
Expand Down Expand Up @@ -683,7 +685,8 @@ export class PostgresStorageAdapter {
if (!schema.fields[fieldName] && className === '_User') {
if (fieldName === '_email_verify_token' ||
fieldName === '_failed_login_count' ||
fieldName === '_perishable_token') {
fieldName === '_perishable_token' ||
fieldName === '_password_history'){
valuesArray.push(object[fieldName]);
}

Expand Down
4 changes: 4 additions & 0 deletions src/Config.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ export class Config {
if(passwordPolicy.doNotAllowUsername && typeof passwordPolicy.doNotAllowUsername !== 'boolean') {
throw 'passwordPolicy.doNotAllowUsername must be a boolean value.';
}

if (passwordPolicy.maxPasswordHistory && (!Number.isInteger(passwordPolicy.maxPasswordHistory) || passwordPolicy.maxPasswordHistory <= 0 || passwordPolicy.maxPasswordHistory > 20)) {
throw 'passwordPolicy.maxPasswordHistory must be an integer ranging 0 - 20';
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Controllers/DatabaseController.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ const filterSensitiveData = (isMaster, aclGroup, className, object) => {
// acl: a list of strings. If the object to be updated has an ACL,
// one of the provided strings must provide the caller with
// write permissions.
const specialKeysForUpdate = ['_hashed_password', '_perishable_token', '_email_verify_token', '_email_verify_token_expires_at', '_account_lockout_expires_at', '_failed_login_count', '_perishable_token_expires_at', '_password_changed_at'];
const specialKeysForUpdate = ['_hashed_password', '_perishable_token', '_email_verify_token', '_email_verify_token_expires_at', '_account_lockout_expires_at', '_failed_login_count', '_perishable_token_expires_at', '_password_changed_at', '_password_history'];

const isSpecialUpdateKey = key => {
return specialKeysForUpdate.indexOf(key) >= 0;
Expand Down
Loading