[Freeipa-devel] [PATCH] 0016 Fixes for{add, set, del}attr with managed attributes

[Rob Crittenden]

Petr Viktorin wrote:
> On 03/15/2012 09:24 PM, Rob Crittenden wrote:
>> Petr Viktorin wrote:
>>> On 02/29/2012 04:34 PM, Petr Viktorin wrote:
>>>> On 02/29/2012 03:50 PM, Rob Crittenden wrote:
>>>>> Petr Viktorin wrote:
>>>>>> On 02/27/2012 11:03 PM, Rob Crittenden wrote:
>>>>>>> Petr Viktorin wrote:
>>>>>>>> Patch 16 defers validation & conversion until after
>>>>>>>> {add,del,set}attr is
>>>>>>>> processed, so that we don't search for an integer in a list of
>>>>>>>> strings
>>>>>>>> (this caused ticket #2405), and so that the end result of these
>>>>>>>> operations is validated (#2407).
>>>>>>>>
>>>>>>>>
>>>>>>>> Patch 17 makes these options honor params marked no_create and
>>>>>>>> no_update.
>>>>>>>>
>>>>>>>>
>>>>>>>> https://fedorahosted.org/freeipa/ticket/2405
>>>>>>>> https://fedorahosted.org/freeipa/ticket/2407
>>>>>>>> https://fedorahosted.org/freeipa/ticket/2408
>>>>>>>
>>>>>>> NACK on Patch 17 which breaks patch 16.
>>>>>>
>>>>>> How is patch 16 broken? It works for me.
>>>>>
>>>>> My point is they rely on one another, IMHO, so without 17 the reported
>>>>> problem still exists.
>>>>>
>>>>>>
>>>>>>> *attr is specifically made to be powerful. We don't want to
>>>>>>> arbitrarily
>>>>>>> block updating certain values.
>>>>>>
>>>>>> Noted
>>>>>>
>>>>>>> Not having patch 17 means that the problem reported in 2408 still
>>>>>>> occurs. It should probably check both the schema and the param to
>>>>>>> determine if something can have multiple values and reject that way.
>>>>>>
>>>>>> I see the problem now: the certificate subject base is defined as a
>>>>>> multi-value attribute in the LDAP schema. If it's changed to
>>>>>> single-value the existing validation should catch it.
>>>>>>
>>>>>> Also, most of the config attributes should probably be required in
>>>>>> the
>>>>>> schema. Am I right?
>>>>>>
>>>>>> I'm a newbie here; what do I need to do when changing the schema? Is
>>>>>> there a patch that does something similar I could use as an example?
>>>>>>
>>>>>
>>>>> The framework should be able to impose its own single-value will as
>>>>> well. If a Param is designated as single-value the *attr should honor
>>>>> it.
>>>>
>>>> Here is the updated patch.
>>>> Since *attr is powerful enough to modify 'no_update' Params, which
>>>> CRUDUpdate forgets about, I need to check the params of the LDAPObject
>>>> itself.
>>>>
>>>>> Updating schema is a bit of a nasty business right now. See
>>>>> 10-selinuxusermap.update for an example.
>>>>
>>>> I'll leave schema changes for after the release, then.
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>
>>> Attached patch includes tests. Note that the test depends on my patches
>>> 12-13, which make ipasearchrecordslimit required.
>>
>> I gather that this eliminates the need for patch 17? It seems to work
>> as-is.
>
> Yes. Patch 17 made *attr honor no_create and no_update, which you said
> is not desired behavior.
>
>> The patch doesn't apply because of an encoding change Martin made
>> recently.
>>
>> It does seem to do the right thing though.
>>
>> rob
>
> Attaching rebased patch.
> This deletes Martin's change, but unless I tested wrong, his bug
> (https://fedorahosted.org/freeipa/ticket/2418) stays fixed. The tests in
> my patch should apply to that ticket as well.
>
> In another fork of this thread, there's discussion if this approach is
> good at all. Maybe we're overengineering a corner case here.
>

Found another issue, a very subtle one.

When using *attr and an exception occurs where the param name would 
appear we want the name passed in to be used.

For example:

$ ipa pwpolicy-mod --setattr=krbpwdmaxfailure=xyz

With this patch it will return:
ipa: ERROR: invalid 'maxfail': must be an integer

It should return:
ipa: ERROR: invalid 'krbpwdmaxfailure': must be an integer

The encoding problem does still exist too:

$ ipa config-mod --setattr ipamigrationenabled=false
ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax: Invalid 
syntax.

rob