Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ResponseParsingException and throw this exception in … #825

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
46 changes: 36 additions & 10 deletions src/Provider/AbstractProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use League\OAuth2\Client\OptionProvider\OptionProviderInterface;
use League\OAuth2\Client\OptionProvider\PostAuthOptionProvider;
use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
use League\OAuth2\Client\Provider\Exception\ResponseParsingException;
use League\OAuth2\Client\Token\AccessToken;
use League\OAuth2\Client\Token\AccessTokenInterface;
use League\OAuth2\Client\Tool\ArrayAccessorTrait;
Expand Down Expand Up @@ -98,6 +99,15 @@ abstract class AbstractProvider
*/
protected $optionProvider;

/**
* If a response body cannot be parsed, a value true of this flag will allow
* the response parser to throw a ResponseParsingException containing
* the response and the body.
*
* @var bool
*/
protected $mayThrowResponseParsingException = false;
tandhika marked this conversation as resolved.
Show resolved Hide resolved

/**
* Constructs an OAuth 2.0 service provider.
*
Expand Down Expand Up @@ -520,6 +530,8 @@ protected function getAccessTokenRequest(array $params)
* @param mixed $grant
* @param array $options
* @throws IdentityProviderException
* @throws ResponseParsingException if the flag $mayThrowResponseParsingException is true and
* response body cannot be parsed.
* @return AccessTokenInterface
*/
public function getAccessToken($grant, array $options = [])
Expand Down Expand Up @@ -613,6 +625,8 @@ public function getResponse(RequestInterface $request)
*
* @param RequestInterface $request
* @throws IdentityProviderException
* @throws ResponseParsingException if the flag $mayThrowResponseParsingException is true and
* response body cannot be parsed.
* @return mixed
*/
public function getParsedResponse(RequestInterface $request)
Expand Down Expand Up @@ -666,8 +680,10 @@ protected function getContentType(ResponseInterface $response)
* 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
* @return array|string
*/
protected function parseResponse(ResponseInterface $response)
{
Expand All @@ -686,18 +702,26 @@ protected function parseResponse(ResponseInterface $response)
return $this->parseJson($content);
} catch (UnexpectedValueException $e) {
if (strpos($type, 'json') !== false) {
throw $e;
throw $this->mayThrowResponseParsingException
? new ResponseParsingException($response, $content, $e->getMessage(), $e->getCode())
tandhika marked this conversation as resolved.
Show resolved Hide resolved
: $e;
}

if ($response->getStatusCode() == 500) {
throw new UnexpectedValueException(
'An OAuth server error was encountered that did not contain a JSON body',
0,
$e
);
// for any other content types
if ($this->mayThrowResponseParsingException) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we always throw the exception, no matter what?

I know this suggestion is somewhat of a BC break, but if you have ResponseParsingException extend UnexpectedValueException, then anyone catching that exception will start catching the new exceptions and can decide what to do about them.

@shadowhand, @stevenmaguire, thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your suggestion. I can implement the changes if no objection coming from @shadowhand and @stevenmaguire

// 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;
}

return $content;
}
}

