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

[Rob Crittenden]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-940-2-validation.patch
Type: application/mbox
Size: 35935 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120206/404d255e/attachment.mbox>