SAML-Toolkits / python3-saml

MIT License
690 stars 307 forks source link

Should NameID be passed to delete_session_cb in process_slo? #285

Closed mateuszmandera closed 1 year ago

mateuszmandera commented 2 years ago

As far as I understand, a LogoutRequest does not need to delivered to the Service Provider in a way that carries a session cookie for the user that's supposed to be logged out. In other words, the IdP could deliver a LogoutRequest for the logout of user A e.g. in a back-channel way, so that the target user cannot be figured out by looking at the session cookie, and thus the NameID needs to be used to determine who to log out.

Also, a LogoutRequest that doesn't specify SessionIndex means that ALL sessions for the user should be terminated:

https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf

If no elements are supplied, then all sessions associated with the principal MUST be invalidated.

For which again, the NameID likely has to be inspected.

process_slo is structured in a way that ignores the NameID:

            logout_request = self.logout_request_class(self._settings, get_data['SAMLRequest'])
            self._last_request = logout_request.get_xml()
            if not self.validate_request_signature(get_data):
                self._errors.append("invalid_logout_request_signature")
                self._errors.append('Signature validation failed. Logout Request rejected')
            elif not logout_request.is_valid(self._request_data):
                self._errors.append('invalid_logout_request')
                self._error_reason = logout_request.get_error()
            else:
                if not keep_local_session:
                    OneLogin_Saml2_Utils.delete_local_session(delete_session_cb)

Should this perhaps be addressed by e.g. passing the NameID as an argument to delete_session_cb?

pitbulk commented 2 years ago

At the delete_session_cb you can do whatever you want, and your callback can include args.

Another alternative is to call process_slo with keep_local_session=True, and then do whatever you need after the process_slo call

mateuszmandera commented 2 years ago

Another alternative is to call process_slo with keep_local_session=True, and then do whatever you need after the process_slo call

Yeah, that's the approach I've taken. However, I meant to suggest that perhaps NameID should be involved here more natively, by being passed to the callback for the sake of correctness. Yes, the code calling this can extract the NameID by using the OneLogin2_Logout_Request class and then use it when defining the callback, but that's kind of a workaround - when validating the NameID looks like it should be the default approach?

eljeffeg commented 1 year ago

I think I'm running into this issue. ADFS requires the SAML logout request to include a NameID. https://www.componentspace.com/Forums/9755/ADFS-IDP-initiated-SLO-not-working-properly?Keywords=ADFS%20Authentication%20Policies