From 40ebbddeae3995e840c059146a6e3d3597087107 Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Tue, 7 May 2019 19:00:20 -0700
Subject: [PATCH 1/3] fix: Improve oAuth option handling

---
 packages/authentication-oauth/src/express.ts  | 33 ++++++------
 packages/authentication-oauth/src/index.ts    |  2 +-
 packages/authentication-oauth/src/strategy.ts | 50 ++++++++++++++-----
 packages/authentication-oauth/src/utils.ts    | 38 +++++---------
 .../authentication-oauth/test/index.test.ts   |  4 +-
 .../test/strategy.test.ts                     | 25 +++++++++-
 .../authentication-oauth/test/utils.test.ts   | 28 +----------
 7 files changed, 94 insertions(+), 86 deletions(-)

diff --git a/packages/authentication-oauth/src/express.ts b/packages/authentication-oauth/src/express.ts
index f69f6de16e..9e30404fb9 100644
--- a/packages/authentication-oauth/src/express.ts
+++ b/packages/authentication-oauth/src/express.ts
@@ -1,7 +1,6 @@
 // @ts-ignore
 import { express as grantExpress } from 'grant';
 import Debug from 'debug';
-import session from 'express-session';
 import querystring from 'querystring';
 import { Application } from '@feathersjs/feathers';
 import { AuthenticationService, AuthenticationResult } from '@feathersjs/authentication';
@@ -10,6 +9,7 @@ import {
   original as express
 } from '@feathersjs/express';
 import { OauthSetupSettings } from './utils';
+import { OAuthStrategy } from '.';
 
 const grant = grantExpress();
 const debug = Debug('@feathersjs/authentication-oauth/express');
@@ -19,7 +19,6 @@ export default (options: OauthSetupSettings) => {
     const { path, authService, linkStrategy } = options;
     const app = feathersApp as ExpressApplication;
     const config = app.get('grant');
-    const secret = Math.random().toString(36).substring(7);
 
     if (!config) {
       debug('No grant configuration found, skipping Express oAuth setup');
@@ -29,11 +28,7 @@ export default (options: OauthSetupSettings) => {
     const grantApp = grant(config);
     const authApp = express();
 
-    authApp.use(session({
-      secret,
-      resave: true,
-      saveUninitialized: true
-    }));
+    authApp.use(options.expressSession);
 
     authApp.get('/:name', (req, res) => {
       const { name } = req.params;
@@ -56,15 +51,21 @@ export default (options: OauthSetupSettings) => {
       const { name } = req.params;
       const { accessToken, grant } = req.session;
       const service: AuthenticationService = app.service(authService);
+      const [ strategy ] = service.getStrategies(name) as OAuthStrategy[];
       const sendResponse = async (data: AuthenticationResult|Error) => {
-        const redirect = await options.getRedirect(service, data);
-
-        if (redirect !== null) {
-          res.redirect(redirect);
-        } else if (data instanceof Error) {
-          next(data);
-        } else {
-          res.json(data);
+        try {
+          const redirect = await strategy.getRedirect(data);
+
+          if (redirect !== null) {
+            res.redirect(redirect);
+          } else if (data instanceof Error) {
+            throw data;
+          } else {
+            res.json(data);
+          }
+        } catch (error) {
+          debug('oAuth error', error);
+          next(error);
         }
       };
 
@@ -95,7 +96,7 @@ export default (options: OauthSetupSettings) => {
         await sendResponse(authResult);
       } catch (error) {
         debug('Received oAuth authentication error', error);
-        sendResponse(error);
+        await sendResponse(error);
       }
     });
 
diff --git a/packages/authentication-oauth/src/index.ts b/packages/authentication-oauth/src/index.ts
index 0e928c4ab7..2770438655 100644
--- a/packages/authentication-oauth/src/index.ts
+++ b/packages/authentication-oauth/src/index.ts
@@ -49,7 +49,7 @@ export const setup = (options: OauthSetupSettings) => (app: Application) => {
   app.set('grant', grant);
 };
 
-export const express = (settings: OauthSetupSettings = {}) => (app: Application) => {
+export const express = (settings: Partial<OauthSetupSettings> = {}) => (app: Application) => {
   const options = getDefaultSettings(app, settings);
 
   app.configure(setup(options));
diff --git a/packages/authentication-oauth/src/strategy.ts b/packages/authentication-oauth/src/strategy.ts
index 28891ec8ad..bfa84c5ba2 100644
--- a/packages/authentication-oauth/src/strategy.ts
+++ b/packages/authentication-oauth/src/strategy.ts
@@ -1,8 +1,9 @@
 // @ts-ignore
 import getProfile from 'grant-profile/lib/client';
+import querystring from 'querystring';
 import Debug from 'debug';
 import {
-  AuthenticationRequest, AuthenticationBaseStrategy
+  AuthenticationRequest, AuthenticationBaseStrategy, AuthenticationResult
 } from '@feathersjs/authentication';
 import { Params } from '@feathersjs/feathers';
 
@@ -29,6 +30,18 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
     return this.configuration.entityId || this.entityService.id;
   }
 
+  async getEntityQuery (profile: OAuthProfile, _params: Params) {
+    return {
+      [`${this.name}Id`]: profile.sub || profile.id
+    };
+  }
+
+  async getEntityData (profile: OAuthProfile, _existingEntity: any, _params: Params) {
+    return {
+      [`${this.name}Id`]: profile.id
+    };
+  }
+
   /* istanbul ignore next */
   async getProfile (data: AuthenticationRequest, _params: Params) {
     const config = this.app.get('grant');
@@ -50,17 +63,33 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
       const authResult = await this.authentication
         .authenticate(authentication, params, strategy);
 
-      return authResult[entity] || null;
+      return authResult[entity];
     }
 
     return null;
   }
 
-  async findEntity (profile: OAuthProfile, params: Params) {
-    const query = {
-      [`${this.name}Id`]: profile.id
+  async getRedirect (data: AuthenticationResult|Error) {
+    const { redirect } = this.authentication.configuration.oauth;
+
+    if (!redirect) {
+      return null;
+    }
+
+    const separator = redirect.endsWith('?') ? '' : '#';
+    const authResult: AuthenticationResult = data;
+    const query = authResult.accessToken ? {
+      access_token: authResult.accessToken
+    } : {
+      error: data.message || 'OAuth Authentication not successful'
     };
 
+    return redirect + separator + querystring.stringify(query);
+  }
+
+  async findEntity (profile: OAuthProfile, params: Params) {
+    const query = await this.getEntityQuery(profile, params);
+
     debug('findEntity with query', query);
 
     const result = await this.entityService.find({
@@ -75,9 +104,7 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
   }
 
   async createEntity (profile: OAuthProfile, params: Params) {
-    const data = {
-      [`${this.name}Id`]: profile.id
-    };
+    const data = await this.getEntityData(profile, null, params);
 
     debug('createEntity with data', data);
 
@@ -86,9 +113,7 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
 
   async updateEntity (entity: any, profile: OAuthProfile, params: Params) {
     const id = entity[this.entityId];
-    const data = {
-      [`${this.name}Id`]: profile.id
-    };
+    const data = await this.getEntityData(profile, entity, params);
 
     debug(`updateEntity with id ${id} and data`, data);
 
@@ -103,8 +128,7 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
 
     debug(`authenticate with (existing) entity`, existingEntity);
 
-    const authEntity = existingEntity === null
-      ? await this.createEntity(profile, params)
+    const authEntity = !existingEntity ? await this.createEntity(profile, params)
       : await this.updateEntity(existingEntity, profile, params);
 
     return {
diff --git a/packages/authentication-oauth/src/utils.ts b/packages/authentication-oauth/src/utils.ts
index 04bda995d0..a5737abcee 100644
--- a/packages/authentication-oauth/src/utils.ts
+++ b/packages/authentication-oauth/src/utils.ts
@@ -1,38 +1,24 @@
-import querystring from 'querystring';
+import { RequestHandler } from 'express';
+import session from 'express-session';
 import { Application } from '@feathersjs/feathers';
-import { AuthenticationService, AuthenticationResult } from '@feathersjs/authentication';
 
 export interface OauthSetupSettings {
-  path?: string;
-  authService?: string;
-  linkStrategy?: string;
-  getRedirect? (service: AuthenticationService, data: AuthenticationResult|Error): Promise<string>;
+  path: string;
+  authService: string;
+  linkStrategy: string;
+  expressSession: RequestHandler;
 }
 
-export const getRedirect = async (service: AuthenticationService, data: AuthenticationResult|Error) => {
-  const { redirect } = service.configuration.oauth;
-
-  if (!redirect) {
-    return null;
-  }
-
-  const separator = redirect.endsWith('?') ? '' : '#';
-  const authResult: AuthenticationResult = data;
-  const query = authResult.accessToken ? {
-    access_token: authResult.accessToken
-  } : {
-    error: data.message || 'OAuth Authentication not successful'
-  };
-
-  return redirect + separator + querystring.stringify(query);
-};
-
-export const getDefaultSettings = (app: Application, other?: OauthSetupSettings) => {
+export const getDefaultSettings = (app: Application, other?: Partial<OauthSetupSettings>) => {
   const defaults: OauthSetupSettings = {
     path: '/auth',
     authService: app.get('defaultAuthentication'),
     linkStrategy: 'jwt',
-    getRedirect,
+    expressSession: session({
+      secret: Math.random().toString(36).substring(7),
+      saveUninitialized: true,
+      resave: true
+    }),
     ...other
   };
 
diff --git a/packages/authentication-oauth/test/index.test.ts b/packages/authentication-oauth/test/index.test.ts
index e03b284d43..834f065666 100644
--- a/packages/authentication-oauth/test/index.test.ts
+++ b/packages/authentication-oauth/test/index.test.ts
@@ -1,6 +1,6 @@
 import { strict as assert } from 'assert';
 import feathers from '@feathersjs/feathers';
-import { setup, express } from '../src';
+import { setup, express, OauthSetupSettings } from '../src';
 import { AuthenticationService } from '@feathersjs/authentication/lib';
 
 describe('@feathersjs/authentication-oauth', () => {
@@ -9,7 +9,7 @@ describe('@feathersjs/authentication-oauth', () => {
       const app = feathers();
 
       try {
-        app.configure(setup({ authService: 'something' }));
+        app.configure(setup({ authService: 'something' } as OauthSetupSettings));
         assert.fail('Should never get here');
       } catch (error) {
         assert.equal(error.message,
diff --git a/packages/authentication-oauth/test/strategy.test.ts b/packages/authentication-oauth/test/strategy.test.ts
index bfd39a3fe1..19d924ff24 100644
--- a/packages/authentication-oauth/test/strategy.test.ts
+++ b/packages/authentication-oauth/test/strategy.test.ts
@@ -4,7 +4,7 @@ import { AuthenticationService } from '@feathersjs/authentication/lib';
 
 describe('@feathersjs/authentication-oauth/strategy', () => {
   const authService: AuthenticationService = app.service('authentication');
-  const [strategy] = authService.getStrategies('test') as TestOAuthStrategy[];
+  const [ strategy ] = authService.getStrategies('test') as TestOAuthStrategy[];
 
   it('initializes, has .entityId and configuration', () => {
     assert.ok(strategy);
@@ -12,6 +12,29 @@ describe('@feathersjs/authentication-oauth/strategy', () => {
     assert.ok(strategy.configuration.entity);
   });
 
+  it('getRedirect', async () => {
+    app.get('authentication').oauth.redirect = '/home';
+
+    let redirect = await strategy.getRedirect({ accessToken: 'testing' });
+    assert.equal(redirect, '/home#access_token=testing');
+
+    redirect = await strategy.getRedirect(new Error('something went wrong'));
+    assert.equal(redirect, '/home#error=something%20went%20wrong');
+
+    redirect = await strategy.getRedirect(new Error());
+    assert.equal(redirect, '/home#error=OAuth%20Authentication%20not%20successful');
+
+    app.get('authentication').oauth.redirect = '/home?';
+
+    redirect = await strategy.getRedirect({ accessToken: 'testing' });
+    assert.equal(redirect, '/home?access_token=testing');
+
+    delete app.get('authentication').oauth.redirect;
+
+    redirect = await strategy.getRedirect({ accessToken: 'testing' });
+    assert.equal(redirect, null);
+  });
+
   it('getProfile', async () => {
     const data = { id: 'getProfileTest' };
     const profile = await strategy.getProfile(data, {});
diff --git a/packages/authentication-oauth/test/utils.test.ts b/packages/authentication-oauth/test/utils.test.ts
index edd03a982d..9f1753a43f 100644
--- a/packages/authentication-oauth/test/utils.test.ts
+++ b/packages/authentication-oauth/test/utils.test.ts
@@ -1,34 +1,8 @@
 import { strict as assert } from 'assert';
-import { getRedirect, getDefaultSettings } from '../src/utils';
+import { getDefaultSettings } from '../src/utils';
 import { app } from './fixture';
-import { AuthenticationService } from '@feathersjs/authentication/lib';
 
 describe('@feathersjs/authentication-oauth/utils', () => {
-  it('getRedirect', async () => {
-    const service: AuthenticationService = app.service('authentication');
-
-    app.get('authentication').oauth.redirect = '/home';
-
-    let redirect = await getRedirect(service, { accessToken: 'testing' });
-    assert.equal(redirect, '/home#access_token=testing');
-
-    redirect = await getRedirect(service, { message: 'something went wrong' });
-    assert.equal(redirect, '/home#error=something%20went%20wrong');
-
-    redirect = await getRedirect(service, {});
-    assert.equal(redirect, '/home#error=OAuth%20Authentication%20not%20successful');
-
-    app.get('authentication').oauth.redirect = '/home?';
-
-    redirect = await getRedirect(service, { accessToken: 'testing' });
-    assert.equal(redirect, '/home?access_token=testing');
-
-    delete app.get('authentication').oauth.redirect;
-
-    redirect = await getRedirect(service, { accessToken: 'testing' });
-    assert.equal(redirect, null);
-  });
-
   it('getDefaultSettings', () => {
     const settings = getDefaultSettings(app);
 

From 3d7028b64e89f2f86e48ec429b4ca7eb923edcb5 Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Wed, 8 May 2019 09:22:01 -0700
Subject: [PATCH 2/3] Fix getEntityData

---
 packages/authentication-oauth/src/strategy.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/authentication-oauth/src/strategy.ts b/packages/authentication-oauth/src/strategy.ts
index bfa84c5ba2..fb2d4e700c 100644
--- a/packages/authentication-oauth/src/strategy.ts
+++ b/packages/authentication-oauth/src/strategy.ts
@@ -38,7 +38,7 @@ export class OAuthStrategy extends AuthenticationBaseStrategy {
 
   async getEntityData (profile: OAuthProfile, _existingEntity: any, _params: Params) {
     return {
-      [`${this.name}Id`]: profile.id
+      [`${this.name}Id`]: profile.sub || profile.id
     };
   }
 

From eea8d669fba57929acfa6b7b8b5f5ec21e7aec97 Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Wed, 8 May 2019 13:34:46 -0700
Subject: [PATCH 3/3] Use proper path configuration and do not pass provider in
 oAuth authentication

---
 packages/authentication-oauth/src/express.ts     |  8 ++++----
 packages/authentication-oauth/src/index.ts       | 13 +++++++------
 packages/authentication-oauth/src/utils.ts       |  2 --
 packages/authentication-oauth/test/utils.test.ts |  1 -
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/packages/authentication-oauth/src/express.ts b/packages/authentication-oauth/src/express.ts
index 9e30404fb9..a445ad4d0b 100644
--- a/packages/authentication-oauth/src/express.ts
+++ b/packages/authentication-oauth/src/express.ts
@@ -16,15 +16,16 @@ const debug = Debug('@feathersjs/authentication-oauth/express');
 
 export default (options: OauthSetupSettings) => {
   return (feathersApp: Application) => {
-    const { path, authService, linkStrategy } = options;
+    const { authService, linkStrategy } = options;
     const app = feathersApp as ExpressApplication;
     const config = app.get('grant');
-
+    
     if (!config) {
       debug('No grant configuration found, skipping Express oAuth setup');
       return;
     }
-
+    
+    const { path } = config.defaults;
     const grantApp = grant(config);
     const authApp = express();
 
@@ -74,7 +75,6 @@ export default (options: OauthSetupSettings) => {
           grant.response : req.query;
 
         const params = {
-          provider: 'rest',
           authStrategies: [ name ],
           authentication: accessToken ? {
             strategy: linkStrategy,
diff --git a/packages/authentication-oauth/src/index.ts b/packages/authentication-oauth/src/index.ts
index 2770438655..188b7f0237 100644
--- a/packages/authentication-oauth/src/index.ts
+++ b/packages/authentication-oauth/src/index.ts
@@ -11,25 +11,26 @@ const debug = Debug('@feathersjs/authentication-oauth');
 export { OauthSetupSettings, OAuthStrategy, OAuthProfile };
 
 export const setup = (options: OauthSetupSettings) => (app: Application) => {
-  const path = options.authService;
-  const service: AuthenticationService = app.service(path);
+  const authPath = options.authService;
+  const service: AuthenticationService = app.service(authPath);
 
   if (!service) {
-    throw new Error(`'${path}' authentication service must exist before registering @feathersjs/authentication-oauth`);
+    throw new Error(`'${authPath}' authentication service must exist before registering @feathersjs/authentication-oauth`);
   }
 
   const { oauth } = service.configuration;
 
   if (!oauth) {
-    debug(`No oauth configuration found at '${path}'. Skipping oAuth setup.`);
+    debug(`No oauth configuration found at '${authPath}'. Skipping oAuth setup.`);
     return;
   }
 
   const { strategyNames } = service;
+  const { path = '/auth' } = oauth.defaults;
   const grant = merge({
     defaults: {
+      path,
       host: `${app.get('host')}:${app.get('port')}`,
-      path: '/auth',
       protocol: app.get('env') === 'production' ? 'https' : 'http',
       transport: 'session'
     }
@@ -37,7 +38,7 @@ export const setup = (options: OauthSetupSettings) => (app: Application) => {
 
   each(grant, (value, key) => {
     if (key !== 'defaults') {
-      value.callback = value.callback || `/auth/${key}/authenticate`;
+      value.callback = value.callback || `${path}/${key}/authenticate`;
 
       if (!strategyNames.includes(key)) {
         debug(`Registering oAuth default strategy for '${key}'`);
diff --git a/packages/authentication-oauth/src/utils.ts b/packages/authentication-oauth/src/utils.ts
index a5737abcee..7f21a7e5fe 100644
--- a/packages/authentication-oauth/src/utils.ts
+++ b/packages/authentication-oauth/src/utils.ts
@@ -3,7 +3,6 @@ import session from 'express-session';
 import { Application } from '@feathersjs/feathers';
 
 export interface OauthSetupSettings {
-  path: string;
   authService: string;
   linkStrategy: string;
   expressSession: RequestHandler;
@@ -11,7 +10,6 @@ export interface OauthSetupSettings {
 
 export const getDefaultSettings = (app: Application, other?: Partial<OauthSetupSettings>) => {
   const defaults: OauthSetupSettings = {
-    path: '/auth',
     authService: app.get('defaultAuthentication'),
     linkStrategy: 'jwt',
     expressSession: session({
diff --git a/packages/authentication-oauth/test/utils.test.ts b/packages/authentication-oauth/test/utils.test.ts
index 9f1753a43f..20fd70a5c1 100644
--- a/packages/authentication-oauth/test/utils.test.ts
+++ b/packages/authentication-oauth/test/utils.test.ts
@@ -6,7 +6,6 @@ describe('@feathersjs/authentication-oauth/utils', () => {
   it('getDefaultSettings', () => {
     const settings = getDefaultSettings(app);
 
-    assert.equal(settings.path, '/auth');
     assert.equal(settings.authService, 'authentication');
     assert.equal(settings.linkStrategy, 'jwt');
   });