Harden raw record processing in DNS plugin

There were cases where DNS plugin was too tolerant in a raw DNS
record option (--<rrtype-rec) processing. It let people specify
DNS record parts options in dnsrecord-mod operations for some
record without specifying the record that should be updated. It
also ignored DNS record parts in dnsrecord-add operation when the
raw DNS record value was already set via --<rrtype>-rec option.

This patch hardens the processing and returns error in both
described cases to make the processes clearer and more robust.

All these use cases were also covered by new unit tests.

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

2 files changed, 64 insertions, 12 deletions

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 7b22f2f7..eef6ab1d 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -2131,6 +2131,7 @@ class dnsrecord_add(LDAPCreate):
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
         precallback_attrs = []
+        processed_attrs = []
         for option in options:
             try:
                 param = self.params[option]
@@ -2142,13 +2143,19 @@ class dnsrecord_add(LDAPCreate):
                 continue
 
             if 'dnsrecord_part' in param.flags:
-                if rrparam.name in entry_attrs:
+                if rrparam.name in processed_attrs:
                     # this record was already entered
                     continue
+                if rrparam.name in entry_attrs:
+                    # this record is entered both via parts and raw records
+                    raise errors.ValidationError(name=param.cli_name or param.name,
+                            error=_('Raw value of a DNS record was already set by "%(name)s" option') \
+                                  % dict(name=rrparam.cli_name or rrparam.name))
 
                 parts = rrparam.get_parts_from_kw(options)
                 dnsvalue = [rrparam._convert_scalar(parts)]
                 entry_attrs[rrparam.name] = dnsvalue
+                processed_attrs.append(rrparam.name)
                 continue
 
             if 'dnsrecord_extra' in param.flags:
@@ -2193,6 +2200,8 @@ class dnsrecord_add(LDAPCreate):
             for attr in old_entry.keys():
                 if attr not in _record_attributes:
                     continue
+                if entry_attrs[attr] is None:
+                    entry_attrs[attr] = []
                 if not isinstance(entry_attrs[attr], (tuple, list)):
                     vals = [entry_attrs[attr]]
                 else:
@@ -2244,26 +2253,23 @@ class dnsrecord_mod(LDAPUpdate):
         # check if any attr should be updated using structured instead of replaced
         # format is recordname : (old_value, new_parts)
         updated_attrs = {}
-        for attr in entry_attrs:
-            param = self.params[attr]
-            if not isinstance(param, DNSRecord):
-                continue
-
+        for param in self.obj.iterate_rrparams_by_parts(options, skip_extra=True):
             parts = param.get_parts_from_kw(options, raise_on_none=False)
 
             if parts is None:
                 # old-style modification
                 continue
 
-            if isinstance(entry_attrs[attr], (tuple, list)):
-                if len(entry_attrs[attr]) > 1:
+            old_value = entry_attrs.get(param.name)
+            if not old_value:
+                raise errors.RequirementError(name=param.name)
+            if isinstance(old_value, (tuple, list)):
+                if len(old_value) > 1:
                     raise errors.ValidationError(name=param.name,
                            error=_('DNS records can be only updated one at a time'))
-                old_value = entry_attrs[attr][0]
-            else:
-                old_value = entry_attrs[attr]
+                old_value = old_value[0]
 
-            updated_attrs[attr] = (old_value, parts)
+            updated_attrs[param.name] = (old_value, parts)
 
         # Run pre_callback validators
         self.obj.run_precallback_validators(dn, entry_attrs, *keys, **options)
diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py
index 93093ec8..1e5ab791 100644
--- a/tests/test_xmlrpc/test_dns_plugin.py
+++ b/tests/test_xmlrpc/test_dns_plugin.py
@@ -622,6 +622,18 @@ class test_dns(Declarative):
         ),
 
         dict(
+            desc='Try to add SRV record to zone %r both via parts and a raw value' % (dnszone1),
+            command=('dnsrecord_add', [dnszone1, u'_foo._tcp'], {'srv_part_priority': 0,
+                                                                 'srv_part_weight' : 0,
+                                                                 'srv_part_port' : 123,
+                                                                 'srv_part_target' : u'foo.bar.',
+                                                                 'srvrecord': [u"1 100 1234 %s" \
+                                                                     % dnszone1_mname]}),
+            expected=errors.ValidationError(name='srv_target',
+                error='Raw value of a DNS record was already set by a_rec option'),
+        ),
+
+        dict(
             desc='Add SRV record to zone %r using dnsrecord_add' % (dnszone1),
             command=('dnsrecord_add', [dnszone1, u'_foo._tcp'], {'srvrecord': u"0 100 1234 %s" % dnszone1_mname}),
             expected={
@@ -637,6 +649,40 @@ class test_dns(Declarative):
         ),
 
         dict(
+            desc='Try to modify SRV record in zone %r without specifying modified value' % (dnszone1),
+            command=('dnsrecord_mod', [dnszone1, u'_foo._tcp'], {'srv_part_priority': 1,}),
+            expected=errors.RequirementError(name='srvrecord'),
+        ),
+
+        dict(
+            desc='Try to modify SRV record in zone %r with non-existent modified value' % (dnszone1),
+            command=('dnsrecord_mod', [dnszone1, u'_foo._tcp'], {'srv_part_priority': 1,
+                                                  'srvrecord' : [u"0 100 1234 does.not.exist."] }),
+            expected=errors.AttrValueNotFound(attr='SRV', value=u'0 100 1234 ns1.dnszone.test.'),
+        ),
+
+        dict(
+            desc='Try to modify SRV record in zone %r with invalid part value' % (dnszone1),
+            command=('dnsrecord_mod', [dnszone1, u'_foo._tcp'], {'srv_part_priority': 100000,
+                                                  'srvrecord' : [u"0 100 1234 %s" % dnszone1_mname] }),
+            expected=errors.ValidationError(name='srv_priority', error=u'can be at most 65535'),
+        ),
+
+        dict(
+            desc='Modify SRV record in zone %r using parts' % (dnszone1),
+            command=('dnsrecord_mod', [dnszone1, u'_foo._tcp'], {'srv_part_priority': 1,
+                                                  'srvrecord' : [u"0 100 1234 %s" % dnszone1_mname] }),
+            expected={
+                'value': u'_foo._tcp',
+                'summary': None,
+                'result': {
+                    'idnsname': [u'_foo._tcp'],
+                    'srvrecord': [u"1 100 1234 %s" % dnszone1_mname],
+                },
+            },
+        ),
+
+        dict(
             desc='Try to add invalid LOC record to zone %r using dnsrecord_add' % (dnszone1),
             command=('dnsrecord_add', [dnszone1, u'@'], {'locrecord': u"91 11 42.4 N 16 36 29.6 E 227.64" }),
             expected=errors.ValidationError(name='locrecord', error=''),