Re: [Freeipa-devel] [PATCH] 0015 Only split CSV strings once
On 03/20/2012 10:08 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/16/2012 12:55 PM, Petr Viktorin wrote:
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]
Just to make things clear: freeipa-pviktori-0015-04 is the latest
version of this patch.
It's complemented by freeipa-pviktori-0021-03 which adds the tests for
this (and other things) (but contains one more refactor).
If and when that's
Re: [Freeipa-devel] [PATCH] 0015 Only split CSV strings once
On 03/16/2012 12:55 PM, Petr Viktorin wrote:
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]
Just to make things clear: freeipa-pviktori-0015-04 is the latest
version of this patch.
It's complemented by freeipa-pviktori-0021-03 which adds the tests for
this (and other things) (but contains one more refactor).
If and when that's all reviewed and pushed, I'd like to discuss
freeipa-pviktori-0018
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] 0015 Only split CSV strings once
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.
I'm not sure why this is broken out of normalize. Isn't this normalizing
the incoming values into something more sane?
rob
___
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 (updated)
On 02/29/2012 07:13 PM, Petr Vobornik wrote: On 02/27/2012 02:01 PM, Petr Viktorin wrote: It seems I didn't communicate the problem and my solution clearly enough, so let me try again. (Also, I learned from the discussions!) Currently, both the client and the server parse CSV options. The client does *not* re-encode the CSV before sending; the parsing is really done twice. This means e.g. that you need 3 backslashes to escape a literal comma: after the client-side split, '\\\,' becomes '\,'; which after the server-side split becomes ','. Since CSV is specific to the command-line, and the client is responsible for translating command-line input to XML-RPC (which has its own syntax for lists), the ideal fix will be to move CSV processing entirely to the client. This will be a rather invasive change, mainly because some parts of the UI now expect the server-side parsing (but they don't escape CSV, so values containing commas or backslashes are broken). So it won't make it to the upcoming release. My patch provides a quick fix: when a call comes from the command-line client, disable the server-side parsing. I investigated all occurrences of CSV creation in Web UI. I removed them and UI is working fine. The patch is on the list: pvoborni 099. So your patch shouldn't affect UI if my patch is applied. I can't get away from moving split_csv() (which is not idempotent) out of normalize() (which is, and gets called lots of times); this is the patch's major change in therms of LOC. I'll note again that this only affects values with backslashes or double quotes. Exactly these options are currently broken (=need double escaping). The normal uses of CSV are completely unaffected. Attaching updated patch; the change vs. the original is that the don't parse again flag is now only set at the server, when a XMLRPC call is received, making the client fully forward-compatible (the flag doesn't get sent through the wire). The ticket is https://fedorahosted.org/freeipa/ticket/2227, but this patch is only the first step in fixing it. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel The webUI patch is in, and also I heard this patch is not making it to the release anyway, so the workaround makes little sense. I'd like to go for the real fix. Meanwhile I found some other bugs (https://fedorahosted.org/freeipa/ticket/2482, https://fedorahosted.org/freeipa/ticket/2483) that prevent me from testing this throroughly. Self-NACK for now. -- Petr³ ___ 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 (updated)
On 02/27/2012 02:01 PM, Petr Viktorin wrote: It seems I didn't communicate the problem and my solution clearly enough, so let me try again. (Also, I learned from the discussions!) Currently, both the client and the server parse CSV options. The client does *not* re-encode the CSV before sending; the parsing is really done twice. This means e.g. that you need 3 backslashes to escape a literal comma: after the client-side split, '\\\,' becomes '\,'; which after the server-side split becomes ','. Since CSV is specific to the command-line, and the client is responsible for translating command-line input to XML-RPC (which has its own syntax for lists), the ideal fix will be to move CSV processing entirely to the client. This will be a rather invasive change, mainly because some parts of the UI now expect the server-side parsing (but they don't escape CSV, so values containing commas or backslashes are broken). So it won't make it to the upcoming release. My patch provides a quick fix: when a call comes from the command-line client, disable the server-side parsing. I investigated all occurrences of CSV creation in Web UI. I removed them and UI is working fine. The patch is on the list: pvoborni 099. So your patch shouldn't affect UI if my patch is applied. I can't get away from moving split_csv() (which is not idempotent) out of normalize() (which is, and gets called lots of times); this is the patch's major change in therms of LOC. I'll note again that this only affects values with backslashes or double quotes. Exactly these options are currently broken (=need double escaping). The normal uses of CSV are completely unaffected. Attaching updated patch; the change vs. the original is that the don't parse again flag is now only set at the server, when a XMLRPC call is received, making the client fully forward-compatible (the flag doesn't get sent through the wire). The ticket is https://fedorahosted.org/freeipa/ticket/2227, but this patch is only the first step in fixing it. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- Petr Vobornik ___ 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 02/23/2012 04:08 PM, Jan Cholasta wrote:
Can't you just re-escape the values before forwarding the call? That
would be a fairly straightforward fix and it would remove the need for
all the _forwarded_call hackery.
All right, I'll do that once we decide on how to escape. Self-NACK for
the patch.
...
I think there was a discussion going on about removing the quoting
sometime in the past.
Was there an agreement? Was backwards compatibility the only problem?
...
On 02/23/2012 04:14 PM, Martin Kosek wrote:
On Thu, 2012-02-23 at 16:08 +0100, Jan Cholasta wrote:
On 23.2.2012 15:29, Petr Viktorin wrote:
...
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.
+1, but I'm not sure if that's acceptable (see
http://www.redhat.com/archives/freeipa-devel/2012-January/msg00197.html)
There's still only one place to change.
And since this is input, not output; and we want backwards
compatibility, a formatting change is pretty hard to push through as it
is :)
I tend to like this solution as well, it is true that CSV parsing is
really a client thing. Our life as a server would be much easier if we
just accept scalar values in an array, either from XMLRPC or JSON
interface. It is doable, but it would break IPAv2.x clients which then
would not be able to use CSV formatted values at all. And we want to
avoid that.
Martin
Keep in mind that currently the parsing is done on *both* sides. The CSV
splitting is first done at the client, then validated at the client, and
then split and validated on the server. For example (without my patch):
--option='a,b,c,de' --option='a\\\,b,c\\d'
Client gets (r'a\,b,c,de', r'a\\\,b,c\\d')
Cli splits to ('a,b' 'c', 'de' r'a\,b', r'c\d'), validates, forwards
Srv splits to ('a', 'b', 'c', 'de', r'a,b', r'cd'), validates, executes
You need four backslashes for a literal backslash, three to escape a
comma. I think 2.1 clients are already broken, and the backwards
incompatibility would only affect workarounds.
As far as I can see the Web UI is tied to the server version, so
changing the JSON communication shouldn't be a problem?
Anyway, since the JSON code for this is scattered across the codebase,
we need to introduce a common JS function first, and only then switch
both that and the server to use plain arrays.
--
Petr³
___
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 02/24/2012 11:09 AM, Petr Viktorin wrote: On 02/23/2012 04:08 PM, Jan Cholasta wrote: Can't you just re-escape the values before forwarding the call? That would be a fairly straightforward fix and it would remove the need for all the _forwarded_call hackery. All right, I'll do that once we decide on how to escape. Self-NACK for the patch. ... I hit Send too early. Re-escaping the values would be a bad thing to do, because it would mean the server would still need to unescape. If I did that, we couldn't fix the problem properly later. The proper fix is to do CSV splitting only at the client. Just like it converts to None or multiple options to lists, it should convert CSV to lists. There's no need for the backend to worry itself over a CLI-specific encoding. -- Petr³ ___ 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 Fri, 2012-02-24 at 11:09 +0100, Petr Viktorin wrote: ... You need four backslashes for a literal backslash, three to escape a comma. I think 2.1 clients are already broken, and the backwards incompatibility would only affect workarounds. Yes, but CSV values without escaping works. And this IMO covers 99% of user cases, especially given the fact that escaping is that difficult to use. Users can simply not use characters that need escaping. We cannot break commands like this one: ipa dnsrecord-add example.com foo --a-rec=10.0.0.1,10.0.0.2 ipa user-mod --phone=555-1234,555-6789 I would be OK with changing CSV formatting if it supports both ways: 1) Plain arrays from new clients where CSV parsing is done just once (only on clients) 2) CSV value which is then parsed on the server Martin ___ 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 02/24/2012 11:09 AM, Petr Viktorin wrote:
As far as I can see the Web UI is tied to the server version, so
changing the JSON communication shouldn't be a problem?
Anyway, since the JSON code for this is scattered across the codebase,
we need to introduce a common JS function first, and only then switch
both that and the server to use plain arrays.
Right, changes shouldn't be a big problem for UI.
Web UI isn't doing any splitting. It gets its values as arrays.
For mod commands UI by default sends an array but for some cases (ACI,
DNS, associations) it uses .join(',').
--
Petr Vobornik
___
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 02/24/2012 12:09 PM, Martin Kosek wrote: On Fri, 2012-02-24 at 11:09 +0100, Petr Viktorin wrote: ... You need four backslashes for a literal backslash, three to escape a comma. I think 2.1 clients are already broken, and the backwards incompatibility would only affect workarounds. Yes, but CSV values without escaping works. And this IMO covers 99% of user cases, especially given the fact that escaping is that difficult to use. Users can simply not use characters that need escaping. CSV values without backslashes and initial double quotes would not be affected at all. We cannot break commands like this one: ipa dnsrecord-add example.com foo --a-rec=10.0.0.1,10.0.0.2 ipa user-mod --phone=555-1234,555-6789 I also don't want to break commands like this: ipa [...] --a-rec=10.0.0.1,10.0.0.2 --a-rec=10.0.0.3 I would be OK with changing CSV formatting if it supports both ways: 1) Plain arrays from new clients where CSV parsing is done just once (only on clients) Old clients *already send* plain arrays; but the server currently errorneously parses each part again. Maybe a better fix for now would be to set the don't parse again flag on the XMLRPC receiving code, instead of the client. That way old and new clients would behave identically. 2) CSV value which is then parsed on the server Is there any reason at all to do CSV parsing on the server, for the CLI/XMLRPC case? I can't see it. --- Petr³ ___ 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
Petr Viktorin wrote: On 02/24/2012 12:09 PM, Martin Kosek wrote: On Fri, 2012-02-24 at 11:09 +0100, Petr Viktorin wrote: ... You need four backslashes for a literal backslash, three to escape a comma. I think 2.1 clients are already broken, and the backwards incompatibility would only affect workarounds. Yes, but CSV values without escaping works. And this IMO covers 99% of user cases, especially given the fact that escaping is that difficult to use. Users can simply not use characters that need escaping. CSV values without backslashes and initial double quotes would not be affected at all. We cannot break commands like this one: ipa dnsrecord-add example.com foo --a-rec=10.0.0.1,10.0.0.2 ipa user-mod --phone=555-1234,555-6789 I also don't want to break commands like this: ipa [...] --a-rec=10.0.0.1,10.0.0.2 --a-rec=10.0.0.3 I would be OK with changing CSV formatting if it supports both ways: 1) Plain arrays from new clients where CSV parsing is done just once (only on clients) Old clients *already send* plain arrays; but the server currently errorneously parses each part again. Maybe a better fix for now would be to set the don't parse again flag on the XMLRPC receiving code, instead of the client. That way old and new clients would behave identically. 2) CSV value which is then parsed on the server Is there any reason at all to do CSV parsing on the server, for the CLI/XMLRPC case? I can't see it. Because we want people to be able to write their own client and need to be able to handle whatever they send. rob ___ 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 02/24/2012 03:08 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/24/2012 12:09 PM, Martin Kosek wrote: On Fri, 2012-02-24 at 11:09 +0100, Petr Viktorin wrote: ... Old clients *already send* plain arrays; but the server currently errorneously parses each part again. Maybe a better fix for now would be to set the don't parse again flag on the XMLRPC receiving code, instead of the client. That way old and new clients would behave identically. 2) CSV value which is then parsed on the server Is there any reason at all to do CSV parsing on the server, for the CLI/XMLRPC case? I can't see it. Because we want people to be able to write their own client and need to be able to handle whatever they send. rob Handle whatever they send? That sounds like we should try to read their minds. I think we should only allow well-structured XMLRPC calls. The format already has a syntax for lists, why force clients to *also* support CSV (e.g. escape commas)? It's a burden for both sides. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0015 Only split CSV strings once
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.
--
Petr³
From 694c81cd6d5ace2115dd89ddbe04d5935be1b7fe Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 23 Feb 2012 07:29:47 -0500
Subject: [PATCH] Only split CSV strings once
Splitting on commas is not an idempotent operation:
'a,b\,c' - ('a', 'b,c') - ('a', 'b', 'c')
That means we can't do it when the call is forwarded.
Also, document Param's csv arguments
---
ipalib/frontend.py | 26 +
ipalib/parameters.py | 50 +++---
tests/test_ipalib/test_parameters.py | 24
3 files changed, 78 insertions(+), 22 deletions(-)
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index da25b4c1aef100cb54d7e248ba4e2ea5dc250cef..6f3a5ded7a3b59d76a8f3cec9548b606fb472465 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -414,6 +414,7 @@ class Command(HasParam):
If not in a server context, the call will be forwarded over
XML-RPC and the executed an the nearest IPA server.
+forwarded = options.pop('_forwarded_call', False)
self.ensure_finalized()
params = self.args_options_2_params(*args, **options)
self.debug(
@@ -425,6 +426,10 @@ class Command(HasParam):
break
params.update(default)
params = self.normalize(**params)
+if not forwarded:
+# split_csv() needs to run exactly once, wherever the command
+# originated (validate() expects the strings to be split already).
+params = self.split_csv(**params)
params = self.convert(**params)
self.debug(
'%s(%s)', self.name, ', '.join(self._repr_iter(**params))
@@ -557,6 +562,26 @@ class Command(HasParam):
if name in params:
yield(name, params[name])
+def split_csv(self, **kw):
+
+Return a dictionary of values where values are decoded from CSV.
+
+For example:
+
+ class my_command(Command):
+... takes_options = (
+... Param('flags', multivalue=True, csv=True),
+... )
+...
+ c = my_command()
+ c.finalize()
+ c.split_csv(flags=u'public,replicated')
+{'flags': (u'public', u'replicated')}
+
+return dict(
+(k, self.params[k].split_csv(v)) for (k, v) in kw.iteritems()
+)
+
def normalize(self, **kw):
Return a dictionary of normalized values.
@@ -715,6 +740,7 @@ class Command(HasParam):
Forward call over XML-RPC to this same command on server.
+kw['_forwarded_call'] = True
return self.Backend.xmlclient.forward(self.name, *args, **kw)
def _on_finalize(self):
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 755d04dd9446a6622bfe99e899158a1ab04d1790..cac3441506bdea65cc1fcf9c85164f56d1618808 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -345,11 +345,16 @@ class
Re: [Freeipa-devel] [PATCH] 0015 Only split CSV strings once
On 23.2.2012 15:29, 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.
Can't you just re-escape the values before forwarding the call? That
would be a fairly straightforward fix and it would remove the need for
all the _forwarded_call hackery.
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.
I think there was a discussion going on about removing the quoting
sometime in the past.
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.
+1, but I'm not sure if that's acceptable (see
http://www.redhat.com/archives/freeipa-devel/2012-January/msg00197.html)
Honza
--
Jan Cholasta
___
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 Thu, 2012-02-23 at 16:08 +0100, Jan Cholasta wrote:
On 23.2.2012 15:29, Petr Viktorin wrote:
...
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.
+1, but I'm not sure if that's acceptable (see
http://www.redhat.com/archives/freeipa-devel/2012-January/msg00197.html)
I tend to like this solution as well, it is true that CSV parsing is
really a client thing. Our life as a server would be much easier if we
just accept scalar values in an array, either from XMLRPC or JSON
interface. It is doable, but it would break IPAv2.x clients which then
would not be able to use CSV formatted values at all. And we want to
avoid that.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
