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

[Jason Gerard DeRose]

On Fri, 2009-05-15 at 12:22 +0200, Pavel Zuna wrote:
> 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.

+1  Well said.

> >>>> 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