https://fedorahosted.org/freeipa/ticket/2159 says various config options are not marked Required, so entering an empty value for it will pass validation (and IPA will blow up later when it expects a string,not None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the attributes *are* marked as Required, an empty string will pass validation. This is because `None` is used in `Param.validate` to mean both "No value supplied" and "Empty value supplied". The method currently assumes the former, and skips validation entirely for `None` values to optional parameters.

For example, the following will delete "membergroup", even though it's a required attribute :

$ ipa delegation-add --attrs=street --group=editors \
 --membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs function, so they aren't affected. That function is specific to LADP objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update object is auto-converted to a non-required parameter * making the`validate` method aware of whether the parameter was supplied; and if it was, honor the "nonempty" flag.


The second patch fixes https://fedorahosted.org/freeipa/ticket/2159 by marking required config options as required.

--
PetrĀ³
>From c58da9f4f1b374ddc84ceb04b3ea58d839cd26a6 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Thu, 16 Feb 2012 07:11:56 -0500
Subject: [PATCH 12/13] Enforce that required attributes can't be set to None
 in CRUD Update

The `required` parameter attribute didn't distinguish between cases
where the parameter is not given and all, and where the parameter is
given but empty. The case of updating a required attribute couldn't
be validated properly, because when it is given but empty, validators
don't run.
This patch introduces a new flag, 'nonempty', that specifies the
parameter can be missing (if not required), but it can't be None.
This flag gets added automatically to required parameters in CRUD
Update.
---
 ipalib/crud.py       |   13 +++++++++++--
 ipalib/frontend.py   |    2 +-
 ipalib/parameters.py |    9 +++++----
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/ipalib/crud.py b/ipalib/crud.py
index 833914cfaa2dd4e99afae8065651435b0bc0f898..505aa5d35b68fc7b40854a29e8bc2e37e4af955e 100644
--- a/ipalib/crud.py
+++ b/ipalib/crud.py
@@ -184,20 +184,29 @@ class Update(PKQuery):
             for option in super(Update, self).get_options():
                 yield option
         for option in self.obj.params_minus_pk():
+            new_flags = option.flags
             attribute = 'virtual_attribute' not in option.flags
+            if option.required:
+                # Required options turn into non-required, since not specifying
+                # them means that they are not changed.
+                # However, they cannot be empty (i.e. explicitly set to None).
+                new_flags = new_flags.union(['nonempty'])
             if 'no_update' in option.flags:
                 continue
             if 'ask_update' in option.flags:
                 yield option.clone(
                     attribute=attribute, query=True, required=False,
-                    autofill=False, alwaysask=True
+                    autofill=False, alwaysask=True, flags=new_flags,
                 )
             elif 'req_update' in option.flags:
                 yield option.clone(
                     attribute=attribute, required=True, alwaysask=False,
+                    flags=new_flags,
                 )
             else:
-                yield option.clone(attribute=attribute, required=False, autofill=False)
+                yield option.clone(attribute=attribute, required=False,
+                    autofill=False, flags=new_flags,
+                )
         if not self.extra_options_first:
             for option in super(Update, self).get_options():
                 yield option
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 028e17e7945c5b193ccbfd84a2f2214e56d2a5fa..da25b4c1aef100cb54d7e248ba4e2ea5dc250cef 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -651,7 +651,7 @@ class Command(HasParam):
         """
         for param in self.params():
             value = kw.get(param.name, None)
-            param.validate(value, self.env.context)
+            param.validate(value, self.env.context, supplied=param.name in kw)
 
     def verify_client_version(self, client_version):
         """
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index c533f9d0b70441003deb3fa399b6c9e204b197a6..755d04dd9446a6622bfe99e899158a1ab04d1790 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -557,9 +557,9 @@ class Param(ReadOnly):
         else:
             value = self.convert(self.normalize(value))
         if hasattr(self, 'env'):
-            self.validate(value, self.env.context)  #pylint: disable=E1101
+            self.validate(value, self.env.context, supplied=self.name in kw)  #pylint: disable=E1101
         else:
-            self.validate(value)
+            self.validate(value, supplied=self.name in kw)
         return value
 
     def kw(self):
@@ -819,15 +819,16 @@ class Param(ReadOnly):
             error=ugettext(self.type_error),
         )
 
-    def validate(self, value, context=None):
+    def validate(self, value, context=None, supplied=None):
         """
         Check validity of ``value``.
 
         :param value: A proposed value for this parameter.
         :param context: The context we are running in.
+        :param supplied: True if this parameter was supplied explicitly.
         """
         if value is None:
-            if self.required:
+            if self.required or (supplied and 'nonempty' in self.flags):
                 if context == 'cli':
                     raise RequirementError(name=self.cli_name)
                 else:
