Optionally raise detailed exceptions vs. returning False

pitbulk · pitbulk · commit b144b06723c1 · 2016-12-22T12:46:20.000+01:00
diff --git a/.travis.yml b/.travis.yml
@@ -1,9 +1,9 @@
 language: python
 python:
   - '2.7.6'
-#  - '2.7.9'  
+# - '2.7.9'  
 # - '2.7.12'
-  - '3.3.4'
+# - '3.3.4'
 # - '3.3.5'
 # - '3.3.6'
   - '3.4.3'
diff --git a/src/onelogin/saml2/logout_request.py b/src/onelogin/saml2/logout_request.py
@@ -10,7 +10,7 @@
 """
 
 from onelogin.saml2.constants import OneLogin_Saml2_Constants
-from onelogin.saml2.utils import OneLogin_Saml2_Utils
+from onelogin.saml2.utils import OneLogin_Saml2_Utils, return_false_on_exception
 from onelogin.saml2.xml_templates import OneLogin_Saml2_Templates
 from onelogin.saml2.xml_utils import OneLogin_Saml2_XML
 
@@ -246,7 +246,7 @@ def is_valid(self, request_data, raise_exceptions=False):
                 if root.get('NotOnOrAfter', None):
                     na = OneLogin_Saml2_Utils.parse_SAML_to_time(root.get('NotOnOrAfter'))
                     if na <= OneLogin_Saml2_Utils.now():
-                        raise Exception('Timing issues (please check your clock settings)')
+                        raise Exception('Could not validate timestamp: expired. Check system clock.)')
 
                 # Check destination
                 if root.get('Destination', None):
diff --git a/src/onelogin/saml2/response.py b/src/onelogin/saml2/response.py
@@ -11,7 +11,7 @@
 
 from copy import deepcopy
 from onelogin.saml2.constants import OneLogin_Saml2_Constants
-from onelogin.saml2.utils import OneLogin_Saml2_Utils
+from onelogin.saml2.utils import OneLogin_Saml2_Utils, return_false_on_exception
 from onelogin.saml2.xml_utils import OneLogin_Saml2_XML
 
 
@@ -124,8 +124,7 @@ def is_valid(self, request_data, request_id=None, raise_exceptions=False):
                     raise Exception('The Assertion must include a Conditions element')
 
                 # Validates Assertion timestamps
-                if not self.validate_timestamps():
-                    raise Exception('Timing issues (please check your clock settings)')
+                self.validate_timestamps(raise_exceptions=True)
 
                 # Checks that an AuthnStatement element exists and is unique
                 if not self.check_one_authnstatement():
@@ -216,11 +215,11 @@ def is_valid(self, request_data, request_id=None, raise_exceptions=False):
                 fingerprintalg = idp_data.get('certFingerprintAlgorithm', None)
 
                 # If find a Signature on the Response, validates it checking the original response
-                if has_signed_response and not OneLogin_Saml2_Utils.validate_sign(self.document, cert, fingerprint, fingerprintalg, xpath=OneLogin_Saml2_Utils.RESPONSE_SIGNATURE_XPATH):
+                if has_signed_response and not OneLogin_Saml2_Utils.validate_sign(self.document, cert, fingerprint, fingerprintalg, xpath=OneLogin_Saml2_Utils.RESPONSE_SIGNATURE_XPATH, raise_exceptions=False):
                     raise Exception('Signature validation failed. SAML Response rejected')
 
                 document_check_assertion = self.decrypted_document if self.encrypted else self.document
-                if has_signed_assertion and not OneLogin_Saml2_Utils.validate_sign(document_check_assertion, cert, fingerprint, fingerprintalg, xpath=OneLogin_Saml2_Utils.ASSERTION_SIGNATURE_XPATH):
+                if has_signed_assertion and not OneLogin_Saml2_Utils.validate_sign(document_check_assertion, cert, fingerprint, fingerprintalg, xpath=OneLogin_Saml2_Utils.ASSERTION_SIGNATURE_XPATH, raise_exceptions=False):
                     raise Exception('Signature validation failed. SAML Response rejected')
 
             return True
@@ -502,6 +501,7 @@ def validate_signed_elements(self, signed_elements):
 
         return True
 
+    @return_false_on_exception
     def validate_timestamps(self):
         """
         Verifies that the document is valid according to Conditions Element
@@ -515,9 +515,9 @@ def validate_timestamps(self):
             nb_attr = conditions_node.get('NotBefore')
             nooa_attr = conditions_node.get('NotOnOrAfter')
             if nb_attr and OneLogin_Saml2_Utils.parse_SAML_to_time(nb_attr) > OneLogin_Saml2_Utils.now() + OneLogin_Saml2_Constants.ALLOWED_CLOCK_DRIFT:
