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

[Ondrej Hamada]

On 03/09/2012 04:34 PM, Martin Kosek wrote:
> 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

external_pre_callback function seems as a good idea, but there is a 
problem how to get the validators for various LDAP objects. For the 
hostname we already have one in ipalib.utils, but for the uid or group 
name we use only patterns specified in the parameter objects.

Below I propose solution how to use the already defined parameter 
objects for validation (the only problem is that I have to assume, that 
it is always the first parameter in takes_params). Do you think this is 
a good approach?

def add_external_pre_callback(memberattr, membertype, externalattr, 
ldap, dn, found, not_found, *keys, **options):
     """
     Pre callback to validate external members.
     """
     if membertype in options:
         validator = api.Object[membertype].takes_params[0]
         for value in options[membertype]:
             try:
                 validator(value)
             except errors.ValidationError as e:
                 error_msg = e[(e.find(':')+1):]
                 raise errors.ValidationError(name=membertype, 
error=e[e.find(':')+1:])
     return dn

-- 
Regards,

Ondrej Hamada
FreeIPA team
jabber: ohama at jabbim.cz
IRC: ohamada