-- 
1.7.7.6

>From 527ad9828c8137d798cba3b3941ac9e689db1bb4 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Mon, 20 Feb 2012 04:03:27 -0500
Subject: [PATCH 13/13] Mark several config options as required

IPA assumes several config options are present, but allowed the user
to delete them. This patch marks them as required.

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

Also, access to the options is changed to use indexing instead of get()
in all cases, as they no nonger need a default.
---
 ipalib/plugins/config.py      |   26 +++++++++++---------------
 ipalib/plugins/group.py       |    2 +-
 ipalib/plugins/migration.py   |    4 ++--
 ipalib/plugins/user.py        |    8 ++++----
 ipaserver/plugins/selfsign.py |    2 +-
 5 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/ipalib/plugins/config.py b/ipalib/plugins/config.py
index ecf424646674040cd9437697691b9dff4c3744bc..1f818f4295e2622eba9879cc8849c287e81fbc19 100644
--- a/ipalib/plugins/config.py
+++ b/ipalib/plugins/config.py
@@ -102,17 +102,17 @@ class config(LDAPObject):
             label=_('Maximum username length'),
             minvalue=1,
         ),
-        IA5Str('ipahomesrootdir?',
+        IA5Str('ipahomesrootdir',
             cli_name='homedirectory',
             label=_('Home directory base'),
             doc=_('Default location of home directories'),
         ),
-        Str('ipadefaultloginshell?',
+        Str('ipadefaultloginshell',
             cli_name='defaultshell',
             label=_('Default shell'),
             doc=_('Default shell for new users'),
         ),
-        Str('ipadefaultprimarygroup?',
+        Str('ipadefaultprimarygroup',
             cli_name='defaultgroup',
             label=_('Default users group'),
             doc=_('Default group for new users'),
@@ -134,34 +134,34 @@ class config(LDAPObject):
             doc=_('Maximum number of records to search (-1 is unlimited)'),
             minvalue=-1,
         ),
-        IA5Str('ipausersearchfields?',
+        IA5Str('ipausersearchfields',
             cli_name='usersearch',
             label=_('User search fields'),
             doc=_('A comma-separated list of fields to search in when searching for users'),
         ),
-        IA5Str('ipagroupsearchfields?',
+        IA5Str('ipagroupsearchfields',
             cli_name='groupsearch',
             label='Group search fields',
             doc=_('A comma-separated list of fields to search in when searching for groups'),
         ),
-        Bool('ipamigrationenabled?',
+        Bool('ipamigrationenabled',
             cli_name='enable_migration',
             label=_('Enable migration mode'),
             doc=_('Enable migration mode'),
         ),
-        Str('ipacertificatesubjectbase?',
+        Str('ipacertificatesubjectbase',
             cli_name='subject',
             label=_('Certificate Subject base'),
             doc=_('Base for certificate subjects (OU=Test,O=Example)'),
             flags=['no_update'],
         ),
-        Str('ipagroupobjectclasses*',
+        Str('ipagroupobjectclasses+',
             cli_name='groupobjectclasses',
             label=_('Default group objectclasses'),
             doc=_('Default group objectclasses (comma-separated list)'),
             csv=True,
         ),
-        Str('ipauserobjectclasses*',
+        Str('ipauserobjectclasses+',
             cli_name='userobjectclasses',
             label=_('Default user objectclasses'),
             doc=_('Default user objectclasses (comma-separated list)'),
@@ -180,11 +180,11 @@ class config(LDAPObject):
             values=(u'AllowLMhash', u'AllowNThash'),
             csv=True,
         ),
-        Str('ipaselinuxusermaporder?',
+        Str('ipaselinuxusermaporder',
             label=_('SELinux user map order'),
             doc=_('Order in increasing priority of SELinux users, delimited by $'),
         ),
-        Str('ipaselinuxusermapdefault?',
+        Str('ipaselinuxusermapdefault',
             label=_('Default SELinux user'),
             doc=_('Default SELinux user when no match is found in SELinux map rule'),
         ),
@@ -244,10 +244,6 @@ class config_mod(LDAPUpdate):
                                 error=_('%(obj)s default attribute %(attr)s would not be allowed!') \
                                 % dict(obj=obj, attr=obj_attr))
 
-        if 'ipaselinuxusermapdefault' in options and options['ipaselinuxusermapdefault'] is None:
-            raise errors.ValidationError(name='ipaselinuxusermapdefault',
-                error=_('SELinux user map default user may not be empty'))
-
         # Make sure the default user is in the list
         if 'ipaselinuxusermapdefault' in options or \
           'ipaselinuxusermaporder' in options:
diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py
index 49bc8232835157d5fffec8e3c4c573399536d818..319bcf40b3e8488f55a7f1fc5d27a90d89a3562b 100644
--- a/ipalib/plugins/group.py
+++ b/ipalib/plugins/group.py
@@ -156,7 +156,7 @@ class group_del(LDAPDelete):
 
     def pre_callback(self, ldap, dn, *keys, **options):
         config = ldap.get_ipa_config()[1]
-        def_primary_group = config.get('ipadefaultprimarygroup', '')
+        def_primary_group = config['ipadefaultprimarygroup']
         def_primary_group_dn = group_dn = self.obj.get_dn(def_primary_group)
         if dn == def_primary_group_dn:
             raise errors.DefaultGroupError()
diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index 688265fd3ea7f62bb22bf78abbc7f26e64f7470b..a58b2670f52851dacdaa14c4b67dbcbfe690b301 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -110,7 +110,7 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
 
     # get default primary group for new users
     if 'def_group_dn' not in ctx:
-        def_group = config.get('ipadefaultprimarygroup')
+        def_group = config['ipadefaultprimarygroup']
         ctx['def_group_dn'] = api.Object.group.get_dn(def_group)
         try:
             (g_dn, g_attrs) = ldap.get_entry(ctx['def_group_dn'], ['gidnumber'])
@@ -628,7 +628,7 @@ can use their Kerberos accounts.''')
         config = ldap.get_ipa_config()[1]
 
         # check if migration mode is enabled
-        if config.get('ipamigrationenabled', ('FALSE', ))[0] == 'FALSE':
+        if config['ipamigrationenabled'][0] == 'FALSE':
             return dict(result={}, failed={}, enabled=False)
 
         # connect to DS
diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index ad9805bec5db0eee248c45022ee7810293a4109d..218e95d296b6152003d7ce5f1a15a59c8991542d 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -422,14 +422,14 @@ class user_add(LDAPCreate):
                         len = int(config.get('ipamaxusernamelength')[0])
                     )
                 )
-        default_shell = config.get('ipadefaultloginshell', ['/bin/sh'])[0]
+        default_shell = config['ipadefaultloginshell'][0]
         entry_attrs.setdefault('loginshell', default_shell)
         # hack so we can request separate first and last name in CLI
         full_name = '%s %s' % (entry_attrs['givenname'], entry_attrs['sn'])
         entry_attrs.setdefault('cn', full_name)
         if 'homedirectory' not in entry_attrs:
             # get home's root directory from config
-            homes_root = config.get('ipahomesrootdir', ['/home'])[0]
+            homes_root = config['ipahomesrootdir'][0]
             # build user's home directory based on his uid
             entry_attrs['homedirectory'] = posixpath.join(homes_root, keys[-1])
         entry_attrs.setdefault('krbpwdpolicyreference', 'cn=global_policy,cn=%s,cn=kerberos,%s' % (api.env.realm, api.env.basedn))
@@ -443,7 +443,7 @@ class user_add(LDAPCreate):
             else:
                 # we're adding new users to a default group, get its gidNumber
                 # get default group name from config
-                def_primary_group = config.get('ipadefaultprimarygroup')
+                def_primary_group = config['ipadefaultprimarygroup']
                 group_dn = self.api.Object['group'].get_dn(def_primary_group)
                 try:
                     (group_dn, group_attrs) = ldap.get_entry(group_dn, ['gidnumber'])
@@ -468,7 +468,7 @@ class user_add(LDAPCreate):
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         config = ldap.get_ipa_config()[1]
         # add the user we just created into the default primary group
-        def_primary_group = config.get('ipadefaultprimarygroup')
+        def_primary_group = config['ipadefaultprimarygroup']
         group_dn = self.api.Object['group'].get_dn(def_primary_group)
         ldap.add_entry_to_group(dn, group_dn)
         if self.api.env.wait_for_attr:
diff --git a/ipaserver/plugins/selfsign.py b/ipaserver/plugins/selfsign.py
index 2f13b1fd583cba851f86ce0fe1897a09c2b1550f..f9e1c5e5a9fba3cde7356fe8d20fd42ab1ebfc16 100644
--- a/ipaserver/plugins/selfsign.py
+++ b/ipaserver/plugins/selfsign.py
@@ -86,7 +86,7 @@ class ra(rabase.rabase):
         """
         try:
             config = api.Command['config_show']()['result']
-            subject_base = config.get('ipacertificatesubjectbase')[0]
+            subject_base = config['ipacertificatesubjectbase'][0]
             hostname = get_csr_hostname(csr)
             base = re.split(',\s*(?=\w+=)', subject_base)
             base.insert(0,'CN=%s' % hostname)
-- 
1.7.7.6

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

Reply via email to