Repository: feathersjs/feathers-authentication (js)

Issue:
[1.0] Only the first failure is returned on auth failure when chaining multiple strategies
### Steps to reproduce

Register mulitple strategies like so and send a bad username and password.

```js
app.service('authentication').hooks({
  before: {
    create: [
      auth.hooks.authenticate(['jwt', 'local'])
    ]
  }
});
```

### Expected behavior
It should return all failures or return the one for the strategy you were attempting to authenticate with.

### Actual behavior
If you have registered multiple strategies to be checked against, it only returns the first one that fails . This is very misleading as to what the actual error is that caused auth failure.

In the above example it if you provided a bad username and password auth would fail but it would return the failure from the `jwt` strategy, which would be that the token is missing or something.

### System configuration

Tell us about the applicable parts of your setup.

**Module versions**: feathers-authentication@1.0.0-beta

**NodeJS version**: all

**Operating System**: all

**Browser Version**: all

**React Native Version**: all

**Module Loader**: all

Pull request description:
A bunch of bug fixes
### Summary

- Now checks strategy options on sockets instead of the global options. #348 
- Now supporting entityId for JWT payload.
- Only authenticates against passed strategy. This results in more accurate errors and faster authentication time. Also errors if an unsupported strategy is provided. Closes #346.

### Other Information

Dependent on some fixes to

- feathers-authentication-jwt
- feathers-authentication-local
- feathers-authentication-oauth1
- feathers-authentication-oauth2
- feathers-authentication-client

