diff --git a/.eslintrc.js b/.eslintrc.js index c4f1cccbb..8d674354b 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -37,6 +37,9 @@ module.exports = { // Server admin UI global variables 'onDocumentReady': 'readonly', + 'setupPasswordStrengthHandler': 'readonly', + '$': 'readonly', + 'zxcvbn': 'readonly', 'tinymce': 'readonly', }, diff --git a/packages/server/package-lock.json b/packages/server/package-lock.json index 5e733474d..14b7c8467 100644 --- a/packages/server/package-lock.json +++ b/packages/server/package-lock.json @@ -1466,6 +1466,12 @@ "integrity": "sha512-gCubfBUZ6KxzoibJ+SCUc/57Ms1jz5NjHe4+dI2krNmU5zCPAphyLJYyTOg06ueIyfj+SaCUqmzun7ImlxDcKg==", "dev": true }, + "@types/zxcvbn": { + "version": "4.4.1", + "resolved": "https://registry.npmjs.org/@types/zxcvbn/-/zxcvbn-4.4.1.tgz", + "integrity": "sha512-3NoqvZC2W5gAC5DZbTpCeJ251vGQmgcWIHQJGq2J240HY6ErQ9aWKkwfoKJlHLx+A83WPNTZ9+3cd2ILxbvr1w==", + "dev": true + }, "abab": { "version": "2.0.5", "resolved": "https://registry.npmjs.org/abab/-/abab-2.0.5.tgz", @@ -5375,6 +5381,11 @@ } } }, + "jquery": { + "version": "3.6.0", + "resolved": "https://registry.npmjs.org/jquery/-/jquery-3.6.0.tgz", + "integrity": "sha512-JVzAR/AjBvVt2BmYhxRCSYysDsPcssdmTFnzyLEts9qNwmjmu4JTAMYubEfwVOSwpQ1I1sKKFcxhZCI2buerfw==" + }, "js-tokens": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/js-tokens/-/js-tokens-4.0.0.tgz", @@ -8535,6 +8546,11 @@ "version": "1.2.1", "resolved": "https://registry.npmjs.org/ylru/-/ylru-1.2.1.tgz", "integrity": "sha512-faQrqNMzcPCHGVC2aaOINk13K+aaBDUPjGWl0teOXywElLjyVAB6Oe2jj62jHYtwsU49jXhScYbvPENK+6zAvQ==" + }, + "zxcvbn": { + "version": "4.4.2", + "resolved": "https://registry.npmjs.org/zxcvbn/-/zxcvbn-4.4.2.tgz", + "integrity": "sha1-KOwXzwl0PtyrBW3dixsGJizHPDA=" } } } diff --git a/packages/server/package.json b/packages/server/package.json index 6e02d1b90..89cb8f586 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -29,6 +29,7 @@ "formidable": "^1.2.2", "fs-extra": "^8.1.0", "html-entities": "^1.3.1", + "jquery": "^3.6.0", "knex": "0.95.4", "koa": "^2.8.1", "markdown-it": "^12.0.4", @@ -44,7 +45,8 @@ "raw-body": "^2.4.1", "sqlite3": "^4.1.0", "stripe": "^8.150.0", - "yargs": "^14.0.0" + "yargs": "^14.0.0", + "zxcvbn": "^4.4.2" }, "devDependencies": { "@joplin/tools": "^1.0.9", @@ -57,6 +59,7 @@ "@types/mustache": "^0.8.32", "@types/nodemailer": "^6.4.1", "@types/yargs": "^13.0.2", + "@types/zxcvbn": "^4.4.1", "jest": "^26.6.3", "jsdom": "^16.4.0", "node-mocks-http": "^1.10.0", diff --git a/packages/server/public/js/main.js b/packages/server/public/js/main.js index e60c1275e..95df012b8 100644 --- a/packages/server/public/js/main.js +++ b/packages/server/public/js/main.js @@ -6,3 +6,39 @@ function onDocumentReady(fn) { document.addEventListener('DOMContentLoaded', fn); } } + +// eslint-disable-next-line no-unused-vars, @typescript-eslint/no-unused-vars +function setupPasswordStrengthHandler() { + $('#password_strength').hide(); + + const scoreToLabel = { + 0: 'Very weak', + 1: 'Weak', + 2: 'Medium', + 3: 'Strong', + 4: 'Very strong', + }; + + function scoreToClass(score) { + return score < 3 ? 'has-text-danger-dark' : 'has-text-success-dark'; + } + + function checkPasswordEventHandler() { + const password = $(this).val(); + + if (!password) { + $('#password_strength').hide(); + } else { + $('#password_strength').show(); + const result = zxcvbn(password); + let msg = [`Strength: ${scoreToLabel[result.score]}.`]; + if (result.feedback.warning) msg.push(result.feedback.warning); + if (result.feedback.suggestions) msg = msg.concat(result.feedback.suggestions); + $('#password_strength').html(msg.join(' ')); + } + } + + $('#password').keydown(checkPasswordEventHandler); + $('#password').keyup(checkPasswordEventHandler); + $('#password').change(checkPasswordEventHandler); +} diff --git a/packages/server/src/middleware/notificationHandler.test.ts b/packages/server/src/middleware/notificationHandler.test.ts index 09c165556..eae48d8f5 100644 --- a/packages/server/src/middleware/notificationHandler.test.ts +++ b/packages/server/src/middleware/notificationHandler.test.ts @@ -19,11 +19,16 @@ describe('notificationHandler', function() { test('should check admin password', async function() { const { session } = await createUserAndSession(1, true); + // The default admin password actually doesn't pass the complexity + // check, so we need to skip validation for testing here. Eventually, a + // better mechanism to set the initial default admin password should + // probably be implemented. + const admin = await models().user().save({ email: defaultAdminEmail, password: defaultAdminPassword, is_admin: 1, - }); + }, { skipValidation: true }); { const ctx = await koaAppContext({ sessionId: session.id }); @@ -41,7 +46,7 @@ describe('notificationHandler', function() { await models().user().save({ id: admin.id, password: 'changed!', - }); + }, { skipValidation: true }); const ctx = await koaAppContext({ sessionId: session.id }); await notificationHandler(ctx, koaNext); diff --git a/packages/server/src/models/UserModel.ts b/packages/server/src/models/UserModel.ts index b936c0543..6d4181b0d 100644 --- a/packages/server/src/models/UserModel.ts +++ b/packages/server/src/models/UserModel.ts @@ -7,6 +7,7 @@ import { _ } from '@joplin/lib/locale'; import { formatBytes, GB, MB } from '../utils/bytes'; import { itemIsEncrypted } from '../utils/joplinUtils'; import { getMaxItemSize, getMaxTotalItemSize } from './utils/user'; +import * as zxcvbn from 'zxcvbn'; export enum AccountType { Default = 0, @@ -186,9 +187,22 @@ export default class UserModel extends BaseModel { } } + private validatePassword(password: string) { + const result = zxcvbn(password); + if (result.score < 3) { + let msg: string[] = [result.feedback.warning]; + if (result.feedback.suggestions) { + msg = msg.concat(result.feedback.suggestions); + } + throw new ErrorUnprocessableEntity(msg.join(' ')); + } + } + protected async validate(object: User, options: ValidateOptions = {}): Promise { const user: User = await super.validate(object, options); + // Note that we don't validate the password here because it's already + // been hashed by then. if (options.isNew) { if (!user.email) throw new ErrorUnprocessableEntity('email must be set'); if (!user.password && !user.must_set_password) throw new ErrorUnprocessableEntity('password must be set'); @@ -270,7 +284,10 @@ export default class UserModel extends BaseModel { public async save(object: User, options: SaveOptions = {}): Promise { const user = this.formatValues(object); - if (user.password) user.password = auth.hashPassword(user.password); + if (user.password) { + if (!options.skipValidation) this.validatePassword(user.password); + user.password = auth.hashPassword(user.password); + } const isNew = await this.isNew(object, options); diff --git a/packages/server/src/routes/api/sessions.test.ts b/packages/server/src/routes/api/sessions.test.ts index 54848f803..be1766f9f 100644 --- a/packages/server/src/routes/api/sessions.test.ts +++ b/packages/server/src/routes/api/sessions.test.ts @@ -20,10 +20,10 @@ async function postSession(email: string, password: string): Promise return context; } -describe('api_sessions', function() { +describe('api/sessions', function() { beforeAll(async () => { - await beforeAllDb('api_sessions'); + await beforeAllDb('api/sessions'); }); afterAll(async () => { @@ -35,9 +35,9 @@ describe('api_sessions', function() { }); test('should login user', async function() { - const { user } = await createUserAndSession(1, false); + const { user, password } = await createUserAndSession(1, false); - const context = await postSession(user.email, '123456'); + const context = await postSession(user.email, password); expect(context.response.status).toBe(200); expect(!!context.response.body.id).toBe(true); diff --git a/packages/server/src/routes/default.ts b/packages/server/src/routes/default.ts index dd03ca912..a21c77cf6 100644 --- a/packages/server/src/routes/default.ts +++ b/packages/server/src/routes/default.ts @@ -20,6 +20,10 @@ const pathToFileMap: PathToFileMap = { 'css/bulma.min.css': 'node_modules/bulma/css/bulma.min.css', 'css/bulma-prefers-dark.min.css': 'node_modules/bulma-prefers-dark/css/bulma-prefers-dark.min.css', 'css/fontawesome/css/all.min.css': 'node_modules/@fortawesome/fontawesome-free/css/all.min.css', + 'js/zxcvbn.js': 'node_modules/zxcvbn/dist/zxcvbn.js', + 'js/zxcvbn.js.map': 'node_modules/zxcvbn/dist/zxcvbn.js.map', + 'js/jquery.min.js': 'node_modules/jquery/dist/jquery.min.js', + 'js/jquery.min.map': 'node_modules/jquery/dist/jquery.min.map', // Hard-coded for now but it could be made dynamic later on // 'apps/joplin/css/note.css': 'src/apps/joplin/css/note.css', diff --git a/packages/server/src/routes/index/signup.test.ts b/packages/server/src/routes/index/signup.test.ts index 870215365..51b96cba5 100644 --- a/packages/server/src/routes/index/signup.test.ts +++ b/packages/server/src/routes/index/signup.test.ts @@ -5,6 +5,7 @@ import { getCanShareFolder, getMaxItemSize } from '../../models/utils/user'; import { MB } from '../../utils/bytes'; import { execRequestC } from '../../utils/testing/apiUtils'; import { beforeAllDb, afterAllTests, beforeEachDb, models } from '../../utils/testing/testUtils'; +import uuidgen from '../../utils/uuidgen'; import { FormUser } from './signup'; describe('index_signup', function() { @@ -22,11 +23,12 @@ describe('index_signup', function() { }); test('should create a new account', async function() { + const password = uuidgen(); const formUser: FormUser = { full_name: 'Toto', email: 'toto@example.com', - password: 'testing', - password2: 'testing', + password: password, + password2: password, }; // First confirm that it doesn't work if sign up is disabled diff --git a/packages/server/src/routes/index/users.test.ts b/packages/server/src/routes/index/users.test.ts index 06087ee48..69ffe6abc 100644 --- a/packages/server/src/routes/index/users.test.ts +++ b/packages/server/src/routes/index/users.test.ts @@ -4,8 +4,11 @@ import { NotificationKey } from '../../models/NotificationModel'; import { ErrorForbidden } from '../../utils/errors'; import { execRequest, execRequestC } from '../../utils/testing/apiUtils'; import { beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, createUserAndSession, models, parseHtml, checkContextError, expectHttpError } from '../../utils/testing/testUtils'; +import uuidgen from '../../utils/uuidgen'; + +export async function postUser(sessionId: string, email: string, password: string = null, props: any = null): Promise { + password = password === null ? uuidgen() : password; -export async function postUser(sessionId: string, email: string, password: string, props: any = null): Promise { const context = await koaAppContext({ sessionId: sessionId, request: { @@ -75,7 +78,8 @@ describe('index/users', function() { test('should create a new user', async function() { const { session } = await createUserAndSession(1, true); - await postUser(session.id, 'test@example.com', '123456', { + const password = uuidgen(); + await postUser(session.id, 'test@example.com', password, { max_item_size: '', }); const newUser = await models().user().loadByEmail('test@example.com'); @@ -91,13 +95,13 @@ describe('index/users', function() { const userFromModel: User = await userModel.load(newUser.id); expect(!!userFromModel.password).toBe(true); - expect(userFromModel.password === '123456').toBe(false); // Password has been hashed + expect(userFromModel.password === password).toBe(false); // Password has been hashed }); test('should create a user with null properties if they are not explicitly set', async function() { const { session } = await createUserAndSession(1, true); - await postUser(session.id, 'test@example.com', '123456'); + await postUser(session.id, 'test@example.com'); const newUser = await models().user().loadByEmail('test@example.com'); expect(newUser.max_item_size).toBe(null); @@ -121,8 +125,9 @@ describe('index/users', function() { test('new user should be able to login', async function() { const { session } = await createUserAndSession(1, true); - await postUser(session.id, 'test@example.com', '123456'); - const loggedInUser = await models().user().login('test@example.com', '123456'); + const password = uuidgen(); + await postUser(session.id, 'test@example.com', password); + const loggedInUser = await models().user().login('test@example.com', password); expect(!!loggedInUser).toBe(true); expect(loggedInUser.email).toBe('test@example.com'); }); @@ -132,8 +137,9 @@ describe('index/users', function() { const { session } = await createUserAndSession(1, true); - await postUser(session.id, email, '123456'); - const loggedInUser = await models().user().login(email, '123456'); + const password = uuidgen(); + await postUser(session.id, email, password); + const loggedInUser = await models().user().login(email, password); expect(!!loggedInUser).toBe(true); expect(loggedInUser.email).toBe('ilikeuppercaseandspaces@example.com'); }); @@ -143,13 +149,14 @@ describe('index/users', function() { const userModel = models().user(); - await postUser(session.id, 'test@example.com', '123456'); + const password = uuidgen(); + await postUser(session.id, 'test@example.com', password); const beforeUserCount = (await userModel.all()).length; expect(beforeUserCount).toBe(2); try { - await postUser(session.id, 'test@example.com', '123456'); + await postUser(session.id, 'test@example.com', password); } catch { // Ignore } @@ -173,8 +180,9 @@ describe('index/users', function() { const userModel = models().user(); - await patchUser(session.id, { id: user.id, password: 'abcdefgh', password2: 'abcdefgh' }); - const modUser = await userModel.login('user1@localhost', 'abcdefgh'); + const password = uuidgen(); + await patchUser(session.id, { id: user.id, password: password, password2: password }); + const modUser = await userModel.login('user1@localhost', password); expect(!!modUser).toBe(true); expect(modUser.id).toBe(user.id); }); @@ -203,7 +211,7 @@ describe('index/users', function() { email: 'user1@localhost', must_set_password: 1, email_confirmed: 0, - password: '123456', + password: uuidgen(), }); const { user: user2 } = await createUserAndSession(2); @@ -227,9 +235,10 @@ describe('index/users', function() { // Check that we can't set the password without the token { + const newPassword = uuidgen(); const context = await execRequestC('', 'POST', path, { - password: 'newpassword', - password2: 'newpassword', + password: newPassword, + password2: newPassword, }); const sessionId = context.cookies.get('sessionId'); expect(sessionId).toBeFalsy(); @@ -237,19 +246,22 @@ describe('index/users', function() { // Check that we can't set the password with someone else's token { + const newPassword = uuidgen(); const token2 = (await models().token().allByUserId(user2.id))[0].value; const context = await execRequestC('', 'POST', path, { - password: 'newpassword', - password2: 'newpassword', + password: newPassword, + password2: newPassword, token: token2, }); const sessionId = context.cookies.get('sessionId'); expect(sessionId).toBeFalsy(); } + const newPassword = uuidgen(); + const context = await execRequestC('', 'POST', path, { - password: 'newpassword', - password2: 'newpassword', + password: newPassword, + password2: newPassword, token: token, }); @@ -259,7 +271,7 @@ describe('index/users', function() { expect(session.user_id).toBe(user1.id); // Check that the password has been set - const loggedInUser = await models().user().login(user1.email, 'newpassword'); + const loggedInUser = await models().user().login(user1.email, newPassword); expect(loggedInUser.id).toBe(user1.id); // Check that the email has been verified @@ -278,7 +290,7 @@ describe('index/users', function() { email: 'user1@localhost', must_set_password: 0, email_confirmed: 0, - password: '123456', + password: uuidgen(), }); const email = (await models().email().all()).find(e => e.recipient_id === user1.id); @@ -316,7 +328,7 @@ describe('index/users', function() { await expectHttpError(async () => execRequest(session1.id, 'GET', `users/${admin.id}`), ErrorForbidden.httpCode); // non-admin user cannot create a new user - await expectHttpError(async () => postUser(session1.id, 'cantdothat@example.com', '123456'), ErrorForbidden.httpCode); + await expectHttpError(async () => postUser(session1.id, 'cantdothat@example.com'), ErrorForbidden.httpCode); // non-admin user cannot update another user await expectHttpError(async () => patchUser(session1.id, { id: admin.id, email: 'cantdothateither@example.com' }), ErrorForbidden.httpCode); diff --git a/packages/server/src/routes/index/users.ts b/packages/server/src/routes/index/users.ts index f1a168cb4..3903bd0f2 100644 --- a/packages/server/src/routes/index/users.ts +++ b/packages/server/src/routes/index/users.ts @@ -143,8 +143,9 @@ router.get('users/:id', async (path: SubPath, ctx: AppContext, user: User = null view.content.postUrl = postUrl; view.content.showDeleteButton = !isNew && !!owner.is_admin && owner.id !== user.id; view.content.showResetPasswordButton = !isNew && owner.is_admin; - + view.content.canSetEmail = isNew || owner.is_admin; view.content.canShareFolderOptions = yesNoDefaultOptions(user, 'can_share_folder'); + view.jsFiles.push('zxcvbn'); if (config().accountTypesEnabled) { view.content.showAccountTypes = true; @@ -178,6 +179,8 @@ router.get('users/:id/confirm', async (path: SubPath, ctx: AppContext, error: Er navbar: false, }; + view.jsFiles.push('zxcvbn'); + return view; } else { await ctx.joplin.models.token().deleteByValue(userId, token); diff --git a/packages/server/src/tools/dbTools.ts b/packages/server/src/tools/dbTools.ts index bc1a0f119..8b3a9f4e2 100644 --- a/packages/server/src/tools/dbTools.ts +++ b/packages/server/src/tools/dbTools.ts @@ -44,9 +44,14 @@ export async function createDb(config: DatabaseConfig, options: CreateDbOptions } } - const db = await connectDb(config); - await migrateDb(db); - await disconnectDb(db); + try { + const db = await connectDb(config); + await migrateDb(db); + await disconnectDb(db); + } catch (error) { + error.message += `: ${config.name}`; + throw error; + } } export async function dropDb(config: DatabaseConfig, options: DropDbOptions = null) { diff --git a/packages/server/src/utils/defaultView.ts b/packages/server/src/utils/defaultView.ts index 75cce0e1f..cb6ae8d87 100644 --- a/packages/server/src/utils/defaultView.ts +++ b/packages/server/src/utils/defaultView.ts @@ -8,5 +8,7 @@ export default function(name: string, title: string): View { content: {}, navbar: true, title: title, + jsFiles: [], + cssFiles: [], }; } diff --git a/packages/server/src/utils/testing/testUtils.ts b/packages/server/src/utils/testing/testUtils.ts index cbf4d7783..904376228 100644 --- a/packages/server/src/utils/testing/testUtils.ts +++ b/packages/server/src/utils/testing/testUtils.ts @@ -19,6 +19,7 @@ import { FolderEntity, NoteEntity, ResourceEntity } from '@joplin/lib/services/d import { ModelType } from '@joplin/lib/BaseModel'; import { initializeJoplinUtils } from '../joplinUtils'; import MustacheService from '../../services/MustacheService'; +import uuidgen from '../uuidgen'; // Takes into account the fact that this file will be inside the /dist directory // when it runs. @@ -58,6 +59,8 @@ function initGlobalLogger() { let createdDbPath_: string = null; export async function beforeAllDb(unitName: string) { + unitName = unitName.replace(/\//g, '_'); + createdDbPath_ = `${packageRootDir}/db-test-${unitName}.sqlite`; const tempDir = `${packageRootDir}/temp/test-${unitName}`; @@ -216,6 +219,7 @@ export const testAssetDir = `${packageRootDir}/assets/tests`; interface UserAndSession { user: User; session: Session; + password: string; } export function db() { @@ -241,9 +245,11 @@ interface CreateUserAndSessionOptions { } export const createUserAndSession = async function(index: number = 1, isAdmin: boolean = false, options: CreateUserAndSessionOptions = null): Promise { + const password = uuidgen(); + options = { email: `user${index}@localhost`, - password: '123456', + password, ...options, }; @@ -252,7 +258,8 @@ export const createUserAndSession = async function(index: number = 1, isAdmin: b return { user: await models().user().load(user.id), - session: session, + session, + password, }; }; diff --git a/packages/server/src/views/index/user.mustache b/packages/server/src/views/index/user.mustache index 1555b83f7..63a635cc6 100644 --- a/packages/server/src/views/index/user.mustache +++ b/packages/server/src/views/index/user.mustache @@ -11,9 +11,11 @@
- +
-

For security reasons the email cannot currently be changed. To request a change please contact {{global.supportEmail}}

+ {{^canSetEmail}} +

For security reasons the email cannot currently be changed. To request a change please contact {{global.supportEmail}}

+ {{/canSetEmail}}
{{#global.owner.is_admin}} @@ -60,8 +62,9 @@
- +
+

@@ -84,12 +87,14 @@ diff --git a/packages/server/src/views/index/users/confirm.mustache b/packages/server/src/views/index/users/confirm.mustache index 94e652f48..bf8524905 100644 --- a/packages/server/src/views/index/users/confirm.mustache +++ b/packages/server/src/views/index/users/confirm.mustache @@ -15,7 +15,8 @@
- + +

@@ -31,3 +32,9 @@
+ + \ No newline at end of file diff --git a/packages/server/src/views/layouts/default.mustache b/packages/server/src/views/layouts/default.mustache index 9f9db47e7..01fce88d3 100644 --- a/packages/server/src/views/layouts/default.mustache +++ b/packages/server/src/views/layouts/default.mustache @@ -9,6 +9,7 @@ {{/global.prefersDarkEnabled}} + {{#cssFiles}}