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

Martin Kosek mkosek at redhat.com
Tue Sep 27 06:47:34 UTC 2011


On Mon, 2011-09-26 at 17:13 +0200, Jan Cholasta wrote:
> On 26.9.2011 14:18, Martin Kosek wrote:
> > On Mon, 2011-09-26 at 11:26 +0200, Jan Cholasta wrote:
> >> On 23.9.2011 09:00, Martin Kosek wrote:
> >>> On Thu, 2011-09-22 at 14:02 +0200, Jan Cholasta wrote:
> >>>> On 22.9.2011 13:27, Martin Kosek wrote:
> >>>>> On Wed, 2011-09-21 at 15:31 -0400, 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?
> >>>>>>
> >>>>>> 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.
> >>>>>>
> >>>>>>>
> >>>>>>> * [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.
> >>>>>>
> >>>>>> 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?
> >>>>>>
> >>>>>>> * [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?
> >>>>>>
> >>>>>> 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
> >>>>>>
> >>>>>
> >>>>> Hm, by the way I am not very excited about adopting this sort of changes
> >>>>> to framework that close to the 6.2 rebase. Are we sure enough that this
> >>>>> won't cause any collateral damage?
> >>>>>
> >>>>> If possible, I would prefer to integrate it to 3.0 branch so that we
> >>>>> have enough time to validate it and fix bugs.
> >>>>>
> >>>>> Martin
> >>>>>
> >>>>
> >>>> I concur.
> >>>>
> >>>> Honza
> >>>>
> >>>
> >>> Ok. I created a Trac ticket for 3.0 branch to do this improvement.
> >>>
> >>> https://fedorahosted.org/freeipa/ticket/1847
> >>>
> >>> As for 2.1.2 bugfix release, lets do just the "stupid" safe fix.
> >>>
> >>> Martin
> >>>
> >>
> >> Stupid safe fix attached (obviously it's ipa-2-1 only).
> >>
> >> Honza
> >>
> >
> > Works fine. Just one nit - I would suggest adding a comment what this
> > hack does. Its not clear just from the code.
> >
> > Martin
> >
> 
> Added comments.
> 
> Honza
> 

ACK. Pushed to ipa-2-1.

The problem should be solved in a more complex way in ticket 1847.

Martin




More information about the Freeipa-devel mailing list