I had a bit of discussion with @jaswilli about this, and it's pretty clear that our thinking to-date about how to manage tokens isn't going to work for more sophisticated use cases, especially as refresh tokens come into play.
We need to design something more complete.
For now, I'm focusing on the model in memory, and assuming that we can figure something out to represent it in the config when we're done.
Consider the case of an application that wants to use TransferClient to perform a set of operations, and already has a Transfer Access Token + Transfer Refresh Token in hand.
We could do this:
tc = TransferClient(access_token=transfer_access_token)
...
However, if the interpreter instance is long-lived or if there's a long delay between getting the access token and this instantiation of the client, we risk having tc throw a TransferAPIError with code 401 at any time, during any operation, as the token expires.
Even without a long period of time passing, the access token could be invalidated in the middle of an operation.
The catastrophically bad case is that of a paginated call which expires part-way through:
for item in tc.operation_ls(...):
call_func(item)could throw a 401 any time that the loop ends and triggers next() on the PaginatedResource.
Even if we tell people that they have to be ready to catch a 401 and refresh, the logic to resume pagination is fairly tricky, and we're losing a lot of the value of the iteration interface.
I'm going to enumerate the options I can think of as I think of them.
There are many more than what I'm listing here.
Option 1: Give TransferClient Refresh Tokens, Catch 401s Internally
My first thought was to give the TransferClient its refresh tokens directly, and have it handle things.
However, this starts to devolve almost immediately:
tc = TransferClient(access_token=transfer_access_token, refresh_token=transfer_refresh_token, auth_client_id=...)
tc.operation_ls(...)
TransferClient methods are then able to talk to auth.globus.org to refresh their tokens whenever they get a 401.
However, it's very weird that the client consumes an auth_client_id.
Without that ID, it's not possible to do the refresh token flow.
It's also a bit aberrant that TransferClient is talking to auth.globus.org -- that seems like a violation of the TransferClient / AuthClient separation.
Option 2: Give TransferClient an AuthClient to use, Catch 401s Internally
The logical next step is to give TransferClient a reference to an AuthClient in addition to its tokens.
ac = AuthClient(client_id=...)
tc = TransferClient(access_token=transfer_access_token, refresh_token=transfer_refresh_token, auth_client=ac)
Now, if we're inside of a method body, we'd do something like this:
def operation_mkdir(self, endpoint_id, path, **params):
resource_path = self.qjoin_path("operation/endpoint", endpoint_id, "mkdir")
json_body = {'DATA_TYPE': 'mkdir', 'path': path}
try:
return self.post(resource_path, json_body=json_body, params=params)
except GlobusTransferAuthRequiredError:
self.access_token = self.auth_client.refresh(self.refresh_token)
return self.post(resource_path, json_body=json_body, params=params)This is starting to look promising, but there are a couple of problems:
- What if the request mutates data in addition to returning a
401?
- Extra network IO when we should really know whether or not the token is about to expire (Auth gives us the
expires_in field)
- Will handle very poorly in the
PaginatedResource code, which must now see the TransferClient and AuthClient to update the access tokens
That first case seems like one we should rule out by just Not Letting APIs Do That.
If we can't make rules about APIs, this is going to get too painful.
A token could be invalidated at any time, so any call can produce errors -- we need to be able to reason about the retry logic in that case.
It would be nice to handle the extra IO load by checking expires_in, but that's not always possible.
If we refreshed recently, it should be.
I'd like to bundle that check with the error handling in some way, if possible.
Options 3: BaseClient accepts an AuthClient + Tokens, Check + Refresh Happens in BaseClient._request
This looks a little weird because BaseClient is a parent of AuthClient, but I think it works quite well.
Using Transfer as an example still,
ac = AuthClient(client_id=...)
tc = TransferClient(access_token=transfer_access_token, refresh_token=transfer_refresh_token, auth_client=ac)
This is the same as above, but then, rather than having tc.operation_mkdir handle the logic, it goes into BaseClient._request something like this:
# conditions necessary for a refresh to be at all possible
can_refresh = (self.auth_type == self.AUTHTYPE_TOKEN and
self.auth_client is not None and self.refresh_token is not None)
if can_refresh:
# looks up the access token + (nullable) expires_in for the given refresh token
# refreshes the token if it's near expiry, return the access token
self.set_token(self.auth_client.refresh(self.refresh_token, check_expiry=True))
r = self._session.request(method=method, url=url, headers=rheaders, params=params,
data=text_body, verify=self._verify)
# allow_refresh_and_retry is a kwarg to all request methods, defaults to True
# it is an explicit control on this behavior, for any endpoints where a retry after 401 is considered
# dangerous
if r.status_code == 401 and can_refresh and allow_refresh_and_retry:
self.set_token(self.auth_client.refresh(self.refresh_token))
r = self._session.request(method=method, url=url, headers=rheaders, params=params,
data=text_body, verify=self._verify)This is not meant to be what we actually write in BaseClient -- I've just simplified it a bit to show the kind of thinking I'm engaged in.
The thing I dislike the most is the relationship between BaseClient and AuthClient that it creates.
There's a bidirectional dependency there, and although no circular imports are being created it's still more complex than I would like.
Another variant on this is to have a AuthTokenRefreshManager object, which can be easily synthesized from an AuthClient object, and use that in BaseClient as well.
It doesn't really solve the relationship between a BaseClient and an AuthClient, but it makes the object model maybe somewhat cleaner.
![dependabot[bot] avatar](https://avatars.githubusercontent.com/in/29110?v=4 "dependabot[bot]")
![pre-commit-ci[bot] avatar](https://avatars.githubusercontent.com/in/68672?v=4 "pre-commit-ci[bot]")
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent