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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel