[Freeipa-devel] [PATCH] 11 Checking and modifying of memberof attribute

[Martin Kosek]

On Tue, 2012-02-07 at 17:04 +0100, Ondrej Hamada wrote:
> On 02/06/2012 05:03 PM, Martin Kosek wrote:
> > On Mon, 2012-02-06 at 12:14 +0100, Ondrej Hamada wrote:
> >> https://fedorahosted.org/freeipa/ticket/2255
> >> https://fedorahosted.org/freeipa/ticket/2286
> >> https://fedorahosted.org/freeipa/ticket/2305
> >>
> >> Added checking of existence of groups that are specified in permission
> >> and delegation module. Also the permission plugin now allows to unset
> >> memberof value. Additional unit tests for checking new behaviour were
> >> created.
> > NACK
> >
> > I think there are few things that could be improved:
> >
> > 1) I don't think that _make_aci function should have any side-effects to
> > kw like deleting some keys from it:
> >
> > @@ -265,8 +265,15 @@ def _make_aci(ldap, current, aciname, kw):
> > ...
> > +            else:
> > +                del kw['memberof']
> >
> > IMO, this may break expectations when _make_aci is called and introduce
> > some issues in the future.
> >
> > I think that entire _make_aci should be fixed to ignore attributes set
> > to None just like with other plugins. We just need to validate if the kw
> > combination is OK.
> >
> > This would mean that the ACI validation should be updated as well:
> > ...
> >      t1 = 'type' in kw<<<<  What if kw['type'] is None?
> >      t2 = 'filter' in kw
> >      t3 = 'subtree' in kw
> >      t4 = 'targetgroup' in kw
> >      t5 = 'attrs' in kw
> >      t6 = 'memberof' in kw
> > ...
> >
> > There are already some related fixes in aci_find.
> >
> > 2) This is a good opportunity to fix also other ACI attributes, like
> > --type. Now, it throws Internal Error:
> >
> > # ipa permission-mod test --type=
> > ipa: ERROR: an internal error has occurred
> >
> > Martin
> >
> The ACI validation was updated to validate all the six mentioned 
> attributes and it was enabled to unset them.
> 

Thanks. That's exactly what I had in mind, works great.

ACK. Pushed to master, ipa-2-2.

Martin