From d4a8021fe2461e98d40bf4aada15637ac356c231 Mon Sep 17 00:00:00 2001 From: David Luecke Date: Fri, 3 May 2019 19:00:31 -0700 Subject: [PATCH] fix: Always require strategy parameter in authentication (#1327) --- packages/authentication-local/src/strategy.ts | 7 +-- packages/authentication-oauth/src/strategy.ts | 5 -- .../test/strategy.test.ts | 12 ----- packages/authentication/src/core.ts | 35 +++--------- packages/authentication/src/jwt.ts | 4 +- packages/authentication/test/core.test.ts | 54 +++++-------------- .../test/hooks/authenticate.test.ts | 1 + packages/authentication/test/jwt.test.ts | 2 +- packages/authentication/test/service.test.ts | 2 +- packages/express/test/authentication.test.js | 2 +- 10 files changed, 27 insertions(+), 97 deletions(-) diff --git a/packages/authentication-local/src/strategy.ts b/packages/authentication-local/src/strategy.ts index 73b453f9a9..f76cbe4c88 100644 --- a/packages/authentication-local/src/strategy.ts +++ b/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); diff --git a/packages/authentication-oauth/src/strategy.ts b/packages/authentication-oauth/src/strategy.ts index 681d5c7926..28891ec8ad 100644 --- a/packages/authentication-oauth/src/strategy.ts +++ b/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) diff --git a/packages/authentication-oauth/test/strategy.test.ts b/packages/authentication-oauth/test/strategy.test.ts index cd2f723c03..bfd39a3fe1 100644 --- a/packages/authentication-oauth/test/strategy.test.ts +++ b/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', diff --git a/packages/authentication/src/core.ts b/packages/authentication/src/core.ts index 269fea200d..a3e7493af2 100644 --- a/packages/authentication/src/core.ts +++ b/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; + }); } /** diff --git a/packages/authentication/src/jwt.ts b/packages/authentication/src/jwt.ts index c6575367d7..c2bb3538c1 100644 --- a/packages/authentication/src/jwt.ts +++ b/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'); } diff --git a/packages/authentication/test/core.test.ts b/packages/authentication/test/core.test.ts index fb2e6df7df..1e0b2e2a2f 100644 --- a/packages/authentication/test/core.test.ts +++ b/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)'); } }); }); diff --git a/packages/authentication/test/hooks/authenticate.test.ts b/packages/authentication/test/hooks/authenticate.test.ts index 8c4d143a59..b565255216 100644 --- a/packages/authentication/test/hooks/authenticate.test.ts +++ b/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' } }); diff --git a/packages/authentication/test/jwt.test.ts b/packages/authentication/test/jwt.test.ts index 526bae8c1c..df5618f558 100644 --- a/packages/authentication/test/jwt.test.ts +++ b/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)'); } }); diff --git a/packages/authentication/test/service.test.ts b/packages/authentication/test/service.test.ts index 7ef32e0841..5b6a3e9d3a 100644 --- a/packages/authentication/test/service.test.ts +++ b/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)'); } }); }); diff --git a/packages/express/test/authentication.test.js b/packages/express/test/authentication.test.js index 270e163f0e..6121413e1d 100644 --- a/packages/express/test/authentication.test.js +++ b/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)'); }); });