support for client_id and oauthlib PR #593 (Issue #585) #331
Conversation
And in reverse, it would be great if someone here can review oauthlib/oauthlib#593 before we merge that. |
requests_oauthlib/oauth2_session.py
Outdated
| raise ValueError('The required paramter `username` was supplied, ' | ||
| 'but `password` was not.') | ||
|
|
||
| # TODO: |
There was a problem hiding this comment.
As discussed in oauthlib/oauthlib#585 (comment), username and password are used only for LegacyApplicationClient. However, we should not restrict or try to remove them from the params, if they are present. But we should not do any processing either, since they are not described in the RFC (for those other grants!).
There was a problem hiding this comment.
Great. so just remove the TODO line that I put in for code review - correct?
There was a problem hiding this comment.
done.
the way i was notified of this by code-highlighting, I thought there may have been another issue or mis-interpretation. I just wanted to ensure we are both talking about the TODO line.
|
LGTM, need to have a review from @requests-oauthlib team now ;-) |
|
This pull request can't be reviewed until all the automated tests pass on Travis CI. |
|
IIRC, this introduced a breaking change(s) against the live version of oauthlib. The next version of oauthlib and this PR have tests to catch an improper usage pattern that trigger an exception, so tests will fail on Travis by design. Last I checked, they all pass against the next oauthlib (master branch), which has the mid set of changes merged already.
At least that's what I remember. It's been a while. I'm on vacation through mid January so someone else working on oauthlib will have to address this.
…Sent from my iPhone
On Dec 27, 2018, at 6:28 AM, David Baumgold ***@***.***> wrote:
This pull request can't be reviewed until all the automated tests pass on Travis CI.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I just merged #341 earlier today, which should restrict compatibility to versions of oauthlib below 3.0. Once oauthlib 3.0 is out, then it will be easier to ensure that requests-oauthlib is compatible with it. |
|
Can you guys take a look at #357, and tell me what you think? |
|
rescinding in favor of #357 |
This PR updates
requests_oauthlibsupport to oauthlib/oauthlib#585 and oauthlib/oauthlib#593Of importance:
The logic in
fetch_tokenwas largely redone. The existing approach tried to support an array of different ways to provide client and user details which led to easily broken implementations. The new approach favors full RFC compliance by default, but still maintains ways to build broken clients and talk to broken servers.New tests were added to ensure the above functionality works.
Some existing tests were changed. Notably calls to
fetch_tokeninOAuth2SessionTestallowed the LegacyApplicationClient to invoke the endpoint without the required user credentials. The tests now ensure an exception is properly raised, and then retest with necessary arguments added in.This PR is waterfalled against a #593 and should not be reviewed until that is accepted.