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

[Rob Crittenden]

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...

>
> The encoding problem does still exist too:
>
> $ ipa config-mod --setattr ipamigrationenabled=false
> ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax: Invalid
> syntax.
>
> rob
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel