Improve grant type checking

[nunofgs]

Currently we require that the supported grants be given to the constructor, ie:

var oauth = oauthserver({
  grants: ['password', 'refresh_token']
});

But we also require that models implement the checkGrantTypeAllowed() function which gives models an opportunity to reject access for a given clientId and grant_type combination.

This implies that if we have different grant_types for different clients, we need to pass all of them to the constructor, even though we'll be verifying the grant_type again in the checkGrantTypeAllowed() function.

I propose that we remove the grants option from the constructor.

Thoughts?

[OllieJennings]

In my opinion, the grants array is simply used as a quick check, meaning that you don't have to worry about the getClient() function calling the database to check the client details first (which then goes onto the checkGrantTypeAllowed()) , instead you get an immediate response.

This helps things such as:

• Much quicker response if an unsupported grant type
• Not wasting database requests (esp if you're on amazon which count requests you make)
• Much clearer when looking at the definition in your app.js file or etc..

[nunofgs]

Hi Ollie.

In my use case I am supporting almost every OAuth grant (except the custom flow), but the actual supported grant is stored with the client, so I am forced to pass every possible flow in the constructor, which seems a bit weird to me.

I understand the necessity of minimizing database calls but I still think that the module's architecture should not suffer for it. In fact, throwing the "unsupported grant type" exception in the middle of extractCredentials() seems to be asking to be refactored out into its own method.

How about adding an optional callback after the extractCredentials() but before getClient()?

For example, we could:

• Add a checkGrantTypeAllowed(grantType) (that doesn't need a client) right after the extractCredentials() method.
• Rename checkGrantTypeAllowed(client, grantType) to checkClientGrantTypeAllowed(client, grantType)

WDYT?

[thomseddon]

I guess the idea of keeping this check inside the module is to dissuade users from creating grant types that are not supported in the spec (intentional or unintentional)

I would also think the current process works for the majority of use cases so wary of adding another model method, I would be up for perhaps supporting a catch all '*' option?