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) {