[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Freeipa-devel] [PATCH] 940 apply some validation to some classes only



Jan Cholasta wrote:
On 20.2.2012 22:56, Rob Crittenden wrote:
Rob Crittenden wrote:
The variable name rdnattr can be misleading. It is only used to give the
name of hte RDN in something that can be renamed. Compare this to
something like netgroups where the DN has no visible relationship to the
content of the object (ipaUniqueId). Only those objects that define
rdnattr get a rename option (look at netgroups, for example).

rdnattr is always the primary key but not always defined. It should
probably be a boolean instead, rdn_is_primary_key or something a bit
more obvious. I can make that change here.

rob

Updated patch. It seems I broke query a few months ago trying to enforce
no white spaces in some names.

I did the rdnattr variable rename. Looking back at the changelog this
was meant to always match the primary key so lets remove the possibility
that it doesn't. By doing it this way we get the pattern for free.

rob

This is certainly better, but I still have a few concerns:

1. --rename is tied to the RDN change operation. I think this is wrong.
--rename should be available for all objects, not only when the object's
RDN attribute and primary key attribute are the same (so that we can
change the primary key of any object). Similarly, modrdn should be
triggered for all objects every time the RDN attribute changes, not only
when the object's RDN attribute and primary key attribute are the same
(so the DN is properly updated for all objects).

I don't know if this is in the scope of this patch, though.

Right, not in this scope. An entry where RDN != primary key doesn't need --rename to do a rename, just a mod on whatever its "key" is. We can fake this to have a consistent interface though. Please open a ticket.


2. In crud.Create/crud.Update, query is set for params which have the
ask_create/ask_update flag set. This is an error, as we are obviously
not querying LDAP with these params (it seems someone forgot to remove
the query=True keyword argument after copy-pasting from crud.Search).

Honza


Updated

rob

>From 81e5502265dab83d5cafc59aa1c80f68c1c1f35e Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcritten redhat com>
Date: Wed, 29 Feb 2012 13:31:20 -0500
Subject: [PATCH] Only apply validation rules when adding and updating.

There may be cases, for whatever reason, that an otherwise illegal
entry gets created that doesn't match the criteria for a valid
user/host/group name. If this happens (i.e. migration) there is no way
to remove this using the IPA tools because we always applied the name
pattern. So you can't, for example, delete a user with an illegal name.

Primary keys are cloned with query=True in PKQuery which causes no
rules to be applied on mod/show/find. This reverts a change from commit
3a5e26a0 which applies class rules when query=True (for enforcing no
white space).

Replace rdnattr with rdn_is_primary_key. This was meant to tell us when
an RDN change was necessary to do a rename. There could be a disconnect
where the rdnattr wasn't the primary key and in that case we don't
need to do an RDN change, so use a boolean instead so that it is
clear that RDN == primary key.

Add a test to ensure that nowhitespace is actually enforced.

https://fedorahosted.org/freeipa/ticket/2115

Related: https://fedorahosted.org/freeipa/ticket/2089

Whitespace tickets:
https://fedorahosted.org/freeipa/ticket/1285
https://fedorahosted.org/freeipa/ticket/1286
https://fedorahosted.org/freeipa/ticket/1287
---
 API.txt                                |   24 +++++++++---------
 ipalib/crud.py                         |    6 +++-
 ipalib/parameters.py                   |    3 +-
 ipalib/plugins/automount.py            |    2 +-
 ipalib/plugins/baseldap.py             |   21 ++++++++-------
 ipalib/plugins/group.py                |    2 +-
 ipalib/plugins/permission.py           |    2 +-
 ipalib/plugins/privilege.py            |    2 +-
 ipalib/plugins/role.py                 |    2 +-
 ipalib/plugins/user.py                 |    2 +-
 tests/test_xmlrpc/test_group_plugin.py |   22 ++++++++++++++++
 tests/test_xmlrpc/test_host_plugin.py  |   42 ++++++++++++++++++++++++++++++++
 tests/test_xmlrpc/test_role_plugin.py  |   10 +++++++
 tests/test_xmlrpc/test_user_plugin.py  |   36 +++++++++++++++++++++++++++
 14 files changed, 145 insertions(+), 31 deletions(-)

