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

Conversation

aaronpk
Copy link
Contributor

@aaronpk aaronpk commented Feb 11, 2021

This PR adds support for the OAuth 2.0 PKCE extension, as discussed in #483. This extension is recommended in the latest version of the OAuth 2.0 Security Best Current Practice.

  • Adds the PKCE logic in the core OAuth 2 provider
  • Enables PKCE with Google
  • Other providers can enable PKCE by setting the new usesPKCE property on the provider
  • The default value is false, which is the safer way to handle backwards compatibility, even though technically OAuth providers are supposed to ignore unknown parameters so it should be theoretically possible to always send the PKCE parameters anyway

* adds the PKCE logic in the core OAuth 2 provider
* enables PKCE with Google
$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.

@driesvints driesvints changed the title add support for OAuth 2.0 PKCE extension [5.x] Add support for OAuth 2.0 PKCE extension Feb 11, 2021
src/Two/AbstractProvider.php Outdated Show resolved Hide resolved
src/Two/AbstractProvider.php Outdated Show resolved Hide resolved
tests/Fixtures/OAuthTwoWithPKCETestProviderStub.php Outdated Show resolved Hide resolved
tests/OAuthTwoTest.php Outdated Show resolved Hide resolved
tests/OAuthTwoTest.php Outdated Show resolved Hide resolved
tests/OAuthTwoTest.php Outdated Show resolved Hide resolved
tests/OAuthTwoTest.php Outdated Show resolved Hide resolved
tests/OAuthTwoTest.php Outdated Show resolved Hide resolved
@taylorotwell taylorotwell merged commit 647394d into laravel:5.x Feb 11, 2021
@driesvints
Copy link
Member

Thanks @aaronpk. A PR to the docs is appreciated as well 🙂

@ijansch
Copy link

ijansch commented Feb 11, 2021

@taylorotwell please check the above comments, the merged version contains a couple of typo's as per @wietsewarendorff's comment.

@driesvints
Copy link
Member

@aaronpk could you maybe send in a follow up pr for those typos? Thanks for mentioning that @ijansch

@aaronpk
Copy link
Contributor Author

aaronpk commented Feb 11, 2021

Here's the fix: #520

taylorotwell pushed a commit that referenced this pull request Feb 12, 2021
* fix PKCE for token request (follow up on #518)

* adds test for PKCE token endpoint request

* make the linter happy
@driesvints
Copy link
Member

Follow up for stateless apps here: #523

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants