feat: Add the soft option to SAML options #101
Conversation
|
This looks fine to me. It shouldn't affect existing users. I do think that the option should be documented in |
|
@md5 I've update the README to include the soft option. Let me know if you think it needs more work or if I've missed a place where it should be documented. |
| @@ -125,6 +127,9 @@ The service provider metadata used to ease configuration of the SAML SP in the I | |||
| *Note*: All attributes can also be found in an array under `auth_hash[:extra][:raw_info]`, | |||
| so this setting should only be used to map attributes that are part of the OmniAuth info hash schema. | |||
|
|
|||
| * `:soft` - Used to enable/disable the soft mode. When is set false (the | |||
There was a problem hiding this comment.
Can you please put backticks around "false"? Same with "true" below.
|
Thanks @quamen. I made some minor comments on the copy in the Also, would you mind amending your commit to have |
|
@md5 I've updated the README and added There wasn't an existing style for security style warnings that I could find so I went with bold warning about security in the README documentation for the soft flag. |
| @@ -10,7 +10,7 @@ def self.inherited(subclass) | |||
| OmniAuth::Strategy.included(subclass) | |||
| end | |||
|
|
|||
| OTHER_REQUEST_OPTIONS = [:skip_conditions, :allowed_clock_drift, :matches_request_id, :skip_subject_confirmation].freeze | |||
| OTHER_REQUEST_OPTIONS = [:skip_conditions, :allowed_clock_drift, :matches_request_id, :skip_subject_confirmation, :soft].freeze | |||
There was a problem hiding this comment.
Is this line necessary? Looking at the ruby-saml source, it seems that soft gets picked up from settings.soft.
There was a problem hiding this comment.
Agreed, this should not be here. It's not necessary, since the soft option is already passed to ruby-saml in the settings hash.
There was a problem hiding this comment.
This is now required after rebasing master.
There was a problem hiding this comment.
I just made the following changes to your branch and the specs still passed:
diff --git a/lib/omniauth/strategies/saml.rb b/lib/omniauth/strategies/saml.rb
index 01f0a59..a82e3c4 100644
--- a/lib/omniauth/strategies/saml.rb
+++ b/lib/omniauth/strategies/saml.rb
@@ -10,7 +10,7 @@ module OmniAuth
OmniAuth::Strategy.included(subclass)
end
- OTHER_REQUEST_OPTIONS = [:skip_conditions, :allowed_clock_drift, :matches_request_id, :skip_subject_confirmation, :soft].freeze
+ OTHER_REQUEST_OPTIONS = [:skip_conditions, :allowed_clock_drift, :matches_request_id, :skip_subject_confirmation].freeze
option :name_identifier_format, nil
option :idp_sso_target_url_runtime_params, {}
@@ -29,6 +29,8 @@ module OmniAuth
}
option :slo_default_relay_state
option :uid_attribute
+ # NOTE: This setting should *never* be set to true in a production environment
+ option :soft, false
def request_phase
options[:assertion_consumer_service_url] ||= callback_url
@@ -177,7 +179,6 @@ module OmniAuth
def handle_response(raw_response, opts, settings)
response = OneLogin::RubySaml::Response.new(raw_response, opts.merge(settings: settings))
response.attributes["fingerprint"] = options.idp_cert_fingerprint
- response.soft = opts[:soft] || false
response.is_valid?
@name_id = response.name_id
@@ -214,7 +215,6 @@ module OmniAuth
# the LogoutResponse, verify it, then actually delete our session.
logout_response = OneLogin::RubySaml::Logoutresponse.new(raw_response, settings, :matches_request_id => session["saml_transaction_id"])
- logout_response.soft = false
logout_response.validate
session.delete("saml_uid")
|
I'm sorry to go back and forth on this, but the more I think about it, the more I think this shouldn't even be mentioned in the We already support passing through a bunch of properties to |
|
@bufferoverflow @supernova32 Do you guys have an opinion on this? |
|
@md5 I'm happy to change the implementation if that is what you and the other maintainers feel is best. Let me know and I'll re-work this pull request. |
|
@quamen I think the implementation is fine. I'm assuming the way you came to make this PR was like this:
Assuming that's the case, I think it's fine for other developers to discover that The only thing I think needs to be done with this is removing |
|
@md5 that's exactly how I came to make this PR. I'll wait to hear from you and the other maintainers, then I'll update the code and/or README to suit. |
|
Im ok with this, the statement within README.md is very clear. |
|
@bufferoverflow Sounds good @quamen I think the only unresolved issue in that case is my comment about |
There was a problem hiding this comment.
@quamen this looks good. Can you please address the comments we left?
@supernova32 I've removed the |
5 similar comments
|
@quamen Sorry for the delay on following up on your changes. Do you mind rebasing this PR? |
3 similar comments
|
@quamen Looks like there are some spec failures now. Mind taking a look at those? |
When setting up the providr you can now set the soft option.
provider :saml,
issuer: 'Example',
idp_sso_target_url: 'example.com/sso',
idp_cert: 'mycert',
soft: true
The default is still false, but you can now over-ride this to turn
off raising of SAML validation errors.
This is useful when testing or in environment where you don't really
want to set up a real SAML server with signed requests etc.
1 similar comment
|
@quamen Could you describe the scenario where you want to have In a testing scenario, it seems like it should be possible to generate a usable SAML response with |
|
Hey, I bump this PR as I'm interested by this option too. Why it's not merged yet? My usecase is: |
When setting up the providr you can now set the soft option.
provider :saml,
issuer: 'Example',
idp_sso_target_url: 'example.com/sso',
idp_cert: 'mycert',
soft: true
The default is still false, but you can now over-ride this to turn
off raising of SAML validation errors.
This is useful when testing or in environment where you don't really
want to set up a real SAML server with signed requests etc.