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

WIP: Add support for flags in ldap.dn2str() #466

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

spaceone
Copy link
Contributor

@spaceone spaceone commented Apr 1, 2022

In C dn2str() supports flags which works by providing LDAP_DN_FORMAT_UFN, LDAP_DN_FORMAT_AD_CANONICAL, ….
These symbols do exist in Python, but could not be used ultimately because the Python counterpart was pure Python and did not pass to dn2str(3).

Fix #257

In C `dn2str()` supports `flags` which works by providing `LDAP_DN_FORMAT_UFN`, `LDAP_DN_FORMAT_AD_CANONICAL`.
These symbols do exist in Python, but could not be used ultimately because the Python counterpart was pure Python and did not pass to `dn2str(3)`.

Fix python-ldap#257
@droideck
Copy link
Contributor

@spaceone hi!
The PR says 'WIP', - is it still true?

@spaceone
Copy link
Contributor Author

@spaceone hi! The PR says 'WIP', - is it still true?

@droideck Well, I would say it needs code clenaup but the functionality is done. But before I try to cleanup I would like to get feedback.

@droideck
Copy link
Contributor

Overall, I think it'll be nice to have the feature.
@mistotebe what do you think?

@mistotebe
Copy link
Contributor

I agree, getting better DN handling would be nice.

@@ -158,15 +158,42 @@ def test_dn2str(self):
[('dc', 'example', 1)],
[('dc', 'com', 1)]
]),
'uid=test\\, 42,ou=Testing,dc=example,dc=com'
'uid=test\\2C 42,ou=Testing,dc=example,dc=com'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

API change: I don't find a way to prevent this

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though libldap does some DN normalisation, we can't guarantee to our users that the output of dn2str is going to be (character to character) identical. Maybe we should make it explicit that the DN they get from it is always going to be "the same one" but the string representation might evolve without notice? Certainly something that makes them think twice before using it as a dict key, etc.

)
self.assertEqual(
ldap.dn.dn2str([
[('cn', 'äöüÄÖÜß', 4)],
[('dc', 'example', 1)],
[('dc', 'com', 1)]
]),
'cn=äöüÄÖÜß,dc=example,dc=com'
r'cn=\C3\A4\C3\B6\C3\BC\C3\84\C3\96\C3\9C\C3\9F,dc=example,dc=com'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

API change: I don't find a way to prevent this

Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely intentional and the old value incorrect, we're telling libldap to treat the value as non-printable (LDAP_AVA_NONPRINTABLE == 0x04) and escape accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, I overlooked that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same vor LDAP_AVA_STRING:

>>> ldap.dn.dn2str([[('cn', 'äöüÄÖÜß', ldap.AVA_BINARY)],[('dc', 'example', 1)],[('dc', 'com', 1)]])
'cn=#C3A4C3B6C3BCC384C396C39CC39F,dc=example,dc=com'
>>> ldap.dn.dn2str([[('cn', 'äöüÄÖÜß', ldap.AVA_NULL)],[('dc', 'example', 1)],[('dc', 'com', 1)]])
'cn=\\C3\\A4\\C3\\B6\\C3\\BC\\C3\\84\\C3\\96\\C3\\9C\\C3\\9F,dc=example,dc=com'
>>> ldap.dn.dn2str([[('cn', 'äöüÄÖÜß', ldap.AVA_STRING)],[('dc', 'example', 1)],[('dc', 'com', 1)]])
'cn=\\C3\\A4\\C3\\B6\\C3\\BC\\C3\\84\\C3\\96\\C3\\9C\\C3\\9F,dc=example,dc=com'
>>> ldap.dn.dn2str([[('cn', 'äöüÄÖÜß', ldap.AVA_NONPRINTABLE)],[('dc', 'example', 1)],[('dc', 'com', 1)]])
'cn=\\C3\\A4\\C3\\B6\\C3\\BC\\C3\\84\\C3\\96\\C3\\9C\\C3\\9F,dc=example,dc=com'

Comment on lines +60 to +61
#if not dn:
# return ''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

required?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@mistotebe mistotebe Sep 28, 2022

Choose a reason for hiding this comment

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

Unless we want to make the distinction for None and make dn2str(None) is None, might be too much hassle though.

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

Successfully merging this pull request may close these issues.

ldap.dn.dn2str() does not support flags
3 participants