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

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

Closed
wants to merge 2 commits into from

Conversation

reb3r
Copy link
Contributor

@reb3r reb3r commented Nov 14, 2017

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
Copy link
Owner

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
Copy link
Owner

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

@reb3r
Copy link
Contributor Author

reb3r commented Nov 15, 2017

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
Copy link
Owner

@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
Copy link
Owner

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

@bshaffer bshaffer closed this Nov 30, 2017
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.

2 participants