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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft/passport strategy #2822

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@jannyHou
Copy link
Contributor

commented May 1, 2019

The 2nd commit is the real change

connect to #2467
and also #2311

A draft PR based on #2763.
Discussion see 53cfeeb#r33364760 and 53cfeeb#r33347455

This PR adds another extension point for passport based strategies:

  • Passport strategy provider as an extension point: link

  • In the strategy provider, it searches for a passport strategy if no result in the non-passport ones: link

  • The passport strategy is registered as a provider, reason and details please check the acceptance test

  • One skipped test to be recovered (need sometime to check the error message, not a big deal), more tests to be added for the new extension point.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

feat: resolve authentication strategy registered via extension point
Resolve authentication strategies registered via extension point

BREAKING CHANGE: the new interface and authentication action in 2.0 will require users to adjust existing code

@jannyHou jannyHou referenced this pull request May 1, 2019

Closed

feat: refactor auth action #2746

3 of 7 tasks complete

jannyHou referenced this pull request May 1, 2019

// instantiate the passport strategy, we cannot add the imported `BasicStrategy`
// class as extension directly, we need to wrap it as a strategy provider,
// then add the provider class as the extension.
// See Line 89 in the function `givenAServer`

This comment has been minimized.

Copy link
@bajtos

bajtos May 2, 2019

Member

Since we are not able to register a Passport strategy class directly and have to create a custom provider class, I would like to propose a slightly different user experience that does not require any new extension points to be created.

Usage:

import {Strategy, PassportStrategyAdapter} from '@loopback/authentication';

class PassportBasicAuthProvider implements Provider<Strategy> {
    value(): Strategy {
      // create an instance of the Passport Strategy
      // configure it with "verify" and any other options/behavior as needed
      const passportStrategy = new BasicStrategy(this.verify.bind(this));

      // wrap the Passport Strategy class instance and 
      // adapt it to match LB Strategy interface contract
      return new PassportStrategyAdapter(passportStrategy);
    }

  verify(username: string, password: string, cb: Function) {
    process.nextTick(() => {
      users.find(username, password, cb);
    });
  }
}

// register PassportBasicAuthProvider as an extension
// for AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME
// (a regular LB4 Authentication strategy)

