Merge pull request #202 from clems4ever/feature/reset-password-enum

Reset password form sends 200 status when user does not exist

server/src/lib/IdentityCheckMiddleware.ts

@@ -144,6 +144,10 @@ export function get_start_validation(handler: IdentityValidable,
         handler.preValidationResponse(req, res);
         return BluebirdPromise.resolve();
       })
+      .catch(Exceptions.IdentityError, function (err: Error) {
+        handler.preValidationResponse(req, res);
+        return BluebirdPromise.resolve();
+      })
       .catch(ErrorReplies.replyWithError401(req, res, vars.logger));
   };
 }

server/src/lib/routes/password-reset/identity/PasswordResetHandler.ts

@@ -26,13 +26,18 @@ export default class PasswordResetHandler implements IdentityValidable {
   }
 
   preValidationInit(req: express.Request): BluebirdPromise<Identity> {
-    const userid: string = objectPath.get<express.Request, string>(req, "query.userid");
+    const that = this;
+    const userid: string =
+      objectPath.get<express.Request, string>(req, "query.userid");
+    return BluebirdPromise.resolve()
+      .then(function () {
+        that.logger.debug(req, "User '%s' requested a password reset", userid);
+        if (!userid)
+          return BluebirdPromise.reject(
+            new exceptions.AccessDeniedError("No user id provided"));
 
-    this.logger.debug(req, "User '%s' requested a password reset", userid);
+        return that.emailsRetriever.retrieve(userid);
-    if (!userid)
+      })
-      return BluebirdPromise.reject(new exceptions.AccessDeniedError("No user id provided"));
-
-    return this.emailsRetriever.retrieve(userid)
       .then(function (emails: string[]) {
         if (!emails && emails.length <= 0) throw new Error("No email found");
         const identity = {
@@ -40,6 +45,9 @@ export default class PasswordResetHandler implements IdentityValidable {
           userid: userid
         };
         return BluebirdPromise.resolve(identity);
+      })
+      .catch(function (err: Error) {
+        return BluebirdPromise.reject(new exceptions.IdentityError(err.message));
       });
   }
 

server/test/IdentityCheckMiddleware.test.ts

@@ -16,6 +16,8 @@ import IdentityValidatorMock = require("./mocks/IdentityValidator");
 import { RequestLoggerStub } from "./mocks/RequestLoggerStub";
 import { ServerVariablesMock, ServerVariablesMockBuilder }
   from "./mocks/ServerVariablesMockBuilder";
+import { PRE_VALIDATION_TEMPLATE }
+  from "../src/lib/IdentityCheckPreValidationTemplate";
 
 
 describe("test identity check process", function () {
@@ -77,7 +79,8 @@ throws a first factor error", function () {
           });
       });
 
