Re: [Freeipa-devel] [PATCH] 44 Fix parameter validation
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 44 Fix parameter validation
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 44 Fix parameter validation
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
--
Jan Cholasta
From 3b7cd2181857defa378e5536c3de20d7e8b0232a Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Thu, 25 Aug 2011 17:57:50 +0200
Subject: [PATCH] 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.
ticket 1627
---
API.txt |6 +++---
VERSION |2 +-
ipalib/cli.py |1 +
ipalib/crud.py|7 ---
ipalib/frontend.py| 35 ++-
ipalib/plugins/dns.py |5 ++---
6 files changed, 37 insertions(+), 19 deletions(-)
diff --git a/API.txt b/API.txt
index 24f7634..8a5a05d 100644
--- a/API.txt
+++ b/API.txt
@@ -739,7 +739,7 @@ output: Output('value', type 'unicode', The primary_key value of the entry, e
command: dnszone_add
args: 1,19,3
arg: Str('idnsname', attribute=True, cli_name='name', default_from=DefaultFrom(lambda, 'name_from_ip'), label=Gettext('Zone name', domain='ipa', localedir=None), multivalue=False, normalizer=lambda, primary_key=True, required=True)
-option: Str('name_from_ip', _validate_ipnet, attribute=True, cli_name='name_from_ip', label=Gettext('Reverse zone IP network', domain='ipa', localedir=None), multivalue=False, required=False)
+option: Str('name_from_ip', _validate_ipnet, attribute=True, cli_name='name_from_ip', flags=['validate_search'], label=Gettext('Reverse zone IP network', domain='ipa', localedir=None), multivalue=False, required=False)
option: Str('idnssoamname', attribute=True, cli_name='name_server', label=Gettext('Authoritative nameserver', domain='ipa', localedir=None), multivalue=False, required=True)
option: Str('idnssoarname', attribute=True, cli_name='admin_email', default_from=DefaultFrom(lambda, 'idnsname'), label=Gettext('Administrator e-mail address', domain='ipa', localedir=None), multivalue=False, normalizer=_rname_normalizer, required=True)
option: Int('idnssoaserial', attribute=True, autofill=True, cli_name='serial', create_default=_create_zone_serial, label=Gettext('SOA serial', domain='ipa', localedir=None), minvalue=1, multivalue=False, required=False)
@@ -784,7 +784,7 @@ command: dnszone_find
args: 1,20,4
arg: Str('criteria?', noextrawhitespace=False)
option: Str('idnsname', attribute=True, autofill=False, cli_name='name', default_from=DefaultFrom(lambda, 'name_from_ip'), label=Gettext('Zone name', domain='ipa', localedir=None), multivalue=False, normalizer=lambda, primary_key=True, query=True, required=False)
-option: Str('name_from_ip', _validate_ipnet, attribute=True, autofill=False, cli_name='name_from_ip', label=Gettext('Reverse zone IP network', domain='ipa', localedir=None), multivalue=False, query=True, required=False)
+option: Str('name_from_ip', _validate_ipnet, attribute=True, autofill=False, cli_name='name_from_ip', flags=['validate_search'], label=Gettext('Reverse zone IP network', domain='ipa', localedir=None), multivalue=False, query=False, required=False)
option: Str('idnssoamname', attribute=True, autofill=False, cli_name='name_server', label=Gettext('Authoritative nameserver', domain='ipa', localedir=None), multivalue=False, query=True, required=False)
option: Str('idnssoarname', attribute=True, autofill=False, cli_name='admin_email', default_from=DefaultFrom(lambda, 'idnsname'), label=Gettext('Administrator e-mail address', domain='ipa', localedir=None), multivalue=False, normalizer=_rname_normalizer, query=True, required=False)
option: Int('idnssoaserial', attribute=True, autofill=False, cli_name='serial', create_default=_create_zone_serial, label=Gettext('SOA serial', domain='ipa', localedir=None), minvalue=1, multivalue=False, query=True, required=False)
@@ -810,7 +810,7 @@ output: Output('truncated', type 'bool', 'True if not all results were returne
command: dnszone_mod
args: 1,18,3
arg: Str('idnsname', attribute=True, cli_name='name', default_from=DefaultFrom(lambda, 'name_from_ip'), label=Gettext('Zone name', domain='ipa', localedir=None), multivalue=False, normalizer=lambda, primary_key=True, query=True, required=True)
-option: Str('name_from_ip', _validate_ipnet, attribute=True, autofill=False, cli_name='name_from_ip', label=Gettext('Reverse zone IP network', domain='ipa', localedir=None), multivalue=False, required=False)
+option: Str('name_from_ip', _validate_ipnet, attribute=True, autofill=False, cli_name='name_from_ip', flags=['validate_search'], label=Gettext('Reverse zone IP network', domain='ipa', localedir=None), multivalue=False, required=False)
option: Str('idnssoamname',
