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

[Petr Viktorin]

On 03/16/2012 06:33 PM, Rob Crittenden wrote:
> Rob Crittenden wrote:
>> 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
>
> On second thought, this may not be related...

This is https://fedorahosted.org/freeipa/ticket/1418, I'll see if it 
makes sense to fix it here.

>
>>
>> The encoding problem does still exist too:
>>
>> $ ipa config-mod --setattr ipamigrationenabled=false
>> ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax: Invalid
>> syntax.
>>

Will fix.

-- 
Petr³