Improve debug by pitbulk · Pull Request #42 · SAML-Toolkits/python3-saml

pitbulk · 2016-12-22T11:09:11Z

Add option to raise response validation exceptions #37 Add option to raise response validation exceptions
Optionally raise detailed exceptions vs. returning False
Implement a more specific exception class for handling some validation errors
Improve/Fix tests
Add support for retrieving the last ID of the generated AuthNRequest / LogoutRequest
Add hooks to retrieve last-sent and last-received requests and responses

…ttps://github.com/quantus/python3-saml into improve_debug

coveralls · 2016-12-22T11:22:18Z

Coverage increased (+1.001%) to 96.262% when pulling 7ac2640 on improve_debug into f011fda on master.

…n errors. Improve tests

coveralls · 2016-12-29T23:30:08Z

Coverage increased (+0.4%) to 95.707% when pulling 9ea32f5 on improve_debug into f011fda on master.

…/ LogoutRequest

coveralls · 2016-12-29T23:43:05Z

Coverage increased (+0.4%) to 95.668% when pulling 73ee977 on improve_debug into f011fda on master.

coveralls · 2016-12-30T00:46:30Z

Coverage increased (+0.5%) to 95.728% when pulling a510f16 on improve_debug into f011fda on master.

nickw444 · 2016-12-30T09:05:02Z

    SETTINGS_INVALID = 2
    METADATA_SP_INVALID = 3
-    SP_CERTS_NOT_FOUND = 4
+    CERT_NOT_FOUND = 4


This will break backward compatibility

Ok I will keep the old and mark as deprecated

nickw444 · 2016-12-30T09:07:18Z

+    """
+
+    # Validation Errors
+    UNSUPPORTED_SAML_VERSION = 0


I personally don't like the use of "codes" in exceptions, since users of the library need to understand they need to inspect a specific property in the exception, but I do see how this is keeping consistency with the implementation of the existing OneLogin_Saml2_Error.

A better solution would be to have an exception for each of these individually, that possibly inherit from a base class exception class OneLogin_Saml2_ValidationError.

I also don't like exception codes. I would like to have OneLogin_Saml2_Error that would be the base class for all exception raised by this library. Then we could have OneLogin_Saml2_ValidationError exception class inheriting that, and then each error would have own exception class.

Some code showing usage:

class OneLogin_Saml2_Error(Exception): .... class OneLogin_Saml2_ValidationError(OneLogin_Saml2_Error): # For backwards compatibility code = 123 class OneLogin_Saml2_ExpiredResponseError(OneLogin_Saml2_ValidationError): # If we want to keep backwards compatibility code = 123 # Usage: try: auth.process_sso(...) except OneLogin_Saml2_ExpiredResponseError as e: # Not important, tell user to retry except OneLogin_Saml2_Error as e: # Inform user about the error and log it somewhere

With that said, I really liked the changes in this PR. 👍 I think that the exception handling can be changed to the way I showed later on without breaking any existing code, so that can be implemented later on.

My idea here is to have 2 different kind of exceptions:

Error, related to how are the toolkit environment/settings

ValidationError, some exception that happened when validating a SAMLResponse

I got your point of view, but then we need 50+ exception methods ...

In real scenarios I don't think that you gonna need to catch many specific exception, rather than just check if there was an environment/settings error, or a validation error.

Yeah, you are right that in most cases there is no need to many specific exceptions, and making separate class for each error would add lots of really small exception classes. I think that in many cases it is quite irrelevant what the exact error is, so one common validation error class makes sense.

I am no longer sure if the validation error should be inherited from the OneLogin_Saml2_Error class. That way the user could always catch all exceptions easily, but the downside of this is that both errors would share the same error code range. Then one would need to make sure that the OneLogin_Saml2_Error and OneLogin_Saml2_ValidationError classes never share any error code values.

nickw444 · 2016-12-30T09:08:29Z

        if name_id is None:
-            raise Exception('Not NameID found in the Logout Request')
+            raise OneLogin_Saml2_ValidationError(
+                'Not NameID found in the Logout Request',


NameID not found in the Logout Request instead?

Ok I will fix that message

quantus · 2016-12-30T11:47:14Z

-            dsig_ctx.verify(signature_node)
-        except Exception:
-            raise Exception('Signature validation failed. SAML Response rejected')
+        dsig_ctx.verify(signature_node)


I think the dsig_ctx.verify(signature_node) code should be inside try except block and then the raised exception should be converted to OneLogin_Saml2_Error exception. This way the user of this library doesn't need to consider different exception types raised by the xmlsec library, and catching OneLogin_Saml2_Error exceptions will cover invalid signature exceptions as well.

I wanted to offer a direct way to get the real cause when a signature validation fails.
instead of catch the exception here on the validate_node_sign method, but I think I can provide this info on an extra parameter. Let me fix that

coveralls · 2016-12-30T14:15:54Z

Coverage increased (+0.5%) to 95.786% when pulling 37788e2 on improve_debug into f011fda on master.

coveralls · 2016-12-30T16:18:17Z

Coverage increased (+0.5%) to 95.786% when pulling a096f36 on improve_debug into f011fda on master.

quantus · 2016-12-30T20:47:46Z

+    NO_SIGNED_ASSERTION = 33
+    NO_SIGNATURE_FOUND = 34
+    KEYINFO_NOT_FOUND_IN_ENCRYPTED_DATA = 35
+    CHILDREN_NODE_NOT_FOIND_IN_KEYINFO = 36


CHILDREN_NODE_NOT_FOIND_IN_KEYINFO -> CHILDREN_NODE_NOT_FOUND_IN_KEYINFO

coveralls · 2016-12-31T09:14:47Z

Coverage increased (+0.5%) to 95.786% when pulling acb88e0 on improve_debug into f011fda on master.

coveralls · 2017-01-02T16:37:02Z

Coverage increased (+0.5%) to 95.786% when pulling fa6d3ea on improve_debug into f011fda on master.

coveralls · 2017-01-03T16:21:53Z

Coverage increased (+0.5%) to 95.786% when pulling cbbe084 on improve_debug into f011fda on master.

coveralls · 2017-01-03T21:36:35Z

Coverage increased (+0.5%) to 95.786% when pulling c90a62d on improve_debug into f011fda on master.

quantus and others added 9 commits November 17, 2016 21:38

Rename deprecated assertations

a42554a

Make logout request function is_valid support raising exceptions

e7b17a7

Improve logout request tests

d3549f9

Make logout response function is_valid support raising exceptions

2965e00

Improve logout response tests

c3d92fb

Make response function is_valid support raising exceptions

5545e21

Improve response tests

16e38be

Merge branch 'feature/re-raising-response-validation-exceptions' of h…

f95abc5

…ttps://github.com/quantus/python3-saml into improve_debug

Rename parameter

7ac2640

This was referenced Dec 22, 2016

Add option to raise response validation exceptions #37

Closed

Implement a more specific exception class for handling some validation errors #41

Closed

pitbulk added 2 commits December 22, 2016 12:46

Optionally raise detailed exceptions vs. returning False

b144b06

Implement a more specific exception class for handling some validatio…

9ea32f5

…n errors. Improve tests

Add support for retrieving the last ID of the generated AuthNRequest …

73ee977

…/ LogoutRequest

Add hooks to retrieve last-sent and last-received requests and responses

a510f16

nickw444 reviewed Dec 30, 2016

View reviewed changes

quantus reviewed Dec 30, 2016

View reviewed changes

Suggested changes

37788e2

Minor bug

a096f36

quantus reviewed Dec 30, 2016

View reviewed changes

Fix typo

acb88e0

Minor fix on docs

fa6d3ea

Fix name

cbbe084

Typo

c90a62d

pitbulk merged commit ce0d716 into master Jan 4, 2017

Uh oh!

Conversation

pitbulk commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 29, 2016

Uh oh!

coveralls commented Dec 29, 2016

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 31, 2016

Uh oh!

coveralls commented Jan 2, 2017

Uh oh!

coveralls commented Jan 3, 2017

Uh oh!

coveralls commented Jan 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pitbulk commented Dec 22, 2016 •

edited

Loading

coveralls commented Dec 22, 2016 •

edited

Loading