Revised patch attached. We'll leave the DN parameter changes till later. This is essentially the same as the original patch with the addition of the fixes necessary to support passing an empty container arg, an issue Martin discovered in his review. FWIW the answer was not to make the param required (actually it would have been adding the flag 'nonempty') because you should be able to say you don't want to introduce a container into the search bases (see commit comment)

--
John Dennis <jden...@redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
>From 29823d7b1c02bb67b097882dc4e002f1a83cb765 Mon Sep 17 00:00:00 2001
From: John Dennis <jden...@redhat.com>
Date: Wed, 11 Apr 2012 21:51:17 -0400
Subject: [PATCH 72-1] Validate DN & RDN parameters for migrate command
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

Ticket #2555

We were generating a traceback (server error) if a malformed RDN was
passed as a parameter to the migrate command.

* add parameter validation functions validate_dn_param() and
  validate_rdn_param() to ipalib.util. Those functions simply invoke
  the DN or RDN constructor from our dn module passing it the string
  representation. If the constructor does not throw an error it's
  valid.

* Add the parameter validation function pointers to the Param objects
  in the migrate command.

* Fix _get_search_bases() so if a container dn is empty it it just
  uses the base dn alone instead of faulting.

* Remove the default values for the usercontainer and
  groupcontainer. It's unlikely the defaults would be correct and by
  having a default value it prevents the argument from being empty. An
  empty container is valuable if you want the search to start from the
  base, one shouldn't be forced to always introduce a container, it
  should be possible to say the container shouldn't be used (i.e. it's
  empty).

* Update the doc for usercontainer and groupcontainer to reflect the
  fact they are DN's not RDN's. A RDN can only be one level and it
  should be possible to have a container more than one RDN removed
  from the base.
---
 ipalib/plugins/migration.py |   24 ++++++++++++------------
 ipalib/util.py              |   16 ++++++++++++++++
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index 873ff4c..16c726c 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -23,6 +23,7 @@ import ldap as _ldap
 from ipalib import api, errors, output
 from ipalib import Command, Password, Str, Flag, StrEnum
 from ipalib.cli import to_cli
+from ipalib.util import validate_dn_param
 from ipalib.dn import *
 from ipalib.plugins.user import NO_UPG_MAGIC
 if api.env.in_server and api.env.context in ['lite', 'server']:
@@ -418,25 +419,21 @@ class migrate_ds(Command):
     )
 
     takes_options = (
-        Str('binddn?',
+        Str('binddn?', validate_dn_param,
             cli_name='bind_dn',
             label=_('Bind DN'),
             default=u'cn=directory manager',
             autofill=True,
         ),
-        Str('usercontainer?',
+        Str('usercontainer?', validate_dn_param,
             cli_name='user_container',
             label=_('User container'),
-            doc=_('RDN of container for users in DS relative to base DN'),
-            default=u'ou=people',
-            autofill=True,
+            doc=_('DN of container for users in DS relative to base DN'),
         ),
-        Str('groupcontainer?',
+        Str('groupcontainer?', validate_dn_param,
             cli_name='group_container',
             label=_('Group container'),
-            doc=_('RDN of container for groups in DS relative to base DN'),
-            default=u'ou=groups',
-            autofill=True,
+            doc=_('DN of container for groups in DS relative to base DN'),
         ),
         Str('userobjectclass*',
             cli_name='user_objectclass',
@@ -589,9 +586,12 @@ can use their Kerberos accounts.''')
     def _get_search_bases(self, options, ds_base_dn, migrate_order):
         search_bases = dict()
         for ldap_obj_name in migrate_order:
-            search_bases[ldap_obj_name] = '%s,%s' % (
-                options['%scontainer' % to_cli(ldap_obj_name)], ds_base_dn
-            )
+            container = options.get('%scontainer' % to_cli(ldap_obj_name))
+            if container:
+                search_base = str(DN(container, ds_base_dn))
+            else:
+                search_base = ds_base_dn
+            search_bases[ldap_obj_name] = search_base
         return search_bases
 
     def migrate(self, ldap, config, ds_ldap, ds_base_dn, options):
diff --git a/ipalib/util.py b/ipalib/util.py
index a79f41c..487d278 100644
--- a/ipalib/util.py
+++ b/ipalib/util.py
@@ -31,6 +31,7 @@ from weakref import WeakKeyDictionary
 
 from ipalib import errors
 from ipalib.text import _
+from ipalib.dn import DN, RDN
 from ipapython import dnsclient
 from ipapython.ipautil import decode_ssh_pubkey
 
@@ -484,3 +485,18 @@ def gen_dns_update_policy(realm, rrtypes=('A', 'AAAA', 'SSHFP')):
     policy += ";"
 
     return policy
+
+def validate_rdn_param(ugettext, value):
+    try:
+        rdn = RDN(value)
+    except Exception, e:
+        return str(e)
+    return None
+
+def validate_dn_param(ugettext, value):
+    try:
+        rdn = DN(value)
+    except Exception, e:
+        return str(e)
+    return None
+
-- 
1.7.7.6

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

Reply via email to