Merge pull request #899 from Sephster/fix-745

Only Add Authenticate Header if Present in Original Request - Tests

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) 
+
 ## [7.1.1] - released 2018-05-21
 
 ### Fixed
@@ -81,8 +84,6 @@ To address feedback from the security release the following change has been made
 
 ## [5.1.4] - 2017-07-01
 
-- Fixed multiple security vulnerabilities as a result of a security audit paid for by the [Mozilla Secure Open Source Fund](https://wiki.mozilla.org/MOSS/Secure_Open_Source). All users of this library are encouraged to update as soon as possible to this version or version 6.0 or greater.
-	- It is recommended on each `AuthorizationServer` instance you set the `setEncryptionKey()`. This will result in stronger encryption being used. If this method is not set messages will be sent to the defined error handling routines (using `error_log`). Please see the examples and documentation for examples.
 - TravisCI now tests PHP 7.1 (Issue #671)
 - Fix middleware example fatal error (Issue #682)
 - Fix typo in the first README sentence (Issue #690)
@@ -429,6 +430,8 @@ Version 5 is a complete code rewrite.
 [3.0.0]: https://github.com/thephpleague/oauth2-server/compare/2.1.1...3.0.0
 [2.1.1]: https://github.com/thephpleague/oauth2-server/compare/2.1.0...2.1.1
 [2.1.0]: https://github.com/thephpleague/oauth2-server/compare/2.0.5...2.1.0
+[2.1.1]: https://github.com/thephpleague/oauth2-server/compare/2.1.0...2.1.1
+[2.1.0]: https://github.com/thephpleague/oauth2-server/compare/2.0.5...2.1.0
 [2.0.5]: https://github.com/thephpleague/oauth2-server/compare/2.0.4...2.0.5
 [2.0.4]: https://github.com/thephpleague/oauth2-server/compare/2.0.3...2.0.4
 [2.0.3]: https://github.com/thephpleague/oauth2-server/compare/2.0.2...2.0.3

src/Exception/OAuthServerException.php

@@ -10,6 +10,7 @@
 namespace League\OAuth2\Server\Exception;
 
 use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
 
 class OAuthServerException extends \Exception
 {
@@ -38,6 +39,11 @@ class OAuthServerException extends \Exception
      */
     private $payload;
 
+    /**
+     * @var ServerRequestInterface
+     */
+    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
+     *
+     * @param ServerRequestInterface $serverRequest
+     */
+    public function setServerRequest($serverRequest)
+    {
+        $this->serverRequest = $serverRequest;
+    }
+
     /**
      * Unsupported grant type error.
      *
@@ -117,13 +133,17 @@ class OAuthServerException extends \Exception
     /**
      * Invalid client error.
      *
+     * @param ServerRequestInterface $serverRequest
+     *
      * @return static
      */
-    public static function invalidClient()
+    public static function invalidClient($serverRequest)
     {
-        $errorMessage = 'Client authentication failed';
+        $exception = new static('Client authentication failed', 4, 'invalid_client', 401);
 
-        return new static($errorMessage, 4, 'invalid_client', 401);
+        $exception->setServerRequest($serverRequest);
+
+        return $exception;
     }
 
     /**
@@ -294,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' && array_key_exists('HTTP_AUTHORIZATION', $_SERVER) !== false) {
+        if ($this->errorType === 'invalid_client' && $this->serverRequest->hasHeader('Authorization') === true) {
-            $authScheme = strpos($_SERVER['HTTP_AUTHORIZATION'], 'Bearer') === 0 ? 'Bearer' : 'Basic';
+            $authScheme = strpos($this->serverRequest->getHeader('Authorization')[0], 'Bearer') === 0 ? 'Bearer' : 'Basic';
 
             $headers['WWW-Authenticate'] = $authScheme . ' realm="OAuth"';
         }

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

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]

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]

tests/Exception/OAuthServerExceptionTest.php

@@ -0,0 +1,67 @@
+<?php
+
+namespace LeagueTests\Exception;
+
+use League\OAuth2\Server\Exception\OAuthServerException;
+use League\OAuth2\Server\Grant\AbstractGrant;
+use League\OAuth2\Server\Repositories\ClientRepositoryInterface;
+use PHPUnit\Framework\TestCase;
+use Zend\Diactoros\Response;
+use Zend\Diactoros\ServerRequest;
+
+class OAuthServerExceptionTest extends TestCase
+{
+    public function testInvalidClientExceptionSetsAuthenticateHeader()
+    {
+        $serverRequest = (new ServerRequest())
+            ->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
+     *
+     * @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);
+    }
+}

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