[Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
Martin Kosek
mkosek at redhat.com
Wed Mar 28 14:25:53 UTC 2012
On Tue, 2012-03-27 at 17:56 +0200, Ondrej Hamada wrote:
> On 03/27/2012 01:57 PM, Martin Kosek wrote:
> > On Fri, 2012-03-23 at 23:10 +0100, Ondrej Hamada wrote:
> >> On 03/15/2012 08:13 AM, Martin Kosek wrote:
> >>> 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
> >>>>
> >> nisdomain validation:
> >> Added pattern to the 'nisdomain' parameter to validate the specified
> >> nisdomain name. According to most common use cases the same pattern as
> >> for netgroup should fit. Unit-tests added.
> >>
> >> https://fedorahosted.org/freeipa/ticket/2448
> >>
> >> 'add_external_pre_callback' function was created to allow validation of
> >> all external members. Validation is based on usage of objects primary
> >> key parameter. The 'add_external_pre_callback' fucntion has to be called
> >> directly from in the 'pre_callback' function. This change affects
> >> netgroup, hbacrule and sudorule commands.
> >>
> >> Special validator is used only for hostname, the validator requires
> >> fully qualified
> >> domain name and enables the hostnames to contain underscores.
> >>
> >> Unit-tests added.
> >>
> >> https://fedorahosted.org/freeipa/ticket/2447
> >>
> > This is better, but I still see few issues:
> >
> > 1) You copied hostname validator instead of extending validate_hostname
> > function in ipalib.util with allow_underscore parameter which is already
> > available in validate_dns_label. Having duplicate functions like this is
> > just wrong and can lead to errors in future.
> >
> > 2) I also wonder if externalHost can contain non-fqdn hosts. In such
> > case, you would just add check_fqdn=False to validate_hostname.
> >
> > Martin
> >
> corrected patch attached
>
Yup, its ok now. ACK. Pushed to master, ipa-2-2.
Martin
More information about the Freeipa-devel
mailing list