On Thu, 2012-03-22 at 16:30 +0100, Petr Viktorin wrote: > On 03/21/2012 01:51 PM, Martin Kosek wrote: > > DNS plugin contains several RR type record validators run in > > pre_callback which cannot be used as standard param validator > > as it needs more data and resources that standard validators > > provide. However, the precallback validators are not run for > > DNS records created by new structured options and thus an invalid > > value may slip in. > > > > This patch moves the execution of these precallback validators > > _after_ the processing of structured DNS options. It also cleans > > them up a little and makes them more robust. > > > > https://fedorahosted.org/freeipa/ticket/2550 > > > > Functionally, the patch works fine. > Since you're cleaning up, I have some nitpicks, but ACK if you disagree. > > > Consider if instead of: > > rtype_cb = '_%s_pre_callback' % rtype > if hasattr(self.obj, rtype_cb): > getattr(self.obj, rtype_cb)(ldap, dn, entry_attrs, *keys, **options) > > this wouldn't be more readable: > > rtype_cb = getattr(self.obj, '_%s_pre_callback' % rtype, None) > if rtype_cb: > rtype_cb(ldap, dn, entry_attrs, *keys, **options) > > and since the block appears twice now, make it a method. > > Also, since is_ns_rec_resolvable raises an exception rather than > returning a bool, it should probably be renamed to something like > check_ns_rec_resolvable. >
These are good ideas, please check the attached patch. Martin
>From eabce81ea88b10079e982b356ec4bbac56db4423 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Wed, 21 Mar 2012 13:25:42 +0100 Subject: [PATCH] Fix precallback validators in DNS plugin DNS plugin contains several RR type record validators run in pre_callback which cannot be used as standard param validator as it needs more data and resources that standard validators provide. However, the precallback validators are not run for DNS records created by new structured options and thus an invalid value may slip in. This patch moves the execution of these precallback validators _after_ the processing of structured DNS options. It also cleans them up a little and makes them more robust. https://fedorahosted.org/freeipa/ticket/2550 --- ipalib/plugins/dns.py | 60 ++++++++++++++++++---------------- tests/test_xmlrpc/test_dns_plugin.py | 27 +++++++++++++++ 2 files changed, 59 insertions(+), 28 deletions(-) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index a1d495449d9ac616447c4b280b3efe63b13af8b8..7b22f2f7b09c9cd501e8ce1ad98bd9c21445dc4e 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -1481,7 +1481,7 @@ def zone_is_reverse(zone_name): return False -def is_ns_rec_resolvable(name): +def check_ns_rec_resolvable(name): try: return api.Command['dns_resolve'](name) except errors.NotFound: @@ -1708,7 +1708,7 @@ class dnszone_add(LDAPCreate): error=unicode(_("Nameserver address is not a fully qualified domain name"))) if not 'ip_address' in options and not options['force']: - is_ns_rec_resolvable(nameserver) + check_ns_rec_resolvable(nameserver) if nameserver[-1] != '.': nameserver += '.' @@ -1877,17 +1877,20 @@ class dnsrecord(LDAPObject): ) def _nsrecord_pre_callback(self, ldap, dn, entry_attrs, *keys, **options): - if options.get('force', False): - return dn - - for ns in options['nsrecord']: - is_ns_rec_resolvable(ns) - return dn + nsrecords = entry_attrs.get('nsrecord') + if options.get('force', False) or nsrecords is None: + return + map(check_ns_rec_resolvable, nsrecords) def _ptrrecord_pre_callback(self, ldap, dn, entry_attrs, *keys, **options): - components = dn.split(',',2) - addr = components[0].split('=')[1] - zone = components[1].split('=')[1] + ptrrecords = entry_attrs.get('ptrrecord') + if ptrrecords is None: + return + zone = keys[-2] + if self.is_pkey_zone_record(*keys): + addr = u'' + else: + addr = keys[-1] zone_len = 0 for valid_zone in _valid_reverse_zones: if zone.find(valid_zone) != -1: @@ -1897,16 +1900,23 @@ class dnsrecord(LDAPObject): if not zone_len: allowed_zones = ', '.join(_valid_reverse_zones) - raise errors.ValidationError(name='cn', + raise errors.ValidationError(name='ptrrecord', error=unicode(_('Reverse zone for PTR record should be a sub-zone of one the following fully qualified domains: %s') % allowed_zones)) - ip_addr_comp_count = len(addr.split('.')) + len(zone.split('.')) + addr_len = len(addr.split('.')) if addr else 0 + ip_addr_comp_count = addr_len + len(zone.split('.')) if ip_addr_comp_count != zone_len: - raise errors.ValidationError(name='cn', + raise errors.ValidationError(name='ptrrecord', error=unicode(_('Reverse zone %(name)s requires exactly %(count)d IP address components, %(user_count)d given') % dict(name=zone_name, count=zone_len, user_count=ip_addr_comp_count))) - return dn + def run_precallback_validators(self, dn, entry_attrs, *keys, **options): + ldap = self.api.Backend.ldap2 + + for rtype in entry_attrs: + rtype_cb = getattr(self, '_%s_pre_callback' % rtype, None) + if rtype_cb: + rtype_cb(ldap, dn, entry_attrs, *keys, **options) def is_pkey_zone_record(self, *keys): idnsname = keys[-1] @@ -2120,11 +2130,6 @@ class dnsrecord_add(LDAPCreate): kw.update(user_options) def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options): - for rtype in options: - rtype_cb = '_%s_pre_callback' % rtype - if hasattr(self.obj, rtype_cb): - dn = getattr(self.obj, rtype_cb)(ldap, dn, entry_attrs, *keys, **options) - precallback_attrs = [] for option in options: try: @@ -2153,6 +2158,9 @@ class dnsrecord_add(LDAPCreate): # extra option is passed, run per-type pre_callback for given RR type precallback_attrs.append(rrparam.name) + # Run pre_callback validators + self.obj.run_precallback_validators(dn, entry_attrs, *keys, **options) + # run precallback also for all new RR type attributes in entry_attrs for attr in entry_attrs: try: @@ -2232,14 +2240,7 @@ class dnsrecord_mod(LDAPUpdate): self.obj.has_cli_options(options, self.no_option_msg, True) return super(dnsrecord_mod, self).args_options_2_entry(*keys, **options) - def pre_callback(self, ldap, dn, entry_attrs, *keys, **options): - for rtype in options: - rtype_cb = '_%s_pre_callback' % rtype - if options[rtype] is None and rtype in _record_attributes: - options[rtype] = [] - if hasattr(self.obj, rtype_cb): - dn = getattr(self.obj, rtype_cb)(ldap, dn, entry_attrs, *keys, **options) - + def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options): # check if any attr should be updated using structured instead of replaced # format is recordname : (old_value, new_parts) updated_attrs = {} @@ -2264,6 +2265,9 @@ class dnsrecord_mod(LDAPUpdate): updated_attrs[attr] = (old_value, parts) + # Run pre_callback validators + self.obj.run_precallback_validators(dn, entry_attrs, *keys, **options) + if len(updated_attrs): try: (dn_, old_entry) = ldap.get_entry( diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py index ec6565197880a2e2dafa07315cd1c715c120cfcd..93093ec8ab0441ae1d28bf2b9b05e724d14ad046 100644 --- a/tests/test_xmlrpc/test_dns_plugin.py +++ b/tests/test_xmlrpc/test_dns_plugin.py @@ -744,6 +744,33 @@ class test_dns(Declarative): ), dict( + desc='Try to add unresolvable NS record to %r using dnsrecord_add' % (dnsres1), + command=('dnsrecord_add', [dnszone1, dnsres1], {'nsrecord': u'does.not.exist'}), + expected=errors.NotFound(reason=u"Nameserver 'does.not.exist' does not have a corresponding A/AAAA record"), + ), + + dict( + desc='Add unresolvable NS record with --force to %r using dnsrecord_add' % (dnsres1), + command=('dnsrecord_add', [dnszone1, dnsres1], {'nsrecord': u'does.not.exist', + 'force' : True}), + expected={ + 'value': dnsres1, + 'summary': None, + 'result': { + 'objectclass': [u'top', u'idnsrecord'], + 'dn': unicode(dnsres1_dn), + 'idnsname': [dnsres1], + 'arecord': [u'10.10.0.1'], + 'cnamerecord': [u'foo-1.example.com.'], + 'kxrecord': [u'1 foo-1'], + 'txtrecord': [u'foo bar'], + 'nsecrecord': [dnszone1 + u' TXT A'], + 'nsrecord': [u'does.not.exist'], + }, + }, + ), + + dict( desc='Delete record %r in zone %r' % (dnsres1, dnszone1), command=('dnsrecord_del', [dnszone1, dnsres1], {'del_all': True }), expected={ -- 1.7.7.6
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
