Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
On 29.2.2012 15:50, 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.
Is that so? Isn't *attr supposed to allow the user to modify attributes
at LDAP level, i.e. skip the usual framework constraints?
Updating schema is a bit of a nasty business right now. See
10-selinuxusermap.update for an example.
rob
Honza
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 239 Tolerate UDP port failures in conncheck
UDP port checks in ipa-replica-conncheck are too strict. The entire
conncheck fails when UDP ports cannot be verified as open. However,
UDP protocol is unrealiable by its nature and the port can also not
be checked if there is an application already bound to it. This can
happen for example when ipa-replica-conncheck is run as a part of
ipa-ca-install and the replica services are thus already running.
This patch changes the behavior of UDP port checks. The conncheck
script now rather reports a warning that UDP port cannot be verified
but does not fail the entire test.
https://fedorahosted.org/freeipa/ticket/2514
From cd39f11b88fe2098a245d2d7983e01ef533d49e3 Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Fri, 16 Mar 2012 10:26:56 +0100
Subject: [PATCH] Tolerate UDP port failures in conncheck
UDP port checks in ipa-replica-conncheck are too strict. The entire
conncheck fails when UDP ports cannot be verified as open. However,
UDP protocol is unrealiable by its nature and the port can also not
be checked if there is an application already bound to it. This can
happen for example when ipa-replica-conncheck is run as a part of
ipa-ca-install and the replica services are thus already running.
This patch changes the behavior of UDP port checks. The conncheck
script now rather reports a warning that UDP port cannot be verified
but does not fail the entire test.
https://fedorahosted.org/freeipa/ticket/2514
---
install/tools/ipa-replica-conncheck | 21 -
1 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index 44b3caa45a20d3a72985c051a7982da1f9716147..77d3bbffdd2224bcc0d65d267282db02c7cca854 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -243,18 +243,29 @@ def port_check(host, port_list):
if not ip:
raise RuntimeError(Port check failed! Unable to resolve host name '%s' % host)
-failed_ports = []
+ports_failed = []
+ports_udp_warning = [] # conncheck could not verify that port is open
for port in port_list:
if ipautil.host_port_open(host, port.port, port.port_type, socket_timeout=CONNECT_TIMEOUT):
result = OK
else:
-failed_ports.append(port)
-result = FAILED
+if port.port_type == socket.SOCK_DGRAM:
+ports_udp_warning.append(port)
+result = WARNING
+else:
+ports_failed.append(port)
+result = FAILED
print_info( %s (%d): %s % (port.description, port.port, result))
-if failed_ports:
+if ports_udp_warning:
+print The following UDP ports could not be verified as open: %s \
+% , .join(str(port.port) for port in ports_udp_warning)
+print This can happen if they are already bound to an application
+print and ipa-replica-conncheck cannot attach own UDP responder.
+
+if ports_failed:
msg_ports = []
-for port in failed_ports:
+for port in ports_failed:
port_type_text = TCP if port.port_type == SOCK_STREAM else UDP
msg_ports.append('%d (%s)' % (port.port, port_type_text))
raise RuntimeError(Port check failed! Inaccessible port(s): %s \
--
1.7.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015 Only split CSV strings once
On 03/15/2012 08:55 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/2227 (Unable to add certain sudo
commands to groups). What an interesting bug to get :)
One problem with our CSV splitting is that it's not idempotent
(baskslashes are eaten when there are escaped commas), but when we
forward a call it gets done on both the client and the server. The
attached patch fixes that in a somewhat hacky way. It's not a complete
fix for the issue though, for that I need to ask what to do.
Some Params use the CSV format when we just need a list of
comma-separated values. Our flavor of CSV does escaping in two different
ways. This is pretty bad for predictability, least-surprise,
documentation, ...
To recap, the two ways are escaping (escaped commas are ignored,
backslashes also need to be escaped) and quoting (values are optionally
enclosed in double quotes, to include a '' in a quoted string, use
'').
Escaping is good because users are used to it, but currently literal
backslashes need to be doubled, making multivalue syntax different from
single-value syntax, even if there are no commas in the value.
Quoting is weird, but complete by itself so it doesn't also need
escaping.
Do we need to use both? Is this documented somewhere? Some of our tests
check only for one, some assume the other. Users are probably even more
confused.
If we only keep one of those, the fix for #2227 should be quite easy.
If not (backwards compatibility), we need to document this properly,
test all the corner cases, and fix the UI to handle CSV escaping.
Since it's subtly broken in lots of places, maybe it's best to break
backwards compatibility and go for a simple solution now.
Anyway, if I want to do a proper fix, CSV handling should be only done
on the client. Unfortunately turning off CSV handling in the server will
break the UI, which at places uses `join(',')` (no escaping commas, no
single place to change it), even though it can just send a list.
I'm with Honza, not too keen on the _forwarded_call part. I'd rather you
do a mix of comparing self.env.in_server and whether the value is a
basestring to determine if we need to split it.
This was a hack necessary because the WebUI relied on server-side
splitting (in a few cases, and it did not escape correctly). Now that
Petr Vobornik's patch 99 is in, it is unnecessary.
This discussion thread has an updated patch (0015-03); what I'm
attaching now builds on top of that to add several new changes in tests.
To summarize it: the splitting is only done in the client; from the RPC
call on no CSV magic is involved at all.
Please tell me your reasons for basestring checking. I think it's
entirely unnecessary and just adds complexity.
In the rest of the framework a string, as a parameter value, has the
same effect as a list of that one string. Why break this?
We are concerned about the API here, not the command line. Shortcuts to
save the user's keystrokes in the common case should *not* win over
predictability and making sure the easier way is the correct, robust way
to do it. Compare -- with basestring checking:
some_command(arg=escape_commas(some_string))
- is correct
some_command(arg=[some_string])
- is also correct
some_command(arg=some_string)
- is WRONG because the string is not escaped
- but is the easiest solution!
The problem is much worse because it is subtle: it only affects values
with commas and backslashes, so *the wrong solution would work* in most
of the cases.
And I can assure you plugin writers *would* pick the wrong way if it
would usually work. Our own web UI had examples.
Contrast to my proposed behavior:
some_command(arg=some_string)
- is correct
some_command(arg=[some_string])
- is also correct
some_command(arg=escape_commas(some_string))
- is wrong, but doesn't make much sense because at this level you
don't have to worry about CSV at all
Basestring checking would add unnecessary complexity for both us *and*
the plugin writer. The only case it would make it easier for the plugin
writer is if the plugin took CSV values -- but why force a random
format, tailored for our specific frontend, into RPC clients?
The client should translate the command line into a RPC call. There's no
reason to arbitrarily expect the server to do part of the job.
I'm not sure why this is broken out of normalize. Isn't this normalizing
the incoming values into something more sane?
Normalization implies it's an idempotent operation, which CSV
splitting is not (at least its current incarnation, without the
basestring checking):
ra,b\,c splits to [a, b,c] which splits to [a, b, c]
-
Petr³
From 92cbcf4bc6ecd86e2301283d3654d89e576c0dee Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 23 Feb 2012 07:29:47 -0500
Subject: [PATCH 15/16] Only split CSV in the client
Neither the Web UI nor other XML-RPC or JSON clients other than the CLI
need CSV handling
Re: [Freeipa-devel] [PATCH] 0021 Add CLI tests
On 03/13/2012 12:23 PM, Petr Viktorin wrote:
On 03/12/2012 09:18 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Most of the tests we have check if the server does the right thing with
XML-RPC calls. How the commandline is converted to command arguments,
including interactive prompting, is untested.
This patch adds some tests in this area. To do that I had to break up
cli.run into more manageable pieces, and initialize the CLI plugins in
test mode.
Also I added nose's --nocapture option to the make-test script. With
this it's possible to use pdb.set_trace() to drop into a debugger while
running the tests.
I went ahead and added a test for ticket 2484, fixed in Martin's patch
231 (Ignore case in yes/no prompts).
John had done similar work with --nocapture a while back but it is still
pending. Perhaps we can merge his changes in with yours, they look
largely overlapping. His patch is [PATCH 54/54] ticet 2135 - enhance
make-test for debugging
rob
I guess it'll be better to make that a separate patch, then. This
updated patch doesn't contain the make-test change.
Updating the patch to also test the CSV splitting in my patch 0015.04
--
Petr³
From 00d584e01336ea8b86f476c8904c3ac1a1dd6855 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 13 Mar 2012 07:10:52 -0400
Subject: [PATCH] Add CLI parsing tests
These test that command lines are parsed to correct Command arguments.
Includes some tests for interactive prompts.
To make this possible cli.run is broken up into several pieces.
---
ipalib/__init__.py |3 +
ipalib/backend.py |1 -
ipalib/cli.py | 24 +-
tests/test_cmdline/test_cli.py | 153
4 files changed, 176 insertions(+), 5 deletions(-)
create mode 100644 tests/test_cmdline/test_cli.py
diff --git a/ipalib/__init__.py b/ipalib/__init__.py
index 1efeeab4a6c5cef8f625c3964be253baf208dd29..dd861a8266614d63a81289672ce2235275c356c0 100644
--- a/ipalib/__init__.py
+++ b/ipalib/__init__.py
@@ -916,5 +916,8 @@ def create_api(mode='dummy'):
api = create_api(mode=None)
if os.environ.get('IPA_UNIT_TEST_MODE', None) == 'cli_test':
+from cli import cli_plugins
+for klass in cli_plugins:
+api.register(klass)
api.bootstrap(context='cli', in_server=False, in_tree=True)
api.finalize()
diff --git a/ipalib/backend.py b/ipalib/backend.py
index 0232fa536ed83273d1c6510ee442915bb8c0c8c1..7be38ecc80faf03e735813fb1e2d0eba5c347800 100644
--- a/ipalib/backend.py
+++ b/ipalib/backend.py
@@ -102,7 +102,6 @@ class Connectible(Backend):
class Executioner(Backend):
-
def create_context(self, ccache=None, client_ip=None):
client_ip: The IP address of the remote client.
diff --git a/ipalib/cli.py b/ipalib/cli.py
index ea320cf652e309592f9906831e3de2d0beb10198..5e58cc47d5e5d61a76bc917268f0e63307228efa 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -123,7 +123,7 @@ class textui(backend.Backend):
def __get_encoding(self, stream):
assert stream in (sys.stdin, sys.stdout)
-if stream.encoding is None:
+if getattr(stream, 'encoding', None) is None:
return 'UTF-8'
return stream.encoding
@@ -1007,7 +1007,11 @@ class cli(backend.Executioner):
Backend plugin for executing from command line interface.
-def run(self, argv):
+def get_command(self, argv):
+Given CLI arguments, return the Command to use
+
+On incorrect invocation, prints out a help message and returns None
+
if len(argv) == 0:
self.Command.help()
return
@@ -1022,15 +1026,27 @@ class cli(backend.Executioner):
if name not in self.Command or self.Command[name].NO_CLI:
raise CommandError(name=key)
cmd = self.Command[name]
-if not isinstance(cmd, frontend.Local):
-self.create_context()
+return cmd
+
+def argv_to_keyword_arguments(self, cmd, argv):
+Get the keyword arguments for a Command
kw = self.parse(cmd, argv)
if self.env.interactive:
self.prompt_interactively(cmd, kw)
kw = cmd.split_csv(**kw)
kw['version'] = API_VERSION
self.load_files(cmd, kw)
+return kw
+
+def run(self, argv):
+cmd = self.get_command(argv)
+if cmd is None:
+return
+name = cmd.name
+if not isinstance(cmd, frontend.Local):
+self.create_context()
try:
+kw = self.argv_to_keyword_arguments(cmd, argv[1:])
result = self.execute(name, **kw)
if callable(cmd.output_for_cli):
for param in cmd.params():
diff --git a/tests/test_cmdline/test_cli.py b/tests/test_cmdline/test_cli.py
new file mode 100644
index ..324c82563dd0bdcc86234097da7fd03efda00305
--- /dev/null
+++ b/tests/test_cmdline/test_cli.py
@@
Re: [Freeipa-devel] [PATCH] 987 Don't allow IPA master hosts and services to be disabled
On 03/15/2012 10:04 PM, Rob Crittenden wrote:
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index
9562ff98729ead6ac9e56d504f6ee0a7c0ca377a..f3c89a0fc5e3f00ed7f132dbff2510d89bc7370d
100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -887,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.
+# Yet {set,add,del}attr are powerful enough to change these
+# (e.g. Config's ipacertificatesubjectbase)
+if not self.obj.params[attr].multivalue:
+if len(entry_attrs[attr]) == 1:
+entry_attrs[attr] = entry_attrs[attr][0]
+elif not entry_attrs[attr]:
+entry_attrs[attr] = None
+else:
+raise errors.OnlyOneValueAllowed(attr=attr)
+# validate and convert params
+entry_attrs[attr] = self.obj.params[attr](entry_attrs[attr])
+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]
+
You've included an unrelated patch (my 0016).
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 986 remove unnecessary serial conversions
On 03/15/2012 06:31 PM, Rob Crittenden wrote: This is related to my patch 924. Petr Viktorin noticed a couple of serial to hex conversions were wrong and it turns out they aren't needed at all. This patch removes them. rob On 03/15/2012 06:32 PM, Rob Crittenden wrote: Petr Viktorin wrote: I think you should also update the docstrings with the new item. What docstrings are those? The big tables, such as: ++---+-+---+ |cms name|cms type |result name |result type| ++===+=+===+ |emailCert |Boolean|email_cert |bool | ++---+-+---+ |noCertImport|Boolean|no_cert_import |bool | ++---+-+---+ |revocationReason|int|revocation_reason|int [1]_ | ++---+-+---+ |certPrettyPrint |string |cert_pretty |unicode| ++---+-+---+ |authorityid |string |authority|unicode| ++---+-+---+ |certFingerprint |string |fingerprint |unicode| ++---+-+---+ |certChainBase64 |string |certificate |unicode [2]_ | ++---+-+---+ |serialNumber|string |serial_number|int|long | ++---+-+---+ |pkcs7ChainBase64|string |pkcs7_chain |unicode [2]_ | ++---+-+---+ etc. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0024 Fix expected exception checking in tests
On 03/15/2012 10:36 PM, Rob Crittenden wrote: Petr Viktorin wrote: Can you spot the bug in this test code? try: do_invalid_operation() except ExpectedError: pass Our test suite had several of those. Nose provides nice tools, `raises` (a decorator) and `assert_raises` (a context manager) that make checking expected exceptions a lot easier and less error-prone. This patch makes our tests use them. If you didn't catch it, the error is that the test will pass when no exception is raised. Some of our tests handled that by adding an `else: assert False`, or an `assert False` at the end of the try block. For consistency, the patch switches these correct ones to raises/assert_raises as well. I've also uncovered and fixed a few test bugs that were hidden by this. test_1a_automountmap_add_indirect() was failing, checking for the wrong exception. Silly me. Thanks for catching this. I also suggest using @raises for clarity in another spot. Here are my suggested changes: Attaching updated patch. -- Petr³ From 2945d42140861bebe8d66dcae29a5c9d577f3dce Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Fri, 9 Mar 2012 09:41:16 -0500 Subject: [PATCH] Use nose tools to check for exceptions Some of our tests checked for exceptions using an error-prone try block: they allowed the expected exception to pass, but sometimes forgot an else block, so the test passed when an exception wasn't thrown. This changes the tests to use the appropriate nose tools (raises, assert_raises). For consistency, tests that had a correct else block are also changed. Also fix some test problems that were hidden by the above: - in some sudorule and HBAC tests, change the *_add_user argument name from `users` to `user` - don't remove HBAC testing data while it was still used --- tests/test_install/test_updates.py | 15 ++--- tests/test_xmlrpc/test_automount_plugin.py | 86 +-- tests/test_xmlrpc/test_cert.py |7 +- tests/test_xmlrpc/test_hbac_plugin.py | 103 tests/test_xmlrpc/test_passwd_plugin.py|9 +-- tests/test_xmlrpc/test_sudorule_plugin.py | 74 6 files changed, 105 insertions(+), 189 deletions(-) diff --git a/tests/test_install/test_updates.py b/tests/test_install/test_updates.py index eb376f19113e534563d02eae51a798f7b9d9c773..8b9ac28eb68535d3820feea6d47639d32df7be56 100644 --- a/tests/test_install/test_updates.py +++ b/tests/test_install/test_updates.py @@ -24,7 +24,8 @@ import os import sys import ldap import nose -from tests.util import raises, PluginTester +from nose.tools import raises +from tests.util import PluginTester from tests.data import unicode_str from ipalib import api from ipalib import errors @@ -182,20 +183,16 @@ class test_update(object): assert(modified == True) +@raises(BadSyntax) def test_8_badsyntax(self): Test the updater with an unknown keyword -try: -modified = self.updater.update([self.testdir + 8_badsyntax.update]) -except BadSyntax: -pass +modified = self.updater.update([self.testdir + 8_badsyntax.update]) +@raises(BadSyntax) def test_9_badsyntax(self): Test the updater with an incomplete line -try: -modified = self.updater.update([self.testdir + 9_badsyntax.update]) -except BadSyntax: -pass +modified = self.updater.update([self.testdir + 9_badsyntax.update]) diff --git a/tests/test_xmlrpc/test_automount_plugin.py b/tests/test_xmlrpc/test_automount_plugin.py index cfea61270fca423fe29e00747b507d170e05d255..dedd2346aaca52434074e028ff9ed1629d69f9d2 100644 --- a/tests/test_xmlrpc/test_automount_plugin.py +++ b/tests/test_xmlrpc/test_automount_plugin.py @@ -22,6 +22,8 @@ Test the `ipalib/plugins/automount.py' module. import sys +from nose.tools import raises, assert_raises # pylint: disable=E0611 + from xmlrpc_test import XMLRPC_test, assert_attr_equal from ipalib import api from ipalib import errors @@ -77,16 +79,12 @@ class test_automount(XMLRPC_test): assert res assert_attr_equal(res, 'automountkey', self.keyname) +@raises(errors.DuplicateEntry) def test_4_automountkey_add(self): Test adding a duplicate key using `xmlrpc.automountkey_add` method. -try: -api.Command['automountkey_add'](self.locname, self.mapname, **self.key_kw) -except errors.DuplicateEntry: -pass -else: -assert False +res = api.Command['automountkey_add'](self.locname, self.mapname, **self.key_kw) def test_5_automountmap_show(self): @@ -153,12 +151,8 @@ class test_automount(XMLRPC_test): assert_attr_equal(res, 'failed', '') # Verify that it is gone -try: +with assert_raises(errors.NotFound):
Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
Jan Cholasta wrote:
On 29.2.2012 15:50, 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.
Is that so? Isn't *attr supposed to allow the user to modify attributes
at LDAP level, i.e. skip the usual framework constraints?
If we make rules around an attribute they should be honored. If we have
not then all bets are off.
*attr was never really made to manage those attributes that IPA knows
about, despite most of the testing being around that. It was to provide
a way to manage things we don't support yet.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
I may be taking things out of context, but I see this: On 03/16/2012 02:07 PM, Rob Crittenden wrote: Jan Cholasta wrote: On 29.2.2012 15:50, Rob Crittenden wrote: Petr Viktorin wrote: On 02/27/2012 11:03 PM, Rob Crittenden wrote: .. snip .. Patch 17 makes these options honor params marked no_create and no_update. .. snip .. *attr is specifically made to be powerful. We don't want to arbitrarily block updating certain values. .. versus .. 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. .. snip .. 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. Is that so? Isn't *attr supposed to allow the user to modify attributes at LDAP level, i.e. skip the usual framework constraints? If we make rules around an attribute they should be honored. If we have not then all bets are off. *attr was never really made to manage those attributes that IPA knows about, despite most of the testing being around that. It was to provide a way to manage things we don't support yet. which strikes me as inconsistent. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
On 16.3.2012 14:14, Petr Viktorin wrote: I may be taking things out of context, but I see this: On 03/16/2012 02:07 PM, Rob Crittenden wrote: Jan Cholasta wrote: On 29.2.2012 15:50, Rob Crittenden wrote: Petr Viktorin wrote: On 02/27/2012 11:03 PM, Rob Crittenden wrote: .. snip .. Patch 17 makes these options honor params marked no_create and no_update. .. snip .. *attr is specifically made to be powerful. We don't want to arbitrarily block updating certain values. .. versus .. 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. .. snip .. 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. Is that so? Isn't *attr supposed to allow the user to modify attributes at LDAP level, i.e. skip the usual framework constraints? If we make rules around an attribute they should be honored. If we have not then all bets are off. *attr was never really made to manage those attributes that IPA knows about, despite most of the testing being around that. It was to provide a way to manage things we don't support yet. which strikes me as inconsistent. Yes, exactly. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
Petr Viktorin wrote: I may be taking things out of context, but I see this: On 03/16/2012 02:07 PM, Rob Crittenden wrote: Jan Cholasta wrote: On 29.2.2012 15:50, Rob Crittenden wrote: Petr Viktorin wrote: On 02/27/2012 11:03 PM, Rob Crittenden wrote: .. snip .. Patch 17 makes these options honor params marked no_create and no_update. .. snip .. *attr is specifically made to be powerful. We don't want to arbitrarily block updating certain values. .. versus .. 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. .. snip .. 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. Is that so? Isn't *attr supposed to allow the user to modify attributes at LDAP level, i.e. skip the usual framework constraints? If we make rules around an attribute they should be honored. If we have not then all bets are off. *attr was never really made to manage those attributes that IPA knows about, despite most of the testing being around that. It was to provide a way to manage things we don't support yet. which strikes me as inconsistent. The original patch read to me that you were creating a new class of parameters that could not be updated via *attr. IMHO that was going too far. Some constraints are required. We don't want someone doing: ipa user-add --first=Super --last=Bad superbad --setattr uidnumber=0 Similarly we impose a certain view of the data to fit our purposes. The right solution in this case is to change the schema of ipaCertificateSubjectBase. But think about it. What does it mean if someone can add multiple values to parameters that IPA otherwise considers single-value? Undefined. I don't think it is unreasonable to enforce the parameter's single value. A user can always use ldapmodify if they really need to set that second value. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0016-17 Fixes for{add, set, del}attr with managed attributes
On 03/16/2012 02:34 PM, Rob Crittenden wrote: Petr Viktorin wrote: I may be taking things out of context, but I see this: On 03/16/2012 02:07 PM, Rob Crittenden wrote: Jan Cholasta wrote: On 29.2.2012 15:50, Rob Crittenden wrote: Petr Viktorin wrote: On 02/27/2012 11:03 PM, Rob Crittenden wrote: .. snip .. Patch 17 makes these options honor params marked no_create and no_update. .. snip .. *attr is specifically made to be powerful. We don't want to arbitrarily block updating certain values. .. versus .. 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. .. snip .. 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. Is that so? Isn't *attr supposed to allow the user to modify attributes at LDAP level, i.e. skip the usual framework constraints? If we make rules around an attribute they should be honored. If we have not then all bets are off. *attr was never really made to manage those attributes that IPA knows about, despite most of the testing being around that. It was to provide a way to manage things we don't support yet. which strikes me as inconsistent. The original patch read to me that you were creating a new class of parameters that could not be updated via *attr. IMHO that was going too far. Not creating a new class, just using what's already marked no_update. Some constraints are required. We don't want someone doing: ipa user-add --first=Super --last=Bad superbad --setattr uidnumber=0 Similarly we impose a certain view of the data to fit our purposes. The right solution in this case is to change the schema of ipaCertificateSubjectBase. But think about it. What does it mean if someone can add multiple values to parameters that IPA otherwise considers single-value? Undefined. I don't think it is unreasonable to enforce the parameter's single value. A user can always use ldapmodify if they really need to set that second value. I thought *attr was supposed to have the power of ldapmodify (for IPA-managed entries)? Anyway, you're right that this should be changed in the schema. But maybe since the checking should be done at the LDAP server, we shouldn't need to put another checking layer here (more code, more corner cases, more bugs). Alternatively, just disable *attr for known attributes. The users can always use ldapmodify if they know what they're doing, and if it's not possible with normal IPA commands it's a case where they do need to know what they're doing (and by having to use ldapmodify, be warned that they're on their own). -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0016 Fixes for{add, set, del}attr with managed attributes
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.
--
Petr³
From 9ae82b294b75efad3affe3ca579913e9296c1aab 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 | 39
tests/test_xmlrpc/test_attr.py | 78
2 files changed, 101 insertions(+), 16 deletions(-)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 9562ff98729ead6ac9e56d504f6ee0a7c0ca377a..df617a4f58982cd3160d95ac3b52aacbd82bc74a 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)
-
Re: [Freeipa-devel] [PATCH] 107 Content is no more overwritten by error message
On 03/13/2012 10:54 PM, Endi Sukma Dewata wrote: On 3/9/2012 7:16 AM, Petr Vobornik wrote: When an error which caused calling of report_error occur, the content of a facet got replaced by error message. There was no way how to force the facet to recreate its content and the facet became unusable. This patch creates a container for an error message. On error, report_error writes its content to error container, content container is hidden and error container is shown. Older comment in a code suggested to move the error message to facet's footer. A message in a footer could be missed by the user and on top of that a footer is sometimes used by various facet and we would have to solve the same problem again. From experience the cause of an error is usually a missing pkey in a path. Therefore error information suggests user to navigate to top level. It causes to load default facets with default values so errors in navigation state shouldn't happen. Facet content is displayed back on facet_show. If user tries to display same object as before facet's need_update() would return false, therefore need_update was modified to always return true if error is displayed. Also it may be possible to display facet content on refresh success. I tried to avoid that because it would require putting show_content calls to various success handlers or load methods. It would add one more thing developer needs to think of when overriding refresh or load method. Reproduction: 1) display any nested entity - ie DNS record 2) delete its parent pkey from path - dnszone-pkey=example.com 3) reload the page with this path https://fedorahosted.org/freeipa/ticket/2449 In the ticket I added 2 more scenarios to reproduce the problem. So we have 3 possible cases: 1. invalid UI state 2. non-existent entry 3. server down For case #1, the patch provides a much better message, but I think ideally if some parameters are missing from the URL (e.g. the parent pkey) it should be detected by the UI before sending the request to the server. This probably should be addressed in a separate ticket. See the note below about the error message. For case #2, the patch fixes the issue by clearing up the error message. This works on all entities except users because the user details page uses a batch operation to get the data and it doesn't handle non-existent users properly. I think this is an existing and separate issue. For case #3, the patch will show a message saying that the UI got into an invalid state, which is actually not the case here. Also, returning to the main page or reloading the page wouldn't solve the problem either. So for this ticket I think it would be better to show a more generic error message, something like this: Reworked. An error has occured (IPA Error 3007) 'idnsname' is required Please try the following options: * Refresh the page. (see note below) * Return to the main page and retry the operation. We are talking about main page. What is it? Identity/Users? Navigation code operates also with currently displayed facet. So when I now navigate to '#' (empty state) it won't have to be Identity/Users. The good part is that it navigates to page in a branch where user was operating. * Reload the browser. If the problem persists please contact the system administrator. Each of the above options could be made into a link that does the mentioned operation. It would be great if we can use the Refresh button to clear the error message. If this requires significant effort we probably can remove this option from the message above and add it in a separate ticket. The patch is quite short. I was more concerned about the fact that when overriding stuff, developer would have to think about one more thing. The UI is getting more and more complex. But it might not be as a big problem as I originally thought. I put the call to refresh success handler, mainly because report_error is in error handler, and these handlers aren't overridden often. Attached as separate patch. One more thing, this may not be a problem now, but the error_container uses both facet-content and facet-error CSS classes. I understand this is done to avoid code duplication, but this also means the facet will have 2 facet-contents. CSS classes can be used for decorative or structural purposes or both, so we need to make sure decorative changes will not affect it structurally. One solution is to duplicate the CSS code from facet-content into facet-error. Another solution is to use a separate decorative class that are added into both facet-content and facet-error elements. It is little bit more difficult. If I look at it structurally the error div is facet-content too. So the facet has two contents - proper and error. Is it OK? Does it break some design principle. If so, would it be better to have separate error_facet? I think it is not good to have two contents but current implementation is not ready for separate error_facet - navigation code
Re: [Freeipa-devel] [PATCH] 0016 Fixes for{add, set, del}attr with managed attributes
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
The encoding problem does still exist too:
$ ipa config-mod --setattr ipamigrationenabled=false
ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax: Invalid
syntax.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0016 Fixes for{add, set, del}attr with managed attributes
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...
The encoding problem does still exist too:
$ ipa config-mod --setattr ipamigrationenabled=false
ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax: Invalid
syntax.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0016 Fixes for{add, set, del}attr with managed attributes
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.
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.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0016 Fixes for{add, set, del}attr with managed attributes
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
[Freeipa-devel] [PATCH] 989 import plugins on context, not in_server
in_server controls how messages are dispatched. We should import on context.
This prevents the error message session memcached servers not running
in ipa-ldap-updater and ipa-replica-manage.
rob
From be57f22856bfe95e0bd19d8b41f19dfd83153384 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Fri, 16 Mar 2012 10:26:17 -0400
Subject: [PATCH] Import the ipaserver plugins based on context, not
env.in_server.
in_server controls how a method is dispatched, it should not also control
what plugins are imported.
This suppresses the error message session memcached servers not running.
https://fedorahosted.org/freeipa/ticket/2499
---
ipalib/plugable.py |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index 4d0011029573df44d8d5e85e0e2b2a3f872c0703..293db9241d55bc664268908bc75b7f67022259de 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -596,7 +596,7 @@ class API(DictProxy):
if self.env.mode in ('dummy', 'unit_test'):
return
self.import_plugins('ipalib')
-if self.env.in_server:
+if self.env.context in ('server', 'lite'):
self.import_plugins('ipaserver')
if self.env.context in ('installer', 'updates'):
self.import_plugins('ipaserver/install/plugins')
--
1.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 990 attribute name in exceptions
When using *attr we should return the param.name of in the exception and
when using a cli option we should return param.cli_name. This didn't
work consistently in the framework.
This is a bit of a kludge, catching exceptions and re-raising them, but
it is a less invasive way of doing it.
I added some examples of things to test in the ticket.
rob
From 461ce7dfa9d2d4a473d949d4f5ec523ba77b3dd7 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Fri, 16 Mar 2012 13:30:59 -0400
Subject: [PATCH] Use a consistent parameter name in errors, defaulting to
cli_name.
For general command-line errors we want to use the cli_name on output.
The exception is when using *attr, we want to return that attribute name
in the exception.
https://fedorahosted.org/freeipa/ticket/1418
---
ipalib/parameters.py | 35 +--
ipalib/plugins/baseldap.py | 11 +--
2 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 48155daf9bc69f6b50f390737b8b69a2c84e12a2..94f11d912c010fb644d9683d7630a2d753f06371 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -563,6 +563,18 @@ class Param(ReadOnly):
self.validate(value, supplied=self.name in kw)
return value
+def get_param_name(self):
+
+Return the right name of an attribute depending on usage.
+
+Normally errors should use cli_name, our friendly name. When
+using the API directly or *attr return the real name.
+
+name = self.cli_name
+if not name:
+name = self.name
+return name
+
def kw(self):
Iterate through ``(key,value)`` for all kwargs passed to constructor.
@@ -861,11 +873,8 @@ class Param(ReadOnly):
for rule in self.all_rules:
error = rule(ugettext, value)
if error is not None:
-name = self.cli_name
-if not name:
-name = self.name
raise ValidationError(
-name=name,
+name=self.get_param_name(),
value=value,
index=index,
error=error,
@@ -1175,7 +1184,7 @@ class Int(Number):
return int(value)
except ValueError:
pass
-raise ConversionError(name=self.name, index=index,
+raise ConversionError(name=self.get_param_name(), index=index,
error=ugettext(self.type_error),
)
@@ -1218,11 +1227,8 @@ class Int(Number):
for rule in self.all_rules:
error = rule(ugettext, value)
if error is not None:
-name = self.cli_name
-if not name:
-name = self.name
raise ValidationError(
-name=name,
+name=self.get_param_name(),
value=value,
index=index,
error=error,
@@ -1309,7 +1315,7 @@ class Decimal(Number):
try:
value = decimal.Decimal(value)
except Exception, e:
-raise ConversionError(name=self.name, index=index,
+raise ConversionError(name=self.get_param_name(), index=index,
error=unicode(e))
if isinstance(value, decimal.Decimal):
@@ -1449,7 +1455,7 @@ class Bytes(Data):
try:
value = base64.b64decode(value)
except TypeError:
-raise ConversionError(name=self.name, index=index, error=self.type_error)
+raise ConversionError(name=self.get_param_name(), index=index, error=self.type_error)
return super(Bytes, self)._convert_scalar(value, index)
@@ -1548,7 +1554,8 @@ class IA5Str(Str):
if isinstance(value, basestring):
for i in xrange(len(value)):
if ord(value[i]) 127:
-raise ConversionError(name=self.name, index=index,
+raise ConversionError(name=self.get_param_name(),
+index=index,
error=_('The character \'%(char)r\' is not allowed.') %
dict(char=value[i],)
)
@@ -1832,10 +1839,10 @@ class AccessTime(Str):
try:
self._check(value)
except ValueError, e:
-raise ValidationError(name=self.cli_name, error=e.args[0])
+raise ValidationError(name=self.get_param_name(), error=e.args[0])
except IndexError:
raise ValidationError(
-name=self.cli_name, error='incomplete time value'
+name=self.get_param_name(), error='incomplete time value'
)
return None
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index
[Freeipa-devel] [PATCH] 349 Fixed boot.ldif permission.
The server installation failed on F17 due to permission problem. The /var/lib/dirsrv/boot.ldif was previously owned and only readable by root. It is now owned by DS user dirsrv. Ticket #2544 -- Endi S. Dewata From 0a9b9c026938a2879cd3d9ed54a14b1afc96f1f0 Mon Sep 17 00:00:00 2001 From: Endi Sukma Dewata edew...@redhat.com Date: Fri, 16 Mar 2012 17:15:26 -0500 Subject: [PATCH] Fixed boot.ldif permission. The server installation failed on F17 due to permission problem. The /var/lib/dirsrv/boot.ldif was previously owned and only readable by root. It is now owned by DS user dirsrv. Ticket #2544 --- ipaserver/install/dsinstance.py |3 +++ 1 file changed, 3 insertions(+) diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py index e549e13ccad711da8fbde359f9263f5828c6d5a0..d82454d04c9482f07879ff1b9296754be3c2a833 100644 --- a/ipaserver/install/dsinstance.py +++ b/ipaserver/install/dsinstance.py @@ -305,6 +305,8 @@ class DsInstance(service.Service): root_logger.critical(failed to add user %s % e) def __create_instance(self): +pent = pwd.getpwnam(DS_USER) + self.backup_state(running, is_ds_running()) self.backup_state(serverid, self.serverid) self.fstore.backup_file(/etc/sysconfig/dirsrv) @@ -320,6 +322,7 @@ class DsInstance(service.Service): # Must be readable for dirsrv os.chmod(target_fname, 0440) +os.chown(target_fname, pent.pw_uid, pent.pw_gid) inf_txt = ipautil.template_str(INF_TEMPLATE, self.sub_dict) root_logger.debug(writing inf template) -- 1.7.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
