refresh token is stored with an expiry the same as the access token, making cronjobs a requirement to use this

[Langmans]

According to the docs, only the access token should respect the expires_in variable from the response. But when the access token expires using the default cache handler, the refresh token is also no longer available in the current logic.

Access tokens expire shortly (usually 1 hour) after they are issued for security reasons. If your integration needs to communicate with our API beyond the access token's lifespan, you will need to request a new access token using the refresh token which was issued with the access token. Note that refresh tokens can only be used once to get a new access token and refresh token.

However, if I look at Connection::storeTokens, I see the following:

    public function storeTokens(string $accessToken, string $refreshToken, int $expiresIn, int $expiresOn): void
    {
        $this->cacheHandler->set('accessToken', $accessToken, $expiresIn / 60);
        $this->cacheHandler->set('refreshToken', $refreshToken, $expiresIn / 60);
        $this->cacheHandler->set('tokenExpire', $expiresOn, $expiresIn / 60);
    }

The default cache handler doesn't respect the $expiresAt parameter and because of that I think tokenExpire is saved to the cache. I think controlling if a cache item is valid should be the responsibility of the cache handler, not the Connection.

Refresh tokens will continue functioning until the user revokes them or uninstalls your integration.

This means one can effectively use a very large expiry. Taking into accounts the definition of the cachehandler interface and the information about the refresh token, that should be a very long time, like a year or month.

I see a couple of things I would change:

1: Make the default cache handler also save a timestamp in set and let get check that timestamp to ensure the cache isn't expired yet. (so you save an array with the timestamp and the value) and then update the storeTokens method.

// default cache handler
    public function get($key)
    {
        $file = $this->getFileName($key);
        if(!file_exists($file)) {
          return null;
        }
        $handle = fopen($file, 'r');
        $fileSize = filesize($file);
        if ($fileSize === 0) {
            return null;
        }

        $data = $this->maybe_unserialize(fread($handle, $fileSize));
        if(!is_array($data) || $data['expires'] < time()) {
          $this->forget($key);
          return null;
        }
        return $data['value'];
    }

    public function set($key, $value, $expiresInMinutes)
    {
        $my_file = $this->getFileName($key);
        $handle  = fopen($my_file, 'w');
        fwrite($handle, $this->maybe_serialize([ 'value'=> $value, 'expires'=> time() + ( $expiresInMinutes * 60)));
    }

2: change the method to store the tokens.

    public function storeTokens(string $accessToken, string $refreshToken, int $expiresIn, int $expiresOn): void
    {
        $this->cacheHandler->set('accessToken', $accessToken, $expiresIn / 60);
        $this->cacheHandler->set('refreshToken', $refreshToken, 60 * 60 * 24 * 365); // a year.
    }

3: change the method to get the access tokens. as the cache handler controls if an item is still valid, You dont have to check for expiry so it can be simplified a lot.

  public method getAccessToken() : string {
    $accesstoken = $this->cache->get('accessToken');
    if($accesstoken)
    {
       return $accesstoken;
    }

    $refreshToken = $this->cache->get('refreshToken');
    if($refreshToken) {
        $this->acquireRefreshToken();

        return $this->getAccessToken());
    }
    return false;
  }

3) I'd also make some methods protected instead of private, so users can write their own custom implementation.

[janhenkes]

Hi @Langmans ,

Thanks for the suggestion, I have adopted your recommendations!