[Freeipa-devel] Re: [PATCHES] Make plugins use baseldap classes.

[Pavel Zuna]

Rob Crittenden wrote:
> Pavel Zůna wrote:
>> This is a series of patches that depends on patches:
>> - Improve attribute printing in the CLI.
>> - Improve ipalib.plugins.baseldap classes.
>>
>> All plugins are converted to extend baseldap classes. This makes 
>> things more consistent, fixes some general bugs (with return values 
>> for example) and it also makes plugins easier to maintain (as it 
>> removes a lot of duplicate code).
>>
>> Because baseldap classes have features that enable us to define 
>> relationships between plugins, I thought it might be best to submit 
>> all of the conversions at once and have all the relationships fully 
>> defined.
>>
>> Affected plugins:
>> config
>> user
>> host
>> service
>> group
>> hostgroup
>> netgroup
>> rolegroup
>> taskgroup
>> pwpolicy
>>
>> There's also a patch that fixes all unit tests.
>>
>> Jenny, I included you to Cc, because the patch introduces a lot of 
>> changes to the UI (and you're probably not going to like me for this).
>>
>> Each command extending the LDAP* base classes now has a --raw option. 
>> Without it, data from LDAP is formated and translated to human 
>> readable. For example:
>>
>> # ipa user-show admin --all
>> ----------
>> user-show:
>> ----------
>> User: admin
>>   user id: admin
>>   full name: Administrator
>>   last name: Administrator
>>   home directory: /home/admin
>>   login shell: /bin/bash
>>   uid number: 999
>>   gid number: 1001
>>   gecos: Administrator
>>   kerberos principal: admin at PZUNA
>>   last password change: 20090904122852Z
>>   password expiration: 20091203122852Z
>>   member of groups: admins
>>
>> # ipa user-show admin --all --raw
>> ----------
>> user-show:
>> ----------
>>   dn: uid=admin,cn=users,cn=accounts,dc=pzuna
>>   uid: admin
>>   cn: Administrator
>>   sn: Administrator
>>   homedirectory: /home/admin
>>   loginshell: /bin/bash
>>   uidnumber: 999
>>   gidnumber: 1001
>>   gecos: Administrator
>>   krbprincipalname: admin at PZUNA
>>   krblastpwdchange: 20090904122852Z
>>   krbpasswordexpiration: 20091203122852Z
>>   memberof: cn=admins,cn=groups,cn=accounts,dc=pzuna
>>   objectclass: top
>>   objectclass: person
>>   objectclass: posixaccount
>>   objectclass: krbprincipalaux
>>   objectclass: inetuser
>>
>> Advantages: more user friendly, allows for easy localization, 
>> translation of DNs to primary keys (immediately usable as input to 
>> other plugin commands)
>>
>> I recommend, that you use the --raw flag for testing.
>>
>>
>>
>> I know it's a lot of changes, so I setup a git repo at:
>> git://fedorapeople.org/~pzuna/freeipa.git
>>
>> It should be up-to-date with master and all my patches are applied 
>> there. I hope it makes reviewing them at least a bit easier.
>>
>> Pavel
> 
> Why are you using a pre_callback() to define default values instead of 
> using default_from? It seems clearer to use that.
You're probably referring to the user plugin, where gecos and krbprincipalname 
defaults are set inside pre_callback. It's a residue from some time ago when we 
didn't use autofill=True with default_from and it didn't have any effect on 
optional parameters. It's a small change, but I included an updated version of 
the patch with this email.

> This also duplicates some values in the attribute_names() dictionary. I 
> wonder if this can be either created from the parameters or define a 
> global set somewhere that covers all plugins.
I know, but I couldn't find a better solution. I thought we could add something 
like a 'real_name' kwarg to params, but this has 2 main disadvantages:
1) it only makes sense with parameters that refer to an LDAP attribute
2) it doesn't work for attributes with no param counterparts

The global set is a good idea as long as we consider only our own plugins. 3rd 
party plugins would have to modify this set to add their own attributes. Also, 
attributes might have a different meaning for different objects. They usually 
don't, but take 'cn' for example. We use it as an ID, name, full name (for 
users), etc.

> In the user plugin 'ou' is in the default attributes. We don't use this 
> attribute.
Since we don't use it, it's not going to get displayed anyway.

All the values in attribute_names, default_attribute and attribute_order are 
subject to change. I set them to initial values I thought are acceptable, but I 
don't think I'm the right person to decide what's going to be there. Ideally, 
someone with more UI/text/user friendliness experience should review it (there's 
not programming knowledge required to change the values).

> I think that in general baseldap needs to be documented to explain how 
> things work. There is no explanation for object_class_config, for 
> example, that it defines the attribute in cn=ipaConfig that contains the 
> default list of objectclasses for the object.
Yeah, there's no documentation at this point, but I'm working on it as we "speak".

> rob
> 

One more thing, I reviewed your latest patches and they make change to host and 
service plugins. Your patches should be pushed first, because they are more 
complex and also more important, so I posted updated versions to the 
corresponding email threads on freeipa-devel.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Make-the-user-plugin-use-baseldap-classes.patch
Type: application/mbox
Size: 13829 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090915/7fd9e983/attachment.mbox>