Re: [Pki-devel] CLI for editing profiles
On 11/20/2014 1:30 PM, Fraser Tweedale wrote:
> Fourth cut of the feature.
>
> Please review w.r.t. the issues below.
>
> The big issue mentioned in the review but *not* dealt with in this
> patchset is support for using file-based profiles OR LDAP-based
> profiles. This will involve copying the new ProfileSystem (as it
> appears in the patches) to a new class ("LDAPProfileSubsystem" I
> suppose), restoring the original ProfileSubsystem with the
> file-based behaviour, providing config and mechanism for loading the
> preferred variant of the subsystem, and reconciling any conflicts.
> But the implementation of the LDAP-based version of ProfileSubsystem
> won't be changed once these patches are ACKed.
>
> (See also patch 0015-2 in the other thread)
Patches #4-4, #5-4, #7-4, #8-4 are ACKed.
Patch #6-4 is ACKed assuming the file-based profiles will be restored later.
Patch #9-4 has some issues:
1. Since the profileId and classId are virtual properties and have their
own LDAP attributes, they should not be stored in certProfileConfig
attribute when a profile is added/modified.
2. Try exporting a profile, change the ID, then add it back to the
server. If you use the XML format, the new profile will not have an
enable and enableBy properties. If you use the RAW format, the new
profile will have enable=false and retain the old enableBy. To be more
consistent these properties should also be removed during add/modify.
Probably the ProfileSubsystem.disableProfile() can be changed to remove
PROP_ENABLE (and PROP_ENABLE_BY too) instead of setting it to "false".
Similarly, in CMSEngine.setSubsystemEnabled() you could also remove
PROP_ENABLED instead of setting it to "false". That way the CS.cfg will
be cleaner and more consistent.
3. In ProfileEditCLI a missing "enable" property is incorrectly
interpreted as enabled.
String enabled = orig.getProperty("enable");
if (enabled == null || !enabled.equalsIgnoreCase("false")) {
System.err.println("Error: Cannot edit profile. Profile
must be disabled.");
System.exit(-1);
}
As a comparison, in ProfileSubsystem.isProfileEnable() a missing
"enable" is considered disabled.
if (enable == null || enable.equals("false"))
return false;
else
return true;
See also the ticket I opened below.
4. If an error happens in ProfileEditCLI, the temporary file may not be
removed. To make sure it's removed we should use a finally block, or use
File.deleteOnExit(). See
https://docs.oracle.com/javase/7/docs/api/java/io/File.html#deleteOnExit().
5. The Properties object used in ProfileClient doesn't order the
properties since it's based on Hashtable so it will be hard to compare
different outputs. Maybe we should use a Map<String, String> in the
method signature, and use a TreeMap object to store the properties.
6. The ProfileAddCLI swallows the exception so if there's an error (e.g.
wrong file format) it will only print a message, it won't display the
stack trace in verbose mode. It should let the exception be handled by
the main program.
I think #1, #3, and #4 should be fixed. The others can be addressed
later. I also opened some tickets for existing issues found during testing:
https://fedorahosted.org/pki/ticket/1218
https://fedorahosted.org/pki/ticket/1220
--
Endi S. Dewata