Re: [Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
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_extern
Re: [Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
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
Re: [Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
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.
>
>
Re: [Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
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
--
Regards,
Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada
From eaf9300a5c4236835932743447c53ea430d5194d Mon Sep 17 00:00:00 2001
From: Ondrej Hamada
Date: Fri, 23 Mar 2012 13:16:36 +0100
Subject: [PATCH] Netgroup nisdomain and hosts validation
nisdomain validation:
Added pattern to the 'nisdomain' parameter to validate the specified
nisdomain name. Acc
Re: [Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
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
>
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
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
Re: [Freeipa-devel] [PATCH] 16 Netgroup nisdomain and hosts validation
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
