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 explicit support for JRuby #234

Merged
merged 16 commits into from Aug 12, 2015

Conversation

ylansegal
Copy link
Contributor

Dear maintainers, I would like to explicitly add support for JRuby to this gem. I was able to make the necessary changes, as follows:

  1. Some stubs where causing some issues that are resolved by either explicitly un-stubbing or stubbing a particular instance, as opposed to any_instance
  2. Nokogiri has it's own java extension different from the one in MRI. The 1.5 series has some bugs that have
    been resolved in 1.6 series, so I use that version when in JRuby
  3. Nokogiri has different behavior when validating XML schemas in MRI and JRuby. I already raised the issue there (Nokogiri::XML::Schema#validate behaves differently in MRI and JRuby sparklemotion/nokogiri#1282). To get around it, I removed the memoization that was earlier added as a performance improvement. Is this acceptable?
  4. The Nokogiri::XML::Node#canonicalize method in Nokogiri also behaves differently in JRuby vs MRI. However, that is easily solved by passing in nil (which is the default parameter) instead of [] for the inclusive_namespaces.

I believe 3 is the only real change in behavior for the gem. I'd appreciate your feedback.

Travis build: https://travis-ci.org/ylansegal/ruby-saml/builds/61692511

Thanks

@pitbulk
Copy link
Collaborator

pitbulk commented May 7, 2015

I'm glad with the fact of supporting JRuby, thanks for contribute it.
I'm ok with the code. @luisvm, @Lordnibbler what do you think?

@ylansegal
Copy link
Contributor Author

I have updated this PR, because of conflicts with the recent changes in master. What are the chances of this being merged?

Also, besides the Nokogiri mentioned above. JRuby 1.7.20 also has some change in behavior (in jruby-openssl) that breaks the tests. I have created an issue for them: jruby/jruby-openssl#42

Thanks,

@ylansegal
Copy link
Contributor Author

JRuby 9000 is out, and jruby-openssl 0.9.8 fixes some X509 cert parsing issues. I updated the pull request. I am aware that there are merge conflicts: I will gladly resolve them again if there is interest from the committers in merging this PR.

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 11, 2015

@ylansegal

Apologies for not merging your PR on time, I think that supporting JRuby makes sense since it not requires much changes and can help some of the java-customers.

Please fix conflicts, we will review the changes, test and include it on the master branch.

…port_0.9_rebase

* upstream/master: (47 commits)
  Handle empty URI references as per http://www.w3.org/TR/xmldsig-core/#sec-Same-Document; thx to @sixto for resolving a test case failure.
  support nameid in attribute values
  first attempt at adding support for scoped attributes needs additional work and tests
  Add some documentation about the soft setting parameter
  Update readme.md for 1.0.0 release
  Update date of the 1.0.0 release
  Update Readme and changelog
  Security improvement: Avoid entity expansion (XEE attacks)
  According to the xsd, the issuer has to be before the status
  Update changelog
  Fix SAML-Toolkits#244, related to PR SAML-Toolkits#243. Fix bug on metadata. Reorder KeyDescriptors
  Add logging information to README
  Allow logging to be delegated to an arbitrary Logger.
  Add tests for existing Logging functionality
  no more silent failure fetching idp metadata
  fix schema validation errors in service provider metadata
  tests to validate service provider metadata xml against the schema
  ignore gemfile.lock files in the gemfiles directory
  Prepare 1.0.0 release
  Improve compatibility with namespaces
  ...
@ylansegal
Copy link
Contributor Author

@pitbulk I've fixed the merge conflicts and added support for JRuby-1.7.21 and JRuby-9.0.0.0.

Thank you,

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 11, 2015

We will review it and get it merged this week. Thanks for contributing!.

pitbulk added a commit that referenced this pull request Aug 12, 2015
@pitbulk pitbulk merged commit 3905e83 into SAML-Toolkits:master Aug 12, 2015
@billyyarosh
Copy link
Contributor

@ylansegal does the issue you have with schema validate relate to not being able to validate on hard validation?

I do get an invalid_ticket error in JRuby but not on MRI.

response = RubySaml::Response.new(request.params['SAMLResponse'], options)

unless response.is_valid?
    raise ValidationError.new(response.errors.join('\n'))
end

@ylansegal
Copy link
Contributor Author

@keaplogik Are you seeing issues after this PR was merged? All the ones I was having where solved.

The issue with the schema validation that I mentioned in the original post was solved by not memoizing the Nokogiri::XML::Schema object, which triggers a difference in behavior between Nokogiri in MRI vs JRuby.

I am not sure what is causing your invalid_ticket

@billyyarosh
Copy link
Contributor

OK thanks. Looks to be a separate issue I may need to do some more verification on my test. Looks like a digest mismatch error.

@coveralls
Copy link

coveralls commented Jan 12, 2017

Coverage Status

Changes Unknown when pulling 7d86fe2 on ylansegal:feature/jruby_support_0.9 into ** on onelogin:master**.

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.

None yet

4 participants