prettify XML string output by registering default namespace prefixes

[spaceone]

[spaceone]

See also https://github.com/rohe/pysaml2/issues/249

[c00kiemon5ter]

This is nice to have. It breaks tests as they depend on the exact output which includes the namespaces. We should look for ways to avoid depending on that.

[peppelinux]

This is an important PR, we shouldn't forget it! More then 3 years are passed

[spaceone]

I replaced/fixed the tests with the following script:

import pipes
import subprocess
NAMESPACE = 'urn:oasis:names:tc:SAML:2.0:assertion'
SAMLP_NAMESPACE = 'urn:oasis:names:tc:SAML:2.0:protocol'
XSI_NAMESPACE = 'http://www.w3.org/2001/XMLSchema-instance'
XS_NAMESPACE = 'http://www.w3.org/2001/XMLSchema'
DS_NAMESPACE = 'http://www.w3.org/2000/09/xmldsig#'
MDUI_NAMESPACE = "urn:oasis:names:tc:SAML:metadata:ui"
MD_NAMESPACE = "urn:oasis:names:tc:SAML:2.0:metadata"
DEFAULT_NS_PREFIXES = {'saml': NAMESPACE, 'samlp': SAMLP_NAMESPACE, 'ds': DS_NAMESPACE, 'xsi': XSI_NAMESPACE, 'xs': XS_NAMESPACE, 'mdui': MD_NAMESPACE, 'md': MD_NAMESPACE}


for ns, xmlns in DEFAULT_NS_PREFIXES.items():
        for i in range(6):
                code = '''sed -i 's/ns%d/%s/g' $(git grep -l 'xmlns:ns%d="%s"')''' % (i, ns, i, xmlns)
                if 0 == subprocess.call(code, shell=True):
                        subprocess.call('git ci -a -m %s' % (pipes.quote(code),), shell=True)

But unfortionately still 7 tests are failing.

[peppelinux]

Hi @spaceone,

I also extended your code here https://github.com/IdentityPython/pysaml2/pull/625/files before your latest commits. If you agree we can work all together to make it to work and get it to be merged.

I don't think that an hard-coded subprocess call, with an embedded shell env in it, could be considered as a good solution, despite this I understand your contribution logic and your personal need to get it to work.

If this particular feature will be considered by lead project mantainers as important, as I also do, I'd like to make this proposals:

• keep my PR as reference for my needs and for comparison purpose, considering at the same time this PR as the master;
• To patch the unit test to let tests passes;
• Move that code from init to a specialized file and refactor the import of its methods and attributes in the source tree;
• Make sign and digest algs overloadable from the config file, because also these should be considered with configurable namespace, in metadata. This will also make the user disable the weak ones (https://github.com/IdentityPython/pysaml2/issues/450 and https://github.com/IdentityPython/pysaml2/issues/421).

I consider this feature important because friends from Federation Service pointed me out that there are some SP that could not understand well those metadata created in this way and this could be a problem for my new pysaml2 IDP validation (but I also don't think strictly the same). Thank you all, hope the best thing for this PR :+1:

[peppelinux]

In my last commit I get all the test passes without any alchemies :)

The key is ElementTree. saml2.__init__.SamlBase uses .register_prefix() and .to_string(), and they can be called or not in different moments, runtime. Forcing nsprefix with default arguments would break the legacy pysaml2 approach, unit tests shows this.

As I also read into SAML standard documents, those namespace will not create functional problems if they should changed, or if they would follow ElementTree's automatic assignment, but I also think that is very important to follow OASIS conventions to remove all doubts to other collegues and operators in the identity federations. I agree about the fact that this is only a cosmetic taste, but help us to give to pysaml2 an even better posture in comparison to leading softwares like Shibboleth.

The even better news is, that a user can register his preferred ns_prefixes in his general config.

[spaceone]

@peppelinux Could you add the register_namespace() thing into the tests, e.g. via pytest conftest.py ? I think I wouldn't like that these namespaces are registered globaly in pysaml itself as I have pysaml2 imported in projects where I also do other XML stuff. Otherwise I am fine, If you have a solution to fix the test cases without touching each test case. But I like my approach also for readability reasons.

[peppelinux]

@spaceone in my PR BaseSaml.register_prefix Is a stati method and It can call register_namespace, globally, runtime. You can change those ns each time in differenti time, also in the settings of your project.

Anyone can register his own namespace globally, calling BaseSaml.register_prefix runtime. The test that checks this behaviour Is test_88.

Each xml string in the tests can be generalized with the defaults, taken by the default ns dict, with .format(). I Will.

I'd like to have OASIS default namespaces in pysaml2 as default, @cookiemonster what do you think?

Thank you for the reply and for your PR, your idea made me see something important

[Vinod83GH]

@spaceone , @peppelinux : Can your team fix and merge this commit as we are totally dependent on this pysaml2 package for generating SAML response which is by default giving me xml namespaces as ns0, ns1, ns2. We are nearing our deployment to PROD. please let me know if any other way to include custom namespace mapping via CONFIG, or any other attribute to pas to - self.server.create_authn_response()

[peppelinux]

@Vinod83GH this is the current implementation that is going to be merged: https://github.com/IdentityPython/pysaml2/pull/625

[c00kiemon5ter]

@spaceone , @peppelinux : Can your team fix and merge this commit as we are totally dependent on this pysaml2 package for generating SAML response which is by default giving me xml namespaces as ns0, ns1, ns2. We are nearing our deployment to PROD. please let me know if any other way to include custom namespace mapping via CONFIG, or any other attribute to pas to - self.server.create_authn_response()

The namespace-prefix names do not matter; they are purely cosmetic. Why do you think ns0, ns1 and so on, is problematic? If any application is dependent on a namespace prefix name, then it is violating the XML specification. There is nothing that pysaml2 can do to help with that.

For this reason, this patch is low-priority.

[danaru]

@c00kiemon5ter I would really appreciate if this patch will be merged as there is an issue with signed responses, when saml namespaces required

[peppelinux]

@c00kiemon5ter I would really appreciate if this patch will be merged as there is an issue with signed responses, when saml namespaces required

@danaru the PR is moved here https://github.com/IdentityPython/pysaml2/pull/625

[spaceone]

Sorry that this has taken so much time! The tests are passing now, I squashed the commits and grouped them.

[peppelinux]

I did the same here https://github.com/IdentityPython/pysaml2/pull/625

[peppelinux]

Ok, it seems that our PRs are coupled... I used some from your and you the same from mine :ok_hand: great

[spaceone]

Ok, it seems that our PRs are coupled... I used some from your and you the same from mine ok_hand great

Oh okay, I thought your implementation was a different approach to reach the same thing. How to proceed? I can have a look what we can unify.

[peppelinux]

The last word to @c00kiemon5ter

[spaceone]

@peppelinux I used your variable name now. @c00kiemon5ter should I remove the global constants or leave it? Should I import the global constants in the submodules as suggested in #625?