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

[Pavel Zůna]

Rob Crittenden wrote:
> 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 was trying to accomplish 2 things by introducing parent_key:
1) provide a generic way of constructing the DN
2) generate different arguments for CRUD methods of sub-entries

Some plugins have to deal with entries and their direct sub-entries (DNS 
plugin: zone->resource, automount plugin: map->key). When accessing a 
sub-entry, we need to know its RDN (primary_key), but also its parent 
RDN (parent_key). parent_key is never returned instead of primary_key. 
If present, parent_key is returned first followed by primary_key.

You're right that it can be confusing when not using LDAPObject as a 
base class, I'll change the CRUD base classes back and override 
get_args() in LDAP* methods. 'parent_key' and 'keys' might not be the 
most descriptive names, but I couldn't think of anything better - any 
suggestions?

> 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?).
Actually they are smarter than they look. :) Since parameters in python 
are passed by reference (well not exactly, it's actually pass by value, 
but names are the values and names are like references...) we can modify 
their contents in the callback. Unfortunately, this only applies to 
mutable types, so DN/filter (immutable strings) have to be returned.

> 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?
Sounds like a good idea.

> You define backend_name in LDAPObject but I don't see it used anywhere. 
> What did you have in mind for this?
backend_name is a forgotten feature I found in the depths of ipalib 
code. When the Object instance is being initialized, its 'backend' 
attribute is set to api.Backend.<backend_name>.

> 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'll add it to my TODO list. I was thinking about writing a general 
HOWTO for plugin authors anyway.

> I wonder if delete_tree() isn't a bit dangerous. What if someone manages 
> to pass in dc=example,dc=com?
I can see your point. I'll remove delete_tree and rewrite LDAPDelete to 
delete sub-entries "manually".

> I think that ldap.passwd_s() will blow up if you pass in None as the old 
> password.
Probably, old pass is supposed to default to '' not None...

> 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)?
To be honest, I don't know. python-ldap docs say nothing about older 
versions, but after some googling, I found code dating 2003 that was 
using this.

> I guess the basegroup class could be re-based to work on this and be 
> even simpler :-)
Yeah, most plugins could be re-based on this and would become a lot 
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.
Ok, that should be pretty straight forward.

> rob

I'm going to fix everything discussed in this e-mail and if it gets the 
green light from Jason, we might finally be ready to make the LDAP 
backend switch (at least for plugins).

Pavel