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

[Martin Kosek]

On Wed, 2012-03-14 at 16:54 +0100, Ondrej Hamada wrote:
> 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?

I think the approach is OK, it can just be much improved in order to get
rid of the hardcoded parts. See comments below.

> 
> 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]

You can use api.Object[membertype].params[memberattr]

>          for value in options[membertype]:
>              try:
>                  validator(value)
>              except errors.ValidationError as e:
>                  error_msg = e[(e.find(':')+1):]

You don't have to parse error message, you can just use e.name or
e.error right from the caught ValidationError.

>                  raise errors.ValidationError(name=membertype, 
> error=e[e.find(':')+1:])
>      return dn
>