fix: Always require strategy parameter in authentication (#1327)

Author: daffl

packages/authentication-local/src/strategy.ts

@@ -94,14 +94,9 @@ export class LocalStrategy extends AuthenticationBaseStrategy {
   }
 
   async authenticate (data: AuthenticationRequest, params: Params) {
-    const { passwordField, usernameField, entity, errorMessage } = this.configuration;
+    const { passwordField, usernameField, entity } = this.configuration;
     const username = data[usernameField];
     const password = data[passwordField];
-
-    if (data.strategy && data.strategy !== this.name) {
-      throw new NotAuthenticated(errorMessage);
-    }
-
     const result = await this.findEntity(username, omit(params, 'provider'));
 
     await this.comparePassword(result, password);

packages/authentication-oauth/src/strategy.ts

@@ -5,7 +5,6 @@ import {
   AuthenticationRequest, AuthenticationBaseStrategy
 } from '@feathersjs/authentication';
 import { Params } from '@feathersjs/feathers';
-import { NotAuthenticated } from '@feathersjs/errors';
 
 const debug = Debug('@feathersjs/authentication-oauth/strategy');
 
@@ -97,10 +96,6 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
   }
 
   async authenticate (authentication: AuthenticationRequest, params: Params) {
-    if (authentication.strategy !== this.name) {
-      throw new NotAuthenticated('Not authenticated');
-    }
-
     const entity: string = this.configuration.entity;
     const profile = await this.getProfile(authentication, params);
     const existingEntity = await this.findEntity(profile, params)

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

@@ -20,18 +20,6 @@ describe('@feathersjs/authentication-oauth/strategy', () => {
   });
 
   describe('authenticate', () => {
-    it('errors when strategy is not set', async () => {
-      try {
-        await strategy.authenticate({
-          id: 'newEntity'
-        }, {});
-        assert.fail('Should never get here');
-      } catch (error) {
-        assert.equal(error.name, 'NotAuthenticated');
-        assert.equal(error.message, 'Not authenticated');
-      }
-    });
-
     it('with new user', async () => {
       const authResult = await strategy.authenticate({
         strategy: 'test',

packages/authentication/src/core.ts

@@ -194,41 +194,20 @@ export class AuthenticationBase {
    * @param allowed A list of allowed strategy names
    */
   async authenticate (authentication: AuthenticationRequest, params: Params, ...allowed: string[]) {
-    debug('Running authenticate for strategies', allowed);
+    const { strategy } = authentication || ({} as AuthenticationRequest);
+    const [ authStrategy ] = this.getStrategies(strategy);
 
-    const strategies = this.getStrategies(...allowed)
-      .filter(current => current && typeof current.authenticate === 'function');
+    debug('Running authenticate for strategy', strategy, allowed);
 
-    if (!authentication || strategies.length === 0) {
+    if (!authentication || !authStrategy || !allowed.includes(strategy)) {
       // If there are no valid strategies or `authentication` is not an object
-      throw new NotAuthenticated(`No valid authentication strategy available`);
+      throw new NotAuthenticated(`Invalid authentication information` + (!strategy ? ' (no `strategy` set)' : ''));
     }
 
-    const { strategy } = authentication;
-    const authParams = {
+    return authStrategy.authenticate(authentication, {
       ...params,
       authenticated: true
-    };
-
-    // Throw an error is a `strategy` is indicated but not in the allowed strategies
-    if (strategy && !allowed.includes(strategy)) {
-      throw new NotAuthenticated(`Invalid authentication strategy '${strategy}'`);
-    }
-
-    let error: Error|null = null;
-
-    for (const authStrategy of strategies) {
-      try {
-        const authResult = await authStrategy.authenticate(authentication, authParams);
-        return authResult;
-      } catch (currentError) {
-        error = error || currentError;
-      }
-    }
-
-    debug('All strategies error. First error is', error);
-
-    throw error;
+    });
   }
 
   /**

packages/authentication/src/jwt.ts

@@ -47,10 +47,10 @@ export class JWTStrategy extends AuthenticationBaseStrategy {
   }
 
   async authenticate (authentication: AuthenticationRequest, params: Params) {
-    const { accessToken, strategy } = authentication;
+    const { accessToken } = authentication;
     const { entity } = this.configuration;
 
-    if (!accessToken || (strategy && strategy !== this.name)) {
+    if (!accessToken) {
       throw new NotAuthenticated('Not authenticated');
     }
 

packages/authentication/test/core.test.ts

@@ -2,7 +2,7 @@ import assert from 'assert';
 import feathers, { Application } from '@feathersjs/feathers';
 import jwt from 'jsonwebtoken';
 
-import { AuthenticationBase } from '../src/core';
+import { AuthenticationBase, AuthenticationRequest } from '../src/core';
 import { Strategy1, Strategy2, MockRequest } from './fixtures';
 import { ServerResponse } from 'http';
 
@@ -23,6 +23,11 @@ describe('authentication/core', () => {
 
     auth.register('first', new Strategy1());
     auth.register('second', new Strategy2());
+    auth.register('dummy', {
+      async authenticate (data: AuthenticationRequest) {
+        return data;
+      }
+    });
   });
 
   describe('configuration', () => {
@@ -80,7 +85,7 @@ describe('authentication/core', () => {
 
   describe('strategies', () => {
     it('strategyNames', () => {
-      assert.deepStrictEqual(auth.strategyNames, [ 'first', 'second' ]);
+      assert.deepStrictEqual(auth.strategyNames, [ 'first', 'second', 'dummy' ]);
     });
 
     it('getStrategies', () => {
@@ -176,14 +181,6 @@ describe('authentication/core', () => {
         }, Strategy2.result));
       });
 
-      it('returns first success when both strategies succeed', async () => {
-        const result = await auth.authenticate({
-          both: true
-        }, {}, ...auth.strategyNames);
-
-        assert.deepStrictEqual(result, Strategy1.result);
-      });
-
       it('throws error when allowed and passed strategy does not match', async () => {
         try {
           await auth.authenticate({
@@ -193,43 +190,18 @@ describe('authentication/core', () => {
           assert.fail('Should never get here');
         } catch (error) {
           assert.strictEqual(error.name, 'NotAuthenticated');
-          assert.strictEqual(error.message, `Invalid authentication strategy 'first'`);
+          assert.strictEqual(error.message, 'Invalid authentication information');
         }
       });
-    });
 
-    describe('with a list of strategies and strategy not set in params', () => {
-      it('returns first success in chain', async () => {
-        const authentication = {
-          v2: true,
-          password: 'supersecret'
-        };
-
-        const result = await auth.authenticate(authentication, {}, 'first', 'second');
-
-        assert.deepStrictEqual(result, Object.assign({
-          authentication,
-          params: { authenticated: true }
-        }, Strategy2.result));
-      });
-
-      it('returns first error when all strategies fail', async () => {
-        try {
-          await auth.authenticate({}, {}, 'first', 'second');
-          assert.fail('Should never get here');
-        } catch (error) {
-          assert.strictEqual(error.name, 'NotAuthenticated');
-          assert.strictEqual(error.message, 'Invalid Dave');
-        }
-      });
-
-      it('errors when there is no valid strategy', async () => {
+      it('throws error when strategy is not set', async () => {
         try {
-          await auth.authenticate({}, {}, 'bla');
+          await auth.authenticate({
+            username: 'Dummy'
+          }, {}, 'second');
           assert.fail('Should never get here');
         } catch (error) {
-          assert.strictEqual(error.name, 'NotAuthenticated');
-          assert.strictEqual(error.message, 'No valid authentication strategy available');
+          assert.strictEqual(error.message, 'Invalid authentication information (no `strategy` set)');
         }
       });
     });

packages/authentication/test/hooks/authenticate.test.ts

@@ -161,6 +161,7 @@ describe('authentication/hooks/authenticate', () => {
     try {
       await app.service('users').get(1, {
         authentication: {
+          strategy: 'first',
           some: 'thing'
         }
       });

packages/authentication/test/jwt.test.ts

@@ -85,7 +85,7 @@ describe('authentication/jwt', () => {
         assert.fail('Should never get here');
       } catch (error) {
         assert.strictEqual(error.name, 'NotAuthenticated');
-        assert.strictEqual(error.message, 'Not authenticated');
+        assert.strictEqual(error.message, 'Invalid authentication information (no `strategy` set)');
       }
     });
 

packages/authentication/test/service.test.ts

@@ -211,7 +211,7 @@ describe('authentication/service', () => {
         await app.service('authentication').remove(null);
         assert.fail('Should never get here');
       } catch (error) {
-        assert.strictEqual(error.message, 'No valid authentication strategy available');
+        assert.strictEqual(error.message, 'Invalid authentication information (no `strategy` set)');
       }
     });
   });

packages/express/test/authentication.test.js

@@ -165,7 +165,7 @@ describe('@feathersjs/express/authentication', () => {
         const { data } = error.response;
 
         assert.strictEqual(data.name, 'NotAuthenticated');
-        assert.strictEqual(data.message, 'No valid authentication strategy available');
+        assert.strictEqual(data.message, 'Invalid authentication information (no `strategy` set)');
       });
     });