[Freeipa-devel] [PATCH] 0015 Only split CSV strings once

[Rob Crittenden]

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