orcasgit / python-nokia

Library for the Nokia Health API
Other
57 stars 23 forks source link

Oauth2 support #14

Closed brad closed 6 years ago

brad commented 6 years ago

Build upon changes from @magnific0 to get OAuth2 supported on Python 3 and 2. Fix tests

Closes #12

brad commented 6 years ago

@magnific0 If you get some time to try this branch I would appreciate it. I'm going to work on django-nokia and merge this once that work is finished.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling c4bc1edd0781f3af727bdb2568807c3c24391613 on oauth2 into 01e42846e793f25602c9a3c4e8e54ec2823da1da on master.

brad commented 6 years ago

I think we may want to add a migrate_to_oauth2 method to NokiaAuth before merging this.

magnific0 commented 6 years ago

@brad thanks for looking into it further. I will have a look later.

In the mean time I have been using it in practice for a while. I think the code still needs to be revised. The Nokia API expects the access_token as part of the parameters. requests_oauthlib only supplies it in the headers ('Authorization': 'Bearer'). That's why I added this line. The problem is that upon refresh it doesn't get updated in params until the next request, while internally requests_oauthlib still (re)tries the request after refreshing, resulting in a 401 response. I have just written a solution which involves leaving token_updater blank and uses "except TokenUpdated" to catch a refresh and reset the params and only then proceed to complete the request. Which seems to do the trick.

It's not very elegant. Maybe you have a better idea? Otherwise I can add another PR on the new oauth2 branch with my solution.

brad commented 6 years ago

Thanks @magnific0. Maybe we can reproduce the situation in the test suite

magnific0 commented 6 years ago

@brad I just put my solution in PR #15 so you have a better idea of the fix. I think putting it in the suite is an option, but I see it more as an error in the implementation. Since the request should use the new access_token after refreshing. It's just a bit quirky that Nokia wants the access_token in the parameters (instead of / alongside the authorization headers).

magnific0 commented 6 years ago

Re: migrate_to_oauth2, I assume you're talking about the option to exchange the old OAuth 1.0 tokens for new OAuth 2.0 tokens. This idea is certainly appealing, although the migration can't be made seamless since users will need to apply for a new application anyway. In the end you will just skip the NokiaAuth step. So I'm not sure whether it's worth the effort for a one time deal.

brad commented 6 years ago

@magnific0 Awesome, that is super helpful, thank you!

brad commented 6 years ago

@magnific0 Yes, I was talking about exchanging the OAuth1 tokens for OAuth2 tokens. I'm going to be making the effort regardless for django-nokia, so if it makes sense to have that code upstream in python-nokia so others can enjoy it, it's a simple copy/paste :smile:

magnific0 commented 6 years ago

That makes sense. I'm happy to test anything you throw my way. So far I can confirm the latest revised version of __init.py__ works well.

There were some issues I encountered with urllib3 hanging so long (at urllib3/util/connection.py", line 69, in create_connection) that on fetch_token the code had already expired (must be exchanged for tokens in 30s!). The same long holds happen on refreshing but not on regular API calls (basically all account.health.nokia.com requests).

The weird part is that there are no connection errors or anything. It just takes very long to establish, but then the request still comes through. There are many similar reportings of this regarding urllib3 on the web, but no real answers.

I fixed this by adding timeout=2 to the fetch_token and request calls. However, I don't think this fix belongs in the code unless there are many similar reportings. Just something to be aware of.

brad commented 6 years ago

This is working great in my testing. I'll release this to pypi as 1.0.0 since we made some major incompatible API changes and any minor patches can be minor releases building on that. :shipit: