Do not convert to bytes the refresh_token

[humitos]

When the body is encoded, a mix of bytes and unicode happens and it
fails at this line

https://github.com/requests/requests-oauthlib/blob/c472e6bf0468f1bd52b70a64e9aac92bee41c491/requests_oauthlib/oauth2_session.py#L285

(Pdb) body
u'grant_type=refresh_token&allow_redirects=True&refresh_token=b%27z9BSRKqf2QD4pcDhqj%27&client_id=CLIENT_ID&client_secret=CLIENT_SECRET'

(note those weird %27 in the string)

Closes #2993
Closes #2992

[ericholscher]

Can we add some logging around this to catch failures better in the future?

[humitos]

I think we should connect the logs from this module, since it's already logging the body: https://github.com/requests/requests-oauthlib/blob/c472e6bf0468f1bd52b70a64e9aac92bee41c491/requests_oauthlib/oauth2_session.py#L287

[ericholscher]

I mean something that would catch the exception/error that would presumably happen on invalid refresh token? The full body isn't gonna help us catch errors, just spam logs :)

[agjohnson]

Ideally, we should still solve the issue of a failure here not updating the GitHub repos that were successfully returned.

[humitos]

I mean something that would catch the exception/error that would presumably happen on invalid refresh token?

I think I will need more directions here. I mean, I can catch the exception as you say but then?

I updated this PR with the solution that I imagine you want :)

The self.get_session is used in a couple of places (sync repos, setup and update webhooks) but in the other cases it seems that the execption is properly handled. So, this is the one that was missing.

Ideally, we should still solve the issue of a failure here not updating the GitHub repos that were successfully returned.

I need to take a look in deep at this (each sync is ran in a separate celery task or the same?). I suppose that it's the same, otherwise it shouldn't break the other service. If this is the case, I think I solved it by returning [] if something fails. Let me know, since I will need to apply the same logic at https://github.com/rtfd/readthedocs.org/blob/f5d3a8aae4a5338b02899e2b9f6ca90df738cd75/readthedocs/oauth/services/github.py#L151-L163 (maybe I can create a comon paginate method that handle this and a _paginate in each subclass)

[agjohnson]

Correct, all providers are synced in the same task. I am confused why catching these exceptions was even needed now though. Most of the calls in the oauth services are themselves wrapped in try catch, and ultimately raise an exception to the user.

That is, we want to raise Exception('You should reconnect your account'), which would raise the error to the user -- at least in theory. I haven't tested this.

[humitos]

I just refactored the code to handle the exception in both services. For that I created a common paginate method in the base class and implement two small methods in each of the subclasses.

If something goes wrong, it returns [] as results so other services requests are not affected by the one that it's failing.

@agjohnson can you take a look again at this PR?

[agjohnson]

This looks great 👍

The abstraction for paginate is a lot nicer now. We'll want to watch for more errors here, but I think the error reporting back should be fine here.