Hello!

During our usage of the module, we encountered that the authentication token needs to be refreshed after some time.

I had a look at the Salesforce Drupal Module (4.0) and thought on using the implemented REST Client (salesforce/src/Rest/RestClient.php:194) since it already wraps a Guzzle Client with said functionality:

try {
      $this->response = new RestResponse($this->apiHttpRequest($url, $params, $method));
    }
    catch (RequestException $e) {
      // RequestException gets thrown for any response status but 2XX.
      $this->response = $e->getResponse();

      // Any exceptions besides 401 get bubbled up.
      if (!$this->response || $this->response->getStatusCode() != 401) {
        throw new RestException($this->response, $e->getMessage(), $e->getCode(), $e);
      }
    }

    if ($this->response->getStatusCode() == 401) {
      // The session ID or OAuth token used has expired or is invalid: refresh
      // token. If refresh_token() throws an exception, or if apiHttpRequest()
      // throws anything but a RequestException, let it bubble up.
      $this->authToken = $this->authManager->refreshToken();
      try {
        $this->response = new RestResponse($this->apiHttpRequest($url, $params, $method));
      }
      catch (RequestException $e) {
        $this->response = $e->getResponse();
        throw new RestException($this->response, $e->getMessage(), $e->getCode(), $e);
      }
    }

But since the 'Authorization' HTTP header are hardcoded to use 'OAuth [Token]',

and FinDockForm (commerce_findock/src/PluginForm/OffsiteRedirect/FinDockForm.php:139)
uses 'Bearer [Token]' as value for the header, I decided to just reimplement catching the Guzzle ClientException, refresh the Auth Token, and try the POST again, instead of figuring out what the differences are and if it's compatible..

I also noticed the FinDockGateway doing a check to confirm the payment (commerce_findock/src/Plugin/Commerce/PaymentGateway/FinDockGateway.php:171) I decided against implementing a token refresh here, since it should still be valid at this point. I might be wrong though.

Any feedback is greatly appreciated.

CommentFileSizeAuthor
#10 refresh_auth_revised-3151752-10.patch2.09 KBvierlex
#8 refresh_plus_exceptions-3151753-8.patch2.21 KBvierlex
refresh_auth_token.patch1.77 KBvierlex

Comments

vierlex created an issue. See original summary.

vierlex’s picture

vierlex
he/him
commented
Issue summary: View changes
vierlex’s picture

vierlex
he/him
commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs work

Interesting, haven't had this problem, maybe the expiration of access tokens is configurable?

Makes sense, wondering if the salesforce module stores the expected expiration time somewhere, so we could already refresh it before trying, but possibly not.

we still need to include error handling for the secondary request though, as it is possible that this then fails for a different reason, especially since the code currently does not check for a specific error code or so, so it could simply be an invalid access token, in which case retrying would be bogus.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Yeah, without checking the code, i would hope that getAccessToken would know about how long a token is valid and transparently refresh it.

Maybe you have a system time problem and your time is off or zone information is wrong? Then sometimes you can end up with crazy such expiration issues with multi system authentication.

This pattern seems odd to me, since basically every post would need to implement a retry as well, if you can't trust the token. Maybe this is a SF module bug?

And yes again, hopefully wrong credentials don't lead to retry loops, but a single retry seems fine. Maybe we need to limit refreshs?
IMHO as this seems a workaround, if we really need it, we should log it so we can monitor and further investigate.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> This pattern seems odd to me, since basically every post would need to implement a retry as well

The pattern isn't that odd, that's pretty common with oauth2 refresh tokens, there's no guarantee it is still valid even if it's not expired yet. other things can result in it being expired too.

criz’s picture

criz
he/him
Location Vienna/Austria
commented

Interesting that you don't have the issue, maybe its a salesforce app configuration issue like mentioned here: https://www.drupal.org/project/salesforce/issues/2631122. We will check this.

Anyhow, re-authentication during checkout should be only the last resort imho, as this could take some time and the user has to wait until she/he can complete the checkout. Not something you want to have.

So if its really an issue, maybe a regular authentication check on cron-run would be nice (and of course logging any authentication errors).

vierlex’s picture

vierlex
he/him
commented
StatusFileSize
new refresh_plus_exceptions-3151753-8.patch2.21 KB

we still need to include error handling for the secondary request though, as it is possible that this then fails for a different reason, especially since the code currently does not check for a specific error code

I will wrap line 157 in a try/catch then and throw it up:

try {
  $response = $this->httpClient->post($configuration['url'], $params);
}catch (ClientException $e) {
  throw $e;
}

But I do check for a specific status code on line 147 already:
if ($e->getCode() === 401) {
401 happens when the session is run out or the user is not authorized.
So I try refreshing the the token and try again once more, since I dont have any while loops in play, endless loops won't happen.

Concerning the life time of the token:
When I get the token via the salesforce auth plugin manager, I get a instance of the object StdOAuth2Token with a the 'endOfLife' member variable set to -9002, but as Berdir said, tokens can be invalidated by other actions too.
Looking at the Drupal Salesforce Module I found this inside the member function parseAccessTokenResponse()
www/vendor/lusitanian/oauth/src/OAuth/OAuth2/Service/Salesforce.php:72

        // Salesforce tokens evidently never expire...
        $token->setEndOfLife(StdOAuth2Token::EOL_NEVER_EXPIRES);

I debugged said function too, to see if salesforce returns any kind of info about the lifetime of the token but it doesn't. So they don't expire with time.

Also what I added in the updated patch:
During my first time setting up module, I remember getting a 'on null' error for not having set up any Payment Gateway yet:
https://git.drupalcode.org/project/commerce_findock/-/blob/1.0.x/src/Plu...
But also when there is no token to be got, for whatever reason (it is possible to revoke tokens in the drupal UI)
https://git.drupalcode.org/project/commerce_findock/-/blob/1.0.x/src/Plu...
So instead of php 'on null' error, a Exception will be thrown.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

401 just means access denied. You would get the same error code if you submit a completely wrong token, not just an expired one. I would assume the json response data contains a more specific error message/code that we could check. But looking at https://docs.findock.com/knowledge-base/troubleshooting-payment-api/, I might be wrong, it says wrong or expired in the same message, so we probably can't be more specific. 

+++ b/src/PluginForm/OffsiteRedirect/FinDockForm.php
@@ -129,18 +129,49 @@ class FinDockForm extends PaymentOffsiteForm implements ContainerInjectionInterf
+          try {
+            $response = $this->httpClient->post($configuration['url'], $params);
+          }
+          catch (ClientException $e){
+            throw $e;
+          }

I misread the initial patch, we're still within the outer try/catch, so no need to catch and rethrow the error, that doesn't add anything then (if anything, we would need to handle it properly, but we can skip that here).

vierlex’s picture

vierlex
he/him
commented
StatusFileSize
new refresh_auth_revised-3151752-10.patch2.09 KB

Revised patch, based of the latest dev commit.

  • Berdir committed 90bdcfe on 1.0.x authored by vierlex 
    Issue #3151753 by vierlex: Refresh Authentication Token when...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Fixed

Thanks. Committed this.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.