Thanks for the review. I have some questions below. I'll post a new
patch after the issues are addressed.
On 11/4/2014 11:36 AM, Petr Viktorin wrote:
The new schema can go to 60basev3.ldif, no need for a new file.
Fixed. Also removed nsContainer as suggested by Simo.
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(...)
Actually the command that generates the NotFound exception is the last
command. The first two commands are preparing the parameters. I moved
them before the try-block.
ipapython/dn.py: This change is not needed. If you have a sequence of
RNDs you can do `DN(*seq)`.
This is actually needed to construct a parent DN from an existing DN:
parent_dn = DN(dn[1:])
Note that the parameter is an array of RDNs, not a sequence of RDNs. I
don't see an existing mechanism to get a parent DN from a DN. Without
this change it would generate an error:
ValueError: tuple or list must be 2-valued, not
"[ipapython.dn.RDN('cn=admin'), ipapython.dn.RDN('cn=users'),
ipapython.dn.RDN('cn=vaults'), ipapython.dn.RDN('dc=idm'),
ipapython.dn.RDN('dc=lab'), ipapython.dn.RDN('dc=bos'),
ipapython.dn.RDN('dc=redhat'), ipapython.dn.RDN('dc=com')]"
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?
I found 20 star imports in the existing plugins :/
All existing plugins are actually using LDAPObject directly, not
baseldap.LDAPObject. Is there a concern doing that? I fixed the code to
do the same thing.
To make translators' jobs easier, split the help text in __doc__ into
strings that can be translated individually.
Replace every blank line by:
""") + _("""
Fixed.
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?
This is actually copied verbatim from user's uid. Could you give some
sample values that show the problem?
I think ideally there should be a similar check for parent, but I
haven't figured out the proper pattern yet. I think we can do this as a
later enhancement. Or maybe we should remove the validation for cn for now.
vaultcontainer.get_dn: Why put '/' at the end of container_id, if the
empty string is ignored later anyway?
I'm still considering some options for container ID:
a) use a slash at the end for all containers (e.g. /users/)
b) don't use a trailing slash, except for root container (/)
I think option (a) is more consistent, container ID always ends with
slash, although in cases like this it can be redundant. This is how the
code is currently written. We can make an exception for
vaultcontainer.get_dn() though.
Option (b) will require more careful coding in many places (e.g.
concatenations) because it needs to handle a special case for root
container.
Maybe a better way is to use an array of names internally and the slash
is only used during input/output. The problem is when calling another
command the array has to be converted into string and parsed back into
array, so it may even introduce more redundancies. I don't see a way to
pass an object parameter to a command. Any suggestion?
Nitpick: In vaultcontainer.get_dn, prefer the if statement over the
if-else expressions; it's a bit longer but more readable.
Fixed.
In vaultcontainer.get_id, what's the purpose of the len() check? Did you
want dn.endswith() instead?
Yes, but my assumption is the DN will always be within the namespace
because it's only used internally, and using len() will be faster than
endswith(). The error case will never happen, but I was just making sure
there won't be an infinite loop. Maybe instead of calling the method
recursively I should just use a loop? That will avoid repetitive
validations.
I sugggest writing doctests for the id manipulation methods (get_id,
get_private_id, ...) – it's not always obvious what exactly they do.
Do you mean sample code in docs? Is it still necessary if we have some
test code?
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.
Yeah, as mentioned above I'm still considering several options. The
design may need to be adjusted.
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.
Hmm.. I'm not sure if we should write a code that will duplicate
everything done by the LDAPCreate class. Shouldn't server-side
command-to-command invocation be relatively faster?
The recursion is mainly used to create the user's private container.
Note that the private container is not created by default. We only
provide /users top-level container. When a user creates a private vault
the command will automatically create the /users/<username> private
container on the user's behalf. This will only happen once for each
user. In most other cases the parent would already exist.
Another option is to require the user or the admin to create the private
container manually, but that would be less user-friendly.
Another option is to check the parent container using direct LDAP
search, but still do the add using vaultcontainer_add. What do you think?
You can delete a container with children; is that expected?
This seems to be LDAPDelete's built-in functionality. Is there an option
to disallow non-empty subtree removal?
For vault purposes, when a user is removed from IPA the server will
remove the user's entire private container, so the feature would
actually be useful here. But probably in general we should warn the user
if they are deleting a non-empty container. If there's an option in
LDAPDelete we can let the user specify that option if necessary.
vault_add should complain if it does not get exactly one of data/text/in.
Actually, no. The primary way to archive data is via vault_archive. The
vault_add is for creating a vault, but optionally you can specify the
initial data to be archived. If nothing is specified it will archive an
empty string. Note that for symmetric vault the encrypted empty string
can be used to enforce that the same password will be used on on
subsequent archival operations.
I'm trying to change vault_archive to raise an error if it's missing the
input data and change vault_add to archive empty string if nothing is
specified, but it doesn't work since the framework seems to be
converting the empty string into None, so vault_archive can't tell if
it's archiving an empty string or missing the input data. Any suggestion?
What's the difference between vault_add and vault_archive? I don't see
vault_archive in the design.
Yes, that's due to a recent 'standard vault' addition. I have not added
vault_archive yet to the design. The vault_archive is used to archive a
generic blob of data. The vaultsecret_archive will later be used to add
secrets into a JSON structure which will then be archived using the
vault_archive.
It seems '/' is equivalent to '-' as far as KRA is concerned; should we
disallow '-' in container/vault names?
I think so, but as I mentioned earlier I haven't look very far on
attribute validations.
The slashes in vault ID is converted into dashes in KRA's client key ID.
KRA actually accepts slashes, but the slashes will not look pretty in
REST URL because they will be URL-encoded (in case it's needed for
troubleshooting KRA via browser) so I changed them to dashes.
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.
Right, but I'm not sure how it will work with the IPA framework. The
first command parameter is the cn (vault name). If we specify a vault ID
(with slashes) there it will be entered into cn. Should we parse the ID
and insert the vault name and parent ID back into keys and options
variables? Which value will eventually be stored into cn in LDAP?
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.
See the above explanation. What does primary_key actually mean in
hierarchical structure like vault? Since cn is only unique locally, is
cn a primary key? Can a virtual attribute be a primary key?
There are no tests.
Right, I'll take a look at how to write tests. Is there a way to run
tests without running the full build?
--
Endi S. Dewata
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
