frontend: don't copy command arguments to output params

Use only object params and params defined in has_output_params as output
params. This removes unnecessary duplication of params defined both in
object plugins and as command arguments.

This requires all command output params to be properly defined in either
the object plugins or the command's has_output_params. Fix the plugins
where this wasn't true.

https://fedorahosted.org/freeipa/ticket/4739

Reviewed-By: David Kupka <dkupka@redhat.com>

9 files changed, 42 insertions, 34 deletions

diff --git a/ipaclient/plugins/otptoken_yubikey.py b/ipaclient/plugins/otptoken_yubikey.py
index c1ca88e08..e9aaba934 100644
--- a/ipaclient/plugins/otptoken_yubikey.py
+++ b/ipaclient/plugins/otptoken_yubikey.py
@@ -61,6 +61,7 @@ class otptoken_add_yubikey(Command):
             values=(1, 2),
         ),
     )
+    has_output_params = takes_options
 
     def get_args(self):
         for arg in self.api.Command.otptoken_add.args():
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 49ff8a174..672f5576b 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -991,12 +991,6 @@ class Command(HasParam):
     def get_output_params(self):
         for param in self._get_param_iterable('output_params', verb='has'):
             yield param
-        if self.params is None:
-            return
-        for param in self.params():
-            if 'no_output' in param.flags:
-                continue
-            yield param
 
     def get_summary_default(self, output):
         if self.msg_summary:
@@ -1421,12 +1415,7 @@ class Method(Attribute, Command):
             if 'no_output' in param.flags:
                 continue
             yield param
-        for param in self.params():
-            if param.name not in list(self.obj.params):
-                if 'no_output' in param.flags:
-                    continue
-                yield param
-        for param in self._get_param_iterable('output_params', verb='has'):
+        for param in super(Method, self).get_output_params():
             yield param
 
 
diff --git a/ipaserver/plugins/aci.py b/ipaserver/plugins/aci.py
index 01c929230..dd14d82b6 100644
--- a/ipaserver/plugins/aci.py
+++ b/ipaserver/plugins/aci.py
@@ -423,6 +423,7 @@ _prefix_option = StrEnum('aciprefix',
                 doc=_('Prefix used to distinguish ACI types ' \
                     '(permission, delegation, selfservice, none)'),
                 values=_valid_prefix_values,
+                flags={'no_create', 'no_update', 'no_search'},
                 )
 
 
@@ -505,6 +506,7 @@ class aci(Object):
              doc=_('Apply ACI to your own entry (self)'),
              flags=('virtual_attribute',),
         ),
+        _prefix_option,
     )
 
 
diff --git a/ipaserver/plugins/automember.py b/ipaserver/plugins/automember.py
index 89b9dfadc..0a7f35191 100644
--- a/ipaserver/plugins/automember.py
+++ b/ipaserver/plugins/automember.py
@@ -155,13 +155,18 @@ regex_attrs = (
         label=_('Inclusive Regex'),
         doc=_('Inclusive Regex'),
         alwaysask=True,
+        flags={'no_create', 'no_update', 'no_search'},
     ),
     Str('automemberexclusiveregex*',
         cli_name='exclusive_regex',
         label=_('Exclusive Regex'),
         doc=_('Exclusive Regex'),
         alwaysask=True,
+        flags={'no_create', 'no_update', 'no_search'},
     ),
+)
+
+regex_key = (
     Str('key',
         label=_('Attribute Key'),
         doc=_('Attribute to filter via regex. For example fqdn for a host, or manager for a user'),
@@ -183,6 +188,7 @@ automember_rule = (
         label=_('Automember Rule'),
         doc=_('Automember Rule'),
         normalizer=lambda value: value.lower(),
+        flags={'no_create', 'no_update', 'no_search'},
     ),
 )
 
@@ -254,7 +260,7 @@ class automember(LDAPObject):
             doc=_('Default group for entries to land'),
             flags=['no_create', 'no_update', 'no_search']
         ),
-    )
+    ) + automember_rule + regex_attrs
 
     def dn_exists(self, otype, oname):
         ldap = self.api.Backend.ldap2
@@ -336,7 +342,7 @@ class automember_add_condition(LDAPUpdate):
         ),
     )
 
-    takes_options = regex_attrs + group_type
+    takes_options = regex_attrs + regex_key + group_type
     takes_args = automember_rule
     msg_summary = _('Added condition(s) to "%(value)s"')
 
@@ -421,7 +427,7 @@ class automember_remove_condition(LDAPUpdate):
     __doc__ = _("""
     Remove conditions from an automember rule.
     """)
-    takes_options = regex_attrs + group_type
+    takes_options = regex_attrs + regex_key + group_type
     takes_args = automember_rule
     msg_summary = _('Removed condition(s) from "%(value)s"')
 
@@ -539,7 +545,6 @@ class automember_find(LDAPSearch):
     Search for automember rules.
     """)
     takes_options = group_type
-    has_output_params = LDAPSearch.has_output_params + automember_rule + regex_attrs
 
     msg_summary = ngettext(
         '%(count)d rules matched', '%(count)d rules matched', 0
@@ -559,7 +564,6 @@ class automember_show(LDAPRetrieve):
     """)
     takes_args = automember_rule
     takes_options = group_type
