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

[Jason Gerard DeRose]

On Fri, 2009-05-15 at 09:40 -0400, Rob Crittenden wrote:
> 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.
> 
> The biggest downside is that you'd get the members which really could be 
> quite large. I think a better solution is to be able to pass into 
> group-show (and really any/all of the plugins) the attributes you want 
> to see, with a reasonable default and the --all options available too.
> 
> The problem with getting the entry within another plugin is you may 
> require knowledge of the schema to do so. If the schema changes, like it 
> did with host recently, you'd have to know to go and change it in other 
> places. When I changed the DN of the host entries the only plugin that 
> changed was host.
> 
> rob

Hmmm, yeah, good point.  I think I am retracting my earlier +1.

Oh, I've been thinking about how to specify what attributes are pulled
by default when an entry is retrieved: I think we should add flag to the
Param to indicate whether it should be included.  This keeps things
easily plugable.

Although we haven't used them yet, the Attribute plugins are designed to
add another Param (in the case of LDAP stuff, an LDAP attribute) to a
corresponding Object plugin.  So you could add a new Param to
Object.user, which would automatically get picked up by all the user
crud methods.  If we have the list of attributes to retrieve hard-coded
in Command.user_show(), an Attribute plugin can't be included in the
list of attributes to pull without overriding the Command.user_show
plugin.

What do you two think?

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