AbstractProvider::getParsedResponse should always return array or throw exception

[ksimka]

Uncaught Exception "TypeError" with message: "Argument 1 passed to League\OAuth2\Client\Provider\AbstractProvider::prepareAccessTokenResponse() must be of the type array, string given

It happens because AbstractProvider::getParsedResponse returns string when content can't be parsed.

1. https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L528

// AbstractProvider::getAccessToken

// ...
$response = $this->getParsedResponse($request);
$prepared = $this->prepareAccessTokenResponse($response);
// ...

2. https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L597

/**
     * Sends a request and returns the parsed response.
     *
     * @param  RequestInterface $request
     * @return mixed
     */
    public function getParsedResponse(RequestInterface $request)
    {
        try {
            $response = $this->getResponse($request);
        } catch (BadResponseException $e) {
            $response = $e->getResponse();
        }
        $parsed = $this->parseResponse($response);
        $this->checkResponse($response, $parsed);
        return $parsed;
    }

Attention to docs — "return mixed". It can not return "mixed" (though actually it is), because the result goes to prepareAccessTokenResponse which receives array only.

3. https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L700

/**
     * Prepares an parsed access token response for a grant.
     *
     * Custom mapping of expiration, etc should be done here. Always call the
     * parent method when overloading this method.
     *
     * @param  mixed $result
     * @return array
     */
    protected function prepareAccessTokenResponse(array $result)

Docs say param mixed $result (was changed here 365e61c with no visible reason), but typehint says array $result.

And the last thing — where mixed comes from?

4. https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L657

/**
     * Parses the response according to its content-type header.
     *
     * @throws UnexpectedValueException
     * @param  ResponseInterface $response
     * @return array
     */
    protected function parseResponse(ResponseInterface $response)
    {
        $content = (string) $response->getBody();
        $type = $this->getContentType($response);
        if (strpos($type, 'urlencoded') !== false) {
            parse_str($content, $parsed);
            return $parsed;
        }
        // Attempt to parse the string as JSON regardless of content type,
        // since some providers use non-standard content types. Only throw an
        // exception if the JSON could not be parsed when it was expected to.
        try {
            return $this->parseJson($content);
        } catch (UnexpectedValueException $e) {
            if (strpos($type, 'json') !== false) {
                throw $e;
            }
            return $content;
        }
    }

return array — one more lie here. Look at the catch block: value is considered unexpected only if type was "expected" — wut? Otherwise $content is returned, which is string. This was called fallback on failure.

So, long story short

• provider returns some wierd response
• oauth2-client tries to parse it as json and fails to
• then the parse method returns a string (raw response body) instead of an exception or at least promised array
• string goes to another method with array typehint
• ???
• TypeError (PHP 7)

I haven't done PR because I don't know what's the right solution in this situation. We should respect BC, I understand. But I'd just throw an exception despite any other condition: we tried to do something and failed — there is nothing we can do with it later.

[ksimka]

Hey, any thoughts on this?

[paul-court]

Failing for me too as a knock on effect of upgrading guzzle.

[shadowhand]

Was this fixed by #621?

[ksimka]

@shadowhand Technically - yes. But logically it'd be better to throw an exception despite of http response code. With this code we still can not guarantee $content is array.

Anyway, it's better than nothing.

[shadowhand]

I'm open to additional PR(s) to improve the behavior.

[dannyverp]

