Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preliminary token revocation support #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Preliminary token revocation support #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions pyramid_oauthlib/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,20 @@

OAUTH_PARAMS = (
'access_token',
'client',
'client_id',
'client_secret',
'code',
'grant_type',
'response_type',
'password',
'redirect_uri',
'refresh_token',
'response_type',
'scope',
'state',
'token',
'user',
'username',
)


Expand Down Expand Up @@ -63,7 +69,15 @@ def create_authorization_response(self, request,

@base.catch_errors_and_unavailability
def create_revocation_response(self, request):
pass
handler = self.response_types.get(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

request.response_type,
self.default_response_type_handler,
)
return handler.create_revocation_response(
request.url,
request.method,
request.body,
request.headers)

@base.catch_errors_and_unavailability
def create_token_response(self, request, credentials=None):
Expand Down Expand Up @@ -204,7 +218,9 @@ def includeme(config):

config.add_request_method(
lambda request:
server.create_revocation_response(request),
oauth_response(
server.create_revocation_response(request)
),
str('create_revocation_response'))

config.add_request_method(
Expand Down