From 759c5a19327a731af965c3604119393b3d09a406 Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Sat, 22 May 2021 09:20:41 -0700
Subject: [PATCH] fix(hooks): Migrate built-in hooks and allow backwards
 compatibility (#2358)

---
 .../src/hooks/authentication.ts               | 10 ++--
 .../src/hooks/populate-header.ts              |  6 +-
 packages/authentication-client/src/index.ts   | 12 ++--
 .../src/hooks/hash-password.ts                | 57 +++++++++----------
 .../authentication-local/src/hooks/protect.ts | 37 ++++++------
 packages/authentication-local/test/fixture.ts | 26 ++++-----
 .../test/hooks/hash-password.test.ts          | 22 -------
 .../test/hooks/protect.test.ts                | 54 ++++++++++--------
 .../authentication/src/hooks/authenticate.ts  | 11 ++--
 .../authentication/src/hooks/connection.ts    | 20 +++----
 packages/authentication/src/hooks/event.ts    |  8 +--
 packages/authentication/src/service.ts        |  6 +-
 .../test/hooks/authenticate.test.ts           |  4 +-
 .../authentication/test/hooks/event.test.ts   |  6 +-
 14 files changed, 122 insertions(+), 157 deletions(-)

diff --git a/packages/authentication-client/src/hooks/authentication.ts b/packages/authentication-client/src/hooks/authentication.ts
index 653b7deabb..52ce76704d 100644
--- a/packages/authentication-client/src/hooks/authentication.ts
+++ b/packages/authentication-client/src/hooks/authentication.ts
@@ -1,20 +1,18 @@
-import { HookContext } from '@feathersjs/feathers';
+import { HookContext, NextFunction } from '@feathersjs/feathers';
 import { stripSlashes } from '@feathersjs/commons';
 
 export const authentication = () => {
-  return (context: HookContext) => {
+  return (context: HookContext, next: NextFunction) => {
     const { app, params, path, method, app: { authentication: service } } = context;
 
     if (stripSlashes(service.options.path) === path && method === 'create') {
-      return context;
+      return next();
     }
 
     return Promise.resolve(app.get('authentication')).then(authResult => {
       if (authResult) {
         context.params = Object.assign({}, authResult, params);
       }
-
-      return context;
-    });
+    }).then(next);
   };
 };
diff --git a/packages/authentication-client/src/hooks/populate-header.ts b/packages/authentication-client/src/hooks/populate-header.ts
index 4907020534..ebafe8811d 100644
--- a/packages/authentication-client/src/hooks/populate-header.ts
+++ b/packages/authentication-client/src/hooks/populate-header.ts
@@ -1,7 +1,7 @@
-import { HookContext } from '@feathersjs/feathers';
+import { HookContext, NextFunction } from '@feathersjs/feathers';
 
 export const populateHeader = () => {
-  return (context: HookContext) => {
+  return (context: HookContext, next: NextFunction) => {
     const { app, params: { accessToken } } = context;
     const authentication = app.authentication;
 
@@ -15,6 +15,6 @@ export const populateHeader = () => {
       }, context.params.headers);
     }
 
-    return context;
+    return next();
   };
 };
diff --git a/packages/authentication-client/src/index.ts b/packages/authentication-client/src/index.ts
index 34f405b971..a9a1909cbe 100644
--- a/packages/authentication-client/src/index.ts
+++ b/packages/authentication-client/src/index.ts
@@ -52,14 +52,10 @@ const init = (_options: Partial<AuthenticationClientOptions> = {}) => {
     app.reAuthenticate = authentication.reAuthenticate.bind(authentication);
     app.logout = authentication.logout.bind(authentication);
 
-    app.hooks({
-      before: {
-        all: [
-          hooks.authentication(),
-          hooks.populateHeader()
-        ]
-      }
-    });
+    app.hooks([
+      hooks.authentication(),
+      hooks.populateHeader()
+    ]);
   };
 };
 
diff --git a/packages/authentication-local/src/hooks/hash-password.ts b/packages/authentication-local/src/hooks/hash-password.ts
index d2be8f8cca..eeca165586 100644
--- a/packages/authentication-local/src/hooks/hash-password.ts
+++ b/packages/authentication-local/src/hooks/hash-password.ts
@@ -3,7 +3,7 @@ import set from 'lodash/set';
 import cloneDeep from 'lodash/cloneDeep';
 import { BadRequest } from '@feathersjs/errors';
 import { createDebug } from '@feathersjs/commons';
