[Freeipa-devel] CLI parameter: TextFile, BinaryFile and mutually exclusive group

[Christian Heimes]

On 2015-07-30 14:37, Jan Cholasta wrote:
> Hi,
> 
> Dne 30.7.2015 v 14:07 Christian Heimes napsal(a):
>> Hello,
>>
>> While I was working on the ticket
>> https://fedorahosted.org/freeipa/ticket/5155, I noticed a couple of
>> additional places that may raise an IOError. Instead of a File()
>> paramaeter, the vault plugin uses Str() paramater in combination with
>> open() to read files.
>>
>> For passwords I can mostly replace the Str() parameter with File().
>> There is only one minor issue. The File() class has no encoding flag.
>> ipalib.cli.cli.load_files() uses the encoding of sys.stdin to
>> determinate the encoding. In some cases the encoding of sys.stdin can be
>> ASCII. For that reason I like to add an encoding parameter to File().
>>
>> For public and private key file I can't use File(). File() is a subclass
>> of Str(), which requires unicode text. The vault code treats public and
>> private key data as bytes. I assume it wants to support DER encoded key
>> data, too. I like to introduce a new BinaryFile() parameter, which
>> subclasses Bytes(). It might make sense to alias File as TextFile and
>> deprecate the File name.
>>
>> Finally the vault plugin has several mutually exclusive paramater, e.g.
>> passsword and password-file. The plugin has seven distinct checks for
>> mutual exclusion. IMHO this should be better handled by the parameter
>> parsing code. Python's argparse module has a similar feature:
>> https://docs.python.org/2/library/argparse.html#mutual-exclusion
>>
>> I like to handle the case with a mutually_exclusive flag such as:
>>
>>      Str(
>>          'password?',
>>          cli_name='password',
>>          doc=_('Vault password'),
>>     mutually_exclusive='password',
>>      ),
>>      File(
>>          'password_file?',
>>          cli_name='password_file',
>>          doc=_('File containing the vault password'),
>>     mutually_exclusive='password',
>>      ),
>>
>> If more than one parameter with the same mutually_exclusive group name
>> is given, then a MutuallyExclusiveError is raised.
> 
> NACK, instead of having duplicate definitions for a single logical
> parameter and dealing with their inherent mutual exclusiveness on the
> framework level, this should be handled exclusively by the CLI by
> generating multiple command line options for different dispositions of
> the logical parameter. If anything, File should be completely removed,
> not further extended, as it is inherently broken and never worked properly.
> 
> I have an almost working patch which implements this, but I don't think
> it's 4.2.1 material, so I would suggest doing a simple fix for #5155 for
> now.

I wasn't aware that you have a mostly working patch. In that case I'll
come up with a simple fix. I can take care of a redesign when your patch
has landed in the future.

Thanks for the feedback!
Christian


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150730/2985dbdc/attachment.sig>