fix: Authentication core improvements (#1260)

packages/authentication-client/lib/core.js

@@ -68,14 +68,10 @@ exports.AuthenticationClient = class AuthenticationClient {
   }
 
   reset () {
-    // Reset the internal authentication state
-    // but not the accessToken from storage
-    const authResult = this.app.get('authentication');
-
     this.app.set('authentication', null);
     this.authenticated = false;
 
-    return authResult;
+    return Promise.resolve(null);
   }
 
   reauthenticate (force = false) {
@@ -88,12 +84,12 @@ exports.AuthenticationClient = class AuthenticationClient {
         if (!accessToken) {
           throw new NotAuthenticated('No accessToken found in storage');
         }
-        
+
         return this.authenticate({
           strategy: this.options.jwtStrategy,
           accessToken
         });
-      });
+      }).catch(error => this.removeJwt().then(() => Promise.reject(error)));
     }
 
     return authPromise;
@@ -109,9 +105,11 @@ exports.AuthenticationClient = class AuthenticationClient {
         const { accessToken } = authResult;
 
         this.authenticated = true;
+        this.app.emit('login', authResult);
+        this.app.emit('authenticated', authResult);
 
         return this.setJwt(accessToken).then(() => authResult);
-      });
+      }).catch(error => this.reset().then(() => Promise.reject(error)));
 
     this.app.set('authentication', promise);
 
@@ -121,7 +119,13 @@ exports.AuthenticationClient = class AuthenticationClient {
   logout () {
     return this.app.get('authentication')
       .then(() => this.service.remove(null))
-      .then(() => this.removeJwt())
-      .then(() => this.reset());
+      .then(authResult => this.removeJwt()
+        .then(this.reset())
+        .then(() => {
+          this.app.emit('logout', authResult);
+
+          return authResult;
+        })
+      );
   }
 };

packages/authentication-client/package.json

