Skip to content

Commit

Permalink
feat: refactor auth action
Browse files Browse the repository at this point in the history
  • Loading branch information
jannyHou committed Apr 17, 2019
1 parent 05bf4ac commit 53cfeeb
Show file tree
Hide file tree
Showing 11 changed files with 204 additions and 82 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,35 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {inject, Provider, ValueOrPromise} from '@loopback/context';
import {Application} from '@loopback/core';
import {anOpenApiSpec} from '@loopback/openapi-spec-builder';
import {api, get} from '@loopback/openapi-v3';
import {
RestBindings,
ParseParams,
FindRoute,
InvokeMethod,
Send,
ParseParams,
Reject,
SequenceHandler,
RestServer,
RestComponent,
RequestContext,
RestBindings,
RestComponent,
RestServer,
Send,
SequenceHandler,
} from '@loopback/rest';
import {api, get} from '@loopback/openapi-v3';
import {Client, createClientForHandler} from '@loopback/testlab';
import {anOpenApiSpec} from '@loopback/openapi-spec-builder';
import {inject, Provider, ValueOrPromise} from '@loopback/context';
import {BasicStrategy} from 'passport-http';
import {
authenticate,
UserProfile,
AuthenticationBindings,
AuthenticateFn,
AuthenticationMetadata,
AuthenticationBindings,
AuthenticationComponent,
AuthenticationMetadata,
AuthenticationStrategy,
StrategyAdapter,
UserProfile,
} from '../..';
import {Strategy} from 'passport';
import {BasicStrategy} from 'passport-http';
import {MockStrategy} from '../unit/fixtures/mock-strategy';

const SequenceActions = RestBindings.SequenceActions;

