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,"""d""""e"""' --option='a\\\,b,c\\d'

Client gets (r'"a\,b",c,"""d""""e"""', r'a\\\,b,c\\d')
Cli splits to ('a,b' 'c', '"d""e"' r'a\,b', r'c\d'), validates, forwards
Srv splits to ('a', 'b', 'c', 'd"e', 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

Reply via email to