picqer / exact-php-client

PHP Client library for Exact Online
MIT License
165 stars 200 forks source link

Could not connect to Exact Error 401: {"error":"unauthorized_client","error_description":"Old refresh token used."} #644

Open fregoe opened 6 months ago

fregoe commented 6 months ago

Sometimes, without any reason, i receive this error: Could not connect to Exact Error 401: {"error":"unauthorized_client","error_description":"Old refresh token used."}

Then I need to manually clear the access, refresh en authorization code and request all new.

We use this packages for automatic sync so this is an issue because requesting new authorization codes requires browser interaction.

remkobrenters commented 6 months ago

This error is the result of an implementation error you made somewhere. Due to the way OAuth works, the only solution after encountering this error is to ask the user to give new consent so you get a new set of tokens that are valid again.

Please see https://github.com/picqer/exact-php-client/issues/640 (or some of the numerous other issues reported by people with refresh token issues).

martijn-coolminds commented 6 months ago

@fregoe @remkobrenters in ticket #606 I had this same issue. Debugging the code, it's most likely the cause of usage of curl_multi in the library. I was able to reproduce this error on all example code.

However, a found a way around it by properly implement token locking.

I've made a copy of the code that I wrote and cleaned it up. You can use this as an example, if you implement the persistent loading/saving/deleting of values.

<?php
class ExactAuthentication {
    private string $CLIENT_ID = "";
    private string $CLIENT_SECRET = "";
    protected string $REDIRECT_URI = "";
    protected string $DIVISION = "";

    // Populate with values from a db or file store
    // "authorizationcode"
    // "accesstoken"
    // "refreshtoken"
    // "expires_in"
    // "acquireaccesstokenlock"

    protected array $values = [];

    private ?\Picqer\Financials\Exact\Connection $connection = null;

    public function getExactClientId():string {
        return $this->CLIENT_ID;
    }

    public function getExactClientSecret():string {
        return $this->CLIENT_SECRET;
    }

    public function getExactRedirectUri():string {
        return $this->REDIRECT_URI;
    }

    public function getExactClientDivision():string {
        return $this->DIVISION;
    }

    public function __construct() {
        $this->CLIENT_ID = "xxyyzz";
        $this->CLIENT_SECRET = "aabbcc";
        $this->REDIRECT_URI = "https://www.example.com/callback";
        $this->DIVISION = 9999;
    }

    public function getConnection():?\Picqer\Financials\Exact\Connection {
        return $this->connection;
    }

    public function connect():void {
        $this->connection = new \Picqer\Financials\Exact\Connection();
        $this->connection->setDivision($this->DIVISION);
        $this->connection->setRedirectUrl($this->REDIRECT_URI);
        $this->connection->setExactClientId($this->CLIENT_ID);
        $this->connection->setExactClientSecret($this->CLIENT_SECRET);

        if ($this->getValue("authorizationcode")) {
            $this->connection->setAuthorizationCode($this->getValue("authorizationcode"));
        } else {
            $this->connection->redirectForAuthorization();
            return;
        }

        if ($this->getValue("accesstoken")) {
            $this->connection->setAccessToken($this->getValue("accesstoken"));
        }

        if ($this->getValue("refreshtoken")) {
            $this->connection->setRefreshToken($this->getValue("refreshtoken"));
        }

        if ($this->getValue("expires_in")) {
            $this->connection->setTokenExpires($this->getValue("expires_in"));
        }

        $this->connection->setTokenUpdateCallback(array($this, "tokenUpdateCallback"));
        $this->connection->setAcquireAccessTokenLockCallback(array($this, "setAcquireAccessTokenLock"));
        $this->connection->setAcquireAccessTokenUnlockCallback(array($this, "setAcquireAccessTokenUnlock"));

        try {
            $this->connection->connect();
        } catch (\Exception $e) {

            var_dump($exception);

            // Clear data
            $this->deleteValue("accesstoken");
            $this->deleteValue("refreshtoken");
            $this->deleteValue("expires_in");

            $this->connection->setAccessToken(null);
            $this->connection->setRefreshToken(null);
            $this->connection->connect();
        }
    }

    public function tokenUpdateCallback(Connection $connection) {
        $at = $connection->getAccessToken();
        $rt = $connection->getRefreshToken();
        $ei = $connection->getTokenExpires();

        $this->setValue("accesstoken", trim($at));
        $this->setValue("refreshtoken", trim($rt));
        $this->setValue("expires_in", trim($ei));
    }

    public function setAcquireAccessTokenLock(Connection $connection) {

        if ( $this->getValue("acquireaccesstokenlock")."" === "1" ) {
            throw new Exception("Exact access token is already locked");
            exit;
        }

        $this->setValue("acquireaccesstokenlock", 1);
    }

    public function setAcquireAccessTokenUnlock(Connection $connection) {
        $this->setValue("acquireaccesstokenlock", 0);
    }

    public function getValue( string $key) {
        // Implement loading from your store

        return isset( $this->values[ $key ] ) ? $this->values[ $key ] : null;
    }

    public function setValue(string $key, $value) {
        $this->values[$key] = $value;

        // Implement saving in your store
    }

    public function deleteValue(string $key)
    {
        unset($this->values[$key]);

        // Implement deleting from your store
    }
}
remkobrenters commented 6 months ago

The reason I am a bit strong about the wrong implementation is that we use this library (without any extra logic for this part) in hundreds of connections of which some of them are heavy users (premium users) with thousands of requests.

The problem (and the provided solution - thanks for chipping in) as I see it come down to concurrent requests to the same connection resulting in the token refreshes of the requests breaking each-other. This can be prevented by locking the tokens (like you suggest) or by making sure that requests for the same connection are queued so they are not performed at the same time.

The thing that I wonder: Are you facing this issue with user-facing logic where users perform actions that can be at the same time? As all our implementations are server-to-server and run in background processes where we have a lot of control over the queue and preventing overlapping requests.

martijn-coolminds commented 6 months ago

I had this problem appear already from day one using the library on a single request. What I figured (it's been a while since developing it), was that it happened in the library already when a result was iterating and needed to fetch more data as paging was required. The library takes care of this internally. Using a simple iterator nor switching to the filterAsGenerator functions didn't solve it.

Perhaps since I am able to reproduce it, I could look in to it some more to get to the bottom (since this question was asked before me, and will most likely come again).

remkobrenters commented 6 months ago

That would be awesome as it comes by every now and than. It might be caused by something that we do not use in our requests making me so sure that the implementation in base works well.

meijdenmedia commented 5 months ago

Just curious, why would you catch the Exception and run following code?

            $this->connection->setAccessToken(null);
            $this->connection->setRefreshToken(null);
            $this->connection->connect();
martijn-coolminds commented 5 months ago

@meijdenmedia it resets the internal data on the Connection class, which in the connect() function then restarts the authentication flow.

meijdenmedia commented 5 months ago

Yeah, but that means you've just lost the refresh token and you need to manually reconnect, right? How should this be handled in cronjobs f.e.? 🤔