[Freeipa-devel] [PATCH] Add group plugin port to new LDAP backend.

[Pavel Zuna]

Jason Gerard DeRose wrote:
> On Wed, 2009-05-13 at 16:04 -0400, Rob Crittenden wrote:
>> Pavel Zuna wrote:
>>> Rob Crittenden wrote:
>>>> Pavel Zuna wrote:
>>>>> Rob Crittenden wrote:
>>>>>> Pavel Zuna wrote:
>>>>>>> Rob Crittenden wrote:
>>>>>>>> Pavel Zuna wrote:
>>>>>>>>> By the way, there's a little bug I discovered while testing this 
>>>>>>>>> plugin. It affects the old group plugin as well. When trying to 
>>>>>>>>> modify a group into a posixGroup, gidNumber doesn't get generated 
>>>>>>>>> automatically resulting in a object violation LDAP error. 
>>>>>>>>> Solution is to generate it ourselves, but I didn't know how it 
>>>>>>>>> works, so I commented that part out for now. (/FIXME in vim)
>>>>>>>>>
>>>>>>>> This should be fixed in FDS 1.2. Can you update and give it a try?
>>>>>>>>
>>>>>>>> rob
>>>>>>> Sure, just updated and you're right, it works.  :)
>>>>>>> Updated patch attached.
>>>>>>>
>>>>>>> Pavel
>>>>>> nack. This won't handle someone using group-mod to set a specific 
>>>>>> gidnumber. The posixGroup objectclass won't be added.
>>>>>>
>>>>>> rob
>>>>> Fixed patch attached.
>>>>>
>>>>> Pavel
>>>> The basegroup2 part looks ok but nack on group2.
>>>>
>>>> I think we should stick with using lower-case attribute names as a 
>>>> rule of thumb rather than camel case. In any case you test for the 
>>>> string posixGroup is in the list of objectclasses, this test needs to 
>>>> be case insensitive.
>>> When no attributes to retrieve are specified, python-ldap retrieves them 
>>> all in the original form - camel case. If we specify them, then it 
>>> returns them in the same form as we requested them. The new LDAP backend 
>>> doesn't use CIDicts anymore, but only the normal python dict type, so 
>>> everything is case sensitive. Of course I can make it return attribute 
>>> names always as lowercase if that's what we want.
>> I think we need consistent naming otherwise all sorts of odd bugs can 
>> creep in.
>>
>>>> I also wonder if we should be using ldap.get_entry(). Why use this 
>>>> over group-show?
>>> It's faster, because we call get_entry directly and because we can 
>>> request objectClass attribute only. Why invoke an IPA command instead of 
>>> a making a direct call?
>> Well, I felt the same way but Jason convinced me that by limiting the 
>> places we do actual LDAP calls will be beneficial in the long-run. The 
>> command is run internally, not over XML-RPC, so there isn't a whole lot 
>> of additional overhead.
>>
>> Part of the idea, which we haven't really utilized much yet, is to try 
>> to make the backend easily replacable.
> 
> Well, first question, is this Backend.ldap.get_entry(), or a direct call
> to the python-ldap bindings?  I feel very strongly that no plugins
> should talk directly to python-ldap (except Backend.ldap).
> 
> But if this is whether to retrieve the group entry via
> Command.group_show() or Backend.ldap.get_entry(), I think it depends on
> what attributes are needed.  If we want the same attributes as
> Command.group_show() returns, we should call it so we aren't defining
> the list of (or logic behind) the attributes to choose in multiple
> places.  If we need all the attributes, calling Backend.ldap.get_entry()
> is probably best.
In this context, we need only the 'objectClass' attribute that 
group_show doesn't return normally unless we pass it the '--all' option 
and then we get a lot of attributes we don't need as side effect.

Generally speaking, I think the decision of what call is made should go 
like this:
We need to get attribute A,B,C,... (even if they happen to be the same 
as group_show returns)? Call get_entry.
We need to get the same attributes group_show returns (whatever those 
may be)? Call group_show.

>>>> I'm not sure if the logic around setting gidnumber is right. If you 
>>>> set the gidnumber but aren't using the --posix flag it looks like it 
>>>> will always append posixgroup to the list of objectclasses. I'm pretty 
>>>> sure the LDAP server is going to reject the update. I suppose making a 
>>>> list(set(objectclasses)) would work for de-duping.
>>> You're right, it's broken. I'll fix it.
>>>
>>> Pavel
>> ok
>>
>> rob

Pavel