Introduce thread safety to SAML schema validation

[phlipper]

Per the references below, use of Dir.chdir is not thread-safe. This usage was causing exceptions to be raised when running on Puma and in other multi-threaded environments.

This patch also moves the schema read up to a class instance, as this data is static and does not need to be read every time an assertion is validated. This boosts performance, especially in environments with higher throughput.

Thanks to @dannyb for the assistance!

References:

• https://www.ruby-forum.com/topic/165079
• https://bugs.ruby-lang.org/issues/9785
• http://www.justskins.com/forums/working-directory-in-thread-42304.html
• http://www.ruby-doc.org/core-2.1.5/Dir.html#method-c-chdir

[daniel-g]

This return will raise an error if soft. You should use break or next

[phlipper]

@daniel-g what error will this raise? If you check the previous line 20, it was doing exactly the same thing. The tests are also passing, so I'm hoping I can trust those.

My goal is not to change all the functionality, my goal is to solve the problem given the tools available.

[daniel-g]

Hi @phlipper . In ruby 2.1.2:

2.1.2 :002 > [1, 2, 3, 4].map{|f| return(f) }
LocalJumpError: unexpected return
    from (irb):2:in `block in irb_binding'
    from (irb):2:in `map'
    from (irb):2

in ruby 1.8.7:

[1, 2, 3, 4].map{|f| return(f) }
LocalJumpError: unexpected return
    from (irb):1
    from (irb):1:in `map'
    from (irb):1

So I wonder how the tests still passed. What am I missing?

[phlipper]

I guess this shows there is some test coverage lacking somewhere.

[phlipper]

@daniel-g per your suggestions, I have updated the use of break vs. return and I have eliminated some useless instance variables. How does this look to you now?

[daniel-g]

I guess @xml => xml

[daniel-g]

other than my last comments, it looks good to me 👍

[phlipper]

@daniel-g good catch on the last typos. This PR should be good to go now.

[cthornton]

Wouldn't it make sense to not even change the directory and read the file relative to __FILE__? i.e.

@schema ||= begin
  path = File.expand_path(File.join(File.dirname(__FILE__), '..', '..', 'schemas', 'saml-schema-protocol-2.0.xsd'))
  Nokogiri::XML::Schema(IO.read(path))
end

[phlipper]

@cgthornt the chdir is required due to a known issue with Nokogiri and schema validation.

References:

• http://ktulu.com.ar/blog/2011/06/26/resolving-validation-errors-using-nokogiri-and-schemas/
• https://stackoverflow.com/questions/11996326/nokogirixmlschema-syntaxerror-on-schema-load
• https://github.com/dougalcorn/nokogiri-chdir-schema-validate-bug

[Lordnibbler]

@phlipper any chance you can rebase off master and throw a test around the new method you introduced? self.schema

We updated the test suite to use Minitest and I just want to ensure its all passing still before we merge.

Thank you for this improvement!

[phlipper]

@Lordnibbler i have rebased on top of the latest master and tests are green.

[Lordnibbler]

I'm 👍 on this but would love a simple unit test around this new self.schema method

[phlipper]

@Lordnibbler what would you like to see for test coverage? I'm happy to add some, but there are no existing tests for the SamlMessage class at the moment. The subclasses are tested and passing so I felt confident in the change.

[Lordnibbler]

@phlipper We will make a followup issue to test the SamlMessage class (unless you want to throw some tests around the class too).

[Lordnibbler]

@cgthornt @pitbulk can you guys review?

[coveralls]

Changes Unknown when pulling ea8e552 on phlipper:thread-safety into * on onelogin:master*.