nascom / teamleaderapiclient Goto Github PK

Hi Nascom team,

thank you for open-sourcing this client!

I am using the latest v2 git 3911595 .

The situation:
Since Teamleader has a short access token lifetime, it might expire in the middle of a sync process.

The current implementation:
\Nascom\TeamleaderApiClient\Http\Guzzle\Middleware\RefreshTokenMiddleware::__invoke() attempts to deal with such situations by checking the expiry date of the locally stored access token before sending the request. If at that time the token has expired - by the low precision DateTime available - a new access token is requested by means of the provided refresh token.

The problem:
However, despite NTP-synced clock, I ran multiple times into HTTP 401 responses, like here:

   Http\Client\Exception\HttpException  : Client error: `GET https://api.teamleader.eu/contacts.info` resulted in a `401 Unauthorized` response:
{"errors":[{"title":"Access token expired","status":401}]}                                                                                                                                   


  at /path/to/vendor/php-http/guzzle6-adapter/src/Promise.php:128
    124| 
    125|         if ($exception instanceof GuzzleExceptions\RequestException) {
    126|             // Make sure we have a response for the HttpException
    127|             if ($exception->hasResponse()) {
  > 128|                 return new HttplugException\HttpException(
    129|                     $exception->getMessage(),
    130|                     $exception->getRequest(),
    131|                     $exception->getResponse(),
    132|                     $exception

  Exception trace:

  1   Http\Adapter\Guzzle6\Promise::handleException(Object(GuzzleHttp\Exception\ClientException), Object(GuzzleHttp\Psr7\Request))
     /path/to/vendor/php-http/guzzle6-adapter/src/Promise.php:64

  2   GuzzleHttp\Exception\ClientException::("Client error: `GET https://api.teamleader.eu/contacts.info` resulted in a `401 Unauthorized` response:
{"errors":[{"title":"Access token expired","status":401}]}                                                                                                                                   
")                                                                                                                                                                                           
      /path/to/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php:113

  Please use the argument -v to see more details.

(The stack traces do not show it, but I was using the Nascom Teamleader Api Client stack for these requests.)

Even re-starting the sync manually in the command line (inspecting the error and re-starting the command takes a few seconds) was not enough to pass enough time to cross the database-stored key expiry DateTime. Maybe the Teamleader API server's clock is off - I don't know.

In any case, this can easily happen when requesting e.g. contact.info responses for the entire contacts.list in quick succession, since the current implementation would require the two clocks (server and client side) to be synchronized perfectly and for the retrieval and storage of the access code to be instant!

Possible workaround:
A workaround I will implement for now will be to manually adjust the access token expiration time by a negative delta before storing it. However, I still wanted to report, so the implementation can be improved.

My suggestions for an improved implementation:

• By default, refresh not when the expiry time as passed, but already minutes before that.
• Catch HTTP 401 Unauthorized responses. Due to the clear error title ({"errors":[{"title":"Access token expired","status":401}]}) this case could easily be distinguished from other cases of Unauthorized responses (e.g. when not an expired but an entirely invalid access token was provided).

I am curious what you think about these suggestions.

Kind regards!

On the v2-refactor branch: When adding a contact, the model requires a DateTime object. However, it is not properly converted to the format Teamleader expects.

{"errors":[{"code":0,"title":"birthdate must be a valid date. Sample format: "2005-12-30"","status":400,"meta":{"field (truncated...)

I'm using the v2-refactor branch, to integrate Teamleader with a Drupal 8 site.

When adding a new draft invoice, I'm getting the following error:

Client error: `GET https://api.teamleader.eu/invoices.draft` resulted in a `405 Method Not Allowed` response: @"errors":[{"code":405000,"title":"Method GET not allowed. Allowed method(s): POST","status":405]} 

When debugging, it seems the $method property of the \Nascom\TeamleaderApiClient\Request\Invoicing\Invoices\InvoicesDraftRequest class is set to NULL.
It should use POST from its PostRequest class inheritance, but it seems the usage of MultipleMethodsTrait overrides this.

Since the method is NULL, it gets set to GET by default.

See the screenshot of my debugger, in \League\OAuth2\Client\Provider\AbstractProvider::createRequest():

Interestingly, when trying to add new contact, I also did debugging to see the difference.
There the same problem occurs, and the method used is also GET.

But it seems in the latter case the Teamleader API does not reject the call, but in the former it does.

Do we need to call setMethod() explicitly in Request classes where a POST is desired?
Or what would be the appropriate way to handle this?

It seems more like a Teamleader API bug, but nevertheless this is something fixable in this very, very helpful library:

When sending a refresh token request, it only succeeds if I remove the &redirect_uri=... parameter from the x-www-form-urlencoded request body.
If the redirect uri is sent to the API, then the /oauth2/authorize endpoint responds with a 401 Unauthorized.

The API documentation does not include redirect_uri as required parameter: https://developer.teamleader.eu/#/introduction/authentication/using-refresh-tokens
But it also does not mention that including it would do any harm.

A somewhat ugly workaround that does it for me is extending \League\OAuth2\Client\Provider\AbstractProvider (which I had done anyway to auto-fill credentials from .env config) and overriding the getAccessToken() method:

        $grant = $this->verifyGrant($grant);

        $params = [
            'client_id'     => $this->clientId,
            'client_secret' => $this->clientSecret,
        ];

        // Workaround for https://github.com/Nascom/TeamleaderApiClient/issues/15
        // The Teamleader API responds with `401 Unauthorized` if the
        // `redirect_uri` is provided at refresh token requests.
        if (!$grant instanceof RefreshToken) {
            $params['redirect_uri'] = $this->redirectUri;
        }

        // ...

I think this problem that occurs now has to do with a Teamleader API update, of feb 2022, but there is no notice of it in that docs.

Since last week I got responses like this when doing a GET:

Client error: `GET https://api.focus.teamleader.eu/companies.info` resulted in a `400 Bad Request` response:
{"errors":[{"code":0,"title":"id must be present","status":400,"meta":{"field":"id"}}]}

Fix -> Adding this:

        if ($psrRequest->getMethod() == 'GET') {
            $psrRequest = $psrRequest->withUri($psrRequest->getUri()->withQuery(http_build_query($body)));
        }

Here https://github.com/Nascom/TeamleaderApiClient/blob/v2/src/Http/ApiClient/ApiClient.php#L110

Do you think that's a good solution?

https://github.com/swaggest/php-code-builder could for example elp if we could embed the json schemas in here. That does raise the pressure on Teamleader to be accurate with their schemas, but could help in later versioning and overall making it easier to not create getters & setters.
It would allow us to generate the object classes everytime Teamleader makes a certain change of bugfix to their schema's. The IDE's won't notice as the generated classes are actual classes.

Curious for ideas/suggestions here.

Classes are generated from the JSON Schemas, and allow something like the following. The generated class can be seen here: https://github.com/swaggest/php-code-builder/blob/master/tests/src/Tmp/Example/User.php

User class:
// Importing raw data to entity class instance will do validation and mapping
$user = User::import(
json_decode('{"name":"John","info":{"lastName":"Doe","birthDate":"1980-01-01","options":{"rememberSession":true,"allowNotifications":false}}}')
);

var_dump($user->info->options->allowNotifications); // bool(false)

I got: nascom/teamleader-api-client v2.x-dev requires php-http/client-implementation ^1.0|^2.0 -> no matching package found.