omniauth-saml feat: Add the soft option to SAML options

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.

Jun 01 '16 23:06 quamen

Coverage increased (+0.9%) to 97.736% when pulling 785a1bde2a6706d95b85d8f7c2d0b95b35ec1516 on CloverAU:feat/add-soft-option-to-saml-options into 146e46987bccd17343f96bb8b408fb1c8c84ec8d on omniauth:master.

Jun 02 '16 00:06 coveralls

This looks fine to me. It shouldn't affect existing users. I do think that the option should be documented in README.md though.

Jun 25 '16 00:06 md5

@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.

Jun 26 '16 23:06 quamen

Coverage increased (+0.9%) to 97.736% when pulling 5c6d2f2e1b9c8eb218318672e616aca2b91d71ee on CloverAU:feat/add-soft-option-to-saml-options into 146e46987bccd17343f96bb8b408fb1c8c84ec8d on omniauth:master.

Jun 26 '16 23:06 coveralls

Thanks @quamen. I made some minor comments on the copy in the README.md. I think those additions are good, but I almost feel that it needs to be more strongly worded to make it very clear that soft should not be used in a production situation.

Also, would you mind amending your commit to have chore: in the commit message? For that matter, it might be good to just squash it into the previous commit.

Jun 27 '16 16:06 md5

@md5 I've updated the README and added chore: to the beginning of the commit message.

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.

Jun 27 '16 22:06 quamen

Coverage increased (+0.9%) to 97.736% when pulling fbb9a40eab990455f9c1094d245f711d6991d439 on CloverAU:feat/add-soft-option-to-saml-options into 998eb57f12815095cfe4bba8c7d1ecb4f406c2ff on omniauth:master.

Jun 27 '16 22:06 coveralls

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 README.md.

We already support passing through a bunch of properties to ruby-saml that are not documented in the README.md, so the only thing that's really needed to support allowing soft to be used is changing the line that unilaterally sets response.soft = false to allow it to be true.

Jun 28 '16 16:06 md5

@bufferoverflow @supernova32 Do you guys have an opinion on this?

Jun 28 '16 16:06 md5

@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.

Jun 30 '16 02:06 quamen

@quamen I think the implementation is fine.

I'm assuming the way you came to make this PR was like this:

You noticed that ruby-saml supports a soft option
You tried setting soft: true in the omniauth-saml settings
You noticed that it didn't work
You looked at the omniauth-saml source code and noticed that soft is hard-coded to false

Assuming that's the case, I think it's fine for other developers to discover that soft now works in the same way (once this PR merges).

The only thing I think needs to be done with this is removing :soft from OTHER_REQUEST_OPTIONS, unless I'm missing something. I'd also like to get agreement from the other maintainers that leaving this option documented only in ruby-saml is fine.

Jun 30 '16 22:06 md5

@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.

Jun 30 '16 22:06 quamen

Im ok with this, the statement within README.md is very clear.

Jul 03 '16 13:07 bufferoverflow

@bufferoverflow Sounds good

@quamen I think the only unresolved issue in that case is my comment about OTHER_REQUEST_OPTIONS. Can you explain why you think that change is necessary or just remove it by amending your commit?

Jul 07 '16 05:07 md5

@quamen this looks good. Can you please address the comments we left?

@supernova32 I've removed the :soft option from the OTHER_REQUEST_OPTIONS array.

Sep 20 '16 22:09 quamen