Not providing sufficient details for an auth provider should not be an error.

[Download]

I am getting started with Feathers JS. One of the things that won me over to pick Feathers JS was the way you guys integrate with express and your focus on using hooks, middleware and services. Great job with this project it's working very well so far.

I do however have one major gripe and that's with this error that I'm getting:

Error: You need to provide a Passport 'strategy' for your bitbucket provider

I am getting this error because I have created a generic configuration section in my config.json which contains the options for a whole list of auth providers, but I'm not actually using all of them (yet).

The problem is that JSON is not a very convenient format during development. I cannot comment out those configuration elements. Basically this error is forcing me to delete the unused configuration elements. This is inconvenient.

What I'm proposing is that we check whether all required options for oauth2 are provided before we attempt to set one up. If they are not all provided, then just log a warning and skip this key. Ideally, we would also allow you to force-skip keys, for example by changing their name.

I am already looking into the code and what I am proposing is already (a bit) in there. The code that is setting up the services for the auth providers is looping over the keys in the JSON and checking whether the current key is a provider key with this code:

src/index.js:114

// Because we are iterating through all the keys we might
// be dealing with a config param and not a provider config
// If that's the case we don't need to merge params and we
// shouldn't try to set up a service for this key.
if (!isObject(config[key]) || key === 'cookie') {
    return;
}

So this skips any keys that have anything else than an object as their value. And it has a hardcoded exception for the 'cookie' object.

I propose we go one step further and allow people to 'comment out' a provider key temporarily by prepending '//' to it's name:

// Because we are iterating through all the keys we might
// be dealing with a config param and not a provider config
// If that's the case we don't need to merge params and we
// shouldn't try to set up a service for this key.
if (!isObject(config[key]) || key === 'cookie' || key.startsWith('//')) {
    return;
}

As a final step, we can make this whole auth configuration a lot more friendly to deal with by just adding some more check to see if all options for the service have been provided before we attempt to initialize the service with them. If they are not all there, log a warning and skip this key.

There is already some checking going on:

src/index.js:126

// If it's not one of our own providers then determine whether it is oauth1 or oauth2
if (!provider && isObject(providerOptions)) {
    // Check to see if it is an oauth2 provider
    if (providerOptions.clientID && providerOptions.clientSecret) {
        provider = _oauth2.default;
    }
    ...

_{(the isObject(providerOptions) check here is redundant BTW, as it was checked just a few lines back)}

Let's just check the strategy field that oauth service is throwing on when not provided:

// If it's not one of our own providers then determine whether it is oauth1 or oauth2
if (!provider) {
    // Check to see if it is an oauth2 provider
    if (providerOptions.clientID && providerOptions.clientSecret) {
        // quick sanity check:
        if (!providerOptions.strategy) {
            debug(`You need to provide a Passport 'strategy' for your ${options.provider} provider. Skipping this provider.`);
            return;
        }
        provider = _oauth2.default;
    ...
}

[marshallswain]

Actually, this is already solved in the 0.8.0 beta on NPM. You now explicitly setup OAuth providers and pass the config in, which is much nicer. I don't recommend diving into 0.8.0, though. There are a lot of changes in the works right now. Instead, I recommend copying your config file and deleting the unused config keys as a temp workaround.

[Download]

@marshallswain Wow thanks for your quick response. Saves me whipping up a PR!

I recommend copying your config file and deleting the unused config keys as a temp workaround.

Yeah it's what I was doing. Great to hear this is going to be fixed!

Where is your development work happening? Not on the master branch?

[marshallswain]

Master is the current npm release. The rest of the branches contain dev work.

[Download]

Thanks @marshallswain !