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

[8.0] Upgrade to league/oauth2-server 8.0 #1050

Merged
merged 2 commits into from
Jul 19, 2019
Merged

[8.0] Upgrade to league/oauth2-server 8.0 #1050

merged 2 commits into from
Jul 19, 2019

Conversation

matt-allan
Copy link
Contributor

This PR updates the league/oauth2-server package to 8.0 and updates Passport to be compatible with it.

The changes are documented in the upgrade guide. The things I had to change for Passport are:

  • Set the AccessToken client property when creating an access token. This isn't documented in the upgrade guide, but the server no longer calls setClient before calling getClient. To prevent a fatal error we need to set the client before returning the token from AccessTokenRepository::getNewToken.

  • Set the Client isConfidential property when creating a client. The League server checks this property to determine if it should validate the client secret. This will always be true for now; I'm going to add support for public clients in a separate PR.

  • Split the ClientRepository::getClientEntity method into two methods, getClientEntity and validateClient. The league server calls validateClient for all grants except for implicit grants and authorization code clients when the client does not have a secret.

  • Pass a DateTimeImmutable instead of DateTime to AccessToken::setExpiryDateTime. This only affected a test.

This PR adds PKCE support (#837) but it's not really useful until Passport supports public clients. As mentioned above I'm working on a separate PR to support public clients.

In addition to running the test suite I manually tested this with all of the grant types and tested PCKE support.

@matt-allan
Copy link
Contributor Author

I have support for public clients working in this branch but it will need to be rebased once this PR is merged.

@driesvints driesvints changed the title Upgrade to league/oauth2-server 8.0 [8.0] Upgrade to league/oauth2-server 8.0 Jul 18, 2019
src/Bridge/Client.php Outdated Show resolved Hide resolved
src/Bridge/Client.php Show resolved Hide resolved
@matt-allan
Copy link
Contributor Author

@driesvints I made the requested changes.

@driesvints driesvints dismissed their stale review July 19, 2019 09:55

Changes were made.

@taylorotwell taylorotwell merged commit c395a02 into laravel:master Jul 19, 2019
@matt-allan matt-allan deleted the 8.0-support branch July 19, 2019 15:03
@jeanlucnguyen
Copy link

@matt-allan Did you have a chance to rebase the branch supporting public clients since this PR has been merged?

@matt-allan
Copy link
Contributor Author

Yep, I opened a PR but closed it for now because I needed to clarify some behavior with the League oauth-server library.

I either need to make another PR to the League server or rework the Passport PR. I'm hoping to have some time for open source at the end of this week.

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.

4 participants