Refactor error handling; allow multiple error messages with soft

[vincentwoo]

When soft was introduced, it allowed business logic to examine failure without having to go through crazy rescue blocks. This was awesome.

However, what was not awesome was that debugging invalid responses in prod was tedious - if your server just output the errors on an object, you'd only see the first error that tripped the validator. Most of the time, there are other screwups too. This change allows errors to accumulate on an object when safe mode is set. It also centralizes some very repeated error handling code.

There are unfortunately a couple of rough spots where assumptions baked into test order right now - like nil derefs that aren't checked in later tests. I had to hack around a couple.

Also, Response's use of doc.validate_document was out of date - options is now the third parameter, but the options hash was being passed as the value for soft.

[pitbulk]

Thanks for this contribution, I will review and try to improve. Right now we have an urgent release to support better signature handle, but this will be introduced in other release.

[vincentwoo]

ping. i also think that this change hugely improves the experience of debugging with ruby-saml

[pitbulk]

The idea of stopping validation after find the first error is based on speed, also rest of the SAML toolkits has this behavior.

If you want to validate a SAMLResponse, LogoutRequest or LogoutResponse you can use samltool.com
or extend the class and create a validator method.

There is a logic followed in order to store a error on errors or raise an exception.
At errors I store SAML errors, Exceptions are raised when wrong import data or wrong settings are used.

[vincentwoo]

The idea of stopping validation after find the first error is based on speed, also rest of the SAML toolkits has this behavior.

The default rails behavior for model.valid? is to return all errors encountered. I think it's pretty much crazy to only return the first error seen, especially when the error accessor you provide is misleadingly named errors: https://github.com/onelogin/ruby-saml/blob/master/lib/onelogin/ruby-saml/response.rb#L25

If you want to validate a SAMLResponse, LogoutRequest or LogoutResponse you can use samltool.com or extend the class and create a validator method.

samltool.com does not function exactly as the toolkit does, as we both know very well at this point. As for creating a "validator" method - isn't that what this PR does?

There is a logic followed in order to store a error on errors or raise an exception.
At errors I store SAML errors, Exceptions are raised when wrong import data or wrong settings are used.

I don't really understand your English here but if you look at what ruby-saml actually does it has probably nothing to do with what you're talking about. soft is set via config and basically takes effect for ALL validations.

[pitbulk]

Thank you for being an active contributor of the ruby-saml toolkit.

samltool.com does not function exactly as the toolkit does

But is a SAML validator, you can detect any issue at any SAMLResponse, LogoutRequest or LogoutResponse, but I understand you want to validate it with the ruby-saml toolkit.

I'm not comfortable with your proposal that change the current behavior of the validate method since it can break some current environments, but if you think that collecting errors makes totally sense to you, what do you think about adding a new parameter to the validate method (collect_errors, by default set to false)

      # Validates the SAML Response (calls several validation methods)
      # @param collect_errors [Boolean] Validate all the SAMLResponse or stop validation when first error appears.
      # @return [Boolean] True if the SAML Response is valid, otherwise False if soft=True
      # @raise [ValidationError] if soft == false and validation fails
      #
      def validate(collect_errors = false)
        reset_errors!

        if collect_errors:
            return false unless validate_response_state
            validate_version
            validate_id
            validate_success_status
            validate_num_assertion
            validate_no_encrypted_attributes
            validate_signed_elements
            validate_structure
            validate_in_response_to
            validate_conditions
            validate_audience
            validate_issuer
            validate_session_expiration
            validate_subject_confirmation
            validate_signature
            @errors.empty?
      else
            validate_response_state &&
            validate_version &&
            validate_id &&
            validate_success_status &&
            validate_num_assertion &&
            validate_no_encrypted_attributes &&
            validate_signed_elements &&
            validate_structure &&
            validate_in_response_to &&
            validate_conditions &&
            validate_audience &&
            validate_destination &&
            validate_issuer &&
            validate_session_expiration &&
            validate_subject_confirmation &&
            validate_signature
      end

As you know there are some errors that may stop the validation process and are not related to the SAML message validation itself. For example, use as SAMLResponse an invalid XML
When that happens, makes no sense to keep the validation process, for example, if the toolkit is unable to load the XML, all validations should fail, but I don't need to register those fails.

I see that you noticed that, that why you set the

return false unless validate_response_state

are you ok with this approach?

[vincentwoo]

I sincerely believe that all users of your library expect, because you name your errors variable errors, for it to contain all errors the library can reasonably be made aware of it. I'm okay with your approach as a user but if I owned this library I would be very unhappy with that big if statement. I think the absolute correct thing to do is always collect errors where possible.

If it's not possible to always collect every conceivable error, or if some errors need to early-return, that is totally okay. I simply expect my libraries to make a best-effort guess at returning all possible errors, just as samltool.com does.

[pitbulk]

Merged, I will release a new gem today.