fix: Improve JWT authentication option handling (#1261)

Author: daffl

packages/authentication/lib/core.js

@@ -4,7 +4,7 @@ const jsonwebtoken = require('jsonwebtoken');
 const uuidv4 = require('uuid/v4');
 const { NotAuthenticated, BadRequest } = require('@feathersjs/errors');
 
-const getOptions = require('./options');
+const defaultOptions = require('./options');
 const debug = require('debug')('@feathersjs/authentication/base');
 const createJWT = promisify(jsonwebtoken.sign);
 const verifyJWT = promisify(jsonwebtoken.verify);
@@ -25,7 +25,7 @@ module.exports = class AuthenticationBase {
 
   get configuration () {
     // Always returns a copy of the authentication configuration
-    return getOptions(this.app.get(this.configKey));
+    return Object.assign({}, defaultOptions, this.app.get(this.configKey));
   }
 
   get strategyNames () {

packages/authentication/lib/jwt.js

@@ -26,6 +26,16 @@ module.exports = class JWTStrategy {
     }, config);
   }
 
+  verifyConfiguration () {
+    const allowedKeys = [ 'entity', 'service', 'header', 'schemes' ];
+
+    for (let key of Object.keys(this.configuration)) {
+      if (!allowedKeys.includes(key)) {
+        throw new Error(`Invalid JwtStrategy option 'authentication.${this.name}.${key}'. Did you mean to set it in 'authentication.jwtOptions'?`);
+      }
+    }
+  }
+
   getEntity (id, params) {
     const { service } = this.configuration;
     const entityService = this.app.service(service);

packages/authentication/lib/options.js

@@ -1,6 +1,4 @@
-const { merge } = require('lodash');
-
-const defaults = {
+module.exports = {
   entity: 'user',
   service: 'users',
   strategies: [],
@@ -12,7 +10,3 @@ const defaults = {
     expiresIn: '1d'
   }
 };
-
-module.exports = function (...otherOptions) {
-  return merge({}, defaults, ...otherOptions);
-};

packages/authentication/lib/service.js

@@ -53,6 +53,10 @@ module.exports = class AuthenticationService extends AuthenticationCore {
           this.getJwtOptions(authResult, params)
         ]);
       }).then(([ authResult, payload, jwtOptions ]) => {
+        if (authResult.accessToken) {
+          return authResult;
+        }
+        
         debug('Creating JWT with', payload, jwtOptions);
 
         return this.createJWT(payload, jwtOptions, params.secret)
@@ -79,7 +83,7 @@ module.exports = class AuthenticationService extends AuthenticationCore {
   setup (app, path) {
     // The setup method checks for valid settings and registers the
     // connection and event (login, logout) hooks
-    const { secret, service, entity, entityId } = this.configuration;
+    const { secret, service, entity, entityId, strategies } = this.configuration;
 
     if (typeof secret !== 'string') {
       throw new Error(`A 'secret' must be provided in your authentication configuration`);
@@ -95,6 +99,10 @@ module.exports = class AuthenticationService extends AuthenticationCore {
       }
     }
 
+    if (strategies.length === 0) {
+      throw new Error(`At least one valid authentication strategy required in '${this.configKey}.strategies'`);
+    }
+
     this.hooks({
       after: [ connection(), events() ]
     });

packages/authentication/test/core.test.js

@@ -39,6 +39,25 @@ describe('authentication/core', () => {
       assert.strictEqual(auth.configuration.entity, 'user');
     });
 
+    it('allows to override jwtOptions, does not merge', () => {
+      const { jwtOptions } = auth.configuration;
+      const auth2options = {
+        jwtOptions: {
+          expiresIn: '1w'
+        }
+      };
+
+      app.set('auth2', auth2options);
+
+      const auth2 = new AuthenticationCore(app, 'auth2');
+
+      assert.ok(jwtOptions);
+      assert.strictEqual(jwtOptions.expiresIn, '1d');
+      assert.strictEqual(jwtOptions.issuer, 'feathers');
+
+      assert.deepStrictEqual(auth2.configuration.jwtOptions, auth2options.jwtOptions);
+    });
+
     it('sets configKey and defaultAuthentication', () => {
       assert.strictEqual(app.get('defaultAuthentication'), 'authentication');
     });

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

@@ -11,10 +11,12 @@ describe('authentication/hooks/authenticate', () => {
   beforeEach(() => {
     app = feathers();
     app.use('/authentication', new AuthenticationService(app, 'authentication', {
-      secret: 'supersecret'
+      secret: 'supersecret',
+      strategies: [ 'first' ]
     }));
     app.use('/auth-v2', new AuthenticationService(app, 'auth-v2', {
-      secret: 'supersecret'
+      secret: 'supersecret',
+      strategies: [ 'test' ]
     }));
     app.use('/users', {
       id: 'name',

packages/authentication/test/jwt.test.js

@@ -150,6 +150,19 @@ describe('authentication/jwt', () => {
         assert.deepStrictEqual(authResult.authentication.payload, payload);
       });
     });
+
+    it('errors when trying to set invalid option', () => {
+      app.get('authentication').otherJwt = {
+        expiresIn: 'something'
+      };
+
+      try {
+        app.service('authentication').register('otherJwt', new JWTStrategy());
+        assert.fail('Should never get here');
+      } catch (error) {
+        assert.strictEqual(error.message, `Invalid JwtStrategy option 'authentication.otherJwt.expiresIn'. Did you mean to set it in 'authentication.jwtOptions'?`);
+      }
+    });
   });
 
   describe('parse', () => {

packages/authentication/test/options.test.js

@@ -4,7 +4,7 @@ const jwt = require('jsonwebtoken');
 const feathers = require('@feathersjs/feathers');
 const memory = require('feathers-memory');
 
-const getOptions = require('../lib/options');
+const defaultOptions = require('../lib/options');
 const AuthService = require('../lib/service');
 const { Strategy1 } = require('./fixtures');
 
@@ -27,7 +27,7 @@ describe('authentication/service', () => {
   });
 
   it('settings returns authentication options', () => {
-    assert.deepStrictEqual(service.configuration, getOptions(app.get('authentication')));
+    assert.deepStrictEqual(service.configuration, Object.assign({}, defaultOptions, app.get('authentication')));
   });
 
   describe('create', () => {
@@ -204,6 +204,17 @@ describe('authentication/service', () => {
       }
     });
 
+    it('errors when there is stragies', () => {
+      app.get('authentication').strategies = [];
+
+      try {
+        app.setup();
+        assert.fail('Should never get here');
+      } catch (error) {
+        assert.strictEqual(error.message, `At least one valid authentication strategy required in 'authentication.strategies'`);
+      }
+    });
+
     it('throws an error if entity service does not exist', () => {
       const app = feathers();