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

[Jan Cholasta]

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?

Honza

-- 
Jan Cholasta