-                return False
+                raise Exception('Could not validate timestamp: not yet valid. Check system clock.')
             if nooa_attr and OneLogin_Saml2_Utils.parse_SAML_to_time(nooa_attr) + OneLogin_Saml2_Constants.ALLOWED_CLOCK_DRIFT <= OneLogin_Saml2_Utils.now():
-                return False
+                raise Exception('Could not validate timestamp: expired. Check system clock.')
         return True
 
     def __query_assertion(self, xpath_expr):
diff --git a/src/onelogin/saml2/utils.py b/src/onelogin/saml2/utils.py
@@ -16,6 +16,7 @@
 from isodate import parse_duration as duration_parser
 import re
 from textwrap import wrap
+from functools import wraps
 from uuid import uuid4
 
 import zlib
@@ -33,6 +34,24 @@
     from urllib import quote_plus  # py2
 
 
+def return_false_on_exception(func):
+    """
+    Decorator. When applied to a function, it will, by default, suppress any exceptions
+    raised by that function and return False. It may be overridden by passing a
+    "raise_exceptions" keyword argument when calling the wrapped function.
+    """
+    @wraps(func)
+    def exceptfalse(*args, **kwargs):
+        if not kwargs.pop('raise_exceptions', False):
+            try:
+                return func(*args, **kwargs)
+            except Exception:
+                return False
+        else:
+            return func(*args, **kwargs)
+    return exceptfalse
+
+
 class OneLogin_Saml2_Utils(object):
     """
 
@@ -719,6 +738,7 @@ def add_sign(xml, key, cert, debug=False, sign_algorithm=OneLogin_Saml2_Constant
         return OneLogin_Saml2_XML.to_string(elem)
 
     @staticmethod
+    @return_false_on_exception
     def validate_sign(xml, cert=None, fingerprint=None, fingerprintalg='sha1', validatecert=False, debug=False, xpath=None):
         """
         Validates a signature (Message or Assertion).
