Re: [Freeipa-devel] [PATCHES] 0016 Fixes for{add, set, del}attr with managed attributes
Petr Viktorin wrote:
On 02/29/2012 04:34 PM, Petr Viktorin wrote:
On 02/29/2012 03:50 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/27/2012 11:03 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Patch 16 defers validation & conversion until after
{add,del,set}attr is
processed, so that we don't search for an integer in a list of
strings
(this caused ticket #2405), and so that the end result of these
operations is validated (#2407).
Patch 17 makes these options honor params marked no_create and
no_update.
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
NACK on Patch 17 which breaks patch 16.
How is patch 16 broken? It works for me.
My point is they rely on one another, IMHO, so without 17 the reported
problem still exists.
*attr is specifically made to be powerful. We don't want to
arbitrarily
block updating certain values.
Noted
Not having patch 17 means that the problem reported in 2408 still
occurs. It should probably check both the schema and the param to
determine if something can have multiple values and reject that way.
I see the problem now: the certificate subject base is defined as a
multi-value attribute in the LDAP schema. If it's changed to
single-value the existing validation should catch it.
Also, most of the config attributes should probably be required in the
schema. Am I right?
I'm a newbie here; what do I need to do when changing the schema? Is
there a patch that does something similar I could use as an example?
The framework should be able to impose its own single-value will as
well. If a Param is designated as single-value the *attr should honor
it.
Here is the updated patch.
Since *attr is powerful enough to modify 'no_update' Params, which
CRUDUpdate forgets about, I need to check the params of the LDAPObject
itself.
Updating schema is a bit of a nasty business right now. See
10-selinuxusermap.update for an example.
I'll leave schema changes for after the release, then.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Attached patch includes tests. Note that the test depends on my patches
12-13, which make ipasearchrecordslimit required.
I gather that this eliminates the need for patch 17? It seems to work as-is.
The patch doesn't apply because of an encoding change Martin made recently.
It does seem to do the right thing though.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0016 Fixes for{add, set, del}attr with managed attributes
On 02/29/2012 04:34 PM, Petr Viktorin wrote:
On 02/29/2012 03:50 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/27/2012 11:03 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Patch 16 defers validation & conversion until after
{add,del,set}attr is
processed, so that we don't search for an integer in a list of strings
(this caused ticket #2405), and so that the end result of these
operations is validated (#2407).
Patch 17 makes these options honor params marked no_create and
no_update.
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
NACK on Patch 17 which breaks patch 16.
How is patch 16 broken? It works for me.
My point is they rely on one another, IMHO, so without 17 the reported
problem still exists.
*attr is specifically made to be powerful. We don't want to arbitrarily
block updating certain values.
Noted
Not having patch 17 means that the problem reported in 2408 still
occurs. It should probably check both the schema and the param to
determine if something can have multiple values and reject that way.
I see the problem now: the certificate subject base is defined as a
multi-value attribute in the LDAP schema. If it's changed to
single-value the existing validation should catch it.
Also, most of the config attributes should probably be required in the
schema. Am I right?
I'm a newbie here; what do I need to do when changing the schema? Is
there a patch that does something similar I could use as an example?
The framework should be able to impose its own single-value will as
well. If a Param is designated as single-value the *attr should honor it.
Here is the updated patch.
Since *attr is powerful enough to modify 'no_update' Params, which
CRUDUpdate forgets about, I need to check the params of the LDAPObject
itself.
Updating schema is a bit of a nasty business right now. See
10-selinuxusermap.update for an example.
I'll leave schema changes for after the release, then.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Attached patch includes tests. Note that the test depends on my patches
12-13, which make ipasearchrecordslimit required.
--
Petr³
From 590e37a1982966de8a600918cabdddb17adba2e8 Mon Sep 17 00:00:00 2001
From: Petr Viktorin
Date: Fri, 24 Feb 2012 12:26:28 -0500
Subject: [PATCH] Defer conversion and validation until after
--{add,del,set}attr are handled
--addattr & friends that modified attributes known to Python sometimes
used converted and validated Python values instead of LDAP strings.
This caused a problem for --delattr, which searched for a converted
integer in a list of raw strings (ticket 2407).
With this patch we work on raw strings, converting only when done.
Deferring validation ensures the end result is valid, so proper errors
are raised instead of failing later (ticket 2405).
Tests included.
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
---
ipalib/plugins/baseldap.py | 37 ++---
tests/test_xmlrpc/test_attr.py | 50
2 files changed, 73 insertions(+), 14 deletions(-)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 725704ee0e2d784a1f5ad8691d90888366f8efec..cdfe0fe2b127b886e1889a68fa263208e055db4b 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -759,8 +759,6 @@ last, after all sets and adds."""),
Convert a string in the form of name/value pairs into a dictionary.
The incoming attribute may be a string or a list.
-Any attribute found that is also a param is validated.
-
:param attrs: A list of name/value pairs
:param append: controls whether this returns a list of values or a single
@@ -776,12 +774,6 @@ last, after all sets and adds."""),
if len(value) == 0:
# None means "delete this attribute"
value = None
-if attr in self.params:
-try:
- value = self.params[attr](value)
-except errors.ValidationError, err:
-(name, error) = str(err.strerror).split(':')
-raise errors.ValidationError(name=attr, error=error)
if append and attr in newdict:
if type(value) in (tuple,):
newdict[attr] += list(value)
@@ -885,12 +877,29 @@ last, after all sets and adds."""),
# normalize all values
changedattrs = setattrs | addattrs | delattrs
for attr in changedattrs:
-# remove duplicite and invalid values
-entry_attrs[attr] = list(set([val for val in entry_attrs[attr] if val]))
-if not entry_attrs[attr]:
-entry_attrs[attr] = None
-
Re: [Freeipa-devel] [PATCHES] 0016 Fixes for{add, set, del}attr with managed attributes
On 02/29/2012 03:50 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/27/2012 11:03 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Patch 16 defers validation & conversion until after
{add,del,set}attr is
processed, so that we don't search for an integer in a list of strings
(this caused ticket #2405), and so that the end result of these
operations is validated (#2407).
Patch 17 makes these options honor params marked no_create and
no_update.
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
NACK on Patch 17 which breaks patch 16.
How is patch 16 broken? It works for me.
My point is they rely on one another, IMHO, so without 17 the reported
problem still exists.
*attr is specifically made to be powerful. We don't want to arbitrarily
block updating certain values.
Noted
Not having patch 17 means that the problem reported in 2408 still
occurs. It should probably check both the schema and the param to
determine if something can have multiple values and reject that way.
I see the problem now: the certificate subject base is defined as a
multi-value attribute in the LDAP schema. If it's changed to
single-value the existing validation should catch it.
Also, most of the config attributes should probably be required in the
schema. Am I right?
I'm a newbie here; what do I need to do when changing the schema? Is
there a patch that does something similar I could use as an example?
The framework should be able to impose its own single-value will as
well. If a Param is designated as single-value the *attr should honor it.
Here is the updated patch.
Since *attr is powerful enough to modify 'no_update' Params, which
CRUDUpdate forgets about, I need to check the params of the LDAPObject
itself.
Updating schema is a bit of a nasty business right now. See
10-selinuxusermap.update for an example.
I'll leave schema changes for after the release, then.
--
Petr³
From 38a30c9215d0d708e1c0a4c9ba92eafffbde1f74 Mon Sep 17 00:00:00 2001
From: Petr Viktorin
Date: Fri, 24 Feb 2012 12:26:28 -0500
Subject: [PATCH] Defer conversion and validation until after
--{add,del,set}attr are handled
--addattr & friends that modified attributes known to Python sometimes
used converted and validated Python values instead of LDAP strings.
This caused a problem for --delattr, which searched for a converted
integer in a list of raw strings (ticket 2407).
With this patch we work on raw strings, converting only when done.
Deferring validation ensures the end result is valid, so proper errors
are raised instead of failing later (ticket 2405).
https://fedorahosted.org/freeipa/ticket/2405
https://fedorahosted.org/freeipa/ticket/2407
https://fedorahosted.org/freeipa/ticket/2408
---
ipalib/plugins/baseldap.py | 37 +++--
1 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 725704ee0e2d784a1f5ad8691d90888366f8efec..cdfe0fe2b127b886e1889a68fa263208e055db4b 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -759,8 +759,6 @@ last, after all sets and adds."""),
Convert a string in the form of name/value pairs into a dictionary.
The incoming attribute may be a string or a list.
-Any attribute found that is also a param is validated.
-
:param attrs: A list of name/value pairs
:param append: controls whether this returns a list of values or a single
@@ -776,12 +774,6 @@ last, after all sets and adds."""),
if len(value) == 0:
# None means "delete this attribute"
value = None
-if attr in self.params:
-try:
- value = self.params[attr](value)
-except errors.ValidationError, err:
-(name, error) = str(err.strerror).split(':')
-raise errors.ValidationError(name=attr, error=error)
if append and attr in newdict:
if type(value) in (tuple,):
newdict[attr] += list(value)
@@ -885,12 +877,29 @@ last, after all sets and adds."""),
# normalize all values
changedattrs = setattrs | addattrs | delattrs
for attr in changedattrs:
-# remove duplicite and invalid values
-entry_attrs[attr] = list(set([val for val in entry_attrs[attr] if val]))
-if not entry_attrs[attr]:
-entry_attrs[attr] = None
-elif isinstance(entry_attrs[attr], (tuple, list)) and len(entry_attrs[attr]) == 1:
-entry_attrs[attr] = entry_attrs[attr][0]
+if attr in self.obj.params:
+# convert single-value params to scalars
+# Need to use the LDAPObject's params, not self's, because the
+# CRUD classes filter their disallowed parameters out.
+
