Hi Endi,
Dne 24.2.2015 v 04:09 Endi Sukma Dewata napsal(a):
On 2/16/2015 2:50 AM, Endi Sukma Dewata wrote:
Hi,
Attached are the updated patches for the password vault, and some new
ones (please disregard previous patch submissions). Please give them a
try. Thanks.
New patches attached replacing all previous vault patches. They include
the new escrow functionality, changes to the asymmetric vault, and some
cleanups. Thanks.
Patch 353:
1) Please follow PEP8 in new code.
The pep8 tool reports these errors in existing files:
./ipalib/constants.py:98:80: E501 line too long (84 > 79 characters)
./ipalib/plugins/baseldap.py:1527:80: E501 line too long (81 > 79
characters)
./ipalib/plugins/user.py:915:80: E501 line too long (80 > 79 characters)
as well as many errors in the files this patch adds.
2) Pylint reports the following error:
ipatests/test_xmlrpc/test_vault_plugin.py:153:
[E0102(function-redefined), test_vault] class already defined line 27)
3) The container_vault config option should be renamed to
container_vaultcontainer, as it is used in the vaultcontainer plugin,
not the vault plugin.
4) The vault object should be child of the vaultcontainer object.
Not only is this correct from the object model perspective, but it would
also make all the container_id hacks go away.
5) When specifying param flags, use set literals.
This is especially wrong, because it's not a tuple, but a string in
parentheses:
+ flags=('virtual_attribute'),
6) The `container` param of vault should actually be an option in
vault_* commands.
Also it should be renamed to `container_id`, for consistency with
vaultcontainer.
7) The `vault_id` param of vault should have "no_option" in flags, since
it is output-only.
8) Don't translate docstrings where not necessary:
+ def get_dn(self, *keys, **options):
+ __doc__ = _("""
+ Generates vault DN from vault ID.
+ """)
Only plugin modules and classes should have translated docstrings.
9) This looks wrong in vault.get_dn() and vaultcontainer.get_dn():
+ name = None
+ if keys:
+ name = keys[0]
Primary key of the object should always be set, so the if statement
should not be there.
Also, primary key of any given object is always last in "keys", so use
keys[-1] instead of keys[0].
10) Use "self.api" instead of "api" to access the API in plugins.
11) No clever optimizations like this please:
+ # vault DN cannot be the container base DN
+ if len(dn) == len(api.Object.vaultcontainer.base_dn):
+ raise ValueError('Invalid vault DN: %s' % dn)
Compare the DNs by value instead.
12) vault.split_id() is not used anywhere.
13) Bytes is not base64-encoded data:
+ Bytes('data?',
+ cli_name='data',
+ doc=_('Base-64 encoded binary data to archive'),
+ ),
It is base64-encoded in the CLI, but on the API level it is not. The doc
should say just "Binary data to archive".
14) Use File instead of Str for input files:
+ Str('in?',
+ cli_name='in',
+ doc=_('File containing data to archive'),
+ ),
15) Use MutuallyExclusiveError instead of ValidationError when there are
mutually exclusive options specified.
16) You do way too much stuff in vault_add.forward(). Only code that
must be done on the client needs to be there, i.e. handling of the
"data", "text" and "in" options.
The vault_archive call must be in vault_add.execute(), otherwise a) we
will be making 2 RPC calls from the client and b) it won't be called at
all when api.env.in_server is True.
17) Why are vaultcontainer objects automatically created in vault_add?
If you have to automatically create them, you also have to automatically
delete them when the command fails. But that's a hassle, so I would just
not create them automatically.
18) Why are vaultcontainer objects automatically created in vault_find?
This is just plain wrong and has to be removed, now.
19) What is the reason behind all the json stuff in vault_transport_cert?
vault_transport_cert.__json__() is exactly the same as
Command.__json__() and hence redundant.
20) Are vault_transport_cert, vault_archive and vault_retrieve supposed
to be runnable by users? If not, add "NO_CLI = True" to the class
definition.
21) vault_archive is not a retrieve operation, it should be based on
LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does
not do anything with LDAP. The same applies to vault_retrieve.
22) vault_archive will break with binary data that is not UTF-8 encoded
text.
This is where it occurs:
+ vault_data[u'data'] = unicode(data)
Generally, don't use unicode() on str values and str() on unicode values
directly, always use .decode() and .encode().
23) Since vault containers are nested, the vaultcontainer object should
be child of itself.
There is no support for nested objects in the framework, but it
shouldn't be too hard to do anyway.
24) Instead of:
+ while len(dn) > len(self.base_dn):
+
+ rdn = dn[0]
...
+ dn = DN(*dn[1:])
you can do:
+ for rdn in dn[:-len(self.base_dn)]:
...
25) Why are parent vaultcontainer objects automatically created in
vaultcontainer_add?
26) Instead of the delete_entry refactoring in baseldap and
vaultcontainer_add, you can put this in vaultcontainer_add's pre_callback:
try:
ldap.get_entries(dn, scope=ldap.SCOPE_ONELEVEL, attrs_list=[])
except errors.NotFound:
pass
else:
if not options.get('force', False):
raise errors.NotAllowedOnNonLeaf()
27) Why are parent vaultcontainer objects automatically created in
vaultcontainer_find?
28) The vault and vaultcontainer plugins seem to be pretty similar, I
think it would make sense to put common stuff in a base class and
inherit vault and vaultcontainer from that.
More later.
Honza
--
Jan Cholasta
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
