From 9c63abef52ddbea3ce585f858b5d7b4762cad4ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 10 Apr 2017 10:36:49 +0200 Subject: [PATCH 1/4] Add missing tests for reset password flow --- test/helpers/wait-for-event.js | 17 ++++++++++++ test/user.test.js | 49 +++++++++++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 test/helpers/wait-for-event.js 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.test.js b/test/user.test.js index 0592b2d3..65008dc2 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; @@ -2165,6 +2166,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 +2827,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); + } }); From d95ec66a23aceaadd1487db89d8dee1c1404ac6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 10 Apr 2017 12:19:20 +0200 Subject: [PATCH 2/4] Fix method setup in authorization-scopes.test Fix the code builing a scoped method to correctly handle the case when the setup method is called twice and the previously defined method has to be overriden with new remoting metadata. --- test/authorization-scopes.test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/authorization-scopes.test.js b/test/authorization-scopes.test.js index 797cbd48..5dd76d01 100644 --- a/test/authorization-scopes.test.js +++ b/test/authorization-scopes.test.js @@ -82,6 +82,10 @@ describe('Authorization scopes', () => { } function givenRemoteMethodWithCustomScope() { + // Delete any previosly 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', { From e27419086ccd5066611720599cdf44ad8c6a81a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 10 Apr 2017 12:20:40 +0200 Subject: [PATCH 3/4] Add User.setPassword(id, new, cb) Implement a new method for changing user password with password-reset token but without the old password. REST API POST /api/users/reset-password Authorization: your-password-reset-token-id Content-Type: application/json {"newPassword": "new-pass"} JavaScript API User.setPassword(userId, newPassword[, cb]) userInstance.setPassword(newPassword[, cb]) Note: the new REST endpoint is not protected by scopes yet, therefore any valid access token can invoke it (similarly to how any valid access token can change the password via PATCH /api/users/:id). --- common/models/user.js | 92 ++++++++++++++++++++++++++++++++-- common/models/user.json | 7 +++ test/user.integration.js | 69 +++++++++++++++++++++++++ test/user.test.js | 105 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 268 insertions(+), 5 deletions(-) diff --git a/common/models/user.js b/common/models/user.js index 8a5fdc97..cd10714e 100644 --- a/common/models/user.js +++ b/common/models/user.js @@ -421,15 +421,82 @@ 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; + } + + const delta = {password: newPassword}; + this.patchAttributes(delta, options, (err, updated) => cb(err)); + return cb.promise; }; @@ -936,6 +1003,21 @@ module.exports = function(User) { } ); + 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'}, + ], + http: {verb: 'POST', path: '/reset-password'}, + } + ); + UserModel.afterRemote('confirm', function(ctx, inst, next) { if (ctx.args.redirect !== undefined) { if (!ctx.res) { 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/user.integration.js b/test/user.integration.js index 39372c19..bc2f31d4 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); @@ -178,6 +180,64 @@ describe('users - integration', function() { expect(injectedOptions).to.have.property('accessToken'); }); }); + + it('resets 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 request', function() { + return this.post('/api/users/reset-password') + .send({ + newPassword: 'new password', + }) + .expect(401); + }); + + it('injects reset password options from remoting context', function() { + const User = app.models.User; + const credentials = {email: 'inject-reset@example.com', password: 'pass'}; + + let injectedOptions; + User.observe('before save', (ctx, next) => { + injectedOptions = 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(injectedOptions).to.have.property('accessToken'); + }); + }); }); describe('sub-user', function() { @@ -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 65008dc2..643058bb 100644 --- a/test/user.test.js +++ b/test/user.test.js @@ -471,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() { @@ -1406,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 user', () => { + 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', testFlag: true}, + + // updateAttributes + {hook: 'before save', testFlag: true}, + + // validate uniqueness of User.email + {hook: 'access', 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}; From c5ca2e1c2ef7f44ca8d6e28fc85870b9c77c584a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 10 Apr 2017 14:07:41 +0200 Subject: [PATCH 4/4] Implement more secure password flow Improve the flow for setting/changing/resetting User password to make it more secure. 1. Modify `User.resetPassword` to create a token scoped to allow invocation of a single remote method: `User.setPassword`. 2. Scope the method `User.setPassword` so that regular tokens created by `User.login` are not allowed to execute it. For backwards compatibility, this new mode (flow) is enabled only when User model setting `restrictResetPasswordTokenScope` is set to `true`. 3. Changing the password via `User.prototype.patchAttributes` (and similar DAO methods) is no longer allowed. Applications must call `User.changePassword` and ask the user to provide the current (old) password. For backwards compatibility, this new mode (flow) is enabled only when User model setting `rejectPasswordChangesViaPatchOrReplace` is set to `true`. --- common/models/user.js | 112 ++++++++++++-- test/authorization-scopes.test.js | 2 +- test/user-password.test.js | 242 ++++++++++++++++++++++++++++++ test/user.integration.js | 20 +-- test/user.test.js | 14 +- 5 files changed, 358 insertions(+), 32 deletions(-) create mode 100644 test/user-password.test.js diff --git a/common/models/user.js b/common/models/user.js index cd10714e..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; }; @@ -494,6 +512,14 @@ module.exports = function(User) { 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)); @@ -806,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); } @@ -817,7 +857,7 @@ module.exports = function(User) { user: user, options: options, }); - }); + } }); return cb.promise; @@ -1003,6 +1043,9 @@ module.exports = function(User) { } ); + const setPasswordScopes = UserModel.settings.restrictResetPasswordTokenScope ? + ['reset-password'] : undefined; + UserModel.remoteMethod( 'setPassword', { @@ -1014,6 +1057,7 @@ module.exports = function(User) { {arg: 'newPassword', type: 'string', required: true, http: {source: 'form'}}, {arg: 'options', type: 'object', http: 'optionsFromRequest'}, ], + accessScopes: setPasswordScopes, http: {verb: 'POST', path: '/reset-password'}, } ); @@ -1079,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/test/authorization-scopes.test.js b/test/authorization-scopes.test.js index 5dd76d01..6d80dd57 100644 --- a/test/authorization-scopes.test.js +++ b/test/authorization-scopes.test.js @@ -82,7 +82,7 @@ describe('Authorization scopes', () => { } function givenRemoteMethodWithCustomScope() { - // Delete any previosly registered instance of the method "scoped" + // Delete any previously registered instance of the method "scoped" User.sharedClass._methods = User.sharedClass._methods .filter(m => m.name !== 'scoped'); 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 bc2f31d4..e669bfb8 100644 --- a/test/user.integration.js +++ b/test/user.integration.js @@ -155,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(); }); @@ -177,11 +177,11 @@ describe('users - integration', function() { .expect(204); }) .then(() => { - expect(injectedOptions).to.have.property('accessToken'); + expect(observedOptions).to.have.property('accessToken'); }); }); - it('resets user\'s password', function() { + it('resets the user\'s password', function() { const User = app.models.User; const credentials = {email: 'reset@example.com', password: 'pass'}; return User.create(credentials) @@ -206,7 +206,7 @@ describe('users - integration', function() { .then(isMatch => expect(isMatch, 'user has new password').to.be.true()); }); - it('rejects unauthenticated reset password request', function() { + it('rejects unauthenticated reset password requests', function() { return this.post('/api/users/reset-password') .send({ newPassword: 'new password', @@ -214,13 +214,13 @@ describe('users - integration', function() { .expect(401); }); - it('injects reset password options from remoting context', function() { + 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 injectedOptions; + let observedOptions; User.observe('before save', (ctx, next) => { - injectedOptions = ctx.options; + observedOptions = ctx.options; next(); }); @@ -235,7 +235,7 @@ describe('users - integration', function() { .expect(204); }) .then(() => { - expect(injectedOptions).to.have.property('accessToken'); + expect(observedOptions).to.have.property('accessToken'); }); }); }); diff --git a/test/user.test.js b/test/user.test.js index 643058bb..bb2375c1 100644 --- a/test/user.test.js +++ b/test/user.test.js @@ -1402,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) { @@ -1461,7 +1461,7 @@ describe('User', function() { }); }); - it('fails with 401 for unknown user', () => { + it('fails with 401 for unknown users', () => { return User.setPassword('unknown-id', 'pass').then( success => { throw new Error('setPassword should have failed'); }, err => { @@ -1489,13 +1489,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) {