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.

Sign up for GitHub

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

Improve debug #42

Merged
merged 19 commits into from
Jan 4, 2017
Merged

Improve debug #42

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a42554a
Rename deprecated assertations
quantus Nov 15, 2016
e7b17a7
Make logout request function is_valid support raising exceptions
quantus Nov 17, 2016
d3549f9
Improve logout request tests
quantus Nov 17, 2016
2965e00
Make logout response function is_valid support raising exceptions
quantus Nov 17, 2016
c3d92fb
Improve logout response tests
quantus Nov 17, 2016
5545e21
Make response function is_valid support raising exceptions
quantus Nov 15, 2016
16e38be
Improve response tests
quantus Nov 15, 2016
f95abc5
Merge branch 'feature/re-raising-response-validation-exceptions' of h…
pitbulk Dec 22, 2016
7ac2640
Rename parameter
pitbulk Dec 22, 2016
b144b06
Optionally raise detailed exceptions vs. returning False
pitbulk Dec 22, 2016
9ea32f5
Implement a more specific exception class for handling some validatio…
pitbulk Dec 29, 2016
73ee977
Add support for retrieving the last ID of the generated AuthNRequest …
pitbulk Dec 29, 2016
a510f16
Add hooks to retrieve last-sent and last-received requests and responses
pitbulk Dec 30, 2016
37788e2
Suggested changes
pitbulk Dec 30, 2016
a096f36
Minor bug
pitbulk Dec 30, 2016
acb88e0
Fix typo
pitbulk Dec 31, 2016
fa6d3ea
Minor fix on docs
pitbulk Jan 2, 2017
cbbe084
Fix name
pitbulk Jan 3, 2017
c90a62d
Typo
pitbulk Jan 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'
Expand Down
50 changes: 31 additions & 19 deletions src/onelogin/saml2/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@
from onelogin.saml2 import compat
from onelogin.saml2.settings import OneLogin_Saml2_Settings
from onelogin.saml2.response import OneLogin_Saml2_Response
from onelogin.saml2.errors import OneLogin_Saml2_Error
from onelogin.saml2.logout_response import OneLogin_Saml2_Logout_Response
from onelogin.saml2.constants import OneLogin_Saml2_Constants
from onelogin.saml2.utils import OneLogin_Saml2_Utils
from onelogin.saml2.utils import OneLogin_Saml2_Utils, OneLogin_Saml2_Error, OneLogin_Saml2_ValidationError
from onelogin.saml2.logout_request import OneLogin_Saml2_Logout_Request
from onelogin.saml2.authn_request import OneLogin_Saml2_Authn_Request

