From 4867539bd440630339210143a765610e33b6f479 Mon Sep 17 00:00:00 2001 From: Hasan Date: Fri, 13 Mar 2020 12:15:39 +0100 Subject: [PATCH 1/6] Introduce ResponseParsingException and throw this exception in AbstractProvider::parseResponse() if necessary --- src/Provider/AbstractProvider.php | 46 ++++++++--- .../Exception/ResponseParsingException.php | 76 +++++++++++++++++++ test/src/Provider/AbstractProviderTest.php | 16 ++++ .../Exception/ResponseParsingException.php | 37 +++++++++ test/src/Provider/Fake.php | 10 +++ 5 files changed, 175 insertions(+), 10 deletions(-) create mode 100644 src/Provider/Exception/ResponseParsingException.php create mode 100644 test/src/Provider/Exception/ResponseParsingException.php diff --git a/src/Provider/AbstractProvider.php b/src/Provider/AbstractProvider.php index d1679998..e3e441d4 100644 --- a/src/Provider/AbstractProvider.php +++ b/src/Provider/AbstractProvider.php @@ -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; @@ -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; + /** * Constructs an OAuth 2.0 service provider. * @@ -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 = []) @@ -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) @@ -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) { @@ -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()) + : $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) { + // 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; } } @@ -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) { diff --git a/src/Provider/Exception/ResponseParsingException.php b/src/Provider/Exception/ResponseParsingException.php new file mode 100644 index 00000000..3f8e02a8 --- /dev/null +++ b/src/Provider/Exception/ResponseParsingException.php @@ -0,0 +1,76 @@ + + * @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 +{ + /** + * @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 + ) { + $this->response = $response; + $this->responseBody = $responseBody; + + if (null === $message) { + $message = sprintf('Cannot parse response body: "%s"', $responseBody); + } + + parent::__construct($message, $code); + } + + /** + * 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; + } +} diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index 4bd54209..ef064d08 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -2,6 +2,7 @@ namespace League\OAuth2\Client\Test\Provider; +use League\OAuth2\Client\Provider\Exception\ResponseParsingException; use UnexpectedValueException; use Eloquent\Liberator\Liberator; use Eloquent\Phony\Phpunit\Phony; @@ -624,6 +625,21 @@ public function testParseResponseNonJsonFailure() $this->testParseResponse('', 'application/xml', null, 500); } + public function testResponseParsingException() + { + $this->provider->allowResponseParsingException(); + $exception = null; + try { + $this->testParseResponse('', '', null, 401); + } catch (ResponseParsingException $exception) { + } + $this->assertInstanceOf(ResponseParsingException::class, $exception); + $response = $exception->getResponse(); + $this->assertSame(401, $response->getStatusCode()); + $this->assertSame('', $exception->getResponseBody()); + $this->provider->disallowResponseParsingException(); + } + public function getAppendQueryProvider() { return [ diff --git a/test/src/Provider/Exception/ResponseParsingException.php b/test/src/Provider/Exception/ResponseParsingException.php new file mode 100644 index 00000000..59463346 --- /dev/null +++ b/test/src/Provider/Exception/ResponseParsingException.php @@ -0,0 +1,37 @@ +result = [ + 'response' => new Response('401'), + 'body' => '' + ]; + $this->exception = new ResponseParsingException($this->result['response'], $this->result['body']); + } + + public function testGetResponse() + { + $this->assertInstanceOf(ResponseInterface::class, $this->exception->getResponse()); + } + + public function testGetResponseBody() + { + $this->assertSame($this->result['body'], $this->exception->getResponseBody()); + } +} diff --git a/test/src/Provider/Fake.php b/test/src/Provider/Fake.php index 5ba99fe4..87e90a26 100644 --- a/test/src/Provider/Fake.php +++ b/test/src/Provider/Fake.php @@ -44,6 +44,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); From c86cf24d9e029f3dfd155c5a4af0a9986ff4f70c Mon Sep 17 00:00:00 2001 From: Hasan Date: Fri, 13 Mar 2020 14:17:47 +0100 Subject: [PATCH 2/6] Improve AbstractProviderTest to avoid php5.6 json_decode bug treating empty string as a valid json --- test/src/Provider/AbstractProviderTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index ef064d08..89fd60ef 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -630,13 +630,13 @@ public function testResponseParsingException() $this->provider->allowResponseParsingException(); $exception = null; try { - $this->testParseResponse('', '', null, 401); + $this->testParseResponse('{13}', '', null, 401); } catch (ResponseParsingException $exception) { } $this->assertInstanceOf(ResponseParsingException::class, $exception); $response = $exception->getResponse(); $this->assertSame(401, $response->getStatusCode()); - $this->assertSame('', $exception->getResponseBody()); + $this->assertSame('{13}', $exception->getResponseBody()); $this->provider->disallowResponseParsingException(); } From 6d3d013e3909454fa58f97ef5054959762e135cb Mon Sep 17 00:00:00 2001 From: Hasan Date: Tue, 3 Nov 2020 16:53:17 +0100 Subject: [PATCH 3/6] Fix AbstractProviderTest::testResponseParsingException --- test/src/Provider/AbstractProviderTest.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index ade51906..5dc8069b 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -617,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, @@ -633,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); } @@ -652,17 +657,18 @@ public function testParseResponseNonJsonFailure() public function testResponseParsingException() { - $this->provider->allowResponseParsingException(); + $provider = $this->getMockProvider(); + $provider->allowResponseParsingException(); $exception = null; try { - $this->testParseResponse('{13}', '', null, 401); + $this->testParseResponse('{13}', 'application/json', null, 401, $provider); } catch (ResponseParsingException $exception) { } $this->assertInstanceOf(ResponseParsingException::class, $exception); $response = $exception->getResponse(); $this->assertSame(401, $response->getStatusCode()); $this->assertSame('{13}', $exception->getResponseBody()); - $this->provider->disallowResponseParsingException(); + $provider->disallowResponseParsingException(); } public function getAppendQueryProvider() From 2281364308baa7a27b4934cd218a83e9466ea73a Mon Sep 17 00:00:00 2001 From: Hasan Date: Tue, 3 Nov 2020 17:33:36 +0100 Subject: [PATCH 4/6] Improve ResponseParsingExceptionTest --- .../Exception/ResponseParsingException.php | 37 ------------------ .../ResponseParsingExceptionTest.php | 39 +++++++++++++++++++ 2 files changed, 39 insertions(+), 37 deletions(-) delete mode 100644 test/src/Provider/Exception/ResponseParsingException.php create mode 100644 test/src/Provider/Exception/ResponseParsingExceptionTest.php diff --git a/test/src/Provider/Exception/ResponseParsingException.php b/test/src/Provider/Exception/ResponseParsingException.php deleted file mode 100644 index 59463346..00000000 --- a/test/src/Provider/Exception/ResponseParsingException.php +++ /dev/null @@ -1,37 +0,0 @@ -result = [ - 'response' => new Response('401'), - 'body' => '' - ]; - $this->exception = new ResponseParsingException($this->result['response'], $this->result['body']); - } - - public function testGetResponse() - { - $this->assertInstanceOf(ResponseInterface::class, $this->exception->getResponse()); - } - - public function testGetResponseBody() - { - $this->assertSame($this->result['body'], $this->exception->getResponseBody()); - } -} diff --git a/test/src/Provider/Exception/ResponseParsingExceptionTest.php b/test/src/Provider/Exception/ResponseParsingExceptionTest.php new file mode 100644 index 00000000..578f82b5 --- /dev/null +++ b/test/src/Provider/Exception/ResponseParsingExceptionTest.php @@ -0,0 +1,39 @@ +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()); + } +} From 1548683c4dd8f0fbe3db8bb85ebb167d7a761781 Mon Sep 17 00:00:00 2001 From: Hasan Date: Tue, 3 Nov 2020 17:57:32 +0100 Subject: [PATCH 5/6] Increase coverage of AbstractProvider --- test/src/Provider/AbstractProviderTest.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index 5dc8069b..e2458fa1 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -655,13 +655,24 @@ public function testParseResponseNonJsonFailure() $this->testParseResponse('', 'application/xml', null, 500); } - public function testResponseParsingException() + public function responseParsingExceptionProvider() + { + return [ + ['application/json'], + [''] + ]; + } + + /** + * @dataProvider responseParsingExceptionProvider + */ + public function testResponseParsingException($type) { $provider = $this->getMockProvider(); $provider->allowResponseParsingException(); $exception = null; try { - $this->testParseResponse('{13}', 'application/json', null, 401, $provider); + $this->testParseResponse('{13}', $type, null, 401, $provider); } catch (ResponseParsingException $exception) { } $this->assertInstanceOf(ResponseParsingException::class, $exception); From 58431224db1a718183a134555659ce0143929ea9 Mon Sep 17 00:00:00 2001 From: Hasan Date: Sat, 7 Nov 2020 14:18:04 +0100 Subject: [PATCH 6/6] Throw ResponseParsingException (extending UnexpectedValueException) if response body cannot be parsed as JSON --- src/Provider/AbstractProvider.php | 40 +++++----------- .../Exception/ResponseParsingException.php | 9 ++-- test/src/Provider/AbstractProviderTest.php | 46 ++++++++----------- test/src/Provider/Fake.php | 10 ---- 4 files changed, 38 insertions(+), 67 deletions(-) diff --git a/src/Provider/AbstractProvider.php b/src/Provider/AbstractProvider.php index e3e441d4..84e1b439 100644 --- a/src/Provider/AbstractProvider.php +++ b/src/Provider/AbstractProvider.php @@ -99,15 +99,6 @@ 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; - /** * Constructs an OAuth 2.0 service provider. * @@ -679,9 +670,7 @@ 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. + * @throws ResponseParsingException if response body cannot be parsed. * @param ResponseInterface $response * @return array|string */ @@ -702,26 +691,21 @@ protected function parseResponse(ResponseInterface $response) return $this->parseJson($content); } catch (UnexpectedValueException $e) { if (strpos($type, 'json') !== false) { - throw $this->mayThrowResponseParsingException - ? new ResponseParsingException($response, $content, $e->getMessage(), $e->getCode()) - : $e; + throw 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; + if ($response->getStatusCode() == 500) { + throw new ResponseParsingException( + $response, + $content, + 'An OAuth server error was encountered that did not contain a JSON body', + 0, + $e + ); } + + return $content; } } diff --git a/src/Provider/Exception/ResponseParsingException.php b/src/Provider/Exception/ResponseParsingException.php index 3f8e02a8..d6f5f151 100644 --- a/src/Provider/Exception/ResponseParsingException.php +++ b/src/Provider/Exception/ResponseParsingException.php @@ -16,11 +16,12 @@ use Exception; use Psr\Http\Message\ResponseInterface; +use UnexpectedValueException; /** * Exception thrown if the parser cannot parse the provider response. */ -class ResponseParsingException extends Exception +class ResponseParsingException extends UnexpectedValueException { /** * @var ResponseInterface @@ -37,12 +38,14 @@ class ResponseParsingException extends Exception * @param string $responseBody The response body * @param null $message * @param int $code + * @param Exception|null $previous */ public function __construct( ResponseInterface $response, $responseBody, $message = null, - $code = 0 + $code = 0, + Exception $previous = null ) { $this->response = $response; $this->responseBody = $responseBody; @@ -51,7 +54,7 @@ public function __construct( $message = sprintf('Cannot parse response body: "%s"', $responseBody); } - parent::__construct($message, $code); + parent::__construct($message, $code, $previous); } /** diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index e2458fa1..cbaebaf9 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -2,26 +2,26 @@ namespace League\OAuth2\Client\Test\Provider; -use League\OAuth2\Client\Provider\Exception\ResponseParsingException; -use League\OAuth2\Client\OptionProvider\PostAuthOptionProvider; -use Mockery; -use ReflectionClass; -use UnexpectedValueException; -use GuzzleHttp\Exception\BadResponseException; use GuzzleHttp\ClientInterface; -use League\OAuth2\Client\Provider\AbstractProvider; -use League\OAuth2\Client\Test\Provider\Fake as MockProvider; +use GuzzleHttp\Exception\BadResponseException; use League\OAuth2\Client\Grant\AbstractGrant; -use League\OAuth2\Client\Grant\GrantFactory; use League\OAuth2\Client\Grant\Exception\InvalidGrantException; +use League\OAuth2\Client\Grant\GrantFactory; +use League\OAuth2\Client\OptionProvider\PostAuthOptionProvider; +use League\OAuth2\Client\Provider\AbstractProvider; +use League\OAuth2\Client\Provider\Exception\IdentityProviderException; +use League\OAuth2\Client\Provider\Exception\ResponseParsingException; +use League\OAuth2\Client\Test\Provider\Fake as MockProvider; use League\OAuth2\Client\Token\AccessToken; use League\OAuth2\Client\Token\AccessTokenInterface; use League\OAuth2\Client\Tool\RequestFactory; -use League\OAuth2\Client\Provider\Exception\IdentityProviderException; +use Mockery; use PHPUnit\Framework\TestCase; -use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\RequestInterface; +use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\StreamInterface; +use ReflectionClass; +use UnexpectedValueException; class AbstractProviderTest extends TestCase { @@ -617,7 +617,7 @@ public function parseResponseProvider() /** * @dataProvider parseResponseProvider */ - public function testParseResponse($body, $type, $parsed, $statusCode = 200, $provider = null) + public function testParseResponse($body, $type, $parsed, $statusCode = 200) { $stream = Mockery::mock(StreamInterface::class, [ '__toString' => $body, @@ -633,12 +633,7 @@ public function testParseResponse($body, $type, $parsed, $statusCode = 200, $pro ->andReturn($type); $method = $this->getMethod(AbstractProvider::class, 'parseResponse'); - - if (null === $provider) { - $provider = $this->getMockProvider(); - } - - $result = $method->invoke($provider, $response); + $result = $method->invoke($this->getMockProvider(), $response); $this->assertEquals($parsed, $result); } @@ -657,29 +652,28 @@ public function testParseResponseNonJsonFailure() public function responseParsingExceptionProvider() { + // List only combinations of content-type and status code that should lead to exception + // if the response body cannot be parsed as JSON return [ - ['application/json'], - [''] + ['application/json', 401], + ['application/xml', 500] ]; } /** * @dataProvider responseParsingExceptionProvider */ - public function testResponseParsingException($type) + public function testResponseParsingException($type, $statusCode) { - $provider = $this->getMockProvider(); - $provider->allowResponseParsingException(); $exception = null; try { - $this->testParseResponse('{13}', $type, null, 401, $provider); + $this->testParseResponse('{13}', $type, null, $statusCode); } catch (ResponseParsingException $exception) { } $this->assertInstanceOf(ResponseParsingException::class, $exception); $response = $exception->getResponse(); - $this->assertSame(401, $response->getStatusCode()); + $this->assertSame($statusCode, $response->getStatusCode()); $this->assertSame('{13}', $exception->getResponseBody()); - $provider->disallowResponseParsingException(); } public function getAppendQueryProvider() diff --git a/test/src/Provider/Fake.php b/test/src/Provider/Fake.php index 38a163f5..b0bedcbf 100644 --- a/test/src/Provider/Fake.php +++ b/test/src/Provider/Fake.php @@ -59,16 +59,6 @@ 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);