-
Notifications
You must be signed in to change notification settings - Fork 489
Password Policy Implementation #1611
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
base: development
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| const { chai, mocha, app } = require('../common'); | ||
|
huss marked this conversation as resolved.
|
||
| const expect = chai.expect; | ||
|
|
||
| const { validatePasswordPolicy } = require('../../util/validatePassword'); | ||
|
|
||
| mocha.describe('Password Policy Validation', () => { | ||
|
|
||
| mocha.it('Rejects short passwords', () => { | ||
| const result = validatePasswordPolicy('short', 'user1', 'user'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "user" is not a valid user role. While it isn't "admin", and I think that is what is intended, it really should use valid roles. Note User.role has the allowed roles that would be good to use here and probably as a test in the actual function. Also, should this test 7 characters since that is right on the border. Even better if the other file had exported constants that would define these values and then used here. Along those lines, the new, stronger parameter testing on routes has PASSWORD_MAX_LENGTH in src/server/routes/obvius.js. I think the value you have and this needs to be tied together. This probably means making the tested value be dependent on the role and some other changes. A sweep of all places using a password and length test should be made. Note some tests below have similar considerations. |
||
| expect(result).to.be.a('string'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this and the other tests: I think that one could get more info and test the result in a stronger way. See other file about providing more feedback for the user that might help here. |
||
| }); | ||
|
|
||
| mocha.it('Rejects admin passwords under 14 chars', () => { | ||
| const result = validatePasswordPolicy('shortpassword', 'admin1', 'admin'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth a comment that the password is 13 characters - just under the min needed. If this converts to using consts for length then it should generate the correct length of password to try based on that. |
||
| expect(result).to.include('14'); | ||
| }); | ||
|
|
||
| mocha.it('Rejects passwords containing username', () => { | ||
| const result = validatePasswordPolicy('user1password', 'user1', 'user'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this be a stronger test if the user name was in the middle of the password and not at the start? |
||
| expect(result).to.include('username'); | ||
| }); | ||
|
|
||
| mocha.it('Rejects weak passwords (zxcvbn)', () => { | ||
| const result = validatePasswordPolicy('aaaaaaaa', 'user1', 'user'); | ||
| expect(result).to.include('weak'); | ||
| }); | ||
|
|
||
| mocha.it('Accepts strong passwords', () => { | ||
| const result = validatePasswordPolicy('StrongPassphrase123!', 'user1', 'user'); | ||
| expect(result).to.equal(null); | ||
| }); | ||
|
|
||
| }); | ||
|
|
||
| const chaiHttp = require('chai-http'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OED prefers all require/include at the top of a file. |
||
| chai.use(chaiHttp); | ||
|
|
||
| mocha.describe('User Creation Password Policy', () => { | ||
|
|
||
| let adminToken; | ||
|
|
||
| mocha.before(async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indenting off. |
||
| const res = await chai.request(app) | ||
| .post('/api/login') | ||
| .send({ | ||
| username: 'test@example.invalid', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This really should use testUser to get the info as in other tests. |
||
| password: 'password' | ||
| }); | ||
|
|
||
| adminToken = res.body.token; | ||
| }); | ||
|
|
||
| mocha.it('Rejects weak password on create', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These will need to be expanded if other routes are used during password testing per another comment. |
||
| const res = await chai.request(app) | ||
| .post('/api/users/create') | ||
| .set('token', adminToken) | ||
| .send({ | ||
| username: 'testuser', | ||
| password: 'password', | ||
| role: 'user', | ||
| note: 'test' | ||
| }); | ||
|
|
||
| expect(res).to.have.status(400); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| const zxcvbn = require('zxcvbn'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be good to put a TODO linking to https://github.com/dropbox/zxcvbn#script-load-latency. This isn't huge (400k) but it isn't trivial either. OED hopes to make a future effort to reduce the bundle size and these ideas might help. It might also be good to note that at this time it isn't used on the client. |
||
|
|
||
| //Temporary list, until we decide a better approach like a specific file | ||
| const COMMON_PASSWORDS = new Set([ | ||
| 'password', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space intending instead of tab. Same in other file. |
||
| '12345678', | ||
| ]); | ||
|
|
||
| function validatePasswordPolicy(password, username, role = []) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JSDoc would be good. I see where this is used in src/server/routes/users.js for /create. I'm wondering about all the other places where passwords are entered: Same route in /edit, src/server/services/user/createUser.js & src/server/services/user/editUser.js. Also, this could be used on the front-end to give feedback without contacting the server but that would mean putting this into the client bundle and this isn't done very often so I'm okay with not doing that now (or maybe ever). |
||
| const minLength = role === 'admin' ? 14 : 8; | ||
| if (password.length < minLength) { | ||
| return `Password must be at least ${minLength} characters long.`; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be good to add "for role ${role}" at the end since people might be confused as it varies by role. |
||
| } | ||
|
|
||
| // Username inclusion | ||
| if (username && password.toLowerCase().includes(username.toLowerCase())) { | ||
| return 'Password must not contain your username.'; | ||
| } | ||
|
|
||
| // Common passwords | ||
| if (COMMON_PASSWORDS.has(password.toLowerCase())) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. zxcvbn has an optional argument of user_inputs that seems to not allow those as passwords. Maybe that should be checked to see if direct usage or inside the password is excluded. If so, would it make sense just to use it? |
||
| return 'Password is too common.'; | ||
| } | ||
|
|
||
| // Strength check | ||
| const result = zxcvbn(password); | ||
| if (result.score < 3) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking about several things related to score usage:
|
||
| return 'Password is too weak.'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above for ideas about making the feedback more informative. |
||
| } | ||
|
|
||
| return null; // valid | ||
| } | ||
| module.exports = { validatePasswordPolicy }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the numeric values are being replaced by constants. Thus, this should happen when PR 1581 is merged.
I also think the message could be more user friendly. Maybe "Password must not exceed 72 bytes which often means 72 characters."