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

FR: callback to catch token exceptions #155

Open
judgej opened this issue Jun 25, 2017 · 7 comments
Open

FR: callback to catch token exceptions #155

judgej opened this issue Jun 25, 2017 · 7 comments
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@judgej
Copy link

judgej commented Jun 25, 2017

When the client deems a token to have expired, using its locally stored creation time and expires_in values, it will automatically renew the token. The renewed token can then be stored/persisted by the application using the callback provided in #110 That works great, and takes the renewal burden away from the business logic of the application.

Now, the locally stored expiry time may be out of step from the real expiry time. This is especially the case at the edge of the token lifetime where HTTP delays in the fetching of the original token may put he expiry time out by one second or more, leaving the application thinking the token has a second more of life, but Google insisting the expired a second ago. This results in an exception, right in the middle of the business logic using the API.

This exception can be caught, the message checked, a token renewal attempted etc and that can get things back on track. However, that is not a domain that should impinge on the business side of the application; it really does not care about the OAuth negotiating wranglings, and it really should not have to care.

So, what I would like to propose for discussion, is a callback that can be used in a similar way to the expired token callback, to allow the application to invisibly attempt to fix any token issues in the event of an invalid token being found. I guess this callback would replace the exception once, then the client would reattempt the remote API call. If it works, great, all carries on happily. If it does not work, then fine, the application had a chance to fix its tokens but couldn't.

Issues I see are the recursive nature of this callback: the callback to the client trying to access the API would need to use the same client to renew the tokens. That may or may not have side-effects.

My workaround at the moment, to try to steer away from this edge-case, is to knock five minutes off the expires_in that the client supplies for a new token, so it renews early. But that's not ideal. This is all in the interest of keeping the lights on in offline access to the API when OAuth authenticated by an end user.

@judgej
Copy link
Author

judgej commented Jun 25, 2017

Before I start diving into the code and doing PRs and suchlike, I just wanted to check if I am missing something with perhaps other workaround, or if other people also have this [potential] problem.

@bshaffer
Copy link
Contributor

Thank you for reporting this issue! We would love to see a PR to fix this. Another way we could do this is we could be conservative when checking to see if the access token is expired, and give ourselves a few seconds of wiggleroom, e.g.:

$wiggleroom = 5; // 5 seconds
$isExpired = time() + $wiggleRoom < $expires;

@judgej
Copy link
Author

judgej commented Sep 25, 2017

I haven't looked at this for a while, so I'll try the latest version and have a dig around again.

I don't have any qualitative evidence, but I would think the wiggle room would help some cases where the network happens to be a bit slow. It is just renewing a few seconds early, which can't be a bad thing. I'm using a longer wiggle room because I have some longer running scheduled jobs. I'm guessing long running jobs would be discouraged on cloud-based services where each job is there to do something simple and finish quickly.

@bshaffer
Copy link
Contributor

I think the main issue here is the library cache by default does not actually look at the token expiration. It just calls for a new access token on every request and caches the fetched access token in memory.

