[Pki-devel] CLI for editing profiles

[Endi Sukma Dewata]

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