From 0a2a45512cce883ef4f0a91b560195c151743e63 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= <mbajtoss@gmail.com>
Date: Thu, 19 Oct 2017 13:29:08 +0200
Subject: [PATCH] Fix "POST /reset-password" for multi-user setup

Fix the code extracting current user id from the access token provided
in the HTTP request, to allow only access tokens created by the target
user models to execute the action.

This fixes the following security vulnerability:

* A UserA with id 1 (for example), requires a resetToken1

* A UserB with the same id requires a resetToken2.

* Using resetToken2, use the UserAs/reset-password endpoint and change
  the password of UserA and/or vice-versa.
---
 common/models/user.js                      | 21 ++++++++--
 test/multiple-user-principal-types.test.js | 47 ++++++++++++++++++++++
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/common/models/user.js b/common/models/user.js
index 3e4cafc7..1e5d34d0 100644
--- a/common/models/user.js
+++ b/common/models/user.js
@@ -1192,9 +1192,7 @@ module.exports = function(User) {
       {
         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: 'id', type: 'any', http: getUserIdFromRequestContext},
           {arg: 'newPassword', type: 'string', required: true, http: {source: 'form'}},
           {arg: 'options', type: 'object', http: 'optionsFromRequest'},
         ],
@@ -1203,6 +1201,23 @@ module.exports = function(User) {
       }
     );
 
+    function getUserIdFromRequestContext(ctx) {
+      const token = ctx.req.accessToken;
+      if (!token) return;
+
+      const hasPrincipalType = 'principalType' in token;
+      if (hasPrincipalType && token.principalType !== UserModel.modelName) {
+        // We have multiple user models related to the same access token model
+        // and the token used to authorize reset-password request was created
+        // for a different user model.
+        const err = new Error(g.f('Access Denied'));
+        err.statusCode = 403;
+        throw err;
+      }
+
+      return token.userId;
+    }
+
     UserModel.afterRemote('confirm', function(ctx, inst, next) {
       if (ctx.args.redirect !== undefined) {
         if (!ctx.res) {
diff --git a/test/multiple-user-principal-types.test.js b/test/multiple-user-principal-types.test.js
index 8532d122..c6979e0e 100644
--- a/test/multiple-user-principal-types.test.js
+++ b/test/multiple-user-principal-types.test.js
@@ -12,6 +12,10 @@ var extend = require('util')._extend;
 var AccessContext = ctx.AccessContext;
 var Principal = ctx.Principal;
 var Promise = require('bluebird');
+const waitForEvent = require('./helpers/wait-for-event');
+const supertest = require('supertest');
+const loggers = require('./helpers/error-loggers');
+const logServerErrorsOtherThan = loggers.logServerErrorsOtherThan;
 
 describe('Multiple users with custom principalType', function() {
   this.timeout(10000);
@@ -55,6 +59,7 @@ describe('Multiple users with custom principalType', function() {
 
     app.enableAuth({dataSource: 'db'});
     app.use(loopback.token({model: AccessToken}));
+    app.use(loopback.rest());
 
     // create one user per user model to use them throughout the tests
     return Promise.all([
@@ -625,6 +630,48 @@ describe('Multiple users with custom principalType', function() {
     });
   });
 
+  describe('setPassword', () => {
+    let resetToken;
+    beforeEach(givenResetPasswordTokenForOneUser);
+
+    it('sets password when the access token belongs to the user', () => {
+      return supertest(app)
+        .post('/OneUsers/reset-password')
+        .set('Authorization', resetToken.id)
+        .send({newPassword: 'new-pass'})
+        .expect(204)
+        .then(() => {
+          return supertest(app)
+            .post('/OneUsers/login')
+            .send({email: commonCredentials.email, password: 'new-pass'})
+            .expect(200);
+        });
+    });
+
+    it('fails when the access token belongs to a different user mode', () => {
+      logServerErrorsOtherThan(403, app);
+      return supertest(app)
+        .post('/AnotherUsers/reset-password')
+        .set('Authorization', resetToken.id)
+        .send({newPassword: 'new-pass'})
+        .expect(403)
+        .then(() => {
+          return supertest(app)
+            .post('/AnotherUsers/login')
+            .send(commonCredentials)
+            .expect(200);
+        });
+    });
+
+    function givenResetPasswordTokenForOneUser() {
+      return Promise.all([
+        OneUser.resetPassword({email: commonCredentials.email}),
+        waitForEvent(OneUser, 'resetPasswordRequest'),
+      ])
+      .spread((reset, info) => resetToken = info.accessToken);
+    }
+  });
+
   // helpers
   function createUserModel(app, name, options) {
     var model = app.registry.createModel(Object.assign({name: name}, options));