[Freeipa-devel] [PATCH] 44 Fix parameter validation

[Jan Cholasta]

On 21.9.2011 21:31, Rob Crittenden wrote:
> Jan Cholasta wrote:
>> On 25.8.2011 18:21, Jan Cholasta wrote:
>>> What this patch does:
>>>
>>> * Make sure arguments are validated and default values are filled in
>>> before calling a command.
>>> * Add new parameter flag "validate_search" to force validation on search
>>> arguments.
>>> * Fix validation of IP network parameters in the DNS plugin.
>>>
>>> https://fedorahosted.org/freeipa/ticket/1627
>>>
>>> Honza
>>>
>>
>> Redone the patch and split it to 3 parts:
>
> I haven't tested these yet, these comments are just from reading the
> patches.
>
>>
>> * [PATCH 46] Add IP address and IP network parameter types
>> Adds two new parameter types, IPAddress and IPNetwork (which replaces
>> the validate_search flag, as it was just a hack).
>
> A param type looks like the way to go. There are other IPaddress
> parameters, such in host, should this be expanded to cover that?

All of them are covered in the patch already.

>
> Not sure about replacing List with Str types. It may make no difference
> since I'm not sure how a List could be passed for some of these. Can you
> make sure there is no possibility that on the wire someone couldn't pass
> these as a list and actually do something reasonable in the server? I
> suspect they can't but this is a rather major datatype change.

I think my patch breaks dnsrecord-add example.com test --a-rec 
192.0.2.122,203.0.113.45 and the like. This TODO item should be 
implemented to fix that:

   * All "comma-separated list of..." parameters should really be 
changed to multivalue and have a flag that tells the CLI whether a 
multivalue should be parsed as comma-separated.  The List type currently 
satisfy this, but it would be nice to have a comma-separated multivalue 
of any type.

>
>>
>> * [PATCH 44] Fix parameter validation
>> Changes Command.get_default so that default_from parameters are
>> validated before they are used to create the default value.
>
> Just a heads-up but this will conflict big time with my password patch.

Yes, I've noticed that.

>
> It throws away the ordering that the parameters had. This could impact
> the order in which interactive prompting happens. Did you verify that it
> still works sanely?

What kind of sanity do you have in mind? The ordering isn't much 
different, just extra care is taken to order parameters with 
default_from right.

I guess I can store the ordering somewhere else and leave the params 
namespace ordering in its original form.

>
>> * [PATCH 47] Remove create_default
>> Removes create_default, as it does exactly the same thing as
>> default_from, but without the advantage of knowing what parameters are
>> used to create the default value. All uses of create_default are
>> replaced by default_from with no arguments, because that's all
>> create_default is currently used for in IPA.
>
> I'm not sure why you want to remove this, is it causing problems with
> validation?

It could, if it was actually used to dynamically create default value 
from existing parameter values (there is no such use of it in IPA ATM), 
because we don't know in advance which parameters it might use and thus 
don't know how to order the parameter properly.

I'm not sure why we should keep it in, I can't think of any use of it 
that can't be done with default_from. Isn't getting rid of redundant 
stuff OK?

>
> In general the patch comments need more details. This patch e-mail has
> more information on what each patch does than the patches themselves,
> including lacking ticket info.
>
> rob

Honza

-- 
Jan Cholasta