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