saml: Add OneLogin_Saml2_ValidationError to caught parsing exceptions.

This is an exception that we should be generally catching like the
others, which will give our standard /login/ redirect and proper logging
- as opposed to a 500 if we don't catch.

Addresses directly a bug we occurred in the wild, where a SAMLResponse
was submitted without issuers specified in a valid way, causing this
exception. The added test tests this specific type of scenario.
This commit is contained in:
Mateusz Mandera 2023-09-27 01:16:15 +02:00 committed by Tim Abbott
parent aa8d3a0c6c
commit a095c34503
2 changed files with 40 additions and 2 deletions

View File

@ -2717,6 +2717,39 @@ class SAMLAuthBackendTest(SocialAuthBase):
],
)
# Now test a slightly different case, where instead of an invalid issuer,
# the issuer is not included in the SAMLResponse at all.
# self.generate_saml_response doesn't support something as fiddly as a SAMLResponse
# with no issuers and it doesn't make sense to clutter it with such a "feature", so
# we manually load and tweak the fixture here.
unencoded_saml_response = self.fixture_data("samlresponse.txt", type="saml").format(
email=self.example_email("hamlet"),
first_name="King",
last_name="Hamlet",
extra_attrs="",
)
# Simple regex to remove Issuer occurrences from the XML.
unencoded_saml_response_without_issuer = re.sub(
r"\<saml2:Issuer.+</saml2:Issuer>", "", unencoded_saml_response
)
# SAMLResponse needs to be base64-encoded.
saml_response_without_issuer: str = base64.b64encode(
unencoded_saml_response_without_issuer.encode()
).decode()
with self.assertLogs(self.logger_string, level="INFO") as m:
post_params = {"RelayState": relay_state, "SAMLResponse": saml_response_without_issuer}
result = self.client_post("/complete/saml/", post_params)
self.assertEqual(result.status_code, 302)
self.assertEqual("/login/", result["Location"])
self.assertEqual(
m.output,
[
"ERROR:zulip.auth.saml:Error parsing SAMLResponse: Issuer of the Assertion not found or multiple.",
"INFO:zulip.auth.saml:/complete/saml/: No valid IdP as issuer of the SAMLResponse.",
],
)
def test_social_auth_complete_valid_get_idp_bad_samlresponse(self) -> None:
"""
This tests for a hypothetical scenario where our basic parsing of the SAMLResponse

View File

@ -51,7 +51,7 @@ from django_auth_ldap.backend import LDAPBackend, _LDAPUser, ldap_error
from lxml.etree import XMLSyntaxError
from onelogin.saml2 import compat as onelogin_saml2_compat
from onelogin.saml2.auth import OneLogin_Saml2_Auth
from onelogin.saml2.errors import OneLogin_Saml2_Error
from onelogin.saml2.errors import OneLogin_Saml2_Error, OneLogin_Saml2_ValidationError
from onelogin.saml2.logout_request import OneLogin_Saml2_Logout_Request
from onelogin.saml2.logout_response import OneLogin_Saml2_Logout_Response
from onelogin.saml2.response import OneLogin_Saml2_Response
@ -2250,7 +2250,12 @@ class SAMLDocument:
for wrapping the fiddly logic of handling these SAML XML documents.
"""
SAML_PARSING_EXCEPTIONS = (OneLogin_Saml2_Error, binascii.Error, XMLSyntaxError)
SAML_PARSING_EXCEPTIONS = (
OneLogin_Saml2_Error,
OneLogin_Saml2_ValidationError,
binascii.Error,
XMLSyntaxError,
)
def __init__(self, encoded_saml_message: str, backend: "SAMLAuthBackend") -> None:
"""