Support for async httpx clients

[maxclaey]

The auth flows for OAuth2ResourceOwnerPasswordCredentials, OAuth2ClientCredentials, OAuth2AuthorizationCode and OAuth2AuthorizationCodePKCE currently use synchronous HTTP requests to request new tokens. However, in the Advanced Usage documentation off httpx it is advised to implement both .sync_auth_flow() and .async_auth_flow() instead of .auth_flow() when I/O is needed in the auth flow. This pull request replaces the auth flow by a synchronous and an asynchronous variant. The asynchronous flow is identical to the synchronous flow, expect for using asynchronous HTTP requests using an httpx.AsyncClient. Thanks in advance for considering this PR!

[Colin-b]

Hello @maxclaey ,

Thanks a lot for this pull request. I will try to find some time to dive into it. However I already have one question:

As there is a token cache, the actual HTTP call(s) to retrieve a token are not supposed to occur that often. From my experience, a token usually lives from 1 to 12 hours, meaning all your HTTP async calls performed during this time will in fact have resulted in a single HTTP sync call.

I wonder if there is a real enhancement here, could you provide me some insight?

Thanks again

[maxclaey]

Hi @Colin-b, thanks a lot for the reply! In agree that the synchronous request should occur too frequently. However, it gave some issues for me in a project I'm currently working on.

We have a multi-service system were currently the token issuing is part of a broader service (will be changed later on). In this setup, that service also needs auth tokens for API calls to the other services. This boils down to a service having to request auth tokens to itself, which locks when using blocking synchronous requests. I fully agree that there are numerous other solutions to this problem (which we'll implement as well at some point), but in general I didn't think it is a bad idea to allow both and sync and async behaviour as intended by httpx, and not mixing async and sync request.

So in short, I agree that the full sync behaviour isn't really problematic, but I'd argue that it's generally better practice to follow the split sync/async guidelines.

Thanks again for considering, please let me know when you have any further questions or remarks!

Best regards

[Colin-b]

I do agree it would indeed be better in any case to avoid blocking the async queue when issuing authentication queries. My main concern here is that the test suite does not test async behavior now, and I will have to ensure that every test case on sync behavior also ensure the corresponding sync behavior (that's ok, that's on me I had it coming).

The test case part is certainly the reason why test coverage is not reached anyway as you introduced new paths for async while no test case currently check async behavior. Please don't add them in your PR, I would rather do it myself this time around.

[maxclaey]

Ok, cool! Again, thanks in advance, and let me know if I could be of any help!

[bobh66]

I also have a scenario that could use this enhancement - we have a kubernetes operator that is running async, and it needs to get a token from a remote system. If the token request is synchronous and the remote system is unavailable for some reason, the operator stops processing until either the remote responds or it gets killed by the kubernetes liveness probe. Even if the timeout is changed to be shorter than the liveness probe, having the operator stopped while the request is outstanding is a problem. It would be preferable to have the token request running async so it's not blocking.

[Colin-b]

Just a heads up, I did not forget this, I just did not had time to properly work on this for now. I still plan to, thanks for your patience.

[sonarcloud[bot]]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
57.3% Duplication

[rafalkrupinski]

Am I getting it right, auth_flow() should yield requests rather than making them itself?

Would it make the code less readable?

[Colin-b]

Just a heads up, I did not forget this, I just did not had time to properly work on this for now. I still plan to, thanks for your patience.

The develop branch is now ensuring coverage on async flows (no change in the codebase for now, async still does sync). Once your conflicts will be resolved I will have a look at this once again.

Thanks for your patience, looking forward to see where we go with async