Support for alternative Passport Strategies

[DOSarrest]

Description

This is a feature request accompanied with code and explanation. I was able to obtain this functionality rather easily with minimal code, but implementation could be elaborated further perhaps.

First by adding the following case statement to the switch statement located at lib/passport-configurator.js#L296

case 'custom':
      let verifyMethod = options.verifyMethod;
      delete options.verifyMethod;
      strategy = new AuthStrategy(options, verifyMethod.bind(self));
      break;

This allows the configuration options passed to the AuthStrategy to contain a function that is executed to verify the passport authentication process. This should allow anyone to implement any of the available passport strategies as well as add their own strategies how ever they want.

Another addition that would allow greater flexibility would be the ability to add support for additional models to be setup. The following code added to the end of the setupModels function located at lib/passport-configurator.js#L40 would allow for the attaching of any additional models to the PassportConfigurator to be used within the verifyMethod mentioned above.

_.each(_.keys(options), (option) => {
    switch (option) {
      case 'userModel':
      case 'userCredentialModel':
      case 'userIdentityModel':
        break;
      default:
        let sOption = option.split('Model');
        if (sOption.length === 2 && sOption[1] === '') {
          let modelType = sOption[0].charAt(0).toUpperCase() + sOption[0].substring(1).toLowerCase();
          this[option] = options[option] || loopback.getModelByType(this.app.models[modelType]);
        }
    }
  });

With these two things in place any currently unsupported strategy could be put into the hands of developers to implement as necessary. This would also allow people to implement their own overrides to currently supported strategies to add additional functionality.

A couple times in the past I have opted to go with an alternative authentication implementation due to this limitation, it would be nice to see it added to the official project.

[DOSarrest]

It looks like the following if statement located at lib/passport-configurator.js#L609 also needs an additional condition.

 } else if (authType === 'custom') {
    var customCallback = options.customCallback || defaultCallback;
    self.app.get(authPath, passport.authenticate(name, _.defaults({
      scope: scope,
      session: session,
    }, options.authOptions)), customCallback);

[virkt25]

@DOSarrest Would you be able to submit a PR with the code changes and accompanying tests / docs for this feature?

cc: @bajtos you've done work with authentication. thoughts on this proposal?

[bajtos]

I am afraid I am not familiar with loopback-component-passport at all. I assumed it's just a thin wrapper around passportjs.org, but judging from the code seen in lib/passport-configurator.js, it's much more involved.

Having said that, I think we should allow users to configure custom authentication/authorization strategies, especially since Passport supports custom strategies too (see e.g. https://github.com/jaredhanson/passport-strategy)

@raymondfeng could you please join this discussion and explain how to properly configure a custom auth strategy with loopback-component-passport?

[DOSarrest]

Before doing any kind of PR I figured this idea should be discussed first to determine of the implementation provided is adequate or if another approach would be more ideal. This implementation appears to work well for our purposes, but perhaps with some additional input an even better implementation could be made as I am not very familiar with all of the internals.

Currently the aforementioned code changes are available here.

[DOSarrest]

No input so far, should I look at doing a PR? Also, I didn't see any where to document this specifically. There is a reference pointing to the official doc page but I did not see anything else.

[virkt25]

That would be the correct page to document this -- or you can create a new page in loopback.io.

cc: @raymondfeng

[DOSarrest]

Documentation has been updated and a pull request has been added. https://github.com/strongloop/loopback.io/pull/729 An example custom strategy was made to check/verify functionality. https://www.npmjs.com/package/loopback-passport-custom-strategy-example Only thing left should be to add a test which I am looking into now before adding a pull request.

[DOSarrest]

Added pull request https://github.com/strongloop/loopback-component-passport/pull/265

[DOSarrest]

If anything additional is required on my part, please let me know.

[DOSarrest]

There was some problems with the checks due to requiring git rebase and a comment that was longer than 50 characters. I have resolved them and everything should be good to go now.

[DOSarrest]

@virkt25 can you take a look at the changes and let me know if there is anything else you would like done or if PR is satisfactory.

[DOSarrest]

@virkt25 have not received a response on latest responses on pull requests, are you able to pull some weight and help move this forward?

[bajtos]

have not received a response on latest responses on pull requests, are you able to pull some weight and help move this forward?

FYI: most people don't receive notifications when a new commit is added to a pull request. It's best to post a comment when you have more or new code to review. I'll ping @raymondfeng and ask him to take another look at your patch #265

[DOSarrest]

@bajtos Thanks, I did post a new comment with @raymondfeng tagged. I received a response today about an hour ago and have since responded as per requested by @raymondfeng .

[DOSarrest]

@raymondfeng awaiting response in PR from you

[DOSarrest]

@bajtos not getting a response from @raymondfeng after tagging him, can you reach out to him again please and see if he can keep a watch on this?

[DOSarrest]

@raymondfeng Looking for some input related to my latest comment in the PR.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.