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

Impossible to catch exceptions that aren't OneLogin_Saml2_Error #40

Closed
nickw444 opened this issue Dec 16, 2016 · 9 comments
Closed

Impossible to catch exceptions that aren't OneLogin_Saml2_Error #40

nickw444 opened this issue Dec 16, 2016 · 9 comments

Comments

@nickw444
Copy link

@nickw444 nickw444 commented Dec 16, 2016

I'm having an issue where the library is throwing an Exception that is of the base Exception type. I want to handle errors gracefully that occur within this library, but don't really want to catch every single exception that occurs.

Is there a particular reason this library uses OneLogin_Saml2_Error in some places, but others, like in https://github.com/onelogin/python3-saml/blob/master/src/onelogin/saml2/response.py#L301, just use Exception? Would it be possible to introduce a new class of Exception which is used in place of these so that they can be caught more easily without catching all Exception types.

Cheers

@pitbulk
Copy link
Contributor

@pitbulk pitbulk commented Dec 16, 2016

Right now I think that OneLogin_Saml2_Error is used to catch errors related to the SAML settings or when SAML messages not detected (bad SSO/SLO flow).

Im agree that we may improve the way we take care of exceptions and help to the dev on the debug process.

@nickw444
Copy link
Author

@nickw444 nickw444 commented Dec 16, 2016

Would you be willing to accept a PR with changes to use a more specific exception class?

@pitbulk
Copy link
Contributor

@pitbulk pitbulk commented Dec 16, 2016

Something like OneLogin_Saml2_ValidationError ?

@nickw444
Copy link
Author

@nickw444 nickw444 commented Dec 16, 2016

After looking into errors.py, it seems the OneLogin_Saml2_Error is pretty multi-purpose.

Thoughts on whether I should add a new error code SAML_RESPONSE_VALIDATION_ERROR, and use it, or use a totally new exception class?

https://github.com/onelogin/python3-saml/blob/master/src/onelogin/saml2/errors.py#L32

@pitbulk
Copy link
Contributor

@pitbulk pitbulk commented Dec 16, 2016

You can see that those error codes are related to settings or anormal SAML flow.

I think we can create a similar OneLogin_Saml2_ValidationError with codes that describes what validation failed, then we get a more complete error handle. What do you think?

@nickw444
Copy link
Author

@nickw444 nickw444 commented Dec 18, 2016

Sounds good to me!

@pitbulk
Copy link
Contributor

@pitbulk pitbulk commented Dec 21, 2016

@nickw444 Do you have more custom ValidationException to submit? If not I will take care of this commit and implement it.

@pitbulk
Copy link
Contributor

@pitbulk pitbulk commented Dec 21, 2016

I'm taking care of it.

@nickw444
Copy link
Author

@nickw444 nickw444 commented Dec 21, 2016

Thanks

@pitbulk pitbulk closed this Dec 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.