Patch:
diff --git a/README.md b/README.md
index 1f2201b..9d94b52 100644
--- a/README.md
+++ b/README.md
@@ -147,8 +147,7 @@ app.service('authentication').hooks({
   before: {
     create: [
       // You can chain multiple strategies
-      auth.hooks.authenticate(['jwt', 'local']),
-      customizeJWTPayload()
+      auth.hooks.authenticate(['jwt', 'local'])
     ],
     remove: [
       auth.hooks.authenticate('jwt')
diff --git a/docs/migrating.md b/docs/migrating.md
index 5560363..fc5afb3 100644
--- a/docs/migrating.md
+++ b/docs/migrating.md
@@ -91,29 +91,13 @@ app.use('/users', memory())
       Strategy: FacebookStrategy
     }));
 
-// This hook customizes your payload.
-function customizeJWTPayload() {
-  return function(hook) {
-    console.log('Customizing JWT Payload');
-    hook.data.payload = {
-      // You need to make sure you have the right id.
-      // You can put whatever you want to be encoded in
-      // the JWT access token.
-      id: hook.params.user.id
-    };
-
-    return Promise.resolve(hook);
-  };
-}
-
 // Authenticate the user using the a JWT or
 // email/password strategy and if successful
 // return a new JWT access token.
 app.service('authentication').hooks({
   before: {
     create: [
-      auth.hooks.authenticate(['jwt', 'local']),
-      customizeJWTPayload()
+      auth.hooks.authenticate(['jwt', 'local'])
     ]
   }
 });
@@ -125,7 +109,7 @@ There are a number of breaking changes since the services have been removed:
 
 - Change `auth.token` -> `auth.jwt` in your config
 - Move `auth.token.secret` -> `auth.secret`
-- `auth.token.payload` option has been removed. See [customizing JWT payload]() for how to do this.
+- `auth.token.payload` option has been removed. See [customizing JWT payload](#customizing-jwt-payload) for how to do this.
 - `auth.idField` has been removed. It is now included in all services so we can pull it internally without you needing to specify it.
 - `auth.shouldSetupSuccessRoute` has been removed. Success redirect middleware is registered automatically but only triggers if you explicitly set a redirect. [See redirecting]() for more details.
 - `auth.shouldSetupFailureRoute` has been removed. Failure redirect middleware is registered automatically but only triggers if you explicitly set a redirect. [See redirecting]() for more details.
@@ -233,6 +217,39 @@ app.authenticate({
 
 We previously made the poor assumption that you are always authenticating a user. This is not always the case, or your app may not care about the current user as you already have their id in the accessToken payload or can encode some additional details in the JWT accessToken.  Therefore, if you need to get the current user you need to request it explicitly after authentication or populate it yourself in an after hook server side. See the new usage above for how to fetch your user.
 
+## Customizing JWT Payload
+
+By default the payload for your JWT is simply your entity id (ie. `{ userId }`). However, you can customize your JWT payloads however you wish by adding a `before` hook to the authentication service. For example:
+
+```js
+// This hook customizes your payload.
+function customizeJWTPayload() {
+  return function(hook) {
+    console.log('Customizing JWT Payload');
+    hook.data.payload = {
+      // You need to make sure you have the right id.
+      // You can put whatever you want to be encoded in
+      // the JWT access token.
+      customId: hook.params.user.id
+    };
+
+    return Promise.resolve(hook);
+  };
+}
+
+// Authenticate the user using the a JWT or
+// email/password strategy and if successful
+// return a new JWT access token.
+app.service('authentication').hooks({
+  before: {
+    create: [
+      auth.hooks.authenticate(['jwt', 'local']),
+      customizeJWTPayload()
+    ]
+  }
+});
+``` 
+
 ## JWT Parsing
 
 The JWT is only parsed from the header and body by default now. It is no longer pulled from the query string unless you explicitly tell `feathers-authentication-jwt` to do so.
diff --git a/example/app.js b/example/app.js
index 0d78f17..392f690 100644
--- a/example/app.js
+++ b/example/app.js
@@ -10,16 +10,6 @@ const local = require('feathers-authentication-local');
 const jwt = require('feathers-authentication-jwt');
 const auth = require('../lib/index');
 
-function customizeJWTPayload() {
-  return function(hook) {
-    hook.data.payload = {
-      id: hook.params.user.id
-    };
-
-    return Promise.resolve(hook);
-  };
-}
-
 const app = feathers();
 app.configure(rest())
   .configure(socketio())
@@ -36,8 +26,7 @@ app.service('authentication').hooks({
   before: {
     create: [
       // You can chain multiple strategies
-      auth.hooks.authenticate(['jwt', 'local']),
-      customizeJWTPayload()
+      auth.hooks.authenticate(['jwt', 'local'])
     ],
     remove: [
       auth.hooks.authenticate('jwt')
diff --git a/src/passport/authenticate.js b/src/passport/authenticate.js
index e7ac244..2da5d1d 100644
--- a/src/passport/authenticate.js
+++ b/src/passport/authenticate.js
@@ -13,9 +13,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 = () => {}) {
-    debug('passport.authenticate called with the following options', passport, name, strategyOptions, callback);
-    
+  return function (passport, name, 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 = {}) {
@@ -26,7 +24,7 @@ export default function authenticate (options = {}) {
         // Default is hook.params.user, req.user and socket.user.
         const entity = strategyOptions.entity || strategyOptions.assignProperty || options.entity;
         let failures = [];
-        let strategies = [name];
+        let strategies;
 
         // Cast `name` to an array, allowing authentication to pass through a chain of
         // strategies.  The first name to succeed, redirect, or error will halt
@@ -38,8 +36,12 @@ 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 (Array.isArray(name)) {
+        if (request.strategy) {
+          strategies = [request.strategy];
+        } else if (Array.isArray(name)) {
           strategies = name;
+        } else {
+          strategies = [name];
         }
 
         function attempt(index) {
@@ -60,7 +62,6 @@ export default function authenticate (options = {}) {
           if (!prototype) {
             return reject(new Error(`Unknown authentication strategy '${layer}'`));
           }
-          
 
           // Implement required passport methods that
           // can be called by a passport strategy.
@@ -88,13 +89,13 @@ export default function authenticate (options = {}) {
             reject(error);
           };
 
-          strategy.success = (data, info) => {
-            debug(`'${layer}' authentication strategy succeeded`, data, info);
+          strategy.success = (data, payload) => {
+            debug(`'${layer}' authentication strategy succeeded`, data, payload);
             resolve({
               success: true,
               data: {
                 [entity]: data,
-                info
+                payload
               }
             });
           };
diff --git a/src/passport/initialize.js b/src/passport/initialize.js
index b36c43d..5e0a93f 100644
--- a/src/passport/initialize.js
+++ b/src/passport/initialize.js
@@ -15,7 +15,22 @@ export default function initialize (options = {}) {
     // app.configure(authentication()).
     
     // Expose our JWT util functions globally
+    passport._feathers = {};
     passport.createJWT = createJWT;
     passport.verifyJWT = verifyJWT;
+    passport.options = function(name, strategyOptions) {
+      if (!name) {
+        return passport._feathers;
+      }
+
+      if (typeof name === 'string' && !strategyOptions) {
+        return passport._feathers[name];
+      }
+
+      if (typeof name === 'string' && strategyOptions) {
+        debug(`Setting ${name} strategy options`, strategyOptions);
+        passport._feathers[name] = Object.assign({}, strategyOptions);
+      }
+    };
   };
 }
\ No newline at end of file
diff --git a/src/service.js b/src/service.js
index 5145595..3deaf17 100644
--- a/src/service.js
+++ b/src/service.js
@@ -10,15 +10,16 @@ class Service {
     this.passport = app.passport;
   }
 
-  create(data, params) {
+  create(data = {}, params = {}) {
     const defaults = this.app.get('auth');
+    const payload = merge(data.payload, params.payload);
 
     // create accessToken
     // TODO (EK): Support refresh tokens
     // TODO (EK): This should likely be a hook
     // TODO (EK): This service can be datastore backed to support blacklists :)
     return this.passport
-      .createJWT(data.payload, merge(defaults, params))
+      .createJWT(payload, merge(defaults, params))
       .then(accessToken => {
         return { accessToken };
       });
diff --git a/src/socket/handler.js b/src/socket/handler.js
index 9a92672..7307cd1 100644
--- a/src/socket/handler.js
+++ b/src/socket/handler.js
@@ -76,17 +76,19 @@ export default function setupSocketHandler(app, options, { feathersParams, provi
         return callback(normalizeError(error));
       }
 
-      const promise = app.authenticate(strategy, options[strategy])(socket._feathers)
+      const stategyOptions = app.passport.options(strategy);
+
+      const promise = app.authenticate(strategy, stategyOptions)(socket._feathers)
         .then(result => {
           if (result.success) {
             // NOTE (EK): I don't think we need to support
             // custom redirects. We can emit this to the client
             // and let the client redirect.
-            // if (options.successRedirect) {
+            // if (stategyOptions.successRedirect) {
             //   return {
             //     redirect: true,
             //     status: 302,
-            //     url: options.successRedirect
+            //     url: stategyOptions.successRedirect
             //   };
             // }
             return Promise.resolve(result.data);
@@ -96,16 +98,16 @@ export default function setupSocketHandler(app, options, { feathersParams, provi
             // NOTE (EK): I don't think we need to support
             // custom redirects. We can emit this to the client
             // and let the client redirect.
-            // if (options.failureRedirect) {
+            // if (stategyOptions.failureRedirect) {
             //   return {
             //     redirect: true,
             //     status: 302,
-            //     url: options.failureRedirect
+            //     url: stategyOptions.failureRedirect
             //   };
             // }
 
             const { challenge } = result;
-            const message = options.failureMessage || (challenge && challenge.message);
+            const message = stategyOptions.failureMessage || (challenge && challenge.message);
             
             return Promise.reject(new errors[401](message, challenge));
           }
diff --git a/src/utils.js b/src/utils.js
index c1f2f25..f6c6cc6 100644
--- a/src/utils.js
+++ b/src/utils.js
@@ -1,5 +1,6 @@
 import Debug from 'debug';
 import pick from 'lodash.pick';
+import omit from 'lodash.omit';
 import jwt from 'jsonwebtoken';
 
 const debug = Debug('feathers-authentication:authentication:utils');
@@ -32,7 +33,7 @@ export function createJWT (payload = {}, options = {}) {
     }
 
     // TODO (EK): Support jwtids. Maybe auto-generate a uuid
-    jwt.sign(payload, secret, pick(settings, VALID_KEYS), function(error, token) {
+    jwt.sign(omit(payload, VALID_KEYS), secret, pick(settings, VALID_KEYS), function(error, token) {
       if (error) {
         debug('Error signing JWT', error);
         return reject(error);


Test patch:
diff --git a/test/fixtures/server.js b/test/fixtures/server.js
index c3f445c..375d5cb 100644
--- a/test/fixtures/server.js
+++ b/test/fixtures/server.js
@@ -17,15 +17,15 @@ const User = {
   permissions: ['*']
 };
 
-function customizeJWTPayload() {
-  return function(hook) {
-    hook.data.payload = {
-      id: hook.params.user.id
-    };
+// function customizeJWTPayload() {
+//   return function(hook) {
+//     hook.data.payload = {
+//       id: hook.params.user.id
+//     };
 
-    return Promise.resolve(hook);
-  };
-}
+//     return Promise.resolve(hook);
+//   };
+// }
 
 export default function(settings, socketProvider) {
   const app = feathers();
@@ -39,6 +39,10 @@ export default function(settings, socketProvider) {
     .use(bodyParser.urlencoded({ extended: true }))
     .configure(auth(settings))
     .configure(local())
+    .configure(local({
+      name: 'org-local',
+      entity: 'org'
+    }))
     .configure(jwt())
     .use('/users', memory())
     .use('/', feathers.static(__dirname + '/public'));
@@ -46,8 +50,8 @@ export default function(settings, socketProvider) {
   app.service('authentication').hooks({
     before: {
       create: [
-        auth.hooks.authenticate(['jwt', 'local']),
-        customizeJWTPayload()
+        auth.hooks.authenticate(['jwt', 'local', 'org-local']),
+        // customizeJWTPayload()
       ],
       remove: [
         auth.hooks.authenticate('jwt')
diff --git a/test/integration/primus.test.js b/test/integration/primus.test.js
index f8ed92e..2755c26 100644
--- a/test/integration/primus.test.js
+++ b/test/integration/primus.test.js
@@ -19,6 +19,7 @@ describe('Primus authentication', function() {
   let server;
   let socket;
   let Socket;
+  let serverSocket;
   let ExpiringSocket;
   let expiringServer;
   let expiringSocket;
@@ -30,7 +31,7 @@ describe('Primus authentication', function() {
     app.passport.createJWT({}, options)
       .then(token => {
         expiredToken = token;
-        return app.passport.createJWT({ id: 0 }, app.get('auth'));
+        return app.passport.createJWT({ userId: 0 }, app.get('auth'));
       })
       .then(token => {
         accessToken = token;
@@ -40,6 +41,7 @@ describe('Primus authentication', function() {
           server = app.listen(port);
           server.once('listening', () => {
             Socket = app.primus.Socket;
+            app.primus.on('connection', s => serverSocket = s);
             done();
           });
         });
@@ -78,11 +80,28 @@ describe('Primus authentication', function() {
             app.passport.verifyJWT(response.accessToken, app.get('auth')).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
               done();
             });
           });
         });
+
+        it('sets the user on the socket', done => {
+          socket.send('authenticate', data, (error, response) => {
+            expect(response.accessToken).to.exist;
+            expect(serverSocket.request.feathers.user).to.not.equal(undefined);
+            done();
+          });
+        });
+
+        it('sets entity specified in strategy', done => {
+          data.strategy = 'org-local';
+          socket.send('authenticate', data, (error, response) => {
+            expect(response.accessToken).to.exist;
+            expect(serverSocket.request.feathers.org).to.not.equal(undefined);
+            done();
+          });
+        });
       });
 
       describe('when using invalid credentials', () => {
@@ -132,7 +151,7 @@ describe('Primus authentication', function() {
             app.passport.verifyJWT(response.accessToken, app.get('auth')).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
               done();
             });
           });
@@ -148,7 +167,7 @@ describe('Primus authentication', function() {
             app.passport.verifyJWT(response.accessToken, app.get('auth')).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
               done();
             });
           });
diff --git a/test/integration/rest.test.js b/test/integration/rest.test.js
index d1c92a0..04a84e6 100644
--- a/test/integration/rest.test.js
+++ b/test/integration/rest.test.js
@@ -20,7 +20,7 @@ describe('REST authentication', function() {
     app.passport.createJWT({}, options)
       .then(token => {
         expiredToken = token;
-        return app.passport.createJWT({ id: 0 }, app.get('auth'));
+        return app.passport.createJWT({ userId: 0 }, app.get('auth'));
       })
       .then(token => {
         accessToken = token;
@@ -37,6 +37,7 @@ describe('REST authentication', function() {
 
       beforeEach(() => {
         data = {
+          strategy: 'local',
           email: 'admin@feathersjs.com',
           password: 'admin'
         };
@@ -53,7 +54,7 @@ describe('REST authentication', function() {
             }).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
             });
         });
       });
@@ -108,7 +109,7 @@ describe('REST authentication', function() {
             }).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
             });
         });
       });
@@ -124,7 +125,7 @@ describe('REST authentication', function() {
             }).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
             });
         });
       });
diff --git a/test/integration/socketio.test.js b/test/integration/socketio.test.js
index bb3ca08..feed666 100644
--- a/test/integration/socketio.test.js
+++ b/test/integration/socketio.test.js
@@ -18,6 +18,7 @@ describe('Socket.io authentication', function() {
 
   let server;
   let socket;
+  let serverSocket;
   let expiringServer;
   let expiringSocket;
   let expiredToken;
@@ -28,14 +29,17 @@ describe('Socket.io authentication', function() {
     app.passport.createJWT({}, options)
       .then(token => {
         expiredToken = token;
-        return app.passport.createJWT({ id: 0 }, app.get('auth'));
+        return app.passport.createJWT({ userId: 0 }, app.get('auth'));
       })
       .then(token => {
         accessToken = token;
         expiringServer = expiringApp.listen(1336);
         expiringServer.once('listening', () => {
           server = app.listen(port);
-          server.once('listening', () => done());
+          server.once('listening', () => {
+            app.io.on('connect', s => serverSocket = s);
+            done();
+          });
         });
       });
   });
@@ -69,11 +73,28 @@ describe('Socket.io authentication', function() {
             app.passport.verifyJWT(response.accessToken, app.get('auth')).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
               done();
             });
           });
         });
+
+        it('sets the user on the socket', done => {
+          socket.emit('authenticate', data, (error, response) => {
+            expect(response.accessToken).to.exist;
+            expect(serverSocket.feathers.user).to.not.equal(undefined);
+            done();
+          });
+        });
+
+        it('sets entity specified in strategy', done => {
+          data.strategy = 'org-local';
+          socket.emit('authenticate', data, (error, response) => {
+            expect(response.accessToken).to.exist;
+            expect(serverSocket.feathers.org).to.not.equal(undefined);
+            done();
+          });
+        });
       });
 
       describe('when using invalid credentials', () => {
@@ -123,7 +144,7 @@ describe('Socket.io authentication', function() {
             app.passport.verifyJWT(response.accessToken, app.get('auth')).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
               done();
             });
           });
@@ -139,7 +160,7 @@ describe('Socket.io authentication', function() {
             app.passport.verifyJWT(response.accessToken, app.get('auth')).then(payload => {
               expect(payload).to.exist;
               expect(payload.iss).to.equal('feathers');
-              expect(payload.id).to.equal(0);
+              expect(payload.userId).to.equal(0);
               done();
             });
           });
diff --git a/test/passport/authenticate.test.js b/test/passport/authenticate.test.js
index b66eb08..39bcd11 100644
--- a/test/passport/authenticate.test.js
+++ b/test/passport/authenticate.test.js
@@ -141,15 +141,15 @@ describe('passport:authenticate', () => {
       });
 
       describe('success', () => {
-        let info;
+        let payload;
         let user;
         let organization;
 
         beforeEach(() => {
-          info = { platform: 'feathers' };
+          payload = { platform: 'feathers' };
           user = { name: 'Bob' };
           organization = { name: 'Apple' };
-          verifier = (cb) => cb(null, user, info);
+          verifier = (cb) => cb(null, user, payload);
           passport.use(new MockStrategy({}, verifier));
           authenticator = authenticate({ entity: 'user' })(passport, 'mock');
         });
@@ -166,9 +166,9 @@ describe('passport:authenticate', () => {
           });
         });
 
-        it('returns the passport info', () => {
+        it('returns the passport payload', () => {
           return authenticator(req).then(result => {
-            expect(result.data.info).to.deep.equal(info);
+            expect(result.data.payload).to.deep.equal(payload);
           });
         });