diff --git a/API.txt b/API.txt
index b5f5774b58449dd5496d85117fa6f970ea34662d..dadd9c11d56d7d1e493a18d9d5218557b9d6099b 100644
--- a/API.txt
+++ b/API.txt
@@ -2036,12 +2036,12 @@ command: permission_add
 args: 1,12,3
 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
 option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=True)
-option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=True, required=False)
-option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=True, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
-option: Str('memberof', alwaysask=True, attribute=True, autofill=False, cli_name='memberof', multivalue=False, query=True, required=False)
-option: Str('filter', alwaysask=True, attribute=True, autofill=False, cli_name='filter', multivalue=False, query=True, required=False)
-option: Str('subtree', alwaysask=True, attribute=True, autofill=False, cli_name='subtree', multivalue=False, query=True, required=False)
-option: Str('targetgroup', alwaysask=True, attribute=True, autofill=False, cli_name='targetgroup', multivalue=False, query=True, required=False)
+option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False)
+option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
+option: Str('memberof', alwaysask=True, attribute=True, autofill=False, cli_name='memberof', multivalue=False, query=False, required=False)
+option: Str('filter', alwaysask=True, attribute=True, autofill=False, cli_name='filter', multivalue=False, query=False, required=False)
+option: Str('subtree', alwaysask=True, attribute=True, autofill=False, cli_name='subtree', multivalue=False, query=False, required=False)
+option: Str('targetgroup', alwaysask=True, attribute=True, autofill=False, cli_name='targetgroup', multivalue=False, query=False, required=False)
 option: Str('setattr*', cli_name='setattr', exclude='webui')
 option: Str('addattr*', cli_name='addattr', exclude='webui')
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
@@ -2092,12 +2092,12 @@ command: permission_mod
 args: 1,15,3
 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
 option: Str('permissions', attribute=True, autofill=False, cli_name='permissions', csv=True, multivalue=True, required=False)
-option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=True, required=False)
-option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=True, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
-option: Str('memberof', alwaysask=True, attribute=True, autofill=False, cli_name='memberof', multivalue=False, query=True, required=False)
-option: Str('filter', alwaysask=True, attribute=True, autofill=False, cli_name='filter', multivalue=False, query=True, required=False)
-option: Str('subtree', alwaysask=True, attribute=True, autofill=False, cli_name='subtree', multivalue=False, query=True, required=False)
-option: Str('targetgroup', alwaysask=True, attribute=True, autofill=False, cli_name='targetgroup', multivalue=False, query=True, required=False)
+option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False)
+option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
+option: Str('memberof', alwaysask=True, attribute=True, autofill=False, cli_name='memberof', multivalue=False, query=False, required=False)
+option: Str('filter', alwaysask=True, attribute=True, autofill=False, cli_name='filter', multivalue=False, query=False, required=False)
+option: Str('subtree', alwaysask=True, attribute=True, autofill=False, cli_name='subtree', multivalue=False, query=False, required=False)
+option: Str('targetgroup', alwaysask=True, attribute=True, autofill=False, cli_name='targetgroup', multivalue=False, query=False, required=False)
 option: Str('setattr*', cli_name='setattr', exclude='webui')
 option: Str('addattr*', cli_name='addattr', exclude='webui')
 option: Str('delattr*', cli_name='delattr', exclude='webui')
diff --git a/ipalib/crud.py b/ipalib/crud.py
index 833914cfaa2dd4e99afae8065651435b0bc0f898..b9dfb025e5f68c57565b485053e8aef406f18dc1 100644
--- a/ipalib/crud.py
+++ b/ipalib/crud.py
@@ -144,7 +144,7 @@ class Create(Method):
                 continue
             if 'ask_create' in option.flags:
                 yield option.clone(
-                    attribute=attribute, query=True, required=False,
+                    attribute=attribute, query=False, required=False,
                     autofill=False, alwaysask=True
                 )
             else:
@@ -161,6 +161,8 @@ class PKQuery(Method):
 
     def get_args(self):
         if self.obj.primary_key:
+            # Don't enforce rules on the primary key so we can reference
+            # any stored entry, legal or not
             yield self.obj.primary_key.clone(attribute=True, query=True)
 
 
