permission plugin: Allow multiple values for memberof

Design: http://www.freeipa.org/page/V3/Multivalued_target_filters_in_permissions
Additional fix for: https://fedorahosted.org/freeipa/ticket/4074

Reviewed-By: Martin Kosek <mkosek@redhat.com>

4 files changed, 55 insertions, 11 deletions

diff --git a/API.txt b/API.txt
index 971edd915..5d1063386 100644
--- a/API.txt
+++ b/API.txt
@@ -2335,7 +2335,7 @@ option: DNOrURL('ipapermlocation', alwaysask=True, attribute=True, autofill=Fals
 option: StrEnum('ipapermright', attribute=True, cli_name='permissions', multivalue=True, required=False, values=(u'read', u'search', u'compare', u'write', u'add', u'delete', u'all'))
 option: DNParam('ipapermtarget', attribute=True, cli_name='target', multivalue=False, required=False)
 option: Str('ipapermtargetfilter', attribute=True, cli_name='filter', multivalue=True, required=False)
-option: Str('memberof', alwaysask=True, attribute=False, autofill=False, cli_name='memberof', multivalue=False, query=False, required=False)
+option: Str('memberof', alwaysask=True, attribute=False, autofill=False, cli_name='memberof', multivalue=True, query=False, required=False)
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
 option: Str('permissions', attribute=False, cli_name='permissions', multivalue=True, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -2393,7 +2393,7 @@ option: DNOrURL('ipapermlocation', attribute=True, autofill=False, cli_name='sub
 option: StrEnum('ipapermright', attribute=True, autofill=False, cli_name='permissions', multivalue=True, query=True, required=False, values=(u'read', u'search', u'compare', u'write', u'add', u'delete', u'all'))
 option: DNParam('ipapermtarget', attribute=True, autofill=False, cli_name='target', multivalue=False, query=True, required=False)
 option: Str('ipapermtargetfilter', attribute=True, autofill=False, cli_name='filter', multivalue=True, query=True, required=False)
-option: Str('memberof', attribute=False, autofill=False, cli_name='memberof', multivalue=False, query=True, required=False)
+option: Str('memberof', attribute=False, autofill=False, cli_name='memberof', multivalue=True, query=True, required=False)
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
 option: Str('permissions', attribute=False, autofill=False, cli_name='permissions', multivalue=True, query=True, required=False)
 option: Flag('pkey_only?', autofill=True, default=False)
@@ -2423,7 +2423,7 @@ option: DNOrURL('ipapermlocation', attribute=True, autofill=False, cli_name='sub
 option: StrEnum('ipapermright', attribute=True, autofill=False, cli_name='permissions', multivalue=True, required=False, values=(u'read', u'search', u'compare', u'write', u'add', u'delete', u'all'))
 option: DNParam('ipapermtarget', attribute=True, autofill=False, cli_name='target', multivalue=False, required=False)
 option: Str('ipapermtargetfilter', attribute=True, autofill=False, cli_name='filter', multivalue=True, required=False)
-option: Str('memberof', attribute=False, autofill=False, cli_name='memberof', multivalue=False, required=False)
+option: Str('memberof', attribute=False, autofill=False, cli_name='memberof', multivalue=True, required=False)
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
 option: Str('permissions', attribute=False, autofill=False, cli_name='permissions', multivalue=True, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
diff --git a/VERSION b/VERSION
index 64472ada1..e889bced8 100644
--- a/VERSION
+++ b/VERSION
@@ -89,5 +89,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=76
-# Last change: npmccallum - otptoken defaults change
+IPA_API_VERSION_MINOR=77
+# Last change: pviktori - permissions: multivalued memberof
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 79335404a..82272d361 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -243,7 +243,7 @@ class permission(baseldap.LDAPObject):
             flags={'no_option'}
         ),
 
-        Str('memberof?',
+        Str('memberof*',
             label=_('Member of group'),  # FIXME: Does this label make sense?
             doc=_('Target members of a group (sets memberOf targetfilter)'),
             flags={'ask_create', 'virtual_attribute'},
@@ -388,9 +388,13 @@ class permission(baseldap.LDAPObject):
 
         if not client_has_capability(options['version'], 'permissions2'):
             # Legacy clients expect some attributes as a single value
-            for attr in 'type', 'targetgroup', 'memberof', 'aci':
+            for attr in 'type', 'targetgroup', 'aci':
                 if attr in entry:
                     entry[attr] = entry.single_value[attr]
+            # memberof was also single-valued, but not any more
+            if entry.get('memberof'):
+                joined_value = u', '.join(str(m) for m in entry['memberof'])
+                entry['memberof'] = joined_value
             if 'subtree' in entry:
                 # Legacy clients expect subtree as a URL
                 dn = entry.single_value['subtree']
@@ -656,14 +660,14 @@ class permission(baseldap.LDAPObject):
 
         # memberof
         if 'memberof' in options:
-            memberof = options.pop('memberof')
             filter_ops['remove'].append(re.compile(r'\(memberOf=.*\)', re.I))
-            if memberof:
+            memberof = options.pop('memberof')
+            for group in (memberof or ()):
                 try:
-                    groupdn = self.api.Object.group.get_dn_if_exists(memberof)
+                    groupdn = self.api.Object.group.get_dn_if_exists(group)
                 except errors.NotFound:
                     raise errors.NotFound(
-                        reason=_('%s: group not found') % memberof)
+                        reason=_('%s: group not found') % group)
                 filter_ops['add'].append(u'(memberOf=%s)' % groupdn)
 
         # targetgroup
diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py
index 29effb9a4..e9e2fea0e 100644
--- a/ipatests/test_xmlrpc/test_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_permission_plugin.py
@@ -3255,4 +3255,44 @@ class test_permission_filters(Declarative):
             '(version 3.0;acl "permission:%s";' % permission1 +
             'allow (write) groupdn = "ldap:///%s";)' % permission1_dn,
         ),
+
+        dict(
+            desc='Add multiple memberof to %r' % permission1,
+            command=(
+                'permission_mod', [permission1],
+                dict(
+                    memberof=[u'admins', u'editors'],
+                ),
+            ),
+            expected=dict(
+                value=permission1,
+                summary=u'Modified permission "%s"' % permission1,
+                result=dict(
+                    dn=permission1_dn,
+                    cn=[permission1],
+                    objectclass=objectclasses.permission,
+                    ipapermright=[u'write'],
+                    memberof=[u'admins', u'editors'],
+                    ipapermbindruletype=[u'permission'],
+                    ipapermissiontype=[u'SYSTEM', u'V2'],
+                    ipapermlocation=[api.env.basedn],
+                    ipapermtargetfilter=[
+                        u'(uid=abc)',
+                        u'(memberOf=%s)' % DN(('cn', 'admins'), groups_dn),
+                        u'(memberOf=%s)' % DN(('cn', 'editors'), groups_dn),
+                    ],
+                ),
+            ),
+        ),
+
+        verify_permission_aci(
+            permission1, api.env.basedn,
+            '(targetfilter = "(&'
+                '(memberOf=%s)' % DN(('cn', 'admins'), groups_dn) +
+                '(memberOf=%s)' % DN(('cn', 'editors'), groups_dn) +
+                '(uid=abc)' +
+            ')")' +
+            '(version 3.0;acl "permission:%s";' % permission1 +
+            'allow (write) groupdn = "ldap:///%s";)' % permission1_dn,
+        ),
     ]