-import { HookContext } from '@feathersjs/feathers';
+import { HookContext, NextFunction } from '@feathersjs/feathers';
 import { LocalStrategy } from '../strategy';
 
 const debug = createDebug('@feathersjs/authentication-local/hooks/hash-password');
@@ -18,47 +18,42 @@ export default function hashPassword (field: string, options: HashPasswordOption
     throw new Error('The hashPassword hook requires a field name option');
   }
 
-  return async (context: HookContext<any, any>) => {
-    if (context.type !== 'before') {
-      throw new Error('The \'hashPassword\' hook should only be used as a \'before\' hook');
-    }
-
+  return async (context: HookContext<any, any>, next?: NextFunction) => {
     const { app, data, params } = context;
 
-    if (data === undefined) {
-      debug('hook.data is undefined. Skipping hashPassword hook.');
-      return context;
-    }
+    if (data !== undefined) {
+      const authService = app.defaultAuthentication(options.authentication);
+      const { strategy = 'local' } = options;
 
-    const authService = app.defaultAuthentication(options.authentication);
-    const { strategy = 'local' } = options;
+      if (!authService || typeof authService.getStrategies !== 'function') {
+        throw new BadRequest('Could not find an authentication service to hash password');
+      }
 
-    if (!authService || typeof authService.getStrategies !== 'function') {
-      throw new BadRequest('Could not find an authentication service to hash password');
-    }
+      const [ localStrategy ] = authService.getStrategies(strategy) as LocalStrategy[];
 
-    const [ localStrategy ] = authService.getStrategies(strategy) as LocalStrategy[];
+      if (!localStrategy || typeof localStrategy.hashPassword !== 'function') {
+        throw new BadRequest(`Could not find '${strategy}' strategy to hash password`);
+      }
 
-    if (!localStrategy || typeof localStrategy.hashPassword !== 'function') {
-      throw new BadRequest(`Could not find '${strategy}' strategy to hash password`);
-    }
+      const addHashedPassword = async (data: any) => {
+        const password = get(data, field);
 
-    const addHashedPassword = async (data: any) => {
-      const password = get(data, field);
+        if (password === undefined) {
+          debug(`hook.data.${field} is undefined, not hashing password`);
+          return data;
+        }
 
-      if (password === undefined) {
-        debug(`hook.data.${field} is undefined, not hashing password`);
-        return data;
-      }
+        const hashedPassword: string = await localStrategy.hashPassword(password, params);
 
-      const hashedPassword: string = await localStrategy.hashPassword(password, params);
+        return set(cloneDeep(data), field, hashedPassword);
+      }
 
-      return set(cloneDeep(data), field, hashedPassword);
+      context.data = Array.isArray(data) ? await Promise.all(data.map(addHashedPassword)) :
+        await addHashedPassword(data);
     }
 
-    context.data = Array.isArray(data) ? await Promise.all(data.map(addHashedPassword)) :
-      await addHashedPassword(data);
-
-    return context;
+    if (typeof next === 'function') {
+      await next();
+    }
   };
 }
diff --git a/packages/authentication-local/src/hooks/protect.ts b/packages/authentication-local/src/hooks/protect.ts
index f9cf2ebe7c..5e2cccbc56 100644
--- a/packages/authentication-local/src/hooks/protect.ts
+++ b/packages/authentication-local/src/hooks/protect.ts
@@ -1,8 +1,7 @@
 import omit from 'lodash/omit';
-import { HookContext } from '@feathersjs/feathers';
+import { HookContext, NextFunction } from '@feathersjs/feathers';
 
-export default (...fields: string[]) => (context: HookContext<any, any>) => {
-  const result = context.dispatch || context.result;
+export default (...fields: string[]) => async (context: HookContext<any, any>, next?: NextFunction) => {
   const o = (current: any) => {
     if (typeof current === 'object' && !Array.isArray(current)) {
       const data = typeof current.toJSON === 'function'
@@ -14,23 +13,25 @@ export default (...fields: string[]) => (context: HookContext<any, any>) => {
     return current;
   };
 
-  if (!result) {
-    return context;
+  if (typeof next === 'function') {
+    await next();
   }
 
-  if (Array.isArray(result)) {
-    context.dispatch = result.map(o);
-  } else if (result.data && context.method === 'find') {
-    context.dispatch = Object.assign({}, result, {
-      data: result.data.map(o)
-    });
-  } else {
-    context.dispatch = o(result);
-  }
+  const result = context.dispatch || context.result;
 
-  if (context.params && context.params.provider) {
-    context.result = context.dispatch;
-  }
+  if (result) {
+    if (Array.isArray(result)) {
+      context.dispatch = result.map(o);
+    } else if (result.data && context.method === 'find') {
+      context.dispatch = Object.assign({}, result, {
+        data: result.data.map(o)
+      });
+    } else {
+      context.dispatch = o(result);
+    }
 
-  return context;
+    if (context.params && context.params.provider) {
+      context.result = context.dispatch;
+    }
+  }
 };
diff --git a/packages/authentication-local/test/fixture.ts b/packages/authentication-local/test/fixture.ts
index 55ac905d7d..040334244a 100644
--- a/packages/authentication-local/test/fixture.ts
+++ b/packages/authentication-local/test/fixture.ts
@@ -37,20 +37,20 @@ export function createApplication (app = feathers<ServiceTypes>()) {
     }
   }));
 
+  app.service('users').hooks([
+    protect('password')
+  ]);
   app.service('users').hooks({
-    before: {
-      create: [ hashPassword('password') ]
-    },
-    after: {
-      all: [ protect('password') ],
-      get: [context => {
-        if (context.params.provider) {
-          context.result.fromGet = true;
-        }
-
-        return context;
-      }]
-    }
+    create: [
+      hashPassword('password')
+    ],
+    get: [ async (context, next) => {
+      await next();
+
+      if (context.params.provider) {
+        context.result.fromGet = true;
+      }
+    }]
   });
 
   return app;
diff --git a/packages/authentication-local/test/hooks/hash-password.test.ts b/packages/authentication-local/test/hooks/hash-password.test.ts
index fba825a855..836e47e756 100644
--- a/packages/authentication-local/test/hooks/hash-password.test.ts
+++ b/packages/authentication-local/test/hooks/hash-password.test.ts
@@ -55,28 +55,6 @@ describe('@feathersjs/authentication-local/hooks/hash-password', () => {
     }
   });
 
-  it('errors when authentication strategy does not exist', async () => {
-    const users = app.service('users');
-
-    users.hooks({
-      after: {
-        create: hashPassword('password')
-      }
-    });
-
-    try {
-      await users.create({
-        email: 'dave@hashpassword.com',
-        password: 'supersecret'
-      });
-      assert.fail('Should never get here');
-    } catch (error) {
-      assert.strictEqual(error.message,
-        'The \'hashPassword\' hook should only be used as a \'before\' hook'
-      );
-    }
-  });
-
   it('hashes password on field', async () => {
     const password = 'supersecret';
 
diff --git a/packages/authentication-local/test/hooks/protect.test.ts b/packages/authentication-local/test/hooks/protect.test.ts
index 994c579402..a5971644f5 100644
--- a/packages/authentication-local/test/hooks/protect.test.ts
+++ b/packages/authentication-local/test/hooks/protect.test.ts
@@ -8,7 +8,7 @@ function testOmit (title: string, property: string) {
   describe(title, () => {
     const fn = protect('password');
 
-    it('omits from object', () => {
+    it('omits from object', async () => {
       const data = {
         email: 'test@user.com',
         password: 'supersecret'
@@ -16,15 +16,16 @@ function testOmit (title: string, property: string) {
       const context = {
         [property]: data
       } as unknown as HookContext;
-      const result = fn(context);
 
-      assert.deepStrictEqual(result, {
+      await fn(context);
+
+      assert.deepStrictEqual(context, {
         [property]: data,
         dispatch: { email: 'test@user.com' }
       });
     });
 
-    it('omits from nested object', () => {
+    it('omits from nested object', async () => {
       const hook = protect('user.password');
       const data = {
         user: {
@@ -35,15 +36,16 @@ function testOmit (title: string, property: string) {
       const context = {
         [property]: data
       } as unknown as HookContext;
-      const result = hook(context);
 
-      assert.deepStrictEqual(result, {
+      await hook(context);
+
+      assert.deepStrictEqual(context, {
         [property]: data,
         dispatch: { user: { email: 'test@user.com' } }
       });
     });
 
-    it('handles `data` property only for find', () => {
+    it('handles `data` property only for find', async () => {
       const data = {
         email: 'test@user.com',
         password: 'supersecret',
@@ -52,15 +54,16 @@ function testOmit (title: string, property: string) {
       const context = {
         [property]: data
       } as unknown as HookContext;
-      const result = fn(context);
 
-      assert.deepStrictEqual(result, {
+      await fn(context);
+
+      assert.deepStrictEqual(context, {
         [property]: data,
         dispatch: { email: 'test@user.com', data: 'yes' }
       });
     });
 
-    it('uses .toJSON (#48)', () => {
+    it('uses .toJSON (#48)', async () => {
       class MyUser {
         toJSON () {
           return {
@@ -74,15 +77,16 @@ function testOmit (title: string, property: string) {
       const context = {
         [property]: data
       } as unknown as HookContext;
-      const result = fn(context);
 
-      assert.deepStrictEqual(result, {
+      await fn(context);
+
+      assert.deepStrictEqual(context, {
         [property]: data,
         dispatch: { email: 'test@user.com' }
       });
     });
 
-    it('omits from array but only objects (#2053)', () => {
+    it('omits from array but only objects (#2053)', async () => {
       const data = [{
         email: 'test1@user.com',
         password: 'supersecret'
@@ -95,9 +99,10 @@ function testOmit (title: string, property: string) {
       const context = {
         [property]: data
       } as unknown as HookContext;
-      const result = fn(context);
 
-      assert.deepStrictEqual(result, {
+      await fn(context);
+
+      assert.deepStrictEqual(context, {
         [property]: data,
         dispatch: [
           { email: 'test1@user.com' },
@@ -108,7 +113,7 @@ function testOmit (title: string, property: string) {
       });
     });
 
-    it('omits from pagination object', () => {
+    it('omits from pagination object', async () => {
       const data = {
         total: 2,
         data: [{
@@ -123,9 +128,10 @@ function testOmit (title: string, property: string) {
         method: 'find',
         [property]: data
       } as unknown as HookContext;
-      const result = fn(context);
 
-      assert.deepStrictEqual(result, {
+      await fn(context);
+
+      assert.deepStrictEqual(context, {
         method: 'find',
         [property]: data,
         dispatch: {
@@ -138,7 +144,7 @@ function testOmit (title: string, property: string) {
       });
     });
 
-    it('updates result if params.provider is set', () => {
+    it('updates result if params.provider is set', async () => {
       const data = [{
         email: 'test1@user.com',
         password: 'supersecret'
@@ -151,9 +157,10 @@ function testOmit (title: string, property: string) {
         [property]: data,
         params
       } as unknown as HookContext;
-      const result = fn(context);
 
-      assert.deepStrictEqual(result, {
+      await fn(context);
+
+      assert.deepStrictEqual(context, {
         [property]: data,
         params,
         result: [
@@ -170,11 +177,10 @@ function testOmit (title: string, property: string) {
 }
 
 describe('@feathersjs/authentication-local/hooks/protect', () => {
-  it('does nothing when called with no result', () => {
+  it('does nothing when called with no result', async () => {
     const fn = protect();
-    const original = {} as unknown as HookContext;
 
-    assert.deepStrictEqual(fn(original), original);
+    assert.deepStrictEqual(await fn({} as any), undefined);
   });
 
   testOmit('with hook.result', 'result');
diff --git a/packages/authentication/src/hooks/authenticate.ts b/packages/authentication/src/hooks/authenticate.ts
index 48e23a0cf2..67ff3f6101 100644
--- a/packages/authentication/src/hooks/authenticate.ts
+++ b/packages/authentication/src/hooks/authenticate.ts
@@ -1,6 +1,6 @@
 import flatten from 'lodash/flatten';
 import omit from 'lodash/omit';
-import { HookContext } from '@feathersjs/feathers';
+import { HookContext, NextFunction } from '@feathersjs/feathers';
 import { NotAuthenticated } from '@feathersjs/errors';
 import { createDebug } from '@feathersjs/commons';
 
@@ -20,7 +20,8 @@ export default (originalSettings: string | AuthenticateHookSettings, ...original
     throw new Error('The authenticate hook needs at least one allowed strategy');
   }
 
-  return async (context: HookContext<any, any>) => {
+  return async (context: HookContext<any, any>, _next?: NextFunction) => {
+    const next = typeof _next === 'function' ? _next : async () => context;
     const { app, params, type, path, service } = context;
     const { strategies } = settings;
     const { provider, authentication } = params;
@@ -42,7 +43,7 @@ export default (originalSettings: string | AuthenticateHookSettings, ...original
     }
 
     if (params.authenticated === true) {
-      return context;
+      return next();
     }
 
     if (authentication) {
@@ -53,12 +54,10 @@ export default (originalSettings: string | AuthenticateHookSettings, ...original
       const authResult = await authService.authenticate(authentication, authParams, ...strategies);
 
       context.params = Object.assign({}, params, omit(authResult, 'accessToken'), { authenticated: true });
-
-      return context;
     } else if (provider) {
       throw new NotAuthenticated('Not authenticated');
     }
 
-    return context;
+    return next();
   };
 };
diff --git a/packages/authentication/src/hooks/connection.ts b/packages/authentication/src/hooks/connection.ts
index e378248d80..c8c01f0e6c 100644
--- a/packages/authentication/src/hooks/connection.ts
+++ b/packages/authentication/src/hooks/connection.ts
@@ -1,19 +1,17 @@
-import { HookContext } from '@feathersjs/feathers';
+import { HookContext, NextFunction } from '@feathersjs/feathers';
 import omit from 'lodash/omit';
 import { AuthenticationBase, ConnectionEvent } from '../core';
 
-export default (event: ConnectionEvent) => async (context: HookContext) => {
-  const { result, params: { connection } } = context;
-
-  if (!connection) {
-    return context;
-  }
+export default (event: ConnectionEvent) => async (context: HookContext, next: NextFunction) => {
+  await next();
 
-  const service = context.service as unknown as AuthenticationBase;
+  const { result, params: { connection } } = context;
 
-  Object.assign(connection, omit(result, 'accessToken', 'authentication'));
+  if (connection) {
+    const service = context.service as unknown as AuthenticationBase;
 
-  await service.handleConnection(event, connection, result);
+    Object.assign(connection, omit(result, 'accessToken', 'authentication'));
 
-  return context;
+    await service.handleConnection(event, connection, result);
+  }
 };
diff --git a/packages/authentication/src/hooks/event.ts b/packages/authentication/src/hooks/event.ts
index 4ca8c55043..4525db1aaf 100644
--- a/packages/authentication/src/hooks/event.ts
+++ b/packages/authentication/src/hooks/event.ts
@@ -1,16 +1,16 @@
-import { HookContext } from '@feathersjs/feathers';
+import { HookContext, NextFunction } from '@feathersjs/feathers';
 import { createDebug } from '@feathersjs/commons';
 import { ConnectionEvent } from '../core';
 
 const debug = createDebug('@feathersjs/authentication/hooks/connection');
 
-export default (event: ConnectionEvent) => async (context: HookContext) => {
+export default (event: ConnectionEvent) => async (context: HookContext, next: NextFunction) => {
+  await next();
+
   const { app, result, params } = context;
 
   if (params.provider && result) {
     debug(`Sending authentication event '${event}'`);
     app.emit(event, result, params, context);
   }
-
-  return context;
 };
diff --git a/packages/authentication/src/service.ts b/packages/authentication/src/service.ts
index 04ab7d9e3e..ef87cef7c6 100644
--- a/packages/authentication/src/service.ts
+++ b/packages/authentication/src/service.ts
@@ -172,10 +172,8 @@ export class AuthenticationService extends AuthenticationBase implements Partial
     }
 
     (this as any).hooks({
-      after: {
-        create: [ connection('login'), event('login') ],
-        remove: [ connection('logout'), event('logout') ]
-      }
+      create: [ connection('login'), event('login') ],
+      remove: [ connection('logout'), event('logout') ]
     });
 
     this.app.on('disconnect', async (connection) => {
diff --git a/packages/authentication/test/hooks/authenticate.test.ts b/packages/authentication/test/hooks/authenticate.test.ts
index c506002b11..33639248f8 100644
--- a/packages/authentication/test/hooks/authenticate.test.ts
+++ b/packages/authentication/test/hooks/authenticate.test.ts
@@ -47,9 +47,7 @@ describe('authentication/hooks/authenticate', () => {
     app.service('auth-v2').register('test', new Strategy1());
 
     app.service('users').hooks({
-      before: {
-        get: authenticate('first', 'second')
-      }
+      get: [authenticate('first', 'second')]
     });
 
     app.service('users').id = 'name';
diff --git a/packages/authentication/test/hooks/event.test.ts b/packages/authentication/test/hooks/event.test.ts
index 9b16006d87..592050454d 100644
--- a/packages/authentication/test/hooks/event.test.ts
+++ b/packages/authentication/test/hooks/event.test.ts
@@ -18,10 +18,8 @@ describe('authentication/hooks/events', () => {
   const service = app.service('authentication');
 
   service.hooks({
-    after: {
-      create: [ hook('login') ],
-      remove: [ hook('logout') ]
-    }
+    create: [ hook('login') ],
+    remove: [ hook('logout') ]
   });
 
   it('login', done => {