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
Description: OpenPGP digital signature
-- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code