From ff5e9f57a59d3865a93df227759b366cd1e4239f Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Thu, 10 May 2018 22:07:03 +0100 Subject: [PATCH 1/8] Only add authenticate header if present in original request thephpleague/oauth2-server#745 --- src/Exception/OAuthServerException.php | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index 65fe861e..a62d961d 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -294,13 +294,9 @@ class OAuthServerException extends \Exception // include the "WWW-Authenticate" response header field // matching the authentication scheme used by the client. // @codeCoverageIgnoreStart - if ($this->errorType === 'invalid_client') { - $authScheme = 'Basic'; - if (array_key_exists('HTTP_AUTHORIZATION', $_SERVER) !== false - && strpos($_SERVER['HTTP_AUTHORIZATION'], 'Bearer') === 0 - ) { - $authScheme = 'Bearer'; - } + if ($this->errorType === 'invalid_client' && array_key_exists('HTTP_AUTHORIZATION', $_SERVER) !== false) { + $authScheme = strpos($_SERVER['HTTP_AUTHORIZATION'], 'Bearer') === 0 ? 'Bearer' : 'Basic'; + $headers['WWW-Authenticate'] = $authScheme . ' realm="OAuth"'; } // @codeCoverageIgnoreEnd From 33ce8496175955ac5503b95f3704fce59fdf350d Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sun, 13 May 2018 17:29:07 +0100 Subject: [PATCH 2/8] Add tests for invalid client exception --- src/Exception/OAuthServerException.php | 30 +++++++-- src/Grant/AbstractGrant.php | 6 +- src/Grant/AuthCodeGrant.php | 8 +-- src/Grant/ImplicitGrant.php | 8 +-- tests/Exception/OAuthServerExceptionTest.php | 66 ++++++++++++++++++++ tests/Grant/AbstractGrantTest.php | 2 +- 6 files changed, 104 insertions(+), 16 deletions(-) create mode 100644 tests/Exception/OAuthServerExceptionTest.php diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index a62d961d..8b296164 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -9,6 +9,7 @@ namespace League\OAuth2\Server\Exception; +use Psr\Http\Message\ServerRequest; use Psr\Http\Message\ResponseInterface; class OAuthServerException extends \Exception @@ -38,6 +39,11 @@ class OAuthServerException extends \Exception */ private $payload; + /** + * @var ServerRequest + */ + private $serverRequest; + /** * Throw a new exception. * @@ -84,6 +90,16 @@ class OAuthServerException extends \Exception $this->payload = $payload; } + /** + * Set the server request that is responsible for generating the exception + * + * @return void + */ + public function setServerRequest($serverRequest) + { + $this->ServerRequest = $serverRequest; + } + /** * Unsupported grant type error. * @@ -117,13 +133,19 @@ class OAuthServerException extends \Exception /** * Invalid client error. * + * @param ServerRequest $serverRequest + * * @return static */ - public static function invalidClient() + public static function invalidClient($serverRequest) { $errorMessage = 'Client authentication failed'; - return new static($errorMessage, 4, 'invalid_client', 401); + $exception = new static('Client authentication failed', 4, 'invalid_client', 401); + + $exception->setServerRequest($serverRequest); + + return $exception; } /** @@ -294,8 +316,8 @@ class OAuthServerException extends \Exception // include the "WWW-Authenticate" response header field // matching the authentication scheme used by the client. // @codeCoverageIgnoreStart - if ($this->errorType === 'invalid_client' && array_key_exists('HTTP_AUTHORIZATION', $_SERVER) !== false) { - $authScheme = strpos($_SERVER['HTTP_AUTHORIZATION'], 'Bearer') === 0 ? 'Bearer' : 'Basic'; + if ($this->errorType === 'invalid_client' && $this->ServerRequest->hasHeader('Authorization') === true) { + $authScheme = strpos($this->ServerRequest->getHeader('Authorization')[0], 'Bearer') === 0 ? 'Bearer' : 'Basic'; $headers['WWW-Authenticate'] = $authScheme . ' realm="OAuth"'; } diff --git a/src/Grant/AbstractGrant.php b/src/Grant/AbstractGrant.php index 79a1ac47..05b73faa 100644 --- a/src/Grant/AbstractGrant.php +++ b/src/Grant/AbstractGrant.php @@ -190,7 +190,7 @@ abstract class AbstractGrant implements GrantTypeInterface if ($client instanceof ClientEntityInterface === false) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } // If a redirect URI is provided ensure it matches what is pre-registered @@ -201,13 +201,13 @@ abstract class AbstractGrant implements GrantTypeInterface && (strcmp($client->getRedirectUri(), $redirectUri) !== 0) ) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } elseif ( is_array($client->getRedirectUri()) && in_array($redirectUri, $client->getRedirectUri(), true) === false ) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } } diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 80e1cd0f..084e21d4 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -236,7 +236,7 @@ class AuthCodeGrant extends AbstractAuthorizeGrant if ($client instanceof ClientEntityInterface === false) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } $redirectUri = $this->getQueryStringParameter('redirect_uri', $request); @@ -247,18 +247,18 @@ class AuthCodeGrant extends AbstractAuthorizeGrant && (strcmp($client->getRedirectUri(), $redirectUri) !== 0) ) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } elseif ( is_array($client->getRedirectUri()) && in_array($redirectUri, $client->getRedirectUri(), true) === false ) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } } elseif (is_array($client->getRedirectUri()) && count($client->getRedirectUri()) !== 1 || empty($client->getRedirectUri())) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } else { $redirectUri = is_array($client->getRedirectUri()) ? $client->getRedirectUri()[0] diff --git a/src/Grant/ImplicitGrant.php b/src/Grant/ImplicitGrant.php index b4157883..c740f75c 100644 --- a/src/Grant/ImplicitGrant.php +++ b/src/Grant/ImplicitGrant.php @@ -131,7 +131,7 @@ class ImplicitGrant extends AbstractAuthorizeGrant if ($client instanceof ClientEntityInterface === false) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } $redirectUri = $this->getQueryStringParameter('redirect_uri', $request); @@ -141,18 +141,18 @@ class ImplicitGrant extends AbstractAuthorizeGrant && (strcmp($client->getRedirectUri(), $redirectUri) !== 0) ) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } elseif ( is_array($client->getRedirectUri()) && in_array($redirectUri, $client->getRedirectUri(), true) === false ) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } } elseif (is_array($client->getRedirectUri()) && count($client->getRedirectUri()) !== 1 || empty($client->getRedirectUri())) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient(); + throw OAuthServerException::invalidClient($request); } else { $redirectUri = is_array($client->getRedirectUri()) ? $client->getRedirectUri()[0] diff --git a/tests/Exception/OAuthServerExceptionTest.php b/tests/Exception/OAuthServerExceptionTest.php new file mode 100644 index 00000000..11704ade --- /dev/null +++ b/tests/Exception/OAuthServerExceptionTest.php @@ -0,0 +1,66 @@ +withParsedBody([ + 'client_id' => 'foo', + ]) + ->withAddedHeader('Authorization', 'Basic fakeauthdetails'); + + try { + $this->issueInvalidClientException($serverRequest); + } catch (OAuthServerException $e) { + $response = $e->generateHttpResponse(new Response()); + + $this->assertTrue($response->hasHeader('WWW-Authenticate')); + } + } + + public function testInvalidClientExceptionOmitsAuthenticateHeader() + { + $serverRequest = (new ServerRequest()) + ->withParsedBody([ + 'client_id' => 'foo', + ]); + + try { + $this->issueInvalidClientException($serverRequest); + } catch (OAuthServerException $e) { + $response = $e->generateHttpResponse(new Response()); + + $this->assertFalse($response->hasHeader('WWW-Authenticate')); + } + } + + /** + * Issue an invalid client exception + * + * @return void + * @throws OAuthServerException + */ + private function issueInvalidClientException($serverRequest) + { + $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); + $clientRepositoryMock->method('getClientEntity')->willReturn(false); + + $grantMock = $this->getMockForAbstractClass(AbstractGrant::class); + $grantMock->setClientRepository($clientRepositoryMock); + + $abstractGrantReflection = new ReflectionClass($grantMock); + + $validateClientMethod = $abstractGrantReflection->getMethod('validateClient'); + $validateClientMethod->setAccessible(true); + + $validateClientMethod->invoke($grantMock, $serverRequest); + } +} diff --git a/tests/Grant/AbstractGrantTest.php b/tests/Grant/AbstractGrantTest.php index 6266df0a..5da2776e 100644 --- a/tests/Grant/AbstractGrantTest.php +++ b/tests/Grant/AbstractGrantTest.php @@ -122,7 +122,7 @@ class AbstractGrantTest extends TestCase $validateClientMethod = $abstractGrantReflection->getMethod('validateClient'); $validateClientMethod->setAccessible(true); - $result = $validateClientMethod->invoke($grantMock, $serverRequest, true, true); + $result = $validateClientMethod->invoke($grantMock, $serverRequest); $this->assertEquals($client, $result); } From c2dcdee26667704843a6b297e7e469926372d4bc Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sun, 13 May 2018 17:34:06 +0100 Subject: [PATCH 3/8] Change order of use statements --- src/Exception/OAuthServerException.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index 8b296164..72f23e6e 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -9,8 +9,8 @@ namespace League\OAuth2\Server\Exception; -use Psr\Http\Message\ServerRequest; use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\ServerRequest; class OAuthServerException extends \Exception { From cbce5f45ba4f227e471f287c7a92de51df7c1528 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sun, 13 May 2018 17:38:07 +0100 Subject: [PATCH 4/8] Fix case for serverRequest variable and remove unused variable --- src/Exception/OAuthServerException.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index 72f23e6e..4713eba5 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -97,7 +97,7 @@ class OAuthServerException extends \Exception */ public function setServerRequest($serverRequest) { - $this->ServerRequest = $serverRequest; + $this->serverRequest = $serverRequest; } /** @@ -139,8 +139,6 @@ class OAuthServerException extends \Exception */ public static function invalidClient($serverRequest) { - $errorMessage = 'Client authentication failed'; - $exception = new static('Client authentication failed', 4, 'invalid_client', 401); $exception->setServerRequest($serverRequest); @@ -316,8 +314,8 @@ class OAuthServerException extends \Exception // include the "WWW-Authenticate" response header field // matching the authentication scheme used by the client. // @codeCoverageIgnoreStart - if ($this->errorType === 'invalid_client' && $this->ServerRequest->hasHeader('Authorization') === true) { - $authScheme = strpos($this->ServerRequest->getHeader('Authorization')[0], 'Bearer') === 0 ? 'Bearer' : 'Basic'; + if ($this->errorType === 'invalid_client' && $this->serverRequest->hasHeader('Authorization') === true) { + $authScheme = strpos($this->serverRequest->getHeader('Authorization')[0], 'Bearer') === 0 ? 'Bearer' : 'Basic'; $headers['WWW-Authenticate'] = $authScheme . ' realm="OAuth"'; } From f8c2e721a05c18917bce16e696df4dc659ab3ddb Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sun, 13 May 2018 17:41:21 +0100 Subject: [PATCH 5/8] Remove return voids and fix docblock and use orders --- src/Exception/OAuthServerException.php | 2 +- tests/Exception/OAuthServerExceptionTest.php | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index 4713eba5..ddd4c534 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -93,7 +93,7 @@ class OAuthServerException extends \Exception /** * Set the server request that is responsible for generating the exception * - * @return void + * @param ServerRequest $serverRequest */ public function setServerRequest($serverRequest) { diff --git a/tests/Exception/OAuthServerExceptionTest.php b/tests/Exception/OAuthServerExceptionTest.php index 11704ade..b86f33a1 100644 --- a/tests/Exception/OAuthServerExceptionTest.php +++ b/tests/Exception/OAuthServerExceptionTest.php @@ -1,9 +1,9 @@ Date: Sun, 13 May 2018 17:52:45 +0100 Subject: [PATCH 6/8] Fix ServerRequestInterface docblock type --- src/Exception/OAuthServerException.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index ddd4c534..345b99a0 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -10,7 +10,7 @@ namespace League\OAuth2\Server\Exception; use Psr\Http\Message\ResponseInterface; -use Psr\Http\Message\ServerRequest; +use Psr\Http\Message\ServerRequestInterface; class OAuthServerException extends \Exception { @@ -40,7 +40,7 @@ class OAuthServerException extends \Exception private $payload; /** - * @var ServerRequest + * @var ServerRequestInterface */ private $serverRequest; @@ -93,7 +93,7 @@ class OAuthServerException extends \Exception /** * Set the server request that is responsible for generating the exception * - * @param ServerRequest $serverRequest + * @param ServerRequestInterface $serverRequest */ public function setServerRequest($serverRequest) { @@ -133,7 +133,7 @@ class OAuthServerException extends \Exception /** * Invalid client error. * - * @param ServerRequest $serverRequest + * @param ServerRequestInterface $serverRequest * * @return static */ From b1b33207ab4f3164f497accdd9dbefadb0189343 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sun, 13 May 2018 18:02:23 +0100 Subject: [PATCH 7/8] Fix namespacing for Exception test --- tests/Exception/OAuthServerExceptionTest.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/Exception/OAuthServerExceptionTest.php b/tests/Exception/OAuthServerExceptionTest.php index b86f33a1..bac914e4 100644 --- a/tests/Exception/OAuthServerExceptionTest.php +++ b/tests/Exception/OAuthServerExceptionTest.php @@ -1,5 +1,7 @@ getMockForAbstractClass(AbstractGrant::class); $grantMock->setClientRepository($clientRepositoryMock); - $abstractGrantReflection = new ReflectionClass($grantMock); + $abstractGrantReflection = new \ReflectionClass($grantMock); $validateClientMethod = $abstractGrantReflection->getMethod('validateClient'); $validateClientMethod->setAccessible(true); From 98812e6fabe5f82917d0a5d8ae143260e83c1bce Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Mon, 21 May 2018 11:21:44 +0100 Subject: [PATCH 8/8] Update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcac3bfd..1e27f9ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +### Changed +- The `invalidClient()` function accepts a PSR-7 compliant `$serverRequest` argument to avoid accessing the `$_SERVER` global variable and improve testing (PR #899) + ### Fixed - No longer set a WWW-Authenticate header for invalid clients if the client did not send an Authorization header in the original request