SAML-Toolkits / python-saml

Python SAML Toolkit
MIT License
662 stars 308 forks source link

Using of private attributes (with double underscore) #137

Open dangusev opened 8 years ago

dangusev commented 8 years ago

Hello!) Thanks for your efforts spent on this library). I've got a question about the code inside. Right now I'm using python-saml library as a part of python-social-auth library. In my case I need to inherit python-saml classes in order to override some signature validation functions. I dived into the code and found out that there are a lot of private variables like errors, request_data etc. If you want to access them in derived class you have to type self._MyBaseClassrequest_data or self._MyBaseClasserrors because simple self.__errors is not working in this case. The question: What's the reason?) Seriously, it makes overriding more complicated than it should be, I think. Thanks for response!

pitbulk commented 8 years ago

Hi @dangusev

I implemented this toolkit based on the php-saml toolkit where I defined some attributes as private attributes.

If you consider that some of those attributes should be converted in public attributes, we can discuss.

For example I don't see why __request_data should be accessible.

What are you trying to develop at python-social-auth library?

dangusev commented 8 years ago

I think that attribute should be defined as private only in exceptional case. In current implementation it makes a lot of troubles during inheritance because of name mangling inside (https://docs.python.org/2/tutorial/classes.html#private-variables-and-class-local-references). If you want to mark attribute as a non-public just use single underscore. Python handles them as usual.

pitbulk commented 8 years ago

I understand the problem you are getting with the inheritance, but I really wanted to define the attributes as private.

There are several OneLogin SAML toolkits (php, java, dotnet, ruby, python) so I'm trying to implement them in a similar way in order to save time on maintenance.

If you describe to me what are you trying to implement, it makes sense and is not possible due an attribute is private then I will provide to you a solution for that.

dangusev commented 8 years ago

In my case private attributes are just annoying inconveniences). I can handle it but code looks ugly because of _OneLogin_Saml2_Authsession_index, _OneLogin_Saml2_Authattributes and other attrs like that.

The real trouble is a lack of possibility to override the way how to validate response signature. In my application I have to use another software for signature validation. In current implementation it's hard to customize this functionality because I cannot change the behavior of OneLogin_Saml2_Utils.validate_sign() function inside OneLogin_Saml2_Response.is_valid() method. And I don't see any other ways to override it except of overriding of whole OneLogin_Saml2_Response.is_valid().

I could provide a PR with some improvements if you don't mind.

pitbulk commented 8 years ago

The fact that you want to override the way how to validate response signature scared me.

If you have an improved version, I can include it in the toolkit, otherwise, If you are not a fan of dm.xmlsec.binding, why don't you use python-3-saml that uses python-xmlsec ? (Or pysaml2)

PRs are always welcome, I will study them but can't promise you to include your proposal.

dangusev commented 8 years ago

Because of some reasons we can use only particular cryptographic software. I would be happy to avoid it, but these are the terms of our project.