bshaffer / oauth2-server-php-docs

documentation for the oauth2-server-php library
231 stars 148 forks source link

Add openid grant to server and fix .travis.yml #101

Closed reb3r closed 6 years ago

reb3r commented 6 years ago

Updated documentation for OpenID Connect to add grant type.

Fixes #74. This will help a lot of people struggling with this missing line of code.

Also added Ruby version to Travis CI config to get builds working again.

bshaffer commented 6 years ago

This step isn't necessary, though. As long as use_openid_connect is set to true and your server has a storage class implementing AuthorizationCode (which should be any of them), then the OAuth2\OpenID\GrantType\AuthorizationCode will be added automatically (see here).

bshaffer commented 6 years ago

I've added your travis fix: 61f6df4feab8f85c5a872e4ae153f3eef9b2b611 Thank you!

reb3r commented 6 years ago

Thank you for pointing me the right direction!

My issue was that I added grant types manually to the server object. In that case the method here is never called.

Is it best practice to rely on that method or should the grant types added manually using the addGrantType-method?

I wonder if there should be a note that the grant types must not be added to work in the example provided in documentation.

bshaffer commented 6 years ago

@c-reber If you use the addGrantTypes method, but add the proper OpenID classes, then it'll work. But yes, it's a little confusing. The Server class essentially tries to set everything up intelligently and if you do things manually such as addGrantTypes then this behavior no longer takes place.

I agree it can get confusing. One way to improve this would be to change it (in the next major version) so everything is set up initially (instead of lazy-loading later). Another way would be to document the behavior, as you have mentioned. I would definitely be open to a solid improvement in the documentation such as the note you mention.

bshaffer commented 6 years ago

Closing this for now, since there's nothing left to merge in this PR.