fix: Improve oAuth option handling and usability (#1335)

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,30 +9,27 @@ import {
   original as express
 } from '@feathersjs/express';
 import { OauthSetupSettings } from './utils';
+import { OAuthStrategy } from '.';
 
 const grant = grantExpress();
 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');
-    const secret = Math.random().toString(36).substring(7);
-
+
     if (!config) {
       debug('No grant configuration found, skipping Express oAuth setup');
       return;
     }
-
+
+    const { path } = config.defaults;
     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 +52,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);
         }
       };
 
@@ -73,7 +75,6 @@ export default (options: OauthSetupSettings) => {
           grant.response : req.query;
 
         const params = {
-          provider: 'rest',
           authStrategies: [ name ],
           authentication: accessToken ? {
             strategy: linkStrategy,
@@ -95,7 +96,7 @@ export default (options: OauthSetupSettings) => {
         await sendResponse(authResult);
       } catch (error) {
         debug('Received oAuth authentication error', error);
-        sendResponse(error);
+        await sendResponse(error);
       }
     });
 

packages/authentication-oauth/src/index.ts

@@ -11,33 +11,34 @@ 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'
     }
   }, omit(oauth, 'redirect'));
 
   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}'`);
@@ -49,7 +50,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));

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.sub || 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 {

packages/authentication-oauth/src/utils.ts

@@ -1,38 +1,22 @@
-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>;
+  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
   };
 

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,

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

@@ -4,14 +4,37 @@ 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);
     assert.strictEqual(strategy.entityId, 'id');
     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, {});

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

@@ -1,38 +1,11 @@
 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);
 
-    assert.equal(settings.path, '/auth');
     assert.equal(settings.authService, 'authentication');
     assert.equal(settings.linkStrategy, 'jwt');
   });