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

[Petr Viktorin]

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.

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