On 6/27/2013 11:04 AM, Ade Lee wrote:
This adds the initial framework for viewing and managing profiles. At this point, only the viewing of profiles is tested.
Because I make some changes to some of the objects used in Cert enrollment, some of the current tests involving enrollment may fail if they use pre-generated XML.
Still, this patch is getting quite large and its time to get some eyes on it.
The next patches will add the CLI code to view profiles and then to add/edit profiles. Following that, will be patches to clean up all the TODOs - like adding the relevant exceptions and auditing.
Ade
Some comments: 1. The log messages in CATest should say 'Enabled:' and 'Visible:' because those are the attribute names being logged: log("Enabled: " + info.isEnabled()); log("Visible: " + info.isVisible() + "\n\n"); 2. The following code in CATest can be simplified: ListIterator<ProfileAttribute> iter = entry.getValue().getAttrs().listIterator(); while (iter.hasNext()) { ProfileAttribute attr = iter.next(); into this: for (ProfileAttribute attr : entry.getValue().getAttrs()) { Similar code exists in CertEnrollmentRequest, BasicProfile, CertProcessor, and EnrollmentProcessor. 3. I don't think the ProfileAttribute should be changed to use IDescriptor instead of Descriptor. Are there other classes implementing IDescriptor? The type adapter for IDescriptor is defined as Descriptor.Adapter, but this adapter will do a simple typecast from IDescriptor to Descriptor and vice versa, so it probably won't work with other classes. To properly marshal any IDescriptor into Descriptor we'd have to create a new Descriptor object and initialize it with values obtained using using IDescriptor methods. I'm also not sure how unmarshal would be able to restore the original object if it's not a Descriptor. I think if there's no other types of descriptor (or if all descriptors will inherit from Descriptor) then we should just use Descriptor in ProfileAttribute. But if we need to support different descriptor types, we need to serialization the class name as well (see PKIException.Data class) and use a factory to recreate the proper object. 4. In ProfileData the inputs, outputs, and policySets attributes are declared between the method declarations. To improve readability we should keep the attributes in the beginning of the class before any method declarations. Similar issues also happens in ProfileInput and ProfileOutput. 5. The ProfileData.policySets maps a String to a Vector. I think unless there's a special need for Vector (e.g. synchronized access) we should use a more generic and light-weight Collection instead. 6. The following code in BasicProfile deleteAllProfileInputs() and deleteAllProfileInputs() can be simplified: Iterator<String> iter = mInputIds.iterator(); while (iter.hasNext()) { String id = iter.next(); into this: for (String id : mInputIds) { 7. In ProfileService the listProfiles() is split into listEEProfiles() and listAdminProfiles() to return different results based on the visibility flag. Splitting the method doesn't prevent a user from calling a method that he's not supposed to call. To prevent that we'd have to configure ACLs, which adds maintenance. Also we probably have to write different CLI to call different methods for different roles. Instead, we could use the original method but it should check the roles of the user and determine the visibility flag based on that. 8. In ProfileService.createProfileDataInfo() the URI is constructed using String concatenation in 2 different ways: if (visibleOnly) { profileBuilder.path(profilePath.value() + "/profiles/" + profileId); } else { profileBuilder.path(profilePath.value() + "/agent/profiles/" + profileId); } ret.setProfileURL(profileBuilder.build().toString()); If we implement #7 it can be simplified like this: URI uri = profileBuilder.path(ProfileResource.class).path("{id}"). build(profileId); ret.setProfileURL(uri.toString()); 9. Some exceptions in ProfileService are being swallowed. I think at least for now we should throw an exception (either make the method throws generic Exception or wrap the exceptions in RuntimeException) so that we know if there's a problem. Later on we can revisit this and handle the exceptions properly. 10. The code in ProfileService.populateProfilePolicies() can be simplified as follows: for (Map.Entry<String,Vector<ProfilePolicy>> policySet : data.getPolicySets().entrySet()) { -- Endi S. Dewata