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

@@ -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
+   * Supported flavours:
-   * @param {Object} [options] The options for access token, such as scope, appId
+   *
+   * ```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) {
+    let tokenData;
-      // createAccessToken(options, cb)
+    if (typeof ttl !== 'object') {
-      options = ttl;
+      // createAccessToken(ttl[, options], cb)
-      ttl = options.ttl;
+      tokenData = {ttl};
+    } else if (options) {
+      // createAccessToken(data, options, cb)
+      tokenData = ttl;
+    } else {
+      // createAccessToken(options, cb);
+      tokenData = {};
     }
-    options = options || {};
+
-    var userModel = this.constructor;
+    var userSettings = this.constructor.settings;
-    ttl = Math.min(ttl || userModel.settings.ttl, userModel.settings.maxTTL);
+    tokenData.ttl = Math.min(tokenData.ttl || userSettings.ttl, userSettings.maxTTL);
-    this.accessTokens.create({
+    this.accessTokens.create(tokenData, options, cb);
-      ttl: ttl,
-    }, 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();

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');
 

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});
+  }
+});

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');
         });
     });
   });

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