Expand Down Expand Up @@ -429,7 +428,7 @@ def __build_signature(self, data, saml_type, sign_algorithm=OneLogin_Saml2_Const
if not key:
raise OneLogin_Saml2_Error(
"Trying to sign the %s but can't load the SP private key." % saml_type,
OneLogin_Saml2_Error.SP_CERTS_NOT_FOUND
OneLogin_Saml2_Error.PRIVATE_KEY_NOT_FOUND
)

msg = self.__build_sign_query(data[saml_type],
Expand Down Expand Up @@ -472,7 +471,7 @@ def validate_response_signature(self, request_data):

return self.__validate_signature(request_data, 'SAMLResponse')

def __validate_signature(self, data, saml_type):
def __validate_signature(self, data, saml_type, raise_exceptions=False):
"""
Validate Signature

Expand All @@ -484,22 +483,30 @@ def __validate_signature(self, data, saml_type):

:param saml_type: The target URL the user should be redirected to
:type saml_type: string SAMLRequest | SAMLResponse
"""

signature = data.get('Signature', None)
if signature is None:
if self.__settings.is_strict() and self.__settings.get_security_data().get('wantMessagesSigned', False):
self.__error_reason = 'The %s is not signed. Rejected.' % saml_type
return False
return True

x509cert = self.get_settings().get_idp_cert()

if x509cert is None:
self.__errors.append("In order to validate the sign on the %s, the x509cert of the IdP is required" % saml_type)
return False

:param raise_exceptions: Whether to return false on failure or raise an exception
:type raise_exceptions: Boolean
"""
try:
signature = data.get('Signature', None)
if signature is None:
if self.__settings.is_strict() and self.__settings.get_security_data().get('wantMessagesSigned', False):
raise OneLogin_Saml2_ValidationError(
'The %s is not signed. Rejected.' % saml_type,
OneLogin_Saml2_ValidationError.NO_SIGNED_RESPONSE
)
return True

x509cert = self.get_settings().get_idp_cert()

if not x509cert:
error_msg = "In order to validate the sign on the %s, the x509cert of the IdP is required" % saml_type
self.__errors.append(error_msg)
raise OneLogin_Saml2_Error(
error_msg,
OneLogin_Saml2_Error.CERT_NOT_FOUND
)

sign_alg = data.get('SigAlg', OneLogin_Saml2_Constants.RSA_SHA1)
if isinstance(sign_alg, bytes):
sign_alg = sign_alg.decode('utf8')
Expand All @@ -520,8 +527,13 @@ def __validate_signature(self, data, saml_type):
x509cert,
sign_alg,
self.__settings.is_debug_active()):
raise Exception('Signature validation failed. %s rejected.' % saml_type)
raise OneLogin_Saml2_ValidationError(
'Signature validation failed. %s rejected.' % saml_type,
OneLogin_Saml2_ValidationError.INVALID_SIGNATURE
)
return True
except Exception as e:
self.__error_reason = str(e)
if raise_exceptions:
raise e
return False
74 changes: 73 additions & 1 deletion src/onelogin/saml2/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class OneLogin_Saml2_Error(Exception):
SETTINGS_INVALID_SYNTAX = 1
SETTINGS_INVALID = 2
METADATA_SP_INVALID = 3
SP_CERTS_NOT_FOUND = 4
CERT_NOT_FOUND = 4
Copy link

Choose a reason for hiding this comment

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

This will break backward compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will keep the old and mark as deprecated

REDIRECT_INVALID_URL = 5
PUBLIC_CERT_FILE_NOT_FOUND = 6
PRIVATE_KEY_FILE_NOT_FOUND = 7
Expand All @@ -34,6 +34,8 @@ class OneLogin_Saml2_Error(Exception):
SAML_LOGOUTREQUEST_INVALID = 10
SAML_LOGOUTRESPONSE_INVALID = 11
SAML_SINGLE_LOGOUT_NOT_SUPPORTED = 12
PRIVATE_KEY_NOT_FOUND = 13
UNSUPPORTED_SETTINGS_OBJECT = 14

def __init__(self, message, code=0, errors=None):
"""
Expand All @@ -50,3 +52,73 @@ def __init__(self, message, code=0, errors=None):

Exception.__init__(self, message)
self.code = code


class OneLogin_Saml2_ValidationError(Exception):
"""
This class implements another custom Exception handler, related
to exceptions that happens during validation process.
Defines custom error codes .
"""

# Validation Errors
UNSUPPORTED_SAML_VERSION = 0
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

MISSING_ID = 1
WRONG_NUMBER_OF_ASSERTIONS = 2
MISSING_STATUS = 3
MISSING_STATUS_CODE = 4
STATUS_CODE_IS_NOT_SUCCESS = 5
WRONG_SIGNED_ELEMENT = 6
ID_NOT_FOUND_IN_SIGNED_ELEMENT = 7
DUPLICATED_ID_IN_SIGNED_ELEMENTS = 8
INVALID_SIGNED_ELEMENT = 9
DUPLICATED_REFERENCE_IN_SIGNED_ELEMENTS = 10
UNEXPECTED_SIGNED_ELEMENTS = 11
WRONG_NUMBER_OF_SIGNATURES_IN_RESPONSE = 12
WRONG_NUMBER_OF_SIGNATURES_IN_ASSERTION = 13
INVALID_XML_FORMAT = 14
WRONG_INRESPONSETO = 15
NO_ENCRYPTED_ASSERTION = 16
NO_ENCRYPTED_NAMEID = 17
MISSING_CONDITIONS = 18
ASSERTION_TOO_EARLY = 19
ASSERTION_EXPIRED = 20
WRONG_NUMBER_OF_AUTHSTATEMENTS = 21
NO_ATTRIBUTESTATEMENT = 22
ENCRYPTED_ATTRIBUTES = 23
WRONG_DESTINATION = 24
EMPTY_DESTINATION = 25
WRONG_AUDIENCE = 26
ISSUER_NOT_FOUND_IN_RESPONSE = 27
ISSUER_NOT_FOUND_IN_ASSERTION = 28
WRONG_ISSUER = 29
SESSION_EXPIRED = 30
WRONG_SUBJECTCONFIRMATION = 31
NO_SIGNED_RESPONSE = 32
NO_SIGNED_ASSERTION = 33
NO_SIGNATURE_FOUND = 34
KEYINFO_NOT_FOUND_IN_ENCRYPTED_DATA = 35
CHILDREN_NODE_NOT_FOIND_IN_KEYINFO = 36
Copy link
Contributor

Choose a reason for hiding this comment

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

CHILDREN_NODE_NOT_FOIND_IN_KEYINFO -> CHILDREN_NODE_NOT_FOUND_IN_KEYINFO

UNSUPPORTED_RETRIEVAL_METHOD = 37
NO_NAMEID = 38
EMPTY_NAMEID = 39
SP_NAME_QUALIFIER_NAME_MISMATCH = 40
DUPLICATED_ATTRIBUTE_NAME_FOUND = 41
INVALID_SIGNATURE = 42
WRONG_NUMBER_OF_SIGNATURES = 43
RESPONSE_EXPIRED = 44

def __init__(self, message, code=0, errors=None):
"""
Initializes the Exception instance.
Arguments are:
* (str) message. Describes the error.
* (int) code. The code error (defined in the error class).
"""
assert isinstance(code, int)

if errors is not None:
message = message % errors

Exception.__init__(self, message)
self.code = code
45 changes: 35 additions & 10 deletions src/onelogin/saml2/logout_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, OneLogin_Saml2_Error, OneLogin_Saml2_ValidationError
from onelogin.saml2.xml_templates import OneLogin_Saml2_Templates
from onelogin.saml2.xml_utils import OneLogin_Saml2_XML

Expand Down Expand Up @@ -141,7 +141,10 @@ def get_nameid_data(request, key=None):

if len(encrypted_entries) == 1:
if key is None:
raise Exception('Key is required in order to decrypt the NameID')
raise OneLogin_Saml2_Error(
'Private Key is required in order to decrypt the NameID, check settings',
OneLogin_Saml2_Error.PRIVATE_KEY_NOT_FOUND
)

encrypted_data_nodes = OneLogin_Saml2_XML.query(elem, '/samlp:LogoutRequest/saml:EncryptedID/xenc:EncryptedData')
if len(encrypted_data_nodes) == 1:
Expand All @@ -153,7 +156,10 @@ def get_nameid_data(request, key=None):
name_id = entries[0]

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',
Copy link

Choose a reason for hiding this comment

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

NameID not found in the Logout Request instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will fix that message

OneLogin_Saml2_ValidationError.NO_NAMEID
)

name_id_data = {
'Value': name_id.text
Expand Down Expand Up @@ -212,12 +218,15 @@ def get_session_indexes(request):
session_indexes.append(session_index_node.text)
return session_indexes

def is_valid(self, request_data):
def is_valid(self, request_data, raise_exceptions=False):
"""
Checks if the Logout Request received is valid
:param request_data: Request Data
:type request_data: dict

:param raise_exceptions: Whether to return false on failure or raise an exception
:type raise_exceptions: Boolean

:return: If the Logout Request is or not valid
:rtype: boolean
"""
Expand All @@ -233,7 +242,10 @@ def is_valid(self, request_data):
if self.__settings.is_strict():
res = OneLogin_Saml2_XML.validate_xml(root, 'saml-schema-protocol-2.0.xsd', self.__settings.is_debug_active())
if isinstance(res, str):
raise Exception('Invalid SAML Logout Request. Not match the saml-schema-protocol-2.0.xsd')
raise OneLogin_Saml2_ValidationError(
'Invalid SAML Logout Request. Not match the saml-schema-protocol-2.0.xsd',
OneLogin_Saml2_ValidationError.INVALID_XML_FORMAT
)

security = self.__settings.get_security_data()

Expand All @@ -243,37 +255,50 @@ def is_valid(self, request_data):
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 OneLogin_Saml2_ValidationError(
'Could not validate timestamp: expired. Check system clock.)',
OneLogin_Saml2_ValidationError.RESPONSE_EXPIRED
)

# Check destination
if root.get('Destination', None):
destination = root.get('Destination')
if destination != '':
if current_url not in destination:
raise Exception(
raise OneLogin_Saml2_ValidationError(
'The LogoutRequest was received at '
'%(currentURL)s instead of %(destination)s' %
{
'currentURL': current_url,
'destination': destination,
}
},
OneLogin_Saml2_ValidationError.WRONG_DESTINATION
)

# Check issuer
issuer = OneLogin_Saml2_Logout_Request.get_issuer(root)
if issuer is not None and issuer != idp_entity_id:
raise Exception('Invalid issuer in the Logout Request')
raise OneLogin_Saml2_ValidationError(
'Invalid issuer in the Logout Request',
OneLogin_Saml2_ValidationError.WRONG_ISSUER
)

if security['wantMessagesSigned']:
if 'Signature' not in get_data:
raise Exception('The Message of the Logout Request is not signed and the SP require it')
raise OneLogin_Saml2_ValidationError(
'The Message of the Logout Request is not signed and the SP require it',
OneLogin_Saml2_ValidationError.NO_SIGNED_RESPONSE
)

return True
except Exception as err:
# pylint: disable=R0801
self.__error = str(err)
debug = self.__settings.is_debug_active()
if debug:
print(err)
if raise_exceptions:
raise
return False

def get_error(self):
Expand Down
35 changes: 28 additions & 7 deletions src/onelogin/saml2/logout_response.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

"""

from onelogin.saml2.utils import OneLogin_Saml2_Utils
from onelogin.saml2.utils import OneLogin_Saml2_Utils, OneLogin_Saml2_ValidationError
from onelogin.saml2.xml_templates import OneLogin_Saml2_Templates
from onelogin.saml2.xml_utils import OneLogin_Saml2_XML

Expand Down Expand Up @@ -63,11 +63,15 @@ def get_status(self):
status = entries[0].attrib['Value']
return status

def is_valid(self, request_data, request_id=None):
def is_valid(self, request_data, request_id=None, raise_exceptions=False):
"""
Determines if the SAML LogoutResponse is valid
:param request_id: The ID of the LogoutRequest sent by this SP to the IdP
:type request_id: string

:param raise_exceptions: Whether to return false on failure or raise an exception
:type raise_exceptions: Boolean

:return: Returns if the SAML LogoutResponse is or not valid
:rtype: boolean
"""
Expand All @@ -80,37 +84,54 @@ def is_valid(self, request_data, request_id=None):
if self.__settings.is_strict():
res = OneLogin_Saml2_XML.validate_xml(self.document, 'saml-schema-protocol-2.0.xsd', self.__settings.is_debug_active())
if isinstance(res, str):
raise Exception('Invalid SAML Logout Request. Not match the saml-schema-protocol-2.0.xsd')
raise OneLogin_Saml2_ValidationError(
'Invalid SAML Logout Request. Not match the saml-schema-protocol-2.0.xsd',
OneLogin_Saml2_ValidationError.INVALID_XML_FORMAT
)

security = self.__settings.get_security_data()

# Check if the InResponseTo of the Logout Response matches the ID of the Logout Request (requestId) if provided
in_response_to = self.document.get('InResponseTo', None)
if request_id is not None and in_response_to and in_response_to != request_id:
raise Exception('The InResponseTo of the Logout Response: %s, does not match the ID of the Logout request sent by the SP: %s' % (in_response_to, request_id))
raise OneLogin_Saml2_ValidationError(
'The InResponseTo of the Logout Response: %s, does not match the ID of the Logout request sent by the SP: %s' % (in_response_to, request_id),
OneLogin_Saml2_ValidationError.WRONG_INRESPONSETO
)

# Check issuer
issuer = self.get_issuer()
if issuer is not None and issuer != idp_entity_id:
raise Exception('Invalid issuer in the Logout Request')
raise OneLogin_Saml2_ValidationError(
'Invalid issuer in the Logout Request',
OneLogin_Saml2_ValidationError.WRONG_ISSUER
)

current_url = OneLogin_Saml2_Utils.get_self_url_no_query(request_data)

# Check destination
destination = self.document.get('Destination', None)
if destination and current_url not in destination:
raise Exception('The LogoutRequest was received at $currentURL instead of $destination')
raise OneLogin_Saml2_ValidationError(
'The LogoutResponse was received at %s instead of %s' % (current_url, destination),
OneLogin_Saml2_ValidationError.WRONG_DESTINATION
)

if security['wantMessagesSigned']:
if 'Signature' not in get_data:
raise Exception('The Message of the Logout Response is not signed and the SP require it')
raise OneLogin_Saml2_ValidationError(
'The Message of the Logout Response is not signed and the SP require it',
OneLogin_Saml2_ValidationError.NO_SIGNED_RESPONSE
)
return True
# pylint: disable=R0801
except Exception as err:
self.__error = str(err)
debug = self.__settings.is_debug_active()
if debug:
print(err)
if raise_exceptions:
raise
return False

def __query(self, query):
Expand Down
Loading