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

[Jan Cholasta]

Dne 7.2.2012 15:15, Rob Crittenden napsal(a):
> 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

Yes please, I had something like this in mind.

Honza

-- 
Jan Cholasta