Fix socket auth (#459)

* Fix variable name typo

* Add Visual Studio Code to .gitignore

* Fix socketio authentication

Channel socket auth through the token service instead of calling app.authenticate manually.

This also makes it so the client has to provide a strategy explicitly, and removes the strategy iteration from `passport/authenticate.js`.

Also updates tests.

* The login event isn’t getting emitted.

I’ve added a couple of debugger statements here and have set up the tests so that the failing test is the only one running.  For some reason the debuggers never get hit.  I have yet to find the cause.

* Remove debuggers and fix tests

* Remove two lines of old code.

* Remove old comments

* Update comment

* Better syntax for reading from request.body.

.gitignore

@@ -38,3 +38,4 @@ yarn.lock
 # local options/storage for JetBrains WebStorm and Visual Studio 2017 IDE folder
 .idea/
 .vs/
+/.vscode

src/hooks/authenticate.js

@@ -1,9 +1,10 @@
 import errors from 'feathers-errors';
 import Debug from 'debug';
+import merge from 'lodash.merge';
 const debug = Debug('feathers-authentication:hooks:authenticate');
 
-export default function authenticate (strategy, options = {}) {
-  if (!strategy) {
+export default function authenticate (strategies, options = {}) {
+  if (!strategies) {
     throw new Error(`The 'authenticate' hook requires one of your registered passport strategies.`);
   }
 
@@ -19,13 +20,29 @@ export default function authenticate (strategy, options = {}) {
       return Promise.reject(new Error(`The 'authenticate' hook should only be used as a 'before' hook.`));
     }
 
-    // NOTE (EK): Bring this in when we decide to make the strategy required by the client
-    // if (!hook.app.passport._strategy(strategy)) {
-    //   return Promise.reject(new Error(`Your '${strategy}' authentication strategy is not registered with passport.`));
-    // }
+    hook.data = hook.data || {};
+
+    let { strategy } = hook.data;
+    if (!strategy) {
+      if (Array.isArray(strategies)) {
+        strategy = strategies[0];
+      } else {
+        strategy = strategies;
+      }
+    }
+
+    // Handle the case where authenticate hook was registered without a passport strategy specified
+    if (!strategy) {
+      return Promise.reject(new errors.GeneralError(`You must provide an authentication 'strategy'`));
+    }
+
+    // The client must send a `strategy` name.
+    if (!app.passport._strategy(strategy)) {
+      return Promise.reject(new errors.BadRequest(`Authentication strategy '${strategy}' is not registered.`));
+    }
 
     // NOTE (EK): Passport expects an express/connect
-    // like request object. So we need to create on.
+    // like request object. So we need to create one.
     let request = {
       query: hook.data,
       body: hook.data,
@@ -35,34 +52,40 @@ export default function authenticate (strategy, options = {}) {
       session: {}
     };
 
-    debug(`Attempting to authenticate using ${strategy} strategy with options`, options);
+    const strategyOptions = merge({}, app.passport.options(strategy), options);
+
+    debug(`Attempting to authenticate using ${strategy} strategy with options`, strategyOptions);
 
-    return app.authenticate(strategy, options)(request).then((result = {}) => {
+    return app.authenticate(strategy, strategyOptions)(request).then((result = {}) => {
       if (result.fail) {
         // TODO (EK): Reject with something...
         // You get back result.challenge and result.status
-        if (options.failureRedirect) {
+        if (strategyOptions.failureRedirect) {
           // TODO (EK): Bypass the service?
           // hook.result = true
-          Object.defineProperty(hook.data, '__redirect', { value: { status: 302, url: options.failureRedirect } });
+          Object.defineProperty(hook.data, '__redirect', { value: { status: 302, url: strategyOptions.failureRedirect } });
         }
 
         const { challenge, status = 401 } = result;
         let message = challenge && challenge.message ? challenge.message : challenge;
 
-        if (options.failureMessage) {
-          message = options.failureMessage;
+        if (strategyOptions.failureMessage) {
+          message = strategyOptions.failureMessage;
         }
 
         return Promise.reject(new errors[status](message, challenge));
       }
 
       if (result.success) {
         hook.params = Object.assign({ authenticated: true }, hook.params, result.data);
-        if (options.successRedirect) {
+
+        // Add the user to the original request object so it's available in the socket handler
+        Object.assign(request.params, hook.params);
+
+        if (strategyOptions.successRedirect) {
           // TODO (EK): Bypass the service?
           // hook.result = true
-          Object.defineProperty(hook.data, '__redirect', { value: { status: 302, url: options.successRedirect } });
+          Object.defineProperty(hook.data, '__redirect', { value: { status: 302, url: strategyOptions.successRedirect } });
         }
       } else if (result.redirect) {
         // TODO (EK): Bypass the service?

src/passport/authenticate.js

@@ -11,7 +11,7 @@ export default function authenticate (options = {}) {
   debug('Initializing custom passport authenticate', options);
 
   // This function is bound by passport and called by passport.authenticate()
-  return function (passport, name, strategyOptions = {}, callback = () => {}) {
+  return function (passport, strategies, strategyOptions = {}, callback = () => {}) {
     // This is called by the feathers middleware, hook or socket. The request object
     // is a mock request derived from an http request, socket object, or hook.
     return function (request = {}) {
@@ -21,10 +21,22 @@ export default function authenticate (options = {}) {
         // Allow you to set a location for the success payload.
         // Default is hook.params.user, req.user and socket.user.
         const entity = strategyOptions.entity || strategyOptions.assignProperty || options.entity;
-        let failures = [];
-        let strategies;
+        request.body = request.body || {};
+        let strategyName = request.body.strategy;
+
+        if (!strategyName) {
+          if (Array.isArray(strategies)) {
+            strategyName = strategies[0];
+          } else {
+            strategyName = strategies;
+          }
+        }
 
-        // Cast `name` to an array, allowing authentication to pass through a chain of
+        if (!strategyName) {
+          return reject(new Error(`You must provide an authentication 'strategy'`));
+        }
+
+        // Make sure `strategies` is an array, allowing authentication to pass through a chain of
         // strategies.  The first name to succeed, redirect, or error will halt
         // the chain.  Authentication failures will proceed through each strategy in
         // series, ultimately failing if all strategies fail.
@@ -34,81 +46,68 @@ export default function authenticate (options = {}) {
         // It is not feasible to construct a chain of multiple strategies that involve
         // redirection (for example both Facebook and Twitter), since the first one to
         // redirect will halt the chain.
-        if (request.body && request.body.strategy) {
-          strategies = [request.body.strategy];
-        } else if (Array.isArray(name)) {
-          strategies = name;
-        } else {
-          strategies = [name];
+        if (!Array.isArray(strategies)) {
+          strategies = [strategies];
         }
 
-        function attempt (index) {
-          const layer = strategies[index];
-
-          if (!layer) {
-            // If there isn't another strategy then they
-            // all failed and we'll return the first failure.
-            // TODO (EK): Support passing multiple failures
-            return resolve(failures[0]);
-          }
-
-          // Get the strategy, which will be used as prototype from which to create
-          // a new instance.  Action functions will then be bound to the strategy
-          // within the context of the HTTP request/response pair.
-          let prototype = passport._strategy(layer);
+        // Return an error if the client is trying to authenticate with a strategy
+        // that the server hasn't allowed for this authenticate call. This is important
+        // because it prevents the user from authenticating with a registered strategy
+        // that is not being allowed for this authenticate call.
+        if (!strategies.includes(strategyName)) {
+          return reject(new Error(`Invalid authentication strategy '${strategyName}'`));
+        }
 
-          if (!prototype) {
-            return reject(new Error(`Unknown authentication strategy '${layer}'`));
-          }
+        // Get the strategy, which will be used as prototype from which to create
+        // a new instance.  Action functions will then be bound to the strategy
+        // within the context of the HTTP request/response pair.
+        let prototype = passport._strategy(strategyName);
 
-          // Implement required passport methods that
-          // can be called by a passport strategy.
-          let strategy = Object.create(prototype);
-
-          strategy.redirect = (url, status = 302) => {
-            debug(`'${layer}' authentication redirecting to`, url, status);
-            resolve({ redirect: true, url, status });
-          };
-
-          strategy.fail = (challenge, status) => {
-            debug(`Authentication strategy '${layer}' failed`, challenge, status);
-            failures.push({
-              fail: true,
-              challenge,
-              status
-            });
-
-            // We failed so attempt with next strategy
-            attempt(index + 1);
-          };
-
-          strategy.error = error => {
-            debug(`Error in '${layer}' authentication strategy`, error);
-            reject(error);
-          };
-
-          strategy.success = (data, payload) => {
-            debug(`'${layer}' authentication strategy succeeded`, data, payload);
-            resolve({
-              success: true,
-              data: {
-                [entity]: data,
-                payload
-              }
-            });
-          };
-
-          strategy.pass = () => {
-            debug(`Passing on '${layer}' authentication strategy`);
-            resolve();
-          };
-
-          debug('Passport request object', request);
-          strategy.authenticate(request, strategyOptions);
+        if (!prototype) {
+          return reject(new Error(`Unknown authentication strategy '${strategyName}'`));
         }
 
-        // Attempt to authenticate with first strategy
-        attempt(0);
+        // Implement required passport methods that
+        // can be called by a passport strategy.
+        let strategy = Object.create(prototype);
+
+        strategy.redirect = (url, status = 302) => {
+          debug(`'${strategyName}' authentication redirecting to`, url, status);
+          resolve({ redirect: true, url, status });
+        };
+
+        strategy.fail = (challenge, status) => {
+          debug(`Authentication strategy '${strategyName}' failed`, challenge, status);
+          resolve({
+            fail: true,
+            challenge,
+            status
+          });
+        };
+
+        strategy.error = error => {
+          debug(`Error in '${strategyName}' authentication strategy`, error);
+          reject(error);
+        };
+
+        strategy.success = (data, payload) => {
+          debug(`'${strategyName}' authentication strategy succeeded`, data, payload);
+          resolve({
+            success: true,
+            data: {
+              [entity]: data,
+              payload
+            }
+          });
+        };
+
+        strategy.pass = () => {
+          debug(`Passing on '${strategyName}' authentication strategy`);
+          resolve();
+        };
+
+        debug('Passport request object', request);
+        strategy.authenticate(request, strategyOptions);
       });
     };
   };