Merge pull request #277 from shadowhand/feature/v1-csprng-for-state

Generate cryptographically secure states
thephpleague · Apr 28, 2015 · d99beed · d99beed
2 parents cd14488 + a27ca8a
commit d99beed
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 11 deletions.
diff --git a/composer.json b/composer.json
@@ -5,7 +5,8 @@
     "require": {
         "php": ">=5.4.0",
         "ext-curl": "*",
-        "egeloen/http-adapter": "0.6.*"
+        "egeloen/http-adapter": "0.6.*",
+        "ircmaxell/random-lib": "~1.1"
     },
     "require-dev": {
         "phpunit/phpunit": "~4.0",

diff --git a/src/Provider/AbstractProvider.php b/src/Provider/AbstractProvider.php
@@ -10,6 +10,7 @@
 use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
 use League\OAuth2\Client\Grant\GrantInterface;
 use League\OAuth2\Client\Token\AccessToken;
+use RandomLib\Factory as RandomFactory;
 use UnexpectedValueException;
 
 abstract class AbstractProvider implements ProviderInterface
@@ -79,6 +80,11 @@ abstract class AbstractProvider implements ProviderInterface
      */
     protected $httpClient;
 
+    /**
+     * @var RandomFactory
+     */
+    protected $randomFactory;
+
     /**
      * @var Closure
      */
@@ -90,15 +96,27 @@ abstract class AbstractProvider implements ProviderInterface
      */
     protected $httpBuildEncType = 1;
 
-    public function __construct($options = [], HttpAdapterInterface $httpClient = null)
+    /**
+     * @param array $options
+     * @param array $collaborators
+     */
+    public function __construct($options = [], array $collaborators = [])
     {
         foreach ($options as $option => $value) {
             if (property_exists($this, $option)) {
                 $this->{$option} = $value;
             }
         }
 
-        $this->setHttpClient($httpClient ?: new CurlHttpAdapter());
+        if (empty($collaborators['httpClient'])) {
+            $collaborators['httpClient'] = new CurlHttpAdapter();
+        }
+        $this->setHttpClient($collaborators['httpClient']);
+
+        if (empty($collaborators['randomFactory'])) {
+            $collaborators['randomFactory'] = new RandomFactory();
+        }
+        $this->setRandomFactory($collaborators['randomFactory']);
     }
 
     public function setHttpClient(HttpAdapterInterface $client)
@@ -115,6 +133,29 @@ public function getHttpClient()
         return $client;
     }
 
+    /**
+     * Set the instance of the CSPRNG random generator factory to use.
+     *
+     * @param  RandomFactory $factory
+     * @return $this
+     */
+    public function setRandomFactory(RandomFactory $factory)
+    {
+        $this->randomFactory = $factory;
+
+        return $this;
+    }
+
+    /**
+     * Get the instance of the CSPRNG random generatory factory.
+     *
+     * @return RandomFactory
+     */
+    public function getRandomFactory()
+    {
+        return $this->randomFactory;
+    }
+
     // Implementing these interfaces methods should not be required, but not
     // doing so will break HHVM because of https://github.com/facebook/hhvm/issues/5170
     // Once HHVM is working, delete the following abstract methods.
@@ -135,17 +176,46 @@ public function setScopes(array $scopes)
         $this->scopes = $scopes;
     }
 
+    /**
+     * Get a new random string to use for auth state.
+     *
+     * @param  integer $length
+     * @return string
+     */
+    protected function getRandomState($length = 32)
+    {
+        $generator = $this
+            ->getRandomFactory()
+            ->getMediumStrengthGenerator();
+
+        return $generator->generateString($length);
+    }
+
     public function getAuthorizationUrl(array $options = [])
     {
-        $this->state = isset($options['state']) ? $options['state'] : md5(uniqid(rand(), true));
+        if (empty($options['state'])) {
+            $options['state'] = $this->getRandomState();
+        }
+
+        // Store the state, it may need to be accessed later.
+        $this->state = $options['state'];
+
+        $options += [
+            // Do not set the default state here! The random generator takes a
+            // non-trivial amount of time to run.
+            'response_type'   => 'code',
+            'approval_prompt' => 'auto',
+        ];
+
+        $scopes = is_array($this->scopes) ? implode($this->scopeSeparator, $this->scopes) : $this->scopes;
 
         $params = [
-            'client_id' => $this->clientId,
-            'redirect_uri' => $this->redirectUri,
-            'state' => $this->state,
-            'scope' => is_array($this->scopes) ? implode($this->scopeSeparator, $this->scopes) : $this->scopes,
-            'response_type' => isset($options['response_type']) ? $options['response_type'] : 'code',
-            'approval_prompt' => isset($options['approval_prompt']) ? $options['approval_prompt'] : 'auto',
+            'client_id'       => $this->clientId,
+            'redirect_uri'    => $this->redirectUri,
+            'state'           => $this->state,
+            'scope'           => $scopes,
+            'response_type'   => $options['response_type'],
+            'approval_prompt' => $options['approval_prompt'],
         ];
 
         return $this->urlAuthorize().'?'.$this->httpBuildQuery($params, '', '&');

diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php
@@ -85,10 +85,18 @@ public function testConstructorSetsHttpAdapter()
     {
         $mockAdapter = m::mock('Ivory\HttpAdapter\HttpAdapterInterface');
 
-        $mockProvider = new MockProvider([], $mockAdapter);
+        $mockProvider = new MockProvider([], ['httpClient' => $mockAdapter]);
         $this->assertSame($mockAdapter, $mockProvider->getHttpClient());
     }
 
+    public function testConstructorSetsRandomFactory()
+    {
+        $mockAdapter = m::mock('RandomLib\Factory');
+
+        $mockProvider = new MockProvider([], ['randomFactory' => $mockAdapter]);
+        $this->assertSame($mockAdapter, $mockProvider->getRandomFactory());
+    }
+
     public function testSetRedirectHandler()
     {
         $this->testFunction = false;
@@ -185,6 +193,33 @@ public function getHeadersTest()
         $this->assertEquals(['Authorization' => 'Bearer xyz'], $provider->getHeaders($token));
     }
 
+    public function testRandomGeneratorCreatesRandomState()
+    {
+        $xstate = str_repeat('x', 32);
+
+        $generator = m::mock('RandomLib\Generator');
+        $generator->shouldReceive('generateString')->with(32)->times(1)->andReturn($xstate);
+
+        $factory = m::mock('RandomLib\Factory');
+        $factory->shouldReceive('getMediumStrengthGenerator')->times(1)->andReturn($generator);
+
+        $provider = new MockProvider([], ['randomFactory' => $factory]);
+
+        $url = $provider->getAuthorizationUrl();
+
+        parse_str(parse_url($url, PHP_URL_QUERY), $qs);
+
+        $this->assertArrayHasKey('state', $qs);
+        $this->assertSame($xstate, $qs['state']);
+
+        // Same test, but using the non-mock implementation
+        $url = $this->provider->getAuthorizationUrl();
+
+        parse_str(parse_url($url, PHP_URL_QUERY), $qs);
+
+        $this->assertRegExp('/[a-zA-Z0-9\/+]{32}\b/', $qs['state']);
+    }
+
     public function testErrorResponsesCanBeCustomizedAtTheProvider()
     {
         $provider = new MockProvider([