[Pki-devel] CLI for editing profiles

[Endi Sukma Dewata]

On 3/30/2015 2:26 AM, Fraser Tweedale wrote:
> New patchset for ldap profiles after a long time working on sub-CAs!
>
> 4, 5, 6, 7, 8 have been ACKed previously.  #6 deserves renewed
> attention as the original (file-based) ProfileSubsystem has been
> restored, and the new subsystem now appears as LDAPProfileSubsystem.

ACK for patch #6, just minor things:

1. The CMS_PROFILE_DELETE_DATABASEERROR probably can be renamed to a 
more generic CMS_PROFILE_DELETE_ERROR because based on the message 
itself (i.e. "Failed to delete profile") the database is irrelevant.

2. The CMS_PROFILE_DELETE_UNKNOWNPROFILE probably can be replaced with 
CMS_PROFILE_DELETE_ERROR as well since the message is actually used to 
describe unknown (i.e. generic) server problem, not unknown profile.

3. Please chain the exceptions, for example:

   } catch (EBaseException e) {
       throw new EProfileException("CMS_PROFILE_DELETE_ERROR", e);
   }

> Comments on patch 9 inline below.
>
>> 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.
>>
> Done.  Because the profile configStore is written and read directly
> by the ProfileService, the solution to achieve this is also in that
> class.

I think the classId provided during createProfileRaw() and 
modifyProfileRaw() should be stored in both mProfileClassIds Hashtable 
and in classId LDAP attribute.

>> 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".
>>
> Deferred.  Will file ticket.
>
>> 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.
>>
> Done.  Also fixed the ticket (#1220) while I was at it.
>
>> 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().
>>
> Done.
>
>> 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.
>>
> Deferred.  Will file ticket.
>
>> 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.
>>
> Deferred.  Will address before merge.

Please check this issue in LDAPProfileSubsystem.createProfile() as well.

Once #1 is fixed, it's ACKed. I think it's safe to push these patches to 
the master branch (10.3) since it doesn't break the current code.

I also created this ticket to enhance the profile CLI in the future:
https://fedorahosted.org/pki/ticket/1331

-- 
Endi S. Dewata