diff --git a/common/models/user.js b/common/models/user.js index 8a5fdc97..34978af0 100644 --- a/common/models/user.js +++ b/common/models/user.js @@ -79,12 +79,26 @@ module.exports = function(User) { * Create access token for the logged in user. This method can be overridden to * customize how access tokens are generated * - * @param {Number} ttl The requested ttl - * @param {Object} [options] The options for access token, such as scope, appId + * Supported flavours: + * + * ```js + * createAccessToken(ttl, cb) + * createAccessToken(ttl, options, cb); + * createAccessToken(options, cb); + * // recent addition: + * createAccessToken(data, options, cb); + * ``` + * + * @options {Number|Object} [ttl|data] Either the requested ttl, + * or an object with token properties to set (see below). + * @property {Number} [ttl] The requested ttl + * @property {String[]} [scopes] The access scopes granted to the token. + * @param {Object} [options] Additional options including remoting context * @callback {Function} cb The callback function * @param {String|Error} err The error string or object * @param {AccessToken} token The generated access token object * @promise + * */ User.prototype.createAccessToken = function(ttl, options, cb) { if (cb === undefined && typeof options === 'function') { @@ -95,17 +109,21 @@ module.exports = function(User) { cb = cb || utils.createPromiseCallback(); - if (typeof ttl === 'object' && !options) { - // createAccessToken(options, cb) - options = ttl; - ttl = options.ttl; + let tokenData; + if (typeof ttl !== 'object') { + // createAccessToken(ttl[, options], cb) + tokenData = {ttl}; + } else if (options) { + // createAccessToken(data, options, cb) + tokenData = ttl; + } else { + // createAccessToken(options, cb); + tokenData = {}; } - options = options || {}; - var userModel = this.constructor; - ttl = Math.min(ttl || userModel.settings.ttl, userModel.settings.maxTTL); - this.accessTokens.create({ - ttl: ttl, - }, cb); + + var userSettings = this.constructor.settings; + tokenData.ttl = Math.min(tokenData.ttl || userSettings.ttl, userSettings.maxTTL); + this.accessTokens.create(tokenData, options, cb); return cb.promise; }; @@ -421,15 +439,90 @@ module.exports = function(User) { return cb(err); } - try { - User.validatePassword(newPassword); - } catch (err) { + this.setPassword(newPassword, options, cb); + }); + return cb.promise; + }; + + /** + * Set this user's password after a password-reset request was made. + * + * @param {*} userId Id of the user changing the password + * @param {string} newPassword The new password to use. + * @param {Object} [options] Additional options including remoting context + * @callback {Function} callback + * @param {Error} err Error object + * @promise + */ + User.setPassword = function(userId, newPassword, options, cb) { + assert(userId != null && userId !== '', 'userId is a required argument'); + assert(!!newPassword, 'newPassword is a required argument'); + + if (cb === undefined && typeof options === 'function') { + cb = options; + options = undefined; + } + cb = cb || utils.createPromiseCallback(); + + // Make sure to use the constructor of the (sub)class + // where the method is invoked from (`this` instead of `User`) + this.findById(userId, options, (err, inst) => { + if (err) return cb(err); + + if (!inst) { + const err = new Error(`User ${userId} not found`); + Object.assign(err, { + code: 'USER_NOT_FOUND', + statusCode: 401, + }); return cb(err); } - const delta = {password: newPassword}; - this.patchAttributes(delta, options, (err, updated) => cb(err)); + inst.setPassword(newPassword, options, cb); }); + + return cb.promise; + }; + + /** + * Set this user's password. The callers of this method + * must ensure the client making the request is authorized + * to change the password, typically by providing the correct + * current password or a password-reset token. + * + * @param {string} newPassword The new password to use. + * @param {Object} [options] Additional options including remoting context + * @callback {Function} callback + * @param {Error} err Error object + * @promise + */ + User.prototype.setPassword = function(newPassword, options, cb) { + assert(!!newPassword, 'newPassword is a required argument'); + + if (cb === undefined && typeof options === 'function') { + cb = options; + options = undefined; + } + cb = cb || utils.createPromiseCallback(); + + try { + this.constructor.validatePassword(newPassword); + } catch (err) { + cb(err); + return cb.promise; + } + + // We need to modify options passed to patchAttributes, but we don't want + // to modify the original options object passed to us by setPassword caller + options = Object.assign({}, options); + + // patchAttributes() does not allow callers to modify the password property + // unless "options.setPassword" is set. + options.setPassword = true; + + const delta = {password: newPassword}; + this.patchAttributes(delta, options, (err, updated) => cb(err)); + return cb.promise; }; @@ -739,7 +832,21 @@ module.exports = function(User) { return cb(err); } - user.createAccessToken(ttl, function(err, accessToken) { + if (UserModel.settings.restrictResetPasswordTokenScope) { + const tokenData = { + ttl: ttl, + scopes: ['reset-password'], + }; + user.createAccessToken(tokenData, options, onTokenCreated); + } else { + // We need to preserve backwards-compatibility with + // user-supplied implementations of "createAccessToken" + // that may not support "options" argument (we have such + // examples in our test suite). + user.createAccessToken(ttl, onTokenCreated); + } + + function onTokenCreated(err, accessToken) { if (err) { return cb(err); } @@ -750,7 +857,7 @@ module.exports = function(User) { user: user, options: options, }); - }); + } }); return cb.promise; @@ -936,6 +1043,25 @@ module.exports = function(User) { } ); + const setPasswordScopes = UserModel.settings.restrictResetPasswordTokenScope ? + ['reset-password'] : undefined; + + UserModel.remoteMethod( + 'setPassword', + { + description: 'Reset user\'s password via a password-reset token.', + accepts: [ + {arg: 'id', type: 'any', + http: ctx => ctx.req.accessToken && ctx.req.accessToken.userId, + }, + {arg: 'newPassword', type: 'string', required: true, http: {source: 'form'}}, + {arg: 'options', type: 'object', http: 'optionsFromRequest'}, + ], + accessScopes: setPasswordScopes, + http: {verb: 'POST', path: '/reset-password'}, + } + ); + UserModel.afterRemote('confirm', function(ctx, inst, next) { if (ctx.args.redirect !== undefined) { if (!ctx.res) { @@ -997,6 +1123,46 @@ module.exports = function(User) { next(); }); + User.observe('before save', function rejectInsecurePasswordChange(ctx, next) { + const UserModel = ctx.Model; + if (!UserModel.settings.rejectPasswordChangesViaPatchOrReplace) { + // In legacy password flow, any DAO method can change the password + return next(); + } + + if (ctx.isNewInstance) { + // The password can be always set when creating a new User instance + return next(); + } + const data = ctx.data || ctx.instance; + const isPasswordChange = 'password' in data; + + // This is the option set by `setPassword()` API + // when calling `this.patchAttritubes()` to change user's password + if (ctx.options.setPassword) { + // Verify that only the password is changed and nothing more or less. + if (Object.keys(data).length > 1 || !isPasswordChange) { + // This is a programmer's error, use the default status code 500 + return next(new Error( + 'Invalid use of "options.setPassword". Only "password" can be ' + + 'changed when using this option.')); + } + + return next(); + } + + if (!isPasswordChange) { + return next(); + } + + const err = new Error( + 'Changing user password via patch/replace API is not allowed. ' + + 'Use changePassword() or setPassword() instead.'); + err.statusCode = 401; + err.code = 'PASSWORD_CHANGE_NOT_ALLOWED'; + next(err); + }); + User.observe('before save', function prepareForTokenInvalidation(ctx, next) { if (ctx.isNewInstance) return next(); if (!ctx.where && !ctx.instance) return next(); diff --git a/common/models/user.json b/common/models/user.json index 230fe7e8..c0e4d66b 100644 --- a/common/models/user.json +++ b/common/models/user.json @@ -89,6 +89,13 @@ "permission": "ALLOW", "property": "changePassword", "accessType": "EXECUTE" + }, + { + "principalType": "ROLE", + "principalId": "$authenticated", + "permission": "ALLOW", + "property": "setPassword", + "accessType": "EXECUTE" } ], "relations": { diff --git a/test/authorization-scopes.test.js b/test/authorization-scopes.test.js index 797cbd48..6d80dd57 100644 --- a/test/authorization-scopes.test.js +++ b/test/authorization-scopes.test.js @@ -82,6 +82,10 @@ describe('Authorization scopes', () => { } function givenRemoteMethodWithCustomScope() { + // Delete any previously registered instance of the method "scoped" + User.sharedClass._methods = User.sharedClass._methods + .filter(m => m.name !== 'scoped'); + const accessScopes = arguments[0] || [CUSTOM_SCOPE]; User.scoped = function(cb) { cb(); }; User.remoteMethod('scoped', { diff --git a/test/helpers/wait-for-event.js b/test/helpers/wait-for-event.js new file mode 100644 index 00000000..3405f3c5 --- /dev/null +++ b/test/helpers/wait-for-event.js @@ -0,0 +1,17 @@ +// Copyright IBM Corp. 2013,2016. All Rights Reserved. +// Node module: loopback +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +'use strict'; + +const Promise = require('bluebird'); + +function waitForEvent(emitter, event) { + return new Promise((resolve, reject) => { + emitter.on(event, resolve); + }); +} + +module.exports = waitForEvent; + diff --git a/test/user-password.test.js b/test/user-password.test.js new file mode 100644 index 00000000..d4b35799 --- /dev/null +++ b/test/user-password.test.js @@ -0,0 +1,242 @@ +// Copyright IBM Corp. 2013,2017. All Rights Reserved. +// Node module: loopback +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +'use strict'; + +const expect = require('./helpers/expect'); +const errorHandler = require('strong-error-handler'); +const loopback = require('../'); +const Promise = require('bluebird'); +const request = require('supertest'); +const waitForEvent = require('./helpers/wait-for-event'); + +describe('User.password', () => { + const credentials = {email: 'test@example.com', password: 'pass'}; + + let app, User, testUser, regularToken, resetToken; + + context('restrict reset password token scope', () => { + beforeEach(givenAppWithRestrictionEnabled); + + context('using regular access token', () => { + beforeEach(givenRegularAccessToken); + + it('allows patching user name', () => { + return changeName(regularToken).expect(200); + }); + + it('allows patching user password', () => { + return patchPassword(regularToken).expect(200); + }); + + it('allows changing user password', () => { + return changePassword(regularToken).expect(204); + }); + + it('denies resetting user password', () => { + return resetPassword(regularToken).expect(401); + }); + }); + + context('using password-reset token', () => { + beforeEach(givenResetPasswordToken); + + it('denies patching user name', () => { + return changeName(resetToken).expect(401); + }); + + it('denies patching user password', () => { + return patchPassword(resetToken).expect(401); + }); + + it('denies changing user password', () => { + return changePassword(resetToken).expect(401); + }); + + it('allows resetting user password', () => { + return resetPassword(resetToken).expect(204); + }); + }); + + function givenAppWithRestrictionEnabled() { + return givenAppWithUser({restrictResetPasswordTokenScope: true}); + } + }); + + context('reject password changes via patch or replace', () => { + beforeEach(givenAppWithRejectionEnabled); + beforeEach(givenRegularAccessToken); + + it('allows patching user name', () => { + return changeName(regularToken).expect(200); + }); + + it('denies patching user password', () => { + return patchPassword(regularToken).expect(401); + }); + + it('allows changing user password', () => { + return changePassword(regularToken).expect(204); + }); + + it('denies setPassword-like call with non-password changes', () => { + return patchNameAndPasswordDirectly().then( + function onSuccess() { + throw new Error('patchAttributes() should have failed'); + }, + function onError(err) { + expect(err.message).to.match(/Invalid use.*options.setPassword/); + }); + }); + + function givenAppWithRejectionEnabled() { + return givenAppWithUser({rejectPasswordChangesViaPatchOrReplace: true}); + } + }); + + context('all feature flags disabled', () => { + beforeEach(givenAppWithNoRestrictions); + + context('using regular access token', () => { + beforeEach(givenRegularAccessToken); + + it('allows changing user name', () => { + return changeName(regularToken).expect(200); + }); + + it('allows patching user password', () => { + return patchPassword(regularToken).expect(200); + }); + + it('allows changing user password', () => { + return changePassword(regularToken).expect(204); + }); + + it('allows resetting user password', () => { + return resetPassword(regularToken).expect(204); + }); + }); + + context('using password-reset token', () => { + beforeEach(givenResetPasswordToken); + + it('allows changing user name', () => { + return changeName(resetToken).expect(200); + }); + + it('allows patching user password', () => { + return patchPassword(resetToken).expect(200); + }); + + it('allows changing user password', () => { + return changePassword(resetToken).expect(204); + }); + + it('allows resetting user password', () => { + return resetPassword(resetToken).expect(204); + }); + }); + + it('allows setPassword-like call with non-password changes', () => { + return patchNameAndPasswordDirectly().then(() => { + // test passed + }); + }); + + function givenAppWithNoRestrictions() { + return givenAppWithUser({ + rejectPasswordChangesViaPatchOrReplace: false, + restrictResetPasswordTokenScope: false, + }); + } + }); + + function givenAppWithUser(userSettings) { + app = loopback({localRegistry: true, loadBuiltinModels: true}); + app.set('remoting', {rest: {handleErrors: false}}); + app.dataSource('db', {connector: 'memory'}); + + userSettings = Object.assign({ + name: 'PwdUser', + base: 'User', + properties: { + name: 'string', + }, + + // Speed up the password hashing algorithm for tests + saltWorkFactor: 4, + + http: {path: '/users'}, + }, userSettings); + + User = app.registry.createModel(userSettings); + app.model(User, {dataSource: 'db'}); + + const AccessToken = app.registry.getModel('AccessToken'); + AccessToken.settings.relations.user.model = User.modelName; + + app.enableAuth({dataSource: 'db'}); + + app.use(loopback.token()); + app.use(loopback.rest()); + app.use(function logUnexpectedError(err, req, res, next) { + const statusCode = err.statusCode || err.status; + if (statusCode > 400 && statusCode !== 401) { + console.log('Unexpected error for %s %s: %s %s', + req.method, req.path, statusCode, err.stack || err); + } + next(err); + }); + app.use(errorHandler({debug: true, log: false})); + + return User.create(credentials) + .then(u => { + testUser = u; + return u.setAttribute('emailVerified', true); + }); + } + + function givenRegularAccessToken() { + return User.login(credentials).then(t => regularToken = t); + } + + function givenResetPasswordToken() { + return Promise.all([ + User.resetPassword({email: credentials.email}), + waitForEvent(User, 'resetPasswordRequest'), + ]) + .spread((reset, info) => resetToken = info.accessToken); + } + + function changeName(token) { + return request(app).patch(`/users/${testUser.id}`) + .set('Authorization', token.id) + .send({name: 'New Name'}); + } + + function patchPassword(token) { + return request(app).patch(`/users/${testUser.id}`) + .set('Authorization', token.id) + .send({password: 'new-pass'}); + } + + function changePassword(token) { + return request(app).post('/users/change-password') + .set('Authorization', token.id) + .send({oldPassword: credentials.password, newPassword: 'new-pass'}); + } + + function resetPassword(token) { + return request(app).post('/users/reset-password') + .set('Authorization', token.id) + .send({newPassword: 'new-pass'}); + } + + function patchNameAndPasswordDirectly() { + return testUser.patchAttributes( + {password: 'new-pass', name: 'New Name'}, + {setPassword: true}); + } +}); diff --git a/test/user.integration.js b/test/user.integration.js index 39372c19..e669bfb8 100644 --- a/test/user.integration.js +++ b/test/user.integration.js @@ -10,6 +10,8 @@ var path = require('path'); var SIMPLE_APP = path.join(__dirname, 'fixtures', 'user-integration-app'); var app = require(path.join(SIMPLE_APP, 'server/server.js')); var expect = require('./helpers/expect'); +const Promise = require('bluebird'); +const waitForEvent = require('./helpers/wait-for-event'); describe('users - integration', function() { lt.beforeEach.withApp(app); @@ -153,13 +155,13 @@ describe('users - integration', function() { .expect(401); }); - it('injects change password options from remoting context', function() { + it('uses change password options provided by the remoting context', function() { const User = app.models.User; const credentials = {email: 'inject@example.com', password: 'pass'}; - let injectedOptions; + let observedOptions; User.observe('before save', (ctx, next) => { - injectedOptions = ctx.options; + observedOptions = ctx.options; next(); }); @@ -175,7 +177,65 @@ describe('users - integration', function() { .expect(204); }) .then(() => { - expect(injectedOptions).to.have.property('accessToken'); + expect(observedOptions).to.have.property('accessToken'); + }); + }); + + it('resets the user\'s password', function() { + const User = app.models.User; + const credentials = {email: 'reset@example.com', password: 'pass'}; + return User.create(credentials) + .then(u => { + this.user = u; + return triggerPasswordReset(credentials.email); + }) + .then(info => { + return this.post('/api/users/reset-password') + .set('Authorization', info.accessToken.id) + .send({ + newPassword: 'new password', + }) + .expect(204); + }) + .then(() => { + return User.findById(this.user.id); + }) + .then(user => { + return user.hasPassword('new password'); + }) + .then(isMatch => expect(isMatch, 'user has new password').to.be.true()); + }); + + it('rejects unauthenticated reset password requests', function() { + return this.post('/api/users/reset-password') + .send({ + newPassword: 'new password', + }) + .expect(401); + }); + + it('uses password reset options provided by the remoting context', function() { + const User = app.models.User; + const credentials = {email: 'inject-reset@example.com', password: 'pass'}; + + let observedOptions; + User.observe('before save', (ctx, next) => { + observedOptions = ctx.options; + next(); + }); + + return User.create(credentials) + .then(u => triggerPasswordReset(credentials.email)) + .then(info => { + return this.post('/api/users/reset-password') + .set('Authorization', info.accessToken.id) + .send({ + newPassword: 'new password', + }) + .expect(204); + }) + .then(() => { + expect(observedOptions).to.have.property('accessToken'); }); }); }); @@ -246,4 +306,13 @@ describe('users - integration', function() { }); }); }); + + function triggerPasswordReset(email) { + const User = app.models.User; + return Promise.all([ + User.resetPassword({email: email}), + waitForEvent(app.models.User, 'resetPasswordRequest'), + ]) + .spread((reset, info) => info); + } }); diff --git a/test/user.test.js b/test/user.test.js index 0592b2d3..bb2375c1 100644 --- a/test/user.test.js +++ b/test/user.test.js @@ -11,7 +11,8 @@ var loopback = require('../'); var async = require('async'); var url = require('url'); var extend = require('util')._extend; -var Promise = require('bluebird'); +const Promise = require('bluebird'); +const waitForEvent = require('./helpers/wait-for-event'); var User, AccessToken; @@ -470,6 +471,25 @@ describe('User', function() { }); }); }); + + it('rejects setPassword when new password is longer than 72 chars', function() { + return User.create({email: 'test@example.com', password: pass72Char}) + .then(u => u.setPassword(pass73Char)) + .then( + success => { throw new Error('setPassword should have failed'); }, + err => { + expect(err.message).to.match(/Password too long/); + + // workaround for chai problem + // object tested must be an array, an object, or a string, + // but error given + const props = Object.assign({}, err); + expect(props).to.contain({ + code: 'PASSWORD_TOO_LONG', + statusCode: 422, + }); + }); + }); }); describe('Access-hook for queries with email NOT case-sensitive', function() { @@ -1382,13 +1402,13 @@ describe('User', function() { {hook: 'access', testFlag: true}, // "before save" hook prepareForTokenInvalidation - {hook: 'access', testFlag: true}, + {hook: 'access', setPassword: true, testFlag: true}, // updateAttributes - {hook: 'before save', testFlag: true}, + {hook: 'before save', setPassword: true, testFlag: true}, // validate uniqueness of User.email - {hook: 'access', testFlag: true}, + {hook: 'access', setPassword: true, testFlag: true}, ])); function saveObservedOptionsForHook(name) { @@ -1405,6 +1425,92 @@ describe('User', function() { } }); + describe('User.setPassword()', () => { + let userId; + beforeEach(givenUserId); + + it('changes the password - callback-style', done => { + User.setPassword(userId, 'new password', (err) => { + if (err) return done(err); + expect(arguments.length, 'changePassword callback arguments length') + .to.be.at.most(1); + + User.findById(userId, (err, user) => { + if (err) return done(err); + user.hasPassword('new password', (err, isMatch) => { + if (err) return done(err); + expect(isMatch, 'user has new password').to.be.true(); + done(); + }); + }); + }); + }); + + it('changes the password - Promise-style', () => { + return User.setPassword(userId, 'new password') + .then(() => { + expect(arguments.length, 'changePassword promise resolution') + .to.equal(0); + return User.findById(userId); + }) + .then(user => { + return user.hasPassword('new password'); + }) + .then(isMatch => { + expect(isMatch, 'user has new password').to.be.true(); + }); + }); + + it('fails with 401 for unknown users', () => { + return User.setPassword('unknown-id', 'pass').then( + success => { throw new Error('setPassword should have failed'); }, + err => { + // workaround for chai problem + // object tested must be an array, an object, or a string, + // but error given + const props = Object.assign({}, err); + expect(props).to.contain({ + code: 'USER_NOT_FOUND', + statusCode: 401, + }); + }); + }); + + it('forwards the "options" argument', () => { + const options = {testFlag: true}; + const observedOptions = []; + + saveObservedOptionsForHook('access'); + saveObservedOptionsForHook('before save'); + + return User.setPassword(userId, 'new', options) + .then(() => expect(observedOptions).to.eql([ + // findById + {hook: 'access', testFlag: true}, + + // "before save" hook prepareForTokenInvalidation + {hook: 'access', setPassword: true, testFlag: true}, + + // updateAttributes + {hook: 'before save', setPassword: true, testFlag: true}, + + // validate uniqueness of User.email + {hook: 'access', setPassword: true, testFlag: true}, + ])); + + function saveObservedOptionsForHook(name) { + User.observe(name, (ctx, next) => { + observedOptions.push(Object.assign({hook: name}, ctx.options)); + next(); + }); + } + }); + + function givenUserId() { + userId = validCredentialsUser.id; + } + }); + describe('Verification', function() { describe('user.verify(verifyOptions, options, cb)', function() { const ctxOptions = {testFlag: true}; @@ -2165,6 +2271,44 @@ describe('User', function() { }); }); + it('creates token that allows patching User with new password', () => { + return triggerPasswordReset(options.email) + .then(info => { + // Make a REST request to change the password + return request(app) + .patch(`/test-users/${info.user.id}`) + .set('Authorization', info.accessToken.id) + .send({password: 'new-pass'}) + .expect(200); + }) + .then(() => { + // Call login to verify the password was changed + const credentials = {email: options.email, password: 'new-pass'}; + return User.login(credentials); + }); + }); + + it('creates token that allows calling other endpoints too', () => { + // Setup a test method that can be executed by $owner only + User.prototype.testMethod = function(cb) { cb(null, 'ok'); }; + User.remoteMethod('prototype.testMethod', { + returns: {arg: 'status', type: 'string'}, + http: {verb: 'get', path: '/test'}, + }); + User.settings.acls.push({ + principalType: 'ROLE', + principalId: '$owner', + permission: 'ALLOW', + property: 'testMethod', + }); + + return triggerPasswordReset(options.email) + .then(info => request(app) + .get(`/test-users/${info.user.id}/test`) + .set('Authorization', info.accessToken.id) + .expect(200)); + }); + describe('User.resetPassword(options, cb) requiring realm', function() { var realmUser; @@ -2788,4 +2932,12 @@ describe('User', function() { expect(User2.settings.ttl).to.equal(10); }); }); + + function triggerPasswordReset(email) { + return Promise.all([ + User.resetPassword({email: email}), + waitForEvent(User, 'resetPasswordRequest'), + ]) + .spread((reset, info) => info); + } });