[Pki-devel] [PATCH]136 - initial framework for restful interfaces for managing profiles

[Endi Sukma Dewata]

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