On 11/04/2014 07:27 AM, Endi Sukma Dewata wrote:
On 10/28/2014 5:34 PM, Endi Sukma Dewata wrote:
The NSSConnection class has to be modified not to shutdown existing
database because some of the vault clients (e.g. vault-archive and
vault-retrieve) also use a database to encrypt/decrypt the secret.


The problem is described in more detail in this ticket:
https://fedorahosted.org/freeipa/ticket/4638

The changes to the NSSConnection in the first patch caused the
installation to fail. Attached is a new patch that uses the solution
proposed by jdennis.

New patch attached. It's now using the correct OID's for the schema. It
also has been rebased on top of #352-1 and #354.

New patch attached to fix the ticket URL. It depends on #352-2 and
#354-1.

New patch attached to fix vault/container ID problems and for some
cleanups.

The new schema can go to 60basev3.ldif, no need for a new file.

ipalib/plugins/user.py: Make the try: block as small as possible; maybe something like this:

     try:
         vaultcontainer_id = ...
     except errors.NotFound:
         pass
     else:
        (vaultcontainer_name, vaultcontainer_parent_id) = ...
        self.api.Command.vaultcontainer_del(...)

ipapython/dn.py: This change is not needed. If you have a sequence of RNDs you can do `DN(*seq)`.


ipalib/plugins/vault.py:
Do not use star imports in new code; remove the line
     from ipalib.plugins.baseldap import *
and use e.g. `baseldap.LDAPObject` instead of just `LDAPObject`.
Hopefully the only star-imported used are the base classes?

To make translators' jobs easier, split the help text in __doc__ into strings that can be translated individually.
Replace every blank line by:
""") + _("""

The pattern and pattern_errmsg for the 'cn' param don't match. Which one is right? Shouldn't a similar check be there for parent?

vaultcontainer.get_dn: Why put '/' at the end of container_id, if the empty string is ignored later anyway?

Nitpick: In vaultcontainer.get_dn, prefer the if statement over the if-else expressions; it's a bit longer but more readable.

In vaultcontainer.get_id, what's the purpose of the len() check? Did you want dn.endswith() instead?

I sugggest writing doctests for the id manipulation methods (get_id, get_private_id, ...) – it's not always obvious what exactly they do. According to the design doc, container IDs shouldn't end in '/'. If you did that I think the manipulation would be simpler, and it could be shared with the vault equivalents.

vaultcontainer_add: You should use ldap backend directly. Calling Commands is costly, most of the call is spent doing validation of what here is already validated. You should add a function to recursively create vault container using just the ldap client, and call that here and in vault_add.

You can delete a container with children; is that expected?

vault_add should complain if it does not get exactly one of data/text/in.

What's the difference between vault_add and vault_archive? I don't see vault_archive in the design.

It seems '/' is equivalent to '-' as far as KRA is concerned; should we disallow '-' in container/vault names?

You can specify an absolute id by starting it with a slash, but only in --parent and not in the name itself. I think this should be possible in the name too.

You can't include slashes in the name, so you always need to specify the prefix with --parent. I don't think there's a technical reason for this limitation.

There are no tests.

--
Petr³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to