[Freeipa-devel] [PATCHES] Add new set of base classes for IPA plugins using LDAP.

[Rob Crittenden]

Pavel Zuna wrote:
> Dear freeipa-devel,
> I know we agreed that we're going to post one patch per email, but it 
> this case I think it's justified. Patches 0001 through 0005 are 
> prerequisites for the new base classes. Before describing the individual 
> patches, let me explain a little bit of what I had in mind when 
> implementing this addition.
> 
> For the last few months, I've been mostly working the new LDAP backend 
> and IPA plugins that use it (i.e. almost all IPA plugins). I tried to 
> make the new backend more powerful and flexible. Unfortunately, to take 
> full advantage of the new potential, old plugins had to be rewritten. As 
> there were several inconsistencies across them (output, naming and 
> sometimes even behavior) and about half of them were still using old 
> features from IPAv1, I thought that some re-factoring couldn't hurt 
> after all.
> 
> After porting a few plugins to the new backend, I realized that most of 
> them were doing pretty much the same thing - manipulating LDAP entries. 
> The only difference was that some plugins were generating attributes in 
> specific ways based on user input. So, I figured that most of the 
> generic work could be done in one place and that's when I started 
> working on these new base classes.
> 
> Let's take a quick look at them:
> 
> LDAPObject
>  - extends frontend.Object
>  - represents a type of LDAP entry
>    (or sub-entry if a parent_key=True parameter is present)
> 
> The following methods are to be used with LDAPObject, they are named 
> after the CRUD methods they extend with an 'LDAP' prefix. They take 1 
> argument (the primary key) in the case their LDAPObject is an entry and 
> 2 arguments (the parent and primary key, in that order) if their 
> LDAPObject is a subentry:
> 
> LDAPCreate
> LDAPRetrieve
> LDAPUpdate
> LDAPDelete
> 
> Then there's LDAPSearch, which is a bit special. It takes only the 
> optional criteria argument if its LDAPObject is an entry. If its 
> LDAPObject is a sub-entry then it requires the parent key as the first 
> argument. The search logic works as follows:
> 
> 1) If there is no parameters (except for the parent key in the case of 
> sub-entries), all entries of the given type are returned.
> 
> 2) If there is only the criteria parameter, all entries of the given 
> type with one or more of their default attributes containing the value 
> of criteria are returned.
> 
> 3) If there is an options specified, all entries of the given type with 
> the corresponding attribute EQUAL to the value specified are returned. 
> If more than one options is specified this way, only entries matching 
> ALL the conditions are returned.
> 
> 2) and 3) can be combined, so for example, we can make a search for 
> entries who have a specific attribute equal to 'foo' and some of their 
> other attributes containing 'bar'.
> 
> All of these base classes implement a 'callback' method, that is called 
> just before invoking python-ldap through the backend. Subclasses can use 
> it to execute their specific code after all the boring stuff has been 
> done for them.
> 
> In the future I'm planning to write more base classes for other uses 
> cases such as adding/removing values from multivalued attributes.
> 
> 
> Patches descriptions:
> ---------------------
> 
> 0001: Make it possible for subclasses of Command and co. to use their 
> own __public__.
> 
> The problem was with PluginProxy only taking the base class' __public__ 
> into consideration.
> 
> 
> 0002: Add parent_key to keyword arguments of Param base class.
> 
> 
> 0003: Add support for parent key in Object base class.
> 
> 
> 0004: Make CRUD base classes take parent key parameters into 
> consideration when
> generating args.
> 
> 
> 0005: Add delete_tree method to ldap2 allowing recursive deletes of 
> whole LDAP branches.
> 
> Some plugins were doing this "manually", so I figured it might as well 
> be provided by the LDAP API.
> 
> 
> 0006: Add new set of base classes for plugins using LDAP.
> 
> 
> To see the new bases classes in action, take a look at the next patch 
> I'm about to send to freeipa-devel.

A few comments.

I'd like Jason to comment on patch 0001 and 0002. It looks ok to me but 
he may balk at changing __public__.

In LDAPObject you use a variable named keys. It isn't immediately clear 
what this is for (made clear after reading get_dn). If I understand 
parent_key and keys you are providing a general way of constructing the 
DN of the record, right? I'm not sure that parent_key and keys are very 
descriptive. It seems like primary_key is the rdn, parent_key is the 
next value in the DN and keys can contain either primary_key or both 
primary_key and parent_key. This is particularly confusing if you aren't 
using LDAPObject as a base class. Why would you want to return 
parent_key instead of primary_key when getting the arguments?

I like the idea of a callback but is this limited by only returning the 
dn? What kinds of things do you envision this being used for? (e.g. what 
it if was smart enough to add in extra attributes the user didn't or 
couldn't ask for?).

BTW this sort of callback was always envisioned, I'm glad you're adding 
it. We had thought about having a pre- and post- callback so you could 
manage the data going in and coming out. What do you think?

You define backend_name in LDAPObject but I don't see it used anywhere. 
What did you have in mind for this?

If this is going to essentially be the base class for all LDAP-based 
plugins I think I'd like to see some more inline documentation so the 
output of epydoc will be helpful.

I wonder if delete_tree() isn't a bit dangerous. What if someone manages 
to pass in dc=example,dc=com?

I think that ldap.passwd_s() will blow up if you pass in None as the old 
password.

You made some changes to the modlist generator. Are you sure that this:

     modlist.append((_ldap.MOD_DELETE, k, None))

will work with python-ldap found on older systems (e.g. RHEL 5)?

I guess the basegroup class could be re-based to work on this and be 
even simpler :-)

Something that this will need at some point is a pointer to where in 
cn=ipaconfig the list of default objectclasses can be found. We may not 
end up defining this for every single object type but I think for many 
we will (users and groups at least). It would be nice if there was a way 
to just pass in an attribute name where this list can be found.

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090604/e01ceb23/attachment.bin>