[Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
Ondrej Hamada
ohamada at redhat.com
Tue Mar 27 15:56:37 UTC 2012
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
--
Regards,
Ondrej Hamada
FreeIPA team
jabber: ohama at jabbim.cz
IRC: ohamada
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ohamada-16-3-Netgroup-nisdomain-and-hosts-validation.patch
Type: text/x-patch
Size: 27131 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120327/6d88f84a/attachment.bin>
More information about the Freeipa-devel
mailing list