Skip to content

Conversation

@rafalkrupinski
Copy link
Contributor

@rafalkrupinski rafalkrupinski commented Mar 7, 2024

Tried to make a minimal change to get some feedback, so only headers are supported.

The entire stack is changed to generators. requesting and refreshing tokens yield requests, TokenMemoryCache.get_token() passes them through.
It's a breaking change for those who use or inherit token cache classes, or inherit from httpx_auth OAuth2 related classes.

MultiAuth has now separate implementations for sync and async.

Most tests changes replace client with headers; token cache test needed to be changed since now get_token is a generator function.

The custom headers parameter is sometimes called headers and other times token_headers. I thought in the API (user-facing) it's sufficient to use shorter name, but internally I wanted to separate headers set to the user request and the ones for OAuth2 requests.

Technically setting meta-auth headers could be delegated to another Auth instance (meta-auth). The previous version of this change implemented this, and it made the code somewhat more complex. I think setting a couple of fields (headers, cookies etc) doesn't require for such complexity. Also I don't expect anyone would need OAuth2 to get an OAuth2 token, which I think wasn't supported by the version with internal Client either.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
41.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@rafalkrupinski
Copy link
Contributor Author

Continuing #72

Copy link
Owner

@Colin-b Colin-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the following remarks:

  1. What would be the benefit of yielding ?
  2. Client needs to be supported, it provide much more than headers. Also I don't want to break the interface for now if possible.

@rafalkrupinski
Copy link
Contributor Author

rafalkrupinski commented Mar 14, 2024

I have the following remarks:

1. What would be the benefit of yielding ?

httpx.Auth instances should yield any auth-related requests before finally yielding the actual user-made request with added auth headers. This way, as long as it doesn't use any IO, a single implementation works with both Client and AsyncClient. Otherwise any IO or locking code should be put in Auth.a/sync_flow() functions.

2. Client needs to be supported, it provide much more than headers.

Like I wrote in the description, I tried to make a minimal change just to get a feedback.

What exactly do Auth implementations need client for, other than simply sending token/refresh requests?

Supporting properties of httpx.Request (cookies, query and path parameters and request body) is trivial. Proxy and TLS settings can be handled by client mounts, like we talked before.

Is there a need to handle redirects or auth for auth requests (meta auth)? Cookie state (beyond what's already there)?
Am I missing anything?

Also I don't want to break the interface for now if possible.

There's a clear upgrade path - request stuff (headers, etc) goes into Auth instances, connection stuff to the client mounts.

For alternatives, there's #48, or perhaps a function in OAuth2Base could handle auth requests: if client is set, use it for the purpose, otherwise yield. I'd recommend against it, except as a temporary solution.

But what's wrong with breaking the API? I thought httpx-auth was in version 0.x for exactly this reason...

@rafalkrupinski
Copy link
Contributor Author

rafalkrupinski commented Mar 26, 2024

@Colin-b any ideas how would you like to move this forward, if at all?

@Colin-b
Copy link
Owner

Colin-b commented Apr 4, 2024

@Colin-b any ideas how would you like to move this forward, if at all?

IRL is pretty busy but I will take a look at this next week if everything goes well.

@arseniiarsenii
Copy link

Hi, @Colin-b! Thank you for this awesome project. I'd really love to use it in my code, but I only use asynchronous requests, which the library currently does not support. Is there any chance that this PR is merged anytime soon? It seems a little stale. Thank you

@rafalkrupinski
Copy link
Contributor Author

Sorry, @Colin-b doesn't seem interested, and I'm migrating away from httpx so I won't maintain it.

@Colin-b
Copy link
Owner

Colin-b commented Sep 19, 2025

I plan on going over every opened PR and issues before end of year. However full support for async auth calls might not be done this year @arseniiarsenii

@rafalkrupinski
Copy link
Contributor Author

I've been thinking about this for my project, and I've concluded that auth objects in httpx don't need to do any IO, so the problem with sync/async disappears. But it would need an architectural change on the app side. Comments are welcome.
python-lapidary/lapidary#87 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants