[Freeipa-devel] Re: [PATCHES] Make plugins use baseldap classes.
Rob Crittenden
rcritten at redhat.com
Mon Oct 5 20:01:26 UTC 2009
Pavel Zuna wrote:
> Pavel Zuna wrote:
>> Pavel Zůna wrote:
>>> Rob Crittenden wrote:
>>>> Rob Crittenden wrote:
>>>>> Pavel Zuna wrote:
>>>>>> 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.
>>>>>
>>>>> Ok. I gather you've moved a lot of logic into the pre_callback
>>>>> plugin to avoid overriding execute? One other goal we wanted was to
>>>>> allow plugins to extend other plugins and this may be good step on
>>>>> the way there. So for example, a user wants to be able to set some
>>>>> extra objectclass, they could do it with a similar pre_callback
>>>>> extension to the user plugin (once we do the plumbing for it, that
>>>>> is).
>>> Right. The goal is to have a common execute in the baseclass, that
>>> does most of the dirty work and let the user/plugin author add the
>>> specifics of his plugin in the pre/post callbacks. All the data
>>> generated by the base (before calling python-ldap) is available for
>>> modification in the pre-callbacks and all data coming out of
>>> python-ldap is made available in the post-callback.
>>>
>>> And yes, the plugins could be almost endlessly extended this way. For
>>> example, someone could do this:
>>>
>>> from ipalib.plugins.user import user_add
>>>
>>> class user_add_extended(user_add):
>>> def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
>>> # let the original user_add plugin do its job
>>> super(user_add_extended, self).pre_callback(
>>> ldap, dn, entry_attrs, *keys, **options)
>>> # add an extra object class
>>> entry_attrs['objectclass'].append('new_object_class')
>>> return dn
>>>
>>> api.register(user_add_extended)
>>>
>>>>>>> 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.
>>>>>
>>>>> Ok, that's a good point. I'm wondering if its overkill to write a
>>>>> registration system for this, something like:
>>>>>
>>>>> def set_label(attr, label, context='')
>>>>>
>>>>> def get_label(attr, context='')
>>>>>
>>>>> In this we'd store a dict that would be keyed on attr+context. Some
>>>>> examples might be:
>>>>>
>>>>> set_label('uid', 'Login')
>>>>> set_label('gn', 'First Name')
>>>>> set_label('cn', 'Full Name', context='user')
>>>>> set_label('cn', 'Group Name', context='group')
>>>>>
>>>>> The lookup would first look for a context-specific entry and fall
>>>>> back to a non-context specific, something like:
>>>>>
>>>>> if attr+context in labeldict: return labeldict[attr+context]
>>>>> elif attr in labeldict: return labeldict[attr]
>>>>> else return "woop"
>>>>>
>>>>> Like I said, might be overkill ;-)
>>>>>
>>>>> But still, the alarms go off when we're putting the same things in
>>>>> multiple places.
>>>>>
>>> Yeah, the registration system sounds like overkill all right. :D
>>>
>>> Seriously, I was thinking about several ways on how to resolve this
>>> issue. Ideally, all information about attributes should be retrieved
>>> from the schema. Unfortunately, it cannot be done. There's just very
>>> little information in there.
>>>
>>> Another idea was to have an LDAPAttribute base class, that we would
>>> extend for each attribute. We could then have a ldapattribute.py
>>> module in ipalib/plugins with all the attributes we use defined
>>> there. If 3rd parties require a new attribute, they just create their
>>> own LDAPAttribute subclass in their plugins. I think that this would
>>> be the best and cleanest approach. LDAPObjects would have a set of
>>> LDAPAttribute references and do all attribute manipulation using
>>> them. Also, attribute parameters could be automatically generated, so
>>> they wouldn't mix with control parameters in takes_params (like
>>> --all, --raw, --posix for groups, etc.). This is probably the way
>>> we'll be going in the future, but there's still a few problems I have
>>> to resolve before implementing it.
>>>
>>> By the way, my goal is to move all this LDAP infrastructure from the
>>> plugin layer into the core library itself once it's mature (and
>>> documented) enough. Probably not going to happen in v2, but we'll see
>>> how it goes.
>>>
>>>>>>> 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).
>>>>>
>>>>> Ok, this particular attribute brings up DIT issues which is why I
>>>>> flagged it.
>>>>>
>>>>>>> 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
>>>>>
>>>>> Ok, I'm not too worried about that, its always a game of catch-up :-)
>>>>>
>>>>
>>>> I've found another problem with the attribute map. I like your idea
>>>> of breaking out memberof by different type but as it is now doesn't
>>>> work, it only prints group membership. You'd have to parse the DN to
>>>> distinguish between groups, taskgroups, etc.
>>>
>>> And that's exactly what I'm doing in
>>> LDAPObject.convert_attribute_members. I mean the DN parsing. It just
>>> doesn't work for users at the moment, because I forgot to add the
>>> other types of groups in users attribute_members['memberof'].
>>>
>>> in ipalib/plugins/user.py:
>>>
>>> class user(LDAPObject):
>>> # ...
>>> attribute_members = {
>>> # add other groups here
>>> # example 'memberof': ['group', 'taskgroup']
>>> 'memberof': ['group']
>>> }
>>> # ...
>>>
>>> try:
>>>
>>> ipa user-add test --first=first --last=last
>>> ipa taskgroup-add tasktest --desc=desc
>>> ipa taskgroup-add-member tasktest --users=test
>>> ipa user-show test --all
>>>
>>> Should work.
>>>
>>> Note: Just noticed it won't work for netgroups, because I forgot to
>>> override LDAPObject.get_primary_key_from_dn in the netgroup class.
>>> Normally get_primary_key_from_dn just returns the value of RDN, but
>>> for netgroups we have to do a search to retrieve the primary key.
>>>
>>> class netgroup(LDAPObject):
>>> # ...
>>> def get_primary_key_from_dn(self, dn):
>>> (dn, entry_attrs) = self.backend.get_entry(
>>> dn, [self.primary_key.name]
>>> )
>>> return entry_attrs.get(self.primary_key.name, '')
>>> # ...
>>>
>>>> I think for the short-term we can make a note to do this and just
>>>> print all memberships so we can get this committed. I'm still not a
>>>> fan of the attribute_names within each plugin though, I need more
>>>> convincing.
>>>>
>>>> rob
>>>>
>>>
>>> By the way, I'm going on vacation next week. So, I think we should
>>> wait anyway before committing this.
>>>
>>> Pavel
>>>
>> I'm sending an updated version of all the patches. They should apply
>> on the current master. I think they should now address most of your
>> concerns and I also fixed some bugs I found on my own.
>>
>> Pavel
> I found a bug in the netgroup plugin, here's an update.
>
> Also, the pwpolicy plugin patch needs update to included your latest
> changes. Should be done by monday.
>
> Pavel
ack, pushed to master.
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/20091005/78a4269d/attachment.bin>
More information about the Freeipa-devel
mailing list