Merge pull request #541 from skoranda/no_saml_subject_name_id

Do not require a SAML authentication response to contain a NameID element - it is not required by the SAML 2.0 specification.

Invoke add_information_about_person only when resp.assertion.subject.name_id is available.

2 files changed, 98 insertions, 42 deletions

diff --git a/src/saml2/client_base.py b/src/saml2/client_base.py
index f8704c20..d0a8e82c 100644
--- a/src/saml2/client_base.py
+++ b/src/saml2/client_base.py
@@ -676,50 +676,49 @@ class Base(Entity):
         :return: An response.AuthnResponse or None
         """
 
-        try:
-            _ = self.config.entityid
-        except KeyError:
+        if not getattr(self.config, 'entityid', None):
             raise SAMLError("Missing entity_id specification")
 
-        resp = None
-        if xmlstr:
-            kwargs = {
-                "outstanding_queries": outstanding,
-                "outstanding_certs": outstanding_certs,
-                "allow_unsolicited": self.allow_unsolicited,
-                "want_assertions_signed": self.want_assertions_signed,
-                "want_response_signed": self.want_response_signed,
-                "return_addrs": self.service_urls(binding=binding),
-                "entity_id": self.config.entityid,
-                "attribute_converters": self.config.attribute_converters,
-                "allow_unknown_attributes":
-                    self.config.allow_unknown_attributes,
-                'conv_info': conv_info
-            }
-            try:
-                resp = self._parse_response(xmlstr, AuthnResponse,
-                                            "assertion_consumer_service",
-                                            binding, **kwargs)
-            except StatusError as err:
-                logger.error("SAML status error: %s", err)
-                raise
-            except UnravelError:
-                return None
-            except Exception as err:
-                logger.error("XML parse error: %s", err)
-                raise
-
-            if resp is None:
-                return None
-            elif isinstance(resp, AuthnResponse):
-                if resp.assertion is not None and len(
-                        resp.response.encrypted_assertion) == 0:
-                    self.users.add_information_about_person(resp.session_info())
-                    logger.info("--- ADDED person info ----")
-                pass
-            else:
-                logger.error("Response type not supported: %s",
-                             saml2.class_name(resp))
+        if not xmlstr:
+            return None
+
+        kwargs = {
+            "outstanding_queries": outstanding,
+            "outstanding_certs": outstanding_certs,
+            "allow_unsolicited": self.allow_unsolicited,
+            "want_assertions_signed": self.want_assertions_signed,
+            "want_response_signed": self.want_response_signed,
+            "return_addrs": self.service_urls(binding=binding),
+            "entity_id": self.config.entityid,
+            "attribute_converters": self.config.attribute_converters,
+            "allow_unknown_attributes":
+                self.config.allow_unknown_attributes,
+            'conv_info': conv_info
+        }
+
+        try:
+            resp = self._parse_response(xmlstr, AuthnResponse,
+                                        "assertion_consumer_service",
+                                        binding, **kwargs)
+        except StatusError as err:
+            logger.error("SAML status error: %s", err)
+            raise
+        except UnravelError:
+            return None
+        except Exception as err:
+            logger.error("XML parse error: %s", err)
+            raise
+
+        if not isinstance(resp, AuthnResponse):
+            logger.error("Response type not supported: %s",
+                         saml2.class_name(resp))
+            return None
+
+        if (resp.assertion and len(resp.response.encrypted_assertion) == 0 and
+                resp.assertion.subject.name_id):
+            self.users.add_information_about_person(resp.session_info())
+            logger.info("--- ADDED person info ----")
+
         return resp
 
     # ------------------------------------------------------------------------
diff --git a/tests/test_51_client.py b/tests/test_51_client.py
index fe7e199f..8571a36a 100644
--- a/tests/test_51_client.py
+++ b/tests/test_51_client.py
@@ -736,6 +736,63 @@ class TestClient:
 
         self.verify_authn_response(idp, authn_response, _client, ava_verify)
 
+    def test_response_no_name_id(self):
+        """ Test that the SP client can parse an authentication response
+        from an IdP that does not contain a <NameID> element."""
+
+        conf = config.SPConfig()
+        conf.load_file("server_conf")
+        client = Saml2Client(conf)
+
+        # Use the same approach as the other tests for mocking up
+        # an authentication response to parse.
+        idp, ava, ava_verify, nameid_policy = (
+                self.setup_verify_authn_response()
+                )
+
+        # Mock up an authentication response but do not encrypt it
+        # nor sign it since below we will modify it directly. Note that
+        # setting name_id to None still results in a response that includes
+        # a <NameID> element.
+        resp = self.server.create_authn_response(
+            identity=ava,
+            in_response_to="id1",
+            destination="http://lingon.catalogix.se:8087/",
+            sp_entity_id="urn:mace:example.com:saml:roland:sp",
+            name_id=None,
+            userid="foba0001@example.com",
+            authn=AUTHN,
+            sign_response=False,
+            sign_assertion=False,
+            encrypt_assertion=False,
+            encrypt_assertion_self_contained=False
+        )
+
+        # The create_authn_response method above will return an instance
+        # of saml2.samlp.Response when neither encrypting nor signing and
+        # so we can remove the <NameID> element directly.
+        resp.assertion.subject.name_id = None
+
+        # Assert that the response does not contain a NameID element so that
+        # the parsing below is a fair test.
+        assert str(resp).find("NameID") == -1
+
+        # Cast the response to a string and encode it to mock up the payload
+        # the SP client is expected to receive via HTTP POST binding.
+        resp_str = encode_fn(str(resp).encode())
+
+        # We do not need the client to verify a signature for this test.
+        client.want_assertions_signed = False
+        client.want_response_signed = False
+
+        # Parse the authentication response that does not include a <NameID>.
+        authn_response = client.parse_authn_request_response(
+            resp_str, BINDING_HTTP_POST,
+            {"id1": "http://foo.example.com/service"})
+
+        # A successful test is parsing the response.
+        assert authn_response is not None
+
     def setup_verify_authn_response(self):
         idp = "urn:mace:example.com:saml:roland:idp"
         ava = {"givenName": ["Derek"], "sn": ["Jeter"],