Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

operational issues around requests.Sessions though Django integration #451

Open
jmhodges-color opened this issue Apr 11, 2022 · 4 comments
Assignees
Labels

Comments

@jmhodges-color
Copy link

jmhodges-color commented Apr 11, 2022

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
Copy link
Author

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

@lepture
Copy link
Owner

lepture commented Apr 12, 2022

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.

@Martin-Lar
Copy link

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?

@lepture
Copy link
Owner

lepture commented Nov 10, 2022

@Martin-Lar yes, a PR is welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants