`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.