Skip to content

Restore request options when retrying after token refresh#365

Open
patricknomad wants to merge 1 commit into
omniphx:masterfrom
patricknomad:fix/refresh-retry-restores-request-options
Open

Restore request options when retrying after token refresh#365
patricknomad wants to merge 1 commit into
omniphx:masterfrom
patricknomad:fix/refresh-retry-restores-request-options

Conversation

@patricknomad

Copy link
Copy Markdown
Contributor

Problem

When a request returns a 401, the Client::request() calls refresh() and retries.

For some auth methods the authenticate() re-fetches the version and resource listings via nested request() calls. Those nested calls overwrite the shared $this->options.

The catch block restored $this->url but not $this->options:

public function request($url, $options)
{
    $this->url = $url;
    $this->options = array_replace_recursive($this->settings['defaults'], $options);

    try {
        return $this->handleRequest();
    } catch (TokenExpiredException $e) {
        $this->refresh();

        // $url restored, but not $options
        $this->url = $url;

        return $this->handleRequest();
    }
}

In our situation, this turned a composite POST into a GET /services/data/vXX.0/composite, which returns the composite resource directory rather than a compositeResponse.

Fix

Restore the original url and options before the retry:

$this->url = $url;
$this->options = array_replace_recursive($this->settings['defaults'], $options);

The assignment is unconditional: it fixes the re-authenticating methods and is a harmless no-op for the others.

Tests

  • ClientTest: an auth-agnostic contract test at the Client::request level, using a fixture whose refresh() clobbers $this->options via a nested request.
  • AuthenticationTest: an end-to-end test of an OAuthJWT refresh path (initial POST, 401, token, versions, resources, retry).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant