Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to raise response validation exceptions #37

Conversation

quantus
Copy link
Contributor

@quantus quantus commented Nov 15, 2016

The current OneLogin_Saml2_Response.is_valid always collects the first raised exceptions and creates the error message from the exception content. I am using Raven (+ Sentry) to track all unhandled exceptions, so I would rather have the function just raise the first found validation error. When the exception is raised from the problem source Raven is able to capture local variables from the exception context. In my use case IdP:s can be dynamically added/removed/modified during the program runtime, so getting all the exception information is important to me.

This PR adds an optional raises argument to OneLogin_Saml2_Response.is_valid. When the argument is truthy, first found exception gets raised. As the option is optional this change is completely backwards compatible.

As I wrote test for this feature I noticed some oddness in some test code:

  • Many tests contained code to test for raised exceptions event thou the functions didn't raise any exceptions. Fixed in cd2a6b4.
  • Some tests used self.assertFalse(True) method to make sure that the exception was thrown. I refactored these tests in a9997f7.
  • Some tests didn't make sure that the tested function actually raised the expected error. Fixed in c007ca7.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 95.265% when pulling b029a8a on quantus:feature/re-raising-response-validation-exceptions into 054f8e6 on onelogin:master.

@@ -47,7 +47,7 @@ def __init__(self, settings, response):
self.encrypted = True
self.decrypted_document = self.__decrypt_assertion(decrypted_document)

def is_valid(self, request_data, request_id=None):
def is_valid(self, request_data, request_id=None, raises=False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add this functionality to rest of is_valid methods (logoutrequest and logoutresponse)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea. I'll let you know when I have made the changes.

valid = response.is_valid(self.get_request_data())
self.assertFalse(valid)
except Exception as e:
self.assertEqual('Missing ID attribute on SAML Response', str(e))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we are able to raise exceptions, why no use them calling
valid = response.is_valid(self.get_request_data(), None, True)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was refactoring these tests I didn't take into consideration the changes that would come from adding the new argument. I wanted to keep the cleaning changes separate from the new feature to keep the commits simple.

I don't have any strong opinion about whether we should run these tests with the raising version. The default code calls these functions without the raises being set, but with the raises set we can more precisely make sure that the function failed with the wanted error.

I can change these functions to use the exception raising if you prefer that.

@quantus quantus force-pushed the feature/re-raising-response-validation-exceptions branch from b029a8a to 67b3407 Compare November 17, 2016 20:37
@quantus quantus force-pushed the feature/re-raising-response-validation-exceptions branch from 67b3407 to 16e38be Compare November 17, 2016 20:47
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.275% when pulling 16e38be on quantus:feature/re-raising-response-validation-exceptions into 054f8e6 on onelogin:master.

@quantus
Copy link
Contributor Author

quantus commented Nov 17, 2016

@pitbulk, I have now added the argument to other places and also made the current tests to test the raised exception. There were couple of response tests where the old code incorrectly tried to test that exception with wrong content was being thrown. The tests where I had to change the expected error message were:

  • testIsInValidIssuer
  • testIsInValidCert
  • testValidateVersion

I also didn't touch the response test testIsInValidReference, as that test has some other issues. I wrote more about this issue to the issue #38.

@pitbulk
Copy link
Contributor

pitbulk commented Nov 17, 2016

Ok thanks for contribute, I will need some time to review

@pitbulk pitbulk mentioned this pull request Dec 22, 2016
@pitbulk
Copy link
Contributor

pitbulk commented Dec 22, 2016

I will merge this on #42

@pitbulk pitbulk closed this Dec 22, 2016
@pitbulk
Copy link
Contributor

pitbulk commented Dec 29, 2016

@quantus can you review #42?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants