On 03/16/2012 06:35 PM, Petr Viktorin wrote:
On 03/16/2012 06:33 PM, Rob Crittenden wrote:
Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/15/2012 09:24 PM, Rob Crittenden wrote:
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.

Yes. Patch 17 made *attr honor no_create and no_update, which you said
is not desired behavior.

The patch doesn't apply because of an encoding change Martin made
recently.

It does seem to do the right thing though.

rob

Attaching rebased patch.
This deletes Martin's change, but unless I tested wrong, his bug
(https://fedorahosted.org/freeipa/ticket/2418) stays fixed. The
tests in
my patch should apply to that ticket as well.

In another fork of this thread, there's discussion if this approach is
good at all. Maybe we're overengineering a corner case here.


Found another issue, a very subtle one.

When using *attr and an exception occurs where the param name would
appear we want the name passed in to be used.

For example:

$ ipa pwpolicy-mod --setattr=krbpwdmaxfailure=xyz

With this patch it will return:
ipa: ERROR: invalid 'maxfail': must be an integer

It should return:
ipa: ERROR: invalid 'krbpwdmaxfailure': must be an integer

On second thought, this may not be related...

This is https://fedorahosted.org/freeipa/ticket/1418, I'll see if it
makes sense to fix it here.

Okay, this is a different problem. A quick grep of ConversionError in parameters.py reveals that all of the "wrong datatype" errors are raised with the raw parameter name. Other errors are raised with either that or the cli_name or they auto-detect. I don't think they follow some rule in this.

Furthermore, our test suite doesn't check exception arguments. Sounds like a major cleanup coming up here...



The encoding problem does still exist too:

$ ipa config-mod --setattr ipamigrationenabled=false
ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax: Invalid
syntax.


Will fix.


Fixed in the attached update, which encodes the value.

I was surprised to find that
config_mod.params['ipamigrationenabled'].attribute is True, while
config_mod.obj.params['ipamigrationenabled'].attribute is False (and so its encode() method does nothing). That's because 'attribute' is only set when the params are cloned from the LDAPObject to the CRUD method.
Is there a reason behind this, or is it just that it was easier to do?

For this case it means that params marked no_update will not be encoded properly -- getting to a working encode() would require either setting 'attribute' on the parent object or some ugly hackery.



---
PetrĀ³
From 5399eb022ff67a6a47d6e5f672ed955a8051c762 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
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     |   46 +++++++++++------
 tests/test_xmlrpc/test_attr.py |  113 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+), 16 deletions(-)

diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 9562ff98729ead6ac9e56d504f6ee0a7c0ca377a..23ba64f600c4a2b0cc4fde7aa30853bb89b0f54f 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,14 +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 self.api.env.in_server:
-                    value = self.params[attr].encode(value)
             if append and attr in newdict:
                 if type(value) in (tuple,):
                     newdict[attr] += list(value)
