Re: [Pki-devel] CLI for editing profiles

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. > > Filed https://fedorahosted.org/pki/ticket/1335 > >>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. > > Filed https://fedorahosted.org/pki/ticket/1336 > >>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