[Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation

[Martin Kosek]

On Thu, 2012-03-08 at 14:52 +0100, Ondrej Hamada wrote:
> Netgroup nisdomain and hosts validation
> 
> nisdomain validation:
> Added pattern to the 'nisdomain' parameter to validate the specified
> nisdomain name. According to most common use cases the same patter as
> for netgroup should fit. Unit-tests added.
> 
> https://fedorahosted.org/freeipa/ticket/2447
> 
> hosts validation:
> Added precallback to netgroup_add_member. It validates the specified
> hostnames and raises ValidationError exception for invalid hostnames.
> Unit-test added.
> 
> https://fedorahosted.org/freeipa/ticket/2448


I checked the host validation part and it could be improved. Issue
described in #2447 (you have switched the ticket IDs) affects all
objects that allow external hosts, users, ..., i.e. those who call
add_external_post_callback in their post_callback.

Should we fix all of these when we deal with this issue? Otherwise user
could do something like this:
# ipa sudorule-add-user foo --users=a+b
  Rule name: foo
  Enabled: TRUE
  External User: a+b

We could create a similar function called add_external_pre_callback()
and pass it attribute name and validating function (which would be
common with the linked object). It would then do the validation for all
these affected objects consistently and without redundant code.

I didn't liked much the implemented pre_callback anyway

+    def pre_callback(self, ldap, dn, found, not_found, *keys,
**options):
+        # validate entered hostnames
+        if 'host' in options:
+            invalid_hostnames=[]
+            for hostname in options['host']:
+                try:
+                    validate_hostname(hostname, False)
+                except ValueError:
+                    invalid_hostnames.append(hostname)
+            if invalid_hostnames:
+                raise errors.ValidationError(name='host',
error='hostnames:\"%s\" contain invalid characters' %
','.join(invalid_hostnames))
+        return dn

I would rather raise the ValidationError with the first invalid hostname
and tell what's wrong (function validate_hostname tells it to you). If
you go with the proposed approach, you wouldn't have to deal with
formatting error messages, you would just raise the one returned by the
validator shared with the linked LDAP object (hostname, user, ...).

Martin