On 03/21/2012 10:28 AM, Petr Viktorin wrote: > 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): >>>> r"a,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 (Simplify CSV escaping syntax). I'm not saying my >>> solution there is perfect, but the flavor of Python's CSV parsing we're >>> using now is quite quirky. It would be nice to get the syntax right, or >>> at least better, while we're fixing this from a totally-broken state. >>> Later there'll be backwards compatibility problems. >>> But, don't let that hold back this patch. >>> >> >> I'm still not completely convinced. This patch looks ok and I'm sold on >> client side only parsing but this doesn't actually fix any of the CSV >> bugs we have open. >> >> I'd like to evaluate this in more context. >> >> I think the simplest fix is to drop escapechar processing in the >> csvreader and have users quote strings with commas instead, this is how >> I originally intended it to work. The UI won't have to do anything >> special since it will already be split and it isn't a lot to ask to put >> quotes around a value on the CLI. >> >> This is my proposed fix on top of your patch: >> >> diff --git a/ipalib/parameters.py b/ipalib/parameters.py >> index 80b175d..ec6020e 100644 >> --- a/ipalib/parameters.py >> +++ b/ipalib/parameters.py >> @@ -702,7 +702,7 @@ class Param(ReadOnly): >> # csv.py doesn't do Unicode; encode temporarily as UTF-8: >> csv_reader = csv.reader(self.__utf_8_encoder(unicode_csv_data), >> dialect=dialect, >> - delimiter=self.csv_separator, escapechar='\\', >> + delimiter=self.csv_separator, quotechar='"', >> skipinitialspace=self.csv_skipspace, >> **kwargs) >> for row in csv_reader: >> >> With this change: >> - all the self-tests pass >> - https://fedorahosted.org/freeipa/ticket/2417 is fixed >> - https://fedorahosted.org/freeipa/ticket/2227 is fixed >> >> I tested with: >> >> $ ipa sudocmdgroup-add-member test --sudocmd='/bin/chown -R >> apache\:developers /var/www/*/shared/log' >> >> $ ipa role-add-privilege --privileges='"u,r stuff"' test >> >> rob > > Yes, that works well. ACK on your change; here it is squashed in. > > https://fedorahosted.org/freeipa/ticket/2402 can wait for 3.0, then?
Is it risky to take it in? > > > _______________________________________________ > Freeipa-devel mailing list > [email protected] > https://www.redhat.com/mailman/listinfo/freeipa-devel -- Thank you, Dmitri Pal Sr. Engineering Manager IPA project, Red Hat Inc. ------------------------------- Looking to carve out IT costs? www.redhat.com/carveoutcosts/
_______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
