From c84866dda76db193388f312a05c0ddcfd0610c03 Mon Sep 17 00:00:00 2001 From: Naz Date: Wed, 6 May 2020 06:37:53 +1200 Subject: [PATCH] Improved password reset and session invalidation for "locked" users (#11790) - Fixed session invalidation for "locked" user - Currently Ghost API was returning 404 for users having status set to "locked". This lead the user to be stuck in Ghost-Admin with "Rousource Not Found" error message. - By returning 401 for non-"active" users it allows for the Ghost-Admin to redirect the user to "signin" screen where they would be instructed to reset their password - Fixed error message returned by session API - Instead of returning generic 'access' denied message when error happens during `User.check` we want to return more specific error thrown inside of the method, e.g.: 'accountLocked' or 'accountSuspended' - Fixed messaging for 'accountLocked' i18n, which not corresponds to the actual UI available to the end user - Added automatic password reset email to locked users on sign-in - uses alternative email for required password reset so it's clear that this is a security related reset and not a user-requested reset - Backported the auto sending of required password reset email to v2 sign-in route - used by 3rd party clients where the email is necessary for users to know why login is failing Co-authored-by: Kevin Ansfield --- core/server/api/canary/authentication.js | 6 ++- core/server/api/canary/session.js | 24 +++++++-- core/server/api/v2/authentication.js | 6 ++- core/server/api/v2/session.js | 24 +++++++-- core/server/lib/common/errors.js | 7 +++ core/server/models/user.js | 4 +- core/server/services/auth/passwordreset.js | 37 +++++++++++-- .../templates/reset-password-required.html | 54 +++++++++++++++++++ core/server/services/permissions/providers.js | 6 ++- core/server/translations/en.json | 2 +- test/unit/models/user_spec.js | 14 +++++ .../services/permissions/providers_spec.js | 25 +++++++++ 12 files changed, 187 insertions(+), 22 deletions(-) create mode 100644 core/server/services/mail/templates/reset-password-required.html diff --git a/core/server/api/canary/authentication.js b/core/server/api/canary/authentication.js index ebd5918bc4..a2d6bcd58a 100644 --- a/core/server/api/canary/authentication.js +++ b/core/server/api/canary/authentication.js @@ -108,7 +108,11 @@ module.exports = { return auth.passwordreset.generateToken(frame.data.passwordreset[0].email, api.settings); }) .then((token) => { - return auth.passwordreset.sendResetNotification(token, api.mail); + if (frame.data.required) { + return auth.passwordreset.sendRequiredResetNotification(token, api.mail); + } else { + return auth.passwordreset.sendResetNotification(token, api.mail); + } }); } }, diff --git a/core/server/api/canary/session.js b/core/server/api/canary/session.js index f57ee94167..2db7f1ef5b 100644 --- a/core/server/api/canary/session.js +++ b/core/server/api/canary/session.js @@ -2,6 +2,7 @@ const Promise = require('bluebird'); const common = require('../../lib/common'); const models = require('../../models'); const auth = require('../../services/auth'); +const api = require('./index'); const session = { read(frame) { @@ -35,11 +36,24 @@ const session = { auth.session.createSession(req, res, next); }); }); - }).catch((err) => { - throw new common.errors.UnauthorizedError({ - message: common.i18n.t('errors.middleware.auth.accessDenied'), - err - }); + }).catch(async (err) => { + if (!common.errors.utils.isIgnitionError(err)) { + throw new common.errors.UnauthorizedError({ + message: common.i18n.t('errors.middleware.auth.accessDenied'), + err + }); + } + + if (err.errorType === 'PasswordResetRequiredError') { + await api.authentication.generateResetToken({ + passwordreset: [{ + email: object.username + }], + required: true + }, frame.options.context); + } + + throw err; }); }, delete() { diff --git a/core/server/api/v2/authentication.js b/core/server/api/v2/authentication.js index ebd5918bc4..a2d6bcd58a 100644 --- a/core/server/api/v2/authentication.js +++ b/core/server/api/v2/authentication.js @@ -108,7 +108,11 @@ module.exports = { return auth.passwordreset.generateToken(frame.data.passwordreset[0].email, api.settings); }) .then((token) => { - return auth.passwordreset.sendResetNotification(token, api.mail); + if (frame.data.required) { + return auth.passwordreset.sendRequiredResetNotification(token, api.mail); + } else { + return auth.passwordreset.sendResetNotification(token, api.mail); + } }); } }, diff --git a/core/server/api/v2/session.js b/core/server/api/v2/session.js index f57ee94167..2db7f1ef5b 100644 --- a/core/server/api/v2/session.js +++ b/core/server/api/v2/session.js @@ -2,6 +2,7 @@ const Promise = require('bluebird'); const common = require('../../lib/common'); const models = require('../../models'); const auth = require('../../services/auth'); +const api = require('./index'); const session = { read(frame) { @@ -35,11 +36,24 @@ const session = { auth.session.createSession(req, res, next); }); }); - }).catch((err) => { - throw new common.errors.UnauthorizedError({ - message: common.i18n.t('errors.middleware.auth.accessDenied'), - err - }); + }).catch(async (err) => { + if (!common.errors.utils.isIgnitionError(err)) { + throw new common.errors.UnauthorizedError({ + message: common.i18n.t('errors.middleware.auth.accessDenied'), + err + }); + } + + if (err.errorType === 'PasswordResetRequiredError') { + await api.authentication.generateResetToken({ + passwordreset: [{ + email: object.username + }], + required: true + }, frame.options.context); + } + + throw err; }); }, delete() { diff --git a/core/server/lib/common/errors.js b/core/server/lib/common/errors.js index 08086fc71c..2c92531f1a 100644 --- a/core/server/lib/common/errors.js +++ b/core/server/lib/common/errors.js @@ -67,6 +67,13 @@ const ghostErrors = { errorType: 'HelperWarning', hideStack: true }, options)); + }, + PasswordResetRequiredError: function PasswordResetRequiredError(options) { + GhostError.call(this, merge({ + errorType: 'PasswordResetRequiredError', + statusCode: 401, + message: 'For security, you need to create a new password. An email has been sent to you with instructions!' + }, options)); } }; diff --git a/core/server/models/user.js b/core/server/models/user.js index 53400c2f32..c5695f3548 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -805,9 +805,7 @@ User = ghostBookshelf.Model.extend({ } if (user.isLocked()) { - throw new common.errors.NoPermissionError({ - message: common.i18n.t('errors.models.user.accountLocked') - }); + throw new common.errors.PasswordResetRequiredError(); } if (user.isInactive()) { diff --git a/core/server/services/auth/passwordreset.js b/core/server/services/auth/passwordreset.js index 781d71786b..7099cf27b7 100644 --- a/core/server/services/auth/passwordreset.js +++ b/core/server/services/auth/passwordreset.js @@ -145,10 +145,37 @@ async function sendResetNotification(data, mailAPI) { return mailAPI.send(payload, {context: {internal: true}}); } +async function sendRequiredResetNotification(data, mailAPI) { + const adminUrl = urlUtils.urlFor('admin', true); + const resetUrl = urlUtils.urlJoin(adminUrl, 'reset', security.url.encodeBase64(data.resetToken), '/'); + + const content = await mail.utils.generateContent({ + data: { + resetUrl: resetUrl + }, + template: 'reset-password-required' + }); + + const payload = { + mail: [{ + message: { + to: data.email, + subject: i18n.t('common.api.authentication.mail.resetPasswordRequired'), + html: content.html, + text: content.text + }, + options: {} + }] + }; + + return mailAPI.send(payload, {context: {internal: true}}); +} + module.exports = { - generateToken: generateToken, - extractTokenParts: extractTokenParts, - protectBruteForce: protectBruteForce, - doReset: doReset, - sendResetNotification: sendResetNotification + generateToken, + extractTokenParts, + protectBruteForce, + doReset, + sendResetNotification, + sendRequiredResetNotification }; diff --git a/core/server/services/mail/templates/reset-password-required.html b/core/server/services/mail/templates/reset-password-required.html new file mode 100644 index 0000000000..c0795abd00 --- /dev/null +++ b/core/server/services/mail/templates/reset-password-required.html @@ -0,0 +1,54 @@ + + + + + + + + + + + + + +
+ + + + + +
+ +
+ + + + +
+ + +

Hello!

+

For security, it's necessary to reset your password on {{ siteUrl }}.

+

Please follow the link below to complete the process:

Click here to reset your password

+

Alternatively please visit your site's admin area and follow the forgot password process.

+

Ghost

+ + +
+
+
+ + + + +
+ Sent by {{siteUrl}} +
+
+ +
+ +
+ + + diff --git a/core/server/services/permissions/providers.js b/core/server/services/permissions/providers.js index ee0b0b7d89..bd9c6c5f7c 100644 --- a/core/server/services/permissions/providers.js +++ b/core/server/services/permissions/providers.js @@ -6,7 +6,7 @@ const {i18n} = require('../../lib/common'); module.exports = { user: function (id) { - return models.User.findOne({id: id, status: 'active'}, {withRelated: ['permissions', 'roles', 'roles.permissions']}) + return models.User.findOne({id: id}, {withRelated: ['permissions', 'roles', 'roles.permissions']}) .then(function (foundUser) { // CASE: {context: {user: id}} where the id is not in our database if (!foundUser) { @@ -15,6 +15,10 @@ module.exports = { })); } + if (foundUser.get('status') !== 'active') { + return Promise.reject(new errors.UnauthorizedError()); + } + const seenPerms = {}; const rolePerms = _.map(foundUser.related('roles').models, function (role) { diff --git a/core/server/translations/en.json b/core/server/translations/en.json index ceaeb735b3..93bbbf7499 100644 --- a/core/server/translations/en.json +++ b/core/server/translations/en.json @@ -12,6 +12,7 @@ "sampleBlogDescription": "Thoughts, stories and ideas.", "mail": { "resetPassword": "Reset Password", + "resetPasswordRequired": "Reset Password", "checkEmailForInstructions": "Check your email for further instructions.", "passwordChanged": "Password changed successfully.", "invitationAccepted": "Invitation accepted.", @@ -227,7 +228,6 @@ "help": "Visit and save your profile after logging in to check for problems." }, "incorrectPassword": "Your password is incorrect.", - "accountLocked": "Your account is locked. Please reset your password to log in again by clicking the \"Forgotten password?\" link!", "accountSuspended": "Your account was suspended.", "newPasswordsDoNotMatch": "Your new passwords do not match", "passwordRequiredForOperation": "Password is required for this operation", diff --git a/test/unit/models/user_spec.js b/test/unit/models/user_spec.js index 361912480c..a9cd07395a 100644 --- a/test/unit/models/user_spec.js +++ b/test/unit/models/user_spec.js @@ -135,6 +135,20 @@ describe('Unit: models/user', function () { (err instanceof common.errors.ValidationError).should.eql(true); }); }); + + it('status is locked', function () { + const user = models.User.forge(testUtils.DataGenerator.forKnex.createUser({ + status: 'locked', + email: 'test@ghost.de' + })); + + sinon.stub(models.User, 'getByEmail').resolves(user); + + return models.User.check({email: user.get('email'), password: 'test'}) + .catch(function (err) { + (err instanceof common.errors.PasswordResetRequiredError).should.eql(true); + }); + }); }); describe('permissible', function () { diff --git a/test/unit/services/permissions/providers_spec.js b/test/unit/services/permissions/providers_spec.js index 1d2dd4c79a..6a3717df99 100644 --- a/test/unit/services/permissions/providers_spec.js +++ b/test/unit/services/permissions/providers_spec.js @@ -36,6 +36,7 @@ describe('Permission Providers', function () { const findUserSpy = sinon.stub(models.User, 'findOne').callsFake(function () { // Create a fake model const fakeUser = models.User.forge(testUtils.DataGenerator.Content.users[0]); + fakeUser.set('status', 'active'); // Roles & Permissions need to be collections const fakeAdminRole = models.Roles.forge(testUtils.DataGenerator.Content.roles[0]); @@ -83,6 +84,7 @@ describe('Permission Providers', function () { const findUserSpy = sinon.stub(models.User, 'findOne').callsFake(function () { // Create a fake model const fakeUser = models.User.forge(testUtils.DataGenerator.Content.users[0]); + fakeUser.set('status', 'active'); // Roles & Permissions need to be collections const fakeAdminRole = models.Roles.forge(testUtils.DataGenerator.Content.roles[0]); @@ -132,6 +134,7 @@ describe('Permission Providers', function () { const findUserSpy = sinon.stub(models.User, 'findOne').callsFake(function () { // Create a fake model const fakeUser = models.User.forge(testUtils.DataGenerator.Content.users[0]); + fakeUser.set('status', 'active'); // Roles & Permissions need to be collections const fakeAdminRole = models.Roles.forge(testUtils.DataGenerator.Content.roles[0]); @@ -176,6 +179,28 @@ describe('Permission Providers', function () { }) .catch(done); }); + + it('throws when user with non-active status is loaded', function (done) { + // This test requires quite a lot of unique setup work + const findUserSpy = sinon.stub(models.User, 'findOne').callsFake(function () { + // Create a fake model + const fakeUser = models.User.forge(testUtils.DataGenerator.Content.users[0]); + fakeUser.set('status', 'locked'); + + return Promise.resolve(fakeUser); + }); + + // Get permissions for the user + providers.user(1) + .then(function (res) { + done(new Error('Locked user should should throw an error')); + }) + .catch((err) => { + err.errorType.should.equal('UnauthorizedError'); + findUserSpy.callCount.should.eql(1); + done(); + }); + }); }); describe('API Key', function () {