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

[Petr Viktorin]

On 02/29/2012 03:53 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> 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?
>>
>
> I think that may be a longer-term fix. I propose we keep the defensive
> code in for now and correct it in the future.
>
> rob

Here is an updated patch 13 that does that.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0013-03-Mark-several-config-options-as-required.patch
Type: text/x-patch
Size: 4909 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120229/a6998253/attachment.bin>