Expand Down Expand Up @@ -774,6 +798,8 @@ public function getResourceOwner(AccessToken $token)
*
* @param AccessToken $token
* @return mixed
* @throws ResponseParsingException if the flag $mayThrowResponseParsingException is true and
* response body cannot be parsed.
*/
protected function fetchResourceOwnerDetails(AccessToken $token)
{
Expand Down
76 changes: 76 additions & 0 deletions src/Provider/Exception/ResponseParsingException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php
/**
* This file is part of the league/oauth2-client library
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @copyright Copyright (c) Alex Bilbie <[email protected]>
* @license http://opensource.org/licenses/MIT MIT
* @link http://thephpleague.com/oauth2-client/ Documentation
* @link https://packagist.org/packages/league/oauth2-client Packagist
* @link https://github.com/thephpleague/oauth2-client GitHub
*/

namespace League\OAuth2\Client\Provider\Exception;

use Exception;
use Psr\Http\Message\ResponseInterface;

/**
* Exception thrown if the parser cannot parse the provider response.
*/
class ResponseParsingException extends Exception
tandhika marked this conversation as resolved.
Show resolved Hide resolved
{
/**
* @var ResponseInterface
*/
protected $response;

/**
* @var string
*/
protected $responseBody;

/**
* @param ResponseInterface $response The response
* @param string $responseBody The response body
* @param null $message
* @param int $code
*/
public function __construct(
ResponseInterface $response,
$responseBody,
$message = null,
$code = 0
tandhika marked this conversation as resolved.
Show resolved Hide resolved
) {
$this->response = $response;
$this->responseBody = $responseBody;

if (null === $message) {
$message = sprintf('Cannot parse response body: "%s"', $responseBody);
}

parent::__construct($message, $code);
tandhika marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Returns the exception's response.
*
* @return ResponseInterface
*/
public function getResponse()
{
return $this->response;
}

/**
* Returns the exception's response body.
*
* @return string
*/
public function getResponseBody()
{
return $this->responseBody;
}
}
37 changes: 35 additions & 2 deletions test/src/Provider/AbstractProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace League\OAuth2\Client\Test\Provider;

use League\OAuth2\Client\Provider\Exception\ResponseParsingException;
use League\OAuth2\Client\OptionProvider\PostAuthOptionProvider;
use Mockery;
use ReflectionClass;
Expand Down Expand Up @@ -616,7 +617,7 @@ public function parseResponseProvider()
/**
* @dataProvider parseResponseProvider
*/
public function testParseResponse($body, $type, $parsed, $statusCode = 200)
public function testParseResponse($body, $type, $parsed, $statusCode = 200, $provider = null)
{
$stream = Mockery::mock(StreamInterface::class, [
'__toString' => $body,
Expand All @@ -632,7 +633,12 @@ public function testParseResponse($body, $type, $parsed, $statusCode = 200)
->andReturn($type);

$method = $this->getMethod(AbstractProvider::class, 'parseResponse');
$result = $method->invoke($this->getMockProvider(), $response);

if (null === $provider) {
$provider = $this->getMockProvider();
}

$result = $method->invoke($provider, $response);

$this->assertEquals($parsed, $result);
}
Expand All @@ -649,6 +655,33 @@ public function testParseResponseNonJsonFailure()
$this->testParseResponse('<xml></xml>', 'application/xml', null, 500);
}

public function responseParsingExceptionProvider()
{
return [
['application/json'],
['']
];
}

/**
* @dataProvider responseParsingExceptionProvider
*/
public function testResponseParsingException($type)
{
$provider = $this->getMockProvider();
$provider->allowResponseParsingException();
$exception = null;
try {
$this->testParseResponse('{13}', $type, null, 401, $provider);
} catch (ResponseParsingException $exception) {
}
$this->assertInstanceOf(ResponseParsingException::class, $exception);
$response = $exception->getResponse();
$this->assertSame(401, $response->getStatusCode());
$this->assertSame('{13}', $exception->getResponseBody());
$provider->disallowResponseParsingException();
}

public function getAppendQueryProvider()
{
return [
Expand Down
39 changes: 39 additions & 0 deletions test/src/Provider/Exception/ResponseParsingExceptionTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php

namespace League\OAuth2\Client\Test\Provider\Exception;

use GuzzleHttp\Psr7\Response;
use League\OAuth2\Client\Provider\Exception\ResponseParsingException;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\ResponseInterface;

class ResponseParsingExceptionTest extends TestCase
{
private $body = 'foo';

protected function generateResponseParsingException()
{
return new ResponseParsingException(new Response('401'), $this->body);
}

public function testGetResponse()
{
$this->assertInstanceOf(
ResponseInterface::class,
$this->generateResponseParsingException()->getResponse()
);
}

public function testGetResponseBody()
{
$this->assertSame(
$this->body,
$this->generateResponseParsingException()->getResponseBody()
);
}

public function testMissingMessage()
{
$this->assertNotEmpty($this->generateResponseParsingException()->getMessage());
}
}
10 changes: 10 additions & 0 deletions test/src/Provider/Fake.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@ public function getAccessTokenMethod()
return $this->accessTokenMethod;
}

public function allowResponseParsingException()
{
$this->mayThrowResponseParsingException = true;
}

public function disallowResponseParsingException()
{
$this->mayThrowResponseParsingException = false;
}

protected function createResourceOwner(array $response, AccessToken $token)
{
return new Fake\User($response);
Expand Down