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