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

Wed Apr 4 10:44:04 UTC 2012

On 03/30/2012 03:22 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 03/16/2012 08:01 PM, Petr Viktorin wrote:
>>> On 03/16/2012 06:35 PM, Petr Viktorin wrote:
>>>> 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.
>>>
>>> Okay, this is a different problem. A quick grep of ConversionError in
>>> parameters.py reveals that all of the "wrong datatype" errors are raised
>>> with the raw parameter name.
>>> Other errors are raised with either that or the cli_name or they
>>> auto-detect. I don't think they follow some rule in this.
>>>
>>> Furthermore, our test suite doesn't check exception arguments. Sounds
>>> like a major cleanup coming up 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.
>>>>
>>>
>>> Fixed in the attached update, which encodes the value.
>>>
>>> I was surprised to find that
>>> config_mod.params['ipamigrationenabled'].attribute is True, while
>>> config_mod.obj.params['ipamigrationenabled'].attribute is False (and so
>>> its encode() method does nothing). That's because 'attribute' is only
>>> set when the params are cloned from the LDAPObject to the CRUD method.
>>> Is there a reason behind this, or is it just that it was easier to do?
>>>
>>> For this case it means that params marked no_update will not be encoded
>>> properly -- getting to a working encode() would require either setting
>>> 'attribute' on the parent object or some ugly hackery.
>>>
>>>
>>>
>>> ---
>>> Petr³
>>>
>>
>> Rebased to current master.
>>
>
> There is a test failure. I ran out of time last night to investigate
> whether the test is valid or it is an issue with the patch, forgot to
> send this out before I left for the day yesterday.
>
> ======================================================================
> FAIL: test_attr[23]: pwpolicy_mod: Try setting non-numeric krbpwdmaxfailure
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/nose/case.py", line 187, in runTest
> self.test(*self.arg)
> File
> "/home/rcrit/redhat/freeipa-beta1/tests/test_xmlrpc/xmlrpc_test.py",
> line 247, in <lambda>
> func = lambda: self.check(nice, **test)
> File
> "/home/rcrit/redhat/freeipa-beta1/tests/test_xmlrpc/xmlrpc_test.py",
> line 260, in check
> self.check_exception(nice, cmd, args, options, expected)
> File
> "/home/rcrit/redhat/freeipa-beta1/tests/test_xmlrpc/xmlrpc_test.py",
> line 277, in check_exception
> UNEXPECTED % (cmd, name, args, options, e.__class__.__name__, e)
> AssertionError: Expected 'pwpolicy_mod' to raise ConversionError, but
> caught different.
> args = []
> options = {'setattr': u'krbpwdmaxfailure=abc'}
> ValidationError: invalid 'krbpwdmaxfailure': must be an integer

Fixed. I should have caught this on Friday.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0016-07-Defer-conversion-and-validation-until-after-add-del-.patch
Type: text/x-patch
Size: 9635 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120404/872f1f41/attachment.bin>