Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add method to reliably get default authentication service #1470

Merged
merged 3 commits into from
Jul 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/authentication-local/src/hooks/hash-password.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { get, set, cloneDeep } from 'lodash';
import { BadRequest } from '@feathersjs/errors';
import Debug from 'debug';
import { HookContext } from '@feathersjs/feathers';
import { LocalStrategy } from '../strategy';

const debug = Debug('@feathersjs/authentication-local/hooks/hash-password');

Expand All @@ -28,15 +29,14 @@ export default function hashPassword (field: string, options: HashPasswordOption
return context;
}

const serviceName = options.authentication || app.get('defaultAuthentication');
const authService = app.service(serviceName);
const authService = app.defaultAuthentication(options.authentication);
const { strategy = 'local' } = options;

if (!authService || typeof authService.getStrategies !== 'function') {
throw new BadRequest(`Could not find '${serviceName}' service to hash password`);
throw new BadRequest(`Could not find an authentication service to hash password`);
}

const [ localStrategy ] = authService.getStrategies(strategy);
const [ localStrategy ] = authService.getStrategies(strategy) as LocalStrategy[];

if (!localStrategy || typeof localStrategy.hashPassword !== 'function') {
throw new BadRequest(`Could not find '${strategy}' strategy to hash password`);
Expand Down
2 changes: 1 addition & 1 deletion packages/authentication-local/src/strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class LocalStrategy extends AuthenticationBaseStrategy {
throw new NotAuthenticated(errorMessage);
}

async hashPassword (password: string) {
async hashPassword (password: string, _params: Params) {
return bcrypt.hash(password, this.configuration.hashSize);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('@feathersjs/authentication-local/hooks/hash-password', () => {
assert.fail('Should never get here');
} catch (error) {
assert.strictEqual(error.message,
`Could not find 'authentication' service to hash password`
'Could not find an authentication service to hash password'
);
}
});
Expand Down
4 changes: 2 additions & 2 deletions packages/authentication-oauth/src/express.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { express as grantExpress } from 'grant';
import Debug from 'debug';
import { Application } from '@feathersjs/feathers';
import { AuthenticationService, AuthenticationResult } from '@feathersjs/authentication';
import { AuthenticationResult } from '@feathersjs/authentication';
import qs from 'querystring';
import {
Application as ExpressApplication,
Expand Down Expand Up @@ -49,7 +49,7 @@ export default (options: OauthSetupSettings) => {
authApp.get('/:name/authenticate', async (req, res, next) => {
const { name } = req.params;
const { accessToken, grant } = req.session;
const service: AuthenticationService = app.service(authService);
const service = app.defaultAuthentication(authService);
const [ strategy ] = service.getStrategies(name) as OAuthStrategy[];
const sendResponse = async (data: AuthenticationResult|Error) => {
try {
Expand Down
8 changes: 3 additions & 5 deletions packages/authentication-oauth/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import Debug from 'debug';
import { merge, each, omit } from 'lodash';
import { Application } from '@feathersjs/feathers';
import { AuthenticationService } from '@feathersjs/authentication';
import { OAuthStrategy, OAuthProfile } from './strategy';
import { default as setupExpress } from './express';
import { OauthSetupSettings, getDefaultSettings } from './utils';
Expand All @@ -11,17 +10,16 @@ const debug = Debug('@feathersjs/authentication-oauth');
export { OauthSetupSettings, OAuthStrategy, OAuthProfile };

export const setup = (options: OauthSetupSettings) => (app: Application) => {
const authPath = options.authService;
const service: AuthenticationService = app.service(authPath);
const service = app.defaultAuthentication ? app.defaultAuthentication(options.authService) : null;

if (!service) {
throw new Error(`'${authPath}' authentication service must exist before registering @feathersjs/authentication-oauth`);
throw new Error('An authentication service must exist before registering @feathersjs/authentication-oauth');
}

const { oauth } = service.configuration;

if (!oauth) {
debug(`No oauth configuration found at '${authPath}'. Skipping oAuth setup.`);
debug(`No oauth configuration found in authentication configuration. Skipping oAuth setup.`);
return;
}

Expand Down
5 changes: 2 additions & 3 deletions packages/authentication-oauth/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@ import session from 'express-session';
import { Application } from '@feathersjs/feathers';

export interface OauthSetupSettings {
authService: string;
authService?: string;
linkStrategy: string;
expressSession: RequestHandler;
}

export const getDefaultSettings = (app: Application, other?: Partial<OauthSetupSettings>) => {
export const getDefaultSettings = (_app: Application, other?: Partial<OauthSetupSettings>) => {
const defaults: OauthSetupSettings = {
authService: app.get('defaultAuthentication'),
linkStrategy: 'jwt',
expressSession: session({
secret: Math.random().toString(36).substring(7),
Expand Down
2 changes: 1 addition & 1 deletion packages/authentication-oauth/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('@feathersjs/authentication-oauth', () => {
assert.fail('Should never get here');
} catch (error) {
assert.equal(error.message,
`'something' authentication service must exist before registering @feathersjs/authentication-oauth`
'An authentication service must exist before registering @feathersjs/authentication-oauth'
);
}
});
Expand Down
2 changes: 1 addition & 1 deletion packages/authentication-oauth/test/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ describe('@feathersjs/authentication-oauth/utils', () => {
it('getDefaultSettings', () => {
const settings = getDefaultSettings(app);

assert.equal(settings.authService, 'authentication');
assert.equal(settings.authService, undefined);
assert.equal(settings.linkStrategy, 'jwt');
});
});
10 changes: 3 additions & 7 deletions packages/authentication/src/hooks/authenticate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { flatten, omit, merge } from 'lodash';
import { HookContext } from '@feathersjs/feathers';
import { NotAuthenticated } from '@feathersjs/errors';
import Debug from 'debug';
import { AuthenticationService } from '../service';

const debug = Debug('@feathersjs/authentication/hooks/authenticate');

Expand All @@ -22,12 +21,9 @@ export default (originalSettings: string|AuthenticateHookSettings, ...originalSt

return async (context: HookContext) => {
const { app, params, type, path, service } = context;
const {
service: authPath = app.get('defaultAuthentication'),
strategies
} = settings;
const { strategies } = settings;
const { provider, authentication } = params;
const authService = app.service(authPath) as unknown as AuthenticationService;
const authService = app.defaultAuthentication(settings.service);

debug(`Running authenticate hook on '${path}'`);

Expand All @@ -36,7 +32,7 @@ export default (originalSettings: string|AuthenticateHookSettings, ...originalSt
}

if (!authService || typeof authService.authenticate !== 'function') {
throw new NotAuthenticated(`Could not find authentication service at '${authPath}'`);
throw new NotAuthenticated('Could not find a valid authentication service');
}

// @ts-ignore
Expand Down
29 changes: 28 additions & 1 deletion packages/authentication/src/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,38 @@ import { merge, get } from 'lodash';
import { NotAuthenticated } from '@feathersjs/errors';
import { AuthenticationBase, AuthenticationResult, AuthenticationRequest } from './core';
import { connection, events } from './hooks';
import { Params, ServiceMethods } from '@feathersjs/feathers';
import { Application, Params, ServiceMethods } from '@feathersjs/feathers';

const debug = Debug('@feathersjs/authentication/service');

declare module '@feathersjs/feathers' {
interface Application<ServiceTypes = {}> {

/**
* Returns the default authentication service or the
* authentication service for a given path.
*
* @param location The service path to use (optional)
*/
defaultAuthentication (location?: string): AuthenticationService;
}
}

export class AuthenticationService extends AuthenticationBase implements Partial<ServiceMethods<AuthenticationResult>> {
constructor (app: Application, configKey: string = 'authentication', options = {}) {
super(app, configKey, options);

if (typeof app.defaultAuthentication !== 'function') {
app.defaultAuthentication = function (location?: string) {
const configKey = app.get('defaultAuthentication');
const path = location || Object.keys(this.services).find(current =>
this.service(current).configKey === configKey
);

return path ? this.service(path) : null;
};
}
}
/**
* Return the payload for a JWT based on the authentication result.
* Called internally by the `create` method.
Expand Down
2 changes: 1 addition & 1 deletion packages/authentication/test/hooks/authenticate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ describe('authentication/hooks/authenticate', () => {
assert.fail('Should never get here');
} catch (error) {
assert.strictEqual(error.name, 'NotAuthenticated');
assert.strictEqual(error.message, `Could not find authentication service at 'authentication'`);
assert.strictEqual(error.message, `Could not find a valid authentication service`);
}
});

Expand Down
8 changes: 6 additions & 2 deletions packages/authentication/test/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import feathers, { Application, Service } from '@feathersjs/feathers';
import memory from 'feathers-memory';

import defaultOptions from '../src/options';
import { AuthenticationService } from '../src/service';
import { AuthenticationResult } from '../src/core';
import { AuthenticationService, AuthenticationResult } from '../src';

import { Strategy1 } from './fixtures';

Expand Down Expand Up @@ -38,6 +37,11 @@ describe('authentication/service', () => {
assert.deepStrictEqual(app.service('authentication').configuration, Object.assign({}, defaultOptions, app.get('authentication')));
});

it('app.defaultAuthentication()', () => {
assert.strictEqual(app.defaultAuthentication(), app.service('authentication'));
assert.strictEqual(app.defaultAuthentication('dummy'), undefined);
});

describe('create', () => {
it('creates a valid accessToken and includes strategy result', async () => {
const service = app.service('authentication');
Expand Down
13 changes: 2 additions & 11 deletions packages/express/lib/authentication.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,11 @@ const normalizeStrategy = (_settings = [], ..._strategies) =>
typeof _settings === 'string'
? { strategies: flatten([ _settings, ..._strategies ]) }
: _settings;
const getService = (settings, app) => {
const path = settings.service || app.get('defaultAuthentication');

if (typeof path !== 'string') {
return null;
}

return app.service(path) || null;
};

exports.parseAuthentication = (settings = {}) => {
return function (req, res, next) {
const { app } = req;
const service = getService(settings, app);
const service = app.defaultAuthentication ? app.defaultAuthentication(settings.service) : null;

if (service === null) {
return next();
Expand Down Expand Up @@ -53,7 +44,7 @@ exports.authenticate = (...strategies) => {

return function (req, res, next) {
const { app, authentication } = req;
const service = getService(settings, app);
const service = app.defaultAuthentication(settings.service);

debug('Authenticating with Express middleware and strategies', settings.strategies);

Expand Down