Draft/passport strategy #2822

jannyHou · 2019-05-01T01:38:12Z

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 👈

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

bajtos · 2019-05-02T11:22:48Z

packages/authentication/src/__tests__/acceptance/basic-auth.acceptance.ts

+  // 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`


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

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.)

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.

raymondfeng · 2019-05-07T19:10:07Z

There are two possible options to adapt Passport strategies:

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

jannyHou · 2019-05-08T01:24:30Z

@raymondfeng Thanks for the suggestion

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.

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 · 2019-05-09T19:23:20Z

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.

emonddr · 2019-05-12T21:57:25Z

packages/authentication/src/__tests__/acceptance/basic-auth.acceptance.ts

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


@jannyHou , use configured instead of configurated.

emonddr · 2019-05-12T22:01:42Z

packages/authentication/src/__tests__/acceptance/basic-auth.acceptance.ts

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


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

emonddr · 2019-05-13T02:41:41Z

packages/authentication/src/__tests__/unit/strategy-adapter.unit.ts

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


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

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

@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 · 2019-05-16T14:12:55Z

@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 · 2019-05-16T15:11:50Z

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 · 2019-05-16T21:58:51Z

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

Recover the auth action tests, merged PR refactor: recover tests for auth action #2859.
[In progress] Spike [Spike] Explore how to roll out experimental features #2676 (comment)
Create the passport adapter in loopback-labs(or the core repo is we decide to have feature branches in the same repo), and address @emonddr 's comments in the PR.
Address Draft/passport strategy #2822 (comment) in the upcoming 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.

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 · 2019-05-17T20:07:28Z

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

feat: resolve authentication strategy registered via extension point

519e0d1

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 mentioned this pull request May 1, 2019

feat: refactor auth action #2746

Closed

7 tasks

jannyHou referenced this pull request May 1, 2019

feat: refactor auth action

53cfeeb

emonddr mentioned this pull request May 1, 2019

feat: resolve authentication strategy registered via extension point #2763

Merged

7 tasks

bajtos reviewed May 2, 2019

View reviewed changes

emonddr force-pushed the dremond_authentication_2312 branch 2 times, most recently from 7edb7ee to 146d9ca Compare May 3, 2019 18:57

feat: add extension point for passport

dbc51ce

jannyHou force-pushed the draft/passport-strategy branch from 881fe37 to dbc51ce Compare May 7, 2019 19:40

jannyHou mentioned this pull request May 9, 2019

refactor: recover tests for auth action #2859

Merged

emonddr reviewed May 12, 2019

View reviewed changes

emonddr reviewed May 13, 2019

View reviewed changes

This was referenced May 14, 2019

Authentication example is not working #2886

Closed

LB4 - Authorization Implementation - Cannot find name AuthResponse - Authorization sample #2896

Closed

jannyHou closed this May 17, 2019

emonddr mentioned this pull request Jun 3, 2019

loopback/authentication this.fail is not a function #3032

Closed

jannyHou deleted the draft/passport-strategy branch June 4, 2019 00:58

jannyHou mentioned this pull request Jun 7, 2019

[LABS] feat: adapter that wraps strategy #3056

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft/passport strategy #2822

Draft/passport strategy #2822

jannyHou commented May 1, 2019 •

edited

Loading

bajtos May 2, 2019 •

edited

Loading

jannyHou May 7, 2019

jannyHou May 7, 2019

raymondfeng commented May 7, 2019

jannyHou commented May 8, 2019 •

edited by emonddr

Loading

jannyHou commented May 9, 2019 •

edited

Loading

emonddr May 12, 2019

emonddr May 12, 2019

emonddr May 13, 2019

emonddr May 16, 2019

jannyHou May 16, 2019

emonddr commented May 16, 2019 •

edited

Loading

emonddr commented May 16, 2019

jannyHou commented May 16, 2019 •

edited

Loading

jannyHou commented May 17, 2019

Draft/passport strategy #2822

Draft/passport strategy #2822

Conversation

jannyHou commented May 1, 2019 • edited Loading

Checklist

bajtos May 2, 2019 • edited Loading

Choose a reason for hiding this comment

jannyHou May 7, 2019

Choose a reason for hiding this comment

jannyHou May 7, 2019

Choose a reason for hiding this comment

raymondfeng commented May 7, 2019

jannyHou commented May 8, 2019 • edited by emonddr Loading

jannyHou commented May 9, 2019 • edited Loading

emonddr May 12, 2019

Choose a reason for hiding this comment

emonddr May 12, 2019

Choose a reason for hiding this comment

emonddr May 13, 2019

Choose a reason for hiding this comment

emonddr May 16, 2019

Choose a reason for hiding this comment

jannyHou May 16, 2019

Choose a reason for hiding this comment

emonddr commented May 16, 2019 • edited Loading

emonddr commented May 16, 2019

jannyHou commented May 16, 2019 • edited Loading

jannyHou commented May 17, 2019

jannyHou commented May 1, 2019 •

edited

Loading

bajtos May 2, 2019 •

edited

Loading

jannyHou commented May 8, 2019 •

edited by emonddr

Loading

jannyHou commented May 9, 2019 •

edited

Loading

emonddr commented May 16, 2019 •

edited

Loading

jannyHou commented May 16, 2019 •

edited

Loading