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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel