Closed sangaline closed 4 years ago
Hey @sangaline, I recognized your name! Thanks again for all of your contributions/advocacy, and for this very detailed issue (no need to apologize for verbosity!)
That makes sense if you're dealing with that sort of orchestrated and scaled out environment and using OAuth.
I like your proposal for how to deal with it - it sounds like a powerful way to implement this sort of functionality. Go right ahead! 😄
Introduction
Hi @jpetrucciani, I've submitted a small handful of PRs in the past and I'm a regular user/advocate of the project. The company I work at would like to use the library in a distributed environment with oauth2 authentication, but we've run into contention issues where the different containers essentially fight over access tokens because only one can be active at any given time. This leads to almost every request initially failing with 401 errors and then only working after a new access token has been requested from Hubspot. It would be ideal if we could share the tokens across workers through an in-memory store like Redis to avoid these issues. This is a significant enough change that I thought it would be good to open an issue to discuss the approach before implementing it (and I would be happy to implement it, this isn't a request for you to do so).
How oauth2 is currently handled
The easiest way to initialize the client is through the
Hubspot3
class in__init__.py
. This supports four options which are all required for Hubspot's oauth authentication scheme:client_id
- The client ID of the app that's configured for oauth2 authentication. This is non-sensitive and doesn't change.client_secret
- The client secret for the app. This is sensitive and doesn't change.access_token
- This is a temporary token that provides access to the API as an alternative to using the API key. This is sensitive and expires after 6 hours.refresh_token
- This is a temporary token that is required to generate new access tokens after they have expired. This is sensitive, and although it doesn't explicitly expire, it will rotate regularly through the course of requesting new access tokens.These are stored in a dictionary called
auth
on aHubspot3
instance after initialization, and the arguments are passed into the different client classes when they're accessed as properties. For example, accessing thecontacts
property on the client will initialize a newContactsClient
instance via this code:All of the various clients inherit from BaseClient which stores the credentials on instance properties when initialized. The credentials are then used in
BaseClient._call_raw()
whenever a request is made, and a new access token is requested if aHubspotUnauthorized
exception is raised. Theaccess_token
andrefresh_token
properties are then replaced on the instance by this code:The
OAuth2Client
class handles refreshing the tokens, but it isn't responsible for storage or maintenance of the tokens.Proposal for supporting dynamic storage of tokens
A relatively simple and backwards compatible change is adding two new initialization parameters to the
Hubspot3
andBaseClient
classes:oauth2_token_setter
- A setter with typetyping.Callable([BaseClient, typing.Literal['access_token', 'refresh_token'], str, str)
, a signature ofcallable(base_client, token_type, client_id, token)
, and a default value oflambda self, token_type, client_id, token: setattr(self, token_type, token)
. This would be used to store either of the tokens through either some external mechanism or just on the instance. The client ID is included to accommodate for the possibility of multiple clients being used in the same application.oauth2_token_getter
- A getter with typetyping.Callable([BaseClient, typing.Literal['access_token', 'refresh_token'], str)
, a signature ofcallable(token_type, client_id)
, and a default value oflambda self, token_type, client_id: getattr(self, token_type)
. This would be used to retrieve either of the tokens through some external mechanism.The logic in the
__init__()
methods ofBaseClient
andHubspot3
as well as theBaseClient._call_raw()
method would then use the setter and getter in place of direct attribute access. Theaccess_token
/refresh_token
attributes would still be used by default, and this should be completely backward compatible while also supporting more sophisticated storage mechanisms.Conclusion
Apologies for the verbose issue, but I wanted to clearly lay out what I'm proposing. If this sounds good to you, then I'll go ahead and implement it.