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

[Martin Kosek]

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