Fix zfcampus#97 and zfcampus#106

nuxwin · Dec 23, 2015 · caab9c5 · caab9c5
1 parent fba9aaa
commit caab9c5
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 14 deletions.
diff --git a/src/Authentication/HttpAdapter.php b/src/Authentication/HttpAdapter.php
@@ -120,11 +120,6 @@ public function preAuth(Request $request, Response $response)
      */
     public function authenticate(Request $request, Response $response, MvcAuthEvent $mvcAuthEvent)
     {
-        if (! $request->getHeader('Authorization', false)) {
-            // No credentials were present at all, so we just return a guest identity.
-            return new Identity\GuestIdentity();
-        }
-
         $this->httpAuth->setRequest($request);
         $this->httpAuth->setResponse($response);
 

diff --git a/src/Authorization/DefaultAuthorizationListener.php b/src/Authorization/DefaultAuthorizationListener.php
@@ -6,6 +6,7 @@
 
 namespace ZF\MvcAuth\Authorization;
 
+use Zend\Http\Headers;
 use Zend\Http\Request;
 use Zend\Http\Response;
 use Zend\Mvc\Router\RouteMatch;
@@ -63,6 +64,23 @@ public function __invoke(MvcAuthEvent $mvcAuthEvent)
 
         $resource = $mvcAuthEvent->getResource();
         $identity = $mvcAuthEvent->getIdentity();
-        return $this->authorization->isAuthorized($identity, $resource, $request->getMethod());
+        $isAuthorized = $this->authorization->isAuthorized($identity, $resource, $request->getMethod());
+
+        // We need reset MVC response which can have been modified
+        // by authentication layer. This avoid challenging client in case a
+        // guest identity is allowed to access the resource after all.
+        if ($isAuthorized) {
+            // Resetting response set on mvc event is not sufficient
+            // This denote another problem which
+            $app = $mvcEvent->getApplication();
+            $response = $app->getResponse();
+
+            $response->setStatusCode(200);
+            $response->setHeaders(new Headers());
+
+            $mvcEvent->setResponse($response);
+        }
+
+        return $isAuthorized;
     }
 }
diff --git a/src/Authorization/DefaultAuthorizationPostListener.php b/src/Authorization/DefaultAuthorizationPostListener.php
@@ -23,15 +23,19 @@ public function __invoke(MvcAuthEvent $mvcAuthEvent)
         $response = $mvcEvent->getResponse();
 
         if ($mvcAuthEvent->isAuthorized()) {
-            if ($response instanceof HttpResponse) {
-                if ($response->getStatusCode() != 200) {
-                    $response->setStatusCode(200);
-                }
+            if ($response instanceof HttpResponse
+                && $response->getStatusCode() != 200
+            ) {
+                $response->setStatusCode(200);
             }
             return;
         }
 
-        if (!$response instanceof HttpResponse) {
+        // If no HTTP response, or an HTTP response already denoting a problem,
+        // return it immediately
+        if (!$response instanceof HttpResponse
+            || $response->getStatusCode() == 401
+        ) {
             return $response;
         }
 

diff --git a/src/MvcRouteListener.php b/src/MvcRouteListener.php
@@ -13,6 +13,7 @@
 use Zend\Mvc\MvcEvent;
 use Zend\Http\Request as HttpRequest;
 use Zend\Stdlib\ResponseInterface as Response;
+use Zend\Http\Response as HttpResponse;
 
 class MvcRouteListener extends AbstractListenerAggregate
 {
@@ -156,7 +157,22 @@ function ($r) {
                 return ($r instanceof Response);
             }
         );
-        return $responses->last();
+
+        $response = $responses->last();
+
+        // In case of a response denoting authentication failure, we cannot
+        // just return it because doing this would short-circuit route event,
+        // meaning that authorization rules would not have any chance to be
+        // evaluated for a guest identity. We ensure that response is set on
+        // MvcEvent instead.
+        if ($response instanceof HttpResponse
+            && $response->getStatusCode() == 401
+        ) {
+            $mvcEvent->setResponse($response);
+            return;
+        }
+
+        return $response;
     }
 
     /**

diff --git a/test/Authentication/HttpAdapterTest.php b/test/Authentication/HttpAdapterTest.php
@@ -37,7 +37,7 @@ public function setUp()
         );
     }
 
-    public function testAuthenticateReturnsGuestIdentityIfNoAuthorizationHeaderProvided()
+    /*public function testAuthenticateReturnsGuestIdentityIfNoAuthorizationHeaderProvided()
     {
         $httpAuth = new HttpAuth([
             'accept_schemes' => 'basic',
@@ -50,7 +50,7 @@ public function testAuthenticateReturnsGuestIdentityIfNoAuthorizationHeaderProvi
         $adapter = new HttpAdapter($httpAuth, $this->authentication);
         $result  = $adapter->authenticate($this->request, $this->response, $this->event);
         $this->assertInstanceOf('ZF\MvcAuth\Identity\GuestIdentity', $result);
-    }
+    }*/
 
     public function testAuthenticateReturnsFalseIfInvalidCredentialsProvidedInAuthorizationHeader()
     {

diff --git a/test/Authorization/DefaultAuthorizationPostListenerTest.php b/test/Authorization/DefaultAuthorizationPostListenerTest.php
@@ -58,6 +58,17 @@ public function testReturnsComposedEventResponseWhenNotAuthorizedButNotAnHttpRes
         $this->assertSame($response, $listener($this->mvcAuthEvent));
     }
 
+    public function testReturns401ResponseWhenNotAuthenticatedAndNotAuthorized()
+    {
+        $listener = $this->listener;
+        $response = $this->mvcAuthEvent->getMvcEvent()->getResponse();
+        $response->setStatusCode('401');
+        $this->mvcAuthEvent->setIsAuthorized(false);
+        $this->assertSame($response, $listener($this->mvcAuthEvent));
+        $this->assertEquals(401, $response->getStatusCode());
+        $this->assertEquals('Unauthorized', $response->getReasonPhrase());
+    }
+
     public function testReturns403ResponseWhenNotAuthorizedAndHttpResponseComposed()
     {
         $listener = $this->listener;