@@ -189,7 +191,7 @@ class Update(PKQuery):
                 continue
             if 'ask_update' in option.flags:
                 yield option.clone(
-                    attribute=attribute, query=True, required=False,
+                    attribute=attribute, query=False, required=False,
                     autofill=False, alwaysask=True
                 )
             elif 'req_update' in option.flags:
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index c533f9d0b70441003deb3fa399b6c9e204b197a6..b1525b4d59e8c95125f424be94917bcb2bd9285e 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -508,7 +508,8 @@ class Param(ReadOnly):
         self.class_rules = tuple(class_rules)
         self.rules = rules
         if self.query:
-            self.all_rules = self.class_rules
+            # by definition a query enforces no class or parameter rules
+            self.all_rules = ()
         else:
             self.all_rules = self.class_rules + self.rules
         for rule in self.all_rules:
diff --git a/ipalib/plugins/automount.py b/ipalib/plugins/automount.py
index 8d743d75b7176a1c3b5d299abff6014c324e5d6b..31c143d8418a9101a14e39ab752bb229e4219094 100644
--- a/ipalib/plugins/automount.py
+++ b/ipalib/plugins/automount.py
@@ -645,7 +645,7 @@ class automountkey(LDAPObject):
     default_attributes = [
         'automountkey', 'automountinformation', 'description'
     ]
-    rdnattr = 'description'
+    rdn_is_primary_key = True
     rdn_separator = ' '
 
     takes_params = (
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 55d93eb28e3d9e41ca43a0cd04755d44c9f7240a..6f51b373ad9b6acc081adf65eda2b6d35b478b1b 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -429,7 +429,7 @@ class LDAPObject(Object):
     rdn_attribute = ''
     uuid_attribute = ''
     attribute_members = {}
-    rdnattr = None
+    rdn_is_primary_key = False # Do we need RDN change to do a rename?
     password_attributes = []
     # Can bind as this entry (has userPassword or krbPrincipalKey)
     bindable = False
@@ -1183,7 +1183,7 @@ class LDAPUpdate(LDAPQuery, crud.Update):
     has_output_params = global_output_params
 
     def _get_rename_option(self):
-        rdnparam = getattr(self.obj.params, self.obj.rdnattr)
+        rdnparam = getattr(self.obj.params, self.obj.primary_key.name)
         return rdnparam.clone_rename('rename',
             cli_name='rename', required=False, label=_('Rename'),
             doc=_('Rename the %(ldap_obj_name)s object') % dict(
@@ -1194,7 +1194,7 @@ class LDAPUpdate(LDAPQuery, crud.Update):
     def get_options(self):
         for option in super(LDAPUpdate, self).get_options():
             yield option
-        if self.obj.rdnattr:
+        if self.obj.rdn_is_primary_key:
             yield self._get_rename_option()
 
     def execute(self, *keys, **options):
@@ -1234,18 +1234,19 @@ class LDAPUpdate(LDAPQuery, crud.Update):
 
         rdnupdate = False
         try:
-            if self.obj.rdnattr and 'rename' in options:
+            if self.obj.rdn_is_primary_key and 'rename' in options:
                 if not options['rename']:
                     raise errors.ValidationError(name='rename', error=u'can\'t be empty')
-                entry_attrs[self.obj.rdnattr] = options['rename']
+                entry_attrs[self.obj.primary_key.name] = options['rename']
 
-            if self.obj.rdnattr and self.obj.rdnattr in entry_attrs:
+            if self.obj.rdn_is_primary_key and self.obj.primary_key.name in entry_attrs:
                 # RDN change
-                ldap.update_entry_rdn(dn, unicode('%s=%s' % (self.obj.rdnattr,
-                    entry_attrs[self.obj.rdnattr])))
-                rdnkeys = keys[:-1] + (entry_attrs[self.obj.rdnattr], )
+                ldap.update_entry_rdn(dn,
+                    unicode('%s=%s' % (self.obj.primary_key.name,
+                    entry_attrs[self.obj.primary_key.name])))
+                rdnkeys = keys[:-1] + (entry_attrs[self.obj.primary_key.name], )
                 dn = self.obj.get_dn(*rdnkeys)
-                del entry_attrs[self.obj.rdnattr]
+                del entry_attrs[self.obj.primary_key.name]
                 options['rdnupdate'] = True
                 rdnupdate = True
 
diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py
index b101d128565275ece89b0603305aeab04ff274df..096cb9eaea0e501afebc2bd0bf3cc914b17870f5 100644
--- a/ipalib/plugins/group.py
+++ b/ipalib/plugins/group.py
@@ -95,7 +95,7 @@ class group(LDAPObject):
         'memberofindirect': ['group', 'netgroup', 'role', 'hbacrule',
         'sudorule'],
     }
-    rdnattr = 'cn'
+    rdn_is_primary_key = True
 
     label = _('User Groups')
     label_singular = _('User Group')
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index c9fd5649f338b5c92b86e471fb817b9d964084d3..ce2536d9921ede73d2c26468f5d99609552e1881 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -144,7 +144,7 @@ class permission(LDAPObject):
     attribute_members = {
         'member': ['privilege'],
     }
-    rdnattr='cn'
+    rdn_is_primary_key = True
 
     label = _('Permissions')
     label_singular = _('Permission')
diff --git a/ipalib/plugins/privilege.py b/ipalib/plugins/privilege.py
index 53f76512ed14a9f346ca2704e5d4071c66a9bac9..53e1de223a2a8018b942255b9911488f762cb338 100644
--- a/ipalib/plugins/privilege.py
+++ b/ipalib/plugins/privilege.py
@@ -60,7 +60,7 @@ class privilege(LDAPObject):
     reverse_members = {
         'member': ['permission'],
     }
-    rdnattr='cn'
+    rdn_is_primary_key = True
 
     label = _('Privileges')
     label_singular = _('Privilege')
diff --git a/ipalib/plugins/role.py b/ipalib/plugins/role.py
index ee6ebcdc00f29ebd0595aeaaa2ee65aefc0ce3d0..2837c418bf8a35ce31ba1f0cb69505c064a7c72b 100644
--- a/ipalib/plugins/role.py
+++ b/ipalib/plugins/role.py
@@ -76,7 +76,7 @@ class role(LDAPObject):
     reverse_members = {
         'member': ['privilege'],
     }
-    rdnattr='cn'
+    rdn_is_primary_key = True
 
     label = _('Roles')
     label_singular = _('Role')
diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index d8da3a373ba613ccdeecc7e450aa4fb979bc73af..591132d36b4de82bc935b60f928e36be69ea40ee 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -168,7 +168,7 @@ class user(LDAPObject):
         'memberof': ['group', 'netgroup', 'role', 'hbacrule', 'sudorule'],
         'memberofindirect': ['group', 'netgroup', 'role', 'hbacrule', 'sudorule'],
     }
-    rdnattr = 'uid'
+    rdn_is_primary_key = True
     bindable = True
     password_attributes = [('userpassword', 'has_password'),
                            ('krbprincipalkey', 'has_keytab')]
diff --git a/tests/test_xmlrpc/test_group_plugin.py b/tests/test_xmlrpc/test_group_plugin.py
index 86c0d90daebd5917ea51d124cb9946aa3607d2db..3ef60f693815dd9a6a00e647394fa3ed4352ebb2 100644
--- a/tests/test_xmlrpc/test_group_plugin.py
+++ b/tests/test_xmlrpc/test_group_plugin.py
@@ -602,6 +602,28 @@ class test_group(Declarative):
             expected=errors.ValidationError(name='cn', error='may only include letters, numbers, _, -, . and $'),
         ),
 
