fix: Improve authentication parameter handling (#1333)

packages/authentication-local/src/strategy.ts

@@ -28,6 +28,7 @@ export class LocalStrategy extends AuthenticationBaseStrategy {
       hashSize: 10,
       service: authConfig.service,
       entity: authConfig.entity,
+      entityId: authConfig.entityId,
       errorMessage: 'Invalid login',
       entityPasswordField: config.passwordField,
       entityUsernameField: config.usernameField,
@@ -59,14 +60,32 @@ export class LocalStrategy extends AuthenticationBaseStrategy {
     if (!Array.isArray(list) || list.length === 0) {
       debug(`No entity found`);
 
-      return Promise.reject(new NotAuthenticated(errorMessage));
+      throw new NotAuthenticated(errorMessage);
     }
 
     const [ entity ] = list;
 
     return entity;
   }
 
+  async getEntity (result: any, params: Params) {
+    const { entityService } = this;
+    const { entityId = entityService.id, entity } = this.configuration;
+
+    if (!entityId || result[entityId] === undefined) {
+      throw new NotAuthenticated('Could not get local entity');
+    }
+
+    if (!params.provider) {
+      return result;
+    }
+
+    return entityService.get(result[entityId], {
+      ...params,
+      [entity]: result
+    });
+  }
+
   async comparePassword (entity: any, password: string) {
     const { entityPasswordField, errorMessage } = this.configuration;
     // find password in entity, this allows for dot notation
@@ -101,11 +120,9 @@ export class LocalStrategy extends AuthenticationBaseStrategy {
 
     await this.comparePassword(result, password);
 
-    const authEntity = await (params.provider ? this.findEntity(username, params) : result);
-
     return {
       authentication: { strategy: this.name },
-      [entity]: authEntity
+      [entity]: await this.getEntity(result, params)
     };
   }
 }

packages/authentication-local/test/fixture.js

@@ -34,7 +34,16 @@ module.exports = (app = feathers()) => {
     before: {
       create: hashPassword('password')
     },
-    after: protect('password')
+    after: {
+      all: protect('password'),
+      get: [context => {
+        if (context.params.provider) {
+          context.result.fromGet = true;
+        }
+
+        return context;
+      }]
+    }
   });
 
   return app;

packages/authentication-local/test/strategy.test.ts

@@ -1,4 +1,5 @@
 import assert from 'assert';
+import { omit } from 'lodash';
 import { LocalStrategy } from '../src';
 // @ts-ignore
 import createApplication from './fixture';
@@ -45,6 +46,29 @@ describe('@feathersjs/authentication-local/strategy', () => {
     }
   });
 
+  it('getEntity', async () => {
+    const [ strategy ] = app.service('authentication').getStrategies('local');
+    let entity = await strategy.getEntity(user, {});
+
+    assert.deepStrictEqual(entity, user);
+
+    entity = await strategy.getEntity(user, {
+      provider: 'testing'
+    });
+
+    assert.deepStrictEqual(entity, {
+      ...omit(user, 'password'),
+      fromGet: true
+    });
+
+    try {
+      await strategy.getEntity({}, {});
+      assert.fail('Should never get here');
+    } catch (error) {
+      assert.strictEqual(error.message, 'Could not get local entity');
+    }
+  });
+
   it('strategy fails when strategy is different', async () => {
     const [ local ] = app.service('authentication').getStrategies('local');
 
@@ -128,6 +152,7 @@ describe('@feathersjs/authentication-local/strategy', () => {
     assert.ok(accessToken);
     assert.strictEqual(authResult.user.email, email);
     assert.strictEqual(authResult.user.passsword, undefined);
+    assert.ok(authResult.user.fromGet);
 
     const decoded = await authService.verifyAccessToken(accessToken);
 

packages/authentication/src/hooks/authenticate.ts

@@ -1,4 +1,4 @@
-import { flatten, merge, omit } from 'lodash';
+import { flatten, omit, merge } from 'lodash';
 import { HookContext } from '@feathersjs/feathers';
 import { NotAuthenticated } from '@feathersjs/errors';
 import Debug from 'debug';
@@ -49,7 +49,7 @@ export default (originalSettings: string|AuthenticateHookSettings, ...originalSt
     }
 
     if (authentication) {
-      const authParams = omit(params, 'provider', 'authentication');
+      const authParams = omit(params, 'provider', 'authentication', 'query');
 
       debug('Authenticating with', authentication, strategies);
 

packages/authentication/src/jwt.ts

@@ -1,4 +1,5 @@
 import { NotAuthenticated } from '@feathersjs/errors';
+import { omit } from 'lodash';
 import { AuthenticationRequest } from './core';
 import { Params } from '@feathersjs/feathers';
 import { IncomingMessage } from 'http';
@@ -36,22 +37,28 @@ export class JWTStrategy extends AuthenticationBaseStrategy {
    * @param params Service call parameters
    */
   async getEntity (id: string, params: Params) {
+    const { entity } = this.configuration;
     const entityService = this.entityService;
 
     if (entityService === null) {
       throw new NotAuthenticated(`Could not find entity service`);
     }
 
-    // @ts-ignore
-    return entityService.get(id, params);
+    const result = await entityService.get(id, omit(params, 'provider'));
+
+    if (!params.provider) {
+      return result;
+    }
+
+    return entityService.get(id, { ...params, [entity]: result });
   }
 
   async authenticate (authentication: AuthenticationRequest, params: Params) {
     const { accessToken } = authentication;
     const { entity } = this.configuration;
 
     if (!accessToken) {
-      throw new NotAuthenticated('Not authenticated');
+      throw new NotAuthenticated('No access token');
     }
 
     const payload = await this.authentication.verifyAccessToken(accessToken, params.jwt);

packages/authentication/test/jwt.test.ts

@@ -50,6 +50,18 @@ describe('authentication/jwt', () => {
       }
     });
 
+    app.service('users').hooks({
+      after: {
+        get: [context => {
+          if (context.params.provider) {
+            context.result.isExternal = true;
+          }
+
+          return context;
+        }]
+      }
+    });
+
     user = await app.service('users').create({
       name: 'David'
     });
@@ -61,6 +73,23 @@ describe('authentication/jwt', () => {
     payload = await service.verifyAccessToken(accessToken);
   });
 
+  it('getEntity', async () => {
+    const [ strategy ] = app.service('authentication').getStrategies('jwt') as JWTStrategy[];
+
+    let entity = await strategy.getEntity(user.id, {});
+
+    assert.deepStrictEqual(entity, user);
+
+    entity = await strategy.getEntity(user.id, {
+      provider: 'rest'
+    });
+
+    assert.deepStrictEqual(entity, {
+      ...user,
+      isExternal: true
+    });
+  });
+
   describe('with authenticate hook', () => {
     it('fails for protected service and external call when not set', async () => {
       try {
@@ -107,6 +136,21 @@ describe('authentication/jwt', () => {
       }
     });
 
+    it('fails when accessToken is not set', async () => {
+      try {
+        await app.service('protected').get('test', {
+          provider: 'rest',
+          authentication: {
+            strategy: 'jwt'
+          }
+        });
+        assert.fail('Should never get here');
+      } catch (error) {
+        assert.strictEqual(error.name, 'NotAuthenticated');
+        assert.strictEqual(error.message, 'No access token');
+      }
+    });
+
     it('passes when authentication is set and merges params', async () => {
       const params = {
         provider: 'rest',