Expand Down Expand Up @@ -115,7 +117,7 @@ describe('Basic Authentication', () => {
@inject(AuthenticationBindings.CURRENT_USER) private user: UserProfile,
) {}

@authenticate('BasicStrategy')
@authenticate('BasicStrategy', {isPassportStrategy: true})

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 30, 2019

Contributor

I don't think isPassportStrategy flag should be exposed to application developers. It should be part of implementation details. We can register two different strategies, one implemented in LB4, another one in Passport.

IMO, here is the extension point structure:

  • AuthStrategyExtension Point
    • StrategyExtension1
    • StrategyExtension2
    • PassportStrategyAdapter (as an extension point to all passport strategies)
      • PassportStrategy1
      • PassportStrategy2

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 30, 2019

Author Contributor

@raymondfeng Thanks we realized that. See my discussion with Miroslav in 53cfeeb#r33347455

I am going to either update this PR or open a new draft PR to change the design.

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 1, 2019

Author Contributor

@raymondfeng @bajtos @emonddr Thanks for all the feedback, opened another draft to implement the new design: #2822

async whoAmI(): Promise<string> {
return this.user.id;
}
Expand Down Expand Up @@ -159,31 +161,67 @@ describe('Basic Authentication', () => {
}

function givenProviders() {
class MyPassportStrategyProvider implements Provider<Strategy | undefined> {
class MyPassportStrategyProvider
implements Provider<AuthenticationStrategy | undefined> {
constructor(
@inject(AuthenticationBindings.METADATA)
private metadata: AuthenticationMetadata,
) {}
value(): ValueOrPromise<Strategy | undefined> {
value(): ValueOrPromise<AuthenticationStrategy | undefined> {
if (!this.metadata) {
return undefined;
}
const isPassportStrategy =
this.metadata.options && this.metadata.options.isPassportStrategy;
if (!isPassportStrategy) {
return undefined;
}

const name = this.metadata.strategy;
if (name === 'BasicStrategy') {
return new BasicStrategy(this.verify);
const basicStrategy = new BasicStrategy(verify);
return new StrategyAdapter(basicStrategy, 'basic');
} else {
return Promise.reject(`The strategy ${name} is not available.`);
}
}
// callback method for BasicStrategy
verify(username: string, password: string, cb: Function) {
process.nextTick(() => {
users.find(username, password, cb);
});
}
class MyStrategyProvider
implements Provider<AuthenticationStrategy | undefined> {
constructor(
@inject(AuthenticationBindings.METADATA)
private metadata: AuthenticationMetadata,
@inject(AuthenticationBindings.PASSPORT_STRATEGY)
private passportStrategy?: AuthenticationStrategy,
) {}
value(): ValueOrPromise<AuthenticationStrategy | undefined> {
if (!this.metadata) {
return undefined;
}
const isPassportStrategy =
this.metadata.options && this.metadata.options.isPassportStrategy;
if (isPassportStrategy) {
return this.passportStrategy;
}

const name = this.metadata.strategy;
if (name === 'BasicStrategy') {
return new MockStrategy();
} else {
return Promise.reject(`The strategy ${name} is not available.`);
}
}
}

// callback method for BasicStrategy
function verify(username: string, password: string, cb: Function) {
process.nextTick(() => {
users.find(username, password, cb);
});
}
server.bind(AuthenticationBindings.STRATEGY).toProvider(MyStrategyProvider);
server
.bind(AuthenticationBindings.STRATEGY)
.bind(AuthenticationBindings.PASSPORT_STRATEGY)
.toProvider(MyPassportStrategyProvider);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright IBM Corp. 2019. All Rights Reserved.
// Node module: @loopback/authentication
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Request} from 'express';
import {AuthenticateOptions, Strategy} from 'passport';
import {UserProfile} from '../../../types';

/**
* Test fixture for a mock asynchronous passport-strategy
*/
export class MockPassportStrategy extends Strategy {
// user to return for successful authentication
private mockUser: UserProfile;

setMockUser(userObj: UserProfile) {
this.mockUser = userObj;
}

/**
* authenticate() function similar to passport-strategy packages
* @param req
*/
async authenticate(req: Request, options?: AuthenticateOptions) {
await this.verify(req);
}
/**
* @param req
* mock verification function; usually passed in as constructor argument for
* passport-strategy
*
* For the purpose of mock tests we have this here
* pass req.query.testState = 'fail' to mock failed authorization
* pass req.query.testState = 'error' to mock unexpected error
*/
async verify(request: Request) {
if (
request.headers &&
request.headers.testState &&
request.headers.testState === 'fail'
) {
this.returnUnauthorized('authorization failed');
return;
} else if (
request.headers &&
request.headers.testState &&
request.headers.testState === 'error'
) {
this.returnError('unexpected error');
return;
}
process.nextTick(this.returnMockUser.bind(this));
}

returnMockUser() {
this.success(this.mockUser);
}

returnUnauthorized(challenge?: string | number, status?: number) {
this.fail(challenge, status);
}

returnError(err: string) {
this.error(err);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,35 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Strategy, AuthenticateOptions} from 'passport';
import {Request} from 'express';
import {Request} from '@loopback/rest';
import {AuthenticationStrategy, UserProfile} from '../../../types';

class AuthenticationError extends Error {
statusCode?: number;
}

/**
* Test fixture for a mock asynchronous passport-strategy
*/
export class MockStrategy extends Strategy {
export class MockStrategy implements AuthenticationStrategy {
name: 'MockStrategy';
// user to return for successful authentication
private mockUser: Object;
private mockUser: UserProfile;

setMockUser(userObj: Object) {
setMockUser(userObj: UserProfile) {
this.mockUser = userObj;
}

returnMockUser(): UserProfile {
return this.mockUser;
}

/**
* authenticate() function similar to passport-strategy packages
* @param req
*/
async authenticate(req: Request, options?: AuthenticateOptions) {
await this.verify(req);
async authenticate(req: Request): Promise<UserProfile> {
return await this.verify(req);
}
/**
* @param req
Expand All @@ -39,28 +48,16 @@ export class MockStrategy extends Strategy {
request.headers.testState &&
request.headers.testState === 'fail'
) {
this.returnUnauthorized('authorization failed');
return;
const err = new AuthenticationError('authorization failed');
err.statusCode = 401;
throw err;
} else if (
request.headers &&
request.headers.testState &&
request.headers.testState === 'error'
) {
this.returnError('unexpected error');
return;
throw new Error('unexpected error');
}
process.nextTick(this.returnMockUser.bind(this));
}

returnMockUser() {
this.success(this.mockUser);
}

returnUnauthorized(challenge?: string | number, status?: number) {
this.fail(challenge, status);
}

returnError(err: string) {
this.error(err);
return this.returnMockUser();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {expect} from '@loopback/testlab';
import {CoreBindings} from '@loopback/core';
import {Context, Provider} from '@loopback/context';
import {AuthenticationMetadata, authenticate} from '../../..';
import {CoreBindings} from '@loopback/core';
import {expect} from '@loopback/testlab';
import {authenticate, AuthenticationMetadata} from '../../..';
import {AuthMetadataProvider} from '../../../providers';

describe('AuthMetadataProvider', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// TBD
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
import {Context, instantiateClass} from '@loopback/context';
import {Request} from '@loopback/rest';
import {expect} from '@loopback/testlab';
import {Strategy} from 'passport';
import {AuthenticateFn, AuthenticationBindings, UserProfile} from '../../..';
import {AuthenticateActionProvider} from '../../../providers';
import {AuthenticationStrategy} from '../../../types';
import {MockStrategy} from '../fixtures/mock-strategy';

describe('AuthenticateActionProvider', () => {
Expand Down Expand Up @@ -67,7 +67,9 @@ describe('AuthenticateActionProvider', () => {

it('throws an error if the injected passport strategy is not valid', async () => {
const context: Context = new Context();
context.bind(AuthenticationBindings.STRATEGY).to({} as Strategy);
context
.bind(AuthenticationBindings.STRATEGY)
.to({} as AuthenticationStrategy);
context
.bind(AuthenticationBindings.AUTH_ACTION)
.toProvider(AuthenticateActionProvider);
Expand Down
28 changes: 16 additions & 12 deletions packages/authentication/src/__tests__/unit/strategy-adapter.unit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {HttpErrors, Request} from '@loopback/rest';
import {expect} from '@loopback/testlab';
import {StrategyAdapter, UserProfile} from '../..';
import {Request, HttpErrors} from '@loopback/rest';
import {MockStrategy} from './fixtures/mock-strategy';
import {AuthenticateOptions} from 'passport';
import {StrategyAdapter, UserProfile} from '../..';
import {MockPassportStrategy} from './fixtures/mock-strategy-passport';

describe('Strategy Adapter', () => {
const mockUser: UserProfile = {name: 'user-name', id: 'mock-id'};
Expand All @@ -16,33 +16,37 @@ describe('Strategy Adapter', () => {
it('calls the authenticate method of the strategy', async () => {
let calledFlag = false;
// TODO: (as suggested by @bajtos) use sinon spy
class Strategy extends MockStrategy {
class Strategy extends MockPassportStrategy {
// override authenticate method to set calledFlag
async authenticate(req: Request, options?: AuthenticateOptions) {
calledFlag = true;
await MockStrategy.prototype.authenticate.call(this, req, options);
await MockPassportStrategy.prototype.authenticate.call(
this,
req,
options,
);
}
}
const strategy = new Strategy();
const adapter = new StrategyAdapter(strategy);
const adapter = new StrategyAdapter(strategy, 'mock-strategy');
const request = <Request>{};
await adapter.authenticate(request);
expect(calledFlag).to.be.true();
});

it('returns a promise which resolves to an object', async () => {
const strategy = new MockStrategy();
const strategy = new MockPassportStrategy();
strategy.setMockUser(mockUser);
const adapter = new StrategyAdapter(strategy);
const adapter = new StrategyAdapter(strategy, 'mock-strategy');
const request = <Request>{};
const user: Object = await adapter.authenticate(request);
expect(user).to.be.eql(mockUser);
});

it('throws Unauthorized error when authentication fails', async () => {
const strategy = new MockStrategy();
const strategy = new MockPassportStrategy();
strategy.setMockUser(mockUser);
const adapter = new StrategyAdapter(strategy);
const adapter = new StrategyAdapter(strategy, 'mock-strategy');
const request = <Request>{};
request.headers = {testState: 'fail'};
let error;
Expand All @@ -55,9 +59,9 @@ describe('Strategy Adapter', () => {
});

it('throws InternalServerError when strategy returns error', async () => {
const strategy = new MockStrategy();
const strategy = new MockPassportStrategy();
strategy.setMockUser(mockUser);
const adapter = new StrategyAdapter(strategy);
const adapter = new StrategyAdapter(strategy, 'mock-strategy');
const request = <Request>{};
request.headers = {testState: 'error'};
let error;
Expand Down
Loading

0 comments on commit 53cfeeb

Please sign in to comment.