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

[Petr Viktorin]

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.


-- 
Petr³