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

ImplicitGrant finalizes scopes without user identifier #923

Merged
merged 4 commits into from Sep 23, 2018

Conversation

Projects
None yet
2 participants
@christiaangoossens
Copy link
Contributor

christiaangoossens commented Jul 13, 2018

This PR fixes #737 and #786.
According to the documentation, the finalizeScopes() method is called: "This method is called right before an access token or authorization code is created." (https://oauth2.thephpleague.com/scope-repository-interface/)

In the Implicit Grant, the scopes were previously finalized before the AuthorizationRequest was even created: this allowed for no changes to the scopes before creating the access token, and made it impossible to finalize the scopes depending upon the user ID (#737).

To adhere to the documentation, the scopes should be finalized right before the access token is issued.

Beware: this may be a breaking change for some implementations (although it shouldn't). Although the tests didn't need any changing (except for passing in a new mock function, which is accessible in real situations anyway), it may still break active implementations.

All tests ran fine, and code coverage was the same.

@christiaangoossens christiaangoossens force-pushed the christiaangoossens:fix_implicit_grant_scopes branch from 51c06e0 to acf16e9 Jul 13, 2018

@Sephster

This comment has been minimized.

Copy link
Member

Sephster commented Sep 23, 2018

This looks good to me. I think there is a change in performance here as we are moving the finalize scopes to a later stage in the process but I don't think this is of too much concern and don't think it constitutes as a BC breaking change so happy to include it in this version. Thanks for your efforts here.

Sephster added some commits Sep 23, 2018

@Sephster Sephster merged commit 9882f67 into thephpleague:master Sep 23, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/styleci/pr The analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment