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

[Jan Cholasta]

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

-- 
Jan Cholasta