[Freeipa-devel] [PATCH] 940 apply some validation to some classes only

[Jan Cholasta]

On 7.2.2012 20:25, Rob Crittenden wrote:
> Rob Crittenden wrote:
>> Jan Cholasta wrote:
>>> Dne 7.2.2012 09:27, Martin Kosek napsal(a):
>>>> On Mon, 2012-02-06 at 11:52 -0500, Rob Crittenden wrote:
>>>>> Martin Kosek wrote:
>>>>>> On Fri, 2012-02-03 at 16:58 -0500, Rob Crittenden wrote:
>>>>>>> There is some validation that we only need to apply when an entry is
>>>>>>> being created, namely the key itself. This is to allow us to
>>>>>>> manage an
>>>>>>> otherwise illegal entry that finds its way into the system (i.e.
>>>>>>> migration).
>>>>>>>
>>>>>>> Consider this. We migrate a group with a space in it. This isn't
>>>>>>> allowed
>>>>>>> in IPA but we also provide no way to delete it because the cn regex
>>>>>>> kicks out the group-del command.
>>>>>>>
>>>>>>> The trick is adding appropriate context so we can know during
>>>>>>> validation
>>>>>>> how we got here. A command object has a bases field which contains
>>>>>>> the
>>>>>>> base classes associated with it, which appears to contain only the
>>>>>>> leaf
>>>>>>> baseclass. So using this we can tell how we got to validation and
>>>>>>> can
>>>>>>> skip based on that baseclass name.
>>>>>>>
>>>>>>> I went back and forth a bit on where the right place to put this
>>>>>>> was,
>>>>>>> I'm open to more fine tuning. I initially skipped just the pattern
>>>>>>> validation then expanded it to apply to all validation in the
>>>>>>> Parameter.
>>>>>>>
>>>>>>> rob
>>>>>>
>>>>>> 1) This patch breaks API.txt, it needs re-generating.
>>>>>>
>>>>>> 2) This may be a matter of opinion but I think that
>>>>>> + if self.onlyvalidateclasses and \
>>>>>> + not any(x in self.onlyvalidateclasses for x in classbases):
>>>>>> + return
>>>>>>
>>>>>> would be more readable than:
>>>>>>
>>>>>> + if self.onlyvalidateclasses and \
>>>>>> + not [x for x in classbases if x in self.onlyvalidateclasses]:
>>>>>> + return
>>>>>>
>>>>>> 3) I would move the entire self.onlyvalidateclasses up to the
>>>>>> validate()
>>>>>> method. It would have several benefits:
>>>>>> - the validation skip would be done just once, not for every value as
>>>>>> the decision does not use the value at all
>>>>>> - we would not have to modify _validate_scalar() methods at all since
>>>>>> they won't need classbases
>>>>>>
>>>>>> 4) I think it would be better to keep validation for --rename
>>>>>> options.
>>>>>> As it is generated from RDN attribute parameter, it inherits
>>>>>> onlyvalidateclasses list as well.
>>>>>>
>>>>>> Otherwise your patch would let user to rename a valid object to an
>>>>>> invalid one:
>>>>>>
>>>>>> # ipa user-mod fbar --rename="bad boy"
>>>>>> --------------------
>>>>>> Modified user "fbar"
>>>>>> --------------------
>>>>>> User login: bad boy
>>>>>> First name: Foo
>>>>>> Last name: Bar
>>>>>> Home directory: /home/fbar
>>>>>> Login shell: /bin/sh
>>>>>> UID: 480800040
>>>>>> GID: 480800040
>>>>>> Account disabled: False
>>>>>> Password: False
>>>>>> Member of groups: ipausers
>>>>>> Kerberos keys available: False
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>
>>>>> This should address your concerns.
>>>>>
>>>>> rob
>>>>>
>>>>
>>>> Its almost OK, there is just one part that's not that OK:
>>>>
>>>> @@ -831,6 +836,9 @@ class Param(ReadOnly):
>>>> else:
>>>> raise RequirementError(name=self.name)
>>>> return
>>>> + if 'rename' not in (self.name, self.cli_name):
>>>> + if self.onlyvalidateclasses and not [x for x in
>>>> self.onlyvalidateclasses if x in classbases]: #pylint: disable=E1101
>>>> + return
>>>> if self.multivalue:
>>>> if type(value) is not tuple:
>>>> raise TypeError(
>>>>
>>>> I don't think that hard-coding this skip for onlyvalidateclasses based
>>>> just on parameter name is a good thing to do and may cause problems in
>>>> the future. For example if we create some option called "rename" and
>>>> deliberately set onlyvalidateclasses for the option - it would then be
>>>> skipped as well.
>>>>
>>>> I think it would be a better solution to just update
>>>> _get_rename_option() in baseldap.py to set onlyvalidateclasses to ()
>>>>
>>>> Martin
>>>>
>>>
>>> I must say I'm not a big fan of this approach. You are healing the
>>> symptoms (custom validation on some parameters makes it impossible to
>>> manipulate existing LDAP entries with invalid attribute values => add a
>>> way to mark parameters to be validated only in certain commands to
>>> prevent that), but the real issue here is that we should not perform
>>> custom validation when referring to existing LDAP attribute values at
>>> all (or only partially), no matter what parameter and command. Fixing
>>> this would make the problem go away for all commands, present or future,
>>> without the need for adding a list of command classes to each parameter
>>> that is affected.
>>>
>>> Honza
>>>
>>
>> It is all about context, and parameters have very little of it. The
>> bottom line is we only want to do validation on new values.
>>
>> I think I can bump this up a level by adding a validate boolean to Param
>> and Command both defaulting to False. crud.Create will override this to
>> True and cloning rename could perhaps also set this flag.
>>
>> Then in validation if the parent object has validation set or the
>> parameter does we validate, otherwise we skip it.
>>
>> This will require some other changes for Enums, they may always require
>> validation (I'll need to be sure --delattr can remove a bad one).
>>
>> rob
>
> Updated patch. The primary key will be validated only on adds. The
> values will be validated on adds and mods.

It turns out there already is infrastructure which does exactly the same 
thing: the "query" kwarg of the Parameter class. We should fix its 
behavior rather than reinvent the wheel.

>
> rob

Also, the "pattern" and "pattern_errmsg" kwargs should not be copied 
from the primary key parameter, but from the RDN parameter (imagine an 
object which has cn as its primary key and ipaUniqueId as RDN, you would 
use the pattern from cn on ipaUniqueId, which is obviously not right). 
It is supposed be done automatically as part of the clone_rename() call, 
so if it's not happening, there's probably a bug in clone_rename.

Honza

-- 
Jan Cholasta