Update authorization.py - use the _create_request method and change requ... #197
Conversation
…equest.oauth_token attribute to resource_owner_key The Request() constructor doesn't give the request.oauth_token attribute that the create_authorization_response relies on. The _create_request() method doesn't either, but it gives something equivalent.
|
Oops. I got too excited to make the same changes in AuthorizationEndpoint.get_realms_and_credentials() |
| raise errors.InvalidClientError() | ||
| if not request.oauth_token: | ||
| raise NotImplementedError('request.oauth_token must be set after ' | ||
| if not request.resource_owner_key: |
There was a problem hiding this comment.
Not sure what caused me to write this check in this manner. If you fancy move the token check up before self.request_valdiator.verify_request_token and make it raise
raise errors.InvalidRequestError(
description=('oauth_token parameter must be present '
'in the URL query.'))
or similar that would be awesome.
There was a problem hiding this comment.
Yeah, I was eyeing that line, but forgot to make those changes yesterday.
|
@kdazzle Thanks for the PR! I initially avoided the use of _create_request because earlier that implementation would cause this to fail because it also checked the request signature (which there is none during this step). It seems like this works well now tho and agree that an update might be in order. Could you please also update the documentation with the error message thrown if oauth_token is not set on request. Oh and feel free to push more changes to this PR. |
…ferred to request.resource_owner_key instead of request.oauth_token
|
@ib-lundgren Thanks for creating (and maintaining) such an awesome library! I made the changes you suggested and a couple of others in the endpoints. However, I couldn't find any documentation related to the NotImplementedError, so I didn't make any changes there. |
…source owner key as an access token, which doesn't yet exist, instead of a request token
| headers=headers) | ||
|
|
||
| if not request.resource_owner_key: | ||
| raise errors.InvalidRequestError('request.resource_owner_key must be set after ' |
There was a problem hiding this comment.
Please change error message to Missing mandatory parameter oauth_token.
|
@kdazzle Great work. I am surprised that I overlooked such large oversights like the nonce validation token parameter. Will push a point release when this is addressed. Many bonus points if you add tests for this =) Feel free to add yourself to authors too. |
…arameter in resource.py from request_token to access_token; updated authors
|
@ib-lundgren Aw sweet, thanks! Consider me added. I haven't been able to do any tests, but I will try to write some soon so that everyone's life (except Travis CI's) can get a bit easier. |
|
Looking good. For the tests an ``assert_called_with` similar to below around https://github.com/idan/oauthlib/blob/master/tests/oauth1/rfc5849/endpoints/test_resource.py#L75 (and equiv. for the other endpoints) would go far. |
|
Sorted out a few quick tests. Thanks a lot for the PR =) |
|
Heh, sorry about that. I swear I was going to try to do some tests later... |
...est.oauth_token attribute to resource_owner_key
The Request() constructor doesn't give the request.oauth_token attribute that the create_authorization_response relies on. The _create_request() method doesn't either, but it gives something equivalent.