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.

--
Regards,

Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada

From 4313a381e44986cdc26c88194297d6a0a4cfd112 Mon Sep 17 00:00:00 2001
From: Ondrej Hamada <oham...@redhat.com>
Date: Tue, 7 Feb 2012 13:07:09 +0100
Subject: [PATCH] Memberof attribute control and update

Checking of parameters used by _make_aci funcion was rewritten.
Additional attributes of ACI(type, attribute, memberof, targetgroup,
subtree, filter) could be unset.

Permission plugin now allows to unset memberof value.
https://fedorahosted.org/freeipa/ticket/2255

Added checking of existence of groups that are specified in permission
and delegation module.

https://fedorahosted.org/freeipa/ticket/2286
https://fedorahosted.org/freeipa/ticket/2305
---
 ipalib/plugins/aci.py                       |   36 ++++++++++-------
 tests/test_xmlrpc/test_delegation_plugin.py |   12 ++++++
 tests/test_xmlrpc/test_permission_plugin.py |   57 +++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 15 deletions(-)

diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index e87ac9bff09fc87fec6987ae40b0cf1dd353dd3b..f0b81f48af1f9fbf8ab267a3d4b113c328ab1170 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -208,22 +208,24 @@ def _make_aci(ldap, current, aciname, kw):
     Given a name and a set of keywords construct an ACI.
     """
     # Do some quick and dirty validation.
-    t1 = 'type' in kw
-    t2 = 'filter' in kw
-    t3 = 'subtree' in kw
-    t4 = 'targetgroup' in kw
-    t5 = 'attrs' in kw
-    t6 = 'memberof' in kw
-    if t1 + t2 + t3 + t4 > 1:
+    checked_args=['type','filter','subtree','targetgroup','attrs','memberof']
+    valid={}
+    for arg in checked_args:
+        if arg in kw:
+            valid[arg]=kw[arg] is not None
+        else:
+            valid[arg]=False
+
+    if valid['type'] + valid['filter'] + valid['subtree'] + valid['targetgroup'] > 1:
         raise errors.ValidationError(name='target', error=_('type, filter, subtree and targetgroup are mutually exclusive'))
 
     if 'aciprefix' not in kw:
         raise errors.ValidationError(name='aciprefix', error=_('ACI prefix is required'))
 
-    if t1 + t2 + t3 + t4 + t5 + t6 == 0:
+    if sum(valid.itervalues()) == 0:
         raise errors.ValidationError(name='target', error=_('at least one of: type, filter, subtree, targetgroup, attrs or memberof are required'))
 
-    if t2 + t6 > 1:
+    if valid['filter'] + valid['memberof'] > 1:
         raise errors.ValidationError(name='target', error=_('filter and memberof are mutually exclusive'))
 
     group = 'group' in kw
@@ -262,12 +264,16 @@ def _make_aci(ldap, current, aciname, kw):
         else:
             dn = entry_attrs['dn']
             a.set_bindrule('groupdn = "ldap:///%s";' % dn)
-        if 'attrs' in kw:
+        if valid['attrs']:
             a.set_target_attr(kw['attrs'])
-        if 'memberof' in kw:
+        if valid['memberof']:
+            try:
+                api.Command['group_show'](kw['memberof'])
+            except errors.NotFound:
+                api.Object['group'].handle_not_found(kw['memberof'])
             groupdn = _group_from_memberof(kw['memberof'])
             a.set_target_filter('memberOf=%s' % groupdn)
-        if 'filter' in kw:
+        if valid['filter']:
             # Test the filter by performing a simple search on it. The
             # filter is considered valid if either it returns some entries
             # or it returns no entries, otherwise we let whatever exception
@@ -279,15 +285,15 @@ def _make_aci(ldap, current, aciname, kw):
             except errors.NotFound:
                 pass
             a.set_target_filter(kw['filter'])
-        if 'type' in kw:
+        if valid['type']:
             target = _type_map[kw['type']]
             a.set_target(target)
-        if 'targetgroup' in kw:
+        if valid['targetgroup']:
             # Purposely no try here so we'll raise a NotFound
             entry_attrs = api.Command['group_show'](kw['targetgroup'])['result']
             target = 'ldap:///%s' % entry_attrs['dn']
             a.set_target(target)
-        if 'subtree' in kw:
+        if valid['subtree']:
             # See if the subtree is a full URI
             target = kw['subtree']
             if not target.startswith('ldap:///'):
diff --git a/tests/test_xmlrpc/test_delegation_plugin.py b/tests/test_xmlrpc/test_delegation_plugin.py
index 1a9c36743d305cc382350db8e866ace21331fc5c..db5f7186527d2e0c6567dd5a727e878144bd3020 100644
--- a/tests/test_xmlrpc/test_delegation_plugin.py
+++ b/tests/test_xmlrpc/test_delegation_plugin.py
@@ -68,6 +68,18 @@ class test_delegation(Declarative):
             ),
         ),
 
+        dict(
+            desc='Try to create %r for non-existing member group' % delegation1,
+            command=(
+                'delegation_add', [delegation1], dict(
+                     attrs=u'street,c,l,st,postalCode',
+                     permissions=u'write',
+                     group=u'editors',
+                     memberof=u'nonexisting',
+                ),
+            ),
+            expected=errors.NotFound(reason='group not found'),
+        ),
 
         # Note that we add postalCode but expect postalcode. This tests
         # the attrs normalizer.
diff --git a/tests/test_xmlrpc/test_permission_plugin.py b/tests/test_xmlrpc/test_permission_plugin.py
index 50d368197cbc080f40fecf2038ae14337ed78b7c..e8e6bebcd387307f30e4a7bc4d266092b7e41424 100644
--- a/tests/test_xmlrpc/test_permission_plugin.py
+++ b/tests/test_xmlrpc/test_permission_plugin.py
@@ -500,6 +500,16 @@ class test_permission(Declarative):
             )
         ),
 
+        dict(
+            desc='Try to create permission %r with non-existing memberof' % permission1,
+            command=(
+                'permission_add', [permission1], dict(
+                     memberof=u'nonexisting',
+                     permissions=u'write',
+                )
+            ),
+            expected=errors.NotFound(reason='group not found'),
+        ),
 
         dict(
             desc='Create memberof permission %r' % permission1,
@@ -507,6 +517,7 @@ class test_permission(Declarative):
                 'permission_add', [permission1], dict(
                      memberof=u'editors',
                      permissions=u'write',
+                     type=u'user',
                 )
             ),
             expected=dict(
@@ -518,6 +529,52 @@ class test_permission(Declarative):
                     objectclass=objectclasses.permission,
                     memberof=u'editors',
                     permissions=[u'write'],
+                    type=u'user',
+                ),
+            ),
+        ),
+
+        dict(
+            desc='Try to update non-existent memberof of %r' % permission1,
+            command=('permission_mod', [permission1], dict(memberof=u'nonexisting')),
+            expected=errors.NotFound(reason='group not found'),
+        ),
+
+        dict(
+            desc='Update memberof permission %r' % permission1,
+            command=(
+                'permission_mod', [permission1], dict(
+                     memberof=u'admins',
+                )
+            ),
+            expected=dict(
+                value=permission1,
+                summary=u'Modified permission "%s"' % permission1,
+                result=dict(
+                    dn=lambda x: DN(x) == permission1_dn,
+                    cn=[permission1],
+                    memberof=u'admins',
+                    permissions=[u'write'],
+                    type=u'user',
+                ),
+            ),
+        ),
+
+        dict(
+            desc='Unset memberof of permission %r' % permission1,
+            command=(
+                'permission_mod', [permission1], dict(
+                     memberof=None,
+                )
+            ),
+            expected=dict(
+                summary=u'Modified permission "%s"' % permission1,
+                value=permission1,
+                result=dict(
+                    dn=lambda x: DN(x) == permission1_dn,
+                    cn=[permission1],
+                    permissions=[u'write'],
+                    type=u'user',
                 ),
             ),
         ),
-- 
1.7.6.5

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to