Implementation wise, I think we just need to export existing StrategyAdapter class for public consumption, preferably under a more descriptive name. In my example, I am using the name PassportStrategyAdapter. In the future, we can implement additional adapters, e.g. Auth0StrategyAdapter (see https://auth0.com, I am totally making this example up.)

An important feature of this approach is extensibility. When each strategy contract (LB4, Passport, etc.) requires a new extension point, then it's difficult to implement support for new strategy contracts in 3rd-party modules - each new contract requires changes in authentication core to recognize the new extension point. With the proposed Adapter approach, the core authentication framework does not need to deal with different strategy contracts and thus adding a new strategy contract does not require any changes in the core.

In fact, I think we should move PassportStrategyAdapter into a standalone package:

  • It will validate extensibility of our design and enforce proper separation of concerns.
  • People using only LB strategies should not have to install passport dependency. By moving the passport adapter to a new package, only consumers of this new package will depend on passport.
  • As we learn more about Passport, we may need to introduce breaking change in the passport adapter. Ideally, these breaking changes should affect only Passport users, they should not trigger a semver-major release of the entire authentication framework.

/cc @raymondfeng

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 7, 2019

Author Contributor

Since we are not able to register a Passport strategy class directly and have to create a custom provider class, I would like to propose a slightly different user experience that does not require any new extension points to be created.

Neat 馃憤 yeah since the adapter implements AuthenticationStrategy it makes sense to only have one extension. And this could also be a good practice for other adapters like you said in

we can implement additional adapters, e.g. Auth0StrategyAdapter (see https://auth0.com, I am totally making this example up.)

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 7, 2019

Author Contributor

With the proposed Adapter approach, the core authentication framework does not need to deal with different strategy contracts and thus adding a new strategy contract does not require any changes in the core.

+1 Fair enough.

@emonddr emonddr force-pushed the dremond_authentication_2312 branch 2 times, most recently from 7edb7ee to 146d9ca May 2, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member

commented May 7, 2019

There are two possible options to adapt Passport strategies:

  1. Adapt a passport strategy into our AuthenticationStrategy and register it as a regular extension.
  2. Adapt passport middleware and use existing passport apis to register passport strategies.

@jannyHou jannyHou force-pushed the draft/passport-strategy branch from 881fe37 to dbc51ce May 7, 2019

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@raymondfeng Thanks for the suggestion

  1. Adapt a passport strategy into our AuthenticationStrategy and register it as a regular extension.

I believe this is the approach that this PR is implementing.

  1. Adapt passport middleware and use existing passport apis to register passport strategies.

馃憤 I am also interested to explorer it, while it looks more like documenting the best practice of applying an express middleware passport, and has nothing to do with our authentication metadata/strategy interface/extension point. And I believe it won't affect the existing authentication action. I can try it in #2311, but keep it out of the scope of #2467, are you good with the plan?
@raymondfeng ^^

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Tried wrapping passport on local, some findings:

Here is a prototype of the passport adapter provider:

  • It injects a Passport instance which is created and bind to app

    const passport = new Passport();
    registePassportStrategies(passport);
    
    // Register the configured passport strategies
    function registePassportStrategies(passport: Passport) {
      passport.use(new BasicStrategy(verify_basic));
      passport.use(new JWTStrategy(verify_jwt));
    }
    
    app.bind(AuthenticationBindings.PASSPORT).to(passport);
  • (?) Invoke the passport.authenticate('strategyName') to authenticate the request. The main blocker is I couldn't imagine how to invoke the passport.authenticate method in the action, with the request from context.

export class PassportAdapterProvider implements Provider<AuthenticateFn> {
  constructor(
    // `Passport` is a function and cannot be used as a type here
    // need to build a type to describe the instance of Passport
    @inject(AuthenticationBindings.PASSPORT)
    readonly passport: Passport,
    @inject(AuthenticationBindings.METADATA)
    private metadata?: AuthenticationMetadata,
  ) { }

  /**
   * @returns authenticateFn
   */
  value(): AuthenticateFn {
    return request => this.action(request);
  }

  /**
   * The implementation of authenticate() sequence action.
   * @param request The incoming request provided by the REST layer
   */
  async action(request: Request): Promise<UserProfile | undefined> {
    // we can infer the strategy name from `metadata.name` as an alternate.
    const strategyName = metadata.options.strategyName;

    const asyncAuthenticate = promisify(passport.authenticate);
    const user = await asyncAuthenticate(strategyName);
    // (Blocker) How to authenticate a request using `passport.authenticate` in the action?

    // We need a function to convert the `user` to `userProfile`
    const userProfile = user;
    return userProfile;
  }
}

Since the existing adapter wraps the passport strategy instead of passport, story #2467 and #2311 are created based on that assumption.

I am afraid I need more time to flush out the uncertainties of wrapping passport, how about I recover the tests:

in story #2467.

Then explorer wrapping passport in story #2311? We have more time to decide which one is better or if we want to keep both.

// See Line 89 in the function `givenAServer`
class PassportBasicAuthProvider implements Provider<AuthenticationStrategy> {
value(): AuthenticationStrategy {
const basicStrategy = this.configuratedBasicStrategy(verify);

This comment has been minimized.

Copy link
@emonddr

emonddr May 12, 2019

Contributor

@jannyHou , use configured instead of configurated.

}

convertToAuthStrategy(basic: BasicStrategy): AuthenticationStrategy {
return new StrategyAdapter(basic, 'basic');

This comment has been minimized.

Copy link
@emonddr

emonddr May 12, 2019

Contributor

@jannyHou , let's store the 'basic' identifier string for the authentication strategy in a constant somewhere else.

@@ -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 {

This comment has been minimized.

Copy link
@emonddr

emonddr May 13, 2019

Contributor

@jannyHou , Please use a custom strategy class other than Strategy so it is not confused by the Strategy class used by passport-strategy. thx.

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Contributor

@jannyHou ^^^ Please see my comments. thx.

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 16, 2019

Author Contributor

@emonddr Thanks good catch. This code change is merged in PR #2859. I can address it in the coming doc PR that moves https://github.com/strongloop/loopback-next/blob/master/packages/authentication/docs/authentication-system.md to the root level README.md file.

@emonddr

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@jannyHou , it is possible for you to rebase off of loopback-next master? my changes from my feature branch (from which you based your feature branch) are in master branch now.

For basic-auth-acceptance.ts, please use this sequence : https://github.com/strongloop/loopback-next/tree/master/packages/authentication/src/__tests__/fixtures/sequences . Also please use : https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/__tests__/fixtures/users/user.repository.ts and https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/__tests__/fixtures/users/user.ts.
This is what basic-auth-extension.acceptance.ts and jwt-auth-extension.acceptance.ts are using.

@emonddr

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

In basic-auth-acceptance.ts,
we can turn off the .skip for this mocha test

      it('throws an error if the injected passport strategy is not valid', async () => {
        const context: Context = new Context();
        context
          .bind(AuthenticationBindings.STRATEGY)
          .to({} as AuthenticationStrategy);
        context
          .bind(AuthenticationBindings.AUTH_ACTION)
          .toProvider(AuthenticateActionProvider);
        const authenticate = await context.get<AuthenticateFn>(
          AuthenticationBindings.AUTH_ACTION,
        );
        const request = <Request>{};
        let error;
        try {
          await authenticate(request);
        } catch (exception) {
          error = exception;
        }
        expect(error).to.have.property('message', 'strategy.authenticate is not a function');
      });

and use the error message listed.
This catches the missing authenticate function error
in line: https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/providers/auth-action.provider.ts#L52 . This is because you used {} as AuthenticationStrategy.

To catch https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/providers/auth-strategy.provider.ts#L45, perhaps add a test like: https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/__tests__/acceptance/basic-auth-extension.acceptance.ts#L145 and https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/__tests__/acceptance/jwt-auth-extension.acceptance.ts#L390.

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@emonddr Thank you for the comments. This draft PR is a PoC of the new strategy adapter, so some implementation details are not refined yet and will be rewrote/improved in the official feature PR.

The plan of moving forward this PR is:

I removed the test file basic-auth-acceptance.ts since it should be part of the new passport adapter module. Thank you for the very detailed comment in #2822 (comment) and #2822 (comment), I will address them in the passport adapter PR.

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

I am closing the draft to keep open PRs less.
The implementation plan is #2822 (comment)

@jannyHou jannyHou closed this May 17, 2019

@jannyHou jannyHou deleted the draft/passport-strategy branch Jun 4, 2019

@jannyHou jannyHou referenced this pull request Jun 7, 2019

Merged

[LABS] feat: adapter that wraps strategy #3056

3 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.