@@ -26,7 +26,7 @@
     "node": ">= 6"
   },
   "scripts": {
-    "test": "../../node_modules/.bin/nyc mocha --opts ../../mocha.opts"
+    "test": "mocha --opts ../../mocha.opts"
   },
   "directories": {
     "lib": "lib"

packages/authentication-client/test/index.test.js

@@ -53,12 +53,18 @@ describe('@feathersjs/authentication-client', () => {
       .then(res => assert.strictEqual(res, undefined));
   });
 
-  it('authenticate and authentication hook', () => {
+  it('authenticate, authentication hook, login event', () => {
     const data = {
       strategy: 'testing'
     };
 
-    return app.authenticate(data).then(result => {
+    const promise = new Promise(resolve => {
+      app.once('login', resolve);
+    });
+
+    app.authenticate(data);
+
+    return promise.then(result => {
       assert.deepStrictEqual(result, {
         accessToken,
         data,
@@ -80,6 +86,18 @@ describe('@feathersjs/authentication-client', () => {
     });
   });
 
+  it('logout event', () => {
+    const promise = new Promise(resolve => app.once('logout', resolve));
+
+    app.authenticate({
+      strategy: 'testing'
+    }).then(() => app.logout());
+
+    return promise.then(result => {
+      assert.deepStrictEqual(result, { id: null });
+    });
+  });
+
   describe('reauthenticate', () => {
     it('fails when no token in storage', () => {
       return app.authentication.reauthenticate().then(() => {

packages/authentication-client/test/integration/commons.js

@@ -28,7 +28,7 @@ module.exports = (getApp, getClient, {
       });
     });
 
-    it('authentication with wrong credentials fails', () => {
+    it('authentication with wrong credentials fails, does not maintain state', () => {
       return client.authenticate({
         strategy: 'local',
         email,
@@ -38,6 +38,7 @@ module.exports = (getApp, getClient, {
         .catch(error => {
           assert.strictEqual(error.name, 'NotAuthenticated');
           assert.strictEqual(error.message, 'Invalid login');
+          assert.ok(!client.get('authentication'), 'Reset client state');
         });
     });
 
@@ -91,7 +92,12 @@ module.exports = (getApp, getClient, {
         email,
         password
       }).then(() => client.logout())
-        .then(() => client.service('dummy').find())
+        .then(result => {
+          assert.ok(result.accessToken);
+          assert.ok(result.user);
+
+          return client.service('dummy').find();
+        })
         .then(() => assert.fail('Should never get here'))
         .catch(error => {
           assert.strictEqual(error.name, 'NotAuthenticated');

packages/authentication-local/lib/index.js

@@ -3,5 +3,4 @@ const hashPassword = require('./hooks/hash-password');
 const protect = require('./hooks/protect');
 
 exports.LocalStrategy = LocalStrategy;
-exports.protect = protect;
-exports.hashPassword = hashPassword;
+exports.hooks = { protect, hashPassword };

packages/authentication-local/test/fixture.js

@@ -2,7 +2,8 @@ const feathers = require('@feathersjs/feathers');
 const memory = require('feathers-memory');
 const { AuthenticationService, JWTStrategy } = require('@feathersjs/authentication');
 
-const { LocalStrategy, hashPassword, protect } = require('../lib');
+const { LocalStrategy, hooks } = require('../lib');
+const { hashPassword, protect } = hooks;
 
 module.exports = (app = feathers()) => {
   const authentication = new AuthenticationService(app);

packages/authentication-local/test/hooks/hash-password.test.js

@@ -1,7 +1,7 @@
 const assert = require('assert');
 
 const getApp = require('../fixture');
-const { hashPassword } = require('../../lib');
+const { hooks: { hashPassword } } = require('../../lib');
 
 describe('@feathersjs/authentication-local/hooks/hash-password', () => {
   let app;

packages/authentication-local/test/hooks/protect.test.js

@@ -1,5 +1,5 @@
 const assert = require('assert');
-const { protect } = require('../../lib');
+const { hooks: { protect } } = require('../../lib');
 
 function testOmit (title, property) {
   describe(title, () => {

packages/authentication/lib/core.js

@@ -104,6 +104,7 @@ module.exports = class AuthenticationBase {
     }
 
     const { strategy } = authentication;
+    const authParams = Object.assign(params, { authentication: true });
 
     // Throw an error is a `strategy` is indicated but not in the allowed strategies
     if (strategy && !allowed.includes(strategy)) {
@@ -116,7 +117,7 @@ module.exports = class AuthenticationBase {
     const promise = strategies.reduce((acc, authStrategy) => {
       return acc.then(({ result, error }) => {
         if (!result) {
-          return authStrategy.authenticate(authentication, params)
+          return authStrategy.authenticate(authentication, authParams)
             // Set result
             .then(newResult => ({ result: newResult }))
             // Use caught error or previous error if it already exists

packages/authentication/lib/hooks/authenticate.js

@@ -40,8 +40,8 @@ module.exports = (_settings, ..._strategies) => {
       );
     }
 
-    if (authentication) {
-      const authParams = omit(params, 'provider');
+    if (authentication && authentication !== true) {
+      const authParams = omit(params, 'provider', 'authentication');
 
       debug('Authenticating with', authentication, strategies);
 

packages/authentication/lib/hooks/connection.js

@@ -2,11 +2,8 @@ const debug = require('debug')('@feathersjs/authentication/hooks/connection');
 
 module.exports = (strategy = 'jwt') => {
   return context => {
-    const {
-      method,
-      result: { accessToken },
-      params: { connection }
-    } = context;
+    const { method, result, params: { connection } } = context;
+    const { accessToken, ...rest } = result;
 
     if (!connection) {
       return context;
@@ -19,7 +16,9 @@ module.exports = (strategy = 'jwt') => {
       delete connection.authentication;
     } else if (method === 'create' && accessToken) {
       debug('Adding authentication information to real-time connection');
-      connection.authentication = { strategy, accessToken };
+      Object.assign(connection, rest, {
+        authentication: { strategy, accessToken }
+      });
     }
 
     return context;

packages/authentication/test/core.test.js

@@ -99,26 +99,37 @@ describe('authentication/core', () => {
       });
 
       it('returns second success', () => {
-        return auth.authenticate({
+        const authentication = {
           strategy: 'second',
           v2: true,
           password: 'supersecret'
-        }, {}, 'first', 'second').then(result => {
-          assert.deepStrictEqual(result, Strategy2.result);
+        };
+
+        return auth.authenticate(authentication, {}, 'first', 'second').then(result => {
+          assert.deepStrictEqual(result, Object.assign({}, Strategy2.result, {
+            authentication,
+            params: { authentication: true }
+          }));
         });
       });
 
       it('passes params', () => {
         const params = {
           some: 'thing'
         };
-
-        return auth.authenticate({
+        const authentication = {
           strategy: 'second',
           v2: true,
           password: 'supersecret'
-        }, params, 'first', 'second').then(result => {
-          assert.deepStrictEqual(result, Object.assign({}, Strategy2.result, params));
+        };
+
+        return auth.authenticate(authentication, params, 'first', 'second').then(result => {
+          assert.deepStrictEqual(result, Object.assign({
+            params: Object.assign(params, {
+              authentication: true
+            }),
+            authentication
+          }, Strategy2.result));
         });
       });
 
@@ -145,11 +156,16 @@ describe('authentication/core', () => {
 
     describe('with a list of strategies and strategy not set in params', () => {
       it('returns first success in chain', () => {
-        return auth.authenticate({
+        const authentication = {
           v2: true,
           password: 'supersecret'
-        }, {}, 'first', 'second').then(result => {
-          assert.deepStrictEqual(result, Strategy2.result);
+        };
+
+        return auth.authenticate(authentication, {}, 'first', 'second').then(result => {
+          assert.deepStrictEqual(result, Object.assign({
+            authentication,
+            params: { authentication: true }
+          }, Strategy2.result));
         });
       });
 

packages/authentication/test/fixtures.js

@@ -42,7 +42,7 @@ exports.Strategy2 = class Strategy2 {
     const isV2 = authentication.v2 === true && authentication.password === 'supersecret';
 
     if (isV2 || authentication.both) {
-      return Promise.resolve(Object.assign({}, Strategy2.result, params));
+      return Promise.resolve(Object.assign({ params, authentication }, Strategy2.result));
     }
 
     return Promise.reject(new NotAuthenticated('Invalid v2 user'));

packages/authentication/test/hooks/authenticate.test.js

@@ -126,7 +126,10 @@ describe('authentication/hooks/authenticate', () => {
     };
 
     return app.service('users').get(1, params).then(result => {
-      assert.deepStrictEqual(result, Object.assign({}, params, Strategy2.result));
+      assert.deepStrictEqual(result, Object.assign({
+        authentication: params.authentication,
+        params: { authentication: true }
+      }, Strategy2.result));
     });
   });
 
@@ -160,6 +163,18 @@ describe('authentication/hooks/authenticate', () => {
     });
   });
 
+  it('passes with authenticaiton true but external call', () => {
+    return app.service('users').get(1, {
+      provider: 'rest',
+      authentication: true
+    }).then(result => {
+      assert.deepStrictEqual(result, {
+        provider: 'rest',
+        authentication: true
+      });
+    });
+  });
+
   it('errors when used on the authentication service', () => {
     const auth = app.service('authentication');
 

packages/authentication/test/hooks/connection.test.js

@@ -14,7 +14,9 @@ describe('authentication/hooks/connection', () => {
         }
 
         return Promise.resolve({
-          accessToken: '1234'
+          accessToken: '1234',
+          authentication: { strategy: 'test' },
+          additionalParams: true
         });
       },
 
@@ -32,17 +34,23 @@ describe('authentication/hooks/connection', () => {
   it('create does nothing when there is no connection', () => {
     return service.create({}, {}).then(result => {
       assert.deepStrictEqual(result, {
-        accessToken: '1234'
+        accessToken: '1234',
+        authentication: { strategy: 'test' },
+        additionalParams: true
       });
     });
   });
 
-  it('create (login) updates `params.connection.authentication`', () => {
+  it('create (login) updates `params.connection.authentication` with all params', () => {
     const connection = {};
 
     return service.create({}, { connection }).then(() => {
       assert.deepStrictEqual(connection, {
-        authentication: { strategy: 'jwt', accessToken: '1234' }
+        authentication: {
+          strategy: 'jwt',
+          accessToken: '1234'
+        },
+        additionalParams: true
       });
     });
   });