From e167cadb57db48060568baf7edd2ff03e9d29c54 Mon Sep 17 00:00:00 2001 From: Rutger Hertogh Date: Mon, 16 Aug 2021 20:19:56 +0200 Subject: [PATCH 1/4] Added support for PKCE --- docs/usage.md | 34 +++++++ src/Provider/AbstractProvider.php | 98 ++++++++++++++++++++ src/Provider/GenericProvider.php | 14 +++ test/src/Provider/AbstractProviderTest.php | 100 +++++++++++++++++++++ test/src/Provider/Fake.php | 24 +++++ test/src/Provider/GenericProviderTest.php | 5 ++ 6 files changed, 275 insertions(+) diff --git a/docs/usage.md b/docs/usage.md index 5950fd95..047b1bf7 100755 --- a/docs/usage.md +++ b/docs/usage.md @@ -16,6 +16,7 @@ The following example uses the out-of-the-box `GenericProvider` provided by this The *authorization code* grant type is the most common grant type used when authenticating users with a third-party service. This grant type utilizes a *client* (this library), a *service provider* (the server), and a *resource owner* (the account with credentials to a protected—or owned—resource) to request access to resources owned by the user. This is often referred to as _3-legged OAuth_, since there are three parties involved. + ```php $provider = new \League\OAuth2\Client\Provider\GenericProvider([ 'clientId' => 'XXXXXX', // The client ID assigned to you by the provider @@ -37,6 +38,10 @@ if (!isset($_GET['code'])) { // Get the state generated for you and store it to the session. $_SESSION['oauth2state'] = $provider->getState(); + // Optional, only required when PKCE is enabled. + // Get the PKCE code generated for you and store it to the session. + $_SESSION['oauth2pkceCode'] = $provider->getPkceCode(); + // Redirect the user to the authorization URL. header('Location: ' . $authorizationUrl); exit; @@ -53,6 +58,10 @@ if (!isset($_GET['code'])) { } else { try { + + // Optional, only required when PKCE is enabled. + // Restore the PKCE code stored in the session. + $provider->setPkceCode($_SESSION['oauth2pkceCode']); // Try to get an access token using the authorization code grant. $accessToken = $provider->getAccessToken('authorization_code', [ @@ -90,6 +99,31 @@ if (!isset($_GET['code'])) { } ``` +### Authorization Code Grant with PKCE + +To enable PKCE (Proof Key for Code Exchange) you can set the `pkceMethod` option for the provider. +Supported methods are: +- `S256` Recommended method. The code challenge will be hashed with sha256. +- `plain` **NOT** recommended. The code challenge will be sent as plain text. Only use this if no other option is possible. + +You can configure the PKCE method as follows: +```php +$provider = new \League\OAuth2\Client\Provider\GenericProvider([ + // ... + // other options + // ... + 'pkceMethod' => \League\OAuth2\Client\Provider\GenericProvider::PKCE_METHOD_S256 +]); +``` +The PKCE code needs to be used between requests and therefore be saved and restored, usually via the session. +In the [example](#authorization-code-grant-example) above this is done as follows: +```php +// Store the PKCE code after the `getAuthorizationUrl()` call. +$_SESSION['oauth2pkceCode'] = $provider->getPkceCode(); +// ... +// Restore the PKCE code before the `getAccessToken()` call. +$provider->setPkceCode($_SESSION['oauth2pkceCode']); +``` Refreshing a Token ------------------ diff --git a/src/Provider/AbstractProvider.php b/src/Provider/AbstractProvider.php index d1679998..65a49ab8 100644 --- a/src/Provider/AbstractProvider.php +++ b/src/Provider/AbstractProvider.php @@ -17,6 +17,7 @@ use GuzzleHttp\Client as HttpClient; use GuzzleHttp\ClientInterface as HttpClientInterface; use GuzzleHttp\Exception\BadResponseException; +use InvalidArgumentException; use League\OAuth2\Client\Grant\AbstractGrant; use League\OAuth2\Client\Grant\GrantFactory; use League\OAuth2\Client\OptionProvider\OptionProviderInterface; @@ -58,6 +59,19 @@ abstract class AbstractProvider */ const METHOD_POST = 'POST'; + /** + * @var string PKCE method used to fetch authorization token. + * The PKCE code challenge will be hashed with sha256 (recommended). + */ + const PKCE_METHOD_S256 = 'S256'; + + /** + * @var string PKCE method used to fetch authorization token. + * The PKCE code challenge will be sent as plain text, this is NOT recommended. + * Only use `plain` if no other option is possible. + */ + const PKCE_METHOD_PLAIN = 'plain'; + /** * @var string */ @@ -78,6 +92,11 @@ abstract class AbstractProvider */ protected $state; + /** + * @var string + */ + protected $pkceCode; + /** * @var GrantFactory */ @@ -264,6 +283,32 @@ public function getState() return $this->state; } + /** + * Set the value of the pkceCode parameter. + * + * When using PKCE this should be set before requesting an access token. + * + * @param string $pkceCode + * @return self + */ + public function setPkceCode($pkceCode) + { + $this->pkceCode = $pkceCode; + return $this; + } + + /** + * Returns the current value of the pkceCode parameter. + * + * This can be accessed by the redirect handler during authorization. + * + * @return string + */ + public function getPkceCode() + { + return $this->pkceCode; + } + /** * Returns the base URL for authorizing a client. * @@ -305,6 +350,27 @@ protected function getRandomState($length = 32) return bin2hex(random_bytes($length / 2)); } + /** + * Returns a new random string to use as PKCE code_verifier and + * hashed as code_challenge parameters in an authorization flow. + * Must be between 43 and 128 characters long. + * + * @param int $length Length of the random string to be generated. + * @return string + */ + protected function getRandomPkceCode($length = 64) + { + return substr( + strtr( + base64_encode(random_bytes($length)), + '+/', + '-_' + ), + 0, + $length + ); + } + /** * Returns the default scopes used by this provider. * @@ -326,6 +392,14 @@ protected function getScopeSeparator() return ','; } + /** + * @return string|null + */ + protected function getPkceMethod() + { + return null; + } + /** * Returns authorization parameters based on provided options. * @@ -355,6 +429,26 @@ protected function getAuthorizationParameters(array $options) // Store the state as it may need to be accessed later on. $this->state = $options['state']; + $pkceMethod = $this->getPkceMethod(); + if (!empty($pkceMethod)) { + $this->pkceCode = $this->getRandomPkceCode(); + if ($pkceMethod === static::PKCE_METHOD_S256) { + $options['code_challenge'] = trim( + strtr( + base64_encode(hash('sha256', $this->pkceCode, true)), + '+/', + '-_' + ), + '=' + ); + } elseif ($pkceMethod === static::PKCE_METHOD_PLAIN) { + $options['code_challenge'] = $this->pkceCode; + } else { + throw new InvalidArgumentException('Unknown PKCE method "' . $pkceMethod . '".'); + } + $options['code_challenge_method'] = $pkceMethod; + } + // Business code layer might set a different redirect_uri parameter // depending on the context, leave it as-is if (!isset($options['redirect_uri'])) { @@ -532,6 +626,10 @@ public function getAccessToken($grant, array $options = []) 'redirect_uri' => $this->redirectUri, ]; + if (!empty($this->pkceCode)) { + $params['code_verifier'] = $this->pkceCode; + } + $params = $grant->prepareRequestParameters($params, $options); $request = $this->getAccessTokenRequest($params); $response = $this->getParsedResponse($request); diff --git a/src/Provider/GenericProvider.php b/src/Provider/GenericProvider.php index 74393ffd..80d3c91a 100644 --- a/src/Provider/GenericProvider.php +++ b/src/Provider/GenericProvider.php @@ -78,6 +78,11 @@ class GenericProvider extends AbstractProvider */ private $responseResourceOwnerId = 'id'; + /** + * @var string + */ + private $pkceMethod = null; + /** * @param array $options * @param array $collaborators @@ -114,6 +119,7 @@ protected function getConfigurableOptions() 'responseCode', 'responseResourceOwnerId', 'scopes', + 'pkceMethod', ]); } @@ -205,6 +211,14 @@ protected function getScopeSeparator() return $this->scopeSeparator ?: parent::getScopeSeparator(); } + /** + * @inheritdoc + */ + protected function getPkceMethod() + { + return $this->pkceMethod ?: parent::getPkceMethod(); + } + /** * @inheritdoc */ diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index bf24ba38..c9c14eaa 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -332,6 +332,106 @@ public function testAuthorizationStateIsRandom() } } + public function testSetGetPkceCode() + { + $pkceCode = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_'; + + $provider = $this->getMockProvider(); + $this->assertEquals($provider, $provider->setPkceCode($pkceCode)); + $this->assertEquals($pkceCode, $provider->getPkceCode()); + } + + /** + * @dataProvider pkceMethodProvider + */ + public function testPkceMethod($pkceMethod, $pkceCode, $expectedChallenge) + { + $provider = $this->getMockProvider(); + $provider->setPkceMethod($pkceMethod); + $provider->setFixedPkceCode($pkceCode); + + $url = $provider->getAuthorizationUrl(); + $this->assertSame($pkceCode, $provider->getPkceCode()); + + parse_str(parse_url($url, PHP_URL_QUERY), $qs); + $this->assertArrayHasKey('code_challenge', $qs); + $this->assertArrayHasKey('code_challenge_method', $qs); + $this->assertSame($pkceMethod, $qs['code_challenge_method']); + $this->assertSame($expectedChallenge, $qs['code_challenge']); + + // Simulate re-initialization of provider after authorization request + $provider = $this->getMockProvider(); + + $raw_response = ['access_token' => 'okay', 'expires' => time() + 3600, 'resource_owner_id' => 3]; + $stream = Mockery::mock(StreamInterface::class); + $stream + ->shouldReceive('__toString') + ->once() + ->andReturn(json_encode($raw_response)); + + $response = Mockery::mock(ResponseInterface::class); + $response + ->shouldReceive('getBody') + ->once() + ->andReturn($stream); + $response + ->shouldReceive('getHeader') + ->once() + ->with('content-type') + ->andReturn('application/json'); + + $client = Mockery::spy(ClientInterface::class, [ + 'send' => $response, + ]); + $provider->setHttpClient($client); + + // restore $pkceCode (normally done by client from session) + $provider->setPkceCode($pkceCode); + + $provider->getAccessToken('authorization_code', ['code' => 'mock_authorization_code']); + + $client + ->shouldHaveReceived('send') + ->once() + ->withArgs(function ($request) use ($pkceCode) { + parse_str((string)$request->getBody(), $body); + return $body['code_verifier'] === $pkceCode; + }); + } + + public function pkceMethodProvider() + { + return [ + [ + AbstractProvider::PKCE_METHOD_S256, + '1234567890123456789012345678901234567890', + 'pOvdVBRUuEzGcMnx9VCLr2f_0_5ZuIMmeAh4H5kqCx0', + ], + [ + AbstractProvider::PKCE_METHOD_PLAIN, + '1234567890123456789012345678901234567890', + '1234567890123456789012345678901234567890', + ], + ]; + } + + public function testPkceCodeIsRandom() + { + $last = null; + $provider = $this->getMockProvider(); + $provider->setPkceMethod('S256'); + + for ($i = 0; $i < 100; $i++) { + // Repeat the test multiple times to verify code_challenge changes + $url = $provider->getAuthorizationUrl(); + + parse_str(parse_url($url, PHP_URL_QUERY), $qs); + $this->assertTrue(1 === preg_match('/^[a-zA-Z0-9-_]{43}$/', $qs['code_challenge'])); + $this->assertNotSame($qs['code_challenge'], $last); + $last = $qs['code_challenge']; + } + } + public function testErrorResponsesCanBeCustomizedAtTheProvider() { $provider = new MockProvider([ diff --git a/test/src/Provider/Fake.php b/test/src/Provider/Fake.php index b0bedcbf..7a02b51a 100644 --- a/test/src/Provider/Fake.php +++ b/test/src/Provider/Fake.php @@ -14,6 +14,10 @@ class Fake extends AbstractProvider private $accessTokenMethod = 'POST'; + private $pkceMethod = null; + + private $fixedPkceCode = null; + public function getClientId() { return $this->clientId; @@ -59,6 +63,26 @@ public function getAccessTokenMethod() return $this->accessTokenMethod; } + public function setPkceMethod($method) + { + $this->pkceMethod = $method; + } + + public function getPkceMethod() + { + return $this->pkceMethod; + } + + public function setFixedPkceCode($code) + { + return $this->fixedPkceCode = $code; + } + + protected function getRandomPkceCode($length = 64) + { + return $this->fixedPkceCode ?: parent::getRandomPkceCode($length); + } + protected function createResourceOwner(array $response, AccessToken $token) { return new Fake\User($response); diff --git a/test/src/Provider/GenericProviderTest.php b/test/src/Provider/GenericProviderTest.php index ae3214f3..65fe93ca 100644 --- a/test/src/Provider/GenericProviderTest.php +++ b/test/src/Provider/GenericProviderTest.php @@ -55,6 +55,7 @@ public function testConfigurableOptions() 'responseCode' => 'mock_code', 'responseResourceOwnerId' => 'mock_response_uid', 'scopes' => ['mock', 'scopes'], + 'pkceMethod' => 'S256', ]; $provider = new GenericProvider($options + [ @@ -88,6 +89,10 @@ public function testConfigurableOptions() $getScopeSeparator = $reflection->getMethod('getScopeSeparator'); $getScopeSeparator->setAccessible(true); $this->assertEquals($options['scopeSeparator'], $getScopeSeparator->invoke($provider)); + + $getPkceMethod = $reflection->getMethod('getPkceMethod'); + $getPkceMethod->setAccessible(true); + $this->assertEquals($options['pkceMethod'], $getPkceMethod->invoke($provider)); } public function testResourceOwnerDetails() From d6c6de0a921e88a11ce3833a9a95aceace440224 Mon Sep 17 00:00:00 2001 From: Rutger Hertogh Date: Thu, 8 Sep 2022 21:01:22 +0200 Subject: [PATCH 2/4] Added tests for missing PKCE coverage --- test/src/Provider/AbstractProviderTest.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index c9c14eaa..b9ebf6f1 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -415,6 +415,15 @@ public function pkceMethodProvider() ]; } + public function testInvalidPkceMethod() + { + $provider = $this->getMockProvider(); + $provider->setPkceMethod('non-existing'); + + $this->expectExceptionMessage('Unknown PKCE method "non-existing".'); + $provider->getAuthorizationUrl(); + } + public function testPkceCodeIsRandom() { $last = null; @@ -432,6 +441,13 @@ public function testPkceCodeIsRandom() } } + public function testPkceMethodIsDisabledByDefault() + { + $provider = $this->getAbstractProviderMock(); + $pkceMethod = $provider->getPkceMethod(); + $this->assertNull($pkceMethod); + } + public function testErrorResponsesCanBeCustomizedAtTheProvider() { $provider = new MockProvider([ From 55cdf43d313c3b508497e072a2620025349bedbd Mon Sep 17 00:00:00 2001 From: rhertogh Date: Fri, 9 Sep 2022 22:36:50 +0200 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Ben Ramsey --- src/Provider/AbstractProvider.php | 2 +- src/Provider/GenericProvider.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Provider/AbstractProvider.php b/src/Provider/AbstractProvider.php index 65a49ab8..20e7d93f 100644 --- a/src/Provider/AbstractProvider.php +++ b/src/Provider/AbstractProvider.php @@ -302,7 +302,7 @@ public function setPkceCode($pkceCode) * * This can be accessed by the redirect handler during authorization. * - * @return string + * @return string|null */ public function getPkceCode() { diff --git a/src/Provider/GenericProvider.php b/src/Provider/GenericProvider.php index 80d3c91a..0fc95f25 100644 --- a/src/Provider/GenericProvider.php +++ b/src/Provider/GenericProvider.php @@ -79,7 +79,7 @@ class GenericProvider extends AbstractProvider private $responseResourceOwnerId = 'id'; /** - * @var string + * @var string|null */ private $pkceMethod = null; From f4d27c7a0ff36a18f19d6e5431dadeb7b8d9946b Mon Sep 17 00:00:00 2001 From: rhertogh Date: Fri, 9 Sep 2022 22:37:33 +0200 Subject: [PATCH 4/4] Update src/Provider/AbstractProvider.php Co-authored-by: Ben Ramsey --- src/Provider/AbstractProvider.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Provider/AbstractProvider.php b/src/Provider/AbstractProvider.php index 20e7d93f..2263e9da 100644 --- a/src/Provider/AbstractProvider.php +++ b/src/Provider/AbstractProvider.php @@ -93,9 +93,9 @@ abstract class AbstractProvider protected $state; /** - * @var string + * @var string|null */ - protected $pkceCode; + protected $pkceCode = null; /** * @var GrantFactory