feat: Support for Single Logout #115
Conversation
7 similar comments
| * `:idp_slo_target_url` - The URL to which the single logout request and response should | ||
| be sent. This would be on the identity provider. Optional. | ||
|
|
||
| * `:default_relay_state` - The URL to use as default RelayState on single logouts. The |
There was a problem hiding this comment.
I'm not sure it's accurate to say that the RelayState is a URL in all cases.
Also, can we code-format "RelayState" with backticks?
| ## Single Logout | ||
|
|
||
| Single Logout can be Service Provider initiated or Identity Provider initiated. | ||
| The SP initiated flow can be integrated in the `SessionsController#destroy`. |
There was a problem hiding this comment.
This sentence seems to start off assuming that Devise will be used with OmniAuth, which is not always the case. I think it should say something like "When using Devise, the SP-initiated flow can be integrated with the SessionsController#destroy action."
| Single Logout can be Service Provider initiated or Identity Provider initiated. | ||
| The SP initiated flow can be integrated in the `SessionsController#destroy`. | ||
| For this to work its important to preserve the `saml_uid` value before Devise | ||
| clears the session and redirect to the spslo subpath to initiate the single logout. |
There was a problem hiding this comment.
This should say "the /spslo subpath" I think.
There was a problem hiding this comment.
@supernova32 This looks good to merge 👍
It's a bummer that this is going to conflict with #101, but I don't see a better way to structure this code to avoid that.
|
@supernova32 I should have been more clear. I think the |
|
@md5 don't worry. I did get that 😄 I'll update the README tomorrow. |
suprnova32
force-pushed
the
single-logout-support
branch
from
aa8ea8d
to
c6e688a
Compare
|
@md5 I updated the README. Let me know what you think. |
| * `:idp_slo_target_url` - The URL to which the single logout request and response should | ||
| be sent. This would be on the identity provider. Optional. | ||
|
|
||
| * `:default_relay_state` - The value to use as default `RelayState` on single log outs. The |
There was a problem hiding this comment.
Now that I read this again, I think it's weird that :default_relay_state setting is only used for SLO, despite the fact that the RelayState mechanism applies to SSO as well as SLO (depending on the binding). It seems like it should be named :slo_default_relay_state or else it should be used in other places as well.
Also, I think this documentation should cover the fact that the :default_relay_state (or whatever it is called) can be a Proc (or other object responding to call) and that the request instance will be passed to this callable if it has an arity of 1.
|
|
||
| # Generate a response to the IdP. | ||
| logout_request_id = logout_request.id | ||
| logout_response = OneLogin::RubySaml::SloLogoutresponse.new.create(settings, logout_request_id, nil, RelayState: relay_state) |
There was a problem hiding this comment.
I'm wondering if it makes sense to add the RelayState parameter when relay_state returns a blank string or nil. This applies below to generate_logout_request as well.
| ## Single Logout | ||
|
|
||
| Single Logout can be Service Provider initiated or Identity Provider initiated. | ||
| When using Devise as authentication solution, the SP initiated flow can be integrated |
There was a problem hiding this comment.
"as an authentication solution"
| When using Devise as authentication solution, the SP initiated flow can be integrated | ||
| in the `SessionsController#destroy` action. | ||
|
|
||
| For this to work its important to preserve the `saml_uid` value before Devise |
There was a problem hiding this comment.
"it's important" or "it is important"
|
bump @supernova32 |
|
@md5 sorry. I'm a bit swamped at work. I'll take a look at your comments tomorrow. |
suprnova32
force-pushed
the
single-logout-support
branch
from
c6e688a
to
65ec2ef
Compare
|
@md5 i addressed your comments. it's a shame that I cannot resolve a discussion that is no longer valid, or that GitHub does not tell you that that discussion was started on an outdated diff. ¯**(ツ)**/¯ |
suprnova32
force-pushed
the
single-logout-support
branch
from
65ec2ef
to
b306ab1
Compare
|
@md5 any chance you can sign off on this today? I'd like to cut the new gem over the weekend so we can release the new version with GitLab 8.13. Thanks! |
| @@ -44,10 +45,6 @@ def request_phase | |||
| end | |||
|
|
|||
| def callback_phase | |||
| unless request.params['SAMLResponse'] | |||
| raise OmniAuth::Strategies::SAML::ValidationError.new("SAML response missing") | |||
There was a problem hiding this comment.
This check still needs to happen here. In a couple of lines, there's a call to response_fingerprint, which will blow up (because of a nil argument to Base64.decode64) if there's no SAMLResponse param.
There was a problem hiding this comment.
(or possibly the check should be in response_fingerprint itself)
There was a problem hiding this comment.
Er, that only applies if someone has supplied the idp_cert_fingerprint_validator option.
There was a problem hiding this comment.
@ErinCall: Nice catch!
Since that's the only call to response_fingerprint, it's probably safe to check for request.params['SAMLResponse'] in that method and return false or nil if is is not present. This will cause the code to raise OmniAuth::Strategies::SAML::ValidationError as it would have previous to this PR.
In general, I think this idp_cert_fingerprint_validator code needs to be deprecated and eventually removed, but it's possible I don't understand the use case. I think it's a bad idea to have ominiauth-saml directly decoding the SAMLResponse instead of relying on ruby-saml to do so in a consistent way.
|
@supernova32 I'll should be able to spare some time this weekend to review this again if you get the |
suprnova32
force-pushed
the
single-logout-support
branch
from
b306ab1
to
cd3fc43
Compare
suprnova32
dismissed
md5’s stale review
Changes needed have been implemented.
|
@ErinCall thanks for the review. You are absolutely right. I added a guard clause to the @md5 I also cleaned the code up a bit and added some line breaks to make the code easier to read. If you have time today or tomorrow, I'd love to get this merged 😄 |
1 similar comment
Replaces #99, as the author there has not responded and we would really like to have this feature for version 1.7.0.
Fixes #81
@md5 can you help me with a review here? I tried solving the conflicts to the best of my abilities, and the tests are green, but I really need more eyes here to make sure the code is sound.
cc @bufferoverflow