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

[Petr Viktorin]

On 02/29/2012 04:09 PM, Petr Viktorin wrote:
> 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.
>

And here is patch 12 rebased against current master.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0012-02-Enforce-that-required-attributes-can-t-be-set-to-Non.patch
Type: text/x-patch
Size: 4625 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120302/a311e6ed/attachment.bin>