Preliminary token revocation support

[dokai]

Hi

I've started testing pyramid-oauthlib and would like to use it in a project. I have interest in supporting token revocation and have managed to get it roughly working with the given changes. I'd be interested in knowing your plans on how to properly implement it and would be happy to work on this part.

One issue I noticed is that the RevocationEndPoint.create_revocation_response builds the request object itself and passes that to the RequestValidator.validate_revocation_request method. This breaks the promise that the request parameters passed to the custom validator would be pyramid.request.Request instead of oauthlib.common.Request.

Looking forward to your comments.

[coveralls]

Coverage decreased (-1.11%) when pulling 20eba34 on dokai:token-revocation into 9552ec9 on tilgovi:master.

[tilgovi]

Hi! Sorry! I wasn't watching this repo, apparently. I'm taking a look! Thanks!

[tilgovi]

The general pattern so far has been to add request parameters so that a lot of the OAuthLib endpoints might not care that it's a pyramid.request.Request object, so they can inspect the arguments they need to and pass through to the request validator. In practice, I've really mostly been using custom grant types and token types that haven't made use of the request validator. I will look into revocation soon and maybe also do some integration testing with the request validator.

[dokai]

Thanks for getting back on this. We have a project where we're using a slightly modified version of pyramid-oauthlib successfully for a combination of Resource Owner Password Credentials Grant, token refresh and token revocation.

I'd be interested in seeing full support in pyramid-oauthlib and happy to help with testing etc.

[tilgovi]

I don't think the revocation handler has to know about response types. It needs to validate the revocation request (the base class implementation of oauth2.RevocationEndpoint.validate_revocation_request should handle that).

What's confusing is that I the revocation endpoint is the only endpoint that needs to know about the RequestValidator directly. The rest of the endpoints all delegate to token, response, and grant types, which may use the validator. These built in handlers all take a request_validator keyword to their constructor.

Since pyramid-oauthlib creates exactly one Server instance when it's included and cannot know what implementation of RequestValidator is needed, what do you think of a directive like config.set_request_validator(). You must call it before you register any of the handlers and whatever you pass will be given implicitly as an extra keyword argument to all of their constructors.

config.set_request_validator('my.validator')
config.add_grant_type('oauthlib.oauth2.ResourceOwnerPasswordCredentialsGrant', 'password')

It means that, while the OAuthLib servers are usually configured to have one class be both a grant type and response type handler, we would actually probably instantiate the grant type twice (once in each role). I don't think that's a problem.

[dokai]

The revocation endpoint does seem like the odd man out here. If we have config.set_request_validator define a default request validator, would the methods (e.g. config.add_grant_type) currently accepting a request_validator parameter still accept one as an override or would it go away? I don't see it such a problem if it goes away but it does change how it is now where you can provide a different request validator for each call.

Would the parameter for config.set_request_validator accept both a python import path to a callable producing the validator and a direct object reference to one?

[tilgovi]

I don't see it such a problem if it goes away but it does change how it is now where you can provide a different request validator for each call.

I'm don't know that I see a use case for different request validators, but maybe there is one. If there is such a use case, one clear response is to just require the user to make some subclass mixing in the various request validator pieces they need so they can create one composite validator.

Would the parameter for config.set_request_validator accept both a python import path to a callable producing the validator and a direct object reference to one?

Definitely. All the directives accept dotted string references.

[tilgovi]

@dokai I'd like to finish this up sometime soon.
Having just done some refactoring of my own application, I think I prefer specifying the request validator for each component, since it avoids lots of conditionals and makes it easier to modularize/encapsulate usage of a particular grant type. Given that revocation is related to the tokens should we maybe just require that for revocation a request validator be passed to add_token_type and use the token type hint (or the default token type) and then call that validator's revoke_token method?

[dokai]

+1 for a simpler, encapsulated approach. Could you show a minimal example how this would be wired in with your proposed approach?