On Mon, Apr 06, 2015 at 08:26:57PM -0500, Endi Sukma Dewata wrote:
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:
Thanks; comments inline.
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); }
1, 2 and 3 have been addressed (trivial, so I'll spare you yet another patch to review).
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.
The classId gets stored in mProfileClassIds, and in the LDAP attribute, by the createProfile() method.
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.
Have un-swallowed these exceptions.
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
Cheers! Fraser