SOLVED --- I'm running into the same problem. Strangely enough I've got two clients running. One in a plugin in a MODX environment which seems to be working perfectly fine. The other one is in a Magento environment and bothers me about the string given in stead of string. They are both hooked up to the same Laravel Passport OAuth 2 server.

			'clientId'                => $this->helper->getConfigValue('authwiseoauth/oauth/client_id'),    // The client ID assigned to you by the provider
			'clientSecret'            => $this->helper->getConfigValue('authwiseoauth/oauth/client_secret'),   // The client password assigned to you by the provider
			'redirectUri'             => $this->helper->getConfigValue('authwiseoauth/oauth/redirect_url'),
			'urlAuthorize'            => $this->helper->getConfigValue('authwiseoauth/oauth/server_url').$this->helper->getConfigValue('authwiseoauth/oauth/oauth_path').'authorize',
			'urlAccessToken'          => $this->helper->getConfigValue('authwiseoauth/oauth/server_url').$this->helper->getConfigValue('authwiseoauth/oauth/oauth_path').'token',
			'urlResourceOwnerDetails' => $this->helper->getConfigValue('authwiseoauth/oauth/server_url').$this->helper->getConfigValue('authwiseoauth/oauth/oauth_path').'resource'
		]);

		// If we don't have an authorization code then get one
		if (!isset($_GET['code'])) {

			// Fetch the authorization URL from the provider; this returns the
			// urlAuthorize option and generates and applies any necessary parameters
			// (e.g. state).
			$authorizationUrl = $provider->getAuthorizationUrl();

			// Get the state generated for you and store it to the session.
			$_SESSION['oauth2state'] = $provider->getState();

			// Redirect the user to the authorization URL.
			header('Location: ' . $authorizationUrl);
			exit;

			// Check given state against previously stored one to mitigate CSRF attack
		} elseif (empty($_GET['state']) || (isset($_SESSION['oauth2state']) && $_GET['state'] !== $_SESSION['oauth2state'])) {

			if (isset($_SESSION['oauth2state'])) {
				unset($_SESSION['oauth2state']);
			}

			exit('Invalid state');

		} else {

			try {

				// Try to get an access token using the authorization code grant.
				$accessToken = $provider->getAccessToken('authorization_code', [
					'code' => $_GET['code']
				]);

				// The provider provides a way to get an authenticated API request for
				// the service, using the access token; it returns an object conforming
				// to Psr\Http\Message\RequestInterface.
				$request = $provider->getAuthenticatedRequest(
					'GET',
					$this->helper->getConfigValue('authwiseoauth/oauth/server_url').$this->helper->getConfigValue('authwiseoauth/oauth/api_path').'user',
					$accessToken
				);

			} catch (\League\OAuth2\Client\Provider\Exception\IdentityProviderException $e) {

				echo 'Test';
				// Failed to get the access token or user details.
				exit($e->getMessage());
			}

		}

Whenever I try to access the page I'm redirected to the login screen. Then I log in and I click authorize when prompted for authorisation. When I'm redirected back again I get the before mentioned error.

As far as I can check the config values seem to check out. The response object I get back from getParsed Response() is null though. I can't seem to find what my server is outputting though. Any suggestions as to how I can go about fixing this?

EDIT: My server is giving out auth codes. Just not access tokens.

EDIT 2: Client error: POST http://auth.dev/oauth/token resulted in a 404 Not Found response. That might explain part of my problem. Though the strange thing is that I believe that I also use the same URL on my other install. I'm seriously confused >.<

Actually managed to solve the problem. Turns out that I was trying to approach one virtual machine from another. That won't work and it'll return a 404 indeed :) Shouldn't be doing this kinda stuff on a friday afternoon. :D

[steverhoades]

A very similar problem occurs with the getResourceOwner API. If the response from the OAUTH2 server does not include a JSON response the library will throw a TypeError Exception.

[2017-06-16 18:20:03] local.ERROR: Symfony\Component\Debug\Exception\FatalThrowableError: Type error: Argument 1 passed to League\OAuth2\Client\Provider\GenericProvider::createResourceOwner() must be of the type array, string given

[steverhoades]

I've added #636 which fixes the issue I described above.

[SeinopSys]

Personally I'm not in favor of how this was handled in #636. This error can and will also happen (just did to me) if the OAuth provider goes down and returns 501-522 HTTP codes, invalid body or not. The check in AbstractProvider.php only handles 500 responses, and the UnexpectedValueException is hard to debug (no access to response code/body).

Something along the lines of this would be a lot more developer friendly (it's a snippet from a class I'm writing to support a provider)

/**
 * Temporarily stores the last request in case there's an error
 *
 * @var RequestInterface
 */
private $_lastRequest;

/**
 * Sends a request instance and returns a response instance.
 * WARNING: This method does not attempt to catch exceptions caused by HTTP
 * errors! It is recommended to wrap this method in a try/catch block.
 *
 * @param  RequestInterface $request
 *
 * @return ResponseInterface
 */