+        # The assumption on these next 4 tests is that if we don't get a
+        # validation error then the request was processed normally.
+        dict(
+            desc='Test that validation is disabled on mods',
+            command=('group_mod', [invalidgroup1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
+
+        dict(
+            desc='Test that validation is disabled on deletes',
+            command=('group_del', [invalidgroup1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
+
+        dict(
+            desc='Test that validation is disabled on show',
+            command=('group_show', [invalidgroup1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
 
         ##### managed entry tests
         dict(
diff --git a/tests/test_xmlrpc/test_host_plugin.py b/tests/test_xmlrpc/test_host_plugin.py
index 0323ac39a812e4e69344e8ebf3cec50a10838efb..4f24b6e3905705aa0403eb9fe4de09e164dd3555 100644
--- a/tests/test_xmlrpc/test_host_plugin.py
+++ b/tests/test_xmlrpc/test_host_plugin.py
@@ -47,6 +47,7 @@ dn3 = DN(('fqdn',fqdn3),('cn','computers'),('cn','accounts'),
 fqdn4 = u'testhost2.lab.%s' % api.env.domain
 dn4 = DN(('fqdn',fqdn4),('cn','computers'),('cn','accounts'),
          api.env.basedn)
+invalidfqdn1 = u'foo_bar.lab.%s' % api.env.domain
 
 # We can use the same cert we generated for the service tests
 fd = open('tests/test_xmlrpc/service.crt', 'r')
@@ -670,4 +671,45 @@ class test_host(Declarative):
             expected=errors.ValidationError(name='fqdn', error='An IPA master host cannot be deleted'),
         ),
 
+        dict(
+            desc='Test that validation is enabled on adds',
+            command=('host_add', [invalidfqdn1], {}),
+            expected=errors.ValidationError(name='fqdn', error='may only include letters, numbers, and -'),
+        ),
+
+
+        # The assumption on these next 4 tests is that if we don't get a
+        # validation error then the request was processed normally.
+        dict(
+            desc='Test that validation is disabled on mods',
+            command=('host_mod', [invalidfqdn1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
+
+        dict(
+            desc='Test that validation is disabled on deletes',
+            command=('host_del', [invalidfqdn1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
+
+        dict(
+            desc='Test that validation is disabled on show',
+            command=('host_show', [invalidfqdn1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
+
+        dict(
+            desc='Test that validation is disabled on find',
+            command=('host_find', [invalidfqdn1], {}),
+            expected=dict(
+                count=0,
+                truncated=False,
+                summary=u'0 hosts matched',
+                result=[],
+            ),
+        ),
+
     ]
diff --git a/tests/test_xmlrpc/test_role_plugin.py b/tests/test_xmlrpc/test_role_plugin.py
index 9c405cff741fd9c64dce146b8b5a0ee68af3cccc..f871e2683a993b4bd65223fb0ae1c20af977e047 100644
--- a/tests/test_xmlrpc/test_role_plugin.py
+++ b/tests/test_xmlrpc/test_role_plugin.py
@@ -33,6 +33,7 @@ role1 = u'test-role-1'
 role1_dn = DN(('cn',role1),api.env.container_rolegroup,
               api.env.basedn)
 renamedrole1 = u'test-role'
+invalidrole1 = u' whitespace '
 
 role2 = u'test-role-2'
 role2_dn = DN(('cn',role2),api.env.container_rolegroup,
@@ -101,6 +102,15 @@ class test_role(Declarative):
 
 
         dict(
+            desc='Create invalid %r' % invalidrole1,
+            command=('role_add', [invalidrole1],
+                dict(description=u'role desc 1')
+            ),
+            expected=errors.ValidationError(name='cn', error='Leading and trailing spaces are not allowed'),
+        ),
+
+
+        dict(
             desc='Create %r' % role1,
             command=('role_add', [role1],
                 dict(description=u'role desc 1')
diff --git a/tests/test_xmlrpc/test_user_plugin.py b/tests/test_xmlrpc/test_user_plugin.py
index d1cdb2f98bd05b3f0a6e00cc59d7fd5ad25ca981..b21737f89b6d06770ef12b97b11570283113038e 100644
--- a/tests/test_xmlrpc/test_user_plugin.py
+++ b/tests/test_xmlrpc/test_user_plugin.py
@@ -643,6 +643,42 @@ class test_user(Declarative):
             expected=errors.ValidationError(name='uid', error='can be at most 33 characters'),
         ),
 
+
+        # The assumption on these next 4 tests is that if we don't get a
+        # validation error then the request was processed normally.
+        dict(
+            desc='Test that validation is disabled on deletes',
+            command=('user_del', [invaliduser1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
+
+        dict(
+            desc='Test that validation is disabled on show',
+            command=('user_show', [invaliduser1], {}),
+            expected=errors.NotFound(reason='no such entry'),
+        ),
+
+
+        dict(
+            desc='Test that validation is disabled on find',
+            command=('user_find', [invaliduser1], {}),
+            expected=dict(
+                count=0,
+                truncated=False,
+                summary=u'0 users matched',
+                result=[],
+            ),
+        ),
+
+
+        dict(
+            desc='Try to rename to invalid username %r' % user1,
+            command=('user_mod', [user1], dict(rename=invaliduser1)),
+            expected=errors.ValidationError(name='uid', error='may only include letters, numbers, _, -, . and $'),
+        ),
+
+
         dict(
             desc='Create %r' % group1,
             command=(
-- 
1.7.6


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]