-    it("should send 401 if email is missing in provided identity", function () {
+    // In that case we answer with 200 to avoid user enumeration.
+    it("should send 200 if email is missing in provided identity", function () {
       const identity = { userid: "abc" };
 
       identityValidable.preValidationInit
@@ -87,14 +90,12 @@ throws a first factor error", function () {
 
       return callback(req as any, res as any, undefined)
         .then(function () {
-          return BluebirdPromise.reject("Should fail");
+          Assert(identityValidable.preValidationResponse.called);
-        })
-        .catch(function () {
-          Assert(res.redirect.calledWith("/error/401"));
         });
     });
 
-    it("should send 401 if userid is missing in provided identity",
+    // In that case we answer with 200 to avoid user enumeration.
+    it("should send 200 if userid is missing in provided identity",
       function () {
         const endpoint = "/protected";
         const identity = { email: "abc@example.com" };
@@ -106,10 +107,7 @@ throws a first factor error", function () {
 
         return callback(req as any, res as any, undefined)
           .then(function () {
-            return BluebirdPromise.reject(new Error("It should fail"));
+            Assert(identityValidable.preValidationResponse.called);
-          })
-          .catch(function (err: Error) {
-            Assert(res.redirect.calledWith("/error/401"));
           });
       });
 

server/test/routes/password-reset/identity/PasswordResetHandler.test.ts

@@ -1,5 +1,6 @@
 
-import PasswordResetHandler from "../../../../src/lib/routes/password-reset/identity/PasswordResetHandler";
+import PasswordResetHandler
+  from "../../../../src/lib/routes/password-reset/identity/PasswordResetHandler";
 import PasswordUpdater = require("../../../../src/lib/ldap/PasswordUpdater");
 import { UserDataStore } from "../../../../src/lib/storage/UserDataStore";
 import Sinon = require("sinon");
@@ -7,7 +8,8 @@ import winston = require("winston");
 import assert = require("assert");
 import BluebirdPromise = require("bluebird");
 import ExpressMock = require("../../../mocks/express");
-import { ServerVariablesMock, ServerVariablesMockBuilder } from "../../../mocks/ServerVariablesMockBuilder";
+import { ServerVariablesMock, ServerVariablesMockBuilder }
+  from "../../../mocks/ServerVariablesMockBuilder";
 import { ServerVariables } from "../../../../src/lib/ServerVariables";
 
 describe("test reset password identity check", function () {
@@ -43,36 +45,49 @@ describe("test reset password identity check", function () {
     mocks = s.mocks;
     vars = s.variables;
 
-    mocks.userDataStore.saveU2FRegistrationStub.returns(BluebirdPromise.resolve({}));
+    mocks.userDataStore.saveU2FRegistrationStub
-    mocks.userDataStore.retrieveU2FRegistrationStub.returns(BluebirdPromise.resolve({}));
+      .returns(BluebirdPromise.resolve({}));
-    mocks.userDataStore.produceIdentityValidationTokenStub.returns(BluebirdPromise.resolve({}));
+    mocks.userDataStore.retrieveU2FRegistrationStub
-    mocks.userDataStore.consumeIdentityValidationTokenStub.returns(BluebirdPromise.resolve({}));
+      .returns(BluebirdPromise.resolve({}));
+    mocks.userDataStore.produceIdentityValidationTokenStub
+      .returns(BluebirdPromise.resolve({}));
+    mocks.userDataStore.consumeIdentityValidationTokenStub
+      .returns(BluebirdPromise.resolve({}));
     res = ExpressMock.ResponseMock();
   });
 
   describe("test reset password identity pre check", () => {
     it("should fail when no userid is provided", function () {
       req.query.userid = undefined;
-      const handler = new PasswordResetHandler(vars.logger, vars.ldapEmailsRetriever);
+      const handler = new PasswordResetHandler(vars.logger,
+        vars.ldapEmailsRetriever);
       return handler.preValidationInit(req as any)
-        .then(function () { return BluebirdPromise.reject("It should fail"); })
+        .then(function () {
+          return BluebirdPromise.reject("It should fail");
+        })
         .catch(function (err: Error) {
           return BluebirdPromise.resolve();
         });
     });
 
     it("should fail if ldap fail", function () {
-      mocks.ldapEmailsRetriever.retrieveStub.returns(BluebirdPromise.reject("Internal error"));
+      mocks.ldapEmailsRetriever.retrieveStub
-      new PasswordResetHandler(vars.logger, vars.ldapEmailsRetriever).preValidationInit(req as any)
+        .returns(BluebirdPromise.reject("Internal error"));
-        .then(function () { return BluebirdPromise.reject(new Error("should not be here")); },
+      new PasswordResetHandler(vars.logger, vars.ldapEmailsRetriever)
+        .preValidationInit(req as any)
+        .then(function () {
+          return BluebirdPromise.reject(new Error("should not be here"));
+        },
         function (err: Error) {
           return BluebirdPromise.resolve();
         });
     });
 
     it("should returns identity when ldap replies", function () {
-      mocks.ldapEmailsRetriever.retrieveStub.returns(BluebirdPromise.resolve(["test@example.com"]));
+      mocks.ldapEmailsRetriever.retrieveStub
-      return new PasswordResetHandler(vars.logger, vars.ldapEmailsRetriever).preValidationInit(req as any);
+        .returns(BluebirdPromise.resolve(["test@example.com"]));
+      return new PasswordResetHandler(vars.logger, vars.ldapEmailsRetriever)
+        .preValidationInit(req as any);
     });
   });
 });