public function getResponse(RequestInterface $request){
	$this->_lastRequest = $request;
	return parent::getResponse($request);
}

/**
 * Parses the response according to its content-type header.
 *
 * @param  ResponseInterface $response
 *
 * @return array
 * @throws BadResponseException
 * @throws UnexpectedValueException
 */
protected function parseResponse(ResponseInterface $response){
	if ($response->getStatusCode() > 500){
		throw new BadResponseException(
			'The OAuth server returned an unexpected response',
			$this->_lastRequest,
			$response
		);
	}

	return parent::parseResponse($response);
}

I'd be much more happier to see this implemented properly in the library, because it gives direct access to the response object to anything higher up that tries to catch it, opening up more possibilities for debugging purposes.

I would submit a PR but I'm not sure how to best integrate storing and getting the extra variable required for the instantiation of BadResponseException, this is just a hack I threw in to mitigate the issue.

[shadowhand]

The correct solution would be for UnexpectedValueException to set the $previous attribute on the exception, so that you could do:

try {
    $response = $provider->getResponse($request);
} catch (UnexpectedValueException $e) {
    $request = $e->getPrevious()->getRequest();
    $response = $e->getPrevious()->getResponse();
}

This would eliminate the need to have $this->_last_request entirely.

[PeterDKC]

I've similarly extended and overridden my Provider locally in my application in order to handle HTTP 204 Responses which by design return a blank Response Body and throw the UnexpectedValueException because json_decode() can't handle empty strings with the following snippet:

        if (empty($content) && $response->getStatusCode() === 204) {
            return $content;
        }

Full overridden method:

/**
     * Parses the response according to its content-type header.
     *
     * @throws UnexpectedValueException
     * @param  ResponseInterface $response
     * @return array
     */
    protected function parseResponse(ResponseInterface $response)
    {
        $content = (string) $response->getBody();

        if (empty($content) && $response->getStatusCode() === 204) {
            return $content;
        }

        $type = $this->getContentType($response);

        if (strpos($type, 'urlencoded') !== false) {
            parse_str($content, $parsed);
            return $parsed;
        }

        // Attempt to parse the string as JSON regardless of content type,
        // since some providers use non-standard content types. Only throw an
        // exception if the JSON could not be parsed when it was expected to.
        try {
            return $this->parseJson($content);
        } catch (UnexpectedValueException $e) {
            if (strpos($type, 'json') !== false) {
                throw $e;
            }

            return $content;
        }
    }

Not super sexy, but it works.

[ramsey]

I've tagged release 2.3.0, which includes the changes from #636. It sounds like those changes might have addressed some of the issues, but @SeinopSys, @shadowhand, and @PeterDKC had some follow-up thoughts. What's remaining to close out this issue?

[SeinopSys]

My main concern still remains: The UnexpectedValueException provides no way to see the contents of the response for debugging purposes. All you can see is that a request failed, and any content the provider might have returned in a non-JSON format (e.g. an HTML error page) is swallowed by the library. I'd like to see this addressed, personally.

[cweiske]

Just for info, I saw this when the oauth server was equipped with http basic auth.

[maticb]

I am getting the same error, I have made my own OAuth2 server, using /bshaffer/oauth2-server-php, and this is the response I get if I echo the response on line 530 (in the AbstractProvider::getAccessToken() function):

HTTP/1.1 200 OK Cache-Control: no-store Content-Type: application/json Pragma: no-cache {"access_token":"XXX","expires_in":3600,"token_type":"Bearer","scope":null,"refresh_token":"XXX"}

This does not get parsed properly. AFAIK this is exactly how an OAuth2 response should look like?

My ugly quick fix for now is to add this to the parseResponse function

preg_match("/{.*}/",$content,$arr);
$content = $arr[0];

where $arr[0] givesme exactly what I need, the JSON content only.

[maddy2101]

Same here, too. Version 2.3.0 on PHP 7.2. The OAuth Server on the other end sends a error string, which goes into prepareAccessTokenResponse without hesitation, which in turn just fails with the exception the thread owner stated in his very first sentence.

