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

Using of private attributes (with double underscore) #137

Open
dangusev opened this issue May 17, 2016 · 6 comments
Open

Using of private attributes (with double underscore) #137

dangusev opened this issue May 17, 2016 · 6 comments

Comments

@dangusev
Copy link

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._MyBaseClass__request_data or self._MyBaseClass__errors 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
Copy link
Contributor

pitbulk commented May 17, 2016

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
Copy link
Author

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
Copy link
Contributor

pitbulk commented May 17, 2016

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
Copy link
Author

In my case private attributes are just annoying inconveniences). I can handle it but code looks ugly because of _OneLogin_Saml2_Auth__session_index, _OneLogin_Saml2_Auth__attributes 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
Copy link
Contributor

pitbulk commented May 17, 2016

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
Copy link
Author

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.

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

No branches or pull requests

2 participants