v2.0: Remove settings.compress_request and compess_response parameters

[johnnyshields]

Fixes #676

Remove the settings.compress_request and settings.compress_response parameters. Set their behavior automatically based on settings.idp_sso_service_binding and settings.idp_slo_service_binding respectively. HTTP-Redirect will always use compression, while HTTP-POST will not.

(Compressing HTTP-POST is non-sensical, while most SAML services will fail if you don't compress redirects, and you may even hit the 2048 char URL limit.)

[pitbulk]

Im worried about other libraries using settings.compress_request and settings.compress_response for building its own bindings, not covered by RubySaml.

Can we instead of removing the settings, give the "default" binding behavior if no settings.compress_request and settings.compress_response was set?

[johnnyshields]

@pitbulk how about we keep the parameters methods but remove their functionality and simply have them display a "deprecated" message if you attempt to access or set them?

As of RubySaml 2.0.0, the compress_request parameter is deprecated and no longer performs any function. Please remove it from your code. The compression behavior is now automatically determined based on the idp_sso_service_binding_url parameter ("HTTP-Redirect" is compressed, "HTTP-POST" is uncompressed.)

The problem with these parameters is that POST binding MUST NOT be compressed, and Redirect binding MUST always be compressed. Any other configuration is invalid, so continuing to support these will only cause issues. See this answer: https://stackoverflow.com/a/21578282

The reason we have these parameters is that they pre-date the introduction of the idp_sso/slo_service_binding_url parameters.

(Another option is to do the above deprecation on 1.7.0, and remove entirely in 2.0.0)

[pitbulk]

My proposal:

At settings.rb, set the default values as:

:compress_request => nil,
:compress_response => nil,

Create at utils.rb an auxiliary like:

# Calculates if compression is required
# @param type [String] SAMLRequest | SAMLResponse
# @param service [String] sso | slo
# @param settings [OneLogin::RubySaml::Settings|nil] Toolkit settings 
def may_compress_message(type, service, settings):
  if settings.compress_request.present? and type == "SAMLRequest"  # Old behavior
    compress = settings.compress_request
  else if settings.compress_response.present? and type == "SAMLResponse"  # Old behavior
    compress = settings.compress_response
  else if service == "sso"
    compress = is_binding_redirect(settings.idp_sso_service_binding)
  else if service == "slo"
    compress = is_binding_redirect(settings.idp_slo_service_binding)
  else 
    compress = false
  return compress

def is_binding_redirect(settings_binding):
  return settings_binding == Utils::BINDINGS[:redirect]

Then in authrequest.rb for example:

request = deflate(request) if Utils::may_compress_message("SAMLRequest" , "sso", settings)
  
  ...
  
if Utils::is_binding_redirect(settings.idp_sso_service_binding) && settings.security[:authn_requests_signed] && sp_signing_key

Or at slo_logoutresponse.rb

response = deflate(response) if Utils::may_compress_message("SAMLResponse" , "slo", settings)
  
  ...
  
if Utils::is_binding_redirect(settings.idp_slo_service_binding) && settings.security[:logout_responses_signed] && sp_signing_key

Also, Leave but add a deprecation warning to:

# Deflate, base64 encode and url-encode a SAML Message (To be used in the HTTP-redirect binding)
# @param saml [String] The plain SAML Message
# @param settings [OneLogin::RubySaml::Settings|nil] Toolkit settings
# @return [String] The deflated and encoded SAML Message (encoded if the compression is requested)
#
def encode_raw_saml(saml, settings)
  saml = deflate(saml) if settings.compress_request

  CGI.escape(encode(saml))
end

And create a new

# Deflate (or not), base64 encode and url-encode a SAML Message (To be used in the HTTP-redirect binding)
# @param saml [String] The plain SAML Message
# @param compress [true|false] Whether or not the SAML should be deflated.
# @return [String] The SAML Message (deflated if requested) base64 encode and url-encode
#
def encode_raw_saml_message(saml, compress)
  saml = deflate(saml) if compress

  CGI.escape(encode(saml))
end

That way, we don't break code using encode_raw_saml, and we don't break code that extends this library and, for example, implements HTTP-Artifact binding and uses settings.compress_request and settings.compress_response.

We are introducing a lot of refactoring in 2.X and I guess people will like to jump to this branch, but if we introduce a lot of changes and do not maintain backward compatibility, we will have the risk of not having the right adoption and project with low maintenance will stay forever in 1.X

[johnnyshields]

@pitbulk Is your concern is specifically about the encode_raw_saml method?

If so, how about this:

1. Leave in Settings.compress_request, but make it do nothing as far the core SAML-flow behavior (AuthnRequests, etc.)
2. Change encode_raw_saml_message as follows:

def encode_raw_saml(saml, settings_or_compress)
  saml = deflate(saml) if settings_or_compress.is_a?(TrueClass)

  unless settings_or_compress.respond_to?(:compress_request) && settings_or_compress.compress_request.nil?
    log_deprecation_warning("[DEPRECATION WARNING] Please change the second argument of `encode_raw_saml_message` to a boolean indicating whether or not to use compression. Using a boolean will be required in RubySaml 2.1.0.")
    saml = deflate(saml) if settings_or_compress.compress_request
  end

  CGI.escape(encode(saml))
end

3. Remove Settings.compress_response as it does not affect encode_raw_saml at all. (Or keep it, but have it raise a deprecation message if you set it.)

(It should be noted that encode_raw_saml is not used anywhere in the actual RubySaml code, it's purely an external-facing Util function.)

I am going to insist that aside from preserving the behavior of encode_raw_saml, we really need to remove these parameters entirely.

[pitbulk]

My concern is that people could be using Settings.compress_request and Settings.compress_response at its code.
We can mark it as deprecated in 2.0 and maybe remove in future 2.1

Are you against using the aux may_compress_message? In 2.1, if we remove compress_request and compress_response, we can refactor this aux method without touching the rest of the code.

I'm ok with the encode_raw_saml idea you proposed, but maybe:

def encode_raw_saml(saml, settings_or_compress)
  if settings_or_compress.is_a?(TrueClass)
    saml = deflate(saml) if settings_or_compress
  else
    log_deprecation_warning("[DEPRECATION WARNING] Please change the second argument of `encode_raw_saml_message` to a boolean indicating whether or not to use compression. Using a boolean will be required in RubySaml 2.1.0.")

    if settings_or_compress.respond_to?(:compress_request)
      saml = deflate(saml) if settings_or_compress.compress_request
    end
  end
  CGI.escape(encode(saml))
end

And later in 2.1, we simply:

def encode_raw_saml(compress)
  saml = deflate(saml) if compress
  CGI.escape(encode(saml))
end

[johnnyshields]

Are you against using the aux may_compress_message

I really think it's not needed. We should simply inform people to properly use the existing idp_sso/slo_service_binding params, rather than making a new may_compress_message method that requires them to think about legacy, soon-to-be-removed compress_ params. Assuming they are setting idp_sso/slo_service_binding properly, their app won't break.

I'm ok with the encode_raw_saml idea you proposed, but maybe

Your code raises a dep message if settings_or_compress is false or nil. Correcting my proposed code here:

def encode_raw_saml(saml, settings_or_compress)
  if settings_or_compress.is_a?(TrueClass)
    saml = deflate(saml)
  elsif settings_or_compress.respond_to?(:compress_request)
    log_deprecation_warning("[DEPRECATION WARNING] Please change the second argument of `encode_raw_saml_message` to a boolean indicating whether or not to use compression. Using a boolean will be required in RubySaml 2.1.0.")
    saml = deflate(saml) if settings_or_compress.compress_request
  end

  CGI.escape(encode(saml))
end

And later in 2.1

Yes agreed to this

[johnnyshields]

Moved to #695