[tandhika]

As this issue is still open, I'd like to raise some concern and share my thought.
This is how it is currently implemented

public function getParsedResponse(RequestInterface $request)
{
    try {
        $response = $this->getResponse($request);
    } catch (BadResponseException $e) {
        $response = $e->getResponse();
    }

    $parsed = $this->parseResponse($response);

    $this->checkResponse($response, $parsed);

    return $parsed;
}

I assume the intended task of parseResponse() is to correctly parse the response body and return it. Currently, parseResponse is able to parse urlencoded or valid json body. In general, the result of a json parsing does not need to be an array. It could even be null, empty string, integer, etc. So the value of $parsed can be of different type, which I don't really care much. It is up to the calling function to correctly handle that value.

/**
 * Parses the response according to its content-type header.
 *
 * @throws UnexpectedValueException
 * @param  ResponseInterface $response
 * @return array
 */
protected function parseResponse(ResponseInterface $response)
{
    $content = (string) $response->getBody();
    $type = $this->getContentType($response);

    if (strpos($type, 'urlencoded') !== false) {
        parse_str($content, $parsed);
        return $parsed;
    }

    // Attempt to parse the string as JSON regardless of content type,
    // since some providers use non-standard content types. Only throw an
    // exception if the JSON could not be parsed when it was expected to.
    try {
        return $this->parseJson($content);
    } catch (UnexpectedValueException $e) {
        if (strpos($type, 'json') !== false) {
            throw $e;
        }

        if ($response->getStatusCode() == 500) {
            throw new UnexpectedValueException(
                'An OAuth server error was encountered that did not contain a JSON body',
                0,
                $e
            );
        }

        return $content;
    }
}

The @return array type hint is surely wrong.

The problem with current implementation of parseResponse() is that it throws an exception which does not help much in the calling function if the response body cannot be parsed as JSON. This includes body having empty string as value.
Thus, I'd suggest to throw a new exception type called ResponseParsingException which encapsulates the response and its body, so that the calling function can reacts accordingly by retrieving the response and/or the body from the catched exception.
To ensure BC we can define a flag in AbstractProvider, e.g.

protected $mayThrowResponseParsingException = false;

which can be used to control the behavior of parseResponse().

 /**
 * Parses the response according to its content-type header.
 *
 * @throws UnexpectedValueException
 * @throws ResponseParsingException if the flag $mayThrowResponseParsingException is true and
 *                                  response body cannot be parsed.
 * @param  ResponseInterface $response
 * @return array|string
 */
protected function parseResponse(ResponseInterface $response)
{
    $content = (string) $response->getBody();
    $type = $this->getContentType($response);

    if (strpos($type, 'urlencoded') !== false) {
        parse_str($content, $parsed);
        return $parsed;
    }

    // Attempt to parse the string as JSON regardless of content type,
    // since some providers use non-standard content types. Only throw an
    // exception if the JSON could not be parsed when it was expected to.
    try {
        return $this->parseJson($content);
    } catch (UnexpectedValueException $e) {
        if (strpos($type, 'json') !== false) {
            throw $this->mayThrowResponseParsingException
                ? new ResponseParsingException($response, $content, $e->getMessage(), $e->getCode())
                : $e;
        }

        // for any other content types
        if ($this->mayThrowResponseParsingException) {
            // let the calling function decide what to do with the response and its body
            throw new ResponseParsingException($response, $content, '', 0);
        } else {
            if ($response->getStatusCode() == 500) {
                throw new UnexpectedValueException(
                    'An OAuth server error was encountered that did not contain a JSON body',
                    0,
                    $e
                );
            }

            return $content;
        }
    }
}

`
What do you think?
I know that the method checkResponse() can be (mis)used to re-parse the response later. But this is most probably not what this method is supposed to do. I assume that checkResponse should be used to check for error messages in the successfully parsed body returned by a well-behaved identity provider.

[tandhika]

I submitted PR #825 which implements that suggestion above

[Hanmac]

Imo it should have thrown an exception

currently i can't see what kind of errors appear