Merge pull request #352 from stloyd/bugfix/oauth_error

Add better detection of oauth errors returned from resource owners
hwi · Aug 3, 2013 · 1dedd5f · 1dedd5f
2 parents 7db3533 + 3f5750f
commit 1dedd5f
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@ Changelog
 =========
 
 ## 0.2.7 (2013-xx-xx)
+* Fix: Polish oauth error detection to cover cases from i.e. Facebook resource owner
 * Fix: Changed authorization url for Vkontakte resource owner
 
 ## 0.2.6 (2013-06-24)

diff --git a/Security/Http/Firewall/OAuthListener.php b/Security/Http/Firewall/OAuthListener.php
@@ -96,6 +96,9 @@ protected function attemptAuthentication(Request $request)
     }
 
     /**
+     * Detects errors returned by resource owners and transform them into
+     * human readable messages
+     *
      * @param Request $request
      *
      * @throws AuthenticationException
@@ -105,7 +108,11 @@ private function handleOAuthError(Request $request)
         $error = null;
 
         // Try to parse content if error was not in request query
-        if ($request->query->has('error')) {
+        if ($request->query->has('error') || $request->query->has('error_code')) {
+            if ($request->query->has('error_message')) {
+                throw new AuthenticationException(rawurldecode($request->query->get('error_message')));
+            }
+
             $content = json_decode($request->getContent(), true);
             if (JSON_ERROR_NONE === json_last_error() && isset($content['error'])) {
                 if (isset($content['error']['message'])) {
@@ -141,45 +148,33 @@ private function transformOAuthError($errorCode)
         // "translate" error to human readable format
         switch ($errorCode) {
             case 'access_denied':
-                $error = 'You have refused access for this site.';
-                break;
+                return 'You have refused access for this site.';
 
             case 'authorization_expired':
-                $error = 'Authorization expired.';
-                break;
+                return 'Authorization expired.';
 
             case 'bad_verification_code':
-                $error = 'Bad verification code.';
-                break;
+                return 'Bad verification code.';
 
             case 'consumer_key_rejected':
-                $error = 'You have refused access for this site.';
-                break;
+                return 'You have refused access for this site.';
 
             case 'incorrect_client_credentials':
-                $error = 'Incorrect client credentials.';
-                break;
+                return 'Incorrect client credentials.';
 
             case 'invalid_assertion':
-                $error = 'Invalid assertion.';
-                break;
+                return 'Invalid assertion.';
 
             case 'redirect_uri_mismatch':
-                $error = 'Redirect URI mismatches configured one.';
-                break;
+                return 'Redirect URI mismatches configured one.';
 
             case 'unauthorized_client':
-                $error = 'Unauthorized client.';
-                break;
+                return 'Unauthorized client.';
 
             case 'unknown_format':
-                $error = 'Unknown format.';
-                break;
-
-            default:
-                $error = sprintf('Unknown OAuth error: "%s".', $errorCode);
+                return 'Unknown format.';
         }
 
-        return $error;
+        return sprintf('Unknown OAuth error: "%s".', $errorCode);
     }
 }
diff --git a/Tests/OAuth/ResourceOwner/FacebookResourceOwnerTest.php b/Tests/OAuth/ResourceOwner/FacebookResourceOwnerTest.php
@@ -40,6 +40,21 @@ public function testGetAccessTokenFailedResponse()
         $this->resourceOwner->getAccessToken($request, 'http://redirect.to/');
     }
 
+    /**
+     * @expectedException \Symfony\Component\Security\Core\Exception\AuthenticationException
+     */
+    public function testGetAccessTokenErrorResponse()
+    {
+        $this->mockBuzz();
+
+        $request = new Request(array(
+            'error_code'    => 901,
+            'error_message' => 'This app is in sandbox mode.  Edit the app configuration at http://developers.facebook.com/apps to make the app publicly visible.'
+        ));
+
+        $this->resourceOwner->getAccessToken($request, 'http://redirect.to/');
+    }
+
     protected function setUpResourceOwner($name, $httpUtils, array $options)
     {
         $options = array_merge(

diff --git a/Tests/OAuth/ResourceOwner/GenericOAuth2ResourceOwnerTest.php b/Tests/OAuth/ResourceOwner/GenericOAuth2ResourceOwnerTest.php
@@ -11,6 +11,8 @@
 
 namespace HWI\Bundle\OAuthBundle\Tests\OAuth\ResourceOwner;
 
+use Buzz\Message\MessageInterface;
+use Buzz\Message\RequestInterface;
 use HWI\Bundle\OAuthBundle\OAuth\ResourceOwner\GenericOAuth2ResourceOwner;
 use Symfony\Component\HttpFoundation\Request;
 
@@ -175,6 +177,10 @@ public function testCustomResponseClass()
         $this->assertEquals('access_token', $userResponse->getOAuthToken());
     }
 
+    /**
+     * @param RequestInterface $request
+     * @param MessageInterface $response
+     */
     public function buzzSendMock($request, $response)
     {
         $response->setContent($this->buzzResponse);