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

Added Response available options list #454

Merged

Conversation

mberlanda
Copy link
Contributor

I recently did a PR in omniauth-saml gem to allow more options for a OneLogin::RubySaml::Response object (omniauth/omniauth-saml#159).

After merging it, I was discussing with @md5 about the possibility to source the allowed options from ruby-saml gem instead of hardcode them in this gem.

As of today, omniauth-saml had ~2.3 million downloads and I believe that introducing a whitelist of response options will make the dependency on ruby-saml stronger and even more reliable.

Moreover, it will allow to developers who are using ruby-saml standalone to have a better overview on the options they can use.

@mberlanda mberlanda force-pushed the feat/whitelist-response-options branch from 6dc2425 to 1f9cc0a Compare March 27, 2018 09:35
@mberlanda
Copy link
Contributor Author

hi @pitbulk

I saw in the travis build that jruby-1.7.21 and jruby-9.0.0.0 are failing since a while. They might need a separated Gemfile (https://docs.travis-ci.com/user/languages/ruby/#JRuby:-C-extensions-are-not-supported).

In ree and ruby-1.8.7 an array of symbols is raising TypeError: Symbol as array index. How are you handling this data structure?

Thanks

@pitbulk
Copy link
Collaborator

pitbulk commented Mar 27, 2018

What happens to projects using ruby-saml that extended the SAMLResponse options with more options?
This PR will break their projects.

Is the whitelist of options really important?

Related to jruby, I haven't got an environment to test it.

@mberlanda
Copy link
Contributor Author

Thank you for your quick reply!

Actually the whitelist of options is a nice workaround to make sure that dependent gems such as omniauth-saml will be updated with the options offered by your gem. I haven't considered the case of someone extending the class and passing custom options.

This is not the only possible solution but it would prevent several prs on other gems which depends on yours.

Would it be suitable to you to rename the ALLOWED_OPTIONS to AVAILABLE_OPTIONS and remove the validation of the options provided? This would achieve the purpose of this PR.

@pitbulk
Copy link
Collaborator

pitbulk commented Mar 27, 2018

Yes, I think adding ALLOWED_OPTIONS is not bad idea

This change introduces a OneLogin::RubySaml::Response::AVAILABLE_OPTIONS array to be exploited by the gems depending on this one. This is not filtering the options to not break custom implementation extending this class.
@mberlanda mberlanda changed the title Added Response options whitelist Added Response available options list Mar 27, 2018
@mberlanda mberlanda force-pushed the feat/whitelist-response-options branch from 36b710f to fb17e8a Compare March 27, 2018 18:41
@mberlanda
Copy link
Contributor Author

Perfect! I changed then to AVAILABLE_OPTIONS and dropped the commit which was introducing the filter. I guess this could be ready for a review.

@mberlanda
Copy link
Contributor Author

hi @pitbulk ! Could you please bump a minor version of the gem including this PR changes?
Thank you very much

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