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

[Rob Crittenden]

Martin Kosek wrote:
> 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 ()

I'm not too keen on it either but otherwise all validation would happen 
whenever there is a rename option, which is what we're trying to prevent.

rob