-    has_output_params = LDAPRetrieve.has_output_params + regex_attrs
 
     def execute(self, *keys, **options):
         result = super(automember_show, self).execute(*keys, **options)
diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
index 9926c1f5e..76a2fbc3e 100644
--- a/ipaserver/plugins/cert.py
+++ b/ipaserver/plugins/cert.py
@@ -553,6 +553,7 @@ class cert_status(VirtualCommand):
         Str('cert_request_status',
             label=_('Request status'),
         ),
+        takes_args[0],
     )
     operation = "certificate status"
 
@@ -607,6 +608,7 @@ class cert_show(VirtualCommand):
         Str('serial_number_hex',
             label=_('Serial number (hex)'),
         ),
+        _serial_number,
     )
 
     takes_options = (
@@ -845,6 +847,9 @@ class cert_find(Command):
         Str('status',
             label=_('Status'),
         ),
+        Str('subject',
+            label=_('Subject'),
+        ),
     )
 
     msg_summary = ngettext(
diff --git a/ipaserver/plugins/group.py b/ipaserver/plugins/group.py
index 2b0c08050..dcd4a91d8 100644
--- a/ipaserver/plugins/group.py
+++ b/ipaserver/plugins/group.py
@@ -139,6 +139,14 @@ register = Registry()
 PROTECTED_GROUPS = (u'admins', u'trust admins', u'default smb group')
 
 
+ipaexternalmember_param = Str('ipaexternalmember*',
+            cli_name='external',
+            label=_('External member'),
+            doc=_('Members of a trusted domain in DOM\\name or name@domain form'),
+            flags=['no_create', 'no_update', 'no_search'],
+        )
+
+
 @register()
 class group(LDAPObject):
     """
@@ -271,17 +279,10 @@ class group(LDAPObject):
             doc=_('GID (use this option to set it manually)'),
             minvalue=1,
         ),
+        ipaexternalmember_param,
     )
 
 
-ipaexternalmember_param = Str('ipaexternalmember*',
-            cli_name='external',
-            label=_('External member'),
-            doc=_('Members of a trusted domain in DOM\\name or name@domain form'),
-            flags=['no_create', 'no_update', 'no_search'],
-        )
-
-
 @register()
 class group_add(LDAPCreate):
     __doc__ = _('Create a new group.')
@@ -498,7 +499,7 @@ class group_find(LDAPSearch):
 @register()
 class group_show(LDAPRetrieve):
     __doc__ = _('Display information about a named group.')
-    has_output_params = LDAPRetrieve.has_output_params + (ipaexternalmember_param,)
+
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         assert isinstance(dn, DN)
         if ('ipaexternalmember' in entry_attrs and
diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py
index 9f19358da..971172766 100644
--- a/ipaserver/plugins/permission.py
+++ b/ipaserver/plugins/permission.py
@@ -165,6 +165,13 @@ def _disallow_colon(option):
     )
 
 
+_ipapermissiontype_param = Str(
+    'ipapermissiontype+',
+    label=_('Permission flags'),
+    flags={'no_create', 'no_update', 'no_search'},
+)
+
+
 @register()
 class permission(baseldap.LDAPObject):
     """
@@ -346,6 +353,8 @@ class permission(baseldap.LDAPObject):
             doc=_('Deprecated; use %s' % new_name),
             flags={'no_option', 'virtual_attribute'})
         for old_name, new_name in _DEPRECATED_OPTION_ALIASES.items()
+    ) + (
+        _ipapermissiontype_param,
     )
 
     def reject_system(self, entry):
@@ -945,9 +954,7 @@ class permission_add_noaci(baseldap.LDAPCreate):
     has_output_params = baseldap.LDAPCreate.has_output_params + output_params
 
     takes_options = (
-        Str('ipapermissiontype+',
-            label=_('Permission flags'),
-        ),
+        _ipapermissiontype_param,
     )
 
     def get_options(self):
diff --git a/ipaserver/plugins/schema.py b/ipaserver/plugins/schema.py
index e5aac6ff9..cbdb70d71 100644
--- a/ipaserver/plugins/schema.py
+++ b/ipaserver/plugins/schema.py
@@ -181,8 +181,7 @@ class command(MetaObject):
 
         if len(command.output_params):
             obj['output_params_param'] = tuple(
-                unicode(n) for n in command.output_params
-                if n not in command.params)
+                unicode(n) for n in command.output_params)
 
         return obj
 
diff --git a/ipatests/test_ipalib/test_frontend.py b/ipatests/test_ipalib/test_frontend.py
index 518dadcda..93ab547b0 100644
--- a/ipatests/test_ipalib/test_frontend.py
+++ b/ipatests/test_ipalib/test_frontend.py
@@ -814,16 +814,16 @@ class test_Command(ClassChecker):
                 'foo',
             )
             takes_options = (
-                Str('bar', flags='no_output'),
+                Str('bar'),
                 'baz',
             )
 
         inst = example(api)
         inst.finalize()
         assert list(inst.get_output_params()) == [
-            'one', 'two', 'three', inst.params.foo, inst.params.baz
+            'one', 'two', 'three'
         ]
-        assert list(inst.output_params) == ['one', 'two', 'three', 'foo', 'baz']
+        assert list(inst.output_params) == ['one', 'two', 'three']
 
 
 class test_LocalOrRemote(ClassChecker):