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

[Ade Lee]

I've attached a patch that addresses most of the comments below and also
also adds CLI and fixes adding/removing/editing and changing the state
of profiles.

As we discussed on #irc, the authentication problem is still unresolved.
That is, I want to be able to do the following:

GET /profiles and then in the method itself determine whether or not I
have already logged in (I will have a principal) and return different
results accordingly.

Right now, that is not working.  The only way that I can guarantee that
client auth takes place and the credential is provided is by putting in
a security constraint that requires /profiles/* to use client
authentication.  But then, I cannot do GET /profiles without
authentication.  It seems clientAuth=want is not working.

If we cannot resolve this issue, then I will need to keep using agent/*
and admin/*, which are url patterns that have security contexts.  And I
will have to add back GET /agent/profiles and GET /agent/profiles/{id}
to see all profiles that are not visible to the EE user.

Thanks,
Ade



On Thu, 2013-06-27 at 16:40 -0500, Endi Sukma Dewata wrote:
> 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()) {
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-vakwetu-0136-1-Add-interfaces-for-managing-profiles.patch
Type: text/x-patch
Size: 112888 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20130719/17336b99/attachment.bin>