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?
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]
for value in options[membertype]:
try:
validator(value)
except errors.ValidationError as e:
error_msg = e[(e.find(':')+1):]
raise errors.ValidationError(name=membertype,
error=e[e.find(':')+1:])
return dn
--
Regards,
Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
