Improve sudooption docs, make the option multi-value

I don't know why this wasn't always multi-value but if one wanted
to set multiple options they needed to call add-option multiple
times. The LDAP attribute is already multi-value.

This shouldn't cause API issues as it understood the attribute as
multi-value just didn't expose it. Client output on the CLI will
look a bit different:

Added option "('one', 'two')" to Sudo Rule "test"

or

Added option "(u'one', u'Two')" to Sudo Rule "test"

instead of with this change:

Added option "one,two" to Sudo Rule "test"

Removing an option works in a similar way.

The value is normalized on the client side in order to ensure that
the option value is always a tuple.

https://pagure.io/freeipa/issue/2278

Signed-off-by: Rob Crittenden <[email protected]>
Reviewed-By: Florence Blanc-Renaud <[email protected]>

API.txt

@@ -5569,7 +5569,7 @@ command: sudorule_add_option/1
 args: 1,5,3
 arg: Str('cn', cli_name='sudorule_name')
 option: Flag('all', autofill=True, cli_name='all', default=False)
-option: Str('ipasudoopt', cli_name='sudooption')
+option: Str('ipasudoopt+', cli_name='sudooption')
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
 option: Str('version?')
@@ -5724,7 +5724,7 @@ command: sudorule_remove_option/1
 args: 1,5,3
 arg: Str('cn', cli_name='sudorule_name')
 option: Flag('all', autofill=True, cli_name='all', default=False)
-option: Str('ipasudoopt', cli_name='sudooption')
+option: Str('ipasudoopt+', cli_name='sudooption')
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
 option: Str('version?')

VERSION.m4

@@ -86,8 +86,8 @@ define(IPA_DATA_VERSION, 20100614120000)
 # #
 ########################################################
 define(IPA_API_VERSION_MAJOR, 2)
-# Last change: add subordinate id feature
-define(IPA_API_VERSION_MINOR, 243)
+# Last change: make sudorule option multivalue
+define(IPA_API_VERSION_MINOR, 244)
 
 
 ########################################################

ipaclient/plugins/sudorule.py

@@ -39,9 +39,11 @@ def output_for_cli(self, textui, result, cn, **options):
 @register(override=True, no_fail=True)
 class sudorule_add_option(MethodOverride):
  def output_for_cli(self, textui, result, cn, **options):
+ opts = self.normalize(**options)
  textui.print_dashed(
  _('Added option "%(option)s" to Sudo Rule "%(rule)s"')
- % dict(option=options['ipasudoopt'], rule=cn))
+ % dict(option=','.join(opts['ipasudoopt']), rule=cn)
+ )
 
  super(sudorule_add_option, self).output_for_cli(textui, result, cn,
  **options)
@@ -50,8 +52,10 @@ def output_for_cli(self, textui, result, cn, **options):
 @register(override=True, no_fail=True)
 class sudorule_remove_option(MethodOverride):
  def output_for_cli(self, textui, result, cn, **options):
+ opts = self.normalize(**options)
  textui.print_dashed(
  _('Removed option "%(option)s" from Sudo Rule "%(rule)s"')
- % dict(option=options['ipasudoopt'], rule=cn))
+ % dict(option=','.join(opts['ipasudoopt']), rule=cn)
+ )
  super(sudorule_remove_option, self).output_for_cli(textui, result, cn,
  **options)

ipaserver/plugins/sudorule.py

@@ -57,6 +57,11 @@
  RunAsGroup: The group(s) whose gid rights Sudo will be invoked with.
  Options: The various Sudoers Options that can modify Sudo's behavior.
 """) + _("""
+Each option needs to be added separately and no validation is done whether
+the option is known by sudo or is in a valid format. Environment variables
+also need to be set individually. For example env_keep="FOO BAR" in sudoers
+needs be represented as --sudooption env_keep=FOO --sudooption env_keep+=BAR.
+""") + _("""
 An order can be added to a sudorule to control the order in which they
 are evaluated (if the client supports it). This order is an integer and
 must be unique.
@@ -89,6 +94,10 @@
 """) + _("""
  Set a default Sudo option:
  ipa sudorule-add-option defaults --sudooption '!authenticate'
+""") + _("""
+ Set multiple default Sudo options:
+ ipa sudorule-add-option defaults --sudooption '!authenticate' \
+ --sudooption mail_badpass
 """) + _("""
  Set SELinux type and role transitions on a rule:
  ipa sudorule-add-option sysadmin_sudo --sudooption type=unconfined_t
@@ -353,7 +362,7 @@ class sudorule(LDAPObject):
  label=_('RunAs External Group'),
  doc=_('External Group the commands can run as (sudorule-find only)'),
  ),