@@ -887,12 +877,36 @@ 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
+                value = entry_attrs[attr]
+                try:
+                    param = self.params[attr]
+                except KeyError:
+                    # The CRUD classes filter their disallowed parameters out.
+                    # Yet {set,add,del}attr are powerful enough to change these
+                    # (e.g. Config's ipacertificatesubjectbase)
+                    # So, use the parent's attribute
+                    param = self.obj.params[attr]
+                if not param.multivalue:
+                    if len(value) == 1:
+                        value = value[0]
+                    elif not value:
+                        value = None
+                    else:
+                        raise errors.OnlyOneValueAllowed(attr=attr)
+                # validate, convert and encode params
+                value = param(value)
+                value = param.encode(value)
+                entry_attrs[attr] = value
+            else:
+                # unknown attribute: 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]
+
 
 class LDAPCreate(BaseLDAPCommand, crud.Create):
     """
diff --git a/tests/test_xmlrpc/test_attr.py b/tests/test_xmlrpc/test_attr.py
index ef239709dba58cca42ad0127e8223d172885e6cd..e6872a67a1fcfa3f782a2415bb79c416d2f75283 100644
--- a/tests/test_xmlrpc/test_attr.py
+++ b/tests/test_xmlrpc/test_attr.py
@@ -402,4 +402,117 @@ class test_attr(Declarative):
             ),
         ),
 
+        dict(
+            desc='Lock %r using setattr' % user1,
+            command=(
+                'user_mod', [user1], dict(setattr=u'nsaccountlock=TrUe')
+            ),
+            expected=dict(
+                result=dict(
+                    givenname=[u'Finkle'],
+                    homedirectory=[u'/home/tuser1'],
+                    loginshell=[u'/bin/sh'],
+                    sn=[u'User1'],
+                    uid=[user1],
+                    uidnumber=[fuzzy_digits],
+                    gidnumber=[fuzzy_digits],
+                    mail=[u't...@example.com', u'te...@example.com'],
+                    memberof_group=[u'ipausers'],
+                    telephonenumber=[u'202-888-9833'],
+                    nsaccountlock=True,
+                    has_keytab=False,
+                    has_password=False,
+                ),
+                summary=u'Modified user "tuser1"',
+                value=user1,
+            ),
+        ),
+
+        dict(
+            desc='Unlock %r using addattr&delattr' % user1,
+            command=(
+                'user_mod', [user1], dict(
+                    addattr=u'nsaccountlock=FaLsE',
+                    delattr=u'nsaccountlock=True')
+            ),
+            expected=dict(
+                result=dict(
+                    givenname=[u'Finkle'],
+                    homedirectory=[u'/home/tuser1'],
+                    loginshell=[u'/bin/sh'],
+                    sn=[u'User1'],
+                    uid=[user1],
+                    uidnumber=[fuzzy_digits],
+                    gidnumber=[fuzzy_digits],
+                    mail=[u't...@example.com', u'te...@example.com'],
+                    memberof_group=[u'ipausers'],
+                    telephonenumber=[u'202-888-9833'],
+                    nsaccountlock=False,
+                    has_keytab=False,
+                    has_password=False,
+                ),
+                summary=u'Modified user "tuser1"',
+                value=user1,
+            ),
+        ),
+
+        dict(
+            desc='Try adding a new group search fields config entry',
+            command=(
+                'config_mod', [], dict(addattr=u'ipagroupsearchfields=newattr')
+            ),
+            expected=errors.OnlyOneValueAllowed(attr='ipagroupsearchfields'),
+        ),
+
+        dict(
+            desc='Try adding a new cert subject base config entry',
+            command=(
+                'config_mod', [], dict(addattr=u'ipacertificatesubjectbase=0=DOMAIN.COM')
+            ),
+            expected=errors.OnlyOneValueAllowed(attr='ipacertificatesubjectbase'),
+        ),
+
+        dict(
+            desc='Try deleting a required config entry',
+            command=(
+                'config_mod', [], dict(delattr=u'ipasearchrecordslimit=100')
+            ),
+            expected=errors.RequirementError(name='ipasearchrecordslimit'),
+        ),
+
+        dict(
+            desc='Try setting nonexistent attribute',
+            command=('config_mod', [], dict(setattr=u'invalid_attr=false')),
+            expected=errors.ObjectclassViolation(
+                info='attribute "invalid_attr" not allowed'),
+        ),
+
+        dict(
+            desc='Try setting out-of-range krbpwdmaxfailure',
+            command=('pwpolicy_mod', [], dict(setattr=u'krbpwdmaxfailure=-1')),
+            expected=errors.ValidationError(name='krbpwdmaxfailure',
+                error='must be at least 0'),
+        ),
+
+        dict(
+            desc='Try setting out-of-range maxfail',
+            command=('pwpolicy_mod', [], dict(krbpwdmaxfailure=u'-1')),
+            expected=errors.ValidationError(name='maxfail',
+                error='must be at least 0'),
+        ),
+
+        dict(
+            desc='Try setting non-numeric krbpwdmaxfailure',
+            command=('pwpolicy_mod', [], dict(setattr=u'krbpwdmaxfailure=abc')),
+            expected=errors.ConversionError(name='krbpwdmaxfailure',
+                error='must be an integer'),
+        ),
+
+        dict(
+            desc='Try setting non-numeric maxfail',
+            command=('pwpolicy_mod', [], dict(krbpwdmaxfailure=u'abc')),
+            expected=errors.ConversionError(name='maxfail',
+                error='must be an integer'),
+        ),
+
     ]
-- 
1.7.7.6

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to