One solution is to store the full access token in the cache, and check the expiration when the access token is retrieved (or set the cache's expiration to the token expiration. either way should work as long as the expiration is valid). This would allow users caching across multiple requests to make a minimum number of token requests to the APIs.

@judgej
Copy link
Author

judgej commented Sep 26, 2017

Let me take a step back and explain how I think this works, where I think - as an application developer - it can make things difficult. If my understanding is incorrect, please say:

  1. On initial authorisation, this library obtains an access token, a refresh token, and starts a 30-minute clock ticking on the access token. These are all passed back to the application to look after in the context of a user account, or some other entity the user owns in the application.
  2. On each execution of the app - perhaps a page load, or a scheduled service run - the access token, refresh token and creation timestamp are used to initialise the library.
  3. If the access token time has expired, i.e the 30 minute countdown has been passed, then the library will fetch a new access token using the refresh token and restart the timer.
  4. If a callback has been registered. the application will be informed of the new access token so that it can be stored against the user's account. Note: the expiry time is not provided in this callback, and the application has to assume it is 30 minutes. I think the expiry time needs to be included here.
  5. The API call to whatever Google service is then made using the current access token.
  6. At this point, if it turns out the access token has expired (assuming we just got the timer wrong) then an exception is raised. This is the main problem for me, as that exception now has to be handled by the application. It throws the execution flow way back out of the context of what was being called. In reality it is not an exception to the application - it is something the library can handle by renewing the access token. The "wiggle room" will help to reduce the chances of this happening, but IMO whatever time you use will ultimately be arbitrary and in all probability will not be long enough at some point when networks are slow.

On point 4, I have seen applications that do not use the callback, and retain the original access token without ever updating it. They end up renewing the token on every page load, once past the first 30 minutes, which can't be desirable. I realise that is a problem in the application development, but I wonder if it just needs to be highlighted more in the documentation?

My use-case for all this is an application that runs on a user's own server or site, that the user authenticates, but then accesses the Google services on its own using scheduled jobs. I would like to be sure expired tokens (that may expire before the 30 minutes, or where the 30 minute timer was started significantly later than Google's own timer for whatever reason) are handled smoothly and preferably invisibly, and that the application is always informed about the new access tokens to help reduce hits on your services.

Hopefully that clarifies my thinking and isn't just me repeating myself :-)

@bshaffer
Copy link
Contributor

This all sounds accurate, thank you for taking the time to type this out. Although I'm not sure about point 2. When are the previous tokens used to initialize the library? I assume this is happening in the application layer.

We could definitely include in the Middleware (or add a new middleware) the logic to retry the authentication call if the request throws an expired exception. What do you think of the following changes?

  1. Add an Expiration Handling / Retrying Middleware.
  2. Provide the full access token to tokenCallback
  3. Take into account token expiration in the caching layer.

I am seeing another problem with the caching, which is that CacheTrait is implemented in both FetchAuthTokenCache and ScopedAccessTokenMiddleware. This is a little wonky. I am hoping this isn't an actual issue as long as nobody's really using ScopedAccessTokenMiddleware.

@judgej
Copy link
Author

judgej commented Sep 28, 2017

On point 2, the initialisations, I meant the authentication client. The "library" was a little too broad.

$client = new Google_Client();
// set scope, redirect URL, access type, approval prompt etc.
// Set the current access token:
$client->setAccessToken($json_token);

The $json_token looks something like this:

{
    "access_token":"ya29.Gl3UBA86bj2DW...very-long...yPLmq1vpffV4",
    "token_type":"Bearer",
    "expires_in":3480,
    "refresh_token":"1\/VRdVb3...Ixs",
    "created":1506594548
}

(The expires_in is 30 minutes minus a 5 minute wiggle-time my application subtracts on first being authorised, hence 3480 and not 3600. When the token is renewed, in theory Google could decide the new token could last longer or for less time, but that never gets captured and passed back - the 30 minutes is hard-coded into the library is not something that the remote API provides.)

The current access token, its expiry and the refresh token are all in there. These details were stored in the application and are all that is needed to access Google services for an authorised user. These are the "initialisation details" that I referred to.

Then here also the $client->setTokenCallback() is created to save any changes to the access token when the library renews it (when it checks the timestamp and deems it to have expired, but not when it tries to use it and the remote end says, "hey, it's expired"). I do that here:

https://github.com/academe/laravel-google-api-auth/blob/master/src/Helper.php#L56-L83

$auth in the above code is a Laravel model, a persistent storage record for an authorisation.

I've not got a full grasp of how "middleware" is defined in this library, but if it is a layer to invisibly catch certain states and act on that, it is probably what we would be looking for. I think the point is that it would need to be caught right where the required API request is sent to Google, so that once the access token is renewed, it has all the information is needs to re-send the same request. So I guess middleware that handles this http message and its response would be the place? But I'm a little out of my depth at this point on implementation, but your suggestions sound good.

I'll explore the middleware a little deeper to try to understand it more.

@JustinBeckwith JustinBeckwith added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. 🚨 This issue needs some love. labels Jun 1, 2018
@JustinBeckwith JustinBeckwith removed the 🚨 This issue needs some love. label Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

3 participants