-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat: Add the soft option to SAML options #101
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now required after rebasing master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@quamen this looks good. Can you please address the comments we left?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once #115 is merged, this PR will need to be updated to take into account the movement of where soft
is handled. That PR also adds an additional place where soft
is explicitly set to false
in the single logout code.
@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.