- Str('ipasudoopt?',
+ Str('ipasudoopt*',
  label=_('Sudo Option'),
  flags=['no_create', 'no_update', 'no_search'],
  ),
@@ -989,7 +998,7 @@ class sudorule_add_option(LDAPQuery):
 
  has_output = output.standard_entry
  takes_options = (
- Str('ipasudoopt',
+ Str('ipasudoopt+',
  cli_name='sudooption',
  label=_('Sudo Option'),
  ),
@@ -1000,16 +1009,17 @@ def execute(self, cn, **options):
 
  dn = self.obj.get_dn(cn)
 
- if not options['ipasudoopt'].strip():
+ if len(options.get('ipasudoopt',[])) == 0:
  raise errors.EmptyModlist()
  entry_attrs = ldap.get_entry(dn, ['ipasudoopt'])
 
  try:
- if options['ipasudoopt'] not in entry_attrs['ipasudoopt']:
- entry_attrs.setdefault('ipasudoopt', []).append(
- options['ipasudoopt'])
- else:
- raise errors.DuplicateEntry
+ entry_attrs.setdefault('ipasudoopt', [])
+ for option in options['ipasudoopt']:
+ if option not in entry_attrs['ipasudoopt']:
+ entry_attrs['ipasudoopt'].append(option)
+ else:
+ raise errors.DuplicateEntry
  except KeyError:
  entry_attrs.setdefault('ipasudoopt', []).append(
  options['ipasudoopt'])
@@ -1037,7 +1047,7 @@ class sudorule_remove_option(LDAPQuery):
 
  has_output = output.standard_entry
  takes_options = (
- Str('ipasudoopt',
+ Str('ipasudoopt+',
  cli_name='sudooption',
  label=_('Sudo Option'),
  ),
@@ -1047,29 +1057,32 @@ def execute(self, cn, **options):
  ldap = self.obj.backend
 
  dn = self.obj.get_dn(cn)
-
- if not options['ipasudoopt'].strip():
+ if len(options.get('ipasudoopt',[])) == 0:
  raise errors.EmptyModlist()
 
  entry_attrs = ldap.get_entry(dn, ['ipasudoopt'])
 
  try:
- if options['ipasudoopt'] in entry_attrs['ipasudoopt']:
- entry_attrs.setdefault('ipasudoopt', []).remove(
- options['ipasudoopt'])
- ldap.update_entry(entry_attrs)
- else:
- raise errors.AttrValueNotFound(
- attr='ipasudoopt',
- value=options['ipasudoopt']
- )
+ entry_attrs.setdefault('ipasudoopt', [])
+ for option in options['ipasudoopt']:
+ if option in entry_attrs['ipasudoopt']:
+ entry_attrs['ipasudoopt'].remove(option)
+ else:
+ raise errors.AttrValueNotFound(
+ attr='ipasudoopt',
+ value=option)
  except ValueError:
  pass
  except KeyError:
  raise errors.AttrValueNotFound(
  attr='ipasudoopt',
  value=options['ipasudoopt']
  )
+
+ try:
+ ldap.update_entry(entry_attrs)
+ except errors.EmptyModlist:
+ pass
  except errors.NotFound:
  raise self.obj.handle_not_found(cn)
 

ipatests/test_xmlrpc/test_sudorule_plugin.py

@@ -25,6 +25,7 @@
 import six
 
 from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test, assert_attr_equal
+from ipatests.test_xmlrpc.xmlrpc_test import assert_deepequal
 from ipalib import api
 from ipalib import errors
 
@@ -61,6 +62,8 @@ class test_sudorule(XMLRPC_test):
  test_runasgroup = u'manager'
  test_category = u'all'
  test_option = u'authenticate'
+ test_option2 = u'fqdn'
+ test_option3 = u'log_allowed'
 
  test_invalid_user = u'+invalid#user'
  test_invalid_host = u'+invalid&host.nonexist.com'
@@ -426,6 +429,65 @@ def test_b_sudorule_remove_option(self):
  )
  assert ret['completed'] == 2
 
+ def test_c_sudorule_add_multiple_options(self):
+ """
+ Test adding two options to Sudo rule using
+ `xmlrpc.sudorule_add_option`.
+ """
+ ret = api.Command['sudorule_add_option'](
+ self.rule_name,
+ ipasudoopt=(self.test_option, self.test_option2, self.test_option3)
+ )
+ entry = ret['result']
+ assert_deepequal(entry.get('ipasudoopt'),
+ (self.test_option, self.test_option2,
+ self.test_option3))
+
+ def test_d_sudorule_add_duplicate_option(self):
+ """
+ Test adding a duplicate option to Sudo rule using
+ `xmlrpc.sudorule_add_option`.
+ """
+ with pytest.raises(errors.DuplicateEntry):
+ api.Command['sudorule_add_option'](
+ self.rule_name,
+ ipasudoopt=(self.test_option,)
+ )
+
+ def test_e_sudorule_remove_one_option(self):
+ """
+ Test removing an option from Sudo rule using
+ `xmlrpc.sudorule_remove_option'.
+ """
+ ret = api.Command['sudorule_remove_option'](
+ self.rule_name, ipasudoopt=self.test_option
+ )
+ entry = ret['result']
+ assert_deepequal(entry.get('ipasudoopt'),
+ (self.test_option2, self.test_option3))
+
+ def test_f_sudorule_remove_multiple_options(self):
+ """
+ Test removing an option from Sudo rule using
+ `xmlrpc.sudorule_remove_option'.
+ """
+ ret = api.Command['sudorule_remove_option'](
+ self.rule_name, ipasudoopt=(self.test_option2, self.test_option3)
+ )
+ entry = ret['result']
+ assert len(entry.get('ipasudoopt', [])) == 0
+
+ def test_g_sudorule_remove_unknown_option(self):
+ """
+ Test removing an unknown option from Sudo rule using
+ `xmlrpc.sudorule_remove_option'.
+ """
+ with pytest.raises(errors.AttrValueNotFound):
+ api.Command['sudorule_remove_option'](
+ self.rule_name,
+ ipasudoopt=(self.test_option,)
+ )
+
  def test_a_sudorule_add_host(self):
  """
  Test adding host and hostgroup to Sudo rule using