@@ -743,36 +763,34 @@ def validate_sign(xml, cert=None, fingerprint=None, fingerprintalg='sha1', valid
 
         :param xpath: The xpath of the signed element
         :type: string
-        """
-        try:
-            if xml is None or xml == '':
-                raise Exception('Empty string supplied as input')
-
-            elem = OneLogin_Saml2_XML.to_etree(xml)
-            xmlsec.enable_debug_trace(debug)
-            xmlsec.tree.add_ids(elem, ["ID"])
 
-            if xpath:
-                signature_nodes = OneLogin_Saml2_XML.query(elem, xpath)
-            else:
-                signature_nodes = OneLogin_Saml2_XML.query(elem, OneLogin_Saml2_Utils.RESPONSE_SIGNATURE_XPATH)
+        :param raise_exceptions: Whether to return false on failure or raise an exception
+        :type raise_exceptions: Boolean
+        """
+        if xml is None or xml == '':
+            raise Exception('Empty string supplied as input')
 
-                if len(signature_nodes) == 0:
-                    signature_nodes = OneLogin_Saml2_XML.query(elem, OneLogin_Saml2_Utils.ASSERTION_SIGNATURE_XPATH)
+        elem = OneLogin_Saml2_XML.to_etree(xml)
+        xmlsec.enable_debug_trace(debug)
+        xmlsec.tree.add_ids(elem, ["ID"])
 
-            if len(signature_nodes) == 1:
-                signature_node = signature_nodes[0]
+        if xpath:
+            signature_nodes = OneLogin_Saml2_XML.query(elem, xpath)
+        else:
+            signature_nodes = OneLogin_Saml2_XML.query(elem, OneLogin_Saml2_Utils.RESPONSE_SIGNATURE_XPATH)
 
-                return OneLogin_Saml2_Utils.validate_node_sign(signature_node, elem, cert, fingerprint, fingerprintalg, validatecert, debug)
-            else:
-                return False
-        except xmlsec.Error as e:
-            if debug:
-                print(e)
+            if len(signature_nodes) == 0:
+                signature_nodes = OneLogin_Saml2_XML.query(elem, OneLogin_Saml2_Utils.ASSERTION_SIGNATURE_XPATH)
 
-            return False
+        if len(signature_nodes) == 1:
+            signature_node = signature_nodes[0]
+            # Raises expection if invalid
+            return OneLogin_Saml2_Utils.validate_node_sign(signature_node, elem, cert, fingerprint, fingerprintalg, validatecert, debug, raise_exceptions=True)
+        else:
+            raise Exception('Expected exactly one signature node; got {}.'.format(len(signature_nodes)))
 
     @staticmethod
+    @return_false_on_exception
     def validate_metadata_sign(xml, cert=None, fingerprint=None, fingerprintalg='sha1', validatecert=False, debug=False):
         """
         Validates a signature of a EntityDescriptor.
@@ -794,35 +812,36 @@ def validate_metadata_sign(xml, cert=None, fingerprint=None, fingerprintalg='sha
 
         :param debug: Activate the xmlsec debug
         :type: bool
+
+        :param raise_exceptions: Whether to return false on failure or raise an exception
+        :type raise_exceptions: Boolean
         """
-        try:
-            if xml is None or xml == '':
-                raise Exception('Empty string supplied as input')
+        if xml is None or xml == '':
+            raise Exception('Empty string supplied as input')
 
-            elem = OneLogin_Saml2_XML.to_etree(xml)
-            xmlsec.enable_debug_trace(debug)
-            xmlsec.tree.add_ids(elem, ["ID"])
+        elem = OneLogin_Saml2_XML.to_etree(xml)
+        xmlsec.enable_debug_trace(debug)
+        xmlsec.tree.add_ids(elem, ["ID"])
 
-            signature_nodes = OneLogin_Saml2_XML.query(elem, '/md:EntitiesDescriptor/ds:Signature')
+        signature_nodes = OneLogin_Saml2_XML.query(elem, '/md:EntitiesDescriptor/ds:Signature')
 
-            if len(signature_nodes) == 0:
-                signature_nodes += OneLogin_Saml2_XML.query(elem, '/md:EntityDescriptor/ds:Signature')
+        if len(signature_nodes) == 0:
+            signature_nodes += OneLogin_Saml2_XML.query(elem, '/md:EntityDescriptor/ds:Signature')
 
-                if len(signature_nodes) == 0:
-                    signature_nodes += OneLogin_Saml2_XML.query(elem, '/md:EntityDescriptor/md:SPSSODescriptor/ds:Signature')
-                    signature_nodes += OneLogin_Saml2_XML.query(elem, '/md:EntityDescriptor/md:IDPSSODescriptor/ds:Signature')
+            if len(signature_nodes) == 0:
+                signature_nodes += OneLogin_Saml2_XML.query(elem, '/md:EntityDescriptor/md:SPSSODescriptor/ds:Signature')
+                signature_nodes += OneLogin_Saml2_XML.query(elem, '/md:EntityDescriptor/md:IDPSSODescriptor/ds:Signature')
 
-            if len(signature_nodes) > 0:
-                for signature_node in signature_nodes:
-                    if not OneLogin_Saml2_Utils.validate_node_sign(signature_node, elem, cert, fingerprint, fingerprintalg, validatecert, debug):
-                        return False
-                return True
-            else:
-                return False
-        except Exception:
-            return False
+        if len(signature_nodes) > 0:
+            for signature_node in signature_nodes:
+                # Raises expection if invalid
+                OneLogin_Saml2_Utils.validate_node_sign(signature_node, elem, cert, fingerprint, fingerprintalg, validatecert, debug, raise_exceptions=True)
+            return True
+        else:
+            raise Exception('Could not validate metadata signature: No signature nodes found.')
 
     @staticmethod
+    @return_false_on_exception
     def validate_node_sign(signature_node, elem, cert=None, fingerprint=None, fingerprintalg='sha1', validatecert=False, debug=False):
         """
         Validates a signature node.
@@ -847,40 +866,42 @@ def validate_node_sign(signature_node, elem, cert=None, fingerprint=None, finger
 
         :param debug: Activate the xmlsec debug
         :type: bool
+
+        :param raise_exceptions: Whether to return false on failure or raise an exception
+        :type raise_exceptions: Boolean
         """
-        try:
-            if (cert is None or cert == '') and fingerprint:
-                x509_certificate_nodes = OneLogin_Saml2_XML.query(signature_node, '//ds:Signature/ds:KeyInfo/ds:X509Data/ds:X509Certificate')
-                if len(x509_certificate_nodes) > 0:
-                    x509_certificate_node = x509_certificate_nodes[0]
-                    x509_cert_value = x509_certificate_node.text
-                    x509_fingerprint_value = OneLogin_Saml2_Utils.calculate_x509_fingerprint(x509_cert_value, fingerprintalg)
-                    if fingerprint == x509_fingerprint_value:
-                        cert = OneLogin_Saml2_Utils.format_cert(x509_cert_value)
-
-            if cert is None or cert == '':
-                return False
+        if (cert is None or cert == '') and fingerprint:
+            x509_certificate_nodes = OneLogin_Saml2_XML.query(signature_node, '//ds:Signature/ds:KeyInfo/ds:X509Data/ds:X509Certificate')
+            if len(x509_certificate_nodes) > 0:
+                x509_certificate_node = x509_certificate_nodes[0]
+                x509_cert_value = x509_certificate_node.text
+                x509_fingerprint_value = OneLogin_Saml2_Utils.calculate_x509_fingerprint(x509_cert_value, fingerprintalg)
+                if fingerprint == x509_fingerprint_value:
+                    cert = OneLogin_Saml2_Utils.format_cert(x509_cert_value)
 
-            # Check if Reference URI is empty
-            reference_elem = OneLogin_Saml2_XML.query(signature_node, '//ds:Reference')
-            if len(reference_elem) > 0:
-                if reference_elem[0].get('URI') == '':
-                    reference_elem[0].set('URI', '#%s' % signature_node.getparent().get('ID'))
+        if cert is None or cert == '':
+            raise Exception('Could not validate node signature: No certificate provided.')
 
-            if validatecert:
-                manager = xmlsec.KeysManager()
-                manager.load_cert_from_memory(cert, xmlsec.KeyFormat.CERT_PEM, xmlsec.KeyDataType.TRUSTED)
-                dsig_ctx = xmlsec.SignatureContext(manager)
-            else:
-                dsig_ctx = xmlsec.SignatureContext()
-                dsig_ctx.key = xmlsec.Key.from_memory(cert, xmlsec.KeyFormat.CERT_PEM, None)
+        # Check if Reference URI is empty
+        reference_elem = OneLogin_Saml2_XML.query(signature_node, '//ds:Reference')
+        if len(reference_elem) > 0:
+            if reference_elem[0].get('URI') == '':
+                reference_elem[0].set('URI', '#%s' % signature_node.getparent().get('ID'))
+
+        if validatecert:
+            manager = xmlsec.KeysManager()
+            manager.load_cert_from_memory(cert, xmlsec.KeyFormat.CERT_PEM, xmlsec.KeyDataType.TRUSTED)
+            dsig_ctx = xmlsec.SignatureContext(manager)
+        else:
+            dsig_ctx = xmlsec.SignatureContext()
+            dsig_ctx.key = xmlsec.Key.from_memory(cert, xmlsec.KeyFormat.CERT_PEM, None)
 
-            dsig_ctx.set_enabled_key_data([xmlsec.KeyData.X509])
+        dsig_ctx.set_enabled_key_data([xmlsec.KeyData.X509])
+        try:
             dsig_ctx.verify(signature_node)
-            return True
-        except xmlsec.Error as e:
-            if debug:
-                print(e)
+        except Exception:
+            raise Exception('Signature validation failed. SAML Response rejected')
+        return True
 
     @staticmethod
     def sign_binary(msg, key, algorithm=xmlsec.Transform.RSA_SHA1, debug=False):
diff --git a/tests/src/OneLogin/saml2_tests/logout_request_test.py b/tests/src/OneLogin/saml2_tests/logout_request_test.py
@@ -285,7 +285,7 @@ def testIsInvalidNotOnOrAfter(self):
 
         settings.set_strict(True)
         logout_request2 = OneLogin_Saml2_Logout_Request(settings, OneLogin_Saml2_Utils.b64encode(request))
-        with self.assertRaisesRegexp(Exception, 'Timing issues \(please check your clock settings\)'):
+        with self.assertRaisesRegexp(Exception, 'Could not validate timestamp: expired. Check system clock.'):
             logout_request2.is_valid(request_data, raise_exceptions=True)
 
     def testIsValid(self):
diff --git a/tests/src/OneLogin/saml2_tests/response_test.py b/tests/src/OneLogin/saml2_tests/response_test.py
@@ -533,7 +533,7 @@ def testIsInValidExpired(self):
 
         settings.set_strict(True)
         response_2 = OneLogin_Saml2_Response(settings, xml)
-        with self.assertRaisesRegexp(Exception, 'Timing issues \(please check your clock settings\)'):
+        with self.assertRaisesRegexp(Exception, 'Could not validate timestamp: expired. Check system clock.'):
             response_2.is_valid(self.get_request_data(), raise_exceptions=True)
 
     def testIsInValidNoStatement(self):
@@ -1043,7 +1043,7 @@ def testIsInValidCert(self):
         xml = self.file_contents(join(self.data_path, 'responses', 'valid_response.xml.base64'))
         response = OneLogin_Saml2_Response(settings, xml)
 
-        with self.assertRaisesRegexp(Exception, 'failed to load key'):
+        with self.assertRaisesRegexp(Exception, 'Signature validation failed. SAML Response rejected'):
             response.is_valid(self.get_request_data(), raise_exceptions=True)
 
     def testIsInValidCert2(self):
