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

Cast response_type to string when checking if it is set in params #36

Merged
merged 2 commits into from
Aug 25, 2019

Conversation

januszm
Copy link
Contributor

@januszm januszm commented Aug 12, 2019

Rails will provide params with indifferent access but that's not guaranteed with other frameworks.
Actually the automated test suite provides params object as a regular Hash

Omniauth sets keys as string, e.g. hash[field] = request.params[field.to_s] for the Developer strategy.

Adding strategy.options.response_type = :code alone to the test_callback_phase test is enough to trigger an error.

Rails will provide params with indifferent access but that's not guaranteed with other frameworks.
Omniauth sets keys as string, e.g. hash[field] = request.params[field.to_s]
@januszm
Copy link
Contributor Author

januszm commented Aug 12, 2019

Unfortunately I cannot spend more time on this PR, so if you guys have any suggestions, please write in comments, or close this PR and start another one with an alternative solution to handling options.response_type passed as string or symbol in general (one, that cleans up the .to_s code duplication).

@januszm
Copy link
Contributor Author

januszm commented Aug 12, 2019

@m0n9oose this can be merged without the other pull request about token verification. It's easier to review them separately.

@m0n9oose
Copy link
Collaborator

@m0n9oose this can be merged without the other pull request about token verification. It's easier to review them separately.

Okay, as you wish. Can you transfer changes from #37 related to this PR?

@krzysiek1507
Copy link
Contributor

Let me push my changes to handle this fix and id_token validation fix.

@januszm
Copy link
Contributor Author

januszm commented Aug 13, 2019

Guys this PR can be merged independently from others. If you want to apply some refactoring later, you can do it in another PR.

Regarding @m0n9oose 's idea to define the response_type method and cache the configuration value as a string, in all use cases the options.response_typevariable is or can be used as string. This means that it'd be more appropriate to change the README and propose a string variant for the option from now on.

@m0n9oose m0n9oose merged commit e05e60d into omniauth:master Aug 25, 2019
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.

3 participants