Allow for commands that use positional parameters to add/remove attributes

Commands that modify a single multivalued attribute of an entry should use
positional parameters to specify both the primary key and the values to
add/remove. Named options are redundant in this case.

The `--certificate option` of `*-add/remove-cert` commands was turned
mandatory to avoid EmptyModlist when it is omitted.

https://fedorahosted.org/freeipa/ticket/3961
https://fedorahosted.org/freeipa/ticket/5413

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

7 files changed, 90 insertions, 41 deletions

diff --git a/API.txt b/API.txt
index 704dcde3c..0c307fbbd 100644
--- a/API.txt
+++ b/API.txt
@@ -2294,7 +2294,7 @@ arg: Str('fqdn', cli_name='hostname')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
@@ -2461,7 +2461,7 @@ arg: Str('fqdn', cli_name='hostname')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
@@ -2698,7 +2698,7 @@ arg: Str('ipaanchoruuid', cli_name='anchor')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('fallback_to_ldap?', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
@@ -2770,7 +2770,7 @@ arg: Str('ipaanchoruuid', cli_name='anchor')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('fallback_to_ldap?', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
@@ -4293,7 +4293,7 @@ arg: Principal('krbprincipalname', cli_name='principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
@@ -4424,7 +4424,7 @@ arg: Principal('krbprincipalname', cli_name='principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
@@ -5668,7 +5668,7 @@ arg: Str('uid', cli_name='login')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
@@ -5828,7 +5828,7 @@ arg: Str('uid', cli_name='login')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Bytes('usercertificate*', alwaysask=True, cli_name='certificate')
+option: Bytes('usercertificate+', alwaysask=True, cli_name='certificate')
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
diff --git a/VERSION b/VERSION
index 23ceecc98..a35aec56c 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=205
-# Last change: Add --ca option to cert-revoke and cert-remove-hold
+IPA_API_VERSION_MINOR=206
+# Last change: mbabinsk: commands that use positional parameters to manage attributes
diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index c35f660c7..6107e43a6 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2288,28 +2288,34 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
         raise exc
 
 
-class LDAPModAttribute(LDAPQuery):
+class BaseLDAPModAttribute(LDAPQuery):
 
     attribute = None
 
     has_output = output.standard_entry
 
-    def get_options(self):
-        for option in super(LDAPModAttribute, self).get_options():
-            yield option
-
-        option = self.obj.params[self.attribute]
-        attribute = 'virtual_attribute' not in option.flags
-        yield option.clone(attribute=attribute, alwaysask=True)
+    def _get_attribute_param(self):
+        arg = self.obj.params[self.attribute]
+        attribute = 'virtual_attribute' not in arg.flags
+        return arg.clone(required=True, attribute=attribute, alwaysask=True)
 
     def _update_attrs(self, update, entry_attrs):
         raise NotImplementedError("%s.update_attrs()", self.__class__.__name__)
 
     def execute(self, *keys, **options):
         ldap = self.obj.backend
+        try:
+            index = tuple(self.args).index(self.attribute)
+        except ValueError:
+            obj_keys = keys
+        else:
+            obj_keys = keys[:index]
 
-        dn = self.obj.get_dn(*keys, **options)
-        entry_attrs = ldap.make_entry(dn, self.args_options_2_entry(**options))
+        dn = self.obj.get_dn(*obj_keys, **options)
+        entry_attrs = ldap.make_entry(dn, self.args_options_2_entry(
+            *keys, **options))
+
+        entry_attrs.pop(self.obj.primary_key.name, None)
 
         if options.get('all', False):
             attrs_list = ['*', self.obj.primary_key.name]
@@ -2326,6 +2332,7 @@ class LDAPModAttribute(LDAPQuery):
         try:
             update = self._exc_wrapper(keys, options, ldap.get_entry)(
                 entry_attrs.dn, list(entry_attrs))
+
             self._update_attrs(update, entry_attrs)
 
             self._exc_wrapper(keys, options, ldap.update_entry)(update)
@@ -2347,7 +2354,7 @@ class LDAPModAttribute(LDAPQuery):
         entry_attrs = entry_to_dict(entry_attrs, **options)
 
         if self.obj.primary_key:
-            pkey = keys[-1]
+            pkey = obj_keys[-1]
         else:
             pkey = None
 
@@ -2367,7 +2374,7 @@ class LDAPModAttribute(LDAPQuery):
         raise exc
 
 
-class LDAPAddAttribute(LDAPModAttribute):
+class BaseLDAPAddAttribute(BaseLDAPModAttribute):
     msg_summary = _('added attribute value to entry %(value)')
 
     def _update_attrs(self, update, entry_attrs):
@@ -2377,14 +2384,13 @@ class LDAPAddAttribute(LDAPModAttribute):
 
             if not old_value.isdisjoint(value_to_add):
                 raise errors.ExecutionError(
-                    message=_('\'%s\' already contains one or more values'
-                              % name)
-                )
+                    message=_('\'%(attr)s\' already contains one or more '
+                              'values') % dict(attr=name))
 
             update[name] = list(old_value | value_to_add)
 
 
-class LDAPRemoveAttribute(LDAPModAttribute):
+class BaseLDAPRemoveAttribute(BaseLDAPModAttribute):
     msg_summary = _('removed attribute values from entry %(value)')
 
     def _update_attrs(self, update, entry_attrs):
@@ -2397,3 +2403,39 @@ class LDAPRemoveAttribute(LDAPModAttribute):
                     attr=name, value=_("one or more values to remove"))
 
             update[name] = list(old_value - value_to_remove)
+
+
+class LDAPModAttribute(BaseLDAPModAttribute):
+
+    def get_args(self):
+        for arg in super(LDAPModAttribute, self).get_args():
+            yield arg
+
+        yield self._get_attribute_param()
+
+
+class LDAPAddAttribute(LDAPModAttribute, BaseLDAPAddAttribute):
+    pass
+
+
+class LDAPRemoveAttribute(LDAPModAttribute, BaseLDAPRemoveAttribute):
+    pass
+
+
+class LDAPModAttributeViaOption(BaseLDAPModAttribute):
+
+    def get_options(self):
+        for option in super(LDAPModAttributeViaOption, self).get_options():
+            yield option
+
+        yield self._get_attribute_param()
+
+
+class LDAPAddAttributeViaOption(LDAPModAttributeViaOption,
+                                BaseLDAPAddAttribute):
+    pass
+
+
+class LDAPRemoveAttributeViaOption(LDAPModAttributeViaOption,
+                                   BaseLDAPRemoveAttribute):
+    pass
diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 6210e8c16..11bddb505 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -32,7 +32,9 @@ from .baseldap import (LDAPQuery, LDAPObject, LDAPCreate,
                                      LDAPRetrieve, LDAPAddMember,
                                      LDAPRemoveMember, host_is_master,
                                      pkey_to_value, add_missing_object_class,
-                                     LDAPAddAttribute, LDAPRemoveAttribute)
+                                     LDAPAddAttribute, LDAPRemoveAttribute,
+                                     LDAPAddAttributeViaOption,
+                                     LDAPRemoveAttributeViaOption)
 from ipaserver.plugins.service import (
     validate_realm, normalize_principal, validate_certificate,
     set_certificate_attrs, ticket_flags_params, update_krbticketflags,
@@ -1311,14 +1313,14 @@ class host_disallow_create_keytab(LDAPRemoveMember):
 
 
 @register()
-class host_add_cert(LDAPAddAttribute):
+class host_add_cert(LDAPAddAttributeViaOption):
     __doc__ = _('Add certificates to host entry')
     msg_summary = _('Added certificates to host "%(value)s"')
     attribute = 'usercertificate'
 
 
 @register()
-class host_remove_cert(LDAPRemoveAttribute):
+class host_remove_cert(LDAPRemoveAttributeViaOption):
     __doc__ = _('Remove certificates from host entry')
     msg_summary = _('Removed certificates from host "%(value)s"')
     attribute = 'usercertificate'
diff --git a/ipaserver/plugins/idviews.py b/ipaserver/plugins/idviews.py
index 755b07c78..92d47f553 100644
--- a/ipaserver/plugins/idviews.py
+++ b/ipaserver/plugins/idviews.py
@@ -23,7 +23,8 @@ import six
 
 from .baseldap import (LDAPQuery, LDAPObject, LDAPCreate,
                                      LDAPDelete, LDAPUpdate, LDAPSearch,
-                                     LDAPAddAttribute, LDAPRemoveAttribute,
+                                     LDAPAddAttributeViaOption,
+                                     LDAPRemoveAttributeViaOption,
                                      LDAPRetrieve, global_output_params)
 from .hostgroup import get_complete_hostgroup_member_list
 from .service import validate_certificate
@@ -961,12 +962,13 @@ class idoverridegroup(baseidoverride):
     override_object = 'group'
 
 @register()
-class idoverrideuser_add_cert(LDAPAddAttribute):
+class idoverrideuser_add_cert(LDAPAddAttributeViaOption):
     __doc__ = _('Add one or more certificates to the idoverrideuser entry')
     msg_summary = _('Added certificates to idoverrideuser "%(value)s"')
     attribute = 'usercertificate'
 
-    takes_options = LDAPAddAttribute.takes_options + (fallback_to_ldap_option,)
+    takes_options = LDAPAddAttributeViaOption.takes_options + (
+        fallback_to_ldap_option,)
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys,
                      **options):
@@ -983,12 +985,13 @@ class idoverrideuser_add_cert(LDAPAddAttribute):
 
 
 @register()
-class idoverrideuser_remove_cert(LDAPRemoveAttribute):
+class idoverrideuser_remove_cert(LDAPRemoveAttributeViaOption):
     __doc__ = _('Remove one or more certificates to the idoverrideuser entry')
     msg_summary = _('Removed certificates from idoverrideuser "%(value)s"')
     attribute = 'usercertificate'
 
-    takes_options = LDAPRemoveAttribute.takes_options + (fallback_to_ldap_option,)
+    takes_options = LDAPRemoveAttributeViaOption.takes_options + (
+        fallback_to_ldap_option,)
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys,
                      **options):
diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index 70bf31fd4..ec0bc5fbe 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -39,7 +39,9 @@ from .baseldap import (
     LDAPRemoveMember,
     LDAPQuery,
     LDAPAddAttribute,
-    LDAPRemoveAttribute)
+    LDAPRemoveAttribute,
+    LDAPAddAttributeViaOption,
+    LDAPRemoveAttributeViaOption)
 from ipalib import x509
 from ipalib import _, ngettext
 from ipalib import util
@@ -881,14 +883,14 @@ class service_disable(LDAPQuery):
 
 
 @register()
-class service_add_cert(LDAPAddAttribute):
+class service_add_cert(LDAPAddAttributeViaOption):
     __doc__ = _('Add new certificates to a service')
     msg_summary = _('Added certificates to service principal "%(value)s"')
     attribute = 'usercertificate'
 
 
 @register()
-class service_remove_cert(LDAPRemoveAttribute):
+class service_remove_cert(LDAPRemoveAttributeViaOption):
     __doc__ = _('Remove certificates from a service')
     msg_summary = _('Removed certificates from service principal "%(value)s"')
     attribute = 'usercertificate'
diff --git a/ipaserver/plugins/user.py b/ipaserver/plugins/user.py
index 7c5221c85..c231847d5 100644
--- a/ipaserver/plugins/user.py
+++ b/ipaserver/plugins/user.py
@@ -53,8 +53,8 @@ from .baseldap import (
     LDAPSearch,
     LDAPQuery,
     LDAPMultiQuery,
-    LDAPAddAttribute,
-    LDAPRemoveAttribute)
+    LDAPAddAttributeViaOption,
+    LDAPRemoveAttributeViaOption)
 from . import baseldap
 from ipalib.request import context
 from ipalib import _, ngettext
@@ -1136,7 +1136,7 @@ class user_status(LDAPQuery):
 
 
 @register()
-class user_add_cert(LDAPAddAttribute):
+class user_add_cert(LDAPAddAttributeViaOption):
     __doc__ = _('Add one or more certificates to the user entry')
     msg_summary = _('Added certificates to user "%(value)s"')
     attribute = 'usercertificate'
@@ -1158,7 +1158,7 @@ class user_add_cert(LDAPAddAttribute):
 
 
 @register()
-class user_remove_cert(LDAPRemoveAttribute):
+class user_remove_cert(LDAPRemoveAttributeViaOption):
     __doc__ = _('Remove one or more certificates to the user entry')
     msg_summary = _('Removed certificates from user "%(value)s"')
     attribute = 'usercertificate'