[Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Petr Viktorin]

On 02/29/2012 11:14 AM, Jan Cholasta wrote:
> On 29.2.2012 11:09, Petr Viktorin wrote:
>> On 02/28/2012 03:19 PM, Jan Cholasta wrote:
>>> On 28.2.2012 11:54, Petr Viktorin wrote:
>>>> On 02/27/2012 10:44 PM, Rob Crittenden wrote:
>>>>> Petr Viktorin wrote:
>>>>>> On 02/20/2012 08:51 PM, Rob Crittenden wrote:
>>>>>>> Petr Viktorin wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> This looks good but I think there are other things to protect in
>>>>>>> config
>>>>>>> as well such as the default e-mail domain. It is probably safe to
>>>>>>> say
>>>>>>> that everything in there is required.
>>>>>>>
>>>>>>> rob
>>>>>>
>>>>>> Let me just double-check this with you.
>>>>>>
>>>>>> According to code in the user plugin (around line 330), if the
>>>>>> default
>>>>>> e-mail domain is not set, users don't get an address
>>>>>> auto-assigned. Do
>>>>>> we really want to require user e-mails?
>>>>>>
>>>>>> ipaconfigstring (the password plugin flags) are a set (multivalue,
>>>>>> not
>>>>>> required).
>>>>>>
>>>>>> The rest of the values I left as not required are for optional
>>>>>> features
>>>>>> or limits: search results & time limit, max. username length,
>>>>>> password
>>>>>> expiry notification. Currently if these are missing, the
>>>>>> feature/limit
>>>>>> is disabled (well, except for the time limit).
>>>>>> But, there are also special values (0 or -1) that have the same
>>>>>> effect
>>>>>> as a missing value. Sometimes they're documented.
>>>>>> So we want to enforce that users use these special values instead of
>>>>>> removing the config entry?
>>>>>
>>>>> I think we want to enforce that these are defined. It will be
>>>>> confusing
>>>>> for users if these are not there at all. I don't think we need to show
>>>>> the special options, just declare that the attribute is required.
>>>>>
>>>>> rob
>>>>>
>>>>
>>>> Attaching updated patch 13.
>>>>
>>>> Only the default e-mail domain
>>>> (https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
>>>> still optional.
>>>>
>>>
>>> You have removed all the config-related defensive code in the patch, is
>>> this a good idea? What will happen if someone e.g. deletes a required
>>> config attribute directly from LDAP?
>>>
>>
>> Then IPA crashes. The defensive code wasn't there for all cases anyway,
>> as ticket #2159 shows.
>>
>> If we want to protect against this it would probably be better to make
>> the config class itself give the default when a required value is
>> missing.
>>
>
> This, and raise an error in cases where no default is available (the
> check should probably be done in ldap.get_ipa_config).
>
> Honza
>

Would a better approach be to modify the LDAP schema to require these 
values?

-- 
Petr³