<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
On 03/21/2012 10:28 AM, Petr Viktorin wrote:
<blockquote cite="mid:4F69E585.1070108@redhat.com" type="cite">On
03/20/2012 10:08 PM, Rob Crittenden wrote:
<br>
<blockquote type="cite">Petr Viktorin wrote:
<br>
<blockquote type="cite">On 03/16/2012 12:55 PM, Petr Viktorin
wrote:
<br>
<blockquote type="cite">On 03/15/2012 08:55 PM, Rob Crittenden
wrote:
<br>
<blockquote type="cite">Petr Viktorin wrote:
<br>
<blockquote type="cite"><a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/2227">https://fedorahosted.org/freeipa/ticket/2227</a>
(Unable to add certain
<br>
sudo
<br>
commands to groups). What an interesting bug to get :)
<br>
<br>
<br>
One problem with our CSV splitting is that it's not
idempotent
<br>
(baskslashes are "eaten" when there are escaped commas),
but when we
<br>
forward a call it gets done on both the client and the
server. The
<br>
attached patch fixes that in a somewhat hacky way. It's
not a complete
<br>
fix for the issue though, for that I need to ask what to
do.
<br>
<br>
<br>
Some Params use the CSV format when we just need a list
of
<br>
comma-separated values. Our flavor of CSV does escaping
in two
<br>
different
<br>
ways. This is pretty bad for predictability,
least-surprise,
<br>
documentation, ...
<br>
<br>
To recap, the two ways are escaping (escaped commas are
ignored,
<br>
backslashes also need to be escaped) and quoting (values
are
<br>
optionally
<br>
enclosed in double quotes, to include a '"' in a quoted
string, use
<br>
'""').
<br>
Escaping is good because users are used to it, but
currently literal
<br>
backslashes need to be doubled, making multivalue syntax
different
<br>
from
<br>
single-value syntax, even if there are no commas in the
value.
<br>
Quoting is weird, but complete by itself so it doesn't
also need
<br>
escaping.
<br>
<br>
Do we need to use both? Is this documented somewhere?
Some of our
<br>
tests
<br>
check only for one, some assume the other. Users are
probably even
<br>
more
<br>
confused.
<br>
<br>
<br>
If we only keep one of those, the fix for #2227 should
be quite easy.
<br>
If not (backwards compatibility), we need to document
this properly,
<br>
test all the corner cases, and fix the UI to handle CSV
escaping.
<br>
Since it's subtly broken in lots of places, maybe it's
best to break
<br>
backwards compatibility and go for a simple solution
now.
<br>
<br>
Anyway, if I want to do a proper fix, CSV handling
should be only done
<br>
on the client. Unfortunately turning off CSV handling in
the server
<br>
will
<br>
break the UI, which at places uses `join(',')` (no
escaping commas, no
<br>
single place to change it), even though it can just send
a list.
<br>
</blockquote>
<br>
I'm with Honza, not too keen on the _forwarded_call part.
I'd rather
<br>
you
<br>
do a mix of comparing self.env.in_server and whether the
value is a
<br>
basestring to determine if we need to split it.
<br>
</blockquote>
<br>
This was a hack necessary because the WebUI relied on
server-side
<br>
splitting (in a few cases, and it did not escape correctly).
Now that
<br>
Petr Vobornik's patch 99 is in, it is unnecessary.
<br>
This discussion thread has an updated patch (0015-03); what
I'm
<br>
attaching now builds on top of that to add several new
changes in tests.
<br>
To summarize it: the splitting is only done in the client;
from the RPC
<br>
call on no CSV magic is involved at all.
<br>
<br>
Please tell me your reasons for basestring checking. I think
it's
<br>
entirely unnecessary and just adds complexity.
<br>
In the rest of the framework a string, as a parameter value,
has the
<br>
same effect as a list of that one string. Why break this?
<br>
We are concerned about the API here, not the command line.
Shortcuts to
<br>
save the user's keystrokes in the common case should *not*
win over
<br>
predictability and making sure the easier way is the
correct, robust way
<br>
to do it. Compare -- with basestring checking:
<br>
<br>
some_command(arg=escape_commas(some_string))
<br>
- is correct
<br>
<br>
some_command(arg=[some_string])
<br>
- is also correct
<br>
<br>
some_command(arg=some_string)
<br>
- is WRONG because the string is not escaped
<br>
- but is the easiest solution!
<br>
<br>
The problem is much worse because it is subtle: it only
affects values
<br>
with commas and backslashes, so *the wrong solution would
work* in most
<br>
of the cases.
<br>
And I can assure you plugin writers *would* pick the wrong
way if it
<br>
would usually work. Our own web UI had examples.
<br>
<br>
Contrast to my proposed behavior:
<br>
some_command(arg=some_string)
<br>
- is correct
<br>
<br>
some_command(arg=[some_string])
<br>
- is also correct
<br>
<br>
some_command(arg=escape_commas(some_string))
<br>
- is wrong, but doesn't make much sense because at this
level you don't
<br>
have to worry about CSV at all
<br>
<br>
Basestring checking would add unnecessary complexity for
both us *and*
<br>
the plugin writer. The only case it would make it easier for
the plugin
<br>
writer is if the plugin took CSV values -- but why force a
random
<br>
format, tailored for our specific frontend, into RPC
clients?
<br>
<br>
The client should translate the command line into a RPC
call. There's no
<br>
reason to arbitrarily expect the server to do part of the
job.
<br>
<br>
<br>
<blockquote type="cite">I'm not sure why this is broken out
of normalize. Isn't this
<br>
normalizing
<br>
the incoming values into something more sane?
<br>
</blockquote>
<br>
"Normalization" implies it's an idempotent operation, which
CSV
<br>
splitting is not (at least its current incarnation, without
the
<br>
basestring checking):
<br>
r"a,b\,c" splits to ["a", "b,c"] which splits to ["a", "b",
"c"]
<br>
<br>
<br>
</blockquote>
<br>
Just to make things clear: freeipa-pviktori-0015-04 is the
latest
<br>
version of this patch.
<br>
It's complemented by freeipa-pviktori-0021-03 which adds the
tests for
<br>
this (and other things) (but contains one more refactor).
<br>
<br>
<br>
If and when that's all reviewed and pushed, I'd like to
discuss
<br>
freeipa-pviktori-0018 (Simplify CSV escaping syntax). I'm not
saying my
<br>
solution there is perfect, but the flavor of Python's CSV
parsing we're
<br>
using now is quite quirky. It would be nice to get the syntax
right, or
<br>
at least better, while we're fixing this from a totally-broken
state.
<br>
Later there'll be backwards compatibility problems.
<br>
But, don't let that hold back this patch.
<br>
<br>
</blockquote>
<br>
I'm still not completely convinced. This patch looks ok and I'm
sold on
<br>
client side only parsing but this doesn't actually fix any of
the CSV
<br>
bugs we have open.
<br>
<br>
I'd like to evaluate this in more context.
<br>
<br>
I think the simplest fix is to drop escapechar processing in the
<br>
csvreader and have users quote strings with commas instead, this
is how
<br>
I originally intended it to work. The UI won't have to do
anything
<br>
special since it will already be split and it isn't a lot to ask
to put
<br>
quotes around a value on the CLI.
<br>
<br>
This is my proposed fix on top of your patch:
<br>
<br>
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
<br>
index 80b175d..ec6020e 100644
<br>
--- a/ipalib/parameters.py
<br>
+++ b/ipalib/parameters.py
<br>
@@ -702,7 +702,7 @@ class Param(ReadOnly):
<br>
# csv.py doesn't do Unicode; encode temporarily as UTF-8:
<br>
csv_reader = csv.reader(self.__utf_8_encoder(unicode_csv_data),
<br>
dialect=dialect,
<br>
- delimiter=self.csv_separator, escapechar='\\',
<br>
+ delimiter=self.csv_separator, quotechar='"',
<br>
skipinitialspace=self.csv_skipspace,
<br>
**kwargs)
<br>
for row in csv_reader:
<br>
<br>
With this change:
<br>
- all the self-tests pass
<br>
- <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/2417">https://fedorahosted.org/freeipa/ticket/2417</a> is fixed
<br>
- <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/2227">https://fedorahosted.org/freeipa/ticket/2227</a> is fixed
<br>
<br>
I tested with:
<br>
<br>
$ ipa sudocmdgroup-add-member test --sudocmd='/bin/chown -R
<br>
apache\:developers /var/www/*/shared/log'
<br>
<br>
$ ipa role-add-privilege --privileges='"u,r stuff"' test
<br>
<br>
rob
<br>
</blockquote>
<br>
Yes, that works well. ACK on your change; here it is squashed in.
<br>
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/2402">https://fedorahosted.org/freeipa/ticket/2402</a> can wait for 3.0,
then?
<br>
</blockquote>
<br>
Is it risky to take it in?<br>
<br>
<blockquote cite="mid:4F69E585.1070108@redhat.com" type="cite">
<br>
<pre wrap="">
<fieldset class="mimeAttachmentHeader"></fieldset>
_______________________________________________
Freeipa-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
<a class="moz-txt-link-abbreviated" href="http://www.redhat.com/carveoutcosts/">www.redhat.com/carveoutcosts/</a>
</pre>
</body>
</html>