Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.x] Add support for OAuth 2.0 PKCE extension #518

Merged
merged 4 commits into from
Feb 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 66 additions & 1 deletion src/Two/AbstractProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ abstract class AbstractProvider implements ProviderContract
*/
protected $stateless = false;

/**
* Indicates if PKCE should be used.
*
* @var bool
*/
protected $usesPKCE = false;

/**
* The custom Guzzle configuration options.
*
Expand Down Expand Up @@ -158,6 +165,10 @@ public function redirect()
$this->request->session()->put('state', $state = $this->getState());
}

if ($this->usesPKCE()) {
$this->request->session()->put('code_verifier', $codeVerifier = $this->getCodeVerifier());
}

return new RedirectResponse($this->getAuthUrl($state));
}

Expand Down Expand Up @@ -192,6 +203,11 @@ protected function getCodeFields($state = null)
$fields['state'] = $state;
}

if ($this->usesPKCE()) {
$fields['code_challenge'] = $this->getCodeChallenge();
$fields['code_challenge_method'] = $this->getCodeChallengeMethod();
}

return array_merge($fields, $this->parameters);
}

Expand Down Expand Up @@ -284,13 +300,19 @@ public function getAccessTokenResponse($code)
*/
protected function getTokenFields($code)
{
return [
$fields = [
'grant_type' => 'authorization_code',
'client_id' => $this->clientId,
'client_secret' => $this->clientSecret,
'code' => $code,
'redirect_uri' => $this->redirectUrl,
];

if ($this->usesPKCE()) {
$field['code_verifier'] = $this->request->session()->pull('code_verifier');
aaronpk marked this conversation as resolved.
Show resolved Hide resolved
}

return $fields;
}

/**
Expand Down Expand Up @@ -434,6 +456,49 @@ protected function getState()
return Str::random(40);
}

/**
* Determine if the provider uses PKCE.
*
* @return bool
*/
protected function usesPKCE()
{
return $this->usesPKCE;
}

/**
* Generates a random string of the right length for the PKCE code verifier.
*
* @return string
*/
protected function getCodeVerifier()
{
return Str::random(96);
}

/**
* Generates the PKCE code challenge based on the PKCE code verifier in the session.
*
* @return string
*/
protected function getCodeChallenge()
{
$verifier = $this->request->session()->pull('code_verifier');
aaronpk marked this conversation as resolved.
Show resolved Hide resolved
$hashed = hash('sha256', $verifier, true);

return rtrim(strtr(base64_encode($hashed), '+/', '-_'), '=');
}

/**
* Returns the hash method used to calculate the PKCE code challenge
*
* @return string
*/
protected function getCodeChallengeMethod()
{
return 'S256';
}

/**
* Set the custom parameters of the request.
*
Expand Down
7 changes: 7 additions & 0 deletions src/Two/GoogleProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ class GoogleProvider extends AbstractProvider implements ProviderInterface
*/
protected $scopeSeparator = ' ';

/**
* Indicates if PKCE should be used.
*
* @var bool
*/
protected $usesPKCE = true;

/**
* The scopes being requested.
*
Expand Down
2 changes: 1 addition & 1 deletion tests/Fixtures/OAuthTwoTestProviderStub.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class OAuthTwoTestProviderStub extends AbstractProvider

protected function getAuthUrl($state)
{
return 'http://auth.url';
return $this->buildAuthUrlFromBase('http://auth.url', $state);
}

protected function getTokenUrl()
Expand Down
8 changes: 8 additions & 0 deletions tests/Fixtures/OAuthTwoWithPKCETestProviderStub.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace Laravel\Socialite\Tests\Fixtures;

class OAuthTwoWithPKCETestProviderStub extends OAuthTwoTestProviderStub
{
protected $usesPKCE = true;
}
60 changes: 57 additions & 3 deletions tests/OAuthTwoTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Illuminate\Http\Request;
use Laravel\Socialite\Tests\Fixtures\FacebookTestProviderStub;
use Laravel\Socialite\Tests\Fixtures\OAuthTwoTestProviderStub;
use Laravel\Socialite\Tests\Fixtures\OAuthTwoWithPKCETestProviderStub;
use Laravel\Socialite\Two\InvalidStateException;
use Laravel\Socialite\Two\User;
use Mockery as m;
Expand All @@ -24,17 +25,70 @@ protected function tearDown(): void
m::close();
}

public function testRedirectGeneratesTheProperIlluminateRedirectResponse()
public function testRedirectGeneratesTheProperIlluminateRedirectResponseWithoutPKCE()
{
$request = Request::create('foo');
$request->setLaravelSession($session = m::mock(Session::class));
$session->shouldReceive('put')->once();

$state = null;
$closure = function($name, $stateInput) use (&$state) {
if ($name === 'state') {
$state = $stateInput;

return true;
}

return false;
};

$session->shouldReceive('put')->once()->withArgs($closure);
$provider = new OAuthTwoTestProviderStub($request, 'client_id', 'client_secret', 'redirect');
$response = $provider->redirect();

$this->assertInstanceOf(SymfonyRedirectResponse::class, $response);
$this->assertInstanceOf(RedirectResponse::class, $response);
$this->assertSame('http://auth.url', $response->getTargetUrl());
$this->assertSame('http://auth.url?client_id=client_id&redirect_uri=redirect&scope=&response_type=code&state='.$state, $response->getTargetUrl());
}

private static $codeVerifier = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unclear on how to get this value stored in the test in a local variable, it seemed to only work when stored as a static class variable. The test needs to know the value the main library generated, and also needs to be able to return it to the main library when pulled from the session. Let me know if there is a better way to do this, it was very difficult to find any docs on how to do this kind of test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to move this into the test method itself like this:

public function testRedirectGeneratesTheProperIlluminateRedirectResponseWithPKCE()
{
    static $codeVerifier = null;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strangely that isn't working. The first closure sets that value, but then it appears to be null by the time the second closure goes to read it.


public function testRedirectGeneratesTheProperIlluminateRedirectResponseWithPKCE()
{
$request = Request::create('foo');
$request->setLaravelSession($session = m::mock(Session::class));

$state = null;
$sessionPutClosure = function($name, $value) use (&$state) {
if ($name === 'state') {
$state = $value;

return true;
} elseif ($name === 'code_verifier') {
self::$codeVerifier = $value;

return true;
}

return false;
};

$sessionPullClosure = function($name) {
if ($name === 'code_verifier') {
return self::$codeVerifier;
}
};

$session->shouldReceive('put')->twice()->withArgs($sessionPutClosure);
$session->shouldReceive('pull')->once()->with('code_verifier')->andReturnUsing($sessionPullClosure);

$provider = new OAuthTwoWithPKCETestProviderStub($request, 'client_id', 'client_secret', 'redirect');
$response = $provider->redirect();

$codeChallenge = rtrim(strtr(base64_encode(hash('sha256', self::$codeVerifier, true)), '+/', '-_'), '=');

$this->assertInstanceOf(SymfonyRedirectResponse::class, $response);
$this->assertInstanceOf(RedirectResponse::class, $response);
$this->assertSame('http://auth.url?client_id=client_id&redirect_uri=redirect&scope=&response_type=code&state='.$state.'&code_challenge='.$codeChallenge.'&code_challenge_method=S256', $response->getTargetUrl());
}

public function testUserReturnsAUserInstanceForTheAuthenticatedRequest()
Expand Down