Re: [Freeipa-devel] [PATCH 24/24] Add utility classes for handling DN's along with their, unittest.
John Dennis wrote: Revised patch attached. Added copyright notice. Added support for concatenation and in-place addition for a few more types. Updated the unit test for the new functionality. Correct import statement in unit test. I can work with the updated patch you sent but it isn't in a format that git-am can handle. See this wiki page for patch naming conventions and patch generation commands: https://fedorahosted.org/freeipa/wiki/PatchFormat rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 24/24] Add utility classes for handling DN's along with their, unittest.
John Dennis wrote: Revised patch attached. Added copyright notice. Added support for concatenation and in-place addition for a few more types. Updated the unit test for the new functionality. Correct import statement in unit test. Ack, pushed to master and ipa-2-0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 24/24] Add utility classes for handling DN's along with their, unittest.
John Dennis wrote:
This adds a new module and set of classes to ipalib for handling DN's.
Please see the module doc and class doc for full explanation.
Included is a very complete unit test for the module. At close to 900
lines of code the unit test exercises just about every conceivable way
these objects can be used.
The module doc touches on some of the problems found in our existing
code which handles DN's, which this module is meant to provide fixes
for. A more complete write-up of the existing code issues will follow on
the list.
Comments welcome of course.
Another patch will follow for comma's in privileges. The
test_role_plugin.py unit test was modified to introduce a comma, but
there were many failures because of improper DN handling in the core
code (as well as limitations of the unit test framework). The next patch
introduces a number of fixes, some of which are dependent upon the use
of the classes introduced here. With the fixes in the next patch the
test_role_plugin unit test once again fully succeeds.
Am I misreading the documentation on how one can create a DN?
print container
cn=users,cn=accounts
print basedn
dc=example,dc=com
str(DN(container, basedn))
'cn=users,cn=accounts=dc\\=example\\,dc\\=com'
uid='rcrit'
rdnattr='uid'
str(DN('%s=%s' % (rdnattr, uid), container, basedn))
'uid=rcrit=cn\\=users\\,cn\\=accounts,dc=example,dc=com'
The patch requires one very minor change, the import from dn should be
from ipalib.dn import ... We run the tests from the top-level.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 24/24] Add utility classes for handling DN's along with their, unittest.
On 06/20/2011 10:01 AM, Rob Crittenden wrote:
Am I misreading the documentation on how one can create a DN?
print container
cn=users,cn=accounts
print basedn
dc=example,dc=com
str(DN(container, basedn))
'cn=users,cn=accounts=dc\\=example\\,dc\\=com'
uid='rcrit'
rdnattr='uid'
str(DN('%s=%s' % (rdnattr, uid), container, basedn))
'uid=rcrit=cn\\=users\\,cn\\=accounts,dc=example,dc=com'
Either you misread the documentation, or I wrote it poorly. In either
case it's obvious it needs to be reworked to be clearer. Let me take
another crack at explaining :-)
[Caveat: I've made some simplifying assumptions below, e.g. RDN's can be
multi-valued, the classes handle everything correctly but only if you
use them properly, if you're working with multi-valued RDN's you'll have
to dig just a tad deeper to use the classes correctly.]
When you supply a sequence of strings those strings are assumed to be
the type (e.g. name) and value of a RDN. But of course since they must
be pairs the parser looks for adjacent pairs of strings in the sequence.
So taking your example cn=users forms the first RDN, thus:
'cn', 'users'
is the type,value pair of an RDN.
If that were followed by:
'cn', 'accounts'
the next RDN would be: cn=accounts and the sequence:
'cn', 'users', 'cn', 'accounts'
would produce:
cn=users,cn=accounts
O.K. so why wouldn't you just say:
'cn=users,cn=accounts'
instead of the 2 pairs:
'cn', 'users', 'cn', 'accounts'
it's so much simpler right?
The reason, and this is key, is because 'cn=users,cn=accounts' is DN
syntax and is subject to DN encoding rules. What is on the left and
right side of the equal sign may NOT be the string values you expect
them to be, rather they might be encoded. The only way to treat the LHS
and RHS of an RDN as the ORIGINAL strings you're expecting is to
reference them individually via the classes in the module. The classes
know how to encode and decode and they can do it in a smart fashion.
It's NEVER a good idea to construct DN's from DN strings. Why? Because
DN strings are subject to various escaping rules which after being
applied produces what I call the encoded value of the DN. To complicate
matters different encodings can produce the same DN. Once you get into
these edge cases most simple expectations go out the window.
The simple coding answer is to always work with DN, RDN, or AVA objects
and never with DN string syntax. The objects are aware of each other and
perform the correct class conversions. The only time you need DN string
syntax is at the moment you pass the DN into a LDAP library, and that is
as simple as calling str() on the object.
O.K. so why do the classes accept DN syntax, you just told me never to
use it! Well welcome to the real world, where not everything has been
converted to use the new classes yet and the reality is sometimes you
get strings in DN syntax. We don't want to be so rigid we barf, rather
than being pedantic we support DN syntax but it comes with a GIANT
WARNING of programmer beware, use at your own risk only if you know what
you're doing.
So if DN syntax is a string and the type and value of an RDN are also
strings how do the classes tell the difference when it's looking at a
sequence of values used to construct a DN? It does it by looking for
contiguous pairs of strings in the sequence, when it finds two adjacent
strings it pulls them from the sequence and forms an RDN from them. A
string is interpreted as DN syntax to be independently parsed if and
only if it's not a member of a pair of strings in the sequence. Recall
the sequence can include DN, RDN and AVA classes as well as strings.
Thus in your case what happened was you had two strings in the
constructor sequence:
'cn=users,cn=accounts', 'dc=example,dc=com'
and that got interpreted as the LHS and RHS of an RDN.
The right way to have done this would have been to construct two DN's,
one for the base and one for the container, for example:
base_dn = DN('dc', 'example', 'dc', 'com')
container_dn = DN('cn', 'users, 'cn', 'accounts')
then any new DN can be constructed via:
user_dn = DN('cn', 'Bob', container_dn, base_dn)
Make sense?
Note the syntax for constructing the DN objects is very flexible, you
could build it up from a sequence of RDN objects or you could put the
values in a list and pass the list to the constructor, e.g.
base_dn_list = ['dc', 'example', 'dc', 'com']
base_dn = DN(*base_dn_list)
or even:
base_dn_list = [RDN('dc', 'example'), RDN('dc', 'com')]
base_dn = DN(*base_dn_list)
The patch requires one very minor change, the import from dn should be
from ipalib.dn import ... We run the tests from the top-level.
O.K. will do. Also I added some new functionality I discovered was
useful when I was making other fixes, such as the ability to use
in-place addition (+= operator) and concatenation (+ operator) with DN
syntax on the RHS. The unit test was enhanced to
Re: [Freeipa-devel] [PATCH 24/24] Add utility classes for handling DN's along with their, unittest.
John Dennis wrote:
On 06/20/2011 10:01 AM, Rob Crittenden wrote:
Am I misreading the documentation on how one can create a DN?
print container
cn=users,cn=accounts
print basedn
dc=example,dc=com
str(DN(container, basedn))
'cn=users,cn=accounts=dc\\=example\\,dc\\=com'
uid='rcrit'
rdnattr='uid'
str(DN('%s=%s' % (rdnattr, uid), container, basedn))
'uid=rcrit=cn\\=users\\,cn\\=accounts,dc=example,dc=com'
Either you misread the documentation, or I wrote it poorly. In either
case it's obvious it needs to be reworked to be clearer. Let me take
another crack at explaining :-)
[Caveat: I've made some simplifying assumptions below, e.g. RDN's can be
multi-valued, the classes handle everything correctly but only if you
use them properly, if you're working with multi-valued RDN's you'll have
to dig just a tad deeper to use the classes correctly.]
When you supply a sequence of strings those strings are assumed to be
the type (e.g. name) and value of a RDN. But of course since they must
be pairs the parser looks for adjacent pairs of strings in the sequence.
So taking your example cn=users forms the first RDN, thus:
'cn', 'users'
is the type,value pair of an RDN.
If that were followed by:
'cn', 'accounts'
the next RDN would be: cn=accounts and the sequence:
'cn', 'users', 'cn', 'accounts'
would produce:
cn=users,cn=accounts
O.K. so why wouldn't you just say:
'cn=users,cn=accounts'
instead of the 2 pairs:
'cn', 'users', 'cn', 'accounts'
it's so much simpler right?
The reason, and this is key, is because 'cn=users,cn=accounts' is DN
syntax and is subject to DN encoding rules. What is on the left and
right side of the equal sign may NOT be the string values you expect
them to be, rather they might be encoded. The only way to treat the LHS
and RHS of an RDN as the ORIGINAL strings you're expecting is to
reference them individually via the classes in the module. The classes
know how to encode and decode and they can do it in a smart fashion.
It's NEVER a good idea to construct DN's from DN strings. Why? Because
DN strings are subject to various escaping rules which after being
applied produces what I call the encoded value of the DN. To complicate
matters different encodings can produce the same DN. Once you get into
these edge cases most simple expectations go out the window.
The simple coding answer is to always work with DN, RDN, or AVA objects
and never with DN string syntax. The objects are aware of each other and
perform the correct class conversions. The only time you need DN string
syntax is at the moment you pass the DN into a LDAP library, and that is
as simple as calling str() on the object.
O.K. so why do the classes accept DN syntax, you just told me never to
use it! Well welcome to the real world, where not everything has been
converted to use the new classes yet and the reality is sometimes you
get strings in DN syntax. We don't want to be so rigid we barf, rather
than being pedantic we support DN syntax but it comes with a GIANT
WARNING of programmer beware, use at your own risk only if you know what
you're doing.
So if DN syntax is a string and the type and value of an RDN are also
strings how do the classes tell the difference when it's looking at a
sequence of values used to construct a DN? It does it by looking for
contiguous pairs of strings in the sequence, when it finds two adjacent
strings it pulls them from the sequence and forms an RDN from them. A
string is interpreted as DN syntax to be independently parsed if and
only if it's not a member of a pair of strings in the sequence. Recall
the sequence can include DN, RDN and AVA classes as well as strings.
Thus in your case what happened was you had two strings in the
constructor sequence:
'cn=users,cn=accounts', 'dc=example,dc=com'
and that got interpreted as the LHS and RHS of an RDN.
The right way to have done this would have been to construct two DN's,
one for the base and one for the container, for example:
base_dn = DN('dc', 'example', 'dc', 'com')
container_dn = DN('cn', 'users, 'cn', 'accounts')
then any new DN can be constructed via:
user_dn = DN('cn', 'Bob', container_dn, base_dn)
Make sense?
Note the syntax for constructing the DN objects is very flexible, you
could build it up from a sequence of RDN objects or you could put the
values in a list and pass the list to the constructor, e.g.
base_dn_list = ['dc', 'example', 'dc', 'com']
base_dn = DN(*base_dn_list)
or even:
base_dn_list = [RDN('dc', 'example'), RDN('dc', 'com')]
base_dn = DN(*base_dn_list)
The patch requires one very minor change, the import from dn should be
from ipalib.dn import ... We run the tests from the top-level.
O.K. will do. Also I added some new functionality I discovered was
useful when I was making other fixes, such as the ability to use
in-place addition (+= operator) and concatenation (+ operator) with DN
syntax on the RHS. The unit test was enhanced to support those cases.
I'll resubmit the patch with better doc
Re: [Freeipa-devel] [PATCH 24/24] Add utility classes for handling DN's along with their, unittest.
On 06/20/2011 04:06 PM, Rob Crittenden wrote: Take a look at ipalib/constants.py, it is full of containers like this. It is hard to review this patch without seeing how it will be used in the framework, are you planning on replacing all of these with DN constructors? Yup, I'm aware of these. There are two easy solutions: 1) Leave the containers as they are. They can always be used with DN class. This is another one of the reasons the DN class accepts DN syntax (for legacy and simplicity). The existing containers are all simple DN's, their encoded value and decoded values are identical. So as long as any programmer who adds a new container understands the encoding rules all will be good. (The problem with your example test was simply you didn't use the constructor correctly. See [PATCH 28/28] for just one way to construct a DN using the existing container and base strings as we currently have them defined.) 2) Convert the containers to DN objects. From a robustness point of view this is preferred. Converting them would be trivial. Once the containers are DN objects the programmer can't make unintentional mistakes and the objects combine correctly. The problem we were having is you CANNOT treat DN's as simple strings, they aren't simple strings, they are complex objects which in some instances are equivalent to simple strings. My thought was to do the conversion to DN objects incrementally. I deliberately wrote the classes to support incremental migration. We start with the bugs which we know are due to problems with DN handling and convert those first on an as needed basis rather than as a potentially large disruptive modification. The bottom line is we need to have some way to form DN's correctly from pieces and pick DN components apart into component pieces again. We want common utility code to do this and not have everybody take a crack at it in isolated cases when trying to fix bugs. We also want it to support our legacy implementation and be simple to use (at least those were the goals I tried to hit). Multi-valued RDNs are 100% guaranteed in IPA so the easier it is to work with them the better. I believe the classes make handling multi-valued RDN's quite easy. It's just when you start to try and explain things it seems easier to not fill the explanation with a bunch of caveats. If you understand mutli-valued RDN's and the AVA's they're composed from the classes will make perfect sense and combine easily. -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 24/24] Add utility classes for handling DN's along with their, unittest.
On 06/20/2011 04:51 PM, John Dennis wrote: On 06/20/2011 04:06 PM, Rob Crittenden wrote: Take a look at ipalib/constants.py, it is full of containers like this. It is hard to review this patch without seeing how it will be used in the framework, are you planning on replacing all of these with DN constructors? Yup, I'm aware of these. There are two easy solutions: 1) Leave the containers as they are. They can always be used with DN class. This is another one of the reasons the DN class accepts DN syntax (for legacy and simplicity). The existing containers are all simple DN's, their encoded value and decoded values are identical. So as long as any programmer who adds a new container understands the encoding rules all will be good. (The problem with your example test was simply you didn't use the constructor correctly. See [PATCH 28/28] for just one way to construct a DN using the existing container and base strings as we currently have them defined.) 2) Convert the containers to DN objects. From a robustness point of view this is preferred. Converting them would be trivial. Once the containers are DN objects the programmer can't make unintentional mistakes and the objects combine correctly. The problem we were having is you CANNOT treat DN's as simple strings, they aren't simple strings, they are complex objects which in some instances are equivalent to simple strings. I meant to add that if the container and base definitions in constants.py are converted to DN objects (a good idea I believe and easy) then in theory everything should still just work because when a DN object is evaluated in a string context (the only way these constants are used I believe) you get the identical string as to what we currently have in constants.py. The pay-off comes mostly with user supplied values which get used in conjunction with the container+base DN's because unlike what's in constants.py user supplied values are not crafted by programmers aware of the rules of LDAP syntax. It's the DN's which result from user supplied values which are the primary problem areas. It really doesn't make much sense to use DN objects in selected known problem areas, for consistency and robustness we should use just one idiom throughout the code base, things should just work much better all around. But the design of the classes allow for incremental conversion of the code as we converge on more consistent DN handling (a win/win situation). BTW, it was very difficult to track down how some values were getting corrupted along the way. After looking at both the client and server side of things and the way we designed the RPC API mechanism it became clear to me there was no easy fix, no band-aids, you really have to treat a DN string as data with known properties and behavior, e.g. an object that knows how to operate on it's internal data, everything else just has different failure modes. My thought was to do the conversion to DN objects incrementally. I deliberately wrote the classes to support incremental migration. We start with the bugs which we know are due to problems with DN handling and convert those first on an as needed basis rather than as a potentially large disruptive modification. The bottom line is we need to have some way to form DN's correctly from pieces and pick DN components apart into component pieces again. We want common utility code to do this and not have everybody take a crack at it in isolated cases when trying to fix bugs. We also want it to support our legacy implementation and be simple to use (at least those were the goals I tried to hit). Multi-valued RDNs are 100% guaranteed in IPA so the easier it is to work with them the better. I believe the classes make handling multi-valued RDN's quite easy. It's just when you start to try and explain things it seems easier to not fill the explanation with a bunch of caveats. If you understand mutli-valued RDN's and the AVA's they're composed from the classes will make perfect sense and combine easily. -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
