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

--
Jan Cholasta
>From 82738f3f595328e7712b60fc0d4b5af03571f311 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Mon, 26 Sep 2011 11:00:32 +0200
Subject: [PATCH] Validate name_from_ip parameter of dnszone.

ticket 1627
---
 ipalib/plugins/dns.py |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index d922cdf..c815c9c 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -167,7 +167,7 @@ def _validate_ipaddr(ugettext, ipaddr):
 def _validate_ipnet(ugettext, ipnet):
     try:
         net = netaddr.IPNetwork(ipnet)
-    except (UnboundLocalError, ValueError):
+    except (netaddr.AddrFormatError, ValueError, UnboundLocalError):
         return u'invalid format'
     return None
 
@@ -417,6 +417,11 @@ class dnszone_add(LDAPCreate):
         ),
     )
 
+    def args_options_2_params(self, *args, **options):
+        if 'name_from_ip' in options:
+            self.obj.params['name_from_ip'](unicode(options['name_from_ip']))
+        return super(dnszone_add, self).args_options_2_params(*args, **options)
+
     def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
         if not dns_container_exists(self.api.Backend.ldap2):
             raise errors.NotFound(reason=_('DNS is not configured'))
@@ -469,6 +474,11 @@ api.register(dnszone_del)
 class dnszone_mod(LDAPUpdate):
     __doc__ = _('Modify DNS zone (SOA record).')
 
+    def args_options_2_params(self, *args, **options):
+        if 'name_from_ip' in options:
+            self.obj.params['name_from_ip'](unicode(options['name_from_ip']))
+        return super(dnszone_mod, self).args_options_2_params(*args, **options)
+
     def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
         if 'name_from_ip' in entry_attrs:
             del entry_attrs['name_from_ip']
@@ -483,6 +493,11 @@ api.register(dnszone_mod)
 class dnszone_find(LDAPSearch):
     __doc__ = _('Search for DNS zones (SOA records).')
 
+    def args_options_2_params(self, *args, **options):
+        if 'name_from_ip' in options:
+            self.obj.params['name_from_ip'](unicode(options['name_from_ip']))
+        return super(dnszone_find, self).args_options_2_params(*args, **options)
+
     def args_options_2_entry(self, *args, **options):
         if 'name_from_ip' in options:
             if 'idnsname' not in options:
-- 
1.7.6.2

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to