authlib icon indicating copy to clipboard operation
authlib copied to clipboard

operational issues around requests.Sessions though Django integration

Open jmhodges-color opened this issue 2 years ago • 4 comments

Thanks so much for the work on authlib. We came to using it from Auth0's recommendation. Now that we've been operationalizing it, we hit a difficult problem.

This is sort of one bug and sort of 3 bugs. I joined them to give the maintainers flexibility in how they wanted to respond without losing their individual motivations:

  1. There's no way to set timeouts on requests to server_metadata_url and jwks_uri. There's no way to pass a timeout parameter to them (preferably one that that a different timeout than the call to DjangoOAuth2App that caused the load_server_metadata function be called) nor can the user of authlib add a HTTPAdapter subcall with session.mount that adds default, overridable timeouts because of the class-level factory variables in authlib. This has mattered because requests.Session has infinite timeouts by default and we're seeing double digit second request latencies (esp to the metadata requests) from Auth0's APIs.
  2. the requests.Sessions are recreated for "meta" requests to server_metadata_url and jwks_uri, and possibly in other codepaths, which means more reconnects than needed in a Python process lifetime.
  3. As hinted at in 1, there's other work that's support by requests but not currently supported by the authlib (through session.mount, but I imagine other things). It's not supported because there's no clear way to override the Adapters on OAuth2Session, especially when the code is coming to it through the Django integration. Currently, it seems like the only option would be to override client_cls in a subclass of DjangoOAuth2App. But, of course, that only works if you know at type definition time all of the info needed to make it and it's not clear what backwards compatibility expectations to put on it since it's pretty deep in the type hierarchy. Hindsight being 20/20, if the factories were not class-level variables that couldn't be overriden from OAuth#register and maybe if some equivalent to OAuth2Session wrapped a requests.Session instead of being one, this could be handled from the user of authlib's position. As is, it seems like some API expansion needs to be done.

Thanks so much for the work you've done. These kinds of operational improvements would be huge for us. Do you folks have any ideas on how to manage these issues?

jmhodges-color avatar Apr 11 '22 21:04 jmhodges-color

To be clear: we're open to providing some patches (balanced against our other tradeoffs), but the design work seems like it needs maintainer thought to be accepted

jmhodges-color avatar Apr 11 '22 21:04 jmhodges-color

1. timeout issue

There is a client_kwargs parameter, but it is not used in .request method. This need to be fixed. But all requests will share the same client_kwargs. I'll fix this.

2. meta requests

This is not a big thing, because those meta data will be cached on instance. If you don't recreate the instance every time, it won't send too many requests to meta endpoint.

3. additions

I would like to accept patches for the session.mount.


Thanks for your feedback.

lepture avatar Apr 12 '22 14:04 lepture

Hello, @lepture we're having similar issues with timeouts when calling load_server_metadata, more specifically, we have no way to add a timeout on OAuth2Session.request in this case. Would you be open to a PR on this?

Martin-Lar avatar Nov 09 '22 16:11 Martin-Lar

@Martin-Lar yes, a PR is welcome.

lepture